Skip to content

Enabling Handling Percent Encoded Username/Password With URLs#1813

Closed
DavidTPate wants to merge 2 commits intonodejs:masterfrom
DavidTPate:url-with-encoded-colon
Closed

Enabling Handling Percent Encoded Username/Password With URLs#1813
DavidTPate wants to merge 2 commits intonodejs:masterfrom
DavidTPate:url-with-encoded-colon

Conversation

@DavidTPate
Copy link

This PR is the beginning of handling the issue raised in #1802 where parsing a URI such as http://us%3Aer:pass%3Aword@localhost/ results in the auth property of the parsed URI to be us:er:pass:word which doesn't allow a separation of the username/password parts.

To account for this, the username and password are being broken out into separate properties of the parsed URI and the auth parameter is kept with the same legacy functionality for backwards compatibility.

@DavidTPate
Copy link
Author

Running the tests locally I'm currently hitting a failure with url.resolve() for the test resolve(mailto:local1@domain1?query1,local2@domain2) == mailto:local2@domain2 I need to do into url.resolve() some more to understand the functionality and better debug the current failure.

lib/url.js Outdated
Copy link
Author

Choose a reason for hiding this comment

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

I think this else if can be removed, because I don't see how someone would create an object which is an instance of the Url module and has a value for auth but not username.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we found in the run-up to 2.0.0 was that people do crazy things like delete myUrl.auth :-/

@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label May 27, 2015
@domenic
Copy link
Contributor

domenic commented May 27, 2015

Just a note that RFC 3986 is not the correct standard to reference for this; instead we follow http://url.spec.whatwg.org/ which obsoletes the RFC and better matches what browsers do. I imagine this change is fine since you said in the previous thread that we don't currently match browsers, but since I saw you referring to the RFC several times I thought it'd be good to step in so you don't end up wasting time trekking through it.

@DavidTPate
Copy link
Author

Thanks @domenic I'll take a look through the link you sent and make sure the specs are matching.

@DavidTPate
Copy link
Author

@domenic Finally had a chance to go through and dig into this some more. Everything is passing for me locally now, not sure how to kick off Jenkins to make sure it works on all of the build targets.

@brendanashworth
Copy link
Contributor

@DavidTPate I can run a CI but could you rebase onto master first?

@DavidTPate
Copy link
Author

@brendanashworth Cool. I'm moving this weekend (so don't have time) but next week I'll make it a priority to rebase and let you know once it's done.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@DavidTPate ... ping ... have you had a chance to look at this again?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@benjamingr
Copy link
Member

This hasn't had any activity in half a year. Going to close now, if anyone wants to resume work or discussion feel free to reopen.

@benjamingr benjamingr closed this Apr 30, 2016
@jasnell
Copy link
Member

jasnell commented Apr 30, 2016

Fine with closing the PR but this is still definitely a legitimate issue. Our url parsing definitely has issues.

@benjamingr
Copy link
Member

Of course, #1802 is still open and I have no intention to close issues about confirmed bugs :)

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

Labels

stalled Issues and PRs that are stalled. url Issues and PRs related to the legacy built-in url module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants