From 83b9ed0be5c3af6eac46b5afaa71975f3ba81d6b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 01:16:26 +0000 Subject: [PATCH 1/2] fix(service-storage): stamp the acting organization on the last two sys_file insert doors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `copyOwnedFile` (the copy-on-claim lifecycle hook) and `materializeDataUri` (the operator backfill pass) insert `sys_file` rows without going through `StorageMetadataStore`, and carried `isSystem` with no tenant — so `buildDriverOptions` emitted no `DriverOptions.tenantId`, `injectTenantOnInsert` stamped nothing, and every row landed `organization_id = NULL` on a tenancy-enabled object. The driver's `(organization_id = :tenantId OR organization_id IS NULL)` term left those rows reachable from every organization. `isSystem` also sets `bypassTenantAudit = true`, which is the guard `auditMissingTenant` returns at, so the `[tenant-audit]` warning that names this defect never fired. Both doors now thread the organization as an execution context, mirroring the `StorageWriteContext` channel the four repaired doors already use — never as a column on the payload, so `resolveTenantField` / `injectTenantOnInsert` keep deciding whether the object has a tenant column and whether an explicit value wins. The copy takes the triggering write's organization from `HookContext.session.organizationId`; the backfill takes the organization of the record whose field held the bytes, resolved by the same `createWallOrganizationResolver` the `sys_file` organization sweep uses. Forward-stamping only: no existing row's organization is written. Where no organization is in scope the `tenantId` key is omitted entirely and the write proceeds exactly as before. Part of #13547 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...ile-copy-backfill-organization-stamping.md | 60 +++ .../src/backfill-file-references.ts | 77 +++- .../src/file-reference-lifecycle.ts | 77 +++- ...opy-backfill-organization-stamping.test.ts | 362 ++++++++++++++++++ 4 files changed, 568 insertions(+), 8 deletions(-) create mode 100644 .changeset/sys-file-copy-backfill-organization-stamping.md create mode 100644 packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts diff --git a/.changeset/sys-file-copy-backfill-organization-stamping.md b/.changeset/sys-file-copy-backfill-organization-stamping.md new file mode 100644 index 0000000000..78f87623f3 --- /dev/null +++ b/.changeset/sys-file-copy-backfill-organization-stamping.md @@ -0,0 +1,60 @@ +--- +"@objectstack/service-storage": patch +--- + +fix(service-storage): stamp the acting organization on the last two `sys_file` insert doors (#13547) + +`sys_file` declares no `tenancy` key, so `isTenancyDisabled()` reads `false` +and the registry provisions `organization_id` on it. Four doors on the object +had been given the acting organization one card at a time — `createFile` +(#12745), `createSession` (#12928), and the `update`/`delete` halves (#13178) — +and all four run through `StorageMetadataStore`, which threads a +`StorageWriteContext` into `context.tenantId` so the platform's insert-side +chokepoint can stamp the column. + +Two doors bypassed that store entirely and carried no organization at all: + +- `copyOwnedFile` (`file-reference-lifecycle.ts`) — the copy-on-claim + lifecycle hook, which inserts a fresh `sys_file` whenever a record writes an + id already owned by another field slot; +- `materializeDataUri` (`backfill-file-references.ts`) — the operator backfill + pass, which inserts one `sys_file` per inline `data:` URI it converts. + +Both passed `{ isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true }`, so +`buildDriverOptions` emitted no `DriverOptions.tenantId`, +`SqlDriver.injectTenantOnInsert` had nothing to stamp from, and every row +landed `organization_id = NULL`. The driver's tenant term is +`(organization_id = :tenantId OR organization_id IS NULL)`, so those rows were +reachable from **every** organization — including through the very update and +delete doors #13178 had just scoped. + +⚠️ Nothing warned, and the silence was explained rather than reassuring: +`isSystem` also sets `bypassTenantAudit = true`, which is exactly the guard +`auditMissingTenant` returns at — so the `[tenant-audit]` line naming this +defect ("writes will not be tenant-isolated") never fired for either door. + +Each door now threads the organization the platform can actually justify, as +an execution context — ⛔ never as a column on the payload, so +`resolveTenantField` / `injectTenantOnInsert` keep deciding whether the object +has a tenant column and whether an explicit value wins: + +- the **copy** takes the organization of the write that triggered it, read + from `HookContext.session.organizationId` (which ObjectQL's `buildSession()` + copies verbatim from `ExecutionContext.tenantId`); +- the **backfill** takes the organization of the record whose field held the + bytes, resolved with the same `createWallOrganizationResolver` the `sys_file` + organization sweep uses, so an object declaring `tenancy.tenantField` is read + by the column it is really walled by. + +Both stamp exactly what that sweep would independently derive from the new +file's field-reference holder, so the forward and repair halves agree by +construction. The backfill needs **no** operator-supplied organization and +deliberately takes none: one run spans every object and organization in the +deployment, so a single supplied value would be stamped onto other tenants' +files — and a wrongly-stamped row is walled into somebody else's tenant, which +is strictly worse than a NULL row that stays reachable. + +Where no organization is in scope — a caller with no active org, an unwalled +object, a legacy row that carries none — the `tenantId` key is omitted +entirely and the write proceeds exactly as before. ⛔ Forward-stamping only: +no existing `sys_file` row's organization is written by either door. diff --git a/packages/services/service-storage/src/backfill-file-references.ts b/packages/services/service-storage/src/backfill-file-references.ts index f8092b9602..dc7a074a38 100644 --- a/packages/services/service-storage/src/backfill-file-references.ts +++ b/packages/services/service-storage/src/backfill-file-references.ts @@ -4,6 +4,7 @@ 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 { keysetWalk } from '@objectstack/types'; +import { createWallOrganizationResolver } from './backfill-sys-file-organizations.js'; /** * Legacy file-value backfill (ADR-0104 D3 wave 2). @@ -156,12 +157,21 @@ function urlOf(value: unknown): string | null { return null; } -/** Upload a `data:` URI's bytes and register the `sys_file` row. */ +/** + * Upload a `data:` URI's bytes and register the `sys_file` row. + * + * `organizationId` is the organization of the RECORD whose field held these + * bytes, threaded as an execution context so the platform's insert-side + * chokepoint stamps `sys_file.organization_id` (#13547). See + * {@link backfillFileReferences} for why the subject record is the right — and + * the only honest — source for it. + */ async function materializeDataUri( engine: BackfillEngine, storage: IStorageService, dataUri: string, legacy: unknown, + organizationId: string | null, ): Promise { const m = DATA_URI_RE.exec(dataUri); if (!m) throw new Error('not a data: URI'); @@ -196,7 +206,12 @@ async function materializeDataUri( created_at: now, updated_at: now, }, - { context: { ...SYSTEM_CTX } }, + { + context: + organizationId != null + ? { ...SYSTEM_CTX, tenantId: organizationId } + : { ...SYSTEM_CTX }, + }, ); return newId; } @@ -204,6 +219,35 @@ async function materializeDataUri( /** * Scan legacy file values and convert what can be converted. * + * ## The organization a materialised `sys_file` is stamped with (#13547) + * + * This pass INSERTS `sys_file` rows — one per inline `data:` URI it + * materialises — and did so carrying `isSystem` and no organization, so every + * one of them landed `organization_id = NULL` on a tenancy-enabled object. + * That is the same defect as the `copyOwnedFile` door, arriving through an + * operator pass instead of a lifecycle hook. + * + * ⭐ It does NOT need an operator-supplied organization, and must not take + * one. `materializeDataUri` is reached only because a SPECIFIC record's field + * held those bytes, and the file it creates is claimed moments later — by the + * rewrite below and the claim hooks it wakes — for that same record's slot. So + * the record being converted already names the answer, and it is the answer + * the `sys_file` organization sweep would independently derive from the file's + * field-reference holder. The two agree by construction rather than by + * coincidence. + * + * ⛔ A single operator-supplied value would be WRONG for most rows: one run + * spans every object and every organization in the deployment, so any one + * organization it was handed would be stamped onto other tenants' files. ⚠️ A + * backfill that stamps the wrong organization is worse than one that stamps + * NULL — a NULL row stays reachable, while a mis-stamped row is walled into + * somebody else's tenant. Hence: derive per record, or stamp nothing. + * + * A record whose own organization column is NULL (a legacy row, or an unwalled + * object) yields nothing to thread and the new file stays unstamped, exactly + * as it did before. ⛔ Nothing here backfills the organization of any EXISTING + * `sys_file` row; forward-stamping only. + * * @param getStorage resolves the storage service; when absent, `data:` values * cannot be materialised and are reported `unresolvable` rather than failing * the run — a URL-only tenant still backfills fully. @@ -227,15 +271,35 @@ export async function backfillFileReferences( (name) => name !== 'sys_file' && fileFieldsOf(engine, name).length > 0, ); + // [#13547] "Which column is THIS object walled by?", asked of the registered + // schema rather than hard-coded to `organization_id` — the same resolver the + // `sys_file` organization sweep uses, so a subject declaring + // `tenancy.tenantField` is read by the column it is actually walled by and + // this pass cannot drift from that one. `getSchema` is the resolver's + // spelling of the lookup this module already does as `getObject`. + const wall = createWallOrganizationResolver({ + find: (object, options) => engine.find(object, (options ?? {}) as Record), + update: (object, data, options) => + engine.update(object, data as Record, (options ?? {}) as Record), + getSchema: (object: string) => engine.getObject(object), + }); + for (const object of scannedObjects) { const fileFields = fileFieldsOf(engine, object); + // Projected only when the subject really carries it: naming a column the + // object does not have would fail the scan for every row, and the whole + // pass with it. + const organizationField = wall.organizationFieldFor(object); + const scanFields = organizationField + ? ['id', ...fileFields, organizationField] + : ['id', ...fileFields]; // Seek by `id` (#4363). This walk WRITES to the rows it is reading — the // rewrite below updates each record in place — and an offset counts into a // set the writes are changing underneath it, so rows slide past the cursor // and are never converted. The key does not move when a row is updated, so // the seek is not affected by the very thing this function does. const walk = keysetWalk>( - (q) => engine.find(object, { ...q, fields: ['id', ...fileFields], context: { ...SYSTEM_CTX } }), + (q) => engine.find(object, { ...q, fields: scanFields, context: { ...SYSTEM_CTX } }), { pageSize: SCAN_PAGE_SIZE, max: maxPerObject }, ); @@ -245,6 +309,11 @@ export async function backfillFileReferences( const recordId = record?.id; if (recordId == null) continue; scannedRecords++; + // The organization of the record that HOLDS these bytes. Null on an + // unwalled object, and on a legacy row that itself carries none — in + // which case the new file stays unstamped rather than being invented + // into a tenant. + const recordOrganization = wall.organizationOf(object, record); for (const field of fileFields) { const raw = record[field]; @@ -319,7 +388,7 @@ export async function backfillFileReferences( continue; } try { - const newId = await materializeDataUri(engine, storage, url, value); + const newId = await materializeDataUri(engine, storage, url, value, recordOrganization); actions.push({ ...record_, kind: 'uploaded_inline_data', diff --git a/packages/services/service-storage/src/file-reference-lifecycle.ts b/packages/services/service-storage/src/file-reference-lifecycle.ts index 0fc2e40799..9850e2684b 100644 --- a/packages/services/service-storage/src/file-reference-lifecycle.ts +++ b/packages/services/service-storage/src/file-reference-lifecycle.ts @@ -83,6 +83,67 @@ const PACKAGE_ID = 'com.objectstack.service.storage'; // every internal page). Inert on write contexts. const SYSTEM_CTX = { isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true } as const; +/** + * The bookkeeping context for a `sys_file` write, carrying the acting + * organization when the triggering write had one (#13547). + * + * ## Why the organization has to travel with the copy + * + * `sys_file` declares no `tenancy` key, so `isTenancyDisabled()` reads `false` + * and the registry provisions `organization_id` on it. {@link copyOwnedFile} + * INSERTS a row on that object, and it did so carrying `isSystem` and nothing + * else — so `ObjectQLEngine.buildDriverOptions` emitted no + * `DriverOptions.tenantId`, `SqlDriver.injectTenantOnInsert` had no value to + * stamp from, and every copied file landed `organization_id = NULL`. The + * driver's tenant term is `(organization_id = :tenantId OR organization_id IS + * NULL)`, so those rows are reachable from every organization. + * + * ⚠️ Nothing warned. `isSystem` also sets `bypassTenantAudit = true`, which is + * exactly the guard `SqlDriver.auditMissingTenant` returns at — so the + * `[tenant-audit]` line that names this defect ("writes will not be + * tenant-isolated") never fired for it. The absence of the warning was + * explained, not reassuring. + * + * ## The same channel the four repaired doors use + * + * This mirrors `StorageMetadataStore`'s `StorageWriteContext` threading + * (`createFile` #12745, `createSession` #12928, the update/delete halves + * #13178) rather than inventing a second convention: the caller hands the + * engine the organization it is acting in as an execution context, and the + * platform's existing insert-side chokepoint decides the rest. ⛔ The + * organization is NOT written onto the payload here — whether this object has + * a tenant column at all, and whether an explicit value on the row wins, are + * `resolveTenantField` / `injectTenantOnInsert`'s answers, and restating them + * one package away from the schema is how the two answers drift apart. + * + * No organization ⇒ the key is absent entirely, and the write proceeds exactly + * as it did before. `tenantId: undefined` would NOT be the same thing: it is a + * key the context carries, and `buildDriverOptions` reads presence. + */ +function systemWriteContext(organizationId?: string | null): Record { + return typeof organizationId === 'string' && organizationId.length > 0 + ? { ...SYSTEM_CTX, tenantId: organizationId } + : { ...SYSTEM_CTX }; +} + +/** + * The organization the write that triggered this hook is acting in, or `null`. + * + * `HookContext.session.organizationId` is the blessed developer-facing name + * for the caller's active org, and ObjectQL's `buildSession()` copies it + * verbatim from `ExecutionContext.tenantId` — the same value that would have + * reached the driver had the caller's own write been the one inserting. So the + * copy is stamped for the organization whose record triggered it, which is + * also the organization the `sys_file` organization sweep derives from a + * file's field-reference holder. ⛔ Never a lookup and never a default: a + * caller with no active organization yields `null` and the insert stays + * unstamped rather than being guessed into somebody's tenant. + */ +function actingOrganizationOf(ctx: any): string | null { + const org = ctx?.session?.organizationId; + return typeof org === 'string' && org.length > 0 ? org : null; +} + /** Bound on owned files released per record delete. */ const RELEASE_BATCH_LIMIT = 1_000; @@ -341,6 +402,7 @@ async function copyOwnedFile( engine: FileReferenceEngine, storage: IStorageService, src: Record, + organizationId: string | null, ): Promise { const srcKey = typeof src.key === 'string' ? src.key : ''; if (!srcKey) throw new Error('source file has no storage key'); @@ -360,6 +422,12 @@ async function copyOwnedFile( const now = new Date().toISOString(); // Ownership columns are deliberately left NULL — the after-hook claims the // copy for the slot that triggered it, on the same path as any other file. + // + // [#13547] The TENANT column is not one of them, and the after-hook does not + // claim it: `claimFile` patches `ref_object` / `ref_id` / `ref_field` (and + // `status` / `deleted_at` on a revive) and never names `organization_id`. + // So the acting organization has to be threaded HERE, on the insert, which + // is the only point that can still stamp it. await engine.insert( 'sys_file', { @@ -377,7 +445,7 @@ async function copyOwnedFile( created_at: now, updated_at: now, }, - { context: { ...SYSTEM_CTX } }, + { context: systemWriteContext(organizationId) }, ); return newId; } @@ -405,6 +473,7 @@ async function applyCopyOnClaim( recordId: string | null, data: Record, fileFields: string[], + organizationId: string | null, ): Promise { for (const field of fileFields) { if (!(field in data)) continue; @@ -447,7 +516,7 @@ async function applyCopyOnClaim( continue; } try { - replacements.set(token, await copyOwnedFile(engine, storage, row)); + replacements.set(token, await copyOwnedFile(engine, storage, row, organizationId)); logger.debug?.( `[storage] file reference: copied ${token} for ${object}.${field} (exclusive ownership)`, ); @@ -619,7 +688,7 @@ export function installFileReferenceHooks( if (!object || !data || typeof data !== 'object') return; const fileFields = activeFileFields(engine, object); if (fileFields.length === 0) return; - await applyCopyOnClaim(engine, getStorage, logger, object, null, data, fileFields); + await applyCopyOnClaim(engine, getStorage, logger, object, null, data, fileFields, actingOrganizationOf(ctx)); }, { packageId: PACKAGE_ID }, ); @@ -673,7 +742,7 @@ export function installFileReferenceHooks( // copy-on-claim pass is what makes "the before hook always reconciles the // payload it is given" a property of this handler instead of a case // analysis a later edit has to re-derive. - await applyCopyOnClaim(engine, getStorage, logger, object, recordId, data, fileFields); + await applyCopyOnClaim(engine, getStorage, logger, object, recordId, data, fileFields, actingOrganizationOf(ctx)); }, { packageId: PACKAGE_ID }, ); 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 new file mode 100644 index 0000000000..f4f0e6f476 --- /dev/null +++ b/packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts @@ -0,0 +1,362 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #13547 — the FIFTH and SIXTH `sys_file` insert doors. +// +// Four doors on this object have been given the acting organization one card +// at a time — `createFile` (#12745), `createSession` (#12928), and the +// `update`/`delete` halves (#13178) — and all four run through +// `StorageMetadataStore`, which threads a `StorageWriteContext` into +// `context.tenantId` so the platform's insert-side chokepoint can stamp the +// column. These two bypass that store entirely: +// +// file-reference-lifecycle.ts copyOwnedFile an engine lifecycle hook +// backfill-file-references.ts materializeDataUri an operator pass +// +// Both passed `{ isSystem: true, [RAW_FILE_VALUES_CONTEXT_KEY]: true }` and no +// tenant, so `buildDriverOptions` emitted no `DriverOptions.tenantId`, +// `injectTenantOnInsert` stamped nothing, and each row landed +// `organization_id = NULL` — reachable from every organization through the +// driver's `(organization_id = :tenantId OR organization_id IS NULL)` term. +// +// ⚠️ These assert the CONTEXT the engine is handed, not a column on the +// payload. Whether `sys_file` has a tenant column at all, and whether an +// explicit value wins, are `resolveTenantField` / `injectTenantOnInsert`'s +// answers; re-deciding them here would be the second convention this card is +// about. + +import { describe, it, expect, vi } from 'vitest'; +import { RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; +import { assertEngineFindOnePredicate } from '@objectstack/objectql'; +import { installFileReferenceHooks } from './file-reference-lifecycle.js'; +import { backfillFileReferences } from './backfill-file-references.js'; + +const silentLogger = () => ({ info: vi.fn(), warn: vi.fn(), debug: vi.fn() }); + +const fakeStorage = () => + ({ + upload: vi.fn(async () => {}), + download: vi.fn(async () => Buffer.from('the-bytes')), + delete: vi.fn(async () => {}), + exists: vi.fn(async () => true), + getInfo: vi.fn(async () => ({ key: 'k', size: 9, contentType: 'image/png', lastModified: new Date() })), + }) as any; + +/** The `sys_file` insert this card is about, with the options bag intact. */ +type Insert = { object: string; data: Record; options: any }; + +const sysFileInsertOf = (inserts: Insert[]): Insert => { + const hit = inserts.filter((i) => i.object === 'sys_file'); + expect(hit).toHaveLength(1); + return hit[0]; +}; + +// --------------------------------------------------------------------------- +// Door 5 — `copyOwnedFile`, reached through the copy-on-claim before-hook +// --------------------------------------------------------------------------- + +const LIFECYCLE_REGISTRY: Record = { + sys_file: { + name: 'sys_file', + fields: { id: { type: 'text' }, key: { type: 'text' }, organization_id: { type: 'text' } }, + }, + product: { + name: 'product', + fields: { id: { type: 'text' }, name: { type: 'text' }, image: { type: 'image' } }, + }, +}; + +function lifecycleEngine(files: Array>) { + const inserts: Insert[] = []; + const updates: Array<{ data: Record; options: any }> = []; + const tables: Record>> = { sys_file: [...files], product: [] }; + const hooks = new Map Promise | void>>(); + + const engine: any = { + registerHook(event: string, handler: any) { + const list = hooks.get(event) ?? []; + list.push(handler); + hooks.set(event, list); + }, + getObject: (name: string) => LIFECYCLE_REGISTRY[name], + async find(object: string, options: any) { + return (tables[object] ?? []).filter((r) => + Object.entries(options?.where ?? {}).every(([k, v]) => + v && typeof v === 'object' && Array.isArray((v as any).$in) + ? (v as any).$in.some((x: unknown) => String(x) === String(r[k])) + : r[k] === v, + ), + ); + }, + async findOne(object: string, options: any) { + assertEngineFindOnePredicate(object, options); + return (tables[object] ?? []).find((r) => String(r.id) === String(options?.where?.id)) ?? null; + }, + async insert(object: string, data: any, options?: any) { + inserts.push({ object, data: { ...data }, options }); + (tables[object] ??= []).push({ ...data }); + return data; + }, + async update(object: string, data: any, options?: any) { + if (object === 'sys_file') updates.push({ data: { ...data }, options }); + const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id)); + if (row) Object.assign(row, data); + return row; + }, + inserts, + updates, + tables, + async trigger(event: string, ctx: any) { + for (const h of hooks.get(event) ?? []) await h(ctx); + }, + }; + return engine; +} + +/** + * 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. + */ +async function driveCopyingInsert(engine: any, session: unknown) { + const data: Record = { image: 'file_owned' }; + const ctx: any = { + object: 'product', + event: 'beforeInsert', + input: { data }, + session, + dispatch: { mode: 'record', index: 0, scope: {} }, + }; + await engine.trigger('beforeInsert', ctx); + const row = { ...(ctx.input.data as Record), id: 'p1' }; + engine.tables.product.push(row); + ctx.event = 'afterInsert'; + ctx.result = row; + await engine.trigger('afterInsert', ctx); + return row; +} + +const ownedFile = () => ({ + id: 'file_owned', + key: 'user/file_owned.png', + name: 'owned.png', + status: 'committed', + ref_object: 'other', + ref_id: 'r9', + ref_field: 'image', +}); + +describe('#13547 door 5 — copyOwnedFile threads the triggering write’s organization', () => { + it('hands the engine `context.tenantId` for the organization the write acts in', async () => { + const engine = lifecycleEngine([ownedFile()]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, { userId: 'u1', organizationId: 'org_A' }); + + const insert = sysFileInsertOf(engine.inserts); + expect(insert.options?.context?.tenantId).toBe('org_A'); + // The bookkeeping markers the copy has always carried are untouched. + expect(insert.options?.context?.isSystem).toBe(true); + expect(insert.options?.context?.[RAW_FILE_VALUES_CONTEXT_KEY]).toBe(true); + }); + + it('⛔ never puts the organization on the PAYLOAD — the driver decides the column', async () => { + const engine = lifecycleEngine([ownedFile()]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, { organizationId: 'org_A' }); + + expect(sysFileInsertOf(engine.inserts).data).not.toHaveProperty('organization_id'); + }); + + it('omits `tenantId` ENTIRELY when the caller has no active organization', async () => { + const engine = lifecycleEngine([ownedFile()]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, { userId: 'u1' }); + + const context = sysFileInsertOf(engine.inserts).options?.context ?? {}; + // Presence, not value: `buildDriverOptions` reads `tenantId !== undefined`, + // so `tenantId: undefined` is NOT the same as an absent key. + expect(Object.prototype.hasOwnProperty.call(context, 'tenantId')).toBe(false); + expect(context.isSystem).toBe(true); + }); + + it('omits it for a caller with no session at all (the pre-repair call shape)', async () => { + const engine = lifecycleEngine([ownedFile()]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, undefined); + + const context = sysFileInsertOf(engine.inserts).options?.context ?? {}; + expect(Object.prototype.hasOwnProperty.call(context, 'tenantId')).toBe(false); + }); + + it('⛔ never derives the organization from the SOURCE file it is copying', async () => { + // The source row is stamped for another organization. The copy belongs to + // the slot that triggered it, not to whoever owned the bytes — deriving + // from the source would wall the copy into a tenant that is not writing. + const engine = lifecycleEngine([{ ...ownedFile(), organization_id: 'org_SOURCE' }]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, { organizationId: 'org_A' }); + + expect(sysFileInsertOf(engine.inserts).options?.context?.tenantId).toBe('org_A'); + }); + + it('pins the measurement the repair rests on: the after-hook claims ownership, NEVER the tenant', async () => { + // `copyOwnedFile` documents that it leaves ownership columns NULL because + // "the after-hook claims the copy for the slot that triggered it". That is + // true of `ref_*` and ONLY of `ref_*` — so the insert is the last point + // that can stamp the tenant, which is why the repair belongs there. + const engine = lifecycleEngine([ownedFile()]); + installFileReferenceHooks(engine, () => fakeStorage(), silentLogger()); + + await driveCopyingInsert(engine, { organizationId: 'org_A' }); + + expect(engine.updates.length).toBeGreaterThan(0); + for (const { data } of engine.updates) { + expect(data).not.toHaveProperty('organization_id'); + } + // …and it does claim the ownership columns, so the double is really + // exercising the claim path rather than passing because nothing ran. + expect(engine.updates.some((u: any) => u.data.ref_object === 'product')).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// Door 6 — the backfill pass materialising inline `data:` bytes +// --------------------------------------------------------------------------- + +const DATA_URI = 'data:image/png;base64,aGVsbG8='; + +/** `product` is walled; `memo` carries no organization column at all. */ +const BACKFILL_REGISTRY: Record = { + sys_file: { fields: { id: { type: 'text' }, organization_id: { type: 'text' } } }, + product: { + fields: { + id: { type: 'text' }, + image: { type: 'image' }, + organization_id: { type: 'text' }, + }, + }, + memo: { fields: { id: { type: 'text' }, image: { type: 'image' } } }, +}; + +function backfillEngine(tables: Record>>) { + const inserts: Insert[] = []; + const projections: Array<{ object: string; fields: unknown }> = []; + const engine: any = { + getObject: (name: string) => BACKFILL_REGISTRY[name], + getConfigs: () => BACKFILL_REGISTRY, + async find(object: string, options: any) { + projections.push({ object, fields: options?.fields }); + const key = options?.orderBy?.[0]?.field; + const seek = (options?.where as any)?.[key]?.$gt; + let rows = tables[object] ?? []; + if (seek !== undefined) rows = rows.filter((r) => String(r[key]) > String(seek)); + const ordered = key ? [...rows].sort((a, b) => String(a[key]).localeCompare(String(b[key]))) : rows; + return typeof options?.limit === 'number' ? ordered.slice(0, options.limit) : ordered; + }, + async insert(object: string, data: any, options?: any) { + inserts.push({ object, data: { ...data }, options }); + (tables[object] ??= []).push({ ...data }); + return data; + }, + async update(object: string, data: any) { + const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id)); + if (row) Object.assign(row, data); + return row; + }, + inserts, + projections, + tables, + }; + return engine; +} + +const runBackfill = (engine: any) => + backfillFileReferences(engine, () => fakeStorage(), silentLogger(), { apply: true }); + +describe('#13547 door 6 — the backfill stamps from the record that HELD the bytes', () => { + it('threads the subject record’s organization onto the materialised sys_file', async () => { + const engine = backfillEngine({ + product: [{ id: 'p1', image: DATA_URI, organization_id: 'org_B' }], + sys_file: [], + }); + + await runBackfill(engine); + + const insert = sysFileInsertOf(engine.inserts); + expect(insert.options?.context?.tenantId).toBe('org_B'); + expect(insert.options?.context?.isSystem).toBe(true); + expect(insert.data).not.toHaveProperty('organization_id'); + }); + + it('projects the organization column so the value is actually in reach', async () => { + const engine = backfillEngine({ + product: [{ id: 'p1', image: DATA_URI, organization_id: 'org_B' }], + sys_file: [], + }); + + await runBackfill(engine); + + const scan = engine.projections.find((p: any) => p.object === 'product'); + expect(scan?.fields).toContain('organization_id'); + expect(scan?.fields).toContain('image'); + }); + + it('stamps NOTHING when the subject row carries no organization — ⛔ never invents one', async () => { + const engine = backfillEngine({ + product: [{ id: 'p1', image: DATA_URI, organization_id: null }], + sys_file: [], + }); + + await runBackfill(engine); + + const context = sysFileInsertOf(engine.inserts).options?.context ?? {}; + expect(Object.prototype.hasOwnProperty.call(context, 'tenantId')).toBe(false); + }); + + it('leaves an object with no organization column unprojected and unstamped', async () => { + const engine = backfillEngine({ memo: [{ id: 'm1', image: DATA_URI }], sys_file: [] }); + + await runBackfill(engine); + + const scan = engine.projections.find((p: any) => p.object === 'memo'); + // Naming a column the object does not have would fail the whole scan. + expect(scan?.fields).not.toContain('organization_id'); + const context = sysFileInsertOf(engine.inserts).options?.context ?? {}; + expect(Object.prototype.hasOwnProperty.call(context, 'tenantId')).toBe(false); + }); + + it('keeps each row on its OWN organization across a multi-tenant scan', async () => { + // The reason an operator-supplied single organization would be wrong: one + // run spans every tenant in the deployment. + const engine = backfillEngine({ + product: [ + { id: 'p1', image: DATA_URI, organization_id: 'org_B' }, + { id: 'p2', image: DATA_URI, organization_id: 'org_C' }, + ], + sys_file: [], + }); + + await runBackfill(engine); + + const stamped = engine.inserts + .filter((i: Insert) => i.object === 'sys_file') + .map((i: Insert) => i.options?.context?.tenantId); + expect(stamped).toEqual(['org_B', 'org_C']); + }); + + it('⛔ writes no organization onto any EXISTING sys_file row — forward-stamping only', async () => { + const legacy = { id: 'file_legacy', key: 'user/legacy.png', organization_id: null }; + const engine = backfillEngine({ + product: [{ id: 'p1', image: DATA_URI, organization_id: 'org_B' }], + sys_file: [legacy], + }); + + await runBackfill(engine); + + expect(engine.tables.sys_file.find((r: any) => r.id === 'file_legacy')).toEqual(legacy); + }); +}); From 38b13f12a399f5d1a5f84938db4e66e5054be26a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 01:30:58 +0000 Subject: [PATCH 2/2] test(service-storage): conform the new sys_file stamping doubles to the engine ledgers The pins added for the two repaired insert doors introduced two fake engines. Route their `update()` through `assertEngineUpdateDispatch` so the doubles cannot accept a call shape the real `ObjectQL.update` refuses, make the lifecycle double refuse the WHERE combinators it does not implement instead of reading one as a field name, and apply the caller's `limit` after the filter. Register the new pinned coverage in the engine-double ledger (added rows only, no losses). Part of #13547 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- ...opy-backfill-organization-stamping.test.ts | 22 +++++++++++++------ scripts/engine-double-contract.pinned.json | 10 +++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) 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 f4f0e6f476..a96595a1ac 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 @@ -26,7 +26,7 @@ import { describe, it, expect, vi } from 'vitest'; import { RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; -import { assertEngineFindOnePredicate } from '@objectstack/objectql'; +import { assertEngineFindOnePredicate, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { installFileReferenceHooks } from './file-reference-lifecycle.js'; import { backfillFileReferences } from './backfill-file-references.js'; @@ -79,13 +79,19 @@ function lifecycleEngine(files: Array>) { }, getObject: (name: string) => LIFECYCLE_REGISTRY[name], async find(object: string, options: any) { - return (tables[object] ?? []).filter((r) => - Object.entries(options?.where ?? {}).every(([k, v]) => - v && typeof v === 'object' && Array.isArray((v as any).$in) + const rows = (tables[object] ?? []).filter((r) => + Object.entries(options?.where ?? {}).every(([k, v]) => { + // Refuse the combinators this double does not implement rather than + // reading one as a field name — a silently-wrong matcher passes by + // matching nothing. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return v && typeof v === 'object' && Array.isArray((v as any).$in) ? (v as any).$in.some((x: unknown) => String(x) === String(r[k])) - : r[k] === v, - ), + : r[k] === v; + }), ); + // The caller's bound, applied AFTER the filter and by presence. + return typeof options?.limit === 'number' ? rows.slice(0, options.limit) : rows; }, async findOne(object: string, options: any) { assertEngineFindOnePredicate(object, options); @@ -97,6 +103,7 @@ function lifecycleEngine(files: Array>) { return data; }, async update(object: string, data: any, options?: any) { + assertEngineUpdateDispatch(data, options); if (object === 'sys_file') updates.push({ data: { ...data }, options }); const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id)); if (row) Object.assign(row, data); @@ -262,7 +269,8 @@ function backfillEngine(tables: Record>>) (tables[object] ??= []).push({ ...data }); return data; }, - async update(object: string, data: any) { + async update(object: string, data: any, options?: any) { + assertEngineUpdateDispatch(data, options); const row = (tables[object] ?? []).find((r) => String(r.id) === String(data.id)); if (row) Object.assign(row, data); return row; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 3056333405..ecff2f5e59 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -3231,6 +3231,16 @@ "verb": "delete", "pinned": 1 }, + { + "file": "packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/services/service-storage/src/sys-file-copy-backfill-organization-stamping.test.ts", + "verb": "update", + "pinned": 2 + }, { "file": "packages/services/service-storage/src/sys-file-organization-stamping.test.ts", "verb": "delete",