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 1 commit
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
70 changes: 67 additions & 3 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 @@ -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 {
_ = os.Remove(artifactPath)
return 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