diff --git a/CHANGELOG.md b/CHANGELOG.md index a5f191a..d49377e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,20 @@ always called out under their own heading. 415. An archive mints kind `file`, is never inline-previewable (the download path's inline allow-list is `application/pdf` only), and is still subject to the existing `MAX_UPLOAD_BYTES` per-file ceiling. +- `GET /api/artifacts/:id/versions/:version` — one version including its + content, reusing `getArtifactVersion`, the same read authorization as + `GET /api/artifacts/:id`, and the same response shape. A malformed or + sub-1 version is `400`; an unknown version collapses into the same `404 + Artifact not found` every other single-artifact failure mode does. +- `GET /api/artifacts/:id/download?version=N` — pins the download to that + version's content for the data-URL and downloadable-text conventions, + where content really is per-version; omitting `version` is unchanged. Same + `400`/`404` rules as the new versions route, plus one more: a blob-backed + upload's `ContentStore` reference lives on the artifact row's own + `source`, never per-version, so `?version=N` for one is `400 "Uploaded + file content is not versioned"` unless `N` names the current version — + rather than silently answering with today's blob under an older version's + name. - `artifact_version.metadata` (jsonb, nullable) and `artifact_version.parent_version_ids` (text[], nullable), added by the new `0004_version_metadata` migration. `metadata` is opaque to the package — diff --git a/README.md b/README.md index 52b0dbb..d5fe075 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,7 @@ happen in major versions. | `POST /api/artifacts/upload` | multipart import. An optional `generatedBy` form field is stored as `source.generatedBy`, a free-form display label nothing here reads back | | `GET /api/artifacts/:id` | Deep link (archived artifacts still load) | | `GET`/`POST /api/artifacts/:id/versions` | Version history (paginated, no content bodies) and revision | +| `GET /api/artifacts/:id/versions/:version` | One version, including content | | `POST /api/artifacts/:id/(un)archive` | Idempotent soft-hide | | `GET /api/artifacts/:id/download` | One path over three storage conventions | | `GET /api/artifacts/counts` | Per-segment counts over the tenant. See [Counts](#counts) | @@ -207,7 +208,18 @@ content server-side; only the response projection changes. **Version history pagination:** `GET /api/artifacts/:id/versions` returns `{ versions, nextCursor }` with the same default/max limit clamps as list. Cursor is the last version number returned (newest-first). Version rows omit content; use -`GET /api/artifacts/:id?version=N` (or `getArtifactVersion`) for a pinned body. +`GET /api/artifacts/:id/versions/:version` (or `getArtifactVersion`) for a pinned body. +That route reuses the detail route's read authorization and response shape — a +non-integer or sub-1 `:version` is `400`, and an unknown version collapses into the +same `404 Artifact not found` every other single-artifact failure mode does. +`GET /api/artifacts/:id/download?version=N` applies the same pin to a downloadable +artifact's bytes, for the conventions where content really is per-version (an inline +data: URL, or downloadable text like `csv-export`); omitting `version` downloads the +current one. A blob-backed upload has no per-version bytes — the `ContentStore` +reference lives in the artifact row's own `source`, never in `artifact_version` — so +`?version=N` for one is `400 {"error":"Uploaded file content is not versioned"}` +unless `N` names the artifact's current version, rather than silently answering with +today's blob under an older version's name. **Version metadata and lineage:** each version optionally carries `metadata` (any JSON object, opaque to the package — stored and returned as-is) and `parentVersionIds` (an diff --git a/examples/reference-host/test/acceptance.test.ts b/examples/reference-host/test/acceptance.test.ts index b38bf32..8c6b6a9 100644 --- a/examples/reference-host/test/acceptance.test.ts +++ b/examples/reference-host/test/acceptance.test.ts @@ -77,6 +77,27 @@ describe("import a URL, read it back, revise it, read the history", () => { expect(history.versions.map((v) => v.version)).toEqual([2, 1]); expect(history.versions[1]!.title).toBe("Launch plan"); }); + + test("GET /versions/:version serves version 1's own content, unaffected by the revision", async () => { + const v1 = await json<{ artifact: { version: number; title: string; content: string } }>( + await host.request(`/api/artifacts/${artifactId}/versions/1`), + ); + expect(v1.artifact).toMatchObject({ + version: 1, + title: "Launch plan", + content: "https://example.com/plan", + }); + + const v2 = await json<{ artifact: { version: number; content: string } }>( + await host.request(`/api/artifacts/${artifactId}/versions/2`), + ); + expect(v2.artifact).toMatchObject({ version: 2, content: "https://example.com/plan-2" }); + }); + + test("GET /versions/:version is 404 for an unknown version and 400 for a malformed one", async () => { + expect((await host.request(`/api/artifacts/${artifactId}/versions/99`)).status).toBe(404); + expect((await host.request(`/api/artifacts/${artifactId}/versions/0`)).status).toBe(400); + }); }); // The ContentStore is a port, and the proof is that the SAME scenarios pass @@ -155,6 +176,77 @@ describe.each<[string, ContentStore]>([ }); }); +// Only DataUrlContentStore keeps its bytes IN `content`, so it is the store +// where ?version=N really changes what downloads. InlineContentStore's blob +// lives out-of-band, referenced from the artifact row's own `source` (never +// versioned) — see the next describe block for how that case is refused +// rather than silently serving the current blob under an older version's name. +describe("download an older version's content over HTTP (DataUrlContentStore)", () => { + let app: { request: (path: string, init?: RequestInit) => Promise }; + let id: string; + const ORIGINAL = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 1, 1, 1]); + const REVISED = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 2, 2, 2]); + + beforeAll(async () => { + app = host.buildApp(DataUrlContentStore); + const form = new FormData(); + form.append("files", new File([ORIGINAL], "chart.png", { type: "image/png" })); + const uploaded = await json<{ artifacts: { id: string }[] }>( + await app.request("/api/artifacts/upload", { method: "POST", body: form }), + ); + id = uploaded.artifacts[0]!.id; + await app.request( + `/api/artifacts/${id}/versions`, + postJson({ content: `data:image/png;base64,${Buffer.from(REVISED).toString("base64")}` }), + ); + }); + + test("?version=1 downloads the original bytes; omitted downloads the current version", async () => { + const original = await app.request(`/api/artifacts/${id}/download?version=1`); + expect(new Uint8Array(await original.arrayBuffer())).toEqual(ORIGINAL); + + const current = await app.request(`/api/artifacts/${id}/download`); + expect(new Uint8Array(await current.arrayBuffer())).toEqual(REVISED); + }); + + test("an unknown ?version is 404 and a non-integer one is 400", async () => { + expect((await app.request(`/api/artifacts/${id}/download?version=99`)).status).toBe(404); + expect((await app.request(`/api/artifacts/${id}/download?version=abc`)).status).toBe(400); + }); +}); + +describe("?version on a blob-backed upload (InlineContentStore) is refused unless current", () => { + let app: { request: (path: string, init?: RequestInit) => Promise }; + let id: string; + let currentVersion: number; + const BYTES = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 3, 3, 3]); + + beforeAll(async () => { + app = host.buildApp(InlineContentStore); + const form = new FormData(); + form.append("files", new File([BYTES], "chart.png", { type: "image/png" })); + const uploaded = await json<{ artifacts: { id: string; version: number }[] }>( + await app.request("/api/artifacts/upload", { method: "POST", body: form }), + ); + ({ id, version: currentVersion } = uploaded.artifacts[0]!); + // Bumps the artifact's version without touching the out-of-band blob — + // `source` (and so the ContentStore reference) is never revised. + await app.request(`/api/artifacts/${id}/versions`, postJson({ title: "Renamed" })); + }); + + test("a non-current ?version is 400, not a silent lie about which bytes came back", async () => { + const res = await app.request(`/api/artifacts/${id}/download?version=${currentVersion}`); + expect(res.status).toBe(400); + expect(await res.json()).toEqual({ error: "Uploaded file content is not versioned" }); + }); + + test("?version naming the CURRENT version still downloads the blob", async () => { + const res = await app.request(`/api/artifacts/${id}/download?version=${currentVersion + 1}`); + expect(res.status).toBe(200); + expect(new Uint8Array(await res.arrayBuffer())).toEqual(BYTES); + }); +}); + describe("an unsupported upload is refused, leaving nothing behind", () => { test("an SVG is rejected 415 and no orphan artifact is created", async () => { const before = await json<{ artifacts: unknown[] }>( @@ -388,6 +480,7 @@ describe("a cross-tenant request fails closed", () => { [`/api/artifacts/${id}`, {}], [`/api/artifacts/${id}/versions`, {}], [`/api/artifacts/${id}/versions`, postJson({ content: "x" })], + [`/api/artifacts/${id}/versions/1`, {}], [`/api/artifacts/${id}/archive`, { method: "POST" }], [`/api/artifacts/${id}/unarchive`, { method: "POST" }], [`/api/artifacts/${id}/download`, {}], @@ -548,6 +641,7 @@ describe("a skill-draft is invisible over the mounted host", () => { [`/api/artifacts/${id}`, {}], [`/api/artifacts/${id}/versions`, {}], [`/api/artifacts/${id}/versions`, postJson({ content: "x" })], + [`/api/artifacts/${id}/versions/1`, {}], [`/api/artifacts/${id}/archive`, { method: "POST" }], [`/api/artifacts/${id}/unarchive`, { method: "POST" }], [`/api/artifacts/${id}/download`, {}], diff --git a/src/mount.test.ts b/src/mount.test.ts index 8c1a30c..f3a6f89 100644 --- a/src/mount.test.ts +++ b/src/mount.test.ts @@ -439,6 +439,7 @@ describe("every way of not getting an artifact is indistinguishable", () => { [`/artifacts/${id}`, {}], [`/artifacts/${id}/versions`, {}], [`/artifacts/${id}/versions`, json({ content: "x" })], + [`/artifacts/${id}/versions/1`, {}], [`/artifacts/${id}/archive`, { method: "POST" }], [`/artifacts/${id}/unarchive`, { method: "POST" }], [`/artifacts/${id}/download`, {}], @@ -680,6 +681,7 @@ describe("versions", () => { [`/artifacts/${id}`, GET], [`/artifacts/${id}/versions`, GET], [`/artifacts/${id}/versions`, json({ content: "x" })], + [`/artifacts/${id}/versions/1`, GET], [`/artifacts/${id}/archive`, POST], [`/artifacts/${id}/unarchive`, POST], [`/artifacts/${id}/download`, GET], @@ -724,6 +726,57 @@ describe("versions", () => { }); }); +describe("GET /artifacts/:id/versions/:version", () => { + test("returns a prior version's content, reusing getArtifactVersion", async () => { + const db = await testDb(); + const app = host(db); + const row = await seedArtifact(db, { title: "Draft", content: "v1" }); + await app.request(`/artifacts/${row.id}/versions`, json({ title: "Final", content: "v2" })); + + const res = await app.request(`/artifacts/${row.id}/versions/1`); + expect(res.status).toBe(200); + const body = (await res.json()) as { artifact: { version: number; title: string; content: string } }; + expect(body.artifact).toMatchObject({ version: 1, title: "Draft", content: "v1" }); + + const current = await app.request(`/artifacts/${row.id}/versions/2`); + expect(await current.json()).toMatchObject({ + artifact: { version: 2, title: "Final", content: "v2" }, + }); + }); + + test("an unknown version is 404", async () => { + const db = await testDb(); + const row = await seedArtifact(db); + const res = await host(db).request(`/artifacts/${row.id}/versions/99`); + expect(res.status).toBe(404); + expect(await res.json()).toEqual({ error: "Artifact not found" }); + }); + + test("a non-integer or sub-1 version is 400", async () => { + const db = await testDb(); + const app = host(db); + const row = await seedArtifact(db); + for (const version of ["0", "-1", "1.5", "abc"]) { + const res = await app.request(`/artifacts/${row.id}/versions/${version}`); + expect(res.status).toBe(400); + } + }); + + test("is 404 before authorization for another tenant's artifact", async () => { + const db = await testDb(); + const foreign = await seedArtifact(db, { tenantId: "other" }); + const res = await host(db).request(`/artifacts/${foreign.id}/versions/1`); + expect(res.status).toBe(404); + }); + + test("is 403 for an unauthenticated caller", async () => { + const db = await testDb(); + const row = await seedArtifact(db); + const res = await host(db, { principal: null }).request(`/artifacts/${row.id}/versions/1`); + expect(res.status).toBe(403); + }); +}); + describe("archive authorization", () => { const post = (app: Hono, id: string, verb: string) => app.request(`/artifacts/${id}/${verb}`, { method: "POST" }); @@ -1125,6 +1178,74 @@ describe("download over HTTP", () => { const row = await seedArtifact(db, { kind: "document" }); expect((await host(db).request(`/artifacts/${row.id}/download`)).status).toBe(400); }); + + test("?version=N downloads that version's content; omitted downloads current", async () => { + const db = await testDb(); + const app = host(db); + const row = await seedArtifact(db, { kind: "csv-export", title: "Keywords", content: "a,b" }); + // The revise route trims content (TrimmedNonEmpty), same as create. + await app.request(`/artifacts/${row.id}/versions`, json({ content: "c,d" })); + + const v1 = await app.request(`/artifacts/${row.id}/download?version=1`); + expect(await v1.text()).toBe("a,b"); + + const current = await app.request(`/artifacts/${row.id}/download`); + expect(await current.text()).toBe("c,d"); + }); + + test("download with an unknown ?version is 404", async () => { + const db = await testDb(); + const row = await seedArtifact(db, { kind: "csv-export" }); + const res = await host(db).request(`/artifacts/${row.id}/download?version=99`); + expect(res.status).toBe(404); + }); + + test("download with a non-integer ?version is 400", async () => { + const db = await testDb(); + const row = await seedArtifact(db, { kind: "csv-export" }); + const res = await host(db).request(`/artifacts/${row.id}/download?version=0`); + expect(res.status).toBe(400); + }); + + // A blob's bytes live in the ContentStore, referenced from the artifact + // row's own `source` — never per-version. Silently serving the current + // blob for an older `?version=N` would misrepresent it as that version's + // content, so a non-current version is refused instead of lying. + test("?version naming a non-current version of a blob-backed upload is 400", async () => { + const db = await testDb(); + const app = host(db); + const png = new File([new Uint8Array([1, 2, 3])], "chart.png", { type: "image/png" }); + const data = new FormData(); + data.append("files", png); + const created = (await ( + await app.request("/artifacts/upload", { method: "POST", body: data }) + ).json()) as { artifacts: { id: string }[] }; + const id = created.artifacts[0]!.id; + await app.request(`/artifacts/${id}/versions`, json({ title: "Renamed" })); + + const stale = await app.request(`/artifacts/${id}/download?version=1`); + expect(stale.status).toBe(400); + expect(await stale.json()).toEqual({ + error: "Uploaded file content is not versioned", + }); + }); + + test("?version naming the CURRENT version of a blob-backed upload still serves it", async () => { + const db = await testDb(); + const app = host(db); + const bytes = new Uint8Array([1, 2, 3]); + const png = new File([bytes], "chart.png", { type: "image/png" }); + const data = new FormData(); + data.append("files", png); + const created = (await ( + await app.request("/artifacts/upload", { method: "POST", body: data }) + ).json()) as { artifacts: { id: string; version: number }[] }; + const { id, version } = created.artifacts[0]!; + + const res = await app.request(`/artifacts/${id}/download?version=${version}`); + expect(res.status).toBe(200); + expect(new Uint8Array(await res.arrayBuffer())).toEqual(bytes); + }); }); describe("mail attachment references", () => { diff --git a/src/mount.ts b/src/mount.ts index 531a9c5..b15173b 100644 --- a/src/mount.ts +++ b/src/mount.ts @@ -11,6 +11,7 @@ import { createArtifact, enrich, getArtifact, + getArtifactVersion, listArtifacts, ListArtifactsQuery, listArtifactVersions, @@ -28,6 +29,7 @@ import { type SerializedArtifactListItem, } from "./artifacts.js"; import { resolveDownload } from "./download.js"; +import { uploadRefFromSource } from "./content-store.js"; import { ArtifactCountsIncompleteError, countArtifactsBySegments, @@ -174,6 +176,16 @@ const idParam = { schema: { type: "string" }, } as const; +// A path/query version reference: a positive integer, or a 400 for anything +// else (non-numeric, fractional, zero, negative). +const VersionRef = type("string").pipe((raw, ctx) => { + const n = Number(raw); + if (!Number.isInteger(n) || n < 1) { + return ctx.error("a positive integer version"); + } + return n; +}); + /** * Mount the artifact routes onto a host Hono app. * @@ -668,6 +680,52 @@ export function mountArtifacts( }, ); + app.get( + "/artifacts/:id/versions/:version", + describeRoute({ + tags: ["Artifacts"], + summary: "Read one version of an artifact, including its content", + description: + "Same read authorization and response shape as GET /artifacts/:id, pinned to a specific version via getArtifactVersion.", + parameters: [ + idParam, + { name: "version", in: "path", required: true, schema: { type: "integer" } }, + ], + responses: { + 200: { description: "The version, including content" }, + 400: { description: "version is not a positive integer" }, + 403: { description: "No resolvable principal" }, + 404: { + description: + "Artifact not found — also the answer for a malformed id, a skill-draft, another tenant's artifact, or an unknown version", + }, + }, + }), + async (c) => { + const loaded = await loadScoped(c); + if ("response" in loaded) return loaded.response; + + const version = VersionRef(c.req.param("version")!); + if (version instanceof type.errors) { + return c.json({ error: version.summary }, 400); + } + + const versionRow = await getArtifactVersion(db, loaded.row.id, version); + if (!versionRow) return c.json({ error: "Artifact not found" }, 404); + + const [artifactJson] = await serialize(loaded.scope, [ + { + ...loaded.row, + title: versionRow.title, + content: versionRow.content, + version: versionRow.version, + metadata: versionRow.metadata, + }, + ]); + return c.json({ artifact: artifactJson }); + }, + ); + app.post( "/artifacts/:id/versions", describeRoute({ @@ -784,26 +842,61 @@ export function mountArtifacts( tags: ["Artifacts"], summary: "Download an artifact's content", description: - "One path over three storage conventions, in precedence order: out-of-band ContentStore blob, inline data: URL (file/image kinds), then downloadable text (csv-export). Served as an attachment except a PDF with ?inline=1; X-Content-Type-Options: nosniff always.", + "One path over three storage conventions, in precedence order: out-of-band ContentStore blob, inline data: URL (file/image kinds), then downloadable text (csv-export). Served as an attachment except a PDF with ?inline=1; X-Content-Type-Options: nosniff always. An optional ?version=N pins the download to that version's content (getArtifactVersion) for the data-URL and downloadable-text conventions, where content really is per-version. A blob-backed upload has no per-version bytes — source.upload.id lives on the artifact row only — so ?version=N for one is 400 unless it names the artifact's current version.", parameters: [ idParam, { name: "inline", in: "query", required: false, schema: { type: "string" } }, + { name: "version", in: "query", required: false, schema: { type: "integer" } }, ], responses: { 200: { description: "The file body" }, - 400: { description: "Artifact kind is not downloadable" }, + 400: { + description: + "Artifact kind is not downloadable, version is not a positive integer, or version names a non-current version of a blob-backed upload", + }, 403: { description: "No resolvable principal" }, - 404: { description: "Artifact not found" }, + 404: { description: "Artifact not found — also the answer for an unknown version" }, }, }), async (c) => { const loaded = await loadScoped(c); if ("response" in loaded) return loaded.response; + let row = loaded.row; + const versionParam = c.req.query("version"); + if (versionParam !== undefined) { + const version = VersionRef(versionParam); + if (version instanceof type.errors) { + return c.json({ error: version.summary }, 400); + } + const versionRow = await getArtifactVersion(db, loaded.row.id, version); + if (!versionRow) return c.json({ error: "Artifact not found" }, 404); + // A blob's bytes live in the ContentStore, referenced from the + // artifact row's own `source` — never from `artifact_version`, so + // there is no per-version blob to serve. Silently falling through to + // today's blob would answer version N's request with the current + // bytes, misrepresenting them as pinned. + if ( + version !== loaded.row.version && + uploadRefFromSource(loaded.row.source)?.id !== undefined + ) { + return c.json( + { error: "Uploaded file content is not versioned" }, + 400, + ); + } + row = { + ...loaded.row, + title: versionRow.title, + content: versionRow.content, + version: versionRow.version, + }; + } + const result = await resolveDownload( db, contentStore, - loaded.row, + row, c.req.query("inline") === "1", ); if ("status" in result) return c.json({ error: result.error }, result.status);