fix(storage): let an object delegate file-read authorization to its service (#3580)

Fixes a regression from the governed-download change (ADR-0104 D3 wave 2, #3534),
found by driving app-showcase in a browser as a real non-admin approver: a
LEGITIMATE APPROVER COULD SEE A DECISION ATTACHMENT'S FILENAME BUT GOT 403
OPENING IT.

The gate authorized a field-owned file by testing whether the caller can READ
the owning row. For an ordinary business object that is right — row readability
IS the access rule. For sys_approval_action it asks the wrong authority: the
audit table is deliberately closed to ordinary approver positions ("operation
'find' on object 'sys_approval_action' is not permitted for positions [auditor,
everyone]"), so the test denied exactly the person the attachment was filed for.
The approvals service has always held the real rule — which is why the timeline
listing the attachment returned 200 while the bytes returned 403.

An object may now name a service to answer instead:

  - ObjectSchema.fileAccessDelegate — the kernel service that authorizes
    downloads of files owned by this object's media fields
  - IFileAccessDelegate.authorizeFileRead(recordId, context) — the contract
  - sys_approval_action declares 'approvals'; ApprovalService.authorizeFileRead
    reuses the SAME gate listActions applies (visibility of the parent request)
    rather than inventing a second, looser rule for the bytes

Fails closed: a declared delegate that is missing or does not implement the
method denies, rather than silently reverting to the raw read it was declared to
replace. Objects that declare nothing are unchanged.

Verified in the browser on both sides of the gate, not just the happy one. The
approver now downloads the real PDF (200, %PDF-1.4 bytes); an anonymous request
is still refused (401), so the anonymous capability URL #3534 set out to close
stays closed. Checking the permissive side mattered: the delegate is exactly as
open as listActions and no more, because approval history is tenant-scoped by
design. The net invariant is that a decision attachment is exactly as readable
as the decision it hangs off — never more, and no longer less.


Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.changeset/file-access-delegate.md

@@ -0,0 +1,40 @@
+---
+"@objectstack/spec": minor
+"@objectstack/service-storage": patch
+"@objectstack/plugin-approvals": patch
+---
+
+fix(storage): let an object delegate file-read authorization to its service
+
+Fixes a regression from the governed-download change (ADR-0104 D3 wave 2): a
+**legitimate approver could see a decision attachment's filename but got 403
+opening it**, found by driving app-showcase in a browser as a real non-admin
+approver.
+
+Cause: a field-owned file's download was authorized by testing whether the
+caller can READ the owning row. For an ordinary business object that is right —
+row readability *is* the access rule. For `sys_approval_action` it is the wrong
+authority: the audit table is deliberately closed to ordinary approver
+positions (`operation 'find' … is not permitted for positions [auditor,
+everyone]`), so the test denied the very approver the attachment was filed for.
+The approvals *service* has always had the real rule, which is why the timeline
+listing the attachment returned 200 while the bytes returned 403.
+
+An object may now name a service to answer the question instead:
+
+- `ObjectSchema.fileAccessDelegate` — a kernel service that authorizes
+  downloads of files owned by that object's media fields.
+- `IFileAccessDelegate.authorizeFileRead(recordId, context)` — the contract.
+- `sys_approval_action` declares `'approvals'`; `ApprovalService.authorizeFileRead`
+  reuses the *same* gate `listActions` applies (visibility of the parent
+  request) rather than inventing a second, looser rule for the bytes.
+
+**Fails closed**: a declared delegate that is missing or does not implement the
+method denies, rather than silently reverting to the raw read it was declared to
+replace. Objects without the declaration are unchanged.
+
+Verified in the browser against app-showcase, both sides of the gate: the
+approver now downloads the real PDF (200), and an anonymous request is still
+refused (401) — the anonymous capability URL the original change closed stays
+closed. A decision attachment ends up exactly as readable as the decision it
+hangs off: never more, and no longer less.

content/docs/references/data/object.mdx

@@ -128,6 +128,7 @@ const result = ApiMethod.parse(data);
 | **access** | `{ default?: Enum<'public' \| 'private'> }` | optional | [ADR-0066 D2] Object exposure posture (public-by-default vs private secure-by-default). |
 | **requiredPermissions** | `string[] \| { read?: string[]; create?: string[]; update?: string[]; delete?: string[] }` | optional | [ADR-0066 D3/⑤] Capabilities required to access this object (AND-gate) — `string[]` gates all CRUD, or a `{read,create,update,delete}` map gates per operation. |
 | **lifecycle** | `{ class: Enum<'record' \| 'audit' \| 'telemetry' \| 'transient' \| 'event'>; retention?: object; ttl?: object; storage?: object; … }` | optional | Data lifecycle contract (ADR-0057): class + retention/ttl/rotation/archive policies enforced by the platform LifecycleService. |
+| **fileAccessDelegate** | `string` | optional | Kernel service that authorizes downloads of files owned by this object's media fields, instead of testing whether the caller can read the owning row. For objects whose access is mediated by a service (e.g. sys_approval_action → approvals). Fails closed. |
 | **validations** | `any[]` | optional | Object-level validation rules |
 | **activityMilestones** | `{ field: string; value: string; summary: string; type?: string }[]` | optional | Declarative semantic activity milestones — emit a templated timeline row when a field transitions into a value, no hook code (ADR-0052 §5b.2). |
 | **nameField** | `string` | optional | [ADR-0079] Canonical primary title field — the stored field used as the record display name (e.g. "name", "title"). |

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -9,7 +9,7 @@
  * the read API, and the global record-lock hook.
  */
 
-import { describe, it, expect, beforeEach } from 'vitest';
+import { describe, it, expect, beforeEach, vi } from 'vitest';
 import { ApprovalService, REMIND_COOLDOWN_MS } from './approval-service.js';
 import { bindApprovalLockHook, bindDelegationWriteGuard, unbindAllHooks } from './lifecycle-hooks.js';
 
@@ -2005,3 +2005,69 @@ describe('ApprovalService — queue approver is unresolved (#3508)', () => {
     expect(warnings.some(([msg]) => String(msg).includes("'queue'") && String(msg).includes('#3508'))).toBe(true);
   });
 });
+
+// ── File-access delegate (ADR-0104 D3 wave 2) ────────────────────────
+//
+// A decision attachment is OWNED by its `sys_approval_action` row, so the
+// storage service would otherwise authorize the download by testing whether
+// the caller can READ that row. It cannot — the table is closed to ordinary
+// approver positions — which denied the very approver the attachment was filed
+// for (reproduced in the browser against app-showcase). `sys_approval_action`
+// therefore declares `fileAccessDelegate: 'approvals'` and the service answers,
+// reusing the rule that already governs seeing a decision: visibility of the
+// PARENT REQUEST, exactly as listActions applies it.
+describe('ApprovalService — authorizeFileRead delegate (ADR-0104 D3 wave 2)', () => {
+  const svcFor = (engine: any) => {
+    let n = 0;
+    return new ApprovalService({
+      engine,
+      clock: { now: () => new Date(1757000000000 + (n++) * 1000) },
+    });
+  };
+
+  const seedDecision = async (engine: any) => {
+    const svc = svcFor(engine);
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9', attachments: ['file_a'] }, SYS);
+    const action = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
+    return { svc, req, actionId: String(action.id) };
+  };
+
+  it('allows a caller who can see the parent request', async () => {
+    const engine = makeFakeEngine();
+    const { svc, actionId } = await seedDecision(engine);
+
+    expect(await svc.authorizeFileRead(actionId, SYS)).toBe(true);
+  });
+
+  it('denies a caller who cannot see the parent request', async () => {
+    const engine = makeFakeEngine();
+    const { svc, actionId } = await seedDecision(engine);
+    // getRequest is the single gate this delegates to — when it yields nothing
+    // for this caller, the bytes must be refused too.
+    vi.spyOn(svc as any, 'getRequest').mockResolvedValue(null);
+
+    expect(await svc.authorizeFileRead(actionId, CTX)).toBe(false);
+  });
+
+  it('denies an unknown action id', async () => {
+    const engine = makeFakeEngine();
+    const { svc } = await seedDecision(engine);
+
+    expect(await svc.authorizeFileRead('aact_does_not_exist', SYS)).toBe(false);
+    expect(await svc.authorizeFileRead('', SYS)).toBe(false);
+  });
+
+  it('fails CLOSED when the lookup throws', async () => {
+    const engine = makeFakeEngine();
+    const { svc, actionId } = await seedDecision(engine);
+    vi.spyOn(engine as any, 'find').mockRejectedValue(new Error('driver down'));
+
+    expect(await svc.authorizeFileRead(actionId, SYS)).toBe(false);
+  });
+
+  it('sys_approval_action declares the delegate, so the storage gate asks the service', async () => {
+    const { SysApprovalAction } = await import('./sys-approval-action.object.js');
+    expect((SysApprovalAction as any).fileAccessDelegate).toBe('approvals');
+  });
+});

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -2887,4 +2887,36 @@ export class ApprovalService implements IApprovalService {
     }
     return actions;
   }
+
+  /**
+   * `IFileAccessDelegate` — may this caller download a decision attachment?
+   * (ADR-0104 D3 wave 2; declared by `sys_approval_action.fileAccessDelegate`.)
+   *
+   * A file referenced by `sys_approval_action.attachments` is owned by that
+   * audit row, so the storage service would otherwise authorize the download by
+   * testing whether the caller can READ the row. It cannot: `sys_approval_action`
+   * is deliberately closed to ordinary approver positions, so that test denies
+   * the very approver the attachment was filed for.
+   *
+   * The rule that actually governs seeing a decision is the one `listActions`
+   * applies — can the caller see the PARENT REQUEST? — so this reuses it
+   * exactly, rather than inventing a second, looser rule for the bytes. Fails
+   * closed on any error.
+   */
+  async authorizeFileRead(actionId: string, context: SharingExecutionContext): Promise<boolean> {
+    if (!actionId) return false;
+    try {
+      const rows = await this.engine.find('sys_approval_action', {
+        where: { id: actionId },
+        limit: 1,
+        context: SYSTEM_CTX,
+      });
+      const requestId = (Array.isArray(rows) ? rows[0] : undefined)?.request_id;
+      if (!requestId) return false;
+      // Same gate as listActions: visibility of the decision's parent request.
+      return !!(await this.getRequest(String(requestId), context));
+    } catch {
+      return false;
+    }
+  }
 }

packages/plugins/plugin-approvals/src/sys-approval-action.object.ts

@@ -26,6 +26,15 @@ export const SysApprovalAction = ObjectSchema.create({
   titleFormat: '{action} · {step_name}',
   highlightFields: ['request_id', 'step_name', 'action', 'actor_id', 'created_at'],
 
+  // ADR-0104 D3 wave 2. `attachments` is a media field, so the files it holds
+  // are OWNED by this row — and the storage service would otherwise authorize
+  // their download by testing whether the caller can READ this row. It cannot:
+  // this table is deliberately closed to ordinary approver positions, so that
+  // test denies the very approver the attachment was filed for. The approvals
+  // service already owns the rule for seeing a decision (visibility of the
+  // parent request, exactly as `listActions` applies it), so it answers.
+  fileAccessDelegate: 'approvals',
+
   listViews: {
     recent: {
       type: 'grid',

packages/services/service-storage/src/storage-service-plugin.ts

@@ -2,7 +2,12 @@
 
 import type { Plugin, PluginContext } from '@objectstack/core';
 import { resolveAuthzContext } from '@objectstack/core';
-import type { IHttpServer, IDataEngine, IStorageService } from '@objectstack/spec/contracts';
+import type {
+  IHttpServer,
+  IDataEngine,
+  IStorageService,
+  IFileAccessDelegate,
+} from '@objectstack/spec/contracts';
 import {
   OBSERVABILITY_METRICS_SERVICE,
   NoopMetricsRegistry,
@@ -498,8 +503,29 @@ function buildFileReadAuthorizer(
       // Field-owned (ADR-0104 D3 wave 2): exactly ONE record's field holds
       // this file, so access is that record's READ access — never a union.
       if (file.ref_object && file.ref_id != null && file.ref_id !== '') {
+        const ownerObject = String(file.ref_object);
+        const ownerId = String(file.ref_id);
+
+        // An object whose access is MEDIATED BY A SERVICE rather than by row
+        // permissions names that service in `fileAccessDelegate`. Asking the
+        // row directly would be asking the wrong authority: `sys_approval_action`
+        // is deliberately unreadable to ordinary approver positions, so a raw
+        // read denies the very approver the attachment is for. Fails closed —
+        // a declared delegate that is missing or incomplete denies rather than
+        // silently reverting to the raw read it was declared to replace.
+        const delegateName = (engine as any).getObject?.(ownerObject)?.fileAccessDelegate;
+        if (typeof delegateName === 'string' && delegateName) {
+          try {
+            const delegate = ctx.getService<IFileAccessDelegate>(delegateName);
+            if (!delegate || typeof delegate.authorizeFileRead !== 'function') return 'deny';
+            return (await delegate.authorizeFileRead(ownerId, authz)) ? 'allow' : 'deny';
+          } catch {
+            return 'deny';
+          }
+        }
+
         try {
-          const visible = (await (engine as any).find(String(file.ref_object), {
+          const visible = (await (engine as any).find(ownerObject, {
             where: { id: file.ref_id },
             fields: ['id'],
             limit: 1,

packages/spec/api-surface.json

@@ -3515,6 +3515,7 @@
     "IEmbedder (interface)",
     "IExportService (interface)",
     "IExternalDatasourceService (interface)",
+    "IFileAccessDelegate (interface)",
     "IHierarchyScopeResolver (interface)",
     "IHttpRequest (interface)",
     "IHttpResponse (interface)",

packages/spec/liveness/object.json

@@ -197,6 +197,11 @@
       "status": "planned",
       "authorWarn": true,
       "note": "[ADR-0090 D11] P1 lands the SPEC SHAPE only (validated external<=internal at authoring; Studio surfaces the Ext badge). Runtime consumption (audience-aware evaluator branch substituting the external dial for external principals) is scheduled with the principal-taxonomy semantics phase; tracked on #2696. `planned` + authorWarn per enforce-or-mark: authors get told the dial does not evaluate yet. Was a bespoke 'authorable' status outside the documented vocabulary."
+    },
+    "fileAccessDelegate": {
+      "status": "live",
+      "evidence": "packages/services/service-storage/src/storage-service-plugin.ts (buildFileReadAuthorizer)",
+      "note": "ADR-0104 D3 wave 2. Named service answers 'may this caller download a file owned by this object's media fields?' instead of the storage service testing raw readability of the owning row — which asks the wrong authority for an object whose access is mediated by a service. sys_approval_action declares 'approvals'; the service reuses the same parent-request visibility gate listActions applies. Fails closed: a declared-but-missing delegate denies."
     }
   }
 }

packages/spec/src/contracts/storage-service.ts

@@ -192,3 +192,28 @@ export interface IStorageService {
      */
     abortChunkedUpload?(uploadId: string): Promise<void>;
 }
+
+/**
+ * A kernel service that answers file-read authorization on behalf of an object
+ * (ADR-0104 D3 wave 2). Named by that object's `fileAccessDelegate`.
+ *
+ * Exists because "can the caller READ the owning row?" — the storage service's
+ * default question — is the wrong question for an object whose access is
+ * mediated by a service rather than by row permissions. `sys_approval_action`
+ * is the motivating case: it is deliberately unreadable to ordinary approver
+ * positions, yet an approver must still be able to open a decision attachment.
+ * The approvals service already knows who may see a request's history, so it
+ * answers instead.
+ *
+ * Implementations should apply the SAME rule that governs reading the record
+ * through their own API — not a looser one. The delegate widens who can reach
+ * the bytes, so a permissive implementation is a data leak.
+ */
+export interface IFileAccessDelegate {
+    /**
+     * May this caller download a file owned by `recordId` on the delegating
+     * object? Return `false` (never throw) to deny; a throw is treated as a
+     * denial too, since authorization must fail closed.
+     */
+    authorizeFileRead(recordId: string, context: unknown): Promise<boolean>;
+}

packages/spec/src/data/object.zod.ts

@@ -953,6 +953,31 @@ const ObjectSchemaBase = z.object({
   // enforced by the LifecycleService. Absent = `record` (today's behavior).
   lifecycle: LifecycleSchema.optional().describe('Data lifecycle contract (ADR-0057): class + retention/ttl/rotation/archive policies enforced by the platform LifecycleService.'),
 
+  /**
+   * Who answers "may this caller download a file owned by this object's media
+   * fields?" (ADR-0104 D3 wave 2).
+   *
+   * By default the storage service asks the question directly — can the caller
+   * READ the owning row? That is right for ordinary business objects, where
+   * row readability *is* the access rule. It is wrong for an object whose
+   * access is **mediated by a service** rather than by row permissions: a
+   * system audit table like `sys_approval_action` is deliberately unreadable
+   * to ordinary positions, yet a legitimate approver must still be able to
+   * open a decision attachment. Testing raw readability there asks the wrong
+   * authority and denies the very people the record is for.
+   *
+   * Naming a kernel service here delegates the question to it. The service
+   * must implement `authorizeFileRead(recordId, context) => boolean` (see
+   * `IFileAccessDelegate`). Fails CLOSED: a declared service that is missing
+   * or does not implement the method denies the download rather than falling
+   * back to the raw read.
+   */
+  fileAccessDelegate: z.string().optional().describe(
+    'Kernel service that authorizes downloads of files owned by this object\'s media fields, '
+    + 'instead of testing whether the caller can read the owning row. For objects whose access '
+    + 'is mediated by a service (e.g. sys_approval_action → approvals). Fails closed.',
+  ),
+
   /**
    * Logic & Validation (Co-located)
    * Best Practice: Define rules close to data.