url: parse() fails if the auth field contains a colon(:) or at-sign(@). #2937
Conversation
lib/url.js
Outdated
| var username, password; | ||
| colon = auth.lastIndexOf(':'); | ||
| if (colon !== -1) { | ||
| username = auth.slice(0, colon); |
There was a problem hiding this comment.
What if the username has no colons but the password contains one or more colons? What if both of them have colons? FWIW (at least) Chrome 45 and Firefox 40 split on the first colon instead of the last. I think it might be better to follow what browsers are doing so that at least there is some similarity.
There was a problem hiding this comment.
It shouldn't matter if you split on the first or the last colon, because there should only ever be one. It's invalid to have a non-encoded colon in the username or password. They should be encoded.
That said I agree with the point of doing it like the browsers do, so I'll change it to the first colon.
|
I depreciated the auth field because none of the major browsers support it and the current implementation can give you a wrong answer. For example: var url = require('url');
var user = encodeURIComponent('us:er');
var password = encodeURIComponent('pass:word');
var uri = 'http://' + user + ':' + password + '@localhost/';
var parsed = url.parse(uri);
console.log(parsed.auth); // us:er:pass:wordOne option is to fix parse so that it contains the encoded username and password separated by a colon, but that could potentially break some applications. I figured it would be better to leave it as is and deprecated it. |
ben-page
changed the title
ben-page
force-pushed
the
url-parse-fix
branch
from
5c08864
to
fcecbb8
Compare
ben-page
force-pushed
the
url-parse-fix
branch
from
fcecbb8
to
77cc372
Compare
|
Closing due to lack of any further activity or discussion. |
|
@jasnell, there is an open issue associated with this. It was references by another issue just 4 days before you closed this. You and @brendanashworth have confirm that issue as a bug. And you said you would look into it, but I've heard nothing back from you. Frankly, I'm frustrate with this entire process. It really make be doubt about my decision to use node. How does one get their contributions reviewed? I've re-based the PR three times already. I've brought it to your attention several times. I've received no direction on what's hold this up. From what I can tell it's simply been ignored. This really is a bad way to treat contributors: ignoring them and eventually closing their issues because of "lack of activity". |
|
@ben-page I'm sorry that this pr has been left in this state of limbo. Node is maintained by a distributed team and a consensus model. This can allow things to move very quickly, but at the same time can leave things forgotten about. I'm going to re open this issue and assign it to myself, I'll work with you and the collaborators to figure out what is blocking this from coming in and help get it landed. Thank you for being patient, contributors like yourself are what make this project awesome! |
|
@ben-page as a start would you be able to rebase one more time? After you do so I'll run both our CI and smoke-tests on this PR and follow up with collaborators to find out what needs to be done next |
|
@ben-page ... given that there had been no activity on this particular PR since september it had appeared as if this particular change request had been abandoned. We do have other efforts underway to look at the same issue (other open PRs and issues) so there appeared (at first) to be little harm in closing the PR because it had appeared to have been abandoned. My apologies for that. |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
|
hey @ben-page were you still interested in working on this? |
|
@thealphanerd, yes. Sorry for not getting to this sooner. I rebased. Let me know anything needs be changed. |
|
@mscdex any thoughts on this? |
|
Looks like there's at least one url-related test failing. |
url.parse() fails if auth portion contains any additional colons. For example, the auth portion of http://user%40name:pass%3Aword@localhost is parsed as user@name:pass:word. The solution is to add two new fields, username and password. Additionally, format(), resolve(), the doc, and unit tests have been updated reflect the new fields.
|
I screwed up the merge. So, I just redid it. All url tests are passing, now. |
|
@thealphanerd, can the build be restarted? |
|
It looks like it's still failing. When I run the |
|
@ben-page I think the doctool test failure is unrelated and is due to another PR that recently got merged |
|
failures are unrelated... a fix is in for the doctool failure. I'll re run the CI once that lands |
|
ci is green... @mscdex thoughts? |
|
@thealphanerd Well, performance-wise |
|
@jasnell do you have any thoughts on this? |
|
Yes, this is one of the issue I'm going to try to fix by introducing the WHATWG URL parsing support (nodejs/node-eps#28). There are several of these kinds of issues that are caused by our parser being less-than-compliant with the spec. |
| * `auth`: The authentication information portion of a URL. | ||
|
|
||
| Example: `'user:pass'` | ||
| Deprecated: `auth` does not handled URL encoding properly, use `username` and `password` instead. |
There was a problem hiding this comment.
typo: "handled" -> "handle"
rvagg
force-pushed
the
master
branch
2 times, most recently
from
c133999
to
83c7a88
Compare
|
@jasnell is this still relevant? |
|
This is not an issue with the new WHATWG URL parser but it still exists with the old |
|
Closing due to lack of forward progress on this. As indicated, this is not an issue with the new URL parser |
The solution is to add two new fields, username and password. Additionally, url.format() is updated to prefer these new fields over the auth field. The tests and docs have been updated accordingly. This should not be breaking as the behavior of the auth field was not modified.
Here's the issues:
nodejs/node-v0.x-archive#25353
#1802