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

Should 'C%3A' or 'C%7C' be interpreted as 'C:' and 'C|'? #210

Closed
jasnell opened this issue Jan 16, 2017 · 11 comments
Closed

Should 'C%3A' or 'C%7C' be interpreted as 'C:' and 'C|'? #210

jasnell opened this issue Jan 16, 2017 · 11 comments
Labels
good first issue Ideal for someone new to a WHATWG standard or software project needs tests Moving the issue forward requires someone to write tests

Comments

@jasnell
Copy link
Collaborator

jasnell commented Jan 16, 2017

Given the examples new URL('file:///C%3A/a/b/c') and new URL('file:///C%7C/a/b/c'), the parsed pathnames do not unescape the %3A (:) and %7C (|). Technically speaking, those should be considered valid Windows drive letters. Currently neither the Node.js, Firefox, or Chrome impls decode the %-encoded : and | characters.

@domenic
Copy link
Member

domenic commented Jan 16, 2017

Technically speaking, those should be considered valid Windows drive letters.

What does "should" mean here? Who says they should be considered valid Windows drive letters?

@jasnell
Copy link
Collaborator Author

jasnell commented Jan 16, 2017

Should in the same sense that /%2e/ is interpreted as a valid single dot segment and /%2e%2e/ is interpreted as a valid double dot segment, etc. It's a bit inconsistent to pct-decode in some cases while not in others.

@jasnell
Copy link
Collaborator Author

jasnell commented Jan 16, 2017

For instance,

> var m = new url.URL('file:///c%3a/%2e/a/%2e%2e/b')
undefined
> m
URL {
  href: file:///c%3a/b
  protocol: file:
  pathname: /c%3a/b
}

@domenic
Copy link
Member

domenic commented Jan 16, 2017

I believe the consensus of recent work was that %2e is special (and is only decoded in special cases, itself). See #87

@jasnell
Copy link
Collaborator Author

jasnell commented Jan 16, 2017

Indeed, but windows drive letters are also already special cased in the URL parsing algorithm and anyone processing the URL appropriately (by decoding any pct-encoded characters prior to using it) can end up with rather unexpected results if they are not being careful.

> var m = new url.URL('file://c%3a/%2e/a/%2e%2e/b')
undefined
> m
URL {
  href: file://c:/b
  protocol: file:
  hostname: c:
  pathname: /b
}

> var m = new url.URL('file:///c%3a/%2e/a/%2e%2e/b')
undefined
> m
URL {
  href: file://c:/b
  protocol: file:
  pathname: /c%3a/b
}

I believe it at least warrants some additional consideration.

@annevk
Copy link
Member

annevk commented Jan 17, 2017

Does Edge do this or something? If nobody does this, I'm not sure why we would. The drive letter system is already complicated enough.

@jasnell
Copy link
Collaborator Author

jasnell commented Jan 17, 2017

I was kind of hoping it didn't but I just checked and the results are fairly surprising...

Edge does nothing with %7C. It ignores it the same as Firefox, Chrome and Node.js

It does, however, decode %3A, with different results based on how it appears:

file:///c%3a/a/b/c

{
   href: "file:///c:/a/b/c",
   pathname: "/c:/a/b/c",
   protocol: "file:",
}

Note the decoded : in the pathname

However,

file://c%3a/a/b/c

Results in an error being thrown with the message "A security problem occurred"

The security error implies that the %3a is being decoded by Edge because the equivalent file://c%7c/a/b/c has zero problems.

@annevk
Copy link
Member

annevk commented Jan 17, 2017

I'm not sure that's worth emulating then. The only thing that would sway this for me personally is data showing such URLs are out there and content relies on them working.

@jasnell
Copy link
Collaborator Author

jasnell commented Jan 17, 2017

Understood! And to be certain, I'm not trying to argue that this should be handled. It's just an edge case that happened to come up while I was putting the parser through some of it's paces and I realized it wasn't covered. At the very least, knowing that it's explicitly out of scope means I don't have to write code to account for it ;-). Will close this for now and we can revisit it again later if necessary

@jasnell jasnell closed this as completed Jan 17, 2017
@annevk
Copy link
Member

annevk commented Jan 17, 2017

Let me leave this open to make sure we add tests to web-platform-tests.

@annevk annevk reopened this Jan 17, 2017
@annevk annevk added good first issue Ideal for someone new to a WHATWG standard or software project needs tests Moving the issue forward requires someone to write tests labels Jan 17, 2017
annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 2, 2017
@annevk
Copy link
Member

annevk commented Feb 2, 2017

Test created, would appreciate review.

annevk added a commit to web-platform-tests/wpt that referenced this issue Feb 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for someone new to a WHATWG standard or software project needs tests Moving the issue forward requires someone to write tests
Development

No branches or pull requests

3 participants