Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

url.parse doesn't decode auth field #2736

Closed
natevw opened this issue Feb 11, 2012 · 6 comments
Closed

url.parse doesn't decode auth field #2736

natevw opened this issue Feb 11, 2012 · 6 comments

Comments

@natevw
Copy link

natevw commented Feb 11, 2012

Shouldn't the url.parse function decode the auth portion of a URL? I'd have expected the following to be true:

require('url').parse("http//either%2for@example").auth === "either/or"

Another user has also requested this via a comment on the somewhat separate issue #1163: #1163 (comment)

@isaacs
Copy link

isaacs commented Feb 11, 2012

Would anyone like to do a patch for this?

Should probably do #1163 along with it.

Note that it's also used by http.request now, to set the Authorization header.

@bnoordhuis
Copy link
Member

It's possible but is it sensible? foo%3Abar:baz decodes to foo:bar:baz. How does the user extract the username and password from that? bnoordhuis/node@c157b16 for better or worse decodes entities in the auth section.

@natevw
Copy link
Author

natevw commented Feb 17, 2012

For Basic Authentication, the username is not allowed to have a colon (http://tools.ietf.org/html/rfc2617#section-2). Furthermore, away from the URL layer (which I would argue is the layer I intend to be at if I am using the results of url.parse) there is no concept of "URL escaping".

@isaacs
Copy link

isaacs commented Feb 18, 2012

@bnoordhuis bnoordhuis/node@c157b16 looks good to me, but we need a test to make sure that http.get(url.parse('http://user%3Afoo%40:bar@localhost:8000/')) sends authorization: 'Basic dXNlcjpmb29AOmJhcg=='

@isaacs
Copy link

isaacs commented Feb 18, 2012

Oh, actually, that commit fixes a failure in http:

  if (options.auth && !this.getHeader('Authorization')) {
    //basic auth
    this.setHeader('Authorization', 'Basic ' +
                   new Buffer(options.auth).toString('base64'));
  }

Since the auth section should have any non-urlsafe chars encoded, this bit in http.js is a bug.

bnoordhuis added a commit to bnoordhuis/node that referenced this issue Feb 18, 2012
@bnoordhuis
Copy link
Member

Fixed in 86f4846. HTTP basic auth test updated in f116e17.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants