RebaseArchiveEntries: fix archive path rebasing - #43
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
- Coverage 65.81% 65.16% -0.65%
==========================================
Files 42 44 +2
Lines 2039 2271 +232
==========================================
+ Hits 1342 1480 +138
- Misses 519 592 +73
- Partials 178 199 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| linkName: "target", | ||
| wantName: "prefix/link", | ||
| wantLinkName: "prefix/target", | ||
| }, |
There was a problem hiding this comment.
What about
{
name: "rebase absolute path from root",
oldBase: "/",
newBase: "prefix",
headerName: "/foo/bar",
wantName: "prefix/foo/bar",
},
?
IIUC this would produce prefix//foo/bar
There was a problem hiding this comment.
Hm; right; don't think headers would have trailing slashes (or could they have?)
There was a problem hiding this comment.
Generall they should be relative but archive/tar accepts absolute header names such as /foo/bar AFAIK.
6384cc7 to
12ef7ab
Compare
There was a problem hiding this comment.
Pull request overview
This PR refines RebaseArchiveEntries so archive entry names (and hardlink targets) are rebased by matching only full path prefixes, with special handling for rebasing from the archive root while preserving the remainder of entry paths verbatim (no cleaning/canonicalization).
Changes:
- Introduces
rebaseNameto perform prefix-only rebasing and handleoldBase == "/"explicitly. - Replaces
strings.Replace-based rebasing forhdr.Nameand hardlinkhdr.Linknamewith the new prefix-safe logic. - Expands test coverage for root rebasing, prefix matching, and preservation of unclean paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| copy.go | Adds rebaseName and uses it to rebase tar header names and hardlink targets safely (prefix-only, root-aware). |
| copy_test.go | Adds test cases covering root rebasing, exact-prefix behavior, and preservation of unclean archive paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
copy_test.go:56
- This test case duplicates the previous "rebase absolute name from root to empty" case (same name and same headerName), and it also omits newBase (implicitly ""). This makes the table harder to read and likely misses the intended coverage (e.g., rebasing a relative name from root to empty).
name: "rebase absolute name from root to empty",
oldBase: "/",
headerName: "/foo/bar",
wantName: "foo/bar",
},
985efe8 to
b1a3ffc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
copy.go:352
- The doc comment says “replacing an occurrence of oldBase with newBase at the beginning of entry names”, but the implementation now only rebases when
oldBasematches a complete path prefix (whole name or followed by "/"), and has special handling for archive-root rebases. Updating the comment would prevent callers from assuming substring replacement semantics.
// RebaseArchiveEntries rewrites the given srcContent archive replacing
// an occurrence of oldBase with newBase at the beginning of entry names.
func RebaseArchiveEntries(srcContent io.Reader, oldBase, newBase string) io.ReadCloser {
rebase := newNameRebaser(oldBase, newBase)
copy.go:328
strings.TrimSuffix(..., "/")removes at most one trailing slash, but the comment says trailing slashes are trimmed and root rebasing should treat base paths like "///" as root. With the current code,oldBase="///"becomes "//" and won’t hit theoldBase == ""root case, andnewBase="prefix//"becomes "prefix/", potentially producing double separators. Usestrings.TrimRight(..., "/")to remove all trailing slashes.
This issue also appears on line 349 of the same file.
oldBase = strings.TrimSuffix(filepath.ToSlash(oldBase), "/")
newBase = strings.TrimSuffix(filepath.ToSlash(newBase), "/")
copy_test.go:56
- This test case is a duplicate of the previous one (same name and effectively the same inputs/expectations since
newBasedefaults to ""). It makes the table harder to read and doesn’t add coverage. Consider repurposing it to cover the missing scenario (relative name from root rebased to empty) or remove it.
name: "rebase absolute name from root to empty",
oldBase: "/",
headerName: "/foo/bar",
wantName: "foo/bar",
},
Handle rebasing from the archive root explicitly so that both relative and absolute entry names are joined to `newBase` with exactly one path separator. Also restrict rebasing to complete path prefixes. The previous use of `strings.Replace` could replace `oldBase` outside the beginning of the name or as part of another path component, such as rebasing `origin` in `original/file`. Apply the same handling to hardlink targets, while preserving the remainder of archive paths without cleaning or canonicalizing them. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b1a3ffc to
8829a25
Compare
…p ci] Bumps the go-modules group in /e2e-go with 2 updates: [github.com/moby/go-archive](https://github.com/moby/go-archive) and [github.com/shirou/gopsutil/v4](https://github.com/shirou/gopsutil). Updates `github.com/moby/go-archive` from 0.2.1 to 0.3.2 Release notes *Sourced from [github.com/moby/go-archive's releases](https://github.com/moby/go-archive/releases).* > v0.3.2 > ------ > > What's Changed > -------------- > > Fix a regression introduced in v0.3.0 that caused archive extraction to fail when paths traversed absolute symlinks inside the destination root, such as `var/run -> /run`. Absolute symlink targets are now resolved relative to the extraction root while relative symlink escapes remain rejected. [moby/go-archive#93](https://redirect.github.com/moby/go-archive/pull/93) > > **Full Changelog**: <moby/go-archive@v0.3.1...v0.3.2> > > v0.3.1 > ------ > > Fixes > ----- > > This patch release fixes a regression introduced in v0.2.1 where archive extraction could fail when an archive omitted explicit entries for parent directories. For example, extracting `etc/dnf/` without a preceding `etc/` entry could return `mkdirat etc/dnf: no such file or directory`. > > This prevented affected images from being extracted. Archive extraction now creates implied parent directories for both file and directory entries. > > What's Changed > -------------- > > * archive: create implied parents for directory entries [moby/go-archive#92](https://redirect.github.com/moby/go-archive/pull/92) > * archive: Tarballer.Go: suppress io.ErrClosedPipe logs on close [moby/go-archive#94](https://redirect.github.com/moby/go-archive/pull/94) > > **Full Changelog**: <moby/go-archive@v0.3.0...v0.3.1> > > v0.3.0 > ------ > > Security > -------- > > This release fixes **CVE-2026-17106** / **[GHSA-hfg8-hc9c-6c3h](https://github.com/moby/go-archive/security/advisories/GHSA-hfg8-hc9c-6c3h)**, where a crafted tar archive could use links to cause extraction operations to create or overwrite files outside the intended destination directory. > > The issue affected `Unpack`, `UnpackLayer`, `Untar`, `UntarUncompressed`, and the `ApplyLayer` helpers. Users should upgrade and avoid extracting untrusted archives with earlier versions. > > What's Changed > -------------- > > * archive: harden tar extraction against path traversal [moby/go-archive#45](https://redirect.github.com/moby/go-archive/pull/45) > * archive: do not follow reparse points in chtimes [moby/go-archive#90](https://redirect.github.com/moby/go-archive/pull/90) > * archive: fix creation time updates on Windows [moby/go-archive#79](https://redirect.github.com/moby/go-archive/pull/79) > * archive: minor cleanups and godoc touch-up [moby/go-archive#87](https://redirect.github.com/moby/go-archive/pull/87) > * archive: RebaseArchiveEntries: fix archive path rebasing [moby/go-archive#43](https://redirect.github.com/moby/go-archive/pull/43) > > Test and CI changes > ------------------- > > * ci: enable dependabot for actions [moby/go-archive#81](https://redirect.github.com/moby/go-archive/pull/81) > * archive: make breakoutErr unwrap its cause [moby/go-archive#91](https://redirect.github.com/moby/go-archive/pull/91) > * archive: use filepath for filesystem paths in tests [moby/go-archive#80](https://redirect.github.com/moby/go-archive/pull/80) > * archive: use filepath for filesystem paths in tests [moby/go-archive#80](https://redirect.github.com/moby/go-archive/pull/80) > > **Full Changelog**: <moby/go-archive@v0.2.1...v0.3.0> Commits * [`9e6d2c7`](moby/go-archive@9e6d2c7) Merge pull request [#93](https://redirect.github.com/moby/go-archive/issues/93) from thaJeztah/fix\_absolute\_symlinks * [`4f6cd58`](moby/go-archive@4f6cd58) archive: resolve hardlinks through absolute symlinks * [`e564ecc`](moby/go-archive@e564ecc) archive: resolve absolute symlinks within extraction root * [`5bb8a45`](moby/go-archive@5bb8a45) Merge pull request [#94](https://redirect.github.com/moby/go-archive/issues/94) from thaJeztah/denoise * [`1bec7ec`](moby/go-archive@1bec7ec) archive: Tarballer.Go: suppress io.ErrClosedPipe logs on close * [`279fa6d`](moby/go-archive@279fa6d) Merge pull request [#92](https://redirect.github.com/moby/go-archive/issues/92) from thaJeztah/fix\_implied\_directories * [`517985a`](moby/go-archive@517985a) archive: create implied parents for directory entries * [`1c23372`](moby/go-archive@1c23372) Merge pull request [#43](https://redirect.github.com/moby/go-archive/issues/43) from thaJeztah/fix\_rebase\_from\_root * [`8829a25`](moby/go-archive@8829a25) RebaseArchiveEntries: fix archive path rebasing * [`c583b20`](moby/go-archive@c583b20) Merge pull request [#90](https://redirect.github.com/moby/go-archive/issues/90) from thaJeztah/chtimes\_nofollow * Additional commits viewable in [compare view](moby/go-archive@v0.2.1...v0.3.2) Updates `github.com/shirou/gopsutil/v4` from 4.26.6 to 4.26.7 Release notes *Sourced from [github.com/shirou/gopsutil/v4's releases](https://github.com/shirou/gopsutil/releases).* > v4.26.7 > ------- > > What's Changed > -------------- > > ### cpu > > * fix: harden parsers against malformed/truncated input by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2109](https://redirect.github.com/shirou/gopsutil/pull/2109) > * [cpu][windows]: compute cpu-total times from integer ticks by [`@skartikey`](https://github.com/skartikey) in [shirou/gopsutil#2111](https://redirect.github.com/shirou/gopsutil/pull/2111) > * [darwin][process]: fix errno handling and library lifetime on darwin by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2119](https://redirect.github.com/shirou/gopsutil/pull/2119) > * [cpu][windows]: compute total counters from individual stats to handle processor groups correctly by [`@srebhan`](https://github.com/srebhan) in [shirou/gopsutil#2125](https://redirect.github.com/shirou/gopsutil/pull/2125) > * [cpu][windows]: harden the cpu-total computation added in [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2128](https://redirect.github.com/shirou/gopsutil/pull/2128) > > ### net > > * fix(net): pad GetExtendedTcpTable buffer to prevent GC thrashing on Windows by [`@HarshalPatel1972`](https://github.com/HarshalPatel1972) in [shirou/gopsutil#2108](https://redirect.github.com/shirou/gopsutil/pull/2108) > > ### process > > * process: implement Darwin IOCounters via proc\_pid\_rusage by [`@DavRack`](https://github.com/DavRack) in [shirou/gopsutil#2117](https://redirect.github.com/shirou/gopsutil/pull/2117) > > ### other > > * feat: add psutil comparison tests for cpu, mem and load by [`@shirou`](https://github.com/shirou) in [shirou/gopsutil#2114](https://redirect.github.com/shirou/gopsutil/pull/2114) > > New Contributors > ---------------- > > * [`@DavRack`](https://github.com/DavRack) made their first contribution in [shirou/gopsutil#2117](https://redirect.github.com/shirou/gopsutil/pull/2117) > * [`@srebhan`](https://github.com/srebhan) made their first contribution in [shirou/gopsutil#2125](https://redirect.github.com/shirou/gopsutil/pull/2125) > > **Full Changelog**: <shirou/gopsutil@v4.26.6...v4.26.7> Commits * [`52a24c8`](shirou/gopsutil@52a24c8) Merge pull request [#2128](https://redirect.github.com/shirou/gopsutil/issues/2128) from shirou/feat/follow-up-2125 * [`268a953`](shirou/gopsutil@268a953) [cpu][windows]: harden the cpu-total computation added in [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) * [`1e34da6`](shirou/gopsutil@1e34da6) Merge pull request [#2125](https://redirect.github.com/shirou/gopsutil/issues/2125) from srebhan/fix\_cpu\_windows\_total * [`61f8802`](shirou/gopsutil@61f8802) Merge pull request [#2122](https://redirect.github.com/shirou/gopsutil/issues/2122) from shirou/dependabot/github\_actions/actions/checko... * [`7fb4dcf`](shirou/gopsutil@7fb4dcf) Merge pull request [#2123](https://redirect.github.com/shirou/gopsutil/issues/2123) from shirou/dependabot/github\_actions/actions/setup-... * [`ae7d91a`](shirou/gopsutil@ae7d91a) Merge pull request [#2119](https://redirect.github.com/shirou/gopsutil/issues/2119) from shirou/fix/darwin-errno-and-libcache * [`49052a1`](shirou/gopsutil@49052a1) [darwin][process]: use a PID above PID\_MAX in the not-running tests * [`991b238`](shirou/gopsutil@991b238) [darwin]: pass the remaining Go pointers as unsafe.Pointer on darwin * [`b9930e2`](shirou/gopsutil@b9930e2) Merge pull request [#2124](https://redirect.github.com/shirou/gopsutil/issues/2124) from shirou/dependabot/github\_actions/actions/labele... * [`38a01b4`](shirou/gopsutil@38a01b4) [cpu][windows]: compute total counters from individual stats to handle proces... * Additional commits viewable in [compare view](shirou/gopsutil@v4.26.6...v4.26.7) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore ` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore ` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore ` will remove the ignore condition of the specified dependency and ignore conditions
Handle rebasing from the archive root explicitly so that both relative
and absolute entry names are joined to
newBasewith exactly one pathseparator.
Also restrict rebasing to complete path prefixes. The previous use of
strings.Replacecould replaceoldBaseoutside the beginning of thename or as part of another path component, such as rebasing
origininoriginal/file.Apply the same handling to hardlink targets, while preserving the
remainder of archive paths without cleaning or canonicalizing them.