fix(service-storage): stop handing out _local/file/:key, a URL nothing mounts (#3641)

.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/<key>`. 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/<token>`).
+
+- **`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.

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/<key>` — 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,

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/<key>` — 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/<token>`).
+      //
+      // 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.
     };
   }
 

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

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/<key>`, 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('');