diff --git a/CHANGELOG.md b/CHANGELOG.md index 440ebecd..9fc4e3ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ loosely while pre-1.0 (breaking changes can land on minor bumps). ## [Unreleased] ### Fixed +- **Nested artifact routes honor `:cid`.** `GET`/`DELETE` + `/v1/conversations/:id/artifacts/:aid` (and `/raw`) 404 when the + conversation is missing or TUI-origin, or when the artifact belongs + to a different conversation. Matches the documented tenant+conversation + pair and the list/create prefix. Tenant-wide `/v1/artifacts/:aid` + is unchanged. - **Sandbox FIFO no longer deadlocks a tenant.** `/write` and `/read` of a named pipe used to block forever in `open()`. `/write` holds the per-tenant sandbox mutex across that open, so a workspace `mkfifo` diff --git a/docs/api/artifacts/conversations-delete.md b/docs/api/artifacts/conversations-delete.md index 604725ef..02fa26a5 100644 --- a/docs/api/artifacts/conversations-delete.md +++ b/docs/api/artifacts/conversations-delete.md @@ -30,7 +30,8 @@ curl -X DELETE \ | Status | When | Body | |--------|------|------| | 401 | Missing / invalid bearer. | `{"error": "..."}` | -| 404 | Id doesn't exist for this tenant + conversation pair. | `{"error": "artifact not found"}` | +| 404 | Conversation is missing or TUI-origin. | `{"error": "conversation not found"}` | +| 404 | Artifact missing or belongs to another conversation. | `{"error": "artifact not found"}` | ## See also diff --git a/docs/api/artifacts/conversations-get.md b/docs/api/artifacts/conversations-get.md index eab8d098..7367e4b4 100644 --- a/docs/api/artifacts/conversations-get.md +++ b/docs/api/artifacts/conversations-get.md @@ -43,7 +43,8 @@ Field schemas: [Data model → ArtifactRecord](../../concepts/data-model.md#arti | Status | When | Body | |--------|------|------| | 401 | Missing / invalid bearer. | `{"error": "..."}` | -| 404 | Id doesn't exist for this tenant + conversation pair. | `{"error": "artifact not found"}` | +| 404 | Conversation is missing or TUI-origin. | `{"error": "conversation not found"}` | +| 404 | Artifact missing or belongs to another conversation. | `{"error": "artifact not found"}` | ## See also diff --git a/docs/api/artifacts/conversations-raw.md b/docs/api/artifacts/conversations-raw.md index f8350d37..680ef2da 100644 --- a/docs/api/artifacts/conversations-raw.md +++ b/docs/api/artifacts/conversations-raw.md @@ -43,7 +43,8 @@ When `If-None-Match` matches. Empty body, includes the same `ETag`. | Status | When | Body | |--------|------|------| | 401 | Missing / invalid bearer. | `{"error": "..."}` | -| 404 | Id doesn't exist for this tenant + conversation pair, or the row's content is missing (data corruption — defensive). | `{"error": "artifact not found"}` or `{"error": "artifact content missing"}` | +| 404 | Conversation is missing or TUI-origin. | `{"error": "conversation not found"}` | +| 404 | Artifact missing, belongs to another conversation, or the row's content is missing (data corruption — defensive). | `{"error": "artifact not found"}` or `{"error": "artifact content missing"}` | ## See also diff --git a/include/tenant_store.h b/include/tenant_store.h index 6303c4a8..d4e1c065 100644 --- a/include/tenant_store.h +++ b/include/tenant_store.h @@ -1039,14 +1039,20 @@ class TenantStore { const std::string& mime_type); // Metadata-only fetch — does NOT load the BLOB. Use this for list - // pages, the JSON metadata endpoint, agent /list, etc. + // pages, the JSON metadata endpoint, agent /list, etc. When + // `conversation_id > 0`, the row must also belong to that + // conversation (nested /v1/conversations/:cid/artifacts/:aid). + // `conversation_id == 0` is tenant-wide by id. std::optional - get_artifact_meta(int64_t tenant_id, int64_t id) const; + get_artifact_meta(int64_t tenant_id, int64_t id, + int64_t conversation_id = 0) const; // BLOB fetch — separate so list paths don't pull megabytes. Returns - // nullopt if the row doesn't exist for this tenant. + // nullopt if the row doesn't exist for this tenant (and, when + // `conversation_id > 0`, for that conversation). std::optional - get_artifact_content(int64_t tenant_id, int64_t id) const; + get_artifact_content(int64_t tenant_id, int64_t id, + int64_t conversation_id = 0) const; // Lookup by (tenant, conversation, path) — used by the agent // /read slash command to address artifacts the way they were @@ -1065,7 +1071,12 @@ class TenantStore { std::vector list_artifacts_tenant(int64_t tenant_id, int limit) const; - bool delete_artifact(int64_t tenant_id, int64_t id); + // Tenant-scoped by default. When `conversation_id > 0`, refuse (and + // do not nullify memory links) unless the row belongs to that + // conversation — so DELETE /v1/conversations/:cid/artifacts/:aid + // cannot remove a sibling thread's blob. + bool delete_artifact(int64_t tenant_id, int64_t id, + int64_t conversation_id = 0); // SUM(size) — used by put_artifact for quota math and by HTTP // callers exposing "you have used X of Y" surfaces. diff --git a/src/api_server.cpp b/src/api_server.cpp index 0760b114..e0d16179 100644 --- a/src/api_server.cpp +++ b/src/api_server.cpp @@ -3466,20 +3466,35 @@ void handle_artifact_list_tenant(int fd, TenantStore& tenants, const Tenant& ten write_json_response(fd, 200, body); } +// conversation_id == 0 is the tenant-wide /v1/artifacts/:aid surface. +// Nested /v1/conversations/:cid/artifacts/:aid passes :cid so a +// sibling-thread (or TUI-origin) id cannot be read or deleted through +// the conversation-scoped path. Missing / TUI conversations 404 like +// list/create on the same prefix. void handle_artifact_get_meta(int fd, int64_t artifact_id, - TenantStore& tenants, const Tenant& tenant) { - auto rec = tenants.get_artifact_meta(tenant.id, artifact_id); + TenantStore& tenants, const Tenant& tenant, + int64_t conversation_id = 0) { + if (conversation_id > 0 && + !get_http_conversation(tenants, tenant.id, conversation_id)) { + return write_artifact_error(fd, 404, "conversation not found"); + } + auto rec = tenants.get_artifact_meta(tenant.id, artifact_id, conversation_id); if (!rec) return write_artifact_error(fd, 404, "artifact not found"); write_json_response(fd, 200, artifact_to_json(*rec)); } // GET /v1/artifacts/:id/raw — content body with proper Content-Type + // ETag (= sha256) for conditional GETs. Tenant-scoped lookup; cross- -// tenant id surfaces as 404. +// tenant id surfaces as 404. Nested callers pass conversation_id. void handle_artifact_get_raw(int fd, int64_t artifact_id, const HttpRequest& req, - TenantStore& tenants, const Tenant& tenant) { - auto rec = tenants.get_artifact_meta(tenant.id, artifact_id); + TenantStore& tenants, const Tenant& tenant, + int64_t conversation_id = 0) { + if (conversation_id > 0 && + !get_http_conversation(tenants, tenant.id, conversation_id)) { + return write_artifact_error(fd, 404, "conversation not found"); + } + auto rec = tenants.get_artifact_meta(tenant.id, artifact_id, conversation_id); if (!rec) return write_artifact_error(fd, 404, "artifact not found"); // ETag honors the strong-validator semantics — sha256 of the bytes. @@ -3497,7 +3512,8 @@ void handle_artifact_get_raw(int fd, int64_t artifact_id, return; } - auto blob = tenants.get_artifact_content(tenant.id, artifact_id); + auto blob = tenants.get_artifact_content(tenant.id, artifact_id, + conversation_id); if (!blob) return write_artifact_error(fd, 404, "artifact content missing"); std::ostringstream ss; @@ -3512,8 +3528,13 @@ void handle_artifact_get_raw(int fd, int64_t artifact_id, } void handle_artifact_delete(int fd, int64_t artifact_id, - TenantStore& tenants, const Tenant& tenant) { - if (!tenants.delete_artifact(tenant.id, artifact_id)) + TenantStore& tenants, const Tenant& tenant, + int64_t conversation_id = 0) { + if (conversation_id > 0 && + !get_http_conversation(tenants, tenant.id, conversation_id)) { + return write_artifact_error(fd, 404, "conversation not found"); + } + if (!tenants.delete_artifact(tenant.id, artifact_id, conversation_id)) return write_artifact_error(fd, 404, "artifact not found"); auto body = jobj(); body->as_object_mut()["deleted"] = jbool(true); @@ -11636,13 +11657,16 @@ void ApiServer::handle_connection(int fd) { "method not allowed\n"); return; } - return handle_artifact_get_raw(fd, aid, req, tenants_, *tenant); + return handle_artifact_get_raw(fd, aid, req, tenants_, + *tenant, id); } if (segs.size() == 5) { if (req.method == "GET") - return handle_artifact_get_meta(fd, aid, tenants_, *tenant); + return handle_artifact_get_meta(fd, aid, tenants_, + *tenant, id); if (req.method == "DELETE") - return handle_artifact_delete(fd, aid, tenants_, *tenant); + return handle_artifact_delete(fd, aid, tenants_, + *tenant, id); write_plain_response(fd, 405, "Method Not Allowed", "method not allowed\n"); return; diff --git a/src/tenant_store.cpp b/src/tenant_store.cpp index 724ea17f..58bfd277 100644 --- a/src/tenant_store.cpp +++ b/src/tenant_store.cpp @@ -3628,23 +3628,33 @@ TenantStore::put_artifact(int64_t tenant_id, int64_t conversation_id, } std::optional -TenantStore::get_artifact_meta(int64_t tenant_id, int64_t id) const { +TenantStore::get_artifact_meta(int64_t tenant_id, int64_t id, + int64_t conversation_id) const { if (!db_) return std::nullopt; - Stmt q(db_, (std::string("SELECT ") + kArtifactMetaCols + - " FROM tenant_artifacts WHERE tenant_id = ? AND id = ?;").c_str()); + std::string sql = std::string("SELECT ") + kArtifactMetaCols + + " FROM tenant_artifacts WHERE tenant_id = ? AND id = ?"; + if (conversation_id > 0) sql += " AND conversation_id = ?"; + sql += ";"; + Stmt q(db_, sql.c_str()); q.bind(1, tenant_id); q.bind(2, id); + if (conversation_id > 0) q.bind(3, conversation_id); if (q.step() != SQLITE_ROW) return std::nullopt; return row_to_artifact(q); } std::optional -TenantStore::get_artifact_content(int64_t tenant_id, int64_t id) const { +TenantStore::get_artifact_content(int64_t tenant_id, int64_t id, + int64_t conversation_id) const { if (!db_) return std::nullopt; - Stmt q(db_, "SELECT content FROM tenant_artifacts " - "WHERE tenant_id = ? AND id = ?;"); + std::string sql = "SELECT content FROM tenant_artifacts " + "WHERE tenant_id = ? AND id = ?"; + if (conversation_id > 0) sql += " AND conversation_id = ?"; + sql += ";"; + Stmt q(db_, sql.c_str()); q.bind(1, tenant_id); q.bind(2, id); + if (conversation_id > 0) q.bind(3, conversation_id); if (q.step() != SQLITE_ROW) return std::nullopt; const void* blob = sqlite3_column_blob(q.raw(), 0); int n = sqlite3_column_bytes(q.raw(), 0); @@ -3700,8 +3710,15 @@ TenantStore::list_artifacts_tenant(int64_t tenant_id, int limit) const { return out; } -bool TenantStore::delete_artifact(int64_t tenant_id, int64_t id) { +bool TenantStore::delete_artifact(int64_t tenant_id, int64_t id, + int64_t conversation_id) { if (!db_) return false; + // Nested HTTP routes pass conversation_id so a mismatched :cid + // cannot nullify memory links or drop a sibling conversation's row. + if (conversation_id > 0 && + !get_artifact_meta(tenant_id, id, conversation_id)) { + return false; + } // Soft cascade: nullify memory_entries.artifact_id referencing this // row before deleting it. The schema-level FK couldn't be added by // ALTER TABLE (SQLite limitation), so we do the SET NULL ourselves. diff --git a/tests/test_artifacts.cpp b/tests/test_artifacts.cpp index 23670b97..73df3632 100644 --- a/tests/test_artifacts.cpp +++ b/tests/test_artifacts.cpp @@ -279,6 +279,54 @@ TEST_CASE("artifacts are tenant-isolated") { CHECK(s.bytes_used_tenant(b) == 6); } +TEST_CASE("id lookup and delete honor optional conversation_id") { + TempDb db; TenantStore s; s.open(db.path.string()); + const int64_t tid = make_tenant(s, "acme"); + const int64_t c1 = make_conversation(s, tid, "thread-1"); + const int64_t c2 = make_conversation(s, tid, "thread-2"); + + auto p1 = s.put_artifact(tid, c1, "a.txt", "from-c1", "text/plain"); + auto p2 = s.put_artifact(tid, c2, "b.txt", "from-c2", "text/plain"); + REQUIRE(p1.record.has_value()); + REQUIRE(p2.record.has_value()); + const int64_t aid1 = p1.record->id; + const int64_t aid2 = p2.record->id; + + // Tenant-wide (conversation_id == 0) still finds either row. + REQUIRE(s.get_artifact_meta(tid, aid1).has_value()); + REQUIRE(s.get_artifact_content(tid, aid2).has_value()); + + // Matching conversation_id returns the row. + auto scoped = s.get_artifact_meta(tid, aid1, c1); + REQUIRE(scoped.has_value()); + CHECK(scoped->path == "a.txt"); + auto blob = s.get_artifact_content(tid, aid1, c1); + REQUIRE(blob.has_value()); + CHECK(*blob == "from-c1"); + + // Sibling :cid is a miss — same 404 the nested HTTP route must emit. + CHECK_FALSE(s.get_artifact_meta(tid, aid1, c2).has_value()); + CHECK_FALSE(s.get_artifact_content(tid, aid1, c2).has_value()); + CHECK_FALSE(s.get_artifact_meta(tid, aid2, c1).has_value()); + + // Mismatched delete is a no-op: row and memory link stay put. + auto entry = s.create_entry(tid, "reference", "Refers", "", "", "[]", aid1); + REQUIRE(entry.artifact_id == aid1); + CHECK_FALSE(s.delete_artifact(tid, aid1, c2)); + REQUIRE(s.get_artifact_meta(tid, aid1, c1).has_value()); + auto still_linked = s.get_entry(tid, entry.id); + REQUIRE(still_linked.has_value()); + CHECK(still_linked->artifact_id == aid1); + + // Matching conversation_id deletes only that row. + REQUIRE(s.delete_artifact(tid, aid1, c1)); + CHECK_FALSE(s.get_artifact_meta(tid, aid1).has_value()); + REQUIRE(s.get_artifact_meta(tid, aid2, c2).has_value()); + auto cleared = s.get_entry(tid, entry.id); + REQUIRE(cleared.has_value()); + CHECK(cleared->artifact_id == 0); +} + TEST_CASE("deleting a conversation cascades to its artifacts") { TempDb db; TenantStore s; s.open(db.path.string()); const int64_t tid = make_tenant(s, "acme");