url: parsing should not serialize windows drive letter twice #15490

bcoe · 2017-09-20T10:09:29Z

es-modules do not currently work on Windows (we missed this when landing them initially because we'd missed adding a variable to vcbuild.bat/Makefile).

The underlying issue was that node::url::URL.path() was serializing windows style paths incorrectly if there was a drive letter in both the path and the base:

resolve("/D:/a/b/c.mjs", "file:///C:/a/b/c")

would result in an incorrect parse of /C:/D:/a/b/c.mjs.

the same parse in Chrome resolves to:

/D:/a/b/c.mjs.

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)

url,es-module

reviewer: @bmeck, @jasnell

bcoe · 2017-09-20T15:51:53Z

Have been talking with @bmeck I think we've actually found an inconsistency in the URL parsing state machine:

new URL("/C:/a/b/c/module.mjs", "file://D:/foo/bar")

parses differently in Node vs. a browser like Chrome, resulting in the path:

/C:/D:/

which is an invalid Windows path; the actual fix is going to be in the URL parsing state machine not, dropping the drive letter is not to spec.

jasnell · 2017-09-20T16:39:32Z

Will need to double check that this conforms with the url standard before signing off. Ping @TimothyGu

(should be fine, but just need to verify)

bmeck · 2017-09-20T16:40:55Z

@jasnell it looks like a bug in url standard, file slash state pushes base drive letter even if specifier being resolved has a drive letter.

jasnell · 2017-09-20T16:41:53Z

Ping @domenic @annevk

bcoe · 2017-09-20T16:42:26Z

