media: add shared filename decoder#71517
Conversation
cf186c2 to
4fadbb9
Compare
Greptile SummaryThis PR adds a shared
Confidence Score: 3/5Not safe to merge without addressing the missing path-sanitisation in A P1 security-relevant finding (path traversal not sanitised in Feishu inbound filenames) prevents a high-confidence score. The core
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fadbb92e7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
4fadbb9 to
0be9dc7
Compare
|
Codex review: needs real behavior proof before merge. Latest ClawSweeper review: 2026-05-21 18:17 UTC / May 21, 2026, 2:17 PM ET. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. by source inspection. PR head still lets Feishu JSON download metadata reach storage unchanged before the fallback wrapper runs, and current-main tests pin that mojibake path. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Keep the core decoder direction, but move Feishu recovery/sanitization to the metadata/save boundary with regression tests for JSON metadata, fallback payload names, and valid Latin-1 preservation before merge. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection. PR head still lets Feishu JSON download metadata reach storage unchanged before the fallback wrapper runs, and current-main tests pin that mojibake path. Is this the best way to solve the issue? No, not yet. The shared decoder is a reasonable direction, but the Feishu bug needs recovery at the metadata/save boundary rather than only around the post-download fallback wrapper. Label justifications:
Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7d5afcbb3f83. |
|
Status update after addressing automated review feedback: This PR is intended as the first scoped fix for #48788, plus the related #48388 filename decoding behavior. It fixes the shared media download path and Feishu inbound filename handling without trying to migrate every remaining channel-specific filename path in one PR. What was fixed in the latest pushed head:
Verification run locally before push:
Remaining scope: #48788 should stay open after this PR unless maintainers want to expand this change. Remaining channel filename migrations and any broader SDK/API documentation or baseline alignment can be handled as follow-up work under #48788. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
0be9dc7 to
9c2a800
Compare
|
Heads up: this PR needs to be updated against current |
Summary
Content-Dispositionedge cases and Feishu mojibake fixes are easy to miss or duplicate.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Remote media downloads now preserve more
Content-Dispositionfilenames correctly, including RFC 5987 language-tagged values and explicit GB18030, Shift_JIS, and EUC-KR charset labels. Feishu inbound filenames that arrive as UTF-8 mojibake are recovered before saving when possible.Security Impact (required)
Repro + Verification
Environment
Steps
Content-Dispositionfilenames with language tags and explicit charset labels.Expected
saveMediaBufferreceives the filename.Actual
Matches expected in targeted tests.
Evidence
Targeted verification:
corepack pnpm tsgowas also run. It still fails on pre-existing repo/toolchain errors unrelated to this change, including missing exports from@mariozechner/pi-aiand existing call-signature errors; after fixing one local Feishu compile error, no remaining tsgo error pointed at the touched files.Human Verification (required)
filename*priority, language tags, GB18030 / Shift_JIS / EUC-KR decoding, Windows path basename reduction, Latin-1/Windows-1252 mojibake recovery, remote media fetch integration../..rejection, normal Latin-1 text fallback.Review Conversations
Compatibility / Migration
Failure Recovery (if this breaks)
src/media/filename.ts,src/media/fetch.ts,src/plugin-sdk/feishu.ts,extensions/feishu/src/bot.ts, and related tests.Risks and Mitigations
Real behavior proof
node --import tsx --input-type=moduleimportingsrc/media/filename.tsandsrc/media/fetch.ts, constructing a realResponse, and callingsaveResponseMedia.{ "rfc5987Decoded": "✓-report.txt", "mojibakeDecoded": "中国银行.pdf", "savedFileName": "photo.txt", "savedContentType": "text/plain", "savedBytes": 2 }✓-report.txt, the mojibake filename recovered to中国银行.pdf, and the response save path reducedC:\temp\photo.txttophoto.txtwhile preservingtext/plaincontent.