Bound remote archive extraction resources #509
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| package packageimport | ||
|
|
||
| import ( | ||
| "archive/tar" | ||
| "archive/zip" | ||
| "bytes" | ||
| "compress/gzip" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestCopyWithByteLimitAllowsExactSize(t *testing.T) { | ||
| var dst bytes.Buffer | ||
| if err := copyWithByteLimit(&dst, strings.NewReader("exact"), int64(len("exact"))); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if dst.String() != "exact" { | ||
| t.Fatalf("copied content = %q", dst.String()) | ||
| } | ||
| } | ||
|
|
||
| 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 TestTarExtractionEnforcesFileCountLimit(t *testing.T) { | ||
| archivePath := filepath.Join(t.TempDir(), "package.tgz") | ||
| writeTarArchive(t, archivePath, tarEntry{name: "package/a", body: "a"}, tarEntry{name: "package/b", body: "b"}) | ||
| err := untarGzWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 1, MaxEntryBytes: 10, MaxTotalBytes: 10}) | ||
| if err == nil || !strings.Contains(err.Error(), "file limit") { | ||
| t.Fatalf("tar file limit error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestTarExtractionEnforcesTotalExpandedSizeLimit(t *testing.T) { | ||
| archivePath := filepath.Join(t.TempDir(), "package.tgz") | ||
| writeTarArchive(t, archivePath, tarEntry{name: "package/a", body: "abc"}, tarEntry{name: "package/b", body: "def"}) | ||
| err := untarGzWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 10, MaxTotalBytes: 5}) | ||
| if err == nil || !strings.Contains(err.Error(), "expanded byte limit") { | ||
| t.Fatalf("tar total size error = %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestZipExtractionEnforcesEntryAndTotalSizeLimits(t *testing.T) { | ||
| archivePath := filepath.Join(t.TempDir(), "package.zip") | ||
| writeZipArchive(t, archivePath, "package/a", "abc") | ||
| // The entry-size check is covered independently; this archive is enough to | ||
| // exercise the total-size check after the first entry. | ||
| archivePath = filepath.Join(t.TempDir(), "package.zip") | ||
| writeZipArchive(t, archivePath, "package/a", "abcdef") | ||
| if err := unzipWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 2, MaxTotalBytes: 10}); err == nil || !strings.Contains(err.Error(), "byte limit") { | ||
| t.Fatalf("zip entry size error = %v", err) | ||
| } | ||
| if err := unzipWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 10, MaxTotalBytes: 5}); err == nil || !strings.Contains(err.Error(), "expanded byte limit") { | ||
| t.Fatalf("zip total size error = %v", err) | ||
| } | ||
|
There was a problem hiding this comment. TestZipExtractionEnforcesEntryAndTotalSizeLimits 含死代码且未真正验证跨条目累计总量限制新增的 TestZipExtractionEnforcesEntryAndTotalSizeLimits 存在两个问题：1) 函数开头 writeZipArchive(t, archivePath, "package/a", "abc") 写入的归档在其后 archivePath 被重新指向新的 t.TempDir() 后即被丢弃，属于无效死代码（t.TempDir() 每次返回全新目录，第一次写入的归档从未被读取）；2) 最终使用的归档仅含单一条目 "abcdef"（6 字节），当 MaxTotalBytes=5 时在第一条目即触发超限，只能证明『单条目超过总量』会被拒绝，无法验证总量限制的核心语义『多个各自未超限（MaxEntryBytes=10 下 3 字节条目）的条目累计超过总量时被拒绝』。而函数内注释却声称 'this archive is enough to exercise the total-size check after the first entry'，暗示存在第二条目，注释与实际代码不符。对比同批新增的 TestTarExtractionEnforcesTotalExpandedSizeLimit（正确使用两个 3 字节条目），本测试未真正覆盖 zip 的跨条目累计总量检查；若未来实现对 zip 仅做单条目总量判断而不再跨条目累计（回归到仅防单条目超大、漏防跨条目 zip-bomb），此测试仍会通过，形成安全相关回归盲区。 Problem code: Recommendation: Suggested diff: archivePath := filepath.Join(t.TempDir(), "package.zip")
- writeZipArchive(t, archivePath, "package/a", "abc")
- // The entry-size check is covered independently; this archive is enough to
- // exercise the total-size check after the first entry.
- archivePath = filepath.Join(t.TempDir(), "package.zip")
- writeZipArchive(t, archivePath, "package/a", "abcdef")
+ // 条目大小限制由独立场景覆盖；此处写入两个各 3 字节的条目，
+ // 每个都低于 MaxEntryBytes，但合计 6 字节超过 MaxTotalBytes=5，
+ // 用于验证跨条目累计后的总量限制。
+ writeZipArchive(t, archivePath, "package/a", "abc")
+ writeZipArchive(t, archivePath, "package/b", "def") // 需使辅助函数支持同一归档追加/多条目写入
if err := unzipWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 2, MaxTotalBytes: 10}); err == nil || !strings.Contains(err.Error(), "byte limit") {
t.Fatalf("zip entry size error = %v", err)
}
if err := unzipWithLimits(archivePath, t.TempDir(), archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 10, MaxTotalBytes: 5}); err == nil || !strings.Contains(err.Error(), "expanded byte limit") {
t.Fatalf("zip total size error = %v", err)
} |
||
| } | ||
|
|
||
| func TestZipExtractionAllowsArchiveWithinLimits(t *testing.T) { | ||
| archivePath := filepath.Join(t.TempDir(), "package.zip") | ||
| writeZipArchive(t, archivePath, "package/data", "ok") | ||
| dst := t.TempDir() | ||
| if err := unzipWithLimits(archivePath, dst, archiveExtractionLimits{MaxFiles: 10, MaxEntryBytes: 10, MaxTotalBytes: 10}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| content, err := os.ReadFile(filepath.Join(dst, "package", "data")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(content) != "ok" { | ||
| t.Fatalf("extracted content = %q", content) | ||
| } | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
| } | ||
|
monkeyscan[bot] marked this conversation as resolved.
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
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:
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: