FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

path: fix input type checking regression by mscdex · Pull Request #5244 · nodejs/node · GitHub

/ node Public

path: fix input type checking regression - #5244

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:path-fix-regression
ConversationCommits (1)ChecksFiles changed
Closed

path: fix input type checking regression#5244
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:path-fix-regression

Conversation

mscdex commented Feb 15, 2016

Copy link
Copy Markdown
Contributor

Before b212be0, input types were not checked in some path functions and the inputs were passed directly to regexp.exec() which implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and adds string coercion for those functions.

/cc @thealphanerd

path: fix input type checking regression
5d31282 
Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.
mscdex added the path Issues and PRs related to the path subsystem. label Feb 15, 2016

MylesBorins commented Feb 15, 2016

Copy link
Copy Markdown
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/1671/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/76/

citgm + CI are green (expected failures on ppc)

LGTM

MylesBorins commented Feb 15, 2016

Copy link
Copy Markdown
Contributor

@mscdex what's the semver on this? Would it bring #5123 back down from Major?

mscdex commented Feb 15, 2016

Copy link
Copy Markdown
Contributor Author

@thealphanerd Yes, it should.

mscdex mentioned this pull request Feb 15, 2016
Closed
This was referenced Feb 15, 2016
Closed
Closed

rvagg commented Feb 15, 2016

Copy link
Copy Markdown
Member

yay for smoke testing! yay for more tests! this is a great outcome of more thorough processes.

rvagg commented Feb 15, 2016

Copy link
Copy Markdown
Member

oh, and fix lgtm, 🚢

MylesBorins commented Feb 15, 2016

Copy link
Copy Markdown
Contributor

@mscdex so it looks like this patch is not applying cleanly to v5.x (although it is for master). Would you be able to take a peak?

jasnell commented Feb 15, 2016

Copy link
Copy Markdown
Member

LGTM

thefourtheye reviewed Feb 16, 2016
View reviewed changes
Comment thread lib/path.js

dirname: function dirname(path) {
assertPath(path);
if (typeof path !== 'string')

thefourtheye Feb 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can skip this check and simply do path = '' + path, right?

mscdex Feb 16, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It probably doesn't matter either way. I just did it this way in case v8 doesn't/can't optimize it by not creating a new string if path is already a string.

thefourtheye Feb 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just thinking out loud here. Will path = path.toString() be better then?

mscdex Feb 16, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's effectively the same as path = '' + path but less safe (e.g. won't work on null and other values).

thefourtheye Feb 16, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh yeah. Totally forgot about them.

thefourtheye commented Feb 16, 2016

Copy link
Copy Markdown
Contributor

LGTM

mscdex added a commit that referenced this pull request Feb 17, 2016
path: fix input type checking regression
9209bf6 
Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.

PR-URL: #5244
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

mscdex commented Feb 17, 2016

Copy link
Copy Markdown
Contributor Author

Landed in 9209bf6.

mscdex closed this Feb 17, 2016

mscdex commented Feb 17, 2016

Copy link
Copy Markdown
Contributor Author

@rvagg Do you want me to submit a separate PR for inclusion into v5.x or will it get cherry-picked at some point (and should be tagged with land-on-v5.x despite being closed now)?

mscdex deleted the path-fix-regression branch February 17, 2016 19:26

rvagg commented Feb 18, 2016

Copy link
Copy Markdown
Member

@mscdex the cherry-picking process for v5.x is opt-out rather than opt-in like v4.x so this'll come across in my next update (within the next couple of ours), no need to take action on your part except for PRs that shouldn't go in to v5.x.

Thanks for fixing!

rvagg pushed a commit that referenced this pull request Feb 18, 2016
path: fix input type checking regression
1c6f927 
Before b212be0, input types were not checked in some path functions
and the inputs were passed directly to `regexp.exec()` which
implicitly converts its argument to a string.

This commit both removes the type checking added in b212be0 and
adds string coercion for those functions.

PR-URL: #5244
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins mentioned this pull request Mar 25, 2016
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

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.

5 participants

Back | FazBrowse Home | New Git URL