diff --git a/.changeset/fix-unmounted-local-file-url.md b/.changeset/fix-unmounted-local-file-url.md new file mode 100644 index 0000000000..aff5e72f31 --- /dev/null +++ b/.changeset/fix-unmounted-local-file-url.md @@ -0,0 +1,35 @@ +--- +"@objectstack/service-storage": patch +--- + +fix(service-storage): stop handing out `_local/file/:key`, a URL nothing mounts (#3641) + +Three call sites built `${basePath}/_local/file/`. No registrar has ever +mounted it, so anyone who followed one got a 404. Found by the tranche-3 +storage ledger (#3636), which recorded the URL as deliberately absent and filed +this; now nothing builds it either. + +Each site is fixed according to what it could honestly do: + +- **`LocalStorageAdapter.getPresignedUpload()`** simply omits `downloadUrl` + (optional on the descriptor). It cannot construct the real capability URL — + that is keyed by `sys_file.id`, and an adapter only ever sees the storage + key. Nothing read the field anyway, which is how it survived: the + presigned-upload route builds its own `downloadUrl` + (`${basePath}/files/:fileId/url`) and ignores this one, while all three real + readers of `desc.downloadUrl` take it from `getPresignedDownload`, whose URL + *is* mounted (`_local/raw/`). + +- **`GET /files/:fileId/url` and `GET /files/:fileId`** answer **501 + `NOT_IMPLEMENTED`** when the adapter has neither `getPresignedDownload` nor + `getSignedUrl`, instead of returning (or redirecting to) the unmounted URL. + The caller now learns the adapter is the limitation rather than chasing a + broken link. + +Behaviour change is confined to adapters implementing neither capability — +`LocalStorageAdapter` and the S3 adapter both implement `getPresignedDownload`, +so no shipped path changes. A 200/302 pointing at a 404 becomes a 501 that says +why. + +Two conformance cases added for the new branches, and mutation-checked: +restoring either dead URL fails them. diff --git a/packages/services/service-storage/src/error-envelope.conformance.test.ts b/packages/services/service-storage/src/error-envelope.conformance.test.ts index a6411f9723..9ccd89ab60 100644 --- a/packages/services/service-storage/src/error-envelope.conformance.test.ts +++ b/packages/services/service-storage/src/error-envelope.conformance.test.ts @@ -222,6 +222,32 @@ describe('storage error envelope (#3675)', () => { return drive(routes, 'PUT', `${BASE}/_local/raw/:token`, { params: { token: 'x' } }); }, }, + { + // #3641: these two used to answer 200/302 with + // `${basePath}/_local/file/` — a URL no registrar mounts — so a + // caller followed a link straight into a 404. An adapter that can + // produce no download URL now says so. + name: 'download URL from an adapter with neither presigned nor signed URLs', + status: 501, + code: 'NOT_IMPLEMENTED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'nocap', { scope: 'user', key: 'user/nocap.bin' }); + const routes = mount({} as any, store); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'nocap' } }); + }, + }, + { + name: 'the redirect twin of the same adapter limitation', + status: 501, + code: 'NOT_IMPLEMENTED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'nocap2', { scope: 'user', key: 'user/nocap2.bin' }); + const routes = mount({} as any, store); + return drive(routes, 'GET', `${BASE}/files/:fileId`, { params: { fileId: 'nocap2' } }); + }, + }, { name: 'raw download with a forged token signature', status: 403, diff --git a/packages/services/service-storage/src/local-storage-adapter.ts b/packages/services/service-storage/src/local-storage-adapter.ts index 8c09abc32d..9b75168555 100644 --- a/packages/services/service-storage/src/local-storage-adapter.ts +++ b/packages/services/service-storage/src/local-storage-adapter.ts @@ -267,7 +267,20 @@ export class LocalStorageAdapter implements IStorageService { method: 'PUT', headers: options?.contentType ? { 'content-type': options.contentType } : { 'content-type': 'application/octet-stream' }, expiresIn, - downloadUrl: `${this.baseUrl}${this.basePath}/_local/file/${encodeURIComponent(key)}`, + // `downloadUrl` is deliberately OMITTED (it is optional on the + // descriptor). It used to be `${basePath}/_local/file/` — a URL no + // registrar has ever mounted, so anyone following it got a 404 (#3641). + // + // Nothing read it, which is why it survived: the presigned-upload route + // builds its own `downloadUrl` (`${basePath}/files/:fileId/url`) and + // ignores this field, and the three real readers of `desc.downloadUrl` + // all take it from `getPresignedDownload`, whose URL IS mounted + // (`_local/raw/`). + // + // Not repaired into a working URL, because this adapter cannot build + // one: the capability URL is keyed by `sys_file.id`, and an adapter only + // ever sees the storage key. Emitting nothing is honest; emitting a + // 404 was not. }; } diff --git a/packages/services/service-storage/src/storage-route-ledger.ts b/packages/services/service-storage/src/storage-route-ledger.ts index 58e25bd4c7..61c81ed095 100644 --- a/packages/services/service-storage/src/storage-route-ledger.ts +++ b/packages/services/service-storage/src/storage-route-ledger.ts @@ -25,8 +25,13 @@ * SCOPE & SHAPE. Rows carry full wire paths at the DEFAULT base * (`/api/v1/storage`); `StorageRoutesOptions.basePath` can move the whole * family, and the conformance test enumerates at the same default so the two - * stay comparable. `GET {base}/_local/file/:key` is deliberately ABSENT — three - * call sites build that URL but no registrar has ever mounted it (#3641). + * stay comparable. `GET {base}/_local/file/:key` is ABSENT because nothing + * mounts it — and since #3641 nothing BUILDS it either. The three call sites + * that used to are gone: the local adapter's upload descriptor simply omits + * the optional `downloadUrl` (it cannot construct the real capability URL, + * which is keyed by `sys_file.id` rather than the storage key), and the two + * download routes now answer 501 when the adapter can produce no URL at all, + * instead of handing back one that 404s. * * This module is package-internal (not exported from the index): it is the * guard's data, not public API. It must stay import-free — the client-side diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 4ca9ce7912..10aa5d7459 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -543,7 +543,11 @@ export function registerStorageRoutes( } else if (storage.getSignedUrl) { url = await storage.getSignedUrl(file.key, ttl, downloadOpts); } else { - url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`; + // See the sibling branch on `GET /files/:fileId`: no adapter capability + // means no download URL, and handing back an unmounted one is worse + // than admitting it (#3641). + sendError(res, 501, 'NOT_IMPLEMENTED', 'This storage adapter cannot issue download URLs'); + return; } res.json({ url }); @@ -583,7 +587,13 @@ export function registerStorageRoutes( } else if (storage.getSignedUrl) { url = await storage.getSignedUrl(file.key, ttl, downloadOpts); } else { - url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`; + // An adapter with neither `getPresignedDownload` nor `getSignedUrl` + // cannot produce a download URL. This used to redirect to + // `${basePath}/_local/file/`, which no registrar mounts — a 302 + // straight into a 404 (#3641). Say so instead: the caller learns the + // adapter is the limitation, rather than chasing a broken link. + sendError(res, 501, 'NOT_IMPLEMENTED', 'This storage adapter cannot issue download URLs'); + return; } res.status(302).header('Location', url).send('');