diff --git a/e2e/migrations.test.ts b/e2e/migrations.test.ts index e8f95ed..aff8b8f 100644 --- a/e2e/migrations.test.ts +++ b/e2e/migrations.test.ts @@ -80,6 +80,71 @@ describe("runArtifactMigrations", () => { ]); }); + const invariants = async () => { + const checks = await testDb.db.execute<{ conname: string }>(sql` + SELECT conname FROM pg_constraint + WHERE conname IN ('artifact_version_gte_1', 'artifact_version_version_gte_1', 'upload_size_gte_0') + ORDER BY conname + `); + const [tenant] = await testDb.db.execute<{ is_nullable: string }>(sql` + SELECT is_nullable FROM information_schema.columns + WHERE table_schema = 'artifacts' AND table_name = 'artifact' AND column_name = 'tenant_id' + `); + return { + checks: checks.map((row) => row.conname), + tenantNullable: tenant?.is_nullable, + }; + }; + + test("restores 0.1.0's CHECK constraints and tenant_id NOT NULL", async () => { + await testDb.db.execute(sql` + ALTER TABLE "artifacts"."artifact" + DROP CONSTRAINT "artifact_version_gte_1", + ALTER COLUMN "tenant_id" DROP NOT NULL + `); + await testDb.db.execute(sql` + ALTER TABLE "artifacts"."artifact_version" DROP CONSTRAINT "artifact_version_version_gte_1" + `); + await testDb.db.execute(sql` + ALTER TABLE "artifacts"."upload" DROP CONSTRAINT "upload_size_gte_0" + `); + await runArtifactMigrations(testDb.config, { schema: "public" }); + expect(await invariants()).toEqual({ + checks: [ + "artifact_version_gte_1", + "artifact_version_version_gte_1", + "upload_size_gte_0", + ], + tenantNullable: "NO", + }); + }); + + test("leaves tenant_id nullable and warns while null tenants remain", async () => { + await testDb.db.execute(sql` + ALTER TABLE "artifacts"."artifact" ALTER COLUMN "tenant_id" DROP NOT NULL + `); + await testDb.db.execute(sql` + INSERT INTO "artifacts"."artifact" ("tenant_id", "kind", "title", "content") + VALUES (NULL, 'document', 'orphan', 'body') + `); + const warnings: unknown[] = []; + const warn = console.warn; + console.warn = (...args: unknown[]) => warnings.push(args[0]); + try { + await runArtifactMigrations(testDb.config, { schema: "public" }); + } finally { + console.warn = warn; + } + expect((await invariants()).tenantNullable).toBe("YES"); + expect(String(warnings[0])).toContain("null tenant_id"); + + await testDb.db.execute( + sql`DELETE FROM "artifacts"."artifact" WHERE "tenant_id" IS NULL`, + ); + await runArtifactMigrations(testDb.config, { schema: "public" }); + expect((await invariants()).tenantNullable).toBe("NO"); + }); + test("a host boots with createArtifactDb after migrating, and close releases it", async () => { const { db, close } = createArtifactDb(connectionString(testDb.config)); try { diff --git a/e2e/mount.test.ts b/e2e/mount.test.ts index f4c1f1f..2392542 100644 --- a/e2e/mount.test.ts +++ b/e2e/mount.test.ts @@ -430,6 +430,60 @@ describe("GET /artifacts", () => { ); }); + test("rejects dates outside years 1 to 9999 and versions past int4 with 400", async () => { + const db = await testDb(); + const app = host(db); + const row = await seedArtifact(db); + for (const path of [ + "/artifacts?createdAfter=10000-01-01", + "/artifacts?createdBefore=-000001-01-01", + `/artifacts/${row.id}/versions/2147483648`, + `/artifacts/${row.id}/versions?cursor=2147483648`, + `/artifacts/${row.id}/download?version=2147483648`, + ]) { + expect((await app.request(path)).status).toBe(400); + } + expect( + ( + await app.request( + `/artifacts/${row.id}/versions`, + json({ content: "v2", expectedVersion: 2147483648 }), + ) + ).status, + ).toBe(400); + }); + + test("a multipart body with a bad or missing boundary is 400", async () => { + const db = await testDb(); + const app = host(db); + const form = new FormData(); + form.append("file", new File(["hello"], "a.txt", { type: "text/plain" })); + const uploaded = await app.request("/artifacts/upload", { + method: "POST", + body: form, + }); + expect(uploaded.status).toBe(201); + const { artifacts } = (await uploaded.json()) as { + artifacts: { id: string }[]; + }; + for (const path of [ + "/artifacts/upload", + `/artifacts/${artifacts[0]!.id}/versions`, + ]) { + for (const contentType of [ + "multipart/form-data", + "multipart/form-data; boundary=nope", + ]) { + const res = await app.request(path, { + method: "POST", + headers: { "content-type": contentType }, + body: "not multipart", + }); + expect(res.status).toBe(400); + } + } + }); + test("a caller-supplied tenant cannot widen the resolved scope", async () => { const db = await testDb(); await seedArtifact(db, { title: "Theirs", tenantId: "other" }); diff --git a/e2e/tools.test.ts b/e2e/tools.test.ts index c6cbf0d..997473d 100644 --- a/e2e/tools.test.ts +++ b/e2e/tools.test.ts @@ -115,12 +115,12 @@ describe("artifact_read", () => { }); }); - test("a missing version is an error naming the version", async () => { + test("a missing version is not found", async () => { const db = await testDb(); const row = await seedArtifact(db); await expect( readArtifact(db, { scope: SCOPE, artifactId: row.id, version: 7 }), - ).rejects.toThrow(/Version 7 not found/); + ).rejects.toBeInstanceOf(ArtifactNotFoundError); }); test("an artifact in another tenant is not found", async () => { diff --git a/e2e/workflow-mount.test.ts b/e2e/workflow-mount.test.ts index 4edf9c7..d9fa887 100644 --- a/e2e/workflow-mount.test.ts +++ b/e2e/workflow-mount.test.ts @@ -422,3 +422,42 @@ describe("PATCH /artifacts/:id", () => { expect(res.status).toBe(400); }); }); + +describe("user-input errors", () => { + test("an oversized binary filename or link-file title is 400", async () => { + const db = await testDb(); + const app = host(db); + const long = "x".repeat(600); + const binary = await app.request( + "/artifacts/binary", + json({ + filename: `${long}.html`, + mimeType: "text/html", + contentBase64: Buffer.from("
hi
").toString("base64"), + }), + ); + expect(binary.status).toBe(400); + const linked = await app.request( + "/artifacts/link-file", + json({ title: long, kind: "document", path: "out/report.md" }), + ); + expect(linked.status).toBe(400); + }); + + test("read of a missing version is 404; a version past int4 is 400", async () => { + const db = await testDb(); + const app = host(db); + const row = await seedArtifact(db, { tenantId: "acme" }); + const read = (query: string) => + app.request(`/artifacts/${row.id}/read${query}`, { headers: authed }); + expect((await read("?version=7")).status).toBe(404); + expect((await read("?version=2147483648")).status).toBe(400); + expect( + ( + await app.request(`/artifacts/${row.id}/chunk?version=2147483648`, { + headers: authed, + }) + ).status, + ).toBe(400); + }); +}); diff --git a/migrations/0005_schema_invariants.sql b/migrations/0005_schema_invariants.sql new file mode 100644 index 0000000..5c19f70 --- /dev/null +++ b/migrations/0005_schema_invariants.sql @@ -0,0 +1,29 @@ +DO $guard$ +BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conrelid = '"artifacts"."artifact"'::regclass AND conname = 'artifact_version_gte_1') THEN + ALTER TABLE "artifacts"."artifact" ADD CONSTRAINT "artifact_version_gte_1" CHECK ("version" >= 1); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conrelid = '"artifacts"."artifact_version"'::regclass AND conname = 'artifact_version_version_gte_1') THEN + ALTER TABLE "artifacts"."artifact_version" ADD CONSTRAINT "artifact_version_version_gte_1" CHECK ("version" >= 1); + END IF; + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conrelid = '"artifacts"."upload"'::regclass AND conname = 'upload_size_gte_0') THEN + ALTER TABLE "artifacts"."upload" ADD CONSTRAINT "upload_size_gte_0" CHECK ("size" >= 0); + END IF; +END +$guard$; +--> statement-breakpoint +DO $guard$ +BEGIN + IF EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_schema = 'artifacts' AND table_name = 'artifact' + AND column_name = 'tenant_id' AND is_nullable = 'YES' + ) THEN + IF EXISTS (SELECT 1 FROM "artifacts"."artifact" WHERE "tenant_id" IS NULL) THEN + RAISE WARNING 'artifacts.artifact has rows with a null tenant_id; left tenant_id nullable. Assign a tenant or delete those rows, then re-run migrations.'; + ELSE + ALTER TABLE "artifacts"."artifact" ALTER COLUMN "tenant_id" SET NOT NULL; + END IF; + END IF; +END +$guard$; diff --git a/src/artifacts.ts b/src/artifacts.ts index f7d5f04..a08bb85 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -300,6 +300,9 @@ export async function createArtifact( return row; } +/** Postgres `int4` max: the largest version a column can hold. */ +export const MAX_VERSION = 2_147_483_647; + export class ArtifactNotFoundError extends Error { constructor(artifactId: string) { super(`Artifact not found: ${artifactId}`); @@ -673,7 +676,13 @@ export type ListArtifactsFilters = { const dateBound = (endOfDay: boolean) => type("string").pipe((raw, ctx) => { const parsed = new Date(raw); - if (Number.isNaN(parsed.getTime())) return ctx.error("a valid date"); + if ( + Number.isNaN(parsed.getTime()) || + parsed.getUTCFullYear() < 1 || + parsed.getUTCFullYear() > 9999 + ) { + return ctx.error("a valid date between years 1 and 9999"); + } if (endOfDay && DATE_ONLY.test(raw)) parsed.setUTCHours(23, 59, 59, 999); return parsed; }); @@ -730,7 +739,7 @@ export const ListArtifactsQuery = type({ export const ListArtifactVersionsQuery = type({ "cursor?": type("string").pipe((raw, ctx) => { const n = Number(raw); - if (!Number.isInteger(n) || n < 1) { + if (!Number.isInteger(n) || n < 1 || n > MAX_VERSION) { return ctx.error("a positive integer version cursor"); } return n; diff --git a/src/migrations.ts b/src/migrations.ts index de4069c..95f667c 100644 --- a/src/migrations.ts +++ b/src/migrations.ts @@ -55,7 +55,11 @@ export async function runArtifactMigrations( password: config.password, database: config.database, max: 1, - onnotice: () => undefined, + onnotice: (notice) => { + if (notice.severity === "WARNING") { + console.warn(`@corbits/artifacts migration: ${notice.message}`); + } + }, }; if (config.ssl !== undefined) clientOptions.ssl = config.ssl; const client = postgres(clientOptions); diff --git a/src/mount.ts b/src/mount.ts index 1b46430..0da35fa 100644 --- a/src/mount.ts +++ b/src/mount.ts @@ -16,6 +16,7 @@ import { ListArtifactsQuery, listArtifactVersions, ListArtifactVersionsQuery, + MAX_VERSION, MAX_ARTIFACT_CONTENT_BYTES, MetadataShape, serializeArtifact, @@ -158,7 +159,8 @@ const GeneratedByField = type("unknown") // string). Omitted entirely preserves today's unconditional-write behavior. const ExpectedVersion = type("number").narrow( (n, ctx) => - (Number.isInteger(n) && n >= 1) || ctx.mustBe("a positive integer"), + (Number.isInteger(n) && n >= 1 && n <= MAX_VERSION) || + ctx.mustBe("a positive integer"), ); const ReviseArtifactRequest = type({ @@ -189,12 +191,21 @@ const idParam = { // else (non-numeric, fractional, zero, negative). const VersionRef = type("string").pipe((raw, ctx) => { const n = Number(raw); - if (!Number.isInteger(n) || n < 1) { + if (!Number.isInteger(n) || n < 1 || n > MAX_VERSION) { return ctx.error("a positive integer version"); } return n; }); +// A malformed multipart body (bad or missing boundary) is the caller's error. +async function parseMultipart(c: Context) { + try { + return await c.req.parseBody({ all: true }); + } catch { + return null; + } +} + // The artifact as it stood at one version, for the version detail and download routes. function rowAtVersion( row: ArtifactRow, @@ -549,7 +560,10 @@ export function createArtifactRoutes({ // principalRequired already ran; a null scope here would mean it didn't. if (!scope) return c.json({ error: "Forbidden" }, 403); - const parsed = await c.req.parseBody({ all: true }); + const parsed = await parseMultipart(c); + if (parsed === null) { + return c.json({ error: "Expected a multipart/form-data body" }, 400); + } const files: File[] = []; for (const value of Object.values(parsed)) { for (const entry of Array.isArray(value) ? value : [value]) { @@ -896,7 +910,10 @@ export function createArtifactRoutes({ 400, ); } - const parsed = await c.req.parseBody({ all: true }); + const parsed = await parseMultipart(c); + if (parsed === null) { + return c.json({ error: "Expected a multipart/form-data body" }, 400); + } const file = parsed["file"]; if (!(file instanceof File)) { return c.json({ error: "Expected one file field named file" }, 400); diff --git a/src/tools.ts b/src/tools.ts index f19ba81..b7086a7 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -135,9 +135,7 @@ async function resolveForRead( const pinned = await getArtifactVersion(db, args.artifactId, args.version); if (!pinned) { - throw new Error( - `Version ${args.version} not found for artifact ${args.artifactId}`, - ); + throw new ArtifactNotFoundError(args.artifactId); } return { base: { diff --git a/src/workflow-mount.ts b/src/workflow-mount.ts index 972ce2d..d6a7e9f 100644 --- a/src/workflow-mount.ts +++ b/src/workflow-mount.ts @@ -25,6 +25,7 @@ import { findArtifactByTitle, getArtifact, listArtifacts, + MAX_VERSION, MetadataShape, serializeArtifact, serializeArtifactListItem, @@ -178,8 +179,16 @@ function parseNumberQuery