Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

url: parse fails to decode auth #7234

Closed
Swaagie opened this Issue Mar 4, 2014 · 14 comments

Comments

Projects
None yet
4 participants

Swaagie commented Mar 4, 2014

Providing a basic auth, where the password contains a % can lead to failure in url.parse. This bug is present in both 0.10.x and the HEAD of master. Per example
doing

require('url').parse('https://test:malfor%5Med@test.example.com')

will lead to

URIError: URI malformed
    at decodeURIComponent (native)
    at Url.parse (url.js:179:19)
    at Object.urlParse [as parse] (url.js:101:5)
    at repl:1:16
    at REPLServer.defaultEval (repl.js:130:27)
    at bound (domain.js:255:14)
    at REPLServer.runBound [as eval] (domain.js:268:12)
    at REPLServer.<anonymous> (repl.js:277:12)
    at REPLServer.EventEmitter.emit (events.js:104:17)
    at REPLServer.Interface._onLine (readline.js:202:10)

Fix might be checking if characters are actually within encoding range and only decode if require.

Swaagie commented Mar 4, 2014

My initial proposal would not be adequate, since auth can theoretically contain both urlencoded characters and an non urlencoded %, like si%lly%20pass

Owner

indutny commented Mar 4, 2014

Shouldn't it be si%25lly%20pass ?

@indutny indutny closed this Mar 4, 2014

Swaagie commented Mar 4, 2014

Why was this closed?

require('url').parse('https://test:malfor%5Med@test.example.com')

Still throws the malformed uri, and is a valid uri that could be provided via basic auth

Owner

indutny commented Mar 4, 2014

Try this, it works:

require('url').parse('https://test:malfor%25Med@test.example.com')

You are just passing incorrect url to the parser and it throws error, sort of expected.

Swaagie commented Mar 4, 2014

The uri is not incorrect, where does it say that url.parse only accepts an urlencoded uri? If the data input is ambiguous, running a encodeURIComponent over the uri could result in a double encoded uri. So basically any dev would have to manually check for the presence of characters that decodeURIComponent could die on before feeding it to parse?

Owner

indutny commented Mar 4, 2014

Indeed it doesn't mention it in docs, would you mind submitting a PR that with a fix for the documentation?

Swaagie commented Mar 4, 2014

sure

Swaagie commented Mar 4, 2014

On second thought (other than patching the docs) there is still a grey area where the developer might not know if they input is encoded or not, resulting in an error being thrown if he opts to use parse without validating input. I can't think of a concrete example that would demonstrate such behavior, but they probably exist, especially where the command line is the input source (e.g. no browser traffic).

What are the guidelines by which to or not include general fixes? I could argue not calling decode would lead to more consistent output from parse.

Owner

indutny commented Mar 4, 2014

Input validation is a necessary step when passing stuff into core functions, otherwise you are just exposing your app to security risks.

jcrugzz commented Mar 4, 2014

@indutny Say you are both the developer and the user of a script you have written. if you pass in a url with basic auth scheme where your password contains a character that causes decodeURIcomponent to throw and error, you are supposed to encode it prior JUST so that it doesn't cause url.parse to crash? To me this just seems inefficient and clumsy as the URL input that we could make this use case MUCH cleaner with a simple check :p.

Owner

indutny commented Mar 4, 2014

@jcrugzz it is just symmetric to url.format, try passing auth parameter there and see how it'll translate % in a %25. Expecting it to accept invalid stuff is just incorrect, and accepting incorrect input is something that you may want to do on a user level, rather than in core.

Owner

indutny commented Mar 4, 2014

And encoding stuff before sending is just fine.

Member

rlidwka commented Mar 6, 2014

you are supposed to encode it prior JUST so that it doesn't cause url.parse to crash

Yes. Otherwise %20 would be ambiguous (is it literal %20 or a space?).

jcrugzz commented Mar 6, 2014

@indutny i concede, this is reasonable, I was just being stubborn :p. Thanks for the info :)

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