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

.changeset/approval-attachment-descriptors.md

@@ -0,0 +1,29 @@
+---
+"@objectstack/spec": patch
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): return decision attachments as file values, not "[object Object]" (#3504)
+
+`sys_approval_action.attachments` is a `Field.file`, so the column **stores an
+opaque `sys_file` id** (ADR-0104 D3 — the stored form of every media field). The
+ObjectQL read path resolves that id into its expanded
+`{ id, name, size, mimeType, url }` form on the way out. But `rowFromAction`
+mapped the column with `.map(String)`, collapsing each expanded value 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
+  expanded file value plus its id, 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).
+- Three read forms are accepted: the expanded value (the normal case), a bare id
+  (nothing to expand it into — storage service absent, file not committed), and
+  a legacy inline blob written before file-as-reference (`file_id` /
+  `mime_type`), until the backfill converts it. The id test reuses the
+  platform's `isFileIdToken`, so this and the engine's read resolver cannot
+  disagree about what counts as an id.
+- The decision *input* (`ApprovalDecisionInput.attachments`) is unchanged — it
+  still takes fileId strings, which is also exactly what the column stores. 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.

.claude/launch.json

@@ -18,6 +18,24 @@
         "file:/tmp/showcase-dogfood-3777/data.db"
       ],
       "port": 3777
+    },
+    {
+      "name": "pr3505-attach-3477",
+      "runtimeExecutable": "pnpm",
+      "runtimeArgs": [
+        "-C",
+        "/home/user/objectstack-pr3505/examples/app-showcase",
+        "exec",
+        "objectstack",
+        "dev",
+        "--ui",
+        "--seed-admin",
+        "-p",
+        "3477",
+        "-d",
+        "file:/tmp/pr3505-attach/data.db"
+      ],
+      "port": 3477
     }
   ]
 }

content/docs/kernel/contracts/storage-service.mdx

@@ -29,15 +29,19 @@ export interface IStorageService {
   list?(prefix: string): Promise<StorageFileInfo[]>;
 
   // Signed URL (optional)
-  getSignedUrl?(key: string, expiresIn: number): Promise<string>;
+  getSignedUrl?(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string>;
 
   // Presigned browser-direct upload / download (optional)
   getPresignedUpload?(
     key: string,
     expiresIn: number,
     options?: StorageUploadOptions,
   ): Promise<PresignedUploadDescriptor>;
-  getPresignedDownload?(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor>;
+  getPresignedDownload?(
+    key: string,
+    expiresIn: number,
+    options?: PresignedDownloadOptions,
+  ): Promise<PresignedDownloadDescriptor>;
 
   // Chunked / multipart upload (optional)
   initiateChunkedUpload?(key: string, options?: StorageUploadOptions): Promise<string>;

content/docs/kernel/runtime-services/storage-service.mdx

@@ -17,14 +17,14 @@ services.storage.delete(key: string): Promise<void>
 services.storage.exists(key: string): Promise<boolean>
 services.storage.getInfo(key: string): Promise<StorageFileInfo>
 services.storage.list?(prefix: string): Promise<StorageFileInfo[]>
-services.storage.getSignedUrl?(key: string, expiresIn: number): Promise<string>
+services.storage.getSignedUrl?(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string>
 ```
 
 ## Presigned / Chunked (optional)
 
 ```ts
 services.storage.getPresignedUpload?(key: string, expiresIn: number, options?: StorageUploadOptions): Promise<PresignedUploadDescriptor>
-services.storage.getPresignedDownload?(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor>
+services.storage.getPresignedDownload?(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<PresignedDownloadDescriptor>
 services.storage.initiateChunkedUpload?(key: string, options?: StorageUploadOptions): Promise<string>
 services.storage.uploadChunk?(uploadId: string, partNumber: number, data: Buffer): Promise<string>
 services.storage.completeChunkedUpload?(uploadId: string, parts: Array<{ partNumber: number; eTag: string }>): Promise<string>

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

@@ -1948,14 +1948,57 @@ 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 normal case. The column STORES an opaque sys_file id (ADR-0104 D3);
+  // the ObjectQL read path resolves it into the expanded
+  // `{ id, name, size, mimeType, url }` form on the way out. The old
+  // `.map(String)` turned that object into "[object Object]", so the inbox chip
+  // had no name and 404'd on open. The mapping must pass it through unmangled.
+  it('passes the engine-expanded file value 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 what the read path hands back after resolving the stored id.
+    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' },
+    ]);
+  });
+
+  // Rows written before file-as-reference hold an inline blob whose keys are
+  // snake_case (`file_id`, `mime_type`). They stay readable until the backfill
+  // converts them, so both casings must map — the same drift that made objectui
+  // stop recognising images when the expanded form arrived.
+  it('maps a legacy inline blob (file_id / mime_type) written before the cutover', 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);
+    const row = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
+    row.attachments = [
+      { file_id: 'file_b', name: 'old.pdf', mime_type: 'application/pdf', size: 12, url: 'https://cdn/old.pdf' },
+    ];
+    const acts = await svc.listActions(req.id, SYS);
+    expect(acts.find(a => a.action === 'approve')?.attachments).toEqual([
+      { id: 'file_b', name: 'old.pdf', mimeType: 'application/pdf', size: 12, url: 'https://cdn/old.pdf' },
+    ]);
   });
 });

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

@@ -19,6 +19,7 @@ import type {
   IApprovalService,
   ApprovalRequestRow,
   ApprovalActionRow,
+  ApprovalActionAttachment,
   ApprovalDecisionInput,
   ApprovalDecisionResult,
   ApprovalRecallInput,
@@ -30,6 +31,7 @@ import type {
   ApprovalStatus,
   SharingExecutionContext,
 } from '@objectstack/spec/contracts';
+import { isFileIdToken } from '@objectstack/spec/data';
 import { isGrantActive } from '@objectstack/core';
 
 /**
@@ -255,7 +257,58 @@ 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}.
+ *
+ * `sys_approval_action.attachments` is a `Field.file` (multiple), so the column
+ * **stores opaque `sys_file` ids** — that is the stored form of every media
+ * field (ADR-0104 D3). What arrives here is whichever of three forms the read
+ * path produced:
+ *
+ *  1. the **expanded** `{ id, name, size, mimeType, url }` the ObjectQL read
+ *     path resolves a stored id into — the normal case;
+ *  2. a **bare id**, when there was nothing to expand it into (storage service
+ *     absent, file not committed);
+ *  3. a **legacy inline blob** (`{ file_id, name, mime_type, url, … }`) written
+ *     before file-as-reference, until the backfill converts it.
+ *
+ * The original mapping did `String(entry)`, which turned form 1 into the
+ * literal `"[object Object]"` — so the inbox timeline showed a nameless,
+ * un-openable attachment chip (#3266 follow-up; caught by browser verification).
+ *
+ * Note the casing: the expanded form carries `mimeType`, the legacy blob
+ * `mime_type`. Both are accepted for the duration of the migration window.
+ */
+function normalizeActionAttachment(entry: any): ApprovalActionAttachment | undefined {
+  if (entry == null) return undefined;
+  // Form 2 — a bare reference. `isFileIdToken` is the platform's single arbiter
+  // of "is this string an opaque file id, or a URL?", shared with the engine's
+  // read resolver, so the two cannot disagree about what counts as an id.
+  if (typeof entry === 'string') {
+    const id = entry.trim();
+    if (!id) return undefined;
+    return isFileIdToken(id) ? { id } : { id, url: id };
+  }
+  if (typeof entry === 'object') {
+    // Forms 1 and 3 — `file_id` is the legacy blob's key for the same thing.
+    const id = entry.id ?? 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),
@@ -264,10 +317,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;
   }