fix(apps): harden signed URL downloads - #2241
Open
liangshuo-1 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe app file download path now uses the internal streaming API with 32 MiB ranged requests. Tests cover request headers, transient failure retries, typed network errors, and removal of partial output. ChangesApp file download streaming
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppsFileDownload
participant openAppsFileDownload
participant ImmutableURLSource
participant SignedURLServer
participant FileIO
AppsFileDownload->>openAppsFileDownload: Open signed-URL stream
openAppsFileDownload->>ImmutableURLSource: Configure 32 MiB parts
ImmutableURLSource->>SignedURLServer: Send ranged GET
SignedURLServer-->>ImmutableURLSource: Return response metadata and body
ImmutableURLSource-->>FileIO: Provide stream and metadata
FileIO-->>AppsFileDownload: Return save result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
size/M
liangshuo-1
marked this pull request as ready for review
August 7, 2026 10:18
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/apps_file_download_test.go`:
- Around line 146-174: Strengthen
TestAppsFileDownload_PreservesNetworkBodyFailure by asserting the expected
network problem.Subtype and verifying the preserved underlying cause with
errors.Is against io.ErrUnexpectedEOF. Keep using errs.ProblemOf for typed
metadata, but do not access Param because this is not a validation error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98bf4dd1-e990-4504-b296-e7b3154dfa0c
📒 Files selected for processing (2)
shortcuts/apps/apps_file_download.goshortcuts/apps/apps_file_download_test.go
Comment on lines
+146
to
+174
| func TestAppsFileDownload_PreservesNetworkBodyFailure(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Length", "10") | ||
| w.WriteHeader(http.StatusOK) | ||
| io.WriteString(w, "short") | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| dir := t.TempDir() | ||
| oldWD, _ := os.Getwd() | ||
| if err := os.Chdir(dir); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { _ = os.Chdir(oldWD) }) | ||
|
|
||
| factory, stdout, reg := newAppsExecuteFactory(t) | ||
| reg.Register(&httpmock.Stub{ | ||
| Method: "POST", URL: fileSignURLForDownload, | ||
| Body: map[string]interface{}{"code": 0, "data": map[string]interface{}{"signed_url": srv.URL}}, | ||
| }) | ||
| err := runAppsShortcut(t, AppsFileDownload, | ||
| []string{"+file-download", "--app-id", "app_x", "--path", "/short.bin", "--output", "out.bin", "--as", "user"}, factory, stdout) | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok || problem.Category != errs.CategoryNetwork { | ||
| t.Fatalf("error = %T %v, want typed network error", err, err) | ||
| } | ||
| if _, statErr := os.Stat(filepath.Join(dir, "out.bin")); !os.IsNotExist(statErr) { | ||
| t.Fatalf("partial output exists after body failure: %v", statErr) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete typed error contract and the preserved cause.
This test asserts only errs.CategoryNetwork. It will pass if the save pipeline changes the error subtype or drops the truncated-body cause. Assert the expected problem.Subtype and the underlying read error with errors.Is, such as io.ErrUnexpectedEOF.
Do not read Param from errs.ProblemOf. Problem has no Param, and this is not a validation error. As per coding guidelines, error-path tests must assert typed metadata and cause preservation. Based on learnings, errs.ProblemOf does not expose Param.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/apps/apps_file_download_test.go` around lines 146 - 174, Strengthen
TestAppsFileDownload_PreservesNetworkBodyFailure by asserting the expected
network problem.Subtype and verifying the preserved underlying cause with
errors.Is against io.ErrUnexpectedEOF. Keep using errs.ProblemOf for typed
metadata, but do not access Param because this is not a validation error.
Sources: Coding guidelines, Learnings
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@bfceb03a5a278755b876453e79e60ae0e3d420b3🧩 Skill updatenpx skills add larksuite/cli#fix/apps-file-download -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2241 +/- ##
==========================================
- Coverage 76.46% 76.38% -0.08%
==========================================
Files 1000 1009 +9
Lines 107351 110981 +3630
==========================================
+ Hits 82083 84774 +2691
- Misses 19042 19736 +694
- Partials 6226 6471 +245 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
apps +file-downloadsigned URLs through the shared validated download pipelineIf-Rangevalidation when availableWhy
The previous direct
http.Client.Dopath issued one unvalidated full-body request. It could not resume a failed body, had no progress-based idle timeout, accepted unexpected response encoding, and could report a network body failure as an output validation error.Apps storage paths identify immutable uploaded objects, and the current object store supports byte ranges and strong ETags. Reusing
internal/downloadkeeps HTTP policy, representation consistency, retry, exact-length validation, and streaming output in their existing owners without adding Apps-specific transport logic.Impact
Successful CLI output and flags are unchanged. Files up to the Apps 100 MiB limit use at most four 32 MiB range requests; servers that ignore Range continue through the returned full response.
Validation
go test ./shortcuts/apps ./internal/download ./internal/vfs/localfileio ./shortcuts/common ./tests/cli_e2e/apps -count=1go test -race ./shortcuts/apps ./internal/download ./internal/vfs/localfileio -count=1go vet ./shortcuts/apps ./internal/downloadgit diff --checkIf-Range, exact request counts (1/2/3/4), and matching SHA-256 checksumsSummary by CodeRabbit