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

http: decode username and password before encoding #31450

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion doc/api/url.md
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,12 @@ the URL, use the [`url.search`][] setter. See [`URLSearchParams`][]
documentation for details.

#### `url.username`
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/31450
description: When used with `http.request()`, this field will now be
percent-decoded.
Copy link
Member

Choose a reason for hiding this comment

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

Missing closing -->.

Copy link
Member

Choose a reason for hiding this comment

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

The section on http.request() in http.md seems like a more logical place to me for this note.

Copy link
Member Author

Choose a reason for hiding this comment

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

The section on http.request() in http.md seems like a more logical place to me for this note.

Why not both? :)


* {string}

Expand All @@ -488,7 +494,8 @@ console.log(myURL.href);
Any invalid URL characters appearing in the value assigned the `username`
property will be [percent-encoded][]. The selection of which
characters to percent-encode may vary somewhat from what the [`url.parse()`][]
and [`url.format()`][] methods would produce.
and [`url.format()`][] methods would produce. When used with
[`http.request()`][], this field will be percent-decoded.

#### `url.toString()`

Expand Down Expand Up @@ -1316,6 +1323,7 @@ console.log(myURL.origin);
[`TypeError`]: errors.html#errors_class_typeerror
[`URLSearchParams`]: #url_class_urlsearchparams
[`array.toString()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toString
[`http.request()`]: http.html#http_http_request_options_callback
[`new URL()`]: #url_constructor_new_url_input_base
[`querystring`]: querystring.html
[`require('url').format()`]: #url_url_format_url_options
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -1284,7 +1284,8 @@ function urlToOptions(url) {
options.port = Number(url.port);
}
if (url.username || url.password) {
options.auth = `${url.username}:${url.password}`;
options.auth =
`${decodeURIComponent(url.username)}:${decodeURIComponent(url.password)}`;
Copy link
Member

Choose a reason for hiding this comment

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

This introduces a spec violation. From RFC 7617, section 2:

 Furthermore, a user-id containing a colon character is invalid, as
 the first colon in a user-pass string separates user-id and password
 from one another; text after the first colon is part of the password.
 User-ids containing colons cannot be encoded in user-pass strings.

With this change, I can now write something like:

https.get('https://not%3Agood:god@example.com')

And the server will interpret that as user="not" and pass="good:god" instead of pass="god" (which is the password we still all use for our root accounts, right? Right!)

Copy link

@izonder izonder Jan 22, 2020

Choose a reason for hiding this comment

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

User-ids containing colons cannot be encoded in user-pass strings.

But what would be proper behavior in case of passing : in username? Encoding username - the wrong way for the computation Authorization header. Pre-validation with throwing an exception? Maybe...

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t really want to introduce new exceptions here … maybe just leave it encoded?

The same question also pops up for e.g. %2F for / in the password.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I think : and / are the only problematic ones; for @ it's "last at-sign wins."

It's probably okay to leave those encoded, that's no worse from how it was before.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Done, PTAL

}
return options;
}
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-http-url-username.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const http = require('http');
const MakeDuplexPair = require('../common/duplexpair');

// Test that usernames from URLs are URL-decoded, as they should be.

{
const url = new URL('http://localhost');
url.username = 'test@test';
url.password = '123456';

const server = http.createServer(
common.mustCall((req, res) => {
assert.strictEqual(
req.headers.authorization,
'Basic ' + Buffer.from('test@test:123456').toString('base64'));
res.statusCode = 200;
res.end();
}));

const { clientSide, serverSide } = MakeDuplexPair();
server.emit('connection', serverSide);

const req = http.request(url, {
createConnection: common.mustCall(() => clientSide)
}, common.mustCall((res) => {
res.resume(); // We don’t actually care about contents.
res.on('end', common.mustCall());
}));
req.end();
}