From 1d3e33fc1d7c0e249b4e0e00d5f3d756fc2bef30 Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Fri, 16 Oct 2026 21:54:27 +0000 Subject: [PATCH 1/2] fix(presigned-url): read a visible file's bucket physical name through the generated reader downloadUrl and the sync S3 delete read the bucket row directly on the request lane, so a reader who sees a file through file RLS (public file in another entity, path share) but not its bucket row got a null downloadUrl. Both now call the storage module's generated _physical_name(bucket_id) reader (SECURITY DEFINER, scoped to the caller's claimed database), on the caller's own request lane. --- .../__tests__/bucket-physical-name.test.ts | 73 +++++++++++++++++++ .../__tests__/custom-key-upload.test.ts | 1 + .../__tests__/dedup-liveness.test.ts | 1 + .../__tests__/managed-upload.test.ts | 1 + .../__tests__/storage-file-recorder.test.ts | 1 + .../src/download-url-field.ts | 26 +++---- .../src/plugin.ts | 17 ++--- .../src/storage-module-cache.ts | 30 ++++++++ .../src/types.ts | 2 + 9 files changed, 127 insertions(+), 25 deletions(-) create mode 100644 graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts diff --git a/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts new file mode 100644 index 0000000000..4bf4195e46 --- /dev/null +++ b/graphile/graphile-presigned-url-plugin/__tests__/bucket-physical-name.test.ts @@ -0,0 +1,73 @@ +import { readBucketPhysicalName } from '../src/storage-module-cache'; +import type { StorageModuleConfig } from '../src/types'; + +function storageConfig( + overrides: Partial = {}, +): StorageModuleConfig { + return { + id: '11111111-1111-1111-1111-111111111111', + bucketsQualifiedName: 'storage_public.app_buckets', + filesQualifiedName: 'storage_public.app_files', + schemaName: 'storage_public', + bucketsTableName: 'app_buckets', + filesTableName: 'app_files', + recorderQualifiedName: 'storage_private.app_files_record_file', + bucketPhysicalNameQualifiedName: 'storage_private.app_buckets_physical_name', + scope: 'app', + entityTableId: null, + entityQualifiedName: null, + endpoint: null, + publicUrlPrefix: null, + provider: 'minio', + region: 'us-east-1', + connectionOverrides: [], + allowedOrigins: null, + uploadUrlExpirySeconds: 900, + downloadUrlExpirySeconds: 3600, + defaultMaxFileSize: 1000, + maxFilenameLength: 1024, + cacheTtlSeconds: 300, + hasPathShares: false, + hasVersioning: false, + hasContentHash: false, + hasConfirmUpload: false, + maxBulkFiles: 100, + maxBulkTotalSize: 1000, + ...overrides, + }; +} + +const BUCKET_ID = '22222222-2222-2222-2222-222222222222'; + +function client(rows: Array>) { + return { query: jest.fn().mockResolvedValue({ rows }) }; +} + +describe('readBucketPhysicalName', () => { + it('reads the coordinate through the generated physical-name reader, not the bucket table', async () => { + const pgClient = client([{ physical_name: 'app-a1b2-public' }]); + + await expect(readBucketPhysicalName(pgClient, storageConfig(), BUCKET_ID)).resolves.toBe('app-a1b2-public'); + + expect(pgClient.query).toHaveBeenCalledTimes(1); + const [{ text, values }] = pgClient.query.mock.calls[0]; + expect(text).toBe('SELECT storage_private.app_buckets_physical_name($1) AS physical_name'); + expect(text).not.toContain('storage_public.app_buckets'); + expect(values).toEqual([BUCKET_ID]); + }); + + it('returns null when no physical bucket was recorded', async () => { + const pgClient = client([{ physical_name: null }]); + + await expect(readBucketPhysicalName(pgClient, storageConfig(), BUCKET_ID)).resolves.toBeNull(); + }); + + it('throws when the storage module has no private schema to read from', async () => { + const pgClient = client([]); + + await expect( + readBucketPhysicalName(pgClient, storageConfig({ bucketPhysicalNameQualifiedName: null }), BUCKET_ID), + ).rejects.toThrow('STORAGE_BUCKET_READER_MISSING'); + expect(pgClient.query).not.toHaveBeenCalled(); + }); +}); diff --git a/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts index 0ddc69ee50..d4399151e1 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/custom-key-upload.test.ts @@ -28,6 +28,7 @@ function storageConfig(overrides: Partial = {}): StorageMod filesQualifiedName: 'storage_public.app_files', filesTableName: 'app_files', recorderQualifiedName: 'storage_private.app_files_record_file', + bucketPhysicalNameQualifiedName: 'storage_private.app_buckets_physical_name', defaultMaxFileSize: 1000, maxFilenameLength: 1024, uploadUrlExpirySeconds: 900, diff --git a/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts index 6338517d98..d027513109 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/dedup-liveness.test.ts @@ -41,6 +41,7 @@ function storageConfig(overrides: Partial = {}): StorageMod filesQualifiedName: 'storage_public.app_files', filesTableName: 'app_files', recorderQualifiedName: 'storage_private.app_files_record_file', + bucketPhysicalNameQualifiedName: 'storage_private.app_buckets_physical_name', defaultMaxFileSize: 1000, maxFilenameLength: 1024, uploadUrlExpirySeconds: 900, diff --git a/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts index cc9e0fb687..e3567852cb 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts @@ -129,6 +129,7 @@ function storageConfig(): StorageModuleConfig { filesQualifiedName: 'storage_public.app_files', filesTableName: 'app_files', recorderQualifiedName: 'storage_private.app_files_record_file', + bucketPhysicalNameQualifiedName: 'storage_private.app_buckets_physical_name', defaultMaxFileSize: 1000, hasPathShares: false, allowedOrigins: null, diff --git a/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts b/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts index 3743d04e76..d15ab309c7 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/storage-file-recorder.test.ts @@ -12,6 +12,7 @@ function storageConfig( bucketsTableName: 'app_buckets', filesTableName: 'app_files', recorderQualifiedName: 'storage_private.app_files_record_file', + bucketPhysicalNameQualifiedName: 'storage_private.app_buckets_physical_name', scope: 'app', entityTableId: null, entityQualifiedName: null, diff --git a/graphile/graphile-presigned-url-plugin/src/download-url-field.ts b/graphile/graphile-presigned-url-plugin/src/download-url-field.ts index 631f82759d..41d0fd9c70 100644 --- a/graphile/graphile-presigned-url-plugin/src/download-url-field.ts +++ b/graphile/graphile-presigned-url-plugin/src/download-url-field.ts @@ -29,7 +29,7 @@ import { DOWNLOAD_URL_FIELD } from 'graphile-storage-registry'; import { resolveS3ForDatabase } from './physical-bucket'; import { generatePresignedGetUrl } from './s3-signer'; -import { loadAllStorageModules, resolveStorageConfigFromCodec, storedPhysicalName } from './storage-module-cache'; +import { loadAllStorageModules, readBucketPhysicalName, resolveStorageConfigFromCodec } from './storage-module-cache'; import type { PresignedUrlPluginOptions } from './types'; const log = new Logger('graphile-presigned-url:download-url'); @@ -126,20 +126,20 @@ export function createDownloadUrlPlugin( ) : null; // The file row already passed the reader's RLS, which is the - // authorization for this URL. The bucket row is read the same - // way, as the request's own role. + // authorization for this URL. The reader may not see the bucket + // row itself (public file, path share), so its coordinate comes + // from the module's narrow bucket physical-name reader. const resolved = config && bucketId - ? await withRequestPgClient(withPgClient, pgSettings, async (pgClient) => { - const bucketResult = await pgClient.query({ - text: `SELECT key, physical_name FROM ${config.bucketsQualifiedName} WHERE id = $1 LIMIT 1`, - values: [bucketId], - }); - const row = bucketResult.rows[0] as { key: string; physical_name?: string | null } | undefined; - return row ? { config, physicalName: storedPhysicalName(row) } : null; - }) + ? { + config, + physicalName: await withRequestPgClient( + withPgClient, + pgSettings, + (pgClient) => readBucketPhysicalName(pgClient, config, bucketId), + ), + } : null; - // No resolvable bucket row, or no physical bucket was ever - // provisioned: no object can exist. + // No physical bucket was ever provisioned: no object can exist. if (!resolved || resolved.physicalName === null) return null; const s3ForDb = resolveS3ForDatabase(options, resolved.config, resolved.physicalName); diff --git a/graphile/graphile-presigned-url-plugin/src/plugin.ts b/graphile/graphile-presigned-url-plugin/src/plugin.ts index 3f996b774d..517bd2727e 100644 --- a/graphile/graphile-presigned-url-plugin/src/plugin.ts +++ b/graphile/graphile-presigned-url-plugin/src/plugin.ts @@ -36,7 +36,7 @@ import { assertBucketReconciled, resolveS3ForDatabase } from './physical-bucket' import { deleteS3Object,generatePresignedPutUrl } from './s3-signer'; import { recordManagedFile } from './storage-file-recorder'; import { fileNotReplaceable } from './file-not-replaceable'; -import { getBucketConfig, loadAllStorageModules, resolveStorageConfigFromCodec, storedPhysicalName } from './storage-module-cache'; +import { getBucketConfig, loadAllStorageModules, readBucketPhysicalName, resolveStorageConfigFromCodec } from './storage-module-cache'; import type { BucketConfig,PresignedUrlPluginOptions, S3Config, StorageModuleConfig } from './types'; const log = new Logger('graphile-presigned-url:plugin'); @@ -554,17 +554,10 @@ export function createPresignedUrlPlugin( // No other references — attempt sync S3 delete. // Read the stored physical coordinate; the object lives in the - // recorded bucket, never a recomputed prefix name. - const bucketResult = await pgClient.query({ - text: `SELECT key, physical_name FROM ${storageConfig.bucketsQualifiedName} WHERE id = $1 LIMIT 1`, - values: [fileRow!.bucket_id], - }); - const bucketRow = bucketResult.rows[0] as { key: string; physical_name?: string | null } | undefined; - if (!bucketRow) { - log.warn(`Bucket not found for bucket_id=${fileRow!.bucket_id}; skipping S3 delete`); - return; - } - const physicalName = storedPhysicalName(bucketRow); + // recorded bucket, never a recomputed prefix name. A deleter may + // not see the bucket row (path share with can_delete), so it is + // read through the module's narrow bucket physical-name reader. + const physicalName = await readBucketPhysicalName(pgClient, storageConfig, fileRow!.bucket_id); if (physicalName === null) { // No physical bucket was ever provisioned — there is no object to delete. log.warn(`Bucket ${fileRow!.bucket_id} has no physical_name; skipping S3 delete`); diff --git a/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts b/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts index 2b6971ec76..e02356d19a 100644 --- a/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts +++ b/graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts @@ -119,6 +119,9 @@ function buildConfig(row: StorageModuleRow): StorageModuleConfig { recorderQualifiedName: row.private_schema ? QuoteUtils.quoteQualifiedIdentifier(row.private_schema, `${row.files_table}_record_file`) : null, + bucketPhysicalNameQualifiedName: row.private_schema + ? QuoteUtils.quoteQualifiedIdentifier(row.private_schema, `${row.buckets_table}_physical_name`) + : null, schemaName: row.buckets_schema, bucketsTableName: row.buckets_table, filesTableName: row.files_table, @@ -274,6 +277,33 @@ const bucketCache = new LRUCache({ * the single place that shape is interpreted — callers branch on `string` * vs `null` and never coalesce a bucket name into existence. */ +/** + * The recorded physical bucket of a file the caller already read under RLS. + * + * A reader may see a file (public, or path-shared) without seeing its bucket + * row, so the bucket table is not read directly. The storage module's + * generated `_physical_name` reader (SECURITY DEFINER, scoped to the + * caller's claimed database) returns only the bucket's coordinate; it runs on + * the caller's own request lane. The visible file row is the authorization. + * + * Returns null when no physical bucket was recorded (never provisioned, so no + * object can exist). + */ +export async function readBucketPhysicalName( + pgClient: { query(opts: { text: string; values?: unknown[] }): Promise<{ rows: Array> }> }, + config: StorageModuleConfig, + bucketId: string, +): Promise { + if (!config.bucketPhysicalNameQualifiedName) { + throw new Error(`STORAGE_BUCKET_READER_MISSING: storage module ${config.id} records no private schema`); + } + const result = await pgClient.query({ + text: `SELECT ${config.bucketPhysicalNameQualifiedName}($1) AS physical_name`, + values: [bucketId], + }); + return storedPhysicalName(result.rows[0] as { physical_name?: string | null }); +} + export function storedPhysicalName(row: { physical_name?: string | null }): string | null { return row.physical_name == null ? null : row.physical_name; } diff --git a/graphile/graphile-presigned-url-plugin/src/types.ts b/graphile/graphile-presigned-url-plugin/src/types.ts index 785517fd89..25a545210f 100644 --- a/graphile/graphile-presigned-url-plugin/src/types.ts +++ b/graphile/graphile-presigned-url-plugin/src/types.ts @@ -38,6 +38,8 @@ export interface StorageModuleConfig { filesTableName: string; /** Qualified generated managed-file recorder, or null when unavailable */ recorderQualifiedName: string | null; + /** Qualified generated request-lane bucket physical-name reader, or null when unavailable */ + bucketPhysicalNameQualifiedName: string | null; // --- Scope identity --- From e0e43fccd15b5361dbb2689fa1b6f1573feb7e1c Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Fri, 16 Oct 2026 22:00:12 +0000 Subject: [PATCH 2/2] style(presigned-url): sort imports --- graphile/graphile-presigned-url-plugin/src/plugin.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphile/graphile-presigned-url-plugin/src/plugin.ts b/graphile/graphile-presigned-url-plugin/src/plugin.ts index 517bd2727e..615f129592 100644 --- a/graphile/graphile-presigned-url-plugin/src/plugin.ts +++ b/graphile/graphile-presigned-url-plugin/src/plugin.ts @@ -31,11 +31,11 @@ import { checkTypeAgreement } from 'mime-bytes'; import { validateCustomKey } from './custom-key'; import { resolveDefaultBucket } from './default-bucket'; import { isLiveFileRow, statusSelectFragment } from './file-lifecycle'; +import { fileNotReplaceable } from './file-not-replaceable'; import { buildFileProjection, type FileProjection } from './managed-upload'; import { assertBucketReconciled, resolveS3ForDatabase } from './physical-bucket'; import { deleteS3Object,generatePresignedPutUrl } from './s3-signer'; import { recordManagedFile } from './storage-file-recorder'; -import { fileNotReplaceable } from './file-not-replaceable'; import { getBucketConfig, loadAllStorageModules, readBucketPhysicalName, resolveStorageConfigFromCodec } from './storage-module-cache'; import type { BucketConfig,PresignedUrlPluginOptions, S3Config, StorageModuleConfig } from './types';