fix: map remaining user-input errors to 4xx and restore 0.1.0 invariants

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 {

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" });

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 () => {

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("<p>hi</p>").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);
+  });
+});

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$;

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;

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);

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);

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: {