Resolve symlinks before the previous-source-map containment check - #2125
Merged
Conversation
relative() is textual and never resolves symlinks, so a symlink at or below the CSS file’s directory passes the guard and readFileSync() follows it out of the tree. Resolve both sides with realpathSync first so the check rejects by outcome rather than syntax.
Member
|
Thanks. I will release it later with some next feature or fix. |
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.
Follow-up to the hardening note in GHSA-r395-w6c4-rj3r — thanks for the invitation to send this as a normal PR.
What
loadFile()guards the previous-source-map load with:path.relative()is textual and never resolves symlinks. A symlink at or below the CSS file's own directory therefore returns something like"link.map"— no.., not absolute — the guard passes, andreadFileSync()follows the link out of the tree.This resolves both sides with
realpathSyncbefore comparing, so the check rejects by outcome rather than by syntax. A missing or dangling target falls back to the literal path; that cannot escape, because the existingexistsSync()gate still applies.Verified against 8.5.23
%2f/%5c, double-encoded, backslash,....//, uppercase.MAP, Windows drive,file://, UNC, null byte, fullwidth solidus, newline).mapstill loadsfromunset)The control row is the one that matters: the patch must not simply stop loading maps, and it doesn't.
Scope
Not a security fix. Reaching this needs an attacker who can already place a symlink inside the project tree, and the threat model here is untrusted CSS content, not an untrusted filesystem — which is why I sent it as a hardening note rather than an advisory in the first place.
Happy to add a regression test in
test/previous-map.test.tsalongside the existing traversal cases (createproject/link.map -> <outside>/secret.map, assert the map is not loaded; skipped on Windows, where unprivileged symlink creation fails). Say the word and I'll push it to this branch.