fix(daemon): avoid custom attachment upload header - #9567
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the quick follow-up! Template: complete ✓ Problem: real, and verifiable in the code as it stands. The daemon's CORS allowlist middleware ( Direction: aligned. Fixing the wire protocol before it ships in a release is exactly the right moment — since the SDK and daemon are published together and #9477 is unreleased, there is no compatibility boundary to protect, and the PR correctly avoids carrying any legacy-path logic. Size: 23 production lines across two files (daemon route + SDK client) plus 36 lines of tests. No core-module paths ( Approach: scope feels right — every edit serves the stated goal. The main alternative I considered is adding Risk: no elevated risk signals — none of the changed files match the revert-history high-risk paths. Moving on to code review. 🔍 中文说明感谢这个及时的后续修复! 模板:完整 ✓ 问题:真实存在,且可以在现有代码中直接验证。daemon 的 CORS 白名单中间件( 方向:对齐。在协议随版本发布之前修掉它正是正确的时机——SDK 与 daemon 一起发布,且 #9477 尚未发包,不存在需要保护的兼容边界,PR 也正确地没有保留任何旧协议兼容逻辑。 规模:两个文件共 23 行生产代码(daemon 路由 + SDK 客户端),另有 36 行测试。未触及核心模块路径( 方案:范围合理——每一处改动都服务于目标。我考虑过的主要替代方案是把 风险:无升级风险信号——改动文件均未命中 revert 历史高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
chiga0
left a comment
There was a problem hiding this comment.
Code Review — PR #9567
HEAD: 7b23df9f | +26/-33, 4 files | follow-up to #9477
No blocking findings. Protocol change is correct and well-tested.
CI disclosure: Test (ubuntu) / Serve A/B / Real daemon E2E still pending at review time. All completed checks green, no failures.
What was verified
Encoding symmetry (SDK to server)
SDK (DaemonClient.ts): urlEncode(name) appended as ?name=.... Test confirms image 你好.png encodes to image%20%E4%BD%A0%E5%A5%BD.png (standard encodeURIComponent). Server (session.ts): reads req.query['name'] — Express/qs auto-decodes percent-encoded UTF-8. No manual decodeURIComponent needed; removing it is correct. Old and new paths are equivalent.
Repeated / missing name parameter
query({ name: ['one.txt','two.txt'] }) — Express gives array — typeof name !== 'string' — 400. Missing param — undefined — same guard — 400. Updated test covers the query-specific rejection correctly.
safeAttachmentName still called downstream
Diff only touches the name-extraction site. Downstream storage and validation code is not modified, consistent with stated scope.
CORS motivation is correct
The custom header triggered an OPTIONS preflight. Removing it reduces preflight requirements. Query parameters carry no additional preflight cost.
12 server tests + 1 SDK test — all mechanical substitutions, no logic gaps.
Reviewed with AI assistance.
chiga0
left a comment
There was a problem hiding this comment.
LGTM (AI Generated)
Code verified clean in the prior review comment. Encoding symmetry correct, error handling correct, no functional changes to storage or validation.
Note: Test (ubuntu) / Serve A/B still pending at approve time — no failures observed in completed checks.
✅ Approve
Code reviewI came into this expecting the obvious alternative — just add The mechanics check out:
One non-blocking nuance: the old route returned a dedicated 400 ( No blockers found. Test evidenceUnattended CI run — this review quotes the PR's own CI results on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The route-level supertest coverage pins the new server contract (encoded non-ASCII names, repeated-parameter rejection, case-insensitive path, 413/400 paths) and the SDK test pins the client side, so a green run would genuinely exercise the change rather than pass around it. Sandboxed verification would settle the remaining gap: 中文说明代码审查我最初以为显而易见的替代方案是把 机制核对无误:
一个非阻塞的细节:旧路由对无法解码的百分号序列(如 未发现阻塞问题。 测试证据无人值守 CI 运行——本审查引用该 PR 自身在 路由级 supertest 覆盖固定了新的服务端契约(编码后的非 ASCII 文件名、重复参数拒绝、大小写不敏感路径、413/400 分支),SDK 测试固定了客户端契约,因此全绿的运行是真正在执行该改动,而不是绕过它。 沙箱验证可以补齐剩余缺口: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal protocol fix for a defect I verified in the base code; one non-blocking semantic nuance noted. This one earns its merge. The problem is real and demonstrable without taking the author's word for it: the CORS allowlist simply never permitted The 4 rather than 5: the old route's dedicated 400 for undecodable percent sequences is gone (such names now pass through literally, still sanitized downstream). Harmless, and arguably fine to lose, but it is a small semantic change the PR text doesn't call out. No action needed on it. Approval is deferred until CI lands green on 中文说明置信度:4/5 —— 针对一个我在 base 代码中亲自验证过的缺陷,给出了干净、最小化的协议修复;仅有一个非阻塞的语义细节。 这个 PR 值得合并。问题真实存在,且无需用作者的话来证明:CORS 允许列表从未允许 给 4 分而不是 5 分的原因:旧路由对无法解码的百分号序列有专门的 400,现在没有了(这类名字会按字面值通过,下游仍会清洗)。无害,甚至可以说丢掉也无妨,但这是 PR 描述未提及的一处小语义变化,无需处理。 审批将推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
|
Released in v0.21.15. |
What this PR does
This is a follow-up fix for #9477. It sends the attachment filename as an encoded upload query parameter instead of a custom request header, while keeping the raw file body, MIME type, size limits, duplicate-name handling, and attachment references unchanged.
Why it's needed
Putting the filename in a custom header may cause cross-origin issues. Using the upload URL for this metadata avoids requiring that custom header while preserving the existing attachment behavior.
Reviewer Test Plan
How to verify
Upload a file with spaces or non-ASCII characters in its name from Web Shell. Confirm that the upload succeeds, the request URL contains the encoded filename, the removed custom header is absent, and the resulting attachment can still be referenced and previewed. Also confirm that a missing or repeated filename parameter is rejected.
Evidence (Before & After)
Before: attachment uploads used a custom filename header that could cause a cross-origin preflight failure.
After: the SDK request-contract test passes with an encoded filename query parameter and no custom filename header; all 12 daemon attachment-route tests pass.
Tested on
Environment (optional)
Node.js 22; focused SDK and daemon route tests, package typechecks, lint, formatting, and the full repository build.
Risk & Scope
Linked Issues
Follow-up to #9477.
中文说明
本 PR 做了什么
这是 #9477 的后续修复。附件文件名不再通过自定义请求 header 发送,而是作为经过编码的上传 query 参数传递;文件原始二进制内容、MIME 类型、大小限制、同名处理和附件引用行为均保持不变。
为什么需要
将文件名放在自定义 header 中可能会存在跨域问题。改为通过上传 URL 传递该元数据,可以移除这个自定义 header,同时保留现有附件行为。
Reviewer 测试计划
如何验证
在 Web Shell 中上传一个文件名包含空格或非 ASCII 字符的文件。确认上传成功、请求 URL 包含编码后的文件名、不再携带已移除的自定义文件名 header,并且生成的附件仍可引用和预览。同时确认缺少文件名参数或重复传递文件名参数时请求会被拒绝。
Before & After 证据
Before:附件上传使用自定义文件名 header,可能导致跨域预检失败。
After:SDK 请求协议测试通过,确认使用编码后的文件名 query 参数且不再发送自定义文件名 header;daemon 的 12 个附件路由测试全部通过。
已测试平台
环境
Node.js 22;已运行 SDK 与 daemon 定向测试、包级 typecheck、lint、格式检查和完整仓库构建。
风险与范围
关联
#9477 的后续修复。