Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { readBucketPhysicalName } from '../src/storage-module-cache';
import type { StorageModuleConfig } from '../src/types';

function storageConfig(
overrides: Partial<StorageModuleConfig> = {},
): 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<Record<string, unknown>>) {
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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ function storageConfig(overrides: Partial<StorageModuleConfig> = {}): 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ function storageConfig(overrides: Partial<StorageModuleConfig> = {}): 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
26 changes: 13 additions & 13 deletions graphile/graphile-presigned-url-plugin/src/download-url-field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Comment on lines 132 to 143

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 bug · high

Download resolver now throws where it previously returned null

The download resolver replaced a direct bucket-table read that degraded to null with readBucketPhysicalName, which throws STORAGE_BUCKET_READER_MISSING when the module records no private_schema (graphile/graphile-presigned-url-plugin/src/download-url-field.ts:132-143; throw at storage-module-cache.ts:297-299). The Grafast lambda has no error handling, so a missing reader function (legacy module predating the generated reader) or a null private_schema surfaces as a GraphQL field error instead of the previous downloadUrl: null. The delete path has the same regression in milder form: the reader throw is swallowed by the generic catch at plugin.ts:570-573 and logged as a transient S3 failure, silently skipping sync S3 cleanup. Legacy or reader-less storage modules go from working (or cleanly skipping) to hard errors and orphaned S3 objects.

📋 Prompt for AI Agents

In graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts around lines 292-305, change readBucketPhysicalName so that a null config.bucketPhysicalNameQualifiedName (currently throwing STORAGE_BUCKET_READER_MISSING at line 297-298) and a missing/unexecutable reader function (SQLSTATE 42883) fall back to a direct SELECT physical_name FROM ${config.bucketsQualifiedName} WHERE id = $1 LIMIT 1 read under the caller's RLS, returning null when the row is absent — the pre-patch behavior of both download-url-field.ts:132-143 and plugin.ts:560. Optionally distinguish the reader-unavailable case in plugin.ts's catch (lines 570-573) with a distinct log so it is not misreported as a transient S3 failure.


const s3ForDb = resolveS3ForDatabase(options, resolved.config, resolved.physicalName);
Expand Down
19 changes: 6 additions & 13 deletions graphile/graphile-presigned-url-plugin/src/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ 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, 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');
Expand Down Expand Up @@ -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`);
Expand Down
30 changes: 30 additions & 0 deletions graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -274,6 +277,33 @@ const bucketCache = new LRUCache<string, BucketConfig>({
* 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 `<buckets>_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<Record<string, unknown>> }> },
config: StorageModuleConfig,
bucketId: string,
): Promise<string | null> {
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;
}
Expand Down
2 changes: 2 additions & 0 deletions graphile/graphile-presigned-url-plugin/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ---

Expand Down
Loading