X Tutup
The Wayback Machine - https://web.archive.org/web/20250814212436/https://github.com/nodejs/node/pull/59286
Skip to content

lib: handle windows reserved device names on UNC #59286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Aug 9, 2025
Merged

lib: handle windows reserved device names on UNC #59286

merged 4 commits into from
Aug 9, 2025

Conversation

RafaelGSS
Copy link
Member

lib: handle windows reserved device names on UNC

We have found that UNC paths weren't covered
when .join/.normalize windows reserved device
names (COM1, LPT1).

Windows paths (specifically, UNC) are confusing, I'm not convinced I have covered all cases where .. would skip device names, but this PR should improve things a bit.

Without this PR a path \\\\?\\COM1:.\\..\\..\\foo2.js would be "normalized" to \\\\?\\foo2.js (skipping the COM1 device completely). This is not a vulnerability because we clearly state that path sanitisation is the user's responsibility in #59262

@RafaelGSS
lib: handle windows reserved device names on UNC
1dade18 
We have found that UNC paths weren't covered
when .join/.normalize windows reserved device
names (COM1, LPT1).
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Jul 30, 2025
@codecov Codecov
Copy link

codecov bot commented Jul 30, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 91.89189% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.91%. Comparing base (751203d) to head (a2ad295).
⚠️ Report is 68 commits behind head on main.

Files with missing lines Patch % Lines
lib/path.js 91.89% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #59286      +/-   ##
==========================================
- Coverage   89.98%   89.91%   -0.07%     
==========================================
  Files         649      655       +6     
  Lines      192180   192865     +685     
  Branches    37655    37806     +151     
==========================================
+ Hits       172934   173419     +485     
- Misses      11866    12005     +139     
- Partials     7380     7441      +61
Files with missing lines Coverage Δ
lib/path.js 96.03% <91.89%> (-0.15%) ⬇️

... and 84 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@RafaelGSS
Copy link
Member Author

cc: @nodejs/path @nodejs/platform-windows

H4ad
H4ad reviewed Jul 30, 2025
View reviewed changes
H4ad
H4ad reviewed Jul 30, 2025
View reviewed changes
H4ad
H4ad reviewed Jul 30, 2025
View reviewed changes
@prabhu
Copy link

prabhu commented Jul 30, 2025 •
edited
Loading

This is good progress! Superscript forms are still missing though. Here is an example.

Apologies, just noticed #59261

@prabhu prabhu mentioned this pull request Jul 31, 2025
Open
@RafaelGSS RafaelGSS added lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x labels Jul 31, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68362/

mcollina
mcollina approved these changes Aug 2, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68404/

@RafaelGSS
Copy link
Member Author

Frequent error on test-startup-empty-regexp-statics, but I'm not certain this is related to my PR nor I have a Windows machine right now to test it locally.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68405/

@richardlau
Copy link
Member

Frequent error on test-startup-empty-regexp-statics, but I'm not certain this is related to my PR nor I have a Windows machine right now to test it locally.

I think it is related as this PR adds a new RegExp usage and is probably triggering #43740. The test itself was added by #43741.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68481/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/68505/

@RafaelGSS RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 9, 2025
@RafaelGSS
Copy link
Member Author

PTAL @jasnell @ronag @mcollina I had to push a small fix to avoid Regexp.

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 9, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 9, 2025
@nodejs-github-bot nodejs-github-bot merged commit a73b575 into nodejs:main Aug 9, 2025
60 of 61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a73b575

RafaelGSS added a commit that referenced this pull request Aug 11, 2025
@RafaelGSS
lib: handle windows reserved device names on UNC
04ad3b4 
We have found that UNC paths weren't covered
when .join/.normalize windows reserved device
names (COM1, LPT1).

PR-URL: #59286
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions github-actions bot mentioned this pull request Aug 11, 2025
Merged
RafaelGSS added a commit that referenced this pull request Aug 12, 2025
@RafaelGSS
lib: handle windows reserved device names on UNC
47543a7 
We have found that UNC paths weren't covered
when .join/.normalize windows reserved device
names (COM1, LPT1).

PR-URL: #59286
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H4ad H4ad H4ad left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@RafaelGSS @prabhu @nodejs-github-bot @richardlau @mcollina @jasnell @ronag @H4ad
X Tutup