fix(service-storage): stamp the acting organization on the last two sys_file insert doors

.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.

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<string> {
  const m = DATA_URI_RE.exec(dataUri);
  if (!m) throw new Error('not a data: URI');
@@ -196,14 +206,48 @@ async function materializeDataUri(
  created_at: now,
  updated_at: now,
  },
- { context: { ...SYSTEM_CTX } },
+ {
+ context:
+ organizationId != null
+ ? { ...SYSTEM_CTX, tenantId: organizationId }
+ : { ...SYSTEM_CTX },
+ },
  );
  return newId;
 }
 
 /**
  * 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<string, unknown>),
+ update: (object, data, options) =>
+ engine.update(object, data as Record<string, unknown>, (options ?? {}) as Record<string, unknown>),
+ 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<Record<string, unknown>>(
- (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',

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<string, unknown> {
+ 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<string, unknown>,
+ organizationId: string | null,
 ): Promise<string> {
  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<string, unknown>,
  fileFields: string[],
+ organizationId: string | null,
 ): Promise<void> {
  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 },
  );