feat(im): use Content-Disposition filename when downloading message r…#536
Conversation
github-actions
Bot
added
domain/im
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Content-Disposition filename parsing (including RFC 5987), threads a Changes
Sequence DiagramsequenceDiagram
actor User as User/CLI
participant Handler as DownloadHandler
participant HTTP as HTTPClient
participant Parser as ContentDispositionParser
participant Resolver as PathResolver
User->>Handler: Request download (with/without --output)
Handler->>Handler: Determine userSpecifiedOutput
Handler->>HTTP: Perform HTTP request
HTTP-->>Handler: Response + headers
Handler->>Parser: Parse Content-Disposition (RFC5987)
Parser-->>Handler: Sanitized filename or empty
Handler->>Resolver: Resolve final path (safePath, userSpecifiedOutput, disposition, content-type)
Resolver-->>Handler: Final output path
Handler->>HTTP: Download content (with/without Range)
HTTP-->>Handler: Success / Error
Handler-->>User: Return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_messages_resources_download.go (1)
35-35:⚠️ Potential issue | 🟡 MinorFlag description is stale after this change.
The
--outputdescription still statesdefaults to file_key, but the new resolution logic prefers theContent-Dispositionfilename when--outputis omitted. Consider refreshing the help text so users understand the new default.✏️ Proposed wording
- {Name: "output", Desc: "local save path (relative only, no .. traversal; defaults to file_key)"}, + {Name: "output", Desc: "local save path (relative only, no .. traversal; defaults to server filename from Content-Disposition, or file_key)"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_messages_resources_download.go` at line 35, Update the stale flag description for the "output" CLI flag (Name: "output") to reflect the new resolution order: state that when omitted the tool will prefer the filename from the HTTP Content-Disposition header and fall back to the file_key, while still requiring a relative path with no ".." traversal; change the Desc string where "output" is defined to this new concise wording.
🧹 Nitpick comments (3)
shortcuts/im/helpers_test.go (2)
668-700: LGTM onTestResolveIMResourceDownloadPath.Matrix covers the main axes: safePath-with-ext (idempotent), default vs user-specified output, CD-present vs absent, RFC 5987, and MIME fallback. One gap worth adding: the precedence case where
userSpecifiedOutput=false, safePath has no extension, CD is absent, andcontentTypeis empty — the expected outcome is thatsafePathis returned unchanged. Optional to add.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers_test.go` around lines 668 - 700, Add a unit test case to TestResolveIMResourceDownloadPath covering the missing precedence: call resolveIMResourceDownloadPath with safePath having no extension (e.g., "file_xxx"), contentType == "" and contentDisposition == "" and userSpecifiedOutput == false, and assert it returns the original safePath unchanged; update the tests slice in shortcuts/im/helpers_test.go where other cases are defined so this new case (name like "default path, no CD, no MIME") sits alongside the existing entries referencing resolveIMResourceDownloadPath.
640-666: Test coverage forparseContentDispositionFilenameis thorough.Good spread across plain/quoted, RFC 5987, precedence, traversal, Windows paths, control chars, malformed, and whitespace. Consider adding one more case to lock in behavior for a filename that is ONLY dots after sanitization (e.g.,
filename="../..") to ensure it returns""rather than"..".✏️ Optional additional case
{name: "whitespace trimmed", header: `attachment; filename=" report.pdf "`, want: "report.pdf"}, + {name: "dots-only after strip", header: `attachment; filename="../.."`, want: ""},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers_test.go` around lines 640 - 666, Add a test case to TestParseContentDispositionFilename that covers a filename composed only of dots after sanitization (e.g., header `attachment; filename="../.."`) and assert parseContentDispositionFilename returns an empty string; this ensures parseContentDispositionFilename correctly treats names that reduce to "." or ".." as invalid/empty after path-strip sanitization.shortcuts/im/im_messages_resources_download.go (1)
324-347: Resolution precedence looks correct; one minor observation.When
!userSpecifiedOutput,safePathis the validatedfile_key(no separators enforced bynormalizeDownloadOutputPath), sofilepath.Dir(safePath)is always"."in production. Thedir != "."branch on lines 334–335 is only exercised by the unit test's synthetic"downloads/file_xxx"input. Not a bug — just noting the branch is effectively dead for the real default flow; you may want to simplify toreturn cdFilenameand drop the join (or keep it as defensive code for future callers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/im_messages_resources_download.go` around lines 324 - 347, In resolveIMResourceDownloadPath, simplify the !userSpecifiedOutput branch: because safePath is the normalized file_key whose filepath.Dir is always ".", return cdFilename directly instead of computing dir and conditionally joining; remove the dir/Join logic (or keep a minimal defensive comment if you prefer) so the function returns cdFilename when cdFilename != "" and !userSpecifiedOutput.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/im/im_messages_resources_download.go`:
- Line 35: Update the stale flag description for the "output" CLI flag (Name:
"output") to reflect the new resolution order: state that when omitted the tool
will prefer the filename from the HTTP Content-Disposition header and fall back
to the file_key, while still requiring a relative path with no ".." traversal;
change the Desc string where "output" is defined to this new concise wording.
---
Nitpick comments:
In `@shortcuts/im/helpers_test.go`:
- Around line 668-700: Add a unit test case to TestResolveIMResourceDownloadPath
covering the missing precedence: call resolveIMResourceDownloadPath with
safePath having no extension (e.g., "file_xxx"), contentType == "" and
contentDisposition == "" and userSpecifiedOutput == false, and assert it returns
the original safePath unchanged; update the tests slice in
shortcuts/im/helpers_test.go where other cases are defined so this new case
(name like "default path, no CD, no MIME") sits alongside the existing entries
referencing resolveIMResourceDownloadPath.
- Around line 640-666: Add a test case to TestParseContentDispositionFilename
that covers a filename composed only of dots after sanitization (e.g., header
`attachment; filename="../.."`) and assert parseContentDispositionFilename
returns an empty string; this ensures parseContentDispositionFilename correctly
treats names that reduce to "." or ".." as invalid/empty after path-strip
sanitization.
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 324-347: In resolveIMResourceDownloadPath, simplify the
!userSpecifiedOutput branch: because safePath is the normalized file_key whose
filepath.Dir is always ".", return cdFilename directly instead of computing dir
and conditionally joining; remove the dir/Join logic (or keep a minimal
defensive comment if you prefer) so the function returns cdFilename when
cdFilename != "" and !userSpecifiedOutput.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76bdd1b6-a6b4-442b-b8d0-70ab3bde606c
📒 Files selected for processing (4)
shortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.goskills/lark-im/references/lark-im-messages-resources-download.md
chenxingtong-bytedance
force-pushed
the
fix/filename_adapt
branch
from
efdd9b8 to
63b3fd8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #536 +/- ##
==========================================
+ Coverage 59.94% 59.96% +0.02%
==========================================
Files 405 405
Lines 42673 42710 +37
==========================================
+ Hits 25580 25611 +31
- Misses 15084 15088 +4
- Partials 2009 2011 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@cfec99743178a3c710498acadd7c1862e1e0703c🧩 Skill updatenpx skills add chenxingtong-bytedance/cli#fix/filename_adapt -y -g |
chenxingtong-bytedance
force-pushed
the
fix/filename_adapt
branch
2 times, most recently
from
f51a3f8 to
1931de5
Compare
…esources Change-Id: I68b48cf428aa8aded4ad9d55fa042f9d68263c3a
chenxingtong-bytedance
force-pushed
the
fix/filename_adapt
branch
from
1931de5 to
cfec997
Compare
3 participants
Summary
When downloading message resources, the saved filename was always derived from
file_key(e.g.file_v2_abc123.xlsx), ignoring the original filename thesender uploaded. This PR resolves filenames from the
Content-Dispositionresponse header first, falling back to
Content-Type-based extension inferenceonly when the header is absent.
Changes
parseContentDispositionFilenameto extract and sanitize filenames fromContent-Disposition, with RFC 5987 (filename*=UTF-8''...) support via thestandard
mimepackageresolveIMResourceDownloadPathto prioritiseContent-Dispositionfilename; when
--outputis not specified, the full server filename is used;when
--outputis specified without an extension, only the extension is takenfrom the
Content-DispositionfilenameuserSpecifiedOutput boolparameter todownloadIMResourceToPathandresolveIMResourceDownloadPathto distinguish default vs. explicit output pathsskills/lark-im/references/lark-im-messages-resources-download.mdtoreflect the new filename resolution priority
Test Plan
go test ./shortcuts/im/...passedTestParseContentDispositionFilename(11 cases): plainfilename,Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation