From 1e32120e423d0893fd73e5fc4bbfec66d95dd9f0 Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Wed, 19 Aug 2026 04:48:38 +0000 Subject: [PATCH] fix(uploads): default an unregistered upload column by plane shape, not scope name Pin the database-scope upload surface end to end: an unprefixed buckets/files plane emits uploadFile alongside the prefixed uploadAppFile, and a presigned PUT lands in a real physical bucket recorded on the tenant's own row. --- .../__tests__/managed-upload.test.ts | 62 +++++ .../src/managed-upload.ts | 35 ++- .../seed/db-scope-storage/schema.sql | 50 ++++ .../seed/db-scope-storage/test-data.sql | 89 +++++++ .../db-scope-upload.integration.test.ts | 234 ++++++++++++++++++ 5 files changed, 463 insertions(+), 7 deletions(-) create mode 100644 graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql create mode 100644 graphql/server-test/__fixtures__/seed/db-scope-storage/test-data.sql create mode 100644 graphql/server-test/__tests__/db-scope-upload.integration.test.ts 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 cb83ea5b4..5961817cb 100644 --- a/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts +++ b/graphile/graphile-presigned-url-plugin/__tests__/managed-upload.test.ts @@ -310,6 +310,68 @@ describe('resolveManagedUploadTarget', () => { ).rejects.toThrow('STORAGE_MODULE_NOT_FOUND'); }); + // The fallback for an unregistered column selects the database's single + // global plane by shape (no entity key), not by the name its scope happens to + // carry: a database-scope plane registers as 'database', and matching 'app' + // sent every one of them to STORAGE_MODULE_NOT_FOUND. + it('defaults an unregistered column to a database-scope plane', async () => { + const { resolveManagedUploadTarget } = await import('../src/managed-upload'); + const db = fakeDb([ + SET_CONFIG, + NO_REGISTRY_ROW, + { + match: /FROM metaschema_modules_public\.storage_module/, + rows: () => [storageModuleRow({ + scope: 'database', + buckets_table: 'buckets', + files_table: 'files', + })], + }, + { match: /resolve_default_bucket/, rows: () => [{ bucket_id: BUCKET_ID, resolved_key: 'default-public', bucket_type: 'public', physical_name: 'myapp-default-public-db' }] }, + { match: /FROM storage_public\.buckets/, rows: () => [bucketRow()] }, + ]); + + const target = await resolveManagedUploadTarget({ + options: options(), + withPgClient: db.withPgClient, + pgSettings: null, + databaseId: DATABASE_ID, + field: FIELD, + defaultPublicAccess: true, + }); + + expect(target.storageConfig.scope).toBe('database'); + expect(target.physicalName).toBe('myapp-default-public-db'); + const resolveCall = db.queries.find((q) => /resolve_default_bucket/.test(q.text)); + expect(resolveCall?.values).toEqual([DATABASE_ID, 'database', null, true, null]); + }); + + it('refuses to guess between two global planes for an unregistered column', async () => { + const { resolveManagedUploadTarget } = await import('../src/managed-upload'); + const db = fakeDb([ + SET_CONFIG, + NO_REGISTRY_ROW, + { + match: /FROM metaschema_modules_public\.storage_module/, + rows: () => [ + storageModuleRow({ scope: 'database', buckets_table: 'buckets', files_table: 'files' }), + storageModuleRow({ id: OTHER_MODULE_ID, scope: 'platform', buckets_table: 'platform_buckets', files_table: 'platform_files' }), + ], + }, + ]); + + await expect( + resolveManagedUploadTarget({ + options: options(), + withPgClient: db.withPgClient, + pgSettings: null, + databaseId: DATABASE_ID, + field: FIELD, + defaultPublicAccess: true, + }), + ).rejects.toThrow('STORAGE_MODULE_AMBIGUOUS'); + }); + it('raises when the registry names a module the database does not have', async () => { const { resolveManagedUploadTarget } = await import('../src/managed-upload'); const db = fakeDb([ diff --git a/graphile/graphile-presigned-url-plugin/src/managed-upload.ts b/graphile/graphile-presigned-url-plugin/src/managed-upload.ts index 0f89bb05c..2588b01db 100644 --- a/graphile/graphile-presigned-url-plugin/src/managed-upload.ts +++ b/graphile/graphile-presigned-url-plugin/src/managed-upload.ts @@ -92,11 +92,19 @@ export interface ManagedUploadTarget { * * a registered column names its storage module, and either a logical bucket * key or the reserved default tag for its declared publicness; * * an unregistered column (a bare `image`/`upload` on a database provisioned - * before the registry) falls back to the app-scope module and the same - * reserved default tag. That is a *tenant* default, not an environment one. + * before the registry) falls back to the database's single global storage + * plane and the same reserved default tag. That is a *tenant* default, not + * an environment one. * - * A database with no storage module raises: there is nowhere tenant-owned to put - * the bytes, and the deployment's configured bucket is not an answer. + * The fallback is by shape, not by scope name: a global plane is one with no + * entity key (`entity_table_id IS NULL`), which is what a database-wide plane is + * whether it registered as 'database', 'platform', or 'app'. Matching the name + * instead sent every tenant whose plane registers as 'database' — i.e. every + * database-scope plane — to STORAGE_MODULE_NOT_FOUND. + * + * A database with no global plane raises, and so does one with several: there is + * nowhere unambiguously tenant-owned to put the bytes, and the deployment's + * configured bucket is not an answer. */ export async function resolveManagedUploadTarget(args: { options: PresignedUrlPluginOptions; @@ -116,7 +124,7 @@ export async function resolveManagedUploadTarget(args: { return await getFileRefFieldBinding(pgClient, databaseId, field); } catch (err: any) { // An unregistered column is a legitimate state (it predates the registry) - // and falls back to the tenant's app-scope default below. Any other + // and falls back to the tenant's global plane below. Any other // failure — a broken connection, a missing registry table — is not. if (err?.name === 'FileRefFieldNotRegisteredError') return null; throw err; @@ -127,9 +135,22 @@ export async function resolveManagedUploadTarget(args: { loadAllStorageModules(pgClient, databaseId), ); + const globalConfigs = allConfigs.filter((c) => c.entityTableId === null); + + if (!binding && globalConfigs.length > 1) { + // Several global planes and no registry row to disambiguate: picking one is + // picking a tenant's bucket at random. Name them and refuse. + throw new Error( + `STORAGE_MODULE_AMBIGUOUS: ${field.schemaName}.${field.tableName}.${field.columnName} is an ` + + `unregistered upload column and database ${databaseId} has ${globalConfigs.length} global ` + + `storage planes (scopes: ${globalConfigs.map((c) => c.scope).join(', ')}); register the column ` + + 'so it names the plane it writes to', + ); + } + const storageConfig = binding ? allConfigs.find((c) => c.id === binding.storageModuleId) - : allConfigs.find((c) => c.scope === 'app'); + : globalConfigs[0]; if (!storageConfig) { throw new Error( @@ -137,7 +158,7 @@ export async function resolveManagedUploadTarget(args: { ? `STORAGE_MODULE_NOT_FOUND: file_ref_field ${binding.id} names storage module ` + `${binding.storageModuleId}, which database ${databaseId} does not have` : `STORAGE_MODULE_NOT_FOUND: ${field.schemaName}.${field.tableName}.${field.columnName} is an ` + - `unregistered upload column and database ${databaseId} has no app-scope storage module to ` + + `unregistered upload column and database ${databaseId} has no global storage plane to ` + 'default to; there is no environment bucket to fall back to', ); } diff --git a/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql b/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql new file mode 100644 index 000000000..7de3e4fa2 --- /dev/null +++ b/graphql/server-test/__fixtures__/seed/db-scope-storage/schema.sql @@ -0,0 +1,50 @@ +-- Schema for the database-scope storage plane. +-- +-- A database-scope plane names its tables `buckets` and `files`, with no module +-- prefix: the plane *is* the database's storage, so there is nothing to +-- disambiguate. The `@storageBuckets`/`@storageFiles` tags are the only thing +-- that marks a plane, and the two tenants here differ only in table naming, so +-- a discovery rule that reads table names instead of tags shows up as a missing +-- upload mutation on this schema. + +CREATE SCHEMA IF NOT EXISTS "tess-storage-public"; + +GRANT USAGE ON SCHEMA "tess-storage-public" TO administrator, authenticated, anonymous; + +CREATE TABLE "tess-storage-public".buckets ( + id uuid PRIMARY KEY DEFAULT uuid_generate_v4(), + key text NOT NULL, + type text NOT NULL DEFAULT 'private', + is_public boolean NOT NULL DEFAULT false, + allowed_mime_types text[] NULL, + max_file_size bigint NULL, + allow_custom_keys boolean NOT NULL DEFAULT false, + allowed_origins text[] NULL, + physical_name text NULL, + created_at timestamptz DEFAULT now(), + updated_at timestamptz DEFAULT now(), + UNIQUE (key) +); + +COMMENT ON TABLE "tess-storage-public".buckets IS E'@storageBuckets\nStorage buckets table'; + +CREATE TABLE "tess-storage-public".files ( + id uuid PRIMARY KEY DEFAULT uuid_generate_v4(), + bucket_id uuid NOT NULL REFERENCES "tess-storage-public".buckets(id), + key text NOT NULL, + content_hash text NOT NULL, + mime_type text NOT NULL, + size bigint, + filename text, + owner_id uuid, + is_public boolean NOT NULL DEFAULT false, + previous_version_id uuid REFERENCES "tess-storage-public".files(id), + created_at timestamptz DEFAULT now(), + updated_at timestamptz DEFAULT now(), + UNIQUE (bucket_id, key) +); + +COMMENT ON TABLE "tess-storage-public".files IS E'@storageFiles\nStorage files table'; + +GRANT SELECT, INSERT, UPDATE, DELETE ON "tess-storage-public".buckets TO administrator, authenticated, anonymous; +GRANT SELECT, INSERT, UPDATE, DELETE ON "tess-storage-public".files TO administrator, authenticated, anonymous; diff --git a/graphql/server-test/__fixtures__/seed/db-scope-storage/test-data.sql b/graphql/server-test/__fixtures__/seed/db-scope-storage/test-data.sql new file mode 100644 index 000000000..05422b663 --- /dev/null +++ b/graphql/server-test/__fixtures__/seed/db-scope-storage/test-data.sql @@ -0,0 +1,89 @@ +-- Test data for the database-scope storage plane (tenant "Tess"). +-- +-- Same rows as the app-scope fixtures, with two differences that are the point +-- of the scenario: the storage module registers under scope 'database', and its +-- tables are the unprefixed `buckets` / `files`. + +SET session_replication_role TO replica; + +INSERT INTO metaschema_public.database (id, owner_id, name, hash) +VALUES ( + 'ce551000-0000-4000-8000-000000000001', + NULL, + 'tess-storage', + '737d3f43-3493-8a83-b801-5d2b3f6a4557' +) ON CONFLICT (id) DO NOTHING; + +INSERT INTO metaschema_public.schema (id, database_id, name, schema_name, description, is_public) +VALUES ( + 'ce552000-0000-4000-8000-000000000001', + 'ce551000-0000-4000-8000-000000000001', + 'public', + 'tess-storage-public', + NULL, + true +) ON CONFLICT (id) DO NOTHING; + +INSERT INTO metaschema_public.table (id, database_id, schema_id, name, description) +VALUES + ('ce553000-0000-4000-8000-000000000001', 'ce551000-0000-4000-8000-000000000001', 'ce552000-0000-4000-8000-000000000001', 'buckets', NULL), + ('ce553000-0000-4000-8000-000000000002', 'ce551000-0000-4000-8000-000000000001', 'ce552000-0000-4000-8000-000000000001', 'files', NULL) +ON CONFLICT (id) DO NOTHING; + +INSERT INTO routing_public.apis (id, database_id, name, dbname, is_published, role_name, anon_role) +VALUES ( + 'ce554000-0000-4000-8000-000000000001', + 'ce551000-0000-4000-8000-000000000001', + 'tess-app', + current_database(), + false, + 'authenticated', + 'anonymous' +) ON CONFLICT (id) DO NOTHING; + +INSERT INTO routing_public.api_schemas (id, database_id, schema_id, api_id) +VALUES ( + 'ce555000-0000-4000-8000-000000000001', + 'ce551000-0000-4000-8000-000000000001', + 'ce552000-0000-4000-8000-000000000001', + 'ce554000-0000-4000-8000-000000000001' +) ON CONFLICT (id) DO NOTHING; + +INSERT INTO metaschema_modules_public.storage_module ( + id, + database_id, + schema_id, + buckets_table_id, + files_table_id, + endpoint, + public_url_prefix, + provider, + allowed_origins, + scope +) +VALUES ( + 'ce556000-0000-4000-8000-000000000001', + 'ce551000-0000-4000-8000-000000000001', + 'ce552000-0000-4000-8000-000000000001', + 'ce553000-0000-4000-8000-000000000001', + 'ce553000-0000-4000-8000-000000000002', + NULL, -- use global CDN_ENDPOINT + NULL, -- use global CDN_PUBLIC_URL_PREFIX + 'minio', + ARRAY['*'], + 'database' +) ON CONFLICT (id) DO NOTHING; + +INSERT INTO "tess-storage-public".buckets (id, key, type, is_public) +VALUES + ('ce557000-0000-4000-8000-000000000001', 'public', 'public', true), + ('ce557000-0000-4000-8000-000000000002', 'private', 'private', false) +ON CONFLICT (id) DO NOTHING; + +INSERT INTO routing_public.database_settings (id, database_id) +VALUES ( + 'ce558000-0000-4000-8000-000000000001', + 'ce551000-0000-4000-8000-000000000001' +) ON CONFLICT (database_id) DO NOTHING; + +SET session_replication_role TO DEFAULT; diff --git a/graphql/server-test/__tests__/db-scope-upload.integration.test.ts b/graphql/server-test/__tests__/db-scope-upload.integration.test.ts new file mode 100644 index 000000000..76f2983c7 --- /dev/null +++ b/graphql/server-test/__tests__/db-scope-upload.integration.test.ts @@ -0,0 +1,234 @@ +/** + * Integration tests — the database-scope upload surface. + * + * A storage plane is marked by its `@storageBuckets`/`@storageFiles` tags, not by + * what its tables are called: an app-scope plane's tables carry the module prefix + * (`app_buckets`/`app_files`) and a database-scope plane's do not + * (`buckets`/`files`), because the plane *is* that database's storage. Both must + * produce an upload surface, so this pins the two together: one schema build over + * both planes has to expose `uploadAppFile` AND `uploadFile`, and the unprefixed + * one has to work end to end — presigned PUT to MinIO, and a physical bucket + * recorded on the tenant's own bucket row. + * + * Uses real MinIO (the `minio_cdn` service in CI, localhost:9000 locally). + * + * pnpm test -- --testPathPattern=db-scope-upload + */ + +import { hashContent, putToPresignedUrl } from '@constructive-io/upload-client'; +import path from 'path'; +import type { PgTestClient } from 'pgsql-test'; +import type supertest from 'supertest'; + +import { getConnections, seed } from '../src'; + +jest.setTimeout(120000); + +const localSeedRoot = path.join(__dirname, '..', '__fixtures__', 'seed'); +const sharedSeedRoot = path.join(__dirname, '..', '..', '..', '__fixtures__', 'seed'); +const sql = (seedDir: string, file: string) => path.join(localSeedRoot, seedDir, file); +const pgpmWorkspace = path.join(sharedSeedRoot, '..', '..'); + +// Alice — the app-scope plane, prefixed tables (`app_buckets`/`app_files`). +const aliceSchema = 'simple-storage-public'; + +// Tess — the database-scope plane, unprefixed tables (`buckets`/`files`). +const tessDatabaseId = 'ce551000-0000-4000-8000-000000000001'; +const tessSchema = 'tess-storage-public'; + +const metaSchemas = [ + 'catalog_private', + 'routing_public', + 'apps_public', + 'metaschema_public', + 'metaschema_modules_public' +]; + +const seedAdapters = [ + seed.pgpm(pgpmWorkspace), + seed.sqlfile([ + sql('simple-seed-storage', 'schema.sql'), + sql('simple-seed-storage', 'test-data.sql'), + sql('db-scope-storage', 'schema.sql'), + sql('db-scope-storage', 'test-data.sql') + ]) +]; + +const INTROSPECT_MUTATIONS = ` + query IntrospectMutations { + __type(name: "Mutation") { + fields { + name + } + } + } +`; + +const UPLOAD_FILE = ` + mutation UploadFile($input: UploadFileInput!) { + uploadFile(input: $input) { + uploadUrl + fileId + key + deduplicated + expiresAt + } + } +`; + +const BUCKETS = ` + query Buckets { + buckets { + nodes { + id + key + isPublic + } + } + } +`; + +function expectSuccess(res: supertest.Response): Record { + expect(res.status).toBe(200); + expect(res.body.errors).toBeUndefined(); + return res.body.data; +} + +describe('database-scope upload surface', () => { + let request: supertest.Agent; + let pg: PgTestClient; + let teardown: () => Promise; + + const post = ( + databaseId: string, + schemas: string[], + payload: { query: string; variables?: Record } + ) => + request + .post('/graphql') + .set('X-Database-Id', databaseId) + .set('X-Schemata', schemas.join(',')) + .send(payload); + + beforeAll(async () => { + ({ request, pg, teardown } = await getConnections( + { + schemas: [aliceSchema, tessSchema], + authRole: 'anonymous', + server: { + useRouting: true, + api: { + isPublic: false, + metaSchemas + } + } + }, + seedAdapters + )); + }); + + afterAll(async () => { + if (teardown) await teardown(); + }); + + describe('mutation generation', () => { + it('emits an upload surface for the prefixed AND the unprefixed plane', async () => { + const res = await post(tessDatabaseId, [aliceSchema, tessSchema], { + query: INTROSPECT_MUTATIONS + }); + const names: string[] = expectSuccess(res).__type.fields.map((f: { name: string }) => f.name); + + // app scope: `app_files` -> AppFile + expect(names).toContain('uploadAppFile'); + expect(names).toContain('uploadAppFiles'); + // database scope: `files` -> File. Naming carries no meaning; the tags do. + expect(names).toContain('uploadFile'); + expect(names).toContain('uploadFiles'); + }); + }); + + describe('presigned upload against the database-scope plane', () => { + const fileContent = 'tenant static site index'; + const contentType = 'text/plain'; + let contentHash: string; + let uploadUrl: string; + + // MinIO uses path-style URLs: http://host:9000//?... + const bucketFromPresignedUrl = (url: string): string => + new URL(url).pathname.replace(/^\/+/, '').split('/')[0]; + + beforeAll(async () => { + contentHash = await hashContent(fileContent); + }); + + it('returns a presigned PUT URL via uploadFile', async () => { + const res = await post(tessDatabaseId, [tessSchema], { + query: UPLOAD_FILE, + variables: { + input: { + bucketKey: 'public', + contentHash, + contentType, + size: Buffer.byteLength(fileContent), + filename: 'index.html' + } + } + }); + + const payload = expectSuccess(res).uploadFile; + expect(payload.uploadUrl).toBeTruthy(); + expect(payload.fileId).toBeTruthy(); + expect(payload.key).toBe(contentHash); + expect(payload.deduplicated).toBe(false); + + uploadUrl = payload.uploadUrl; + }); + + it('accepts the PUT, into a physical bucket recorded on the tenant row', async () => { + const putRes = await putToPresignedUrl(uploadUrl, fileContent, contentType); + expect(putRes.ok).toBe(true); + + const stored = await pg.query( + `SELECT physical_name FROM "${tessSchema}".buckets WHERE key = 'public'` + ); + const physicalName: string | null = stored.rows[0]?.physical_name ?? null; + expect(physicalName).toBeTruthy(); + expect(physicalName).toMatch(/-public-[a-f0-9]{12}$/); + expect(bucketFromPresignedUrl(uploadUrl)).toBe(physicalName); + }); + + it('records the file row against the tenant plane, not the app-scope one', async () => { + const tess = await pg.query( + `SELECT content_hash FROM "${tessSchema}".files WHERE content_hash = $1`, + [contentHash] + ); + expect(tess.rows).toHaveLength(1); + + const alice = await pg.query( + `SELECT content_hash FROM "${aliceSchema}".app_files WHERE content_hash = $1`, + [contentHash] + ); + expect(alice.rows).toHaveLength(0); + }); + + it('serves the tenant its own buckets under the unprefixed plane', async () => { + const res = await post(tessDatabaseId, [tessSchema], { query: BUCKETS }); + const keys = expectSuccess(res).buckets.nodes.map((n: { key: string }) => n.key); + expect(keys.sort()).toEqual(['private', 'public']); + }); + }); + + describe('tenant isolation across the two planes', () => { + it('does not expose the app-scope tenant’s buckets to the database-scope tenant', async () => { + const res = await post(tessDatabaseId, [tessSchema], { query: BUCKETS }); + const nodes = expectSuccess(res).buckets.nodes as { id: string }[]; + const aliceBucketIds = ( + await pg.query(`SELECT id FROM "${aliceSchema}".app_buckets`) + ).rows.map((r: { id: string }) => r.id); + + for (const node of nodes) { + expect(aliceBucketIds).not.toContain(node.id); + } + }); + }); +});