fix(approvals+storage): decision attachments — name, open, correct download filename (#3504)

.changeset/approval-attachment-descriptors.md

@@ -0,0 +1,22 @@
+---
+"@objectstack/spec": patch
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): return decision attachments as file descriptors, not "[object Object]" (#3504)
+
+The `sys_approval_action.attachments` file field stores rich descriptors
+(`{ id, name, url, mimeType, size }`), not bare fileId strings — a fileId passed
+to the decision is resolved to a full descriptor on write. But `rowFromAction`
+mapped the column with `.map(String)`, collapsing each descriptor object to the
+literal string `"[object Object]"`. Every `listActions` consumer (the approval
+inbox timeline) then received garbage: the attachment chip had no filename and
+its id was `"[object Object]"`, so opening it 404'd.
+
+- `ApprovalActionRow.attachments` is now `ApprovalActionAttachment[]` — the
+  descriptor carries `id` + display `name` + a download `url`, so a consumer can
+  label and open an attachment without needing read access to the system
+  `sys_file` object (which regular approvers do not have). A bare-string fileId
+  still normalizes to `{ id }` for safety.
+- The decision *input* (`ApprovalDecisionInput.attachments`) is unchanged — it
+  still takes fileId strings. Only the read shape changed.

.changeset/storage-download-filename.md

@@ -0,0 +1,22 @@
+---
+"@objectstack/spec": patch
+"@objectstack/service-storage": patch
+---
+
+fix(storage): downloads carry the real filename + content-type, not the URL token (#3504)
+
+A presigned download served the bytes as `application/octet-stream` with no
+`Content-Disposition`, so a browser saved the file under the opaque URL token
+(e.g. `eyJrIjoiYXR0YWNo…`) instead of its real name — an approval's
+`signed-contract.pdf` downloaded as a nameless blob.
+
+- `IStorageService.getSignedUrl` / `getPresignedDownload` take an optional
+  `PresignedDownloadOptions` (`filename`, `contentType`, `disposition`).
+- The REST download routes (`GET /storage/files/:id/url` and `/:id`) pass the
+  `sys_file` record's `name` + `mime_type`.
+- The local adapter carries them in the signed token; the `_local/raw` route
+  emits `Content-Type` + an RFC 5987 `Content-Disposition` (ASCII fallback +
+  `filename*=UTF-8''…` for non-ASCII names). The S3 adapter bakes the same into
+  the signed URL via `ResponseContentType` / `ResponseContentDisposition`.
+- Default disposition is `inline`, so previewable types (PDF, images) still open
+  in the browser — now with the correct name when saved.

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

@@ -1688,14 +1688,36 @@ describe('ApprovalService — decision_progress & deep links (#2678 P1.5)', () =
 // listActions must surface decision attachments through the contract mapping
 // (#3266 — the column existed but rowFromAction dropped it; caught in browser).
 describe('ApprovalService — listActions attachments mapping (#3266)', () => {
-  it('returns the attachments recorded on a decision', async () => {
+  it('normalizes a bare fileId string into an attachment descriptor', async () => {
     const engine = makeFakeEngine();
     let n = 0;
     const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
     const req = await svc.openNodeRequest(openInput(['u9']), CTX);
     await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9', attachments: ['file_a'] }, SYS);
     const acts = await svc.listActions(req.id, SYS);
     const approve = acts.find(a => a.action === 'approve');
-    expect(approve?.attachments).toEqual(['file_a']);
+    expect(approve?.attachments).toEqual([{ id: 'file_a' }]);
+  });
+
+  // The real column value: `attachments` is a `Field.file` (multiple), which
+  // stores rich descriptors — NOT fileId strings. The old `.map(String)` turned
+  // each into "[object Object]", so the inbox chip had no name and 404'd on open.
+  // The mapping must pass the descriptor (id + name + url) through unmangled.
+  it('passes a stored file descriptor object through with its name and url', async () => {
+    const engine = makeFakeEngine();
+    let n = 0;
+    const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
+    // Simulate the Field.file column's real shape (what the engine returns).
+    const row = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
+    row.attachments = [
+      { id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
+    ];
+    const acts = await svc.listActions(req.id, SYS);
+    const approve = acts.find(a => a.action === 'approve');
+    expect(approve?.attachments).toEqual([
+      { id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
+    ]);
   });
 });

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

@@ -17,6 +17,7 @@ import type {
   IApprovalService,
   ApprovalRequestRow,
   ApprovalActionRow,
+  ApprovalActionAttachment,
   ApprovalDecisionInput,
   ApprovalDecisionResult,
   ApprovalRecallInput,
@@ -203,7 +204,42 @@ function slaDueAt(createdAt: unknown, cfg: any): string | undefined {
   return new Date(t + hours * 3600_000).toISOString();
 }
 
+/**
+ * Normalize one raw `attachments` entry into an {@link ApprovalActionAttachment}.
+ *
+ * The `sys_approval_action.attachments` file field stores **rich descriptors**
+ * (`{ id, name, url, mimeType, size }`), not bare fileId strings — even when the
+ * decision was recorded with a fileId, the field resolves it on write. The
+ * original mapping did `String(entry)`, which turned each descriptor object into
+ * the literal `"[object Object]"` — so the inbox timeline showed a nameless,
+ * un-openable attachment chip (#3266 follow-up; caught by browser verification).
+ * We pass the descriptor through, tolerating a bare-string fileId for safety.
+ */
+function normalizeActionAttachment(entry: any): ApprovalActionAttachment | undefined {
+  if (entry == null) return undefined;
+  if (typeof entry === 'string') {
+    const id = entry.trim();
+    return id ? { id } : undefined;
+  }
+  if (typeof entry === 'object') {
+    const id = entry.id ?? entry.fileId ?? entry.file_id;
+    if (id == null || String(id) === '') return undefined;
+    const mimeType = entry.mimeType ?? entry.mime_type;
+    return {
+      id: String(id),
+      name: typeof entry.name === 'string' ? entry.name : undefined,
+      url: typeof entry.url === 'string' ? entry.url : undefined,
+      mimeType: typeof mimeType === 'string' ? mimeType : undefined,
+      size: typeof entry.size === 'number' ? entry.size : undefined,
+    };
+  }
+  return undefined;
+}
+
 function rowFromAction(row: any): ApprovalActionRow {
+  const attachments = Array.isArray(row.attachments)
+    ? row.attachments.map(normalizeActionAttachment).filter((a: ApprovalActionAttachment | undefined): a is ApprovalActionAttachment => !!a)
+    : [];
   return {
     id: String(row.id),
     request_id: String(row.request_id),
@@ -212,10 +248,9 @@ function rowFromAction(row: any): ApprovalActionRow {
     action: row.action,
     actor_id: row.actor_id ?? undefined,
     comment: row.comment ?? undefined,
-    // Decision attachments (#3266). The column shipped in #3268 but this
-    // contract mapping didn't — the raw engine row carried the fileIds while
-    // every consumer of listActions saw none (caught by browser verification).
-    attachments: Array.isArray(row.attachments) && row.attachments.length ? row.attachments.map(String) : undefined,
+    // Decision attachments (#3266): rich descriptors carrying the display name +
+    // download URL, so consumers label/open them without reading `sys_file`.
+    attachments: attachments.length ? attachments : undefined,
     created_at: row.created_at ?? undefined,
   };
 }

packages/services/service-storage/src/content-disposition.test.ts

@@ -0,0 +1,30 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect } from 'vitest';
+import { contentDispositionValue } from './content-disposition.js';
+
+describe('contentDispositionValue', () => {
+  it('defaults to inline and emits both filename and filename*', () => {
+    expect(contentDispositionValue('signed-contract.pdf')).toBe(
+      "inline; filename=\"signed-contract.pdf\"; filename*=UTF-8''signed-contract.pdf",
+    );
+  });
+
+  it('honors an attachment disposition', () => {
+    expect(contentDispositionValue('report.csv', 'attachment')).toBe(
+      "attachment; filename=\"report.csv\"; filename*=UTF-8''report.csv",
+    );
+  });
+
+  it('keeps a non-ASCII name in filename* and sanitizes the ASCII fallback', () => {
+    const v = contentDispositionValue('季度报告.pdf');
+    // ASCII fallback replaces each of the 4 non-ascii chars; filename* preserves them.
+    expect(v).toContain('filename="____.pdf"');
+    expect(v).toContain("filename*=UTF-8''%E5%AD%A3%E5%BA%A6%E6%8A%A5%E5%91%8A.pdf");
+  });
+
+  it('neutralizes quotes/backslashes in the ASCII fallback (no header injection)', () => {
+    const v = contentDispositionValue('a"b\\c.txt');
+    expect(v).toContain('filename="a_b_c.txt"');
+  });
+});

packages/services/service-storage/src/content-disposition.ts

@@ -0,0 +1,22 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Build a `Content-Disposition` header value that survives non-ASCII filenames.
+ *
+ * Emits both a sanitized ASCII `filename=` (legacy fallback) and the RFC 5987
+ * `filename*=UTF-8''…` form that modern browsers prefer — so a download saves
+ * under its real name instead of the opaque URL token. Used by the local
+ * adapter's `_local/raw` route (header) and the S3 adapter's
+ * `ResponseContentDisposition` (baked into the signed URL).
+ */
+export function contentDispositionValue(
+  filename: string,
+  type: 'inline' | 'attachment' = 'inline',
+): string {
+  const asciiFallback = filename.replace(/[^\x20-\x7e]/g, '_').replace(/["\\]/g, '_');
+  const encoded = encodeURIComponent(filename).replace(
+    /['()*]/g,
+    (c) => '%' + c.charCodeAt(0).toString(16).toUpperCase(),
+  );
+  return `${type}; filename="${asciiFallback}"; filename*=UTF-8''${encoded}`;
+}

packages/services/service-storage/src/local-storage-adapter.test.ts

@@ -151,6 +151,21 @@ describe('LocalStorageAdapter', () => {
       expect(desc.downloadUrl).toContain('/_local/raw/');
       expect(desc.expiresIn).toBe(60);
     });
+
+    it('carries filename + content-type into the download token (so the browser saves the real name)', async () => {
+      await createTempDir();
+      await adapter.upload('attachments/abc.pdf', Buffer.from('%PDF-'));
+      const desc = await adapter.getPresignedDownload!('attachments/abc.pdf', 60, {
+        filename: 'signed-contract.pdf',
+        contentType: 'application/pdf',
+        disposition: 'inline',
+      });
+      const token = desc.downloadUrl.split('/_local/raw/')[1];
+      const payload = adapter.verifyToken(token, 'get');
+      expect(payload.n).toBe('signed-contract.pdf');
+      expect(payload.ct).toBe('application/pdf');
+      expect(payload.d).toBe('inline');
+    });
   });
 
   // =========================================================================

packages/services/service-storage/src/local-storage-adapter.ts

@@ -14,6 +14,7 @@ import type {
   StorageFileInfo,
   PresignedUploadDescriptor,
   PresignedDownloadDescriptor,
+  PresignedDownloadOptions,
 } from '@objectstack/spec/contracts';
 
 /**
@@ -48,6 +49,8 @@ export interface LocalStorageAdapterOptions {
 interface PresignTokenPayload {
   k: string;       // storage key
   ct?: string;     // content-type
+  n?: string;      // download filename (Content-Disposition)
+  d?: 'inline' | 'attachment'; // disposition type (default 'inline')
   exp: number;     // expiry epoch seconds
   op: 'put' | 'get';
 }
@@ -268,17 +271,31 @@ export class LocalStorageAdapter implements IStorageService {
     };
   }
 
-  async getPresignedDownload(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor> {
+  async getPresignedDownload(
+    key: string,
+    expiresIn: number,
+    options?: PresignedDownloadOptions,
+  ): Promise<PresignedDownloadDescriptor> {
     const exp = Math.floor(Date.now() / 1000) + Math.max(1, expiresIn);
-    const token = this.signToken({ k: key, exp, op: 'get' });
+    // Carry filename + content-type in the token so the `_local/raw` route can
+    // emit a real Content-Disposition / Content-Type (else the browser saves
+    // the file under the opaque token, as `application/octet-stream`).
+    const token = this.signToken({
+      k: key,
+      exp,
+      op: 'get',
+      ct: options?.contentType,
+      n: options?.filename,
+      d: options?.disposition,
+    });
     return {
       downloadUrl: `${this.baseUrl}${this.basePath}/_local/raw/${token}`,
       expiresIn,
     };
   }
 
-  async getSignedUrl(key: string, expiresIn: number): Promise<string> {
-    const desc = await this.getPresignedDownload(key, expiresIn);
+  async getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string> {
+    const desc = await this.getPresignedDownload(key, expiresIn, options);
     return desc.downloadUrl;
   }
 

packages/services/service-storage/src/s3-storage-adapter.ts

@@ -11,7 +11,9 @@ import type {
   StorageFileInfo,
   PresignedUploadDescriptor,
   PresignedDownloadDescriptor,
+  PresignedDownloadOptions,
 } from '@objectstack/spec/contracts';
+import { contentDispositionValue } from './content-disposition.js';
 
 /**
  * Configuration for the S3 storage adapter.
@@ -227,8 +229,8 @@ export class S3StorageAdapter implements IStorageService {
   // Presigned URLs
   // ---------------------------------------------------------------------------
 
-  async getSignedUrl(key: string, expiresIn: number): Promise<string> {
-    const desc = await this.getPresignedDownload(key, expiresIn);
+  async getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string> {
+    const desc = await this.getPresignedDownload(key, expiresIn, options);
     return desc.downloadUrl;
   }
 
@@ -256,11 +258,24 @@ export class S3StorageAdapter implements IStorageService {
     };
   }
 
-  async getPresignedDownload(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor> {
+  async getPresignedDownload(
+    key: string,
+    expiresIn: number,
+    options?: PresignedDownloadOptions,
+  ): Promise<PresignedDownloadDescriptor> {
     const client = await this.getClient();
     const s3 = await this.s3Mod();
     const { getSignedUrl } = await this.presignerMod();
-    const cmd = new s3.GetObjectCommand({ Bucket: this.bucket, Key: key });
+    // S3 bakes these response overrides into the signed URL, so the download
+    // carries the real filename + type instead of the object key + octet-stream.
+    const cmd = new s3.GetObjectCommand({
+      Bucket: this.bucket,
+      Key: key,
+      ...(options?.contentType ? { ResponseContentType: options.contentType } : {}),
+      ...(options?.filename
+        ? { ResponseContentDisposition: contentDispositionValue(options.filename, options.disposition ?? 'inline') }
+        : {}),
+    });
     const url = await getSignedUrl(client, cmd, { expiresIn });
     return { downloadUrl: url, expiresIn };
   }

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

@@ -4,6 +4,7 @@ import { randomUUID } from 'node:crypto';
 import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from '@objectstack/spec/contracts';
 import type { StorageMetadataStore, FileRecord } from './metadata-store.js';
 import type { LocalStorageAdapter } from './local-storage-adapter.js';
+import { contentDispositionValue } from './content-disposition.js';
 
 /** Authorization verdict for an attachments-scope download (#2970 item 2). */
 export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated';
@@ -495,12 +496,13 @@ export function registerStorageRoutes(
       const ttl = await authorizeDownload(file, req, res);
       if (ttl === false) return;
 
+      const downloadOpts = { filename: file.name, contentType: file.mime_type };
       let url: string;
       if (storage.getPresignedDownload) {
-        const desc = await storage.getPresignedDownload(file.key, ttl);
+        const desc = await storage.getPresignedDownload(file.key, ttl, downloadOpts);
         url = desc.downloadUrl;
       } else if (storage.getSignedUrl) {
-        url = await storage.getSignedUrl(file.key, ttl);
+        url = await storage.getSignedUrl(file.key, ttl, downloadOpts);
       } else {
         url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`;
       }
@@ -534,12 +536,13 @@ export function registerStorageRoutes(
       const ttl = await authorizeDownload(file, req, res);
       if (ttl === false) return;
 
+      const downloadOpts = { filename: file.name, contentType: file.mime_type };
       let url: string;
       if (storage.getPresignedDownload) {
-        const desc = await storage.getPresignedDownload(file.key, ttl);
+        const desc = await storage.getPresignedDownload(file.key, ttl, downloadOpts);
         url = desc.downloadUrl;
       } else if (storage.getSignedUrl) {
-        url = await storage.getSignedUrl(file.key, ttl);
+        url = await storage.getSignedUrl(file.key, ttl, downloadOpts);
       } else {
         url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`;
       }
@@ -598,6 +601,11 @@ export function registerStorageRoutes(
 
       res.header('content-type', payload.ct ?? 'application/octet-stream');
       res.header('content-length', String(data.byteLength));
+      // When the token carries the original filename, advertise it so the
+      // browser saves the file under its real name (not the opaque URL token).
+      if (payload.n) {
+        res.header('content-disposition', contentDispositionValue(payload.n, payload.d ?? 'inline'));
+      }
       res.send(data);
     } catch (err: any) {
       const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500;