archive: skip files that cannot be represented on Windows - #60
Draft
thaJeztah wants to merge 9 commits into
Draft
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 65.75% 64.90% -0.85%
==========================================
Files 42 42
Lines 2038 2060 +22
==========================================
- Hits 1340 1337 -3
- Misses 528 545 +17
- Partials 170 178 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Windows compatibility during tar extraction by skipping archive entries whose names (or hardlink targets) can’t be faithfully represented on Windows filesystem semantics.
Changes:
- Add a Windows-only guard during
Unpackto skip entries with names/hardlink targets containing problematic characters. - Introduce
unrepresentableOnWindows(*tar.Header)helper to centralize the representability check and produce a descriptive error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+936
to
+948
| func unrepresentableOnWindows(hdr *tar.Header) error { | ||
| if runtime.GOOS != "windows" { | ||
| return nil | ||
| } | ||
| if strings.ContainsAny(hdr.Name, `:\`) { | ||
| return fmt.Errorf("entry name %q contains a character Windows cannot represent in a path", hdr.Name) | ||
| } | ||
| // A hardlink target is resolved within the root by os.Root.Link; a symlink | ||
| // target is stored verbatim, so only hardlinks need the target checked. | ||
| if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) { | ||
| return fmt.Errorf("hardlink target %q contains a character Windows cannot represent in a path", hdr.Linkname) | ||
| } | ||
| return nil |
Member
Author
|
Fun; looks like shelling out to tar can produce ... unexpected results; looks like bash shell / MSYS2 is doing magic conversions. === RUN TestUntarPathWithInvalidDest
time="2026-07-21T17:58:11Z" level=warning msg="Windows: ignoring entry: entry name \"c\\\\Users\\\\RUNNER~1\\\\AppData\\\\Local\\\\Temp\\\\TestUntarPathWithInvalidDest980358182\\\\001\\\\src\" contains a character Windows cannot represent in a path"
archive_test.go:128: UntarPath with invalid destination path should throw an error.
--- FAIL: TestUntarPathWithInvalidDest (0.09s)
=== RUN TestUntarPathWithInvalidSrc
--- PASS: TestUntarPathWithInvalidSrc (0.00s)
=== RUN TestUntarPath
time="2026-07-21T17:58:12Z" level=warning msg="Windows: ignoring entry: entry name \"c\\\\Users\\\\RUNNER~1\\\\AppData\\\\Local\\\\Temp\\\\TestUntarPath785004182\\\\001\\\\src\" contains a character Windows cannot represent in a path"
archive_test.go:169: Destination folder should contain the source file but did not.
--- FAIL: TestUntarPath (0.05s)
=== RUN TestUntarPathWithDestinationFile
time="2026-07-21T17:58:12Z" level=warning msg="Windows: ignoring entry: entry name \"c\\\\Users\\\\RUNNER~1\\\\AppData\\\\Local\\\\Temp\\\\TestUntarPathWithDestinationFile3555294284\\\\001\\\\src\" contains a character Windows cannot represent in a path"
archive_test.go:198: UntarPath should throw an error if the destination if a file
--- FAIL: TestUntarPathWithDestinationFile (0.04s)
=== RUN TestUntarPathWithDestinationSrcFileAsFolder
time="2026-07-21T17:58:12Z" level=warning msg="Windows: ignoring entry: entry name \"c\\\\Users\\\\RUNNER~1\\\\AppData\\\\Local\\\\Temp\\\\TestUntarPathWithDestinationSrcFileAsFolder1938143071\\\\001\\\\src\" contains a character Windows cannot represent in a path"
--- PASS: TestUntarPathWithDestinationSrcFileAsFolder (0.05s) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
archive.go:849
unrepresentableOnWindows()is called afterhdr.Name = filepath.Clean(hdr.Name). On Windows,filepath.Cleannormalizes separators to\, sostrings.ContainsAny(hdr.Name,:\)will match for virtually every non-root entry (because it now contains\path separators), causing Unpack to skip most archive entries on Windows.
Call unrepresentableOnWindows() before the filepath.Clean normalization so that the backslash check only applies to backslashes that were present in the tar header (i.e., literal \ in the tar name), not path separators introduced by normalization.
// Skip entries whose name (or hardlink target) Windows cannot represent.
if err := unrepresentableOnWindows(hdr); err != nil {
log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err)
continue loop
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
archive.go:849
- On Windows, hdr.Name is already normalized with filepath.Clean above, which converts forward slashes to backslashes. That means unrepresentableOnWindows() will see "\" for ordinary nested tar paths (e.g. "a/b" -> "a\b") and incorrectly skip most/all entries. Run the Windows-representability check on the raw tar header name before calling filepath.Clean, or preserve the original name for the check.
// Skip entries whose name (or hardlink target) Windows cannot represent.
if err := unrepresentableOnWindows(hdr); err != nil {
log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err)
continue loop
}
archive.go:936
- The doc comment mentions os.Root/os.Root.Link as the reason backslashes are problematic, but this package doesn’t appear to use os.Root at all. This makes the rationale hard to follow for readers; consider rewording to describe the actual behavior here (Windows path semantics + filepath.Clean/Join treating "\" as a separator).
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by os.Root (e.g. "a\b" resolved as two components). Symlink
// targets are stored verbatim (not resolved at creation), so they are exempt.
func unrepresentableOnWindows(hdr *tar.Header) error {
archive.go:936
- New Windows-specific skipping behavior (invalid tar entry names / hardlink targets) isn’t covered by tests. A small Windows-only test (e.g. in *_windows_test.go) that verifies entries with "\" or ":" are skipped (and valid entries still extract) would help prevent regressions like ordering/normalization issues.
func unrepresentableOnWindows(hdr *tar.Header) error {
Member
Author
|
OK; quick hack to use The proper (predictable) solution is to actually construct the Tar with Go, but I was also slightly curious if this is a real-world scenario. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Skip entries whose name (or hardlink target) Windows cannot represent. | ||
| if err := unrepresentableOnWindows(hdr); err != nil { | ||
| log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err) | ||
| continue loop |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
archive.go:960
- This introduces new Windows-only behavior (skipping entries/hardlink targets containing ':' or '\'), but there is no targeted test validating that Untar/Unpack skips these entries (and does not create unexpected paths) on Windows. Adding a Windows-only unit test that builds an in-memory tar with such headers and asserts the destination remains unchanged would help prevent regressions.
// unrepresentableOnWindows returns an error describing why a tar entry cannot
// be faithfully created on Windows, or nil if it can (always on non-Windows).
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by os.Root (e.g. "a\b" resolved as two components). Symlink
// targets are stored verbatim (not resolved at creation), so they are exempt.
func unrepresentableOnWindows(hdr *tar.Header) error {
if runtime.GOOS != "windows" {
return nil
}
if strings.ContainsAny(hdr.Name, `:\`) {
return fmt.Errorf("entry name %q contains a character Windows cannot represent in a path", hdr.Name)
}
// A hardlink target is resolved within the root by os.Root.Link; a symlink
// target is stored verbatim, so only hardlinks need the target checked.
if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) {
return fmt.Errorf("hardlink target %q contains a character Windows cannot represent in a path", hdr.Linkname)
}
return nil
}
Comment on lines
+869
to
872
| dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name)) | ||
| rel, err := filepath.Rel(dest, dstPath) | ||
| if err != nil { | ||
| return err |
Comment on lines
+856
to
+860
| // Skip entries whose name (or hardlink target) Windows cannot represent. | ||
| if err := unrepresentableOnWindows(hdr); err != nil { | ||
| log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err) | ||
| continue loop | ||
| } |
| // Strip a leading "/" so absolute entries stay root-relative, and | ||
| // normalize the POSIX tar path. Skip entries referring to the extraction | ||
| // root and reject paths that escape it. | ||
| name := path.Clean(strings.TrimLeft(hdr.Name, "/")) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
archive.go:943
- The doc comments for unrepresentableOnWindows mention os.Root and os.Root.Link, but this package doesn't use os.Root anywhere. This makes the rationale harder to follow and could mislead future maintainers.
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by os.Root (e.g. "a\b" resolved as two components). Symlink
// targets are stored verbatim (not resolved at creation), so they are exempt.
func unrepresentableOnWindows(hdr *tar.Header) error {
Comment on lines
+842
to
+844
| if !filepath.IsLocal(name) { | ||
| return breakoutError(fmt.Errorf("invalid entry name %q", hdr.Name)) | ||
| } |
Comment on lines
+937
to
+939
| // unrepresentableOnWindows returns an error describing why a tar entry cannot | ||
| // be faithfully created on Windows, or nil if it can (always on non-Windows). | ||
| // On Windows ":" is illegal in a filename and "\" is a path separator, so a tar |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
archive.go:943
- The
unrepresentableOnWindowsdocstring mentionsfilepath.Clean, but this code now normalizes tar paths usingpath.Cleanand converts separators viafilepath.FromSlashbefore joining. Updating the comment avoids misleading future readers about where misinterpretation can occur.
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by filepath.Clean / filepath.Join (e.g. "a\b" treated as two
// components). Symlink targets are stored verbatim (not resolved at creation),
// so they are exempt.
archive.go:856
- This introduces new Windows-specific behavior (skipping entries whose names or hardlink targets contain ':' or '\'). There are Windows-only tests in this repo, but none appear to cover this extraction behavior; adding a regression test would help ensure the skip logic doesn’t accidentally turn into an error or a path-traversal issue on future refactors.
// Skip entries whose name (or hardlink target) Windows cannot represent.
if err := unrepresentableOnWindows(hdr); err != nil {
log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err)
continue loop
}
Comment on lines
98
to
100
| // #nosec G305 -- The joined path is guarded against path traversal. | ||
| dstPath := filepath.Join(dest, hdr.Name) | ||
| rel, err := filepath.Rel(dest, dstPath) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| // Note as these operations are platform specific, so must the slash be. | ||
| if strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { | ||
| return 0, breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest)) | ||
| } | ||
| dstPath := filepath.Join(dest, filepath.FromSlash(hdr.Name)) | ||
| base := filepath.Base(dstPath) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
archive.go:943
- The doc comment mentions filepath.Clean, but tar entry normalization in this code path uses path.Clean + filepath.FromSlash; the misinterpretation risk here is primarily from filepath operations (e.g. Join/FromSlash) on Windows separators. Please update the comment to match the actual implementation so it stays accurate over time.
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by filepath.Clean / filepath.Join (e.g. "a\b" treated as two
// components). Symlink targets are stored verbatim (not resolved at creation),
// so they are exempt.
archive.go:953
- This comment refers to os.Root.Link, but the hardlink handling in createTarFile uses filepath.Join + prefix checks + os.Link (archive.go:475-481). Updating the comment avoids sending readers looking for a non-existent API in this codebase.
// A hardlink target is resolved within the root by os.Root.Link; a symlink
// target is stored verbatim, so only hardlinks need the target checked.
if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) {
archive.go:856
- New Windows-specific skip behavior (via unrepresentableOnWindows) doesn’t appear to be covered by tests. Since the repo already has Untar/ApplyLayer breakout tests and Windows-specific test files, consider adding a focused test that verifies Unpack/UntagLayer skip (not fail) for entries with Windows-invalid characters (e.g. ':' or '\') and for hardlink targets.
// Skip entries whose name (or hardlink target) Windows cannot represent.
if err := unrepresentableOnWindows(hdr); err != nil {
log.G(context.TODO()).Warnf("Windows: ignoring entry: %v", err)
continue loop
}
Comment on lines
+948
to
+950
| if strings.ContainsAny(hdr.Name, `:\`) { | ||
| return fmt.Errorf("entry name %q contains a character Windows cannot represent in a path", hdr.Name) | ||
| } |
Comment on lines
+953
to
+955
| if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) { | ||
| return fmt.Errorf("hardlink target %q contains a character Windows cannot represent in a path", hdr.Linkname) | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
archive.go:957
- The new Windows skip behavior is not covered by tests. Consider adding Windows-only unit tests (e.g.
//go:build windows) that verifyUnpack/UnpackLayerskip entries whosehdr.Namecontains ':' or '\', and that hardlinks with suchhdr.Linknameare skipped rather than causing extraction to fail.
// unrepresentableOnWindows returns an error describing why a tar entry cannot
// be faithfully created on Windows, or nil if it can (always on non-Windows).
// On Windows ":" is illegal in a filename and "\" is a path separator, so a tar
// name or hardlink target containing them (they use POSIX semantics) would be
// misinterpreted by filepath.Clean / filepath.Join (e.g. "a\b" treated as two
// components). Symlink targets are stored verbatim (not resolved at creation),
// so they are exempt.
func unrepresentableOnWindows(hdr *tar.Header) error {
if runtime.GOOS != "windows" {
return nil
}
if strings.ContainsAny(hdr.Name, `:\`) {
return fmt.Errorf("entry name %q contains a character Windows cannot represent in a path", hdr.Name)
}
// A hardlink target is resolved within the root by os.Root.Link; a symlink
// target is stored verbatim, so only hardlinks need the target checked.
if hdr.Typeflag == tar.TypeLink && strings.ContainsAny(hdr.Linkname, `:\`) {
return fmt.Errorf("hardlink target %q contains a character Windows cannot represent in a path", hdr.Linkname)
}
return nil
}
Comment on lines
+81
to
+85
| basename := path.Base(hdr.Name) | ||
| localBasename, err := filepath.Localize(basename) | ||
| if err != nil || filepath.Base(localBasename) != localBasename { | ||
| return 0, breakoutError(fmt.Errorf("invalid AUFS hardlink name %q", hdr.Name)) | ||
| } |
Comment on lines
+162
to
165
| localBasename, err := filepath.Localize(linkBasename) | ||
| if err != nil || filepath.Base(localBasename) != localBasename { | ||
| return 0, breakoutError(fmt.Errorf("invalid AUFS hardlink name %q", hdr.Linkname)) | ||
| } |
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
This fixes a bug where ExcludePatterns could fail to match valid archive
entries on Windows because filepath.Clean converted tar paths to native
path separators before exclusion matching.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Normalize tar entry names with path.Clean instead of filepath.Clean.
Tar header paths always use POSIX ('/') separators. Preserve their POSIX
form while operating on archive metadata, deferring conversion to native
paths until the filesystem boundary.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use filepath.Localize when converting AUFS hardlink basenames from tar header paths to native filesystem paths. This validates that the basename is a single native path component before it is passed to filepath.Join(), avoiding reinterpretation of Windows path separators. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Only treat hardlinks targeting entries inside /.wh..wh.plnk as AUFS metadata by requiring a path-component boundary after the directory name. This avoids matching similarly prefixed paths while preserving existing handling of AUFS metadata entries and opaque whiteouts. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Normalize tar entry names and reject paths that are not local using filepath.IsLocal. This rejects entries that would escape the extraction root after normalization. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Normalize tar entry names and reject paths that are not local using filepath.IsLocal. This rejects entries that would escape the extraction root after normalization. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Co-authored-by: Cesar Talledo <cesar.talledo@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Uh oh!
There was an error while loading. Please reload this page.