Skip to content

Commit 2ab0656

Browse files
baozhoutaoclaude
andcommitted
fix(approvals+storage): surface decision attachments with name, open, and correct download filename (#3504)
The approval inbox timeline showed a nameless "附件" chip that did nothing on click, and even the downloaded file was named after the opaque URL token. Two root causes, both in the framework: approvals — `sys_approval_action.attachments` (a `Field.file`) stores rich descriptors `{ id, name, url, mimeType, size }`, but `rowFromAction` mapped them with `.map(String)`, collapsing each to "[object Object]". `ApprovalActionRow. attachments` is now `ApprovalActionAttachment[]`; the descriptor carries name + url, so consumers label/open attachments without reading the system `sys_file`. storage — presigned downloads served `application/octet-stream` with no `Content-Disposition`. `getSignedUrl`/`getPresignedDownload` now take `PresignedDownloadOptions { filename, contentType, disposition }`; the REST download routes pass the `sys_file` name+mime; the local adapter carries them in the token and `_local/raw` emits an RFC 5987 Content-Disposition; S3 bakes the same into the signed URL. Default `inline` preserves in-browser preview. Verified end-to-end in app-showcase. Refs objectui #2820. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 879ea13 commit 2ab0656

13 files changed

Lines changed: 269 additions & 26 deletions
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/plugin-approvals": patch
4+
---
5+
6+
fix(approvals): return decision attachments as file descriptors, not "[object Object]" (#3504)
7+
8+
The `sys_approval_action.attachments` file field stores rich descriptors
9+
(`{ id, name, url, mimeType, size }`), not bare fileId strings — a fileId passed
10+
to the decision is resolved to a full descriptor on write. But `rowFromAction`
11+
mapped the column with `.map(String)`, collapsing each descriptor object to the
12+
literal string `"[object Object]"`. Every `listActions` consumer (the approval
13+
inbox timeline) then received garbage: the attachment chip had no filename and
14+
its id was `"[object Object]"`, so opening it 404'd.
15+
16+
- `ApprovalActionRow.attachments` is now `ApprovalActionAttachment[]` — the
17+
descriptor carries `id` + display `name` + a download `url`, so a consumer can
18+
label and open an attachment without needing read access to the system
19+
`sys_file` object (which regular approvers do not have). A bare-string fileId
20+
still normalizes to `{ id }` for safety.
21+
- The decision *input* (`ApprovalDecisionInput.attachments`) is unchanged — it
22+
still takes fileId strings. Only the read shape changed.
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/service-storage": patch
4+
---
5+
6+
fix(storage): downloads carry the real filename + content-type, not the URL token (#3504)
7+
8+
A presigned download served the bytes as `application/octet-stream` with no
9+
`Content-Disposition`, so a browser saved the file under the opaque URL token
10+
(e.g. `eyJrIjoiYXR0YWNo…`) instead of its real name — an approval's
11+
`signed-contract.pdf` downloaded as a nameless blob.
12+
13+
- `IStorageService.getSignedUrl` / `getPresignedDownload` take an optional
14+
`PresignedDownloadOptions` (`filename`, `contentType`, `disposition`).
15+
- The REST download routes (`GET /storage/files/:id/url` and `/:id`) pass the
16+
`sys_file` record's `name` + `mime_type`.
17+
- The local adapter carries them in the signed token; the `_local/raw` route
18+
emits `Content-Type` + an RFC 5987 `Content-Disposition` (ASCII fallback +
19+
`filename*=UTF-8''…` for non-ASCII names). The S3 adapter bakes the same into
20+
the signed URL via `ResponseContentType` / `ResponseContentDisposition`.
21+
- Default disposition is `inline`, so previewable types (PDF, images) still open
22+
in the browser — now with the correct name when saved.

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1948,14 +1948,36 @@ describe('ApprovalService — decision_progress & deep links (#2678 P1.5)', () =
19481948
// listActions must surface decision attachments through the contract mapping
19491949
// (#3266 — the column existed but rowFromAction dropped it; caught in browser).
19501950
describe('ApprovalService — listActions attachments mapping (#3266)', () => {
1951-
it('returns the attachments recorded on a decision', async () => {
1951+
it('normalizes a bare fileId string into an attachment descriptor', async () => {
19521952
const engine = makeFakeEngine();
19531953
let n = 0;
19541954
const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
19551955
const req = await svc.openNodeRequest(openInput(['u9']), CTX);
19561956
await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9', attachments: ['file_a'] }, SYS);
19571957
const acts = await svc.listActions(req.id, SYS);
19581958
const approve = acts.find(a => a.action === 'approve');
1959-
expect(approve?.attachments).toEqual(['file_a']);
1959+
expect(approve?.attachments).toEqual([{ id: 'file_a' }]);
1960+
});
1961+
1962+
// The real column value: `attachments` is a `Field.file` (multiple), which
1963+
// stores rich descriptors — NOT fileId strings. The old `.map(String)` turned
1964+
// each into "[object Object]", so the inbox chip had no name and 404'd on open.
1965+
// The mapping must pass the descriptor (id + name + url) through unmangled.
1966+
it('passes a stored file descriptor object through with its name and url', async () => {
1967+
const engine = makeFakeEngine();
1968+
let n = 0;
1969+
const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
1970+
const req = await svc.openNodeRequest(openInput(['u9']), CTX);
1971+
await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
1972+
// Simulate the Field.file column's real shape (what the engine returns).
1973+
const row = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
1974+
row.attachments = [
1975+
{ id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
1976+
];
1977+
const acts = await svc.listActions(req.id, SYS);
1978+
const approve = acts.find(a => a.action === 'approve');
1979+
expect(approve?.attachments).toEqual([
1980+
{ id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
1981+
]);
19601982
});
19611983
});

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

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type {
1919
IApprovalService,
2020
ApprovalRequestRow,
2121
ApprovalActionRow,
22+
ApprovalActionAttachment,
2223
ApprovalDecisionInput,
2324
ApprovalDecisionResult,
2425
ApprovalRecallInput,
@@ -255,7 +256,42 @@ function slaDueAt(createdAt: unknown, cfg: any): string | undefined {
255256
return new Date(t + hours * 3600_000).toISOString();
256257
}
257258

259+
/**
260+
* Normalize one raw `attachments` entry into an {@link ApprovalActionAttachment}.
261+
*
262+
* The `sys_approval_action.attachments` file field stores **rich descriptors**
263+
* (`{ id, name, url, mimeType, size }`), not bare fileId strings — even when the
264+
* decision was recorded with a fileId, the field resolves it on write. The
265+
* original mapping did `String(entry)`, which turned each descriptor object into
266+
* the literal `"[object Object]"` — so the inbox timeline showed a nameless,
267+
* un-openable attachment chip (#3266 follow-up; caught by browser verification).
268+
* We pass the descriptor through, tolerating a bare-string fileId for safety.
269+
*/
270+
function normalizeActionAttachment(entry: any): ApprovalActionAttachment | undefined {
271+
if (entry == null) return undefined;
272+
if (typeof entry === 'string') {
273+
const id = entry.trim();
274+
return id ? { id } : undefined;
275+
}
276+
if (typeof entry === 'object') {
277+
const id = entry.id ?? entry.fileId ?? entry.file_id;
278+
if (id == null || String(id) === '') return undefined;
279+
const mimeType = entry.mimeType ?? entry.mime_type;
280+
return {
281+
id: String(id),
282+
name: typeof entry.name === 'string' ? entry.name : undefined,
283+
url: typeof entry.url === 'string' ? entry.url : undefined,
284+
mimeType: typeof mimeType === 'string' ? mimeType : undefined,
285+
size: typeof entry.size === 'number' ? entry.size : undefined,
286+
};
287+
}
288+
return undefined;
289+
}
290+
258291
function rowFromAction(row: any): ApprovalActionRow {
292+
const attachments = Array.isArray(row.attachments)
293+
? row.attachments.map(normalizeActionAttachment).filter((a: ApprovalActionAttachment | undefined): a is ApprovalActionAttachment => !!a)
294+
: [];
259295
return {
260296
id: String(row.id),
261297
request_id: String(row.request_id),
@@ -264,10 +300,9 @@ function rowFromAction(row: any): ApprovalActionRow {
264300
action: row.action,
265301
actor_id: row.actor_id ?? undefined,
266302
comment: row.comment ?? undefined,
267-
// Decision attachments (#3266). The column shipped in #3268 but this
268-
// contract mapping didn't — the raw engine row carried the fileIds while
269-
// every consumer of listActions saw none (caught by browser verification).
270-
attachments: Array.isArray(row.attachments) && row.attachments.length ? row.attachments.map(String) : undefined,
303+
// Decision attachments (#3266): rich descriptors carrying the display name +
304+
// download URL, so consumers label/open them without reading `sys_file`.
305+
attachments: attachments.length ? attachments : undefined,
271306
created_at: row.created_at ?? undefined,
272307
};
273308
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { contentDispositionValue } from './content-disposition.js';
5+
6+
describe('contentDispositionValue', () => {
7+
it('defaults to inline and emits both filename and filename*', () => {
8+
expect(contentDispositionValue('signed-contract.pdf')).toBe(
9+
"inline; filename=\"signed-contract.pdf\"; filename*=UTF-8''signed-contract.pdf",
10+
);
11+
});
12+
13+
it('honors an attachment disposition', () => {
14+
expect(contentDispositionValue('report.csv', 'attachment')).toBe(
15+
"attachment; filename=\"report.csv\"; filename*=UTF-8''report.csv",
16+
);
17+
});
18+
19+
it('keeps a non-ASCII name in filename* and sanitizes the ASCII fallback', () => {
20+
const v = contentDispositionValue('季度报告.pdf');
21+
// ASCII fallback replaces each of the 4 non-ascii chars; filename* preserves them.
22+
expect(v).toContain('filename="____.pdf"');
23+
expect(v).toContain("filename*=UTF-8''%E5%AD%A3%E5%BA%A6%E6%8A%A5%E5%91%8A.pdf");
24+
});
25+
26+
it('neutralizes quotes/backslashes in the ASCII fallback (no header injection)', () => {
27+
const v = contentDispositionValue('a"b\\c.txt');
28+
expect(v).toContain('filename="a_b_c.txt"');
29+
});
30+
});
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Build a `Content-Disposition` header value that survives non-ASCII filenames.
5+
*
6+
* Emits both a sanitized ASCII `filename=` (legacy fallback) and the RFC 5987
7+
* `filename*=UTF-8''…` form that modern browsers prefer — so a download saves
8+
* under its real name instead of the opaque URL token. Used by the local
9+
* adapter's `_local/raw` route (header) and the S3 adapter's
10+
* `ResponseContentDisposition` (baked into the signed URL).
11+
*/
12+
export function contentDispositionValue(
13+
filename: string,
14+
type: 'inline' | 'attachment' = 'inline',
15+
): string {
16+
const asciiFallback = filename.replace(/[^\x20-\x7e]/g, '_').replace(/["\\]/g, '_');
17+
const encoded = encodeURIComponent(filename).replace(
18+
/['()*]/g,
19+
(c) => '%' + c.charCodeAt(0).toString(16).toUpperCase(),
20+
);
21+
return `${type}; filename="${asciiFallback}"; filename*=UTF-8''${encoded}`;
22+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,21 @@ describe('LocalStorageAdapter', () => {
151151
expect(desc.downloadUrl).toContain('/_local/raw/');
152152
expect(desc.expiresIn).toBe(60);
153153
});
154+
155+
it('carries filename + content-type into the download token (so the browser saves the real name)', async () => {
156+
await createTempDir();
157+
await adapter.upload('attachments/abc.pdf', Buffer.from('%PDF-'));
158+
const desc = await adapter.getPresignedDownload!('attachments/abc.pdf', 60, {
159+
filename: 'signed-contract.pdf',
160+
contentType: 'application/pdf',
161+
disposition: 'inline',
162+
});
163+
const token = desc.downloadUrl.split('/_local/raw/')[1];
164+
const payload = adapter.verifyToken(token, 'get');
165+
expect(payload.n).toBe('signed-contract.pdf');
166+
expect(payload.ct).toBe('application/pdf');
167+
expect(payload.d).toBe('inline');
168+
});
154169
});
155170

156171
// =========================================================================

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {
1414
StorageFileInfo,
1515
PresignedUploadDescriptor,
1616
PresignedDownloadDescriptor,
17+
PresignedDownloadOptions,
1718
} from '@objectstack/spec/contracts';
1819

1920
/**
@@ -48,6 +49,8 @@ export interface LocalStorageAdapterOptions {
4849
interface PresignTokenPayload {
4950
k: string; // storage key
5051
ct?: string; // content-type
52+
n?: string; // download filename (Content-Disposition)
53+
d?: 'inline' | 'attachment'; // disposition type (default 'inline')
5154
exp: number; // expiry epoch seconds
5255
op: 'put' | 'get';
5356
}
@@ -268,17 +271,31 @@ export class LocalStorageAdapter implements IStorageService {
268271
};
269272
}
270273

271-
async getPresignedDownload(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor> {
274+
async getPresignedDownload(
275+
key: string,
276+
expiresIn: number,
277+
options?: PresignedDownloadOptions,
278+
): Promise<PresignedDownloadDescriptor> {
272279
const exp = Math.floor(Date.now() / 1000) + Math.max(1, expiresIn);
273-
const token = this.signToken({ k: key, exp, op: 'get' });
280+
// Carry filename + content-type in the token so the `_local/raw` route can
281+
// emit a real Content-Disposition / Content-Type (else the browser saves
282+
// the file under the opaque token, as `application/octet-stream`).
283+
const token = this.signToken({
284+
k: key,
285+
exp,
286+
op: 'get',
287+
ct: options?.contentType,
288+
n: options?.filename,
289+
d: options?.disposition,
290+
});
274291
return {
275292
downloadUrl: `${this.baseUrl}${this.basePath}/_local/raw/${token}`,
276293
expiresIn,
277294
};
278295
}
279296

280-
async getSignedUrl(key: string, expiresIn: number): Promise<string> {
281-
const desc = await this.getPresignedDownload(key, expiresIn);
297+
async getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string> {
298+
const desc = await this.getPresignedDownload(key, expiresIn, options);
282299
return desc.downloadUrl;
283300
}
284301

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

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ import type {
1111
StorageFileInfo,
1212
PresignedUploadDescriptor,
1313
PresignedDownloadDescriptor,
14+
PresignedDownloadOptions,
1415
} from '@objectstack/spec/contracts';
16+
import { contentDispositionValue } from './content-disposition.js';
1517

1618
/**
1719
* Configuration for the S3 storage adapter.
@@ -227,8 +229,8 @@ export class S3StorageAdapter implements IStorageService {
227229
// Presigned URLs
228230
// ---------------------------------------------------------------------------
229231

230-
async getSignedUrl(key: string, expiresIn: number): Promise<string> {
231-
const desc = await this.getPresignedDownload(key, expiresIn);
232+
async getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise<string> {
233+
const desc = await this.getPresignedDownload(key, expiresIn, options);
232234
return desc.downloadUrl;
233235
}
234236

@@ -256,11 +258,24 @@ export class S3StorageAdapter implements IStorageService {
256258
};
257259
}
258260

259-
async getPresignedDownload(key: string, expiresIn: number): Promise<PresignedDownloadDescriptor> {
261+
async getPresignedDownload(
262+
key: string,
263+
expiresIn: number,
264+
options?: PresignedDownloadOptions,
265+
): Promise<PresignedDownloadDescriptor> {
260266
const client = await this.getClient();
261267
const s3 = await this.s3Mod();
262268
const { getSignedUrl } = await this.presignerMod();
263-
const cmd = new s3.GetObjectCommand({ Bucket: this.bucket, Key: key });
269+
// S3 bakes these response overrides into the signed URL, so the download
270+
// carries the real filename + type instead of the object key + octet-stream.
271+
const cmd = new s3.GetObjectCommand({
272+
Bucket: this.bucket,
273+
Key: key,
274+
...(options?.contentType ? { ResponseContentType: options.contentType } : {}),
275+
...(options?.filename
276+
? { ResponseContentDisposition: contentDispositionValue(options.filename, options.disposition ?? 'inline') }
277+
: {}),
278+
});
264279
const url = await getSignedUrl(client, cmd, { expiresIn });
265280
return { downloadUrl: url, expiresIn };
266281
}

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { randomUUID } from 'node:crypto';
44
import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from '@objectstack/spec/contracts';
55
import type { StorageMetadataStore, FileRecord } from './metadata-store.js';
66
import type { LocalStorageAdapter } from './local-storage-adapter.js';
7+
import { contentDispositionValue } from './content-disposition.js';
78

89
/** Authorization verdict for an attachments-scope download (#2970 item 2). */
910
export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated';
@@ -518,12 +519,13 @@ export function registerStorageRoutes(
518519
const ttl = await authorizeDownload(file, req, res);
519520
if (ttl === false) return;
520521

522+
const downloadOpts = { filename: file.name, contentType: file.mime_type };
521523
let url: string;
522524
if (storage.getPresignedDownload) {
523-
const desc = await storage.getPresignedDownload(file.key, ttl);
525+
const desc = await storage.getPresignedDownload(file.key, ttl, downloadOpts);
524526
url = desc.downloadUrl;
525527
} else if (storage.getSignedUrl) {
526-
url = await storage.getSignedUrl(file.key, ttl);
528+
url = await storage.getSignedUrl(file.key, ttl, downloadOpts);
527529
} else {
528530
url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`;
529531
}
@@ -557,12 +559,13 @@ export function registerStorageRoutes(
557559
const ttl = await authorizeDownload(file, req, res);
558560
if (ttl === false) return;
559561

562+
const downloadOpts = { filename: file.name, contentType: file.mime_type };
560563
let url: string;
561564
if (storage.getPresignedDownload) {
562-
const desc = await storage.getPresignedDownload(file.key, ttl);
565+
const desc = await storage.getPresignedDownload(file.key, ttl, downloadOpts);
563566
url = desc.downloadUrl;
564567
} else if (storage.getSignedUrl) {
565-
url = await storage.getSignedUrl(file.key, ttl);
568+
url = await storage.getSignedUrl(file.key, ttl, downloadOpts);
566569
} else {
567570
url = `${basePath}/_local/file/${encodeURIComponent(file.key)}`;
568571
}
@@ -621,6 +624,11 @@ export function registerStorageRoutes(
621624

622625
res.header('content-type', payload.ct ?? 'application/octet-stream');
623626
res.header('content-length', String(data.byteLength));
627+
// When the token carries the original filename, advertise it so the
628+
// browser saves the file under its real name (not the opaque URL token).
629+
if (payload.n) {
630+
res.header('content-disposition', contentDispositionValue(payload.n, payload.d ?? 'inline'));
631+
}
624632
res.send(data);
625633
} catch (err: any) {
626634
const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500;

0 commit comments

Comments
 (0)