@TimothyGu, @jasnell, @bmeck and I were talking a bit before this pull, this seems to pull us closer to the current behavior of Chrome (and brings us closer to @bmeck's interpretation of the spec) please loop me into any tracking issues I should be following ... this was fun to debug 😛

TimothyGu · 2017-09-20T16:48:24Z

I think the central issue is:

The underlying issue was that node::url::URL.path() was serializing a Windows style file path like so:

/C:/My Awesome Module/foo.

which really shouldn't be the case if the dedicated function for converting a file URL to a path is used:

node/src/node_url.h

Lines 166 to 168 in 75606c4

    
           // Get the path of the file: URL in a format consumable by native file system 
        
           // APIs. Returns an empty string if something went wrong. 
        
           std::string ToFilePath();

bcoe · 2017-09-20T16:50:32Z

@TimothyGu this turned out to be a red herring, the root issue was that the parse method incorrectly uses the Windows drive portion of both the path and base when parsing, creating an invalid Windows path.

bmeck · 2017-09-20T16:59:34Z

@TimothyGu no, the behavior is in url, see:

new URL("/C:/in_c", "file:///D:/in_d")

// spec => file///D:/in_c <- bug in question

// chrome => file:///C:/in_c
// doesn't perform https://url.spec.whatwg.org/#file-slash-state step 2.1.1. ?

// node => file:///D:/C:/in_c
// seems to improperly skip empty check of https://url.spec.whatwg.org/#path-state 1.4.1. ?

TimothyGu

Ha! So this bug has just been fixed in the spec two days ago: whatwg/url#343. We haven't had a chance to implement the change yet, but this PR actually unwittingly implements a variant of that PR.

You might also want to import the Web Platform Tests associated with that change to test/fixtures/url-tests.js, available at web-platform-tests/wpt#7326.

TimothyGu · 2017-09-20T17:27:08Z

src/node_url.cc

@@ -1698,7 +1699,8 @@ void URL::Parse(const char* input,
        } else {
          if (has_base &&
              base->scheme == "file:") {
-            if (IsNormalizedWindowsDriveLetter(base->path[0])) {
+            if (IsNormalizedWindowsDriveLetter(base->path[0]) &&
+                !(remaining > 0 && IsWindowsDriveLetter(ch, p[1]))) {


The spec uses the "starts with a Windows drive letter" algorithm which checks a few other things. You might want to use that instead by factoring out

node/src/node_url.cc

Lines 1670 to 1676 in 75606c4

if ((remaining == 0 ||

!IsWindowsDriveLetter(ch, p[1]) ||

(remaining >= 2 &&

p[2] != '/' &&

p[2] != '\\' &&

p[2] != '?' &&

p[2] != '#'))) {

@TimothyGu 👍 something like?

static inline bool StartsWithWindowsDriveLetter(char ch, const char* p, int remaining) { bool starts_with_drive_letter = false; if !IsWindowsDriveLetter(ch, p[1]) || (remaining >= 2 && p[2] != '/' && p[2] != '\\' && p[2] != '?' && p[2] != '#'))) { starts_with_windows_drive_letter = true; } return starts_with_windows_drive_letter; }

whoops logic is reversed in that snippet, but you get the idea; we'd switch the code you shared to !StartsWithDriveLetter().

You might want to check remaining before accessing p[1]?

bcoe · 2017-09-20T20:09:56Z

see fix in: whatwg/url#343, discussion in whatwg/url#345

TimothyGu · 2017-09-20T20:33:36Z

@bcoe Hey uh, have you seen #15490 (review) yet?

bcoe · 2017-09-20T20:37:55Z

@TimothyGu whoops missed that 👍 lol, all converging on the problem at the same time. I've got a work day at npm, Inc today, but would love to see this over the finish line tonight if you don't mind. It unblocks some other pulls I have open around getting es-module tests passing in CI.

Thanks for your help.

bcoe · 2017-09-21T02:01:35Z

@TimothyGu DRYd up as requested, switched tests over to test the same scenarios as whatwg.

rmisev

I added some suggestions how to make this implementation closer to the URL spec.

rmisev · 2017-09-21T09:16:28Z

src/node_url.cc

+// https://url.spec.whatwg.org/#start-with-a-windows-drive-letter
+static inline bool StartsWithWindowsDriveLetter(char ch,
+                                                const char* p,
+                                                int remaining) {


The URL spec. does not use the remaining here, because it isn't (well) defined when c is EOF.
So I think this function can be rewritten in a bit simpler and universal form:

static inline bool StartsWithWindowsDriveLetter(const char* p, const char* end) { const size_t length = end - p; return length >= 2 && IsWindowsDriveLetter(p[0], p[1]) && (length == 2 || p[2] == '/' || p[2] == '\\' || p[2] == '?' || p[2] == '#'); }

rmisev · 2017-09-21T09:34:43Z

src/node_url.cc

-                    p[2] != '?' &&
-                    p[2] != '#'))) {
+              if (remaining == 0 ||
+                  !StartsWithWindowsDriveLetter(ch, p, remaining)) {


The remaining == 0 is unnecessary, because StartsWithWindowsDriveLetter tests for an empty or too short substring, so:

- if (remaining == 0 || - !StartsWithWindowsDriveLetter(ch, p, remaining)) { + if (!StartsWithWindowsDriveLetter(p, end)) {

rmisev · 2017-09-21T09:56:14Z

src/node_url.cc

@@ -1698,7 +1710,8 @@ void URL::Parse(const char* input,
        } else {
          if (has_base &&
              base->scheme == "file:") {
-            if (IsNormalizedWindowsDriveLetter(base->path[0])) {
+            if (IsNormalizedWindowsDriveLetter(base->path[0]) &&
+              !StartsWithWindowsDriveLetter(ch, p, remaining)) {


I see your implementation is a bit different than in the spec. I thought about such implementation too, but it has a performance problem. Try the test: new URL("/c:/foo/bar", "file://host/path"), it sets URL’s host to base’s host here and later empties URL's host in the path state 1.4.1.1.
So I think it is better to follow the spec. and avoid unnecessary steps:

if (has_base && - base->scheme == "file:") { + base->scheme == "file:" && + !StartsWithWindowsDriveLetter(p, end)) { + if (IsNormalizedWindowsDriveLetter(base->path[0])) { - if (IsNormalizedWindowsDriveLetter(base->path[0]) && - !StartsWithWindowsDriveLetter(ch, p, remaining)) {

@rmisev thank you for the through review, I've implemented your suggestions. I think it might be good to eventually pull in all of url/urltestdata.json to our test suite, I might do that as a follow up pull request (thought @TimothyGu?).

TimothyGu · 2017-09-21T17:53:56Z

@bcoe We already have the entirety of urltestdata.json. It's called test/fixtures/url-test.js. See #15490 (review).

TimothyGu

LGTM after url-test.js is updated (and the nits are addressed).

TimothyGu · 2017-09-21T17:54:56Z

test/cctest/test_url.cc

@@ -104,3 +104,15 @@ TEST_F(URLTest, ToFilePath) {

 #undef T
 }
+
+// https://github.com/w3c/web-platform-tests/pull/7326/files
+TEST_F(URLTest, PathDriveLetter) {


I don't think this test will be necessary once url-test.js is updated.

TimothyGu · 2017-09-21T17:56:41Z

src/node_url.cc

@@ -552,6 +552,19 @@ static inline bool IsSpecial(std::string scheme) {
  return false;
 }

+// https://url.spec.whatwg.org/#start-with-a-windows-drive-letter


nit: add a https://

I think I'm missing something:

// https://url.spec.whatwg.org/#start-with-a-windows-drive-letter

Does have https, perhaps misread?

Oh I'm sorry. It's probably a Chrome extension I'm using.

TimothyGu · 2017-09-21T18:19:27Z

Would you mind updating the commit hash in the file header for url-tests.js as well?

node/test/fixtures/url-tests.js

Line 5 in cd1b55a

    
              https://github.com/w3c/web-platform-tests/blob/8df7c9c215/url/urltestdata.json

Address issue with Windows drive letter handling that was causing es-module test suite to fail, see: whatwg/url#343

bcoe · 2017-09-21T18:31:15Z

@TimothyGu I think I've addressed your comments.

BridgeAR · 2017-09-24T15:05:16Z

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

BridgeAR · 2017-09-24T17:14:36Z

Landed in 456d8e2

Address issue with Windows drive letter handling that was causing es-module test suite to fail. PR-URL: #15490 Ref: whatwg/url#343 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Address issue with Windows drive letter handling that was causing es-module test suite to fail. PR-URL: nodejs/node#15490 Ref: whatwg/url#343 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 20, 2017

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Sep 20, 2017

mscdex added the windows Issues and PRs related to the Windows platform. label Sep 20, 2017

jasnell requested a review from TimothyGu September 20, 2017 16:38

bcoe changed the title ~~url: path() should not serialize drive letter~~ url: url parsing should not serialize windows drive letter twice Sep 20, 2017

bcoe changed the title ~~url: url parsing should not serialize windows drive letter twice~~ url: parsing should not serialize windows drive letter twice Sep 20, 2017

TimothyGu reviewed Sep 20, 2017

View reviewed changes

TimothyGu removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 20, 2017

YurySolovyov mentioned this pull request Sep 20, 2017

File paths have C:\ 2 times martpie/museeks#354

Closed

bcoe mentioned this pull request Sep 20, 2017

Inconsistency between Chrome/Node.js in url parsing spec whatwg/url#345

Closed

rmisev reviewed Sep 21, 2017

View reviewed changes

TimothyGu approved these changes Sep 21, 2017

View reviewed changes

url: fix windows drive letter handling

8c0a56a

Address issue with Windows drive letter handling that was causing es-module test suite to fail, see: whatwg/url#343

bcoe force-pushed the fix-15276 branch from 149f6cf to 8c0a56a Compare September 21, 2017 18:30

jasnell approved these changes Sep 21, 2017

View reviewed changes

rmisev approved these changes Sep 21, 2017

View reviewed changes

TimothyGu approved these changes Sep 21, 2017

View reviewed changes

This was referenced Sep 23, 2017

test: make it easier to run tests for subsystems #15450

Closed

build: run es-module tests in ci environment #15276

Closed

BridgeAR approved these changes Sep 24, 2017

View reviewed changes

BridgeAR closed this Sep 24, 2017

sam-github mentioned this pull request Sep 25, 2017

url: remove unused variable #15614

Closed

2 tasks

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade eslint from 7.32.0 to 8.7.0 Iceymann18777/node#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: parsing should not serialize windows drive letter twice #15490

url: parsing should not serialize windows drive letter twice #15490

bcoe commented Sep 20, 2017 •

edited

Loading

bcoe commented Sep 20, 2017

jasnell commented Sep 20, 2017 •

edited

Loading

bmeck commented Sep 20, 2017

jasnell commented Sep 20, 2017

bcoe commented Sep 20, 2017 •

edited

Loading

TimothyGu commented Sep 20, 2017

bcoe commented Sep 20, 2017

bmeck commented Sep 20, 2017

TimothyGu left a comment

TimothyGu Sep 20, 2017

bcoe Sep 20, 2017

bcoe Sep 20, 2017

TimothyGu Sep 20, 2017

bcoe commented Sep 20, 2017 •

edited

Loading

TimothyGu commented Sep 20, 2017

bcoe commented Sep 20, 2017

bcoe commented Sep 21, 2017

rmisev left a comment

rmisev Sep 21, 2017

rmisev Sep 21, 2017 •

edited

Loading

rmisev Sep 21, 2017

bcoe Sep 21, 2017

TimothyGu commented Sep 21, 2017

TimothyGu left a comment

TimothyGu Sep 21, 2017

TimothyGu Sep 21, 2017

bcoe Sep 21, 2017 •

edited

Loading

TimothyGu Sep 21, 2017

TimothyGu commented Sep 21, 2017

bcoe commented Sep 21, 2017

BridgeAR commented Sep 24, 2017

BridgeAR commented Sep 24, 2017

	if ((remaining == 0 \|\|
	!IsWindowsDriveLetter(ch, p[1]) \|\|
	(remaining >= 2 &&
	p[2] != '/' &&
	p[2] != '\\' &&
	p[2] != '?' &&
	p[2] != '#'))) {

url: parsing should not serialize windows drive letter twice #15490

url: parsing should not serialize windows drive letter twice #15490

Conversation

bcoe commented Sep 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

bcoe commented Sep 20, 2017

jasnell commented Sep 20, 2017 • edited Loading

bmeck commented Sep 20, 2017

jasnell commented Sep 20, 2017

bcoe commented Sep 20, 2017 • edited Loading

TimothyGu commented Sep 20, 2017

bcoe commented Sep 20, 2017

bmeck commented Sep 20, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Sep 20, 2017 • edited Loading

TimothyGu commented Sep 20, 2017

bcoe commented Sep 20, 2017

bcoe commented Sep 21, 2017

rmisev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmisev Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 21, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Sep 21, 2017

bcoe commented Sep 21, 2017

BridgeAR commented Sep 24, 2017

BridgeAR commented Sep 24, 2017

bcoe commented Sep 20, 2017 •

edited

Loading

jasnell commented Sep 20, 2017 •

edited

Loading

bcoe commented Sep 20, 2017 •

edited

Loading

bcoe commented Sep 20, 2017 •

edited

Loading

rmisev Sep 21, 2017 •

edited

Loading

bcoe Sep 21, 2017 •

edited

Loading