path: match Windows reserved names by component boundary - #61545
path: match Windows reserved names by component boundary#61545HassanFleyah wants to merge 1 commit into
Conversation
|
Review requested:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens path.win32.normalize() handling for Windows device paths so that reserved device names prefixed with \\.\ or \\?\ are correctly detected even when the trailing colon is missing. It aims to close an edge case where such paths could bypass existing normalization logic.
Changes:
- Refactors the UNC/device-root detection branch in
win32.normalize()to special-casefirstPart === '.' || firstPart === '?'for device roots. - Adds new logic to detect reserved device names when a
\\.\or\\?\namespace prefix is present but the path lacks a colon, usingWINDOWS_RESERVED_NAMESdirectly on the substring after the prefix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Gentle reminder for the maintainers to authorize the pending workflows |
1 similar comment
|
Gentle reminder for the maintainers to authorize the pending workflows |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61545 +/- ##
=======================================
Coverage 89.67% 89.68%
=======================================
Files 676 676
Lines 206555 206567 +12
Branches 39554 39555 +1
=======================================
+ Hits 185230 185250 +20
+ Misses 13461 13451 -10
- Partials 7864 7866 +2
🚀 New features to boost your workflow:
|
e01e1c1 to
1fb9ea6
Compare
|
Enhanced coverage with a new test suite, squashed all changes into a single commit, and updated the existing tests for better comprehensiveness. @Flarna could you please approve the pending workflows to trigger the CI checks? |
1fb9ea6 to
ab22425
Compare
|
Fixed the no-lonely-if linter error. @Flarna , could you please approve the pending workflows to trigger the CI checks? Thank you |
ab22425 to
86e06aa
Compare
|
Hi @Flarna , gentle follow-up on this. The PR is now green with all checks passing and coverage improved. Would appreciate a final review when you have a moment. Thank you! |
|
I'm not really an expert in this area. Maybe anyone from @nodejs/path could take a look please? |
| const colonIndex = StringPrototypeIndexOf(path, ':'); | ||
| if (isWindowsReservedName(path, colonIndex)) { | ||
| // Ensure colonIndex is valid before calling isWindowsReservedName | ||
| if (colonIndex !== -1 && isWindowsReservedName(path, colonIndex)) { |
There was a problem hiding this comment.
I think this breaks the existing { input: 'COM9.', expected: '.\\COM9.' }, testcase.
|
I see that at least these 2 tests (maybe more) fail now:
Please look into that (or even better, run the full test suite) and fix all of the issues. Thanks in advance. |
86e06aa to
68f19a1
Compare
|
I've rewritten this PR completely — new title, new description, nothing left @Flarna was right about @StefanStojanovic was right about the failing tests. The device-root branch I What replaces it addresses the underlying problem rather than the symptom. The Details, the two assertions this changes, and the fuzzing I used to bound the Sorry for the noise on the earlier revisions, and thanks to both of you for |
|
request-ci |
|
I couldn't apply the \semver-major\ label due repository permissions. Could a maintainer please add it? |
4ec2201 to
49fb702
Compare
What
win32.normalize()prefixes a path with.\when it starts with a Windowsreserved device name (
CON,NUL,PRN,COM1,LPT1, ...), so the resultcannot be read as a device reference.
The no-colon case was handled by calling
isWindowsReservedName(path, -1),which slices off the last character and compares the remainder against the
reserved list. That is an accident of
String.prototype.slice(0, -1), not arule, and it is wrong in both directions.
False positives — ordinary files rewritten as devices:
False negatives — real device references left untouched:
Per Microsoft's file-naming documentation, "avoid these names followed
immediately by an extension; NUL.txt and NUL.tar.gz are both equivalent to
NUL", so the second group are device references and the prefix is what they
need.
How
Take the leading path component, and if the part before its first
.or:is a reserved name, treat it as a device. A bare reserved name carrying
neither is left alone, so
normalize('CON') === 'CON'is preserved. One rulecovers both directions.
What this breaks
Two existing assertions change, both in the false-negative group:
They are updated here. This is user-visible, so the PR should carry
semver-major.Nothing else moves. The CVE-2024-36139 assertions, the UNC cases, the
drive-letter cases and the
\\.\/\\?\cases are unchanged — 80 of the 82assertions across
test-path-win32-normalize-device-names.jsandtest-path-normalize.jsproduce identical output.Verification
I built executable copies of
lib/path.js— currentmain, the narrowing in#64266, and this patch — and confirmed the unmodified copy matches
require('path')exactly before comparing.Against #64266 over 297,026 generated inputs: 7,219 inputs differ, every one
of them a reserved name followed by a dot and an extension. Zero differences
of any other kind, so this is a strict superset of that narrowing rather than
a competing rule.
Refs
colonIndex !== -1; landed, then revertedin Revert "path: fix win32 normalize false-positive on reserved names" #64216 because it also dropped the
COM9.casepositives, leaves the false negatives
This PR previously proposed the same
colonIndex !== -1guard, in January.See the comment below for why it was rewritten.