Uh oh!
There was an error while loading. Please reload this page.
fix: allow safe relative subfolders in filename while still blocking traversal - #32
Merged
Merged
Conversation
…traversal The previous fix for path traversal (CWE-22) in FilenameSchema reduced any filename to its bare basename via path.basename(), which is more aggressive than necessary: it also strips legitimate relative subfolders developers commonly use (e.g. filename: 'output/data.json'), even though those never posed a traversal risk. FilenameSchema now allows relative subfolder segments, and instead explicitly rejects (with a clear ValidationError, not a silent rewrite): - '..' segments anywhere in the path - absolute paths (POSIX '/x', Windows 'C:\x', UNC '\\host\share') Backslashes are always treated as separators (not just on Windows), so a Windows-shaped traversal payload can't slip through on a POSIX host by switching separator style. getAbsAndEnsureDir (utils/files.ts) is the single chokepoint all three write paths converge on (saveResults, SnapshotAPI.download, and BaseResult.save() — the last of which accepts a raw path that never passes through FilenameSchema at all), so it now enforces containment itself rather than assuming callers already sanitized: 1. Lexical containment: resolved candidate must stay under baseDir. 2. Real-path containment: after mkdir, re-resolve via fs.realpath and re-check, so a subfolder that is (or contains) a symlink pointing outside baseDir can't be used to escape it — lexical resolution alone is blind to symlinks. Containment is only enforced for *relative* inputs: an absolute path is a deliberate choice by the calling code (same trust model as calling fs.writeFile directly) — BaseResult.save() has always supported writing to an arbitrary absolute destination, and that's preserved. Tests: rewrote tests/files.test.ts's FilenameSchema suite for the new allow-subfolders/reject-traversal behavior, added a dedicated getAbsAndEnsureDir suite (nested subfolders, relative-traversal rejection, explicit-absolute-path pass-through, symlink-escape rejection), extended saveResults integration tests (nested subfolder write, symlink-escape rejection) and added BaseResult.save() coverage for the schema-bypass path.
…min) fs.symlink() typically requires Developer Mode or admin rights on Windows, unrelated to whether the containment fix itself works there. Skip the two symlink-escape tests via it.skipIf(isWindows) instead of letting them fail on an environment limitation rather than an actual regression.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Summary
Follow-up to the path-traversal fix (CWE-22) already on
main. That fix reduced anyfilenameto its bare basename viapath.basename()— safe, but more aggressive than necessary: it also stripped legitimate relative subfolders developers commonly use (e.g.filename: 'output/data.json'), even though those never posed a traversal risk. Since that basename-only behavior was never published to npm, this PR is what users will actually see as the shipped behavior.What changed
FilenameSchema(src/schemas/shared.ts) — now allows relative subfolder segments (output/data.json,a/b/c/file.txt), and instead explicitly rejects, with a clearValidationError(not a silent rewrite):".."segments anywhere in the path/etc/x), Windows (C:\x), UNC (\\host\share)Backslashes are always treated as separators (not just on Windows), so a Windows-shaped traversal payload can't slip through on a POSIX host by switching separator style.
getAbsAndEnsureDir(src/utils/files.ts) — the single chokepoint all three write paths converge on (saveResults,SnapshotAPI.download, andBaseResult.save()— the last of which accepts a raw path that never passes throughFilenameSchemaat all) — now enforces containment itself rather than assuming callers already sanitized:mkdir, re-resolve viafs.realpathand re-check, so a subfolder that is (or contains) a symlink pointing outside the base directory can't be used to escape it — lexical resolution alone is blind to symlinks.Containment is only enforced for relative inputs. An absolute path is a deliberate choice by the calling code (same trust model as calling
fs.writeFiledirectly) —BaseResult.save()has always supported writing to an arbitrary absolute destination (see the pre-existing/tmp/...test intests/result.test.ts), and that's preserved unchanged.User-facing behavior (vs. the currently-published 1.1.0)
filename/filepathsaveResults/SnapshotAPI.downloadresult.save()'output/data.json'(relative subfolder)output/and writes there'../etc/passwd','output/../../x'ValidationError(clear, fails fast)FSError(containment check)'/etc/passwd','C:\x','\\\\host\\share'ValidationError.., but a non-..-shaped symlinked folder is still caught)FSError(symlink escape detected viafs.realpath)Tests
Rewrote
tests/files.test.ts:FilenameSchemasuite for the new allow-subfolders/reject-traversal behavior.getAbsAndEnsureDirsuite: nested subfolders, relative-traversal rejection, explicit-absolute-path pass-through, symlink-escape rejection.saveResultsintegration tests: nested subfolder write, symlink-escape rejection.BaseResult.save()coverage for the schema-bypass path (still enforces relative-traversal/symlink protection, still honors absolute paths).fs.symlink()needs Developer Mode/admin there — an environment limitation, not a regression) viait.skipIf.Verification
sanity(lint + typecheck): clean.build+smoke-dist(ESM/CJS load from actualdist/): clean.