Skip to content

Bound remote archive extraction resources #509

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
GOLDKUN wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Bound remote archive extraction resources #509

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41f43f2
Bound remote archive extraction resources
Sep 2, 2026
a6989c2
Add source context to archive limit errors
Sep 2, 2026
1f45fd5
Cover archive resource limit boundaries
Sep 2, 2026
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
74 changes: 69 additions & 5 deletions internal/packageimport/importer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,20 @@ type Importer struct {
Store *store.Store
}

const maxRemoteArchiveBytes int64 = 512 << 20

var defaultArchiveExtractionLimits = archiveExtractionLimits{
MaxFiles: 100_000,
MaxEntryBytes: 512 << 20,
MaxTotalBytes: 2 << 30,
}

type archiveExtractionLimits struct {
MaxFiles int
MaxEntryBytes uint64
MaxTotalBytes uint64
}

type Options struct {
ServiceID string `json:"service_id"`
Name string `json:"name"`
Expand Down Expand Up @@ -649,7 +663,7 @@ func (i *Importer) prepareSource(ctx context.Context, opts Options, staging stri
err = untarGz(artifactPath, packageDir)
}
if err != nil {
return preparedSource{}, err
return preparedSource{}, fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err)

@monkeyscan monkeyscan Bot Sep 2, 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.

本地包解压失败被误标为 "extract remote package"，来源上下文包装加错了分支

变更把 prepareSource 中 default 分支的解压失败错误包装为 fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err)。但该分支位于 classifySource switch 之后：远程归档（sourceRemoteArchive）、npm、Git 都在 switch 内提前 return，此处的 else 分支只处理本地文件系统文件（os.Stat 为文件的本地路径），永远不会是远程包。因此：1) 任何本地 .tgz/.zip（或落入 untarGz 的其他扩展名文件）解压失败时，错误信息会误导性地声称是 "remote package"；2) redactedRemoteArchiveSource 用 url.Parse 处理本地路径，文件名含 #、? 等合法字符时会被当作 fragment/query 剥离，报错显示的路径被截断（例如本地 /tmp/a#b.tgz 会显示为 /tmp/a），反而丢失了要补充的定位信息；3) 真正需要来源上下文的远程解压失败路径 prepareRemoteArchiveSource（unzip/untarGz 失败，约 806-809 行）仍直接返回裸 err，本次改动没有给远程解压错误补充上下文，目标未达成且加错了位置。

Problem code:

Changed code at internal/packageimport/importer.go:666

Recommendation:
将 "extract remote package %q" 包装移到真正处理远程归档的 prepareRemoteArchiveSource 中 unzip/untarGz 失败分支（补充远程解压错误上下文，符合既有 "download remote package %q" 风格）；本地文件解压分支改用不含 URL 语义的文案并直接使用原始本地路径 source，避免 url.Parse 截断，例如：if err != nil { return preparedSource{}, fmt.Errorf("extract package %q: %w", source, err) }。

Suggested diff:

diff --git a/internal/packageimport/importer.go b/internal/packageimport/importer.go
--- a/internal/packageimport/importer.go
+++ b/internal/packageimport/importer.go
@@ -663,7 +663,7 @@ func (i *Importer) prepareSource(ctx context.Context, opts Options, staging stri
 		}
 		if err != nil {
-			return preparedSource{}, fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err)
+			return preparedSource{}, fmt.Errorf("extract package %q: %w", source, err)
 		}
 		packageDir = normalizePackageDir(packageDir)
 	}
@@ -806,7 +806,7 @@ func prepareRemoteArchiveSource(ctx context.Context, source, serviceRoot, staging
 			err = untarGz(artifactPath, packageDir)
 		}
 		if err != nil {
-			return preparedSource{}, err
+			return preparedSource{}, fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err)
 		}
 		packageDir = normalizePackageDir(packageDir)

}
packageDir = normalizePackageDir(packageDir)
}
Expand Down Expand Up @@ -832,21 +846,39 @@ func downloadRemoteArchive(ctx context.Context, source, artifactPath string) err
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return fmt.Errorf("download remote package %q: HTTP %d", redactedRemoteArchiveSource(source), resp.StatusCode)
}
if resp.ContentLength > maxRemoteArchiveBytes {
return fmt.Errorf("download remote package %q: archive exceeds %d byte limit", redactedRemoteArchiveSource(source), maxRemoteArchiveBytes)
}
if err := os.MkdirAll(filepath.Dir(artifactPath), 0o755); err != nil {
return err
}
out, err := os.OpenFile(artifactPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0o644)
if err != nil {
return err
}
_, copyErr := io.Copy(out, resp.Body)
copyErr := copyWithByteLimit(out, resp.Body, maxRemoteArchiveBytes)

@monkeyscan monkeyscan Bot Sep 2, 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.

资源限制错误信息缺少包来源上下文，故障定位困难

本次变更新增的运行时资源限制错误路径与同函数内其他错误路径不一致：downloadRemoteArchive 中 ContentLength 预检失败返回 "download remote package %q: archive exceeds ..." 带 URL 上下文，但 copyWithByteLimit 运行时超限返回的 "content exceeds ... byte limit" 被直接透传，未包装 source；同样，prepareRemoteArchiveSource 中 unzip/untarGz 返回的 "archive exceeds ... file limit" / "archive exceeds ... expanded byte limit" / "archive entry ... exceeds ... byte limit" 也未包装任何归档来源。当导入失败触发资源限制时，用户/运维无法从错误信息判断是哪个远程包或上传包导致的，增加了安全事件与故障排查成本。

Problem code:

Changed code at internal/packageimport/importer.go:859

Recommendation:
在 downloadRemoteArchive 中用 fmt.Errorf("download remote package %q: %w", redactedRemoteArchiveSource(source), err) 包装 copyWithByteLimit 的错误；在 prepareRemoteArchiveSource 中对 unzip/untarGz 的错误统一包装归档来源（如 redactedRemoteArchiveSource(source)），使所有限制错误都带可定位的来源上下文。

Suggested diff:

diff --git a/internal/packageimport/importer.go b/internal/packageimport/importer.go
--- a/internal/packageimport/importer.go
+++ b/internal/packageimport/importer.go
@@ downloadRemoteArchive
 	copyErr := copyWithByteLimit(out, resp.Body, maxRemoteArchiveBytes)
 	closeErr := out.Close()
 	if copyErr != nil {
 		_ = os.Remove(artifactPath)
-		return copyErr
+		return fmt.Errorf("download remote package %q: %w", redactedRemoteArchiveSource(source), copyErr)
 	}
@@ prepareRemoteArchiveSource
 	if err != nil {
-		return preparedSource{}, err
+		return preparedSource{}, fmt.Errorf("extract %q: %w", redactedRemoteArchiveSource(source), err)
 	}

closeErr := out.Close()
if copyErr != nil {
return copyErr
_ = os.Remove(artifactPath)
return fmt.Errorf("download remote package %q: %w", redactedRemoteArchiveSource(source), copyErr)
}
if closeErr != nil {
_ = os.Remove(artifactPath)
}
return closeErr
}

func copyWithByteLimit(dst io.Writer, src io.Reader, max int64) error {
written, err := io.Copy(dst, io.LimitReader(src, max+1))
if err != nil {
return err
}
if written > max {
return fmt.Errorf("content exceeds %d byte limit", max)
}
return nil
}

func redactedRemoteArchiveSource(source string) string {
u, err := url.Parse(source)
if err != nil {
Expand Down Expand Up @@ -1630,6 +1662,10 @@ func tarGzDir(src, dst string) error {
}

func untarGz(src, dst string) error {
return untarGzWithLimits(src, dst, defaultArchiveExtractionLimits)
}

func untarGzWithLimits(src, dst string, limits archiveExtractionLimits) error {
file, err := os.Open(src)
if err != nil {
return err
Expand All @@ -1641,6 +1677,8 @@ func untarGz(src, dst string) error {
}
defer gz.Close()
tr := tar.NewReader(gz)
files := 0
var total uint64
for {
hdr, err := tr.Next()
if errors.Is(err, io.EOF) {
Expand All @@ -1649,6 +1687,17 @@ func untarGz(src, dst string) error {
if err != nil {
return err
}
files++
if files > limits.MaxFiles {
return fmt.Errorf("archive exceeds %d file limit", limits.MaxFiles)
}
if hdr.Size < 0 || uint64(hdr.Size) > limits.MaxEntryBytes {
return fmt.Errorf("archive entry %q exceeds %d byte limit", hdr.Name, limits.MaxEntryBytes)
}
if uint64(hdr.Size) > limits.MaxTotalBytes-total {
return fmt.Errorf("archive exceeds %d expanded byte limit", limits.MaxTotalBytes)
}
total += uint64(hdr.Size)
clean := filepath.Clean(hdr.Name)
if strings.HasPrefix(clean, "..") || filepath.IsAbs(clean) {
return fmt.Errorf("unsafe archive path %q", hdr.Name)
Expand All @@ -1667,7 +1716,7 @@ func untarGz(src, dst string) error {
if err != nil {
return err
}
_, copyErr := io.Copy(out, tr)
copyErr := copyWithByteLimit(out, tr, hdr.Size)
closeErr := out.Close()
if copyErr != nil {
return copyErr
Expand All @@ -1680,12 +1729,27 @@ func untarGz(src, dst string) error {
}

func unzip(src, dst string) error {
return unzipWithLimits(src, dst, defaultArchiveExtractionLimits)
}

func unzipWithLimits(src, dst string, limits archiveExtractionLimits) error {
r, err := zip.OpenReader(src)
if err != nil {
return err
}
defer r.Close()
if len(r.File) > limits.MaxFiles {
return fmt.Errorf("archive exceeds %d file limit", limits.MaxFiles)
}
var total uint64
for _, file := range r.File {
if file.UncompressedSize64 > limits.MaxEntryBytes {
return fmt.Errorf("archive entry %q exceeds %d byte limit", file.Name, limits.MaxEntryBytes)
}
if file.UncompressedSize64 > limits.MaxTotalBytes-total {
return fmt.Errorf("archive exceeds %d expanded byte limit", limits.MaxTotalBytes)
}
total += file.UncompressedSize64
clean := filepath.Clean(file.Name)
if strings.HasPrefix(clean, "..") || filepath.IsAbs(clean) {
return fmt.Errorf("unsafe archive path %q", file.Name)
Expand All @@ -1709,7 +1773,7 @@ func unzip(src, dst string) error {
_ = in.Close()
return err
}
_, copyErr := io.Copy(out, in)
copyErr := copyWithByteLimit(out, in, int64(file.UncompressedSize64))
closeIn := in.Close()
closeOut := out.Close()
if copyErr != nil {
Expand Down
88 changes: 88 additions & 0 deletions internal/packageimport/resource_limits_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package packageimport

import (
"archive/tar"
"archive/zip"
"bytes"
"compress/gzip"
"os"
"path/filepath"
"strings"
"testing"
)

func TestCopyWithByteLimitRejectsOversizedContent(t *testing.T) {
var dst bytes.Buffer
err := copyWithByteLimit(&dst, strings.NewReader("oversized"), 4)
if err == nil || !strings.Contains(err.Error(), "byte limit") {
t.Fatalf("copy limit error = %v", err)
}
}

func TestTarExtractionEnforcesExpandedSizeLimit(t *testing.T) {
archivePath := filepath.Join(t.TempDir(), "package.tgz")
file, err := os.Create(archivePath)
if err != nil {
t.Fatal(err)
}
gz := gzip.NewWriter(file)
tarWriter := tar.NewWriter(gz)
body := []byte("0123456789")
if err := tarWriter.WriteHeader(&tar.Header{Name: "package/data", Mode: 0o600, Size: int64(len(body))}); err != nil {
t.Fatal(err)
}
if _, err := tarWriter.Write(body); err != nil {
t.Fatal(err)
}
if err := tarWriter.Close(); err != nil {
t.Fatal(err)
}
if err := gz.Close(); err != nil {
t.Fatal(err)
}
if err := file.Close(); err != nil {
t.Fatal(err)
}

err = untarGzWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{
MaxFiles: 10,
MaxEntryBytes: 5,
MaxTotalBytes: 5,
})
if err == nil || !strings.Contains(err.Error(), "byte limit") {
t.Fatalf("tar limit error = %v", err)
}
}

func TestZipExtractionEnforcesFileCountLimit(t *testing.T) {
archivePath := filepath.Join(t.TempDir(), "package.zip")
file, err := os.Create(archivePath)
if err != nil {
t.Fatal(err)
}
zipWriter := zip.NewWriter(file)
for _, name := range []string{"package/a", "package/b"} {
entry, err := zipWriter.Create(name)
if err != nil {
t.Fatal(err)
}
if _, err := entry.Write(bytes.Repeat([]byte{'x'}, 1)); err != nil {
t.Fatal(err)
}
}
if err := zipWriter.Close(); err != nil {
t.Fatal(err)
}
if err := file.Close(); err != nil {
t.Fatal(err)
}

err = unzipWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{
MaxFiles: 1,
MaxEntryBytes: 10,
MaxTotalBytes: 10,
})
if err == nil || !strings.Contains(err.Error(), "file limit") {
t.Fatalf("zip limit error = %v", err)
}
}
Comment thread
monkeyscan[bot] marked this conversation as resolved.
Loading