Allow unpacking symlinks that don't escape cwd#450
Merged
isaacs merged 1 commit intoisaacs:mainfrom Jan 20, 2026
Merged
Conversation
Owner
|
Oh, I see, so the target is within the tarball cwd, but just includes |
Contributor
Author
Exactly, yeah. I'll put up a proposed fix in this PR - feel free to take it or rework it yourself. Thanks for the quick response. |
Contributor
Author
nwalters512
changed the title
isaacs
reviewed
Jan 20, 2026
src/unpack.ts
Outdated
| return false | ||
| } | ||
| // Valid relative symlink that stays within cwd | ||
| return true |
Owner
There was a problem hiding this comment.
Not quite. For example, it could be an absolute path that includes .., like consider what would happen if the linkpath is /../etc/passwd.
I'd add a field === 'path' in the next section, but let it fall through to the absolute strip in either case.
Owner
There was a problem hiding this comment.
I think this fixes it:
diff --git a/src/unpack.ts b/src/unpack.ts
index 8f256d0..51c9488 100644
--- a/src/unpack.ts
+++ b/src/unpack.ts
@@ -275,43 +275,43 @@ export class Unpack extends Parser {
const parts = p.split('/')
- // For linkpath, check if the resolved path escapes cwd rather than
- // just rejecting any path with '..' - relative symlinks like
- // '../sibling/file' are valid if they resolve within the cwd.
- if (field === 'linkpath' && parts.includes('..')) {
- // Resolve linkpath relative to the entry's directory. `path.posix` is
- // safe to use because we're operating on tar paths, not a filesystem.
- const entryDir = path.posix.dirname(entry.path)
- const resolved = path.posix.normalize(
- path.posix.join(entryDir, p),
- )
- // If the resolved path escapes (starts with ..), reject it
- if (resolved.startsWith('../') || resolved === '..') {
- this.warn(
- 'TAR_ENTRY_ERROR',
- `${field} escapes extraction directory`,
- {
- entry,
- [field]: p,
- },
- )
- return false
- }
- // Valid relative symlink that stays within cwd
- return true
- }
-
if (
parts.includes('..') ||
/* c8 ignore next */
(isWindows && /^[a-z]:\.\.$/i.test(parts[0] ?? ''))
) {
- this.warn('TAR_ENTRY_ERROR', `${field} contains '..'`, {
- entry,
- [field]: p,
- })
- // not ok!
- return false
+ // For linkpath, check if the resolved path escapes cwd rather than
+ // just rejecting any path with '..' - relative symlinks like
+ // '../sibling/file' are valid if they resolve within the cwd.
+ // For paths, they just simply may not ever use .. at all.
+ if (field === 'path') {
+ this.warn('TAR_ENTRY_ERROR', `${field} contains '..'`, {
+ entry,
+ [field]: p,
+ })
+ // not ok!
+ return false
+ } else {
+ // Resolve linkpath relative to the entry's directory.
+ // `path.posix` is safe to use because we're operating on
+ // tar paths, not a filesystem.
+ const entryDir = path.posix.dirname(entry.path)
+ const resolved = path.posix.normalize(
+ path.posix.join(entryDir, p),
+ )
+ // If the resolved path escapes (starts with ..), reject it
+ if (resolved.startsWith('../') || resolved === '..') {
+ this.warn(
+ 'TAR_ENTRY_ERROR',
+ `${field} escapes extraction directory`,
+ {
+ entry,
+ [field]: p,
+ },
+ )
+ return false
+ }
+ }
}
// strip off the root
Contributor
Author
PR-URL: isaacs#450 Credit: @nwalters512 Close: isaacs#450 Reviewed-by: @isaacs EDIT(@isaacs): fixed for test coverage and to disallow absolute linkpaths that contain `..`
isaacs
force-pushed
the
nwalters512/symlink-dotdot-repro
branch
from
46e98fd to
e9a1ddb
Compare
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
2 participants
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.
Given that I was able to write a failing test to demonstrate my issue, I opted to open a PR to facilitate discussion. If you'd rather I close this and open an issue instead, please let me know!
340eb28 ended up being a regression for my application, which relies on being able to un-tar archives that contain symlinks that point within the archive itself. Note that I do not use, and do not want to use,
preservePaths: truebecause of the obvious security implications. To the best of my knowledge, there's no security reason to avoid resolving symlinks that don't refer to other paths outside the archive.If there's agreement that (a) this is a problem and (b) a reasonable fix would be to permit relative symlinks that don't point outside of the tarball, I'd be happy to add an attempted fix to this PR. My proposal would be to replace this check with one that doesn't just check for the presence of
..:node-tar/src/unpack.ts
Line 278 in 911c886
If behaving this way under the default
preservePaths: falsestill isn't acceptable, I'd propose a new option to opt into what should still be safe and correct behavior.