Bound remote archive extraction resources - #509
Conversation
|
PR Title: Bound remote archive extraction resources Commit: 本次变更为内部包导入的远程归档下载与解压流程添加资源限制，用于防御超大下载、zip/tar 解压炸弹。主要变更：(1) 新增 maxRemoteArchiveBytes（512MB）下载大小限制，通过 resp.ContentLength 预检 + copyWithByteLimit 运行时限制双重防护，下载超限或关闭失败时删除残留文件；(2) 新增 defaultArchiveExtractionLimits（MaxFiles=10万、MaxEntryBytes=512MB、MaxTotalBytes=2GB），untarGz/unzip 改为委托 untarGzWithLimits/unzipWithLimits，对条目数、单文件大小、累计展开大小分别做前置校验，并在逐条复制时用 copyWithByteLimit 做运行时兜底；(3) 新增 resource_limits_test.go 覆盖三条超限路径。 整体评估：限制逻辑实现稳健——检查顺序正确（先校验再累加，无 uint64 下溢风险），int64/uint64 边界在调用前均已受 MaxEntryBytes 约束不会溢出，copyWithByteLimit 的 max+1 判界正确，zip 与 tar 两条路径的运行时兜底均有效。主要不足在可观测性与测试覆盖：运行时超限错误未带包来源上下文（与同一函数内 ContentLength 预检错误路径不一致，解压错误也未在 prepareRemoteArchiveSource 中包装来源），新增测试未覆盖 tar 的 MaxFiles/MaxEntryBytes、zip 的 MaxEntryBytes/MaxTotalBytes 以及正常通过路径。 |
![monkeyscan[bot]](https://avatars.githubusercontent.com/u/231811742?s=60&v=4){kind=small}
| return err | ||
| } | ||
| _, copyErr := io.Copy(out, resp.Body) | ||
| copyErr := copyWithByteLimit(out, resp.Body, maxRemoteArchiveBytes) |
There was a problem hiding this comment.
资源限制错误信息缺少包来源上下文，故障定位困难
本次变更新增的运行时资源限制错误路径与同函数内其他错误路径不一致：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)
}
|
PR Title: Bound remote archive extraction resources Commit: 本次变更仅修改 internal/packageimport/importer.go 的两处错误返回，目的是为包导入失败补充来源上下文：1) prepareSource 本地文件解压失败改为 fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err)；2) downloadRemoteArchive 的 copyErr 返回改为 fmt.Errorf("download remote package %q: %w", ...)。第二处与同函数既有错误文案（HTTP、ContentLength 预检等）一致，是合理且一致性的改进。第一处存在问题：该分支位于 classifySource switch 的 default 分支，只处理本地文件系统源（远程归档已在 switch 的 sourceRemoteArchive 分支由 prepareRemoteArchiveSource 提前返回），错误却标注为 "remote package"，且用 url.Parse 类脱敏函数处理本地路径，文件名含 #/? 等字符时会被当作 URL 组成部分剥离导致路径截断；真正需要来源上下文的远程解压失败路径 prepareRemoteArchiveSource（unzip/untarGz）反而未被包装。总体为小型错误信息改动，无安全、数据面或行为逻辑影响，但变更 1 的上下文包装加错了位置并引入误导性文案，建议修正。无其它新增问题，未发现需要报告的测试缺失（纯错误文案改动）。 |
| } | ||
| if err != nil { | ||
| return preparedSource{}, err | ||
| return preparedSource{}, fmt.Errorf("extract remote package %q: %w", redactedRemoteArchiveSource(source), err) |
There was a problem hiding this comment.
本地包解压失败被误标为 "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)
|
PR Title: Bound remote archive extraction resources Commit: 本 PR 仅修改 internal/packageimport/resource_limits_test.go，新增 5 个测试：copyWithByteLimit 恰好等于上限时放行、tar 文件数上限、tar 累计展开大小上限、zip 条目/累计大小上限、以及 zip 未超限时正常解压。整体来看这些补测基本填补了历史 finding 所指出的覆盖缺口（tar MaxFiles/MaxTotalBytes、zip MaxEntryBytes/MaxTotalBytes、正常通过路径与精确边界），测试隔离性良好（均使用 t.TempDir），断言字符串与既有错误信息风格一致，未引入明显的正确性问题。 主要问题集中在新增的 TestZipExtractionEnforcesEntryAndTotalSizeLimits：函数开头先 writeZipArchive 写入一个含 "abc" 的归档，随后 archivePath 被重新指向新的 t.TempDir() 并再次写入，前一次写入成为完全无效的死代码；且最终归档仅含单一条目（"abcdef"），在 MaxTotalBytes=5 时第一条目即超限触发，无法真正验证「各条目单独未超限但累计超过总量」这一总量限制的核心语义（同函数 tar 版本用两个 3 字节条目正确验证了累计逻辑）。注释 "exercise the total-size check after the first entry" 与单条目实际代码不一致，容易误导读者高估该测试对跨条目 zip-bomb 累计防护的覆盖。已就此提交一条 low 严重度、high 置信度的维护性 finding。 |
|
Follow-up fixes after review:
Verification: resource-limit packageimport tests pass. |
| } | ||
| 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:
Changed code at internal/packageimport/resource_limits_test.go:86-97
Recommendation:
删除开头两次无效的 writeZipArchive/archivePath 赋值死代码；将归档改为包含两个各自低于 MaxEntryBytes 但合计超过 MaxTotalBytes 的条目（例如 'package/a' 内容 'abc'、'package/b' 内容 'def'，MaxTotalBytes 设为 5），以真正驱动跨条目累计后的总量检查，并同步修正注释。若现有 writeZipArchive 辅助函数仅支持写入单一条目，可仿照 writeTarArchive 的变参形式扩展它。
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)
}1 participant
问题
远程归档下载使用无限制复制，tar/zip 解压也没有文件数量、单文件大小或总展开大小限制。
影响
恶意远程源可通过超大响应、压缩炸弹或海量小文件耗尽磁盘、inode 和 CPU，导致守护进程或宿主机拒绝服务。
修复内容
Content-Length。Content-Length绕过。验证
go test ./internal/packageimport -run 'Test(CopyWithByteLimit|TarExtractionEnforcesExpandedSizeLimit|ZipExtractionEnforcesFileCountLimit)'通过。