Skip to content

fix(apps): harden signed URL downloads #2241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
liangshuo-1 wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −21
Open

fix(apps): harden signed URL downloads #2241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 16 additions & 21 deletions shortcuts/apps/apps_file_download.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,23 @@
"context"
"fmt"
"io"
"net/http"
"path"
"strings"

"github.com/larksuite/cli/errs"
"github.com/larksuite/cli/extension/fileio"
"github.com/larksuite/cli/internal/download"
"github.com/larksuite/cli/shortcuts/common"
)

const appsFileDownloadPartSize = 32 * 1024 * 1024

func openAppsFileDownload(ctx context.Context, signedURL string) (*download.Stream, error) {
urlTransport := download.URL(newFileTransferClient(), signedURL)
source := download.ImmutableSource(urlTransport)
return download.Open(ctx, source, download.Options{PartSize: appsFileDownloadPartSize})
}

// AppsFileDownload downloads a file to a local path via a signed URL。
//
// 两步：POST /apps/{app_id}/storage/file_sign 拿 signed_url（presigned，直连对象存储），
Expand Down Expand Up @@ -83,30 +91,17 @@
out = "download"
}
}
req, err := http.NewRequestWithContext(rctx.Ctx(), http.MethodGet, signedURL, nil) //nolint:forbidigo // GET from a presigned object-storage URL bypasses the Lark gateway; raw HTTP required (not a Lark API call).
if err != nil {
return errs.NewNetworkError(errs.SubtypeNetworkTransport, "build download request").WithCause(err)
}
resp, err := newFileTransferClient().Do(req) //nolint:forbidigo // see above: direct presigned-URL download, RuntimeContext.DoAPI does not apply.
stream, err := openAppsFileDownload(ctx, signedURL)
if err != nil {
// dial/transport 失败是典型可重试场景。
return errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed").WithCause(err).WithRetryable()
}
defer resp.Body.Close()
if resp.StatusCode >= 400 {
io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
// 5xx 是上游瞬时故障，标 retryable；4xx（如签名过期）需重新签名而非盲重试，不标。
if resp.StatusCode >= 500 {
return errs.NewNetworkError(errs.SubtypeNetworkServer, "download failed: HTTP %d", resp.StatusCode).WithRetryable()
}
return errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed: HTTP %d", resp.StatusCode)
return err

Check warning on line 96 in shortcuts/apps/apps_file_download.go

View check run for this annotation

Codecov / codecov/patch

shortcuts/apps/apps_file_download.go#L96 

Added line #L96 was not covered by tests
}
defer stream.Body.Close()
saved, err := rctx.FileIO().Save(out, fileio.SaveOptions{
ContentType: resp.Header.Get("Content-Type"),
ContentLength: resp.ContentLength,
}, resp.Body)
ContentType: stream.Header.Get("Content-Type"),
ContentLength: stream.ContentLength,
}, stream.Body)
if err != nil {
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output: %v", err).WithParam("--output").WithCause(err)
return common.WrapSaveErrorTyped(err)
}
resolved, perr := rctx.FileIO().ResolvePath(out)
if perr != nil || resolved == "" {
Expand Down
86 changes: 86 additions & 0 deletions shortcuts/apps/apps_file_download_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"os"
"path/filepath"
"strings"
"sync/atomic"
"testing"

"github.com/larksuite/cli/errs"
Expand Down Expand Up @@ -50,12 +51,23 @@ func TestAppsFileDownload_DryRunSignsFirst(t *testing.T) {

// sign → 客户端 GET presigned signed_url → 落盘 --output。
func TestAppsFileDownload_EndToEnd(t *testing.T) {
var requests atomic.Int32
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
requests.Add(1)
if r.Method != http.MethodGet {
w.WriteHeader(http.StatusMethodNotAllowed)
return
}
if got, want := r.Header.Get("Range"), "bytes=0-33554431"; got != want {
t.Errorf("Range = %q, want %q", got, want)
}
if got := r.Header.Get("Accept-Encoding"); got != "identity" {
t.Errorf("Accept-Encoding = %q, want identity", got)
}
w.Header().Set("Content-Type", "image/png")
w.Header().Set("Content-Range", "bytes 0-6/7")
w.Header().Set("Content-Length", "7")
w.WriteHeader(http.StatusPartialContent)
io.WriteString(w, "PNGDATA")
}))
defer srv.Close()
Expand Down Expand Up @@ -86,6 +98,80 @@ func TestAppsFileDownload_EndToEnd(t *testing.T) {
if !strings.Contains(stdout.String(), `"size_bytes": 7`) {
t.Errorf("output json missing size_bytes:7\n%s", stdout.String())
}
if got := requests.Load(); got != 1 {
t.Fatalf("download requests = %d, want 1", got)
}
}

func TestAppsFileDownload_RetriesTransientPresignedFailure(t *testing.T) {
var requests atomic.Int32
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if requests.Add(1) == 1 {
w.WriteHeader(http.StatusServiceUnavailable)
return
}
w.Header().Set("Content-Type", "application/octet-stream")
io.WriteString(w, "recovered")
}))
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}},
})
if err := runAppsShortcut(t, AppsFileDownload,
[]string{"+file-download", "--app-id", "app_x", "--path", "/retry.bin", "--output", "out.bin", "--as", "user"}, factory, stdout); err != nil {
t.Fatalf("execute err=%v", err)
}
data, err := os.ReadFile(filepath.Join(dir, "out.bin"))
if err != nil {
t.Fatalf("read output file: %v", err)
}
if got, want := string(data), "recovered"; got != want {
t.Fatalf("downloaded content = %q, want %q", got, want)
}
if got := requests.Load(); got != 2 {
t.Fatalf("download requests = %d, want 2", got)
}
}

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)
}
Comment on lines +146 to +174

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

}

// 不传 --output → 默认远端 basename。
Expand Down
Loading