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

url: return valid file: urls fom url.format() #7234

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@Trott
Member

Trott commented Jun 8, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

url

Description of change

file: URLs that do not start with file:// are invalid. Browsers convert file:/etc/passwd to file:///etc/passwd. This is also what the docs indicate we are doing, but we're not.

Labeling semver-major because tests have to be updated for it.

Fixes: #3361

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

@mscdex

View changes

lib/url.js Outdated
if (this.slashes || host) {
if (pathname && pathname.charCodeAt(0) !== 47/*/*/)
pathname = '/' + pathname;
host = `//${host}`;

This comment has been minimized.

@mscdex

mscdex Jun 8, 2016

Contributor

minor nit: last time i checked template strings had noticeable overhead, may need to check again on master now.

This comment has been minimized.

@Trott

Trott Jun 9, 2016

Member

Changed to string concatenation.

@mscdex

View changes

lib/url.js Outdated
if (pathname && pathname.charCodeAt(0) !== 47/*/*/)
pathname = '/' + pathname;
host = `//${host}`;
} else if (protocol.startsWith('file')) {

This comment has been minimized.

@mscdex

mscdex Jun 8, 2016

Contributor

IIRC startsWith() was still slower than manual checking, for example:

protocol.length >= 4 &&
protocol.charCodeAt(0) === 102/*f*/ &&
protocol.charCodeAt(1) === 105/*i*/ &&
protocol.charCodeAt(2) === 108/*l*/ &&
protocol.charCodeAt(3) === 101/*e*/

This comment has been minimized.

@Trott

Trott Jun 9, 2016

Member

Whoa, I benchmarked it and yeah, that's quite a performance bump. Will put that change in momentarily...

@Trott Trott referenced this pull request Jun 9, 2016

Closed

benchmark: add benchmark for url.format() #7250

3 of 3 tasks complete

@Trott Trott force-pushed the Trott:fileslashslash branch to 4e8c661 Jun 9, 2016

Trott added some commits Jun 8, 2016

url: return valid file: urls fom url.format()
`file:` URLs that do not start with `file://` are invalid. Browsers
convert `file:/etc/passwd` to `file:///etc/passwd`. This is also what
the docs indicate we are doing, but we're not.

Fixes: #3361
@Trott

This comment has been minimized.

Member

Trott commented Jun 10, 2016

/cc @silverwind @claudiorodriguez (who had substantial comments on the issue this tries to address)

@Trott

This comment has been minimized.

@silverwind

This comment has been minimized.

Contributor

silverwind commented Jun 10, 2016

LGTM if CI passes.

@Trott

This comment has been minimized.

Member

Trott commented Jun 10, 2016

I guess since it's semver-major and our docs say that semver-major changes need to be reviewed in some fashion by CTC: @nodejs/ctc

@Trott

This comment has been minimized.

Member

Trott commented Jun 10, 2016

Couple of unrelated-seeming issues on that first CI run. Here's a second one: https://ci.nodejs.org/job/node-test-commit/3707/

@mscdex

This comment has been minimized.

Contributor

mscdex commented Jun 10, 2016

LGTM

@Trott

This comment has been minimized.

Member

Trott commented Jun 10, 2016

@jasnell This would seem to be orthogonal to nodejs/node-eps#28 but uh, just in case, care to review?

pathname = '/' + pathname;
host = '//' + host;
} else if (protocol.length >= 4 &&
protocol.charCodeAt(0) === 102/*f*/ &&

This comment has been minimized.

@indutny

indutny Jun 11, 2016

Member

Nitpick: why not 0x.. instead?

This comment has been minimized.

@Trott

Trott Jun 13, 2016

Member

@indutny I used decimal because:

  • It's decimal in most of the existing charCodeAt() calls, including ~10 lines later in lines 621 and 622, as well as the switch statement ~30 lines earlier starting on line 581.
  • The sample code (provided by @mscdex) used decimal and I did a lazy copy/paste.

I don't feel strongly about it, though, and I'm happy to change it to hex format if that's the difference between a quick LGTM and a not-sure-about-this.

@Trott

This comment has been minimized.

Member

Trott commented Jun 13, 2016

Trying to get more eyes on this... @nodejs/collaborators

@Trott Trott added the ctc-agenda label Jun 14, 2016

@Trott

This comment has been minimized.

Member

Trott commented Jun 14, 2016

Adding ctc-agenda to give CTC a concrete opportunity to stop this from landing if someone has concerns. I think it's pretty mild as semver-major changes go, but a breaking change is a breaking change...

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 15, 2016

For anyone here wondering what whatwg-url spec does, ref: https://url.spec.whatwg.org/#url-serializing.

It always starts file: urls with file://.

@rvagg rvagg removed the ctc-agenda label Jun 15, 2016

@rvagg

This comment has been minimized.

Member

rvagg commented Jun 15, 2016

no objections from ctc

@Trott Trott added this to the 7.0.0 milestone Jun 15, 2016

Trott added a commit to Trott/io.js that referenced this pull request Jun 16, 2016

url: return valid file: urls fom url.format()
`file:` URLs that do not start with `file://` are invalid. Browsers
convert `file:/etc/passwd` to `file:///etc/passwd`. This is also what
the docs indicate we are doing, but we're not.

PR-URL: nodejs#7234
Fixes: nodejs#3361
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Brian White <mscdex@mscdex.net>
@Trott

This comment has been minimized.

Member

Trott commented Jun 16, 2016

Landed in 336b027

@Trott Trott closed this Jun 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment