fix(storage)!: an unclaimed file is served only within its organization

.changeset/25693-unclaimed-file-organization-scope.md

@@ -0,0 +1,21 @@
+---
+'@objectstack/service-storage': patch
+---
+
+fix(storage)!: an unclaimed file is served only within its organization
+
+The two download doors (`GET /api/v1/storage/files/:fileId/url` and `GET /api/v1/storage/files/:fileId`) now serve a file with neither an attachments scope nor a field owner (an upload no record has claimed yet, or one whose record let it go) only within its organization. The signed-in check comes first, as before. The download authorizer then judges the file as every other organization-scoped row is read: the organization wall's verdict for the tenancy posture in force, and the driver's tenant scope for the reach. A caller outside that reach is refused `403 FILE_DOWNLOAD_DENIED`, the status and code the doors already give a file the caller may not read, with a message that names no record. No error code is added.
+
+What does not change: the uploader is still served their own upload, as for every other private file. Under the `single` posture every signed-in caller is served as before. A file marked `acl: 'public_read'` stays anonymous. Attachments-scope and field-owned files keep their parent-derived verdicts. An anonymous caller still gets `401 AUTH_REQUIRED`. A kernel with no auth service still serves these files without a check.
+
+Clause-②: no (narrowing) — the download doors serve an unclaimed file to fewer callers under a walled posture (`isolated`, `group`); nothing is widened.
+
+Metadata-protocol: none (how two storage download doors authorize a request; nothing a metadata author writes changes)
+
+<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, object definition, stored row or stored shape is added, removed, renamed or re-shaped, and no exported declaration is removed or narrowed, so there is nothing for `objectstack migrate meta` to rewrite. What changes is which callers the storage download doors serve an unclaimed file to under a walled tenancy posture. The remedy is operational (act in the file's organization, or have a record claim the file, or mark it `acl: 'public_read'`), never a rewrite of a package source or a stored row. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered); and nothing exported is removed or narrowed (not runtime-interface-only, type-surface-only or runtime-export-retired). -->
+
+**BREAKING** for a walled deployment (`isolated` or `group` posture) whose users download unclaimed files from outside the file's organization, shipped as `patch` because the metadata protocol does not move.
+
+- **Before.** The download doors did not apply the organization wall to an unclaimed file.
+- **Now.** A signed-in caller is served an unclaimed file when they uploaded it, or when the file is in their organization's reach under the posture in force (their active organization under `isolated`, any organization they belong to under `group`). Anyone else gets `403 FILE_DOWNLOAD_DENIED`. A caller with no organization under a walled posture is in no organization's reach.
+- **Fix.** Download the file while acting in its organization. Have a record claim it (a file field or an attachment), so that the record's access decides. Or, for a file that must be readable by everyone, mark its stored file record `acl: 'public_read'`.

packages/services/service-storage/src/storage-routes.test.ts

@@ -481,12 +481,14 @@ describe('Storage REST Routes', () => {
       expect(authorizeFileRead).not.toHaveBeenCalled();
     });
 
-    it('never gates a file with neither an attachments scope nor a field owner', async () => {
+    it('[#25693] asks the authorizer about a file with neither an attachments scope nor a field owner, and answers its deny 403 FILE_DOWNLOAD_DENIED', async () => {
       const { server, store: s, authorizeFileRead } = serverWith('deny');
       await commit(s, { id: 'a5', scope: 'user', key: 'user/a5.png' });
       const res = await hit(server, '/api/v1/storage/files/:fileId', 'a5');
-      expect(res._status).toBe(302);
-      expect(authorizeFileRead).not.toHaveBeenCalled();
+      expect(res._status).toBe(403);
+      expect(res._json?.error?.code).toBe('FILE_DOWNLOAD_DENIED');
+      expect(res._headers.Location).toBeUndefined();
+      expect(authorizeFileRead).toHaveBeenCalledOnce();
     });
 
     it('stays open when no authorizer is wired (back-compat)', async () => {
@@ -542,34 +544,45 @@ describe('Storage REST Routes', () => {
         expect(authorizeFileRead).not.toHaveBeenCalled();
       });
 
+      /** Seconds of life the minted local capability URL carries. */
+      const ttlOf = (url: string) =>
+        adapter.verifyToken(url.split('/').pop()!, 'get').exp - Math.floor(Date.now() / 1000);
+
       /**
        * DUAL-MODE REGRESSION. A pre-cutover field holds an inline blob or an
-       * external URL, never a sys_file id — so no legacy file is ever claimed,
-       * `ref_object` stays unset, and this change gates nothing that used to
-       * be open. An unclaimed upload behaves the same way.
+       * external URL, never a sys_file id — so no legacy file is ever claimed
+       * and `ref_object` stays unset. Such a file is the UNCLAIMED class, not
+       * the field-owned one: [#25693] the authorizer judges it as unclaimed,
+       * and an allowed download keeps the presigned TTL that class was always
+       * minted with rather than the field-owned class's short one.
        */
-      it('does not gate an unclaimed file (no ref_object) — legacy fields keep working', async () => {
-        const { server, store: s, authorizeFileRead } = serverWith('deny');
+      it('judges an unclaimed file (no ref_object) as the unclaimed class, not the field-owned one', async () => {
+        const { server, store: s, authorizeFileRead } = serverWith('allow', { downloadTtl: 120, presignedTtl: 1800 });
         await commit(s, { id: 'fo5', scope: 'user', key: 'user/fo5.png' });
-        const res = await hit(server, '/api/v1/storage/files/:fileId', 'fo5');
-        expect(res._status).toBe(302);
-        expect(authorizeFileRead).not.toHaveBeenCalled();
+        const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'fo5');
+        expect(res._status).toBe(200);
+        expect(ttlOf(res._json.data.url)).toBeGreaterThan(1700);
+        expect(authorizeFileRead).toHaveBeenCalledOnce();
       });
 
-      it('does not gate a file whose ownership was released', async () => {
-        const { server, store: s, authorizeFileRead } = serverWith('deny');
+      it('judges a file whose ownership was released as the unclaimed class', async () => {
+        const { server, store: s, authorizeFileRead } = serverWith('allow', { downloadTtl: 120, presignedTtl: 1800 });
         await commit(s, owned('fo6', { ref_object: null, ref_id: null, ref_field: null }));
-        const res = await hit(server, '/api/v1/storage/files/:fileId', 'fo6');
-        expect(res._status).toBe(302);
-        expect(authorizeFileRead).not.toHaveBeenCalled();
+        const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'fo6');
+        expect(res._status).toBe(200);
+        expect(ttlOf(res._json.data.url)).toBeGreaterThan(1700);
+        expect(authorizeFileRead).toHaveBeenCalledOnce();
       });
     });
   });
 
   // ── [#22431] The unclaimed class: neither an attachments scope nor a field
   // owner. It has no parent record to derive access from, so the download
-  // asks for one thing only — a signed-in caller — unless the file declares
-  // `acl: 'public_read'` (ADR-0104's one anonymous download).
+  // asks for a signed-in caller first — unless the file declares
+  // `acl: 'public_read'` (ADR-0104's one anonymous download). [#25693] A
+  // signed-in caller is then judged by the authorizer, which serves such a
+  // file only within its organization (pinned on a real engine in
+  // `unclaimed-file-organization-scope.test.ts`).
   describe('a file with neither an attachments scope nor a field owner needs a signed-in caller (#22431)', () => {
     const DOORS = ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId'] as const;
 
@@ -624,7 +637,7 @@ describe('Storage REST Routes', () => {
       }
       expect(minted).not.toHaveBeenCalled();
       expect(resolveSession).toHaveBeenCalledTimes(2);
-      // Not the parent-governed authorizer's file: there is no parent record.
+      // Refused at the session gate, before the authorizer is asked.
       expect(authorizeFileRead).not.toHaveBeenCalled();
       minted.mockRestore();
     });
@@ -656,8 +669,9 @@ describe('Storage REST Routes', () => {
       }
     });
 
-    it('serves a signed-in caller as before: a 302 and a URL carrying the presigned TTL', async () => {
-      const { server, store: s, authorizeFileRead } = serverWith(async () => ({ userId: 'user-7' }), { presignedTtl: 1800 });
+    it('serves a signed-in caller the authorizer allows: a 302 and a URL carrying the presigned TTL', async () => {
+      const authorizeFileRead = vi.fn(async () => 'allow' as const);
+      const { server, store: s } = serverWith(async () => ({ userId: 'user-7' }), { presignedTtl: 1800, authorizeFileRead });
       await commit(s, { id: 'u4' });
       const url = await hit(server, DOORS[0], 'u4');
       expect(url._status).toBe(200);
@@ -668,7 +682,26 @@ describe('Storage REST Routes', () => {
       const redirect = await hit(server, DOORS[1], 'u4');
       expect(redirect._status).toBe(302);
       expect(redirect._headers.Location).toContain('/_local/raw/');
-      expect(authorizeFileRead).not.toHaveBeenCalled();
+      // [#25693] Asked once per door, after the session gate passed.
+      expect(authorizeFileRead).toHaveBeenCalledTimes(2);
+    });
+
+    it('[#25693] refuses a signed-in caller the authorizer denies 403 FILE_DOWNLOAD_DENIED at both doors, minting no URL', async () => {
+      const resolveSession = vi.fn(async () => ({ userId: 'user-7', organizationId: 'org-2' }));
+      const { server, store: s, authorizeFileRead } = serverWith(resolveSession);
+      await commit(s, { id: 'u4d' });
+      const minted = vi.spyOn(adapter, 'getPresignedDownload');
+      for (const door of DOORS) {
+        const res = await hit(server, door, 'u4d');
+        expect(res._status, door).toBe(403);
+        expect(res._json?.success, door).toBe(false);
+        expect(res._json?.error?.code, door).toBe('FILE_DOWNLOAD_DENIED');
+        expect(res._headers.Location, door).toBeUndefined();
+      }
+      expect(minted).not.toHaveBeenCalled();
+      expect(resolveSession).toHaveBeenCalledTimes(2);
+      expect(authorizeFileRead).toHaveBeenCalledTimes(2);
+      minted.mockRestore();
     });
 
     it("keeps an acl: 'public_read' file anonymous without asking for a session", async () => {
@@ -705,7 +738,9 @@ describe('Storage REST Routes', () => {
     });
 
     it('keeps a kernel with no session resolver open, and says so once', async () => {
-      const { server, store: s, logger } = serverWith(undefined);
+      // A bare kernel: no auth service, so neither a session resolver nor an
+      // authorizer is bound (`composeStorageRoutes`).
+      const { server, store: s, logger } = serverWith(undefined, { authorizeFileRead: undefined });
       await commit(s, { id: 'u6' });
       expect((await hit(server, DOORS[1], 'u6'))._status).toBe(302);
       expect((await hit(server, DOORS[0], 'u6'))._status).toBe(200);

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

@@ -292,21 +292,24 @@ export interface StorageRoutesOptions {
    * with neither an attachments scope nor a field owner (an upload no record
    * has claimed — an avatar or an organization logo stored as a URL, a pick
    * not yet saved, a released file). Such a file has no parent record to
-   * derive access from, so the one thing its download requires is a
-   * signed-in caller; none ⇒ `401 AUTH_REQUIRED`, the answer the upload
-   * routes give. A file marked `acl: 'public_read'` stays anonymous
+   * derive access from, so its download first requires a signed-in caller;
+   * none ⇒ `401 AUTH_REQUIRED`, the answer the upload routes give. [#25693]
+   * A signed-in caller is then judged by
+   * {@link StorageRoutesOptions.authorizeFileRead} when one is wired: the
+   * file is served only within its organization. A file marked
+   * `acl: 'public_read'` stays anonymous
    * (ADR-0104). A browser's `<img src>` / `<a href>` reaches this resolver
    * with the session cookie its sign-in set, which reads the same as a bearer
    * header, so a signed-in page keeps rendering these files. Absent: these
    * downloads stay open as before — logged once.
    */
   resolveSession?: (req: IHttpRequest) => Promise<StorageUploadSession | null | undefined>;
    /**
-   * Authorize a DOWNLOAD of a parent-governed file (#2970 item 2, extended by
-   * ADR-0104 D3 wave 2). When wired, the download endpoints
+   * Authorize a DOWNLOAD of a private file (#2970 item 2, extended by
+   * ADR-0104 D3 wave 2 and #25693). When wired, the download endpoints
    * (`/files/:fileId` and `/files/:fileId/url`) consult this for
-   * `scope==='attachments'` files AND for field-owned files (`ref_object` set),
-   * excluding `public_read` ones:
+   * `scope==='attachments'` files, for field-owned files (`ref_object` set)
+   * AND for the unclaimed files below, excluding `public_read` ones:
    *   - `unauthenticated` → 401 (no session)
    *   - `deny` → 403 (session, but cannot read the parent record the file
    *     belongs to / is attached to, and is not the owner)
@@ -316,14 +319,17 @@ export interface StorageRoutesOptions {
    * `SERVICE_UNAVAILABLE` rather than flattened into the `deny` 403 — the
    * store was unreadable, so no verdict was ever reached. Every other throw
    * still fails closed to `deny`.
-   * A file with neither an attachments scope nor a field owner is not this
-   * authorizer's to judge — it has no parent record — and needs only a
-   * signed-in caller ({@link StorageRoutesOptions.resolveSession}, #22431).
+   * [#25693] A file with neither an attachments scope nor a field owner has
+   * no parent record. It first needs a signed-in caller
+   * ({@link StorageRoutesOptions.resolveSession}, #22431), and then this
+   * authorizer judges it too: such a file is served only within its
+   * organization, as every other organization-scoped row is read. Its
+   * `deny` answers `403 FILE_DOWNLOAD_DENIED`, naming no record.
    * Any file explicitly marked `acl: 'public_read'` keeps the stable
    * anonymous capability URL (ADR-0104: the opt-in for genuinely public
    * embedding, which no sign-in precedes).
    * When absent (bare kernels, tests), parent-governed downloads stay open
-   * (back-compat).
+   * (back-compat), and an unclaimed file needs only the signed-in caller.
    */
   authorizeFileRead?: (file: FileRecord, req: IHttpRequest) => Promise<FileReadVerdict>;
   /**
@@ -505,9 +511,11 @@ export function registerStorageFamily(
   //     (`ref_object`/`ref_id`, ADR-0104 D3 wave 2).
   // [#22431] Every OTHER file — neither an attachments scope nor a field
   // owner: an upload no record has claimed — has no parent record to derive
-  // access from, and needs a signed-in caller instead
+  // access from, and needs a signed-in caller first
   // (`requireDownloadSession` below). It used to be the one class still
-  // served as an anonymous capability URL by default.
+  // served as an anonymous capability URL by default. [#25693] The same
+  // authorizer then judges it within its organization, as every other
+  // organization-scoped row is read (`StorageRoutesOptions.authorizeFileRead`).
   // `acl: 'public_read'` opts any file back out to the stable anonymous
   // capability URL — the explicit declaration for genuinely public
   // embedding, which no sign-in precedes (ADR-0104). A signed-in browser
@@ -528,10 +536,12 @@ export function registerStorageFamily(
     if (file.acl === 'public_read') return currentLimits().presignedTtl;
     const fieldOwned = !!file.ref_object && file.ref_id != null && file.ref_id !== '';
     const gated = file.scope === 'attachments' || fieldOwned;
-    if (!gated) {
-      if (!(await requireDownloadSession(req, res))) return false;
-      return currentLimits().presignedTtl;
-    }
+    // [#22431] The unclaimed class asks for a signed-in caller first, with the
+    // session gate's own 401. [#25693] Then, like every other private file, it
+    // is judged by the download authorizer below: it is served only within its
+    // organization (see `StorageRoutesOptions.authorizeFileRead`). It keeps the
+    // presigned TTL it was always minted with.
+    if (!gated && !(await requireDownloadSession(req, res))) return false;
     if (!opts.authorizeFileRead) {
       return currentLimits().presignedTtl;
     }
@@ -581,12 +591,17 @@ export function registerStorageFamily(
     if (verdict === 'deny') {
       if (fieldOwned) {
         sendError(res, 403, 'FILE_DOWNLOAD_DENIED', 'You do not have access to the record this file belongs to');
-      } else {
+      } else if (gated) {
         sendError(res, 403, 'ATTACHMENT_DOWNLOAD_DENIED', 'You do not have access to a record this file is attached to');
+      } else {
+        // [#25693] An unclaimed file belongs to no record, so the refusal names
+        // none: the status and code the door gives every other file the caller
+        // may not read, and nothing about whose file it is.
+        sendError(res, 403, 'FILE_DOWNLOAD_DENIED', 'You do not have access to this file');
       }
       return false;
     }
-    return downloadTtl;
+    return gated ? downloadTtl : currentLimits().presignedTtl;
   };
 
   // ── Download servability (#10246) ────────────────────────────────────