fix(file): stop signing Content-Disposition into presigned upload PUT (#218)

[an9xyz]

问题 (closes #218)

Web 端上传文件名较长且含空格的文件（如 Lincoln 2026 Q2 Lincoln Z Launch Campaign Jun Autohome.xlsx，1.1 MB）时报「上传失败，点击图标重试」；把同一个文件改名为短且无空格的名字（1111.xlsx）即可上传成功。失败只与文件名内容有关，与大小无关。生产存储用的是 Tencent COS。

根因

预签名上传处理器先 contentDisposition := BuildContentDisposition(filename)，再传入 PresignedPutURL。各后端（MinIO/COS/S3 走 minio-go PresignHeader，OSS 走 SignURL）都会把这个值签进预签名 PUT URL 的签名头（X-Amz-SignedHeaders 含 content-disposition）。这让「上传成功」与「浏览器逐字节一致地回传 Content-Disposition」强耦合。对含空格的 ASCII 名字，这个带空格的签名头值在 浏览器 → 代理 → 存储网关 各环节规范化不一致，网关因此拒绝 PUT（403 SignatureDoesNotMatch）。无空格的名字签名值简单、能匹配，所以成功——完全吻合「改名即可」的现象。

注：这不是编码问题——代码里其实已经同时做了引号包裹和 RFC 5987 filename*= 编码。问题是签名头耦合本身。

修复

不再把 Content-Disposition 签进预签名 PUT：在 4 个预签名 handler 调用点统一给 PresignedPutURL 传 ""，并从 JSON 响应里去掉 contentDisposition 字段。

• modules/file/api.go getUploadCredentials
• modules/robot/api.go botUploadPresigned
• modules/botfather/api.go botUploadPresigned
• modules/bot_api/file.go botUploadPresigned

不改 PresignedPutURL 接口或任何后端实现，blast radius 最小。

下载文件名不受影响

友好下载名本就由 GET 阶段下发，且更稳健：

• PresignedGetURL（/v1/file/download）通过 response-content-disposition 查询参数带上友好名——是签名 query 的一部分，不受 header 规范化问题影响。
• COS 桶通常私有，下载必须走 getDownloadURL，因此「裸 downloadUrl 退化成 UUID 名」在 COS 上不会真实发生。

向后兼容

旧/缓存版前端即便仍在 PUT 上带 Content-Disposition，现在它是未签名头，会被 SigV4 与 OSS V1 SignURL 忽略——PUT 照样成功。

保持不变

服务端 multipart UploadFile 路径（服务端自己 PUT，无浏览器耦合）继续使用 BuildContentDisposition，不动。

测试 / 验证

• go build ./...、四个模块 go vet 均通过。
• 相关预签名单测已反转为断言「响应不含 contentDisposition 且 "" 传给 service」，全部通过。
• TestBuildContentDisposition* 保持不变（函数未改，仍守护 multipart 路径）。

建议合并前：用真实 COS 网关跑一遍含空格的 1.1MB 文件，确认预签名 PUT 从 403 变 200。

Commits

• fix(file): stop signing Content-Disposition into presigned upload PUT
• docs(file): update presigned-upload contract for Content-Disposition removal（同步过时的 docstring 与 configs/tsdd.yaml 说明，无行为变更）

https://claude.ai/code/session_01V1ZWGnGbCG7wNj6bADutwa

---

Generated by Claude Code

[Jerry-Xin]

The PR is relevant to octo-server and the code change correctly removes Content-Disposition from the browser presigned PUT contract without touching backend signing implementations.

💬 Non-blocking

🟡 Warning: Some comments now contradict the new contract. In modules/file/api.go, the comments still say filename is used for Content-Disposition and preserved in the upload header, but the handler now passes "" to PresignedPutURL. Similar stale guidance remains in modules/file/service_oss.go, configs/tsdd.yaml, and modules/file/testdata/octo-server.yaml. This is not a merge blocker, but it could mislead future client or backend work.

🔵 Suggestion: modules/bot_api/file_presigned_test.go has a helper comment saying the response echoes the same Content-Disposition; the assertions correctly check the opposite. Worth cleaning up.

✅ Highlights

The changed handlers consistently pass an empty disposition to PresignedPutURL, and the tests assert both response omission and service-call behavior across file, robot, botfather, and bot_api paths.

Verification run:
go test ./modules/file ./modules/robot ./modules/bot_api passed. go test ./modules/botfather hit an existing MySQL-dependent test failure, but the targeted botfather presigned/upload tests passed.

[lml2468]

APPROVED — PR #219 at d48ad87

Summary

Clean bugfix for issue #218: stop signing Content-Disposition into presigned PUT URLs. The SigV4 canonical-headers canonicalization differs across browser/proxy/gateway for whitespace-bearing values, causing 403 SignatureDoesNotMatch on filenames with spaces.

Verification

1. Root cause correct: SigV4 signs headers byte-exactly, but browsers/proxies/gateways canonicalize Content-Disposition whitespace differently → signature mismatch. ✅
2. Fix is complete across all 4 endpoints:
   • modules/file/api.go (main file endpoint) ✅
   • modules/bot_api/file.go (bot API) ✅
   • modules/botfather/api.go (botfather) ✅
   • modules/robot/api.go (robot) ✅
3. Consistent pattern: All 4 pass "" to PresignedPutURL and remove contentDisposition from the response. ✅
4. No download regression: Download filename is already handled at GET time via response-content-disposition query override (PresignedGetURL/DownloadURL). ✅
5. Tests actively assert absence: Not just removed — tests verify contentDisposition is NOT in the response AND empty string is passed to the service. ✅
6. API contract docs updated: configs/tsdd.yaml and modules/file/api.go comments updated to reflect the new contract. ✅
7. Backward compatible: PresignedPutURL interface unchanged, "" argument means storage layer skips the header. ✅
8. Net -27 lines: Pure simplification. ✅

CI Note

• check-sprint fails (linked issue #218 likely needs Sprint assigned on the Octo Board). Administrative, not code-related.
• Build passes. Lint/Vet/Test/CodeQL were pending at review time.

No findings.

[yujiawei]

Code Review — PR #219 (octo-server)

Summary

This PR fixes issue #218 (file uploads failing with 403 SignatureDoesNotMatch for filenames containing spaces) by no longer signing Content-Disposition into the presigned PUT URL. The root-cause analysis is correct and the fix is minimal, consistent, and well-tested. Approving.

Verification

Check	Result	Evidence
Root cause is accurate	✅	On SigV4 backends, a non-empty contentDisposition lands in X-Amz-SignedHeaders, coupling PUT success to a byte-exact header echo. Whitespace-bearing header values canonicalize inconsistently across browser/proxy/gateway → 403. Removing it from the signed set is the correct fix.
All presigned PUT call sites covered	✅	All 4 handlers now pass "": modules/file/api.go:527, modules/robot/api.go:1631, modules/bot_api/file.go:291, modules/botfather/api.go:1413. No other non-test callers of PresignedPutURL exist.
Service interface unchanged	✅	PresignedPutURL/UploadFile signatures in modules/file/service.go are untouched — blast radius is limited to the call sites.
Server-side multipart path preserved	✅	UploadFile callers still build a real disposition (modules/file/api.go:258, modules/robot/api.go:1476, modules/bot_api/file.go:89, modules/botfather/api.go:1155); ServiceCOS.UploadFile still applies it (service_cos.go:259). Server-uploaded objects keep friendly names.
Download filenames preserved	✅	getDownloadURL (modules/file/api.go:548) sets the friendly name via the signed query param response-content-disposition at GET time (service_cos.go:509, service_minio.go:453, service_s3.go:353, service_oss.go:239). This path is unaffected by request-header canonicalization.
Backward compatibility	✅	An old/cached frontend that still sends a Content-Disposition request header is now harmless: it is an unsigned header, ignored by SigV4 (MinIO/COS/S3) and by OSS V1 SignURL. The PUT still succeeds.
Tests updated and passing	✅	The 4 presigned tests are inverted to assert the field is absent and "" is passed to the service. go build, go vet, and the upload/presigned test suites across all 4 modules pass locally.
BuildContentDisposition guarded	✅	Function is unchanged and TestBuildContentDisposition* continues to protect the multipart path.

Findings

No P0/P1 issues. The change is correct and low-risk.

Minor (non-blocking)

• P2 — Stored object metadata for presigned uploads. Objects uploaded via the presigned PUT path will no longer carry stored Content-Disposition metadata. As a result, a raw, non-signed downloadUrl served from a public bucket would download with the UUID object key as the filename rather than the friendly name. This is acceptable: the PR documents it, production COS buckets are private (downloads must go through getDownloadURL, which re-applies the friendly name), and trading a cosmetic raw-URL filename for fixing an upload-breaking 403 is the right call. Worth a line in release notes for any deployment that exposes a public bucket and links the raw downloadUrl directly.

Recommendation

The documentation/comment updates accurately reflect the new contract (the configs/tsdd.yaml matrix and the getUploadCredentials docstring). As the PR author notes, a real-COS smoke test with a spaced ~1.1 MB filename (confirming the PUT flips from 403 → 200) is the right pre/post-merge validation, since the failure only reproduces against an actual gateway.

Verdict: APPROVED — no blocking issues; the single minor item is a documented, acceptable trade-off.

[lml2468]

APPROVED — re-review at 8095e3f

Comment/doc cleanup addressing Jerry-Xin and Allen's non-blocking feedback from the previous round. No logic changes — all diffs are comment/docstring updates:

1. configs/tsdd.yaml — Removed stale Content-Disposition refs from OSS caveat section ✅
2. bot_api/file_presigned_test.go — Mock comment updated to match new behavior ✅
3. file/api.go — path+filename semantics comment no longer references signed disposition ✅
4. file/service_oss.go — PresignedPutURL docstring updated for empty disposition default ✅
5. file/testdata/octo-server.yaml — Test config comments cleaned up ✅

check-sprint now passing. All prior verification carries forward. No new findings.