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

path: fix normalize on directories with two dots #14107

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 6, 2017

Fixes: #14105

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

path

@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Jul 6, 2017
@targos
Copy link
Member Author

targos commented Jul 6, 2017

It's probably not the best way to fix the issue.

/cc @mscdex

@refack
Copy link
Contributor

refack commented Jul 6, 2017

Ref: #13683 & #13738

@@ -406,6 +406,7 @@ assert.strictEqual(path.win32.normalize('a//b//.'), 'a\\b');
assert.strictEqual(path.win32.normalize('//server/share/dir/file.ext'),
'\\\\server\\share\\dir\\file.ext');
assert.strictEqual(path.win32.normalize('/a/b/c/../../../x/y/z'), '\\x\\y\\z');
assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\'), 'bar\\');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add:

assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\'), 'bar\\');
assert.strictEqual(path.win32.normalize('bar\\foo..\\..\\baz'), 'bar\\baz');
assert.strictEqual(path.win32.normalize('bar\\foo..\\'), 'bar\\foo..');

Or alternatively find the coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@targos targos force-pushed the fix-path-normalize branch 2 times, most recently from eb9ff15 to 1250227 Compare July 7, 2017 09:18
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM
We could probably refactor this whole thing for TF&I anyway

@mscdex
Copy link
Contributor

mscdex commented Jul 7, 2017

We could probably refactor this whole thing for TF&I anyway

Good luck.

@BridgeAR
Copy link
Member

This needs a rebase

@targos
Copy link
Member Author

targos commented Aug 28, 2017

Looks like I forgot about this PR... Done.

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

@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

CI failed on arm, trying again https://ci.nodejs.org/job/node-test-commit-arm/11784/

targos added a commit to targos/node that referenced this pull request Aug 30, 2017
@targos
Copy link
Member Author

targos commented Aug 30, 2017

Landed in b98e8d9

@targos targos closed this Aug 30, 2017
@targos targos deleted the fix-path-normalize branch August 30, 2017 12:45
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
@MylesBorins
Copy link
Contributor

I've backported to v6.x

Please let me know if it should be backed out

@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
MylesBorins pushed a commit that referenced this pull request Oct 16, 2017
@MylesBorins
Copy link
Contributor

I've landed this back on v6.x along with 19d2d6611c which fixed the security vulnerability.

Please let me know if you think they should be backed out

/cc @nodejs/security @nodejs/tsc

@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants