Skip to content
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.

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

win32 path.normalize() not correctly normalizing relative paths containing ../ that advance above root #17928

Closed
peteward44 opened this issue Jan 1, 2018 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.

Comments

@peteward44
Copy link

peteward44 commented Jan 1, 2018

  • Version: 9.3.0
  • Platform: Windows 10 64bit
  • Subsystem: path

edit: See below comment for correct replication code

const path = require( 'path' );
const n = path.normalize( '../dir1/../../dir2' );
console.log( n );

Should output ../../dir2 but outputs dir2.

After some investigation it appears to have been introduced via commit b98e8d995efb426bbdee56ce503017bdcbbc6332 (path: fix normalize on directories with two dots)

@gibfahn gibfahn assigned gibfahn and unassigned gibfahn Jan 2, 2018
@gibfahn gibfahn added path Issues and PRs related to the path subsystem. windows Issues and PRs related to the Windows platform. labels Jan 2, 2018
@gibfahn
Copy link
Member

gibfahn commented Jan 2, 2018

cc/ @nodejs/platform-windows @targos

@gibfahn
Copy link
Member

gibfahn commented Jan 2, 2018

Can't reproduce on macOS, so assuming this is Windows specific.

@targos
Copy link
Member

targos commented Jan 2, 2018

Cant' reproduce on Windows either:

> path.normalize('../dir1/../../dir2')
'..\\..\\dir2'

@peteward44
Copy link
Author

This is strange, let me take another look to see what's happening

@peteward44
Copy link
Author

peteward44 commented Jan 2, 2018

OK - my example was bad. Sorry about that, try this:

path.normalize( '../../../dir1/../../../dir2' );

Should output ../../../../../dir2 but outputs ../dir2

@gibfahn gibfahn removed the windows Issues and PRs related to the Windows platform. label Jan 2, 2018
@gibfahn
Copy link
Member

gibfahn commented Jan 2, 2018

Okay, can reproduce that with v6.12.0 but not v6.11.5 (on macOS), which suggests b98e8d9 is indeed the cause of the issue.

▶▶▶ nvm i v6.11.5                                                                                                                                                 ~/wrk/com/DANGER/node (v8.9.4-proposal)
v6.11.5 is already installed.
Now using node v6.11.5 (npm v3.10.10)
▶▶▶ node -p "path.normalize( '../../../dir1/../../../dir2' );"                                                                                                 ~/wrk/com/DANGER/node 2s (v8.9.4-proposal)
../../../../../dir2
▶▶▶ nvm use 6.12.0                                                                                                                                                ~/wrk/com/DANGER/node (v8.9.4-proposal)
Now using node v6.12.0 (npm v3.10.10)
▶▶▶ node -p "path.normalize( '../../../dir1/../../../dir2' );"                                                                                                 ~/wrk/com/DANGER/node 1s (v8.9.4-proposal)
../dir2

@peteward44
Copy link
Author

Cool, i would have submitted a PR myself but i was unsure of the correct behaviour that b98e8d9 was supposed to fix. Thanks for looking at this 👍

@starkwang starkwang added the confirmed-bug Issues with confirmed bugs. label Jan 4, 2018
starkwang added a commit to starkwang/node that referenced this issue Jan 4, 2018
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

Fixes: nodejs#17928
evanlucas pushed a commit that referenced this issue Jan 22, 2018
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
evanlucas pushed a commit that referenced this issue Jan 30, 2018
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 27, 2018
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 27, 2018
After slicing, the `lastSegmentLength` should be calculated again,
instead of assigning value `j`.

PR-URL: #17974
Fixes: #17928
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants