Skip to content

Commit f4f5ce8

Browse files
committed
fix(service-storage): a download of a file with no attachments scope and no field owner requires a signed-in caller
The two download doors now ask the session resolver for a file that has neither an attachments scope nor a field owner, unless the file is acl public_read (ADR-0104). No session answers 401 AUTH_REQUIRED, the pair the routes' other unauthenticated refusals answer. A kernel with no session resolver keeps serving these downloads and says so once. Claude-Session: https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ Co-authored-by: Claude <noreply@anthropic.com>
1 parent b9222dc commit f4f5ce8

3 files changed

Lines changed: 243 additions & 21 deletions

File tree

‎packages/services/service-storage/src/error-envelope.conformance.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,19 @@ describe('storage error envelope (#3675)', () => {
314314
return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'a2' } });
315315
},
316316
},
317+
{
318+
// #22431: a file with neither an attachments scope nor a field owner
319+
// needs a signed-in caller — the same pair as the two 401s above.
320+
name: 'anonymous download of a file with neither an attachments scope nor a field owner',
321+
status: 401,
322+
code: 'AUTH_REQUIRED',
323+
run: async () => {
324+
const store = new StorageMetadataStore(null);
325+
await committedAttachment(store, 'u1', { scope: 'user', key: 'user/u1.png' });
326+
const routes = mount(await tmpAdapter(), store, { resolveSession: async () => null });
327+
return drive(routes, 'GET', `${BASE}/files/:fileId`, { params: { fileId: 'u1' } });
328+
},
329+
},
317330
{
318331
name: 'raw upload against an adapter with no token support',
319332
status: 501,

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

Lines changed: 153 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,155 @@ describe('Storage REST Routes', () => {
563563
});
564564
});
565565

566+
// ── [#22431] The unclaimed class: neither an attachments scope nor a field
567+
// owner. It has no parent record to derive access from, so the download
568+
// asks for one thing only — a signed-in caller — unless the file declares
569+
// `acl: 'public_read'` (ADR-0104's one anonymous download).
570+
describe('a file with neither an attachments scope nor a field owner needs a signed-in caller (#22431)', () => {
571+
const DOORS = ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId'] as const;
572+
573+
const commit = async (s: StorageMetadataStore, rec: Partial<import('./metadata-store.js').FileRecord>) =>
574+
s.createFile({
575+
id: rec.id ?? 'u-dl',
576+
key: rec.key ?? `user/${rec.id ?? 'u-dl'}.png`,
577+
name: 'x.png',
578+
status: 'committed',
579+
acl: 'private',
580+
scope: 'user',
581+
...rec,
582+
} as any);
583+
584+
function serverWith(resolveSession: any, extra: any = {}) {
585+
const server = createMockHttpServer();
586+
const s = new StorageMetadataStore(null);
587+
const authorizeFileRead = vi.fn(async () => 'deny' as const);
588+
const logger = { info: vi.fn(), warn: vi.fn() };
589+
registerStorageRoutes(server as any, adapter, s, {
590+
basePath: '/api/v1/storage',
591+
resolveSession,
592+
authorizeFileRead,
593+
logger,
594+
downloadTtl: 120,
595+
...extra,
596+
});
597+
return { server, store: s, authorizeFileRead, logger };
598+
}
599+
600+
const hit = async (server: any, path: string, fileId: string) => {
601+
const res = createMockRes();
602+
await server._getHandler('GET', path)!(createMockReq({ params: { fileId } }), res);
603+
return res;
604+
};
605+
606+
/** Seconds of life the minted local capability URL carries. */
607+
const ttlOf = (url: string) =>
608+
adapter.verifyToken(url.split('/').pop()!, 'get').exp - Math.floor(Date.now() / 1000);
609+
610+
it('refuses an anonymous caller 401 AUTH_REQUIRED at both download doors, minting no URL', async () => {
611+
const resolveSession = vi.fn(async () => null);
612+
const { server, store: s, authorizeFileRead } = serverWith(resolveSession);
613+
await commit(s, { id: 'u1' });
614+
const minted = vi.spyOn(adapter, 'getPresignedDownload');
615+
for (const door of DOORS) {
616+
const res = await hit(server, door, 'u1');
617+
expect(res._status, door).toBe(401);
618+
expect(res._json?.success, door).toBe(false);
619+
expect(res._json?.error?.code, door).toBe('AUTH_REQUIRED');
620+
expect(res._headers.Location, door).toBeUndefined();
621+
}
622+
expect(minted).not.toHaveBeenCalled();
623+
expect(resolveSession).toHaveBeenCalledTimes(2);
624+
// Not the parent-governed authorizer's file: there is no parent record.
625+
expect(authorizeFileRead).not.toHaveBeenCalled();
626+
minted.mockRestore();
627+
});
628+
629+
it('answers the same code and status the upload doors give an anonymous caller', async () => {
630+
const { server, store: s } = serverWith(async () => null);
631+
await commit(s, { id: 'u2' });
632+
const download = await hit(server, DOORS[0], 'u2');
633+
const upload = createMockRes();
634+
await server._getHandler('POST', '/api/v1/storage/upload/presigned')!(
635+
createMockReq({ body: { filename: 'a.png', mimeType: 'image/png', size: 3 } }),
636+
upload,
637+
);
638+
expect([download._status, download._json?.error?.code]).toEqual([upload._status, upload._json?.error?.code]);
639+
});
640+
641+
it('fails closed when the session resolver throws, or resolves a session with no user', async () => {
642+
for (const resolveSession of [
643+
async () => { throw new Error('auth backend down'); },
644+
async () => ({ organizationId: 'org-1' }),
645+
]) {
646+
const { server, store: s } = serverWith(resolveSession);
647+
await commit(s, { id: 'u3' });
648+
for (const door of DOORS) {
649+
const res = await hit(server, door, 'u3');
650+
expect(res._status, door).toBe(401);
651+
expect(res._json?.error?.code, door).toBe('AUTH_REQUIRED');
652+
}
653+
}
654+
});
655+
656+
it('serves a signed-in caller as before: a 302 and a URL carrying the presigned TTL', async () => {
657+
const { server, store: s, authorizeFileRead } = serverWith(async () => ({ userId: 'user-7' }), { presignedTtl: 1800 });
658+
await commit(s, { id: 'u4' });
659+
const url = await hit(server, DOORS[0], 'u4');
660+
expect(url._status).toBe(200);
661+
expect(url._json.data.url).toContain('/_local/raw/');
662+
const ttl = ttlOf(url._json.data.url);
663+
expect(ttl).toBeGreaterThan(1700);
664+
expect(ttl).toBeLessThanOrEqual(1800);
665+
const redirect = await hit(server, DOORS[1], 'u4');
666+
expect(redirect._status).toBe(302);
667+
expect(redirect._headers.Location).toContain('/_local/raw/');
668+
expect(authorizeFileRead).not.toHaveBeenCalled();
669+
});
670+
671+
it("keeps an acl: 'public_read' file anonymous without asking for a session", async () => {
672+
const resolveSession = vi.fn(async () => null);
673+
const { server, store: s } = serverWith(resolveSession);
674+
await commit(s, { id: 'u5', acl: 'public_read' });
675+
expect((await hit(server, DOORS[0], 'u5'))._status).toBe(200);
676+
expect((await hit(server, DOORS[1], 'u5'))._status).toBe(302);
677+
expect(resolveSession).not.toHaveBeenCalled();
678+
});
679+
680+
it('leaves the parent-governed classes to the authorizer: attachments-scope and field-owned verdicts are unchanged', async () => {
681+
const resolveSession = vi.fn(async () => ({ userId: 'user-7' }));
682+
const { server, store: s, authorizeFileRead } = serverWith(resolveSession);
683+
await commit(s, { id: 'g1', scope: 'attachments', key: 'attachments/g1.bin' });
684+
await commit(s, { id: 'g2', ref_object: 'product', ref_id: 'p1', ref_field: 'image' });
685+
const attached = await hit(server, DOORS[0], 'g1');
686+
expect(attached._status).toBe(403);
687+
expect(attached._json?.error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
688+
const owned = await hit(server, DOORS[0], 'g2');
689+
expect(owned._status).toBe(403);
690+
expect(owned._json?.error?.code).toBe('FILE_DOWNLOAD_DENIED');
691+
expect(authorizeFileRead).toHaveBeenCalledTimes(2);
692+
expect(resolveSession).not.toHaveBeenCalled();
693+
});
694+
695+
it('answers a missing file 404 before the session is asked', async () => {
696+
const resolveSession = vi.fn(async () => null);
697+
const { server } = serverWith(resolveSession);
698+
const res = await hit(server, DOORS[0], 'missing');
699+
expect(res._status).toBe(404);
700+
expect(res._json?.error?.code).toBe('FILE_NOT_FOUND');
701+
expect(resolveSession).not.toHaveBeenCalled();
702+
});
703+
704+
it('keeps a kernel with no session resolver open, and says so once', async () => {
705+
const { server, store: s, logger } = serverWith(undefined);
706+
await commit(s, { id: 'u6' });
707+
expect((await hit(server, DOORS[1], 'u6'))._status).toBe(302);
708+
expect((await hit(server, DOORS[0], 'u6'))._status).toBe(200);
709+
const notices = logger.info.mock.calls.map((c) => String(c[0])).filter((m) => m.includes('no session resolver wired'));
710+
expect(notices).toHaveLength(1);
711+
expect(notices[0]).toContain('download');
712+
});
713+
});
714+
566715
describe('PUT/GET /_local/raw/:token', () => {
567716
it('should accept raw upload with valid token and serve download', async () => {
568717
// Generate a presigned upload
@@ -644,14 +793,16 @@ describe('Storage REST Routes', () => {
644793
expect(file?.owner_id).toBe('user-42');
645794
});
646795

647-
it('download routes stay open even with a resolver wired (capability URLs)', async () => {
796+
it('download routes are not behind the upload gate: an anonymous read of a missing file is a 404', async () => {
648797
const server = registerWithResolver(async () => null);
649798
const res = createMockRes();
650799
await server._getHandler('GET', '/api/v1/storage/files/:fileId/url')!(
651800
createMockReq({ params: { fileId: 'missing' } }),
652801
res,
653802
);
654-
expect(res._status).toBe(404); // not 401 — anonymous reads reach the handler
803+
// Not 401: the download doors look the file up first and judge the
804+
// caller per file class (#22431 — see the unclaimed-class block above).
805+
expect(res._status).toBe(404);
655806
});
656807

657808
it('stays open (back-compat) when no resolver is wired', async () => {

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

Lines changed: 77 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -221,9 +221,19 @@ export interface StorageRoutesOptions {
221221
* progress doors also refuse a caller who is not the file's uploader
222222
* (`isFileUploader`, `403 PERMISSION_DENIED`). When absent (bare kernels,
223223
* tests), the routes stay open — back-compat, logged once — and there is no
224-
* caller identity for the ownership rule to compare. Download routes are NOT
225-
* gated here (capability URLs embedded in <img src>/<a href>; gating them
226-
* is a tracked follow-up needing cookie sessions or signed links).
224+
* caller identity for the ownership rule to compare.
225+
*
226+
* [#22431] The two DOWNLOAD routes ask it too, for one file class: a file
227+
* with neither an attachments scope nor a field owner (an upload no record
228+
* has claimed — an avatar or an organization logo stored as a URL, a pick
229+
* not yet saved, a released file). Such a file has no parent record to
230+
* derive access from, so the one thing its download requires is a
231+
* signed-in caller; none ⇒ `401 AUTH_REQUIRED`, the answer the upload
232+
* routes give. A file marked `acl: 'public_read'` stays anonymous
233+
* (ADR-0104). A browser's `<img src>` / `<a href>` reaches this resolver
234+
* with the session cookie its sign-in set, which reads the same as a bearer
235+
* header, so a signed-in page keeps rendering these files. Absent: these
236+
* downloads stay open as before — logged once.
227237
*/
228238
resolveSession?: (req: IHttpRequest) => Promise<StorageUploadSession | null | undefined>;
229239
/**
@@ -241,11 +251,14 @@ export interface StorageRoutesOptions {
241251
* `SERVICE_UNAVAILABLE` rather than flattened into the `deny` 403 — the
242252
* store was unreadable, so no verdict was ever reached. Every other throw
243253
* still fails closed to `deny`.
244-
* A file with neither an attachments scope nor a field owner — an unclaimed
245-
* upload, an org logo — keeps the stable anonymous capability URL, as does
246-
* any file explicitly marked `acl: 'public_read'` (the opt-in for genuinely
247-
* public embedding, since `<img src>` cannot carry a bearer token).
248-
* When absent (bare kernels, tests), all downloads stay open (back-compat).
254+
* A file with neither an attachments scope nor a field owner is not this
255+
* authorizer's to judge — it has no parent record — and needs only a
256+
* signed-in caller ({@link StorageRoutesOptions.resolveSession}, #22431).
257+
* Any file explicitly marked `acl: 'public_read'` keeps the stable
258+
* anonymous capability URL (ADR-0104: the opt-in for genuinely public
259+
* embedding, which no sign-in precedes).
260+
* When absent (bare kernels, tests), parent-governed downloads stay open
261+
* (back-compat).
249262
*/
250263
authorizeFileRead?: (file: FileRecord, req: IHttpRequest) => Promise<FileReadVerdict>;
251264
/**
@@ -338,19 +351,59 @@ export function registerStorageRoutes(
338351
return false;
339352
};
340353

354+
// ── Download session gate (#22431) ───────────────────────────────────
355+
// The one refusal both download doors give a caller with no session —
356+
// for a file of the unclaimed class below, and for a parent-governed file
357+
// whose authorizer answers `unauthenticated`. `401` / `AUTH_REQUIRED`, the
358+
// pair the upload gate answers too, so a client branches on one answer.
359+
const refuseAnonymousDownload = (res: IHttpResponse): false => {
360+
sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to download this file');
361+
return false;
362+
};
363+
364+
// `true` ⇒ the caller is signed in, or no resolver is wired (bare kernels,
365+
// tests: open, as before — said once, here, because the upload gate's own
366+
// notice names only the upload routes). `false` ⇒ the 401 was already sent.
367+
// A resolver that throws fails closed, exactly as the upload gate does.
368+
let warnedOpenDownloads = false;
369+
const requireDownloadSession = async (req: IHttpRequest, res: IHttpResponse): Promise<boolean> => {
370+
if (!opts.resolveSession) {
371+
if (!warnedOpenDownloads) {
372+
warnedOpenDownloads = true;
373+
opts.logger?.info(
374+
'[storage] no session resolver wired — a file with neither an attachments scope nor a field owner ' +
375+
'downloads without a signed-in caller (bare-kernel mode)',
376+
);
377+
}
378+
return true;
379+
}
380+
let session: StorageUploadSession | null | undefined;
381+
try {
382+
session = await opts.resolveSession(req);
383+
} catch {
384+
session = null;
385+
}
386+
return session?.userId ? true : refuseAnonymousDownload(res);
387+
};
388+
341389
// ── Download authorization gate (#2970 item 2, ADR-0104 D3 wave 2) ───
342390
// Two kinds of file are gated, both deriving access from a PARENT record:
343391
// - `attachments`-scope files, via their sys_attachment join rows;
344392
// - field-owned files, via the single record whose field owns them
345393
// (`ref_object`/`ref_id`, ADR-0104 D3 wave 2).
346-
// `acl: 'public_read'` opts a file back out to the stable anonymous
347-
// capability URL — needed for genuinely public embedding (`<img src>`
348-
// cannot carry a bearer token), and now an explicit declaration rather
349-
// than the silent default it used to be for every field file.
394+
// [#22431] Every OTHER file — neither an attachments scope nor a field
395+
// owner: an upload no record has claimed — has no parent record to derive
396+
// access from, and needs a signed-in caller instead
397+
// (`requireDownloadSession` below). It used to be the one class still
398+
// served as an anonymous capability URL by default.
399+
// `acl: 'public_read'` opts any file back out to the stable anonymous
400+
// capability URL — the explicit declaration for genuinely public
401+
// embedding, which no sign-in precedes (ADR-0104). A signed-in browser
402+
// needs no such opt-out: its `<img src>` carries the session cookie.
350403
//
351404
// Dual-mode safe: a legacy field holds an inline blob or an external URL,
352405
// never a `sys_file` id, so no legacy file has `ref_object` set and none of
353-
// them start being gated by this change.
406+
// them start being gated by the parent-derived arm.
354407
//
355408
// Returns the signed-URL TTL to use, or `false` if a response was already
356409
// sent (401/403, and since #15999 the `503 SERVICE_UNAVAILABLE` an
@@ -360,9 +413,14 @@ export function registerStorageRoutes(
360413
req: IHttpRequest,
361414
res: IHttpResponse,
362415
): Promise<number | false> => {
416+
if (file.acl === 'public_read') return currentLimits().presignedTtl;
363417
const fieldOwned = !!file.ref_object && file.ref_id != null && file.ref_id !== '';
364418
const gated = file.scope === 'attachments' || fieldOwned;
365-
if (!gated || file.acl === 'public_read' || !opts.authorizeFileRead) {
419+
if (!gated) {
420+
if (!(await requireDownloadSession(req, res))) return false;
421+
return currentLimits().presignedTtl;
422+
}
423+
if (!opts.authorizeFileRead) {
366424
return currentLimits().presignedTtl;
367425
}
368426
let verdict: FileReadVerdict;
@@ -399,10 +457,7 @@ export function registerStorageRoutes(
399457
}
400458
verdict = 'deny'; // a failed authz check must never fall open
401459
}
402-
if (verdict === 'unauthenticated') {
403-
sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to download this file');
404-
return false;
405-
}
460+
if (verdict === 'unauthenticated') return refuseAnonymousDownload(res);
406461
if (verdict === 'deny') {
407462
if (fieldOwned) {
408463
sendError(res, 403, 'FILE_DOWNLOAD_DENIED', 'You do not have access to the record this file belongs to');
@@ -1248,7 +1303,10 @@ export function registerStorageRoutes(
12481303
// - serves the bytes directly when followed
12491304
// The `/url` endpoint above returns JSON. This sibling endpoint resolves
12501305
// to the same short-lived signed URL and 302-redirects so it can be used
1251-
// verbatim in any browser context.
1306+
// verbatim in any browser context. It answers exactly as `/url` does
1307+
// (`authorizeDownload`): a browser following it is judged by the session
1308+
// cookie it carries, and only an `acl: 'public_read'` file is served to
1309+
// a caller with none (#22431).
12521310
// ---------------------------------------------------------------------------
12531311
httpServer.get(`${basePath}/files/:fileId`, async (req: IHttpRequest, res: IHttpResponse) => {
12541312
try {

0 commit comments

Comments
 (0)