diff --git a/.changeset/25694-anonymous-write-carries-no-file.md b/.changeset/25694-anonymous-write-carries-no-file.md new file mode 100644 index 00000000000..536812b0740 --- /dev/null +++ b/.changeset/25694-anonymous-write-carries-no-file.md @@ -0,0 +1,19 @@ +--- +'@objectstack/service-storage': patch +--- + +fix(storage)!: an anonymous write carries no file + +Ruling C ("an anonymous submission carries no file") now holds at every anonymous write door of ADR-0138 D2, not only at the public form door. A record write whose caller has no user, such as the guest an `authRequired: false` endpoint executes as under the grants a deployment binds to the guest, may not put a value in a file field (`file`, `image`, `avatar`, `video`, `audio`). It is refused before anything is written, with the form door's envelope: `400 VALIDATION_FAILED`, one `fields[]` entry per refused field carrying the ADR-0114 field code `invalid_value` and the field's label. An empty or absent file field is written as before. Signed-in callers, system writes and context-less programmatic writes are unchanged. No error code is added. + +Clause-②: no (narrowing) — an anonymous write that carried a value in a file field is refused. Nothing that was refused becomes accepted. + +Metadata-protocol: none (a write path's file rule; nothing a metadata author writes changes) + + + +**BREAKING** for an anonymous write that carried a file value, shipped as `patch` because the metadata protocol does not move. + +- **Before.** Only the public form door refused a file value from an anonymous caller. +- **Now.** Every write whose caller has no user is refused with `400 VALIDATION_FAILED` when a file field carries a value. +- **Fix.** Leave file fields empty in an anonymous write. A file is provided by a signed-in caller, through the authenticated upload doors. diff --git a/packages/services/service-storage/src/backfill-sys-file-public-scope.test.ts b/packages/services/service-storage/src/backfill-sys-file-public-scope.test.ts index a88f0d03f95..b63dd1544c3 100644 --- a/packages/services/service-storage/src/backfill-sys-file-public-scope.test.ts +++ b/packages/services/service-storage/src/backfill-sys-file-public-scope.test.ts @@ -125,9 +125,10 @@ describe('sys_file public-scope backfill (#22443 ruling B) — a real ObjectQL o * the source row's scope on the copy. */ // The claiming write acts in an organization: `product` is created by this - // release, whose tenant column is NOT NULL (ADR-0131 D1). + // release, whose tenant column is NOT NULL (ADR-0131 D1). [#25694] Its + // caller is signed in: an anonymous write carries no file. const copyBySecondReference = (recordId: string, fileId: string) => - engine.insert('product', { id: recordId, title: recordId, image: fileId }, { context: { tenantId: 'org_a' } } as any); + engine.insert('product', { id: recordId, title: recordId, image: fileId }, { context: { userId: 'u_writer', tenantId: 'org_a' } } as any); it('a deployment with no public rows runs clean: it counts zero and writes nothing', async () => { await engine.insert('sys_file', { ...legacyPublicRow('f_user'), scope: 'user' }, SYSTEM as any); diff --git a/packages/services/service-storage/src/file-reference-lifecycle.test.ts b/packages/services/service-storage/src/file-reference-lifecycle.test.ts index e540ae4260e..fb83cc17550 100644 --- a/packages/services/service-storage/src/file-reference-lifecycle.test.ts +++ b/packages/services/service-storage/src/file-reference-lifecycle.test.ts @@ -9,6 +9,7 @@ import { type FileReferenceEngine, } from './file-reference-lifecycle.js'; import { assertEngineFindOnePredicate } from '@objectstack/objectql'; +import { resolveThrownHttpError } from '@objectstack/types'; const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); @@ -1357,3 +1358,191 @@ describe('a write references only a file its caller may read', () => { }); }); }); + +describe('an anonymous write carries no file (#25694, ruling C at every anonymous write door)', () => { + /** The guest principal an `authRequired: false` endpoint's write executes as (ADR-0138 D9). */ + const GUEST = { principalKind: 'guest', positions: ['guest'], isSystem: false, tenantId: 'org_1' }; + const guestSession = { organizationId: 'org_1', positions: ['guest'] }; + const SIGNED_IN = { userId: 'u_writer', tenantId: 'org_1' }; + + const LABELLED: Record = { + sys_file: REGISTRY.sys_file, + product: { + name: 'product', + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + image: { type: 'image', label: 'Photo' }, + gallery: { type: 'image', multiple: true }, + manual: { type: 'file' }, + }, + }, + }; + + function installAdmitting(engine: Engine, storage: any = fakeStorage()) { + // A verdict that admits every file: the refusal must not lean on it. + const authorizeRead = vi.fn(async () => true); + installFileReferenceHooks(engine, () => storage, silentLogger(), authorizeRead); + return { storage, authorizeRead }; + } + + async function write( + engine: Engine, + phase: 'insert' | 'update', + data: Record, + id: string, + caller: { session?: any; context?: any }, + ) { + const ctx: any = { + object: 'product', + event: phase === 'insert' ? 'beforeInsert' : 'beforeUpdate', + input: { ...(phase === 'update' ? { id } : {}), data, options: caller.context ? { context: caller.context } : {} }, + session: caller.session, + dispatch: recordDispatch(), + }; + await engine.trigger(ctx.event, ctx); + let row: Record & { id: string }; + if (phase === 'insert') { + row = { ...(ctx.input.data as Record), id }; + (engine.tables.product ??= []).push(row); + } else { + row = (engine.tables.product ?? []).find((r) => String(r.id) === id) as any; + Object.assign(row, ctx.input.data); + } + ctx.event = phase === 'insert' ? 'afterInsert' : 'afterUpdate'; + ctx.result = row; + await engine.trigger(ctx.event, ctx); + return row; + } + + const asGuest = { session: guestSession, context: GUEST }; + const refusedBy = (p: Promise) => p.then(() => null, (e) => e); + const sysFileWrites = (engine: Engine) => + engine.calls.filter((c) => (c.op === 'update' || c.op === 'insert') && c.object === 'sys_file'); + + describe('a value in a file field is refused before anything is written', () => { + const VALUES: Array<[string, unknown]> = [ + ['an unclaimed file id', 'file_a'], + ['a file id another record owns', 'file_b'], + ['an external URL', 'https://example.test/x.png'], + ['an inline file value', { url: 'https://example.test/y.png', name: 'y.png' }], + ]; + for (const [what, value] of VALUES) { + it(`on insert: ${what}`, async () => { + const engine = fakeEngine({ + registry: LABELLED, + files: [file(), file({ id: 'file_b', key: 'user/file_b.png', ref_object: 'product', ref_id: 'p0', ref_field: 'image' })], + records: { product: [{ id: 'p0', image: 'file_b' }] }, + }); + const { storage, authorizeRead } = installAdmitting(engine); + + const err = await refusedBy(write(engine, 'insert', { name: 'n', image: value }, 'p1', asGuest)); + + const answered = resolveThrownHttpError(err); + expect({ status: answered.status, code: answered.code }).toEqual({ status: 400, code: 'VALIDATION_FAILED' }); + expect(err?.fields).toEqual([expect.objectContaining({ field: 'image', code: 'invalid_value', label: 'Photo' })]); + expect(err?.fields[0].message).toContain('Photo'); + // Nothing was written: no row, no claim, no copy, and no file was looked up for it. + expect((engine.tables.product ?? []).map((r) => r.id)).toEqual(['p0']); + expect(sysFileWrites(engine)).toEqual([]); + expect(storage.download).not.toHaveBeenCalled(); + expect(authorizeRead).not.toHaveBeenCalled(); + }); + } + + it('on update', async () => { + const engine = fakeEngine({ registry: LABELLED, files: [file()], records: { product: [{ id: 'p1', image: null }] } }); + installAdmitting(engine); + + const err = await refusedBy(write(engine, 'update', { image: 'file_a' }, 'p1', asGuest)); + + expect(err?.code).toBe('VALIDATION_FAILED'); + expect(err?.fields).toEqual([expect.objectContaining({ field: 'image', code: 'invalid_value' })]); + expect(engine.tables.product[0].image).toBeNull(); + expect(engine.tables.sys_file[0].ref_id ?? null).toBeNull(); + }); + + it('every file field carrying a value is named in one envelope, by its label or else its name', async () => { + const engine = fakeEngine({ registry: LABELLED, files: [file()] }); + installAdmitting(engine); + + const err = await refusedBy( + write(engine, 'insert', { image: 'file_a', gallery: ['file_a'], manual: 'https://example.test/m.pdf' }, 'p1', asGuest), + ); + + expect(err?.code).toBe('VALIDATION_FAILED'); + expect(err?.fields.map((f: any) => [f.field, f.code, f.label])).toEqual([ + ['image', 'invalid_value', 'Photo'], + ['gallery', 'invalid_value', 'gallery'], + ['manual', 'invalid_value', 'manual'], + ]); + }); + }); + + describe('CONTROL — what the rule leaves as it was', () => { + it('an anonymous write with every file field empty or absent is written', async () => { + const engine = fakeEngine({ registry: LABELLED, files: [file()] }); + installAdmitting(engine); + + for (const [id, data] of [ + ['p1', { name: 'absent' }], + ['p2', { name: 'null', image: null }], + ['p3', { name: 'blank', image: '' }], + ['p4', { name: 'empty set', gallery: [] }], + ] as const) { + await write(engine, 'insert', { ...data }, id, asGuest); + } + + expect(engine.tables.product.map((r) => r.id)).toEqual(['p1', 'p2', 'p3', 'p4']); + expect(engine.tables.sys_file[0].ref_id ?? null).toBeNull(); + }); + + it('an anonymous update that clears a file field, or leaves it out, is written', async () => { + const engine = fakeEngine({ registry: LABELLED, records: { product: [{ id: 'p1', name: 'a', image: null }] } }); + installAdmitting(engine); + + await write(engine, 'update', { name: 'b' }, 'p1', asGuest); + await write(engine, 'update', { image: null }, 'p1', asGuest); + + expect(engine.tables.product[0]).toMatchObject({ name: 'b', image: null }); + }); + + it('a signed-in caller\'s write still claims the file', async () => { + const engine = fakeEngine({ registry: LABELLED, files: [file()] }); + installAdmitting(engine); + + await write(engine, 'insert', { image: 'file_a' }, 'p1', { + session: { userId: SIGNED_IN.userId, organizationId: 'org_1' }, + context: SIGNED_IN, + }); + + expect(engine.tables.sys_file[0]).toMatchObject({ ref_object: 'product', ref_id: 'p1', ref_field: 'image' }); + }); + + it('a system write and a context-less write still claim the file', async () => { + const engine = fakeEngine({ registry: LABELLED, files: [file(), file({ id: 'file_c', key: 'user/file_c.png' })] }); + installAdmitting(engine); + + await write(engine, 'insert', { image: 'file_a' }, 'p1', { session: { isSystem: true }, context: { isSystem: true } }); + await write(engine, 'insert', { image: 'file_c' }, 'p2', {}); + + expect(engine.tables.sys_file.map((f) => [f.id, f.ref_id ?? null])).toEqual([ + ['file_a', 'p1'], + ['file_c', 'p2'], + ]); + }); + + it('an object with no file field is not judged', async () => { + const engine = fakeEngine({ files: [file()] }); + installAdmitting(engine); + const ctx: any = { + object: 'tag', event: 'beforeInsert', input: { data: { label: 'file_a' }, options: { context: GUEST } }, + session: guestSession, dispatch: recordDispatch(), + }; + + await engine.trigger('beforeInsert', ctx); + + expect(engine.calls).toEqual([]); + }); + }); +}); diff --git a/packages/services/service-storage/src/file-reference-lifecycle.ts b/packages/services/service-storage/src/file-reference-lifecycle.ts index 5bb83f791d9..a20601e3c08 100644 --- a/packages/services/service-storage/src/file-reference-lifecycle.ts +++ b/packages/services/service-storage/src/file-reference-lifecycle.ts @@ -4,6 +4,8 @@ import { randomUUID } from 'node:crypto'; import { FILE_REFERENCE_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; import type { IStorageService } from '@objectstack/spec/contracts'; import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { isEmptyForRequired } from '@objectstack/core'; +import { validationFailure } from '@objectstack/types'; import { callerContext } from './attachment-access-hooks.js'; /** @@ -51,6 +53,18 @@ import { callerContext } from './attachment-access-hooks.js'; * the read check. System writes and context-less programmatic calls carry no * caller to ask, and reference files as before. * + * ### An anonymous write carries no file + * + * [#25694] Ruling C (record 6384525042), held at every anonymous write door + * rather than at the public form door alone: a write whose caller has no user + * (ADR-0138 D2's anonymous write doors: the guest envelope of an + * `authRequired: false` endpoint, under whatever grants a deployment binds to + * the guest) may not put a value in a file field. It is refused before the driver runs, naming each + * field, with the form door's envelope ({@link assertAnonymousWriteCarriesNoFile}). + * The read verdict above would admit a `public_read` file to such a caller, + * and keep any other id as written; neither is a file the anonymous caller + * may provide. An empty or absent file field is written as before. + * * ### The one write that cannot honour it, and is refused * * [#7102] A predicate (`multi: true`) update has ONE payload for N matched @@ -277,6 +291,50 @@ async function findReferenceableFile( return (await authorizeRead(row, caller)) ? row : null; } +/** + * [#25694] Ruling C at every anonymous write door: an anonymous write carries + * no file. A file is provided by a signed-in caller, through the authenticated + * upload doors; a caller with no user has none to provide, so a value in a + * file field of its write is refused, whether it names a platform file (one it + * may download or not) or anything else. Judged before the driver runs and + * before any file is looked up, so a refused write leaves nothing behind. + * + * The form door's own judgement (`form-routes.ts`, #25572), held at the + * engine: the same emptiness rule (`isEmptyForRequired`), and the same + * envelope — one `400 VALIDATION_FAILED` naming every refused field by its + * label with the closed ADR-0114 field code `invalid_value`. The form door + * keeps its own judgement, which answers before anything reaches the engine. + * + * `caller` is {@link referencingCallerOf}'s answer, the caller the read + * verdict is asked about: a system write and a context-less programmatic + * write have none, and are not judged; a caller with no user is anonymous. + */ +function assertAnonymousWriteCarriesNoFile( + engine: FileReferenceEngine, + object: string, + data: Record, + fileFields: string[], + caller: ExecutionContext | null, +): void { + if (!caller || caller.userId) return; + const refused: Array<{ field: string; code: 'invalid_value'; message: string; label: string }> = []; + for (const field of fileFields) { + if (!Object.prototype.hasOwnProperty.call(data, field)) continue; + const def = fieldDefOf(engine, object, field); + if (isEmptyForRequired(def, data[field])) continue; + const declared = typeof def?.label === 'string' ? def.label.trim() : ''; + const label = declared.length > 0 ? declared : field; + refused.push({ + field, + code: 'invalid_value', + message: `${label} cannot take a file here: an anonymous write carries no file. ` + + 'Leave it empty, or sign in and provide the file from your account.', + label, + }); + } + if (refused.length > 0) throw validationFailure(refused.map((f) => f.message).join('; '), refused); +} + /** Bound on owned files released per record delete. */ const RELEASE_BATCH_LIMIT = 1_000; @@ -869,9 +927,11 @@ export function installFileReferenceHooks( if (!object || !data || typeof data !== 'object') return; const fileFields = activeFileFields(engine, object); if (fileFields.length === 0) return; + const caller = referencingCallerOf(ctx); + assertAnonymousWriteCarriesNoFile(engine, object, data, fileFields, caller); await applyCopyOnClaim( engine, getStorage, logger, object, null, data, fileFields, - actingOrganizationOf(ctx), referencingCallerOf(ctx), authorizeRead, mintedCopiesFor(ctx), + actingOrganizationOf(ctx), caller, authorizeRead, mintedCopiesFor(ctx), ); }, { packageId: PACKAGE_ID }, @@ -885,6 +945,10 @@ export function installFileReferenceHooks( if (!object || !data || typeof data !== 'object') return; const fileFields = activeFileFields(engine, object); if (fileFields.length === 0) return; + // [#25694] Before the predicate refusal below: an anonymous write is + // refused for what it carries, whichever shape the write takes. + const caller = referencingCallerOf(ctx); + assertAnonymousWriteCarriesNoFile(engine, object, data, fileFields, caller); // [#6966] "Is this a multi-update?" is the engine's marker, not the shape // of `input.id`. A per-row context names ITS row, so the old // `ids.length === 1` test answered "single update" on every row of a @@ -928,7 +992,7 @@ export function installFileReferenceHooks( // analysis a later edit has to re-derive. await applyCopyOnClaim( engine, getStorage, logger, object, recordId, data, fileFields, - actingOrganizationOf(ctx), referencingCallerOf(ctx), authorizeRead, mintedCopiesFor(ctx), + actingOrganizationOf(ctx), caller, authorizeRead, mintedCopiesFor(ctx), ); }, { packageId: PACKAGE_ID }, diff --git a/packages/services/service-storage/src/file-reference-read-verdict.integration.test.ts b/packages/services/service-storage/src/file-reference-read-verdict.integration.test.ts index 2e16d2d6b1a..9491e222827 100644 --- a/packages/services/service-storage/src/file-reference-read-verdict.integration.test.ts +++ b/packages/services/service-storage/src/file-reference-read-verdict.integration.test.ts @@ -60,6 +60,7 @@ import type { IStorageService, RouteHandler } from '@objectstack/spec/contracts' import { RAW_FILE_VALUES_CONTEXT_KEY, type EngineQueryOptions } from '@objectstack/spec/data'; import { ObjectQL } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; +import { resolveThrownHttpError } from '@objectstack/types'; import { StorageServicePlugin } from './storage-service-plugin.js'; import { installDefaultOrganization } from './default-organization.testkit.js'; @@ -86,6 +87,10 @@ const F_FREE = 'f_free_ff'; const F_DEC_ALLOW = 'f_decallow_gg'; /** Owned by `decision/dec_deny.proof` — the delegate refuses the writer. */ const F_DEC_DENY = 'f_decdeny_hh'; +/** No owner at all, `acl: 'public_read'`: anyone's to download. */ +const F_PUBLIC_FREE = 'f_publicfree_ii'; +/** [#25694] The guest principal an `authRequired: false` endpoint's write executes as (ADR-0138 D9). */ +const GUEST_CTX = { principalKind: 'guest' as const, positions: ['guest'], isSystem: false }; const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn(), error: vi.fn(), @@ -123,6 +128,7 @@ const FILES = [ fileRow(F_FREE), fileRow(F_DEC_ALLOW, owned('decision', 'dec_allow', 'proof')), fileRow(F_DEC_DENY, owned('decision', 'dec_deny', 'proof')), + fileRow(F_PUBLIC_FREE, { acl: 'public_read' }), ]; function makeCtx() { @@ -420,4 +426,68 @@ describe('#25692 — a record write claims or copies only a file the download do expect(outcome).toBe('copied'); }); + + describe('[#25694] an anonymous write carries no file, whatever the door would serve it', () => { + /** + * The refusal as the HTTP boundary answers it (ADR-0112: code and status), + * through the one mapping both dispatcher exits and the REST door share. + */ + const expectAnswered400 = (err: unknown) => { + const answered = resolveThrownHttpError(err); + expect({ status: answered.status, code: answered.code }).toEqual({ status: 400, code: 'VALIDATION_FAILED' }); + expect(answered.details?.fields).toEqual([expect.objectContaining({ field: 'doc', code: 'invalid_value' })]); + }; + + /** The write's refusal, and proof that nothing was written: no record, no new file, no owner moved. */ + const expectRefused = async (data: Record, context: Record) => { + const recordId = `c_new_${++seq}`; + const before = await fileRows(); + const err = await engine + .insert('contract', { id: recordId, title: recordId, ...data } as any, { context }) + .then(() => null, (e: any) => e); + expectAnswered400(err); + expect(err?.fields).toEqual([expect.objectContaining({ field: 'doc', code: 'invalid_value' })]); + expect(await storedDoc(recordId)).toBeUndefined(); + expect(await fileRows()).toEqual(before); + }; + + it('a public_read file another record owns is not copied for the guest', async () => { + await expectRefused({ doc: F_PUBLIC }, GUEST_CTX); + expect(await fileById(F_PUBLIC)).toMatchObject(owned('vault', 'v_mine', 'annex')); + }); + + it('an unclaimed public_read file is not claimed for the guest', async () => { + await expectRefused({ doc: F_PUBLIC_FREE }, GUEST_CTX); + expect((await fileById(F_PUBLIC_FREE))?.ref_id ?? null).toBeNull(); + }); + + it('a file id the guest may not read is not stored either', async () => { + await expectRefused({ doc: F_FREE }, GUEST_CTX); + }); + + it('a value that names no platform file is not stored either', async () => { + await expectRefused({ doc: 'https://example.test/report.pdf' }, GUEST_CTX); + }); + + it('an update carrying a file is refused, and the record keeps what it held', async () => { + const recordId = `c_new_${++seq}`; + await engine.insert('contract', { id: recordId, title: recordId } as any, { context: GUEST_CTX }); + const err = await engine + .update('contract', { id: recordId, doc: F_PUBLIC_FREE } as any, { context: GUEST_CTX }) + .then(() => null, (e: any) => e); + expectAnswered400(err); + expect(await storedDoc(recordId)).toBeNull(); + expect((await fileById(F_PUBLIC_FREE))?.ref_id ?? null).toBeNull(); + }); + + it('CONTROL — a guest write that carries no file is written', async () => { + const recordId = `c_new_${++seq}`; + await engine.insert('contract', { id: recordId, title: recordId, doc: null } as any, { context: GUEST_CTX }); + expect(await storedDoc(recordId)).toBeNull(); + }); + + it('CONTROL — a signed-in writer still takes the same public_read file', async () => { + expect((await writeAs(F_PUBLIC_FREE, WRITER_CTX)).outcome).toBe('claimed'); + }); + }); }); diff --git a/packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts b/packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts index acdf025a76d..b4caa6663ab 100644 --- a/packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts +++ b/packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts @@ -123,6 +123,7 @@ function lifecycleEngine(files: Array>) { * Drive an insert of `product` whose `image` already names a file owned by a * DIFFERENT slot — the one condition that reaches `copyOwnedFile` — with the * session the engine's `buildSession()` would have built for the caller. + * [#25694] Every caller here is signed in: an anonymous write carries no file. */ async function driveCopyingInsert(engine: any, session: unknown) { const data: Record = { image: 'file_owned' }; @@ -170,7 +171,7 @@ describe('#13547 door 5 — copyOwnedFile threads the triggering write’s organ const engine = lifecycleEngine([ownedFile()]); installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); - await driveCopyingInsert(engine, { organizationId: 'org_A' }); + await driveCopyingInsert(engine, { userId: 'u1', organizationId: 'org_A' }); expect(sysFileInsertOf(engine.inserts).data).not.toHaveProperty('organization_id'); }); @@ -205,7 +206,7 @@ describe('#13547 door 5 — copyOwnedFile threads the triggering write’s organ const engine = lifecycleEngine([{ ...ownedFile(), organization_id: 'org_SOURCE' }]); installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); - await driveCopyingInsert(engine, { organizationId: 'org_A' }); + await driveCopyingInsert(engine, { userId: 'u1', organizationId: 'org_A' }); expect(sysFileInsertOf(engine.inserts).options?.context?.tenantId).toBe('org_A'); }); @@ -218,7 +219,7 @@ describe('#13547 door 5 — copyOwnedFile threads the triggering write’s organ const engine = lifecycleEngine([ownedFile()]); installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); - await driveCopyingInsert(engine, { organizationId: 'org_A' }); + await driveCopyingInsert(engine, { userId: 'u1', organizationId: 'org_A' }); expect(engine.updates.length).toBeGreaterThan(0); for (const { data } of engine.updates) {