fix(uploads): underscore-led keys, bounded bucket names, dedup only on live objects

graphile/graphile-presigned-url-plugin/__tests__/custom-key.test.ts

@@ -0,0 +1,61 @@
+/**
+ * Unit tests for custom object-key validation.
+ *
+ * The rule being pinned is containment, not aesthetics: a key may look however a
+ * build tool wants it to as long as it cannot escape the bucket's namespace or
+ * mean something different to S3 than to the gateway serving it. A static export
+ * puts its immutable assets under `_next/static/**`, so a leading underscore must
+ * be accepted while the traversal guards stay in force.
+ */
+
+import { validateCustomKey } from '../src/custom-key';
+
+describe('validateCustomKey', () => {
+  it.each([
+    '_next/static/chunks/main-abc123.js',
+    '_next/static/css/app.css',
+    '_headers',
+    'index.html',
+    'assets/img/logo.svg',
+    'docs/v1.2.3/guide.pdf',
+    'a',
+    'my-file_name.v2.tar.gz',
+  ])('accepts %s', (key) => {
+    expect(validateCustomKey(key)).toBeNull();
+  });
+
+  it('accepts a key at the length limit and rejects one past it', () => {
+    expect(validateCustomKey('a'.repeat(1024))).toBeNull();
+    expect(validateCustomKey('a'.repeat(1025))).toMatch(/INVALID_KEY_LENGTH/);
+  });
+
+  it('rejects an empty key', () => {
+    expect(validateCustomKey('')).toMatch(/INVALID_KEY_LENGTH/);
+  });
+
+  it.each(['../etc/passwd', 'assets/../../secret', '_next/../..'])(
+    'rejects path traversal in %s',
+    (key) => {
+      expect(validateCustomKey(key)).toMatch(/path traversal/);
+    },
+  );
+
+  it('rejects a leading slash', () => {
+    expect(validateCustomKey('/_next/static/main.js')).toMatch(/leading slash/);
+  });
+
+  it('rejects NUL bytes', () => {
+    expect(validateCustomKey('index.html\0.png')).toMatch(/null bytes/);
+  });
+
+  it.each([
+    '-leading-hyphen.js',
+    '.leading-dot.js',
+    'has space.js',
+    'has:colon.js',
+    'has?query=1',
+    'emoji-🚀.png',
+  ])('rejects %s', (key) => {
+    expect(validateCustomKey(key)).toMatch(/^INVALID_KEY:/);
+  });
+});

graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts

@@ -471,6 +471,90 @@ describe('finalizeStagedUpload', () => {
     expect(s3.client.send).toHaveBeenCalledTimes(1);
   });
 
+  // With the confirm-upload lifecycle, a row is a claim on bytes rather than
+  // proof of them, so only a confirmed row may absorb an upload.
+  describe('with the confirm-upload lifecycle', () => {
+    const lifecycleTarget = {
+      ...target,
+      storageConfig: { ...storageConfig(), hasConfirmUpload: true } as StorageModuleConfig,
+    };
+
+    const existingRow = (status: string) => ({
+      id: FILE_ID,
+      key: staged.contentHash,
+      mime_type: 'image/png',
+      size: 16,
+      filename: 'original.png',
+      status,
+    });
+
+    it.each(['uploaded', 'processed'])('deduplicates against a %s row', async (status) => {
+      const { finalizeStagedUpload } = await import('../src/managed-upload');
+      const db = fakeDb([
+        SET_CONFIG,
+        { match: /SELECT id, key, mime_type/, rows: () => [existingRow(status)] },
+      ]);
+
+      const { projection, deduplicated } = await finalizeStagedUpload({
+        target: lifecycleTarget, withPgClient: db.withPgClient, pgSettings: null, staged,
+      });
+
+      expect(deduplicated).toBe(true);
+      expect(projection.id).toBe(FILE_ID);
+      expect(db.queries.some((q) => /INSERT|DELETE/.test(q.text))).toBe(false);
+    });
+
+    it.each(['requested', 'rejected', 'expired'])(
+      'drops the %s row and uploads afresh instead of reporting a dedup hit',
+      async (status) => {
+        const { finalizeStagedUpload } = await import('../src/managed-upload');
+        const NEW_FILE_ID = '55555555-5555-5555-5555-555555555555';
+        const db = fakeDb([
+          SET_CONFIG,
+          { match: /SELECT id, key, mime_type/, rows: () => [existingRow(status)] },
+          { match: /DELETE FROM storage_public\.app_files/, rows: () => [] },
+          { match: /INSERT INTO storage_public\.app_files/, rows: () => [{ id: NEW_FILE_ID }] },
+        ]);
+
+        const { projection, deduplicated } = await finalizeStagedUpload({
+          target: lifecycleTarget, withPgClient: db.withPgClient, pgSettings: null, staged,
+        });
+
+        expect(deduplicated).toBe(false);
+        // The caller is handed the row that actually names the promoted bytes.
+        expect(projection.id).toBe(NEW_FILE_ID);
+        const del = db.queries.find((q) => /DELETE/.test(q.text));
+        expect(del?.values).toEqual([FILE_ID]);
+        // Promote to the content key, then drop the staged object.
+        expect(s3.client.send).toHaveBeenCalledTimes(2);
+      },
+    );
+
+    it('asks for the status column only when the module has one', async () => {
+      const { finalizeStagedUpload } = await import('../src/managed-upload');
+      const withLifecycle = fakeDb([
+        SET_CONFIG,
+        { match: /SELECT id, key, mime_type/, rows: () => [existingRow('uploaded')] },
+      ]);
+      await finalizeStagedUpload({
+        target: lifecycleTarget, withPgClient: withLifecycle.withPgClient, pgSettings: null, staged,
+      });
+      expect(withLifecycle.queries.find((q) => /SELECT id, key/.test(q.text))!.text).toContain('status');
+
+      const without = fakeDb([
+        SET_CONFIG,
+        {
+          match: /SELECT id, key, mime_type/,
+          rows: () => [{ id: FILE_ID, key: staged.contentHash, mime_type: 'image/png', size: 16, filename: null }],
+        },
+      ]);
+      await finalizeStagedUpload({
+        target, withPgClient: without.withPgClient, pgSettings: null, staged,
+      });
+      expect(without.queries.find((q) => /SELECT id, key/.test(q.text))!.text).not.toContain('status');
+    });
+  });
+
   it('abandons both keys when the files row cannot be inserted', async () => {
     const { finalizeStagedUpload } = await import('../src/managed-upload');
     const db = fakeDb([

graphile/graphile-presigned-url-plugin/src/custom-key.ts

@@ -0,0 +1,38 @@
+/**
+ * Validation for a caller-supplied ("custom") object key.
+ *
+ * A custom key is the one place a client names an S3 object directly, so what is
+ * enforced here is containment: the key must land inside the bucket's namespace
+ * and mean the same thing to S3 as it does to the gateway that later serves it.
+ */
+
+const MAX_CUSTOM_KEY_LENGTH = 1024;
+
+/**
+ * The key alphabet. A leading underscore is legal — a static export puts its
+ * hashed assets under `_next/static/**` — and containment is enforced by the
+ * `..`, leading-slash and NUL checks below rather than by the first character.
+ */
+const CUSTOM_KEY_REGEX = /^[a-zA-Z0-9_][a-zA-Z0-9_.\-/]*$/;
+
+/**
+ * Returns an error string describing why `key` is unusable, or null if it is fine.
+ */
+export function validateCustomKey(key: string): string | null {
+  if (key.length === 0 || key.length > MAX_CUSTOM_KEY_LENGTH) {
+    return 'INVALID_KEY_LENGTH: must be 1-1024 characters';
+  }
+  if (key.includes('..')) {
+    return 'INVALID_KEY: path traversal (..) not allowed';
+  }
+  if (key.startsWith('/')) {
+    return 'INVALID_KEY: leading slash not allowed';
+  }
+  if (key.includes('\0')) {
+    return 'INVALID_KEY: null bytes not allowed';
+  }
+  if (!CUSTOM_KEY_REGEX.test(key)) {
+    return 'INVALID_KEY: must start with alphanumeric or underscore and contain only alphanumeric, dots, hyphens, underscores, and slashes';
+  }
+  return null;
+}

graphile/graphile-presigned-url-plugin/src/file-lifecycle.ts

@@ -0,0 +1,29 @@
+import type { StorageModuleConfig } from './types';
+
+/**
+ * The statuses in which a files row stands for bytes a reader can actually GET.
+ * A `requested` row is a claim, not an object — its presigned PUT may never have
+ * run — and `rejected`/`expired` are settled failures.
+ */
+export const LIVE_FILE_STATUSES = ['uploaded', 'processed'];
+
+/**
+ * The `status` column, when the module has one, for splicing into a select list.
+ */
+export function statusSelectFragment(storageConfig: StorageModuleConfig): string {
+  return storageConfig.hasConfirmUpload ? ', status' : '';
+}
+
+/**
+ * Whether an existing row may be handed back as a dedup hit.
+ *
+ * Modules without the confirm-upload lifecycle have no `status` column, so there
+ * is nothing to read and every row is presumed live, as before.
+ */
+export function isLiveFileRow(
+  storageConfig: StorageModuleConfig,
+  row: { status?: string }
+): boolean {
+  if (!storageConfig.hasConfirmUpload) return true;
+  return LIVE_FILE_STATUSES.includes(row.status as string);
+}

graphile/graphile-presigned-url-plugin/src/index.ts

@@ -33,6 +33,7 @@ export {
   type ConfirmUploadInput,
   type ConfirmUploadVerdict,
 } from './confirm-upload';
+export { validateCustomKey } from './custom-key';
 export type { ResolvedBucketCoordinate } from './default-bucket';
 export { resolveDefaultBucket } from './default-bucket';
 export { createDownloadUrlPlugin } from './download-url-field';

graphile/graphile-presigned-url-plugin/src/managed-upload.ts

@@ -21,6 +21,7 @@
 import { Logger } from '@pgpmjs/logger';
 
 import { resolveDefaultBucket } from './default-bucket';
+import { isLiveFileRow, statusSelectFragment } from './file-lifecycle';
 import { type FileRefFieldBinding, getFileRefFieldBinding } from './file-ref-registry';
 import { provisionAndRecordPhysicalBucket, resolveS3ForDatabase } from './physical-bucket';
 import { type WithPgClient, withRequestPgClient } from './request-pg-client';
@@ -289,18 +290,41 @@ export async function finalizeStagedUpload(args: {
 
   const existing = await withRequestPgClient(withPgClient, pgSettings, async (pgClient) => {
     const result = await pgClient.query({
-      text: `SELECT id, key, mime_type, size, filename
+      text: `SELECT id, key, mime_type, size, filename${statusSelectFragment(storageConfig)}
              FROM ${storageConfig.filesQualifiedName}
              WHERE content_hash = $1 AND bucket_id = $2
              LIMIT 1`,
       values: [staged.contentHash, bucket.id],
     });
     return result.rows[0] as
-      | { id: string; key: string; mime_type: string; size: number; filename: string | null }
+      | {
+          id: string;
+          key: string;
+          mime_type: string;
+          size: number;
+          filename: string | null;
+          status?: string;
+        }
       | undefined;
   });
 
-  if (existing) {
+  // Only a row that already stands for stored bytes may absorb this upload. One
+  // that never received them is dropped, and the staged object is promoted as a
+  // fresh file below — which is also what keeps the insert possible, since the
+  // final key is the content hash and (bucket_id, key) is unique. The GC job the
+  // delete enqueues re-takes the reference count when it runs, by which point
+  // the replacement row exists, so it no-ops.
+  if (existing && !isLiveFileRow(storageConfig, existing)) {
+    log.info(
+      `Restarting upload of hash ${staged.contentHash}: file ${existing.id} is ${existing.status}, so it carries no bytes`
+    );
+    await withRequestPgClient(withPgClient, pgSettings, async (pgClient) => {
+      await pgClient.query({
+        text: `DELETE FROM ${storageConfig.filesQualifiedName} WHERE id = $1`,
+        values: [existing.id],
+      });
+    });
+  } else if (existing) {
     log.info(`Dedup hit: file ${existing.id} already carries hash ${staged.contentHash}`);
     await deleteS3Object(s3, staged.stagingKey);