http: decode username and password before encoding #31450

addaleax · 2020-01-21T22:38:39Z

@nodejs/url @nodejs/security I’m marking this as blocked, because I’d like somebody to take a look at this who is somewhat familiar with the security implications of this – there are no obvious downsides to me here, but I know this can be quite a sensitive area.

Fixes: #31439

/cc @izonder

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Fixes: nodejs#31439

mcollina · 2020-01-21T22:45:00Z

I think this PR should also change the docs: https://nodejs.org/api/url.html#url_url_username.

Technically this can be seen as a patch or semver-major change, and I'm uncertain between the two.

addaleax · 2020-01-21T22:54:41Z

I think this PR should also change the docs: https://nodejs.org/api/url.html#url_url_username.

Done, PTAL

lpinca · 2020-01-22T06:50:44Z

semver-patch in my opinion.

mcollina

lgtm

richardlau · 2020-01-22T07:58:15Z

doc/api/url.md

+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31450
+    description: When used with `http.request()`, this field will now be
+                 percent-decoded.


Missing closing -->.

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

bnoordhuis · 2020-01-22T10:22:39Z

doc/api/url.md

+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/31450
+    description: When used with `http.request()`, this field will now be
+                 percent-decoded.


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

bnoordhuis · 2020-01-22T10:38:31Z

lib/internal/url.js

@@ -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)}`;


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!)

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...

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.

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.

@bnoordhuis Done, PTAL

jasnell · 2020-01-23T16:49:16Z

I'm generally unconvinced we should do this; and if we do make any changes here, it should be done in a manner that is consistent with the WHATWG URL parser. For instance, given the input: https://not%3Agood:g%3aod@example.com, the WHATWG URL parser produces:

URL {
  href: 'https://not%3Agood:g%3aod@example.com/',
  origin: 'https://example.com',
  protocol: 'https:',
  username: 'not%3Agood',
  password: 'g%3aod',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

(note that https://not%3Agood:g:od@example.com produces the same result.)

addaleax · 2020-01-23T21:31:12Z

if we do make any changes here, it should be done in a manner that is consistent with the WHATWG URL parser. For instance, given the input: https://not%3Agood:g%3aod@example.com, the WHATWG URL parser produces:

@jasnell I’m not 100 % sure what “consistent with the WHATWG URL parser” means here – the parser itself is not changed in any way. If you do pass in special characters as part of the original URL that are not : or /, this will be treated properly:

> url = new URL('https://not"good:g^od@example.com')
URL {
  href: 'https://not%22good:g%5Eod@example.com/',
  origin: 'https://example.com',
  protocol: 'https:',
  username: 'not%22good',
  password: 'g%5Eod',
  host: 'example.com',
  hostname: 'example.com',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> urlToOptions(url)
{
  protocol: 'https:',
  hostname: 'example.com',
  hash: '',
  search: '',
  pathname: '/',
  path: '/',
  href: 'https://not%22good:g%5Eod@example.com/',
  auth: 'not"good:g^od'
}

Trott · 2020-01-25T15:45:30Z

(needs a rebase)

jasnell · 2020-01-27T17:50:25Z

@addaleax, what I mean is that any decoding/encoding that happens with the username and password here should be identical to what is produced when parsing/serializing with the WHATWG URL parser. This is close, the URL standard does specify some specifics for username and password that are not covered by decodeURIComponent and encodeURIComponent.

izonder · 2020-02-20T09:54:15Z

Any updates on the MR?

markablov · 2020-04-29T05:16:55Z

lib/internal/url.js

@@ -1265,6 +1265,10 @@ function domainToUnicode(domain) {
  return _domainToUnicode(`${domain}`);
 }

+function decodeAuth(str) {
+  return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F');


i think you need to use global replace at least ;)

@addaleax This is still unaddressed (and, apparently, untested.)

markablov · 2020-04-29T05:18:49Z

lib/internal/url.js

@@ -1265,6 +1265,10 @@ function domainToUnicode(domain) {
  return _domainToUnicode(`${domain}`);
 }

+function decodeAuth(str) {
+  return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F');


it would be unexpected outcome with undefined url.password or url.username

Is it possible for url.username to be undefined? Isn't it an empty string instead?

jasnell · 2020-05-29T20:03:24Z

@addaleax ... I took another look at this and I want to remove any objection I have on this. The change looks good

ronag · 2020-06-21T15:37:44Z

@addaleax: Is this still blocked?

addaleax · 2020-06-22T19:10:10Z

@ronag No, I don’t think so, although it’s not ready to land either (there’s a few valid comments above and this needs a rebase)

szmarczak · 2020-07-16T12:43:53Z

lib/internal/url.js

@@ -1265,6 +1265,10 @@ function domainToUnicode(domain) {
  return _domainToUnicode(`${domain}`);
 }

+function decodeAuth(str) {
+  return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F');


https://github.com/addaleax/node/blob/5ebbb79a3bd56b99821081c23e1f5db9c879fce8/lib/internal/url.js#L1296

const forwardSlashRegEx = /\//g; +const colonRegEx = /:/g;

Suggested change

return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F');

return decodeURIComponent(str).replace(colonRegEx, '%3A').replace(forwardSlashRegEx, '%2F');

This is the only thing todo here I think...

szmarczak · 2020-07-28T11:22:49Z

Friendly ping :)

ronag · 2020-08-15T09:55:34Z

Conflicts

ronag · 2020-08-15T09:56:42Z

I'm removing the blocked label as I suspect it might make collaborators less likely to engage.

@ronag No, I don’t think so, although it’s not ready to land either (there’s a few valid comments above and this needs a rebase)

ronag

Adding a request changes to avoid landing while there are still things to address.

sindresorhus · 2020-09-16T23:20:41Z

@addaleax Would you be able to address the remaining feedback? 🙏

ronag · 2020-12-27T22:12:11Z

@addaleax friendly reminder

szmarczak · 2021-03-20T04:31:42Z

Friendly ping :)

Uzlopak · 2024-01-06T00:11:28Z

@jasnell

Do you think we can close this old PR, as the underlying issue was closed by you few years ago?

http: decode username and password before encoding

17ef5bf

Fixes: nodejs#31439

addaleax added http Issues or PRs related to the http subsystem. blocked PRs that are blocked by other issues or PRs. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 21, 2020

fixup! http: decode username and password before encoding

76ca736

lpinca approved these changes Jan 22, 2020

View reviewed changes

mcollina approved these changes Jan 22, 2020

View reviewed changes

richardlau reviewed Jan 22, 2020

View reviewed changes

bnoordhuis reviewed Jan 22, 2020

View reviewed changes

fixup! http: decode username and password before encoding

8ccf69f

fixup! http: decode username and password before encoding

5ebbb79

bruno-xo7 mentioned this pull request Apr 22, 2020

The authorization header is improperly encoded sindresorhus/got#1169

Closed

2 tasks

markablov reviewed Apr 29, 2020

View reviewed changes

jasnell approved these changes May 29, 2020

View reviewed changes

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

sindresorhus mentioned this pull request Jul 4, 2020

Password with special characters throws a URI malformed exception sindresorhus/got#1317

Closed

2 tasks

szmarczak reviewed Jul 16, 2020

View reviewed changes

addaleax requested a review from a team as a code owner August 10, 2020 16:07

ronag mentioned this pull request Aug 15, 2020

http: decode username and password before encoding stevenvachon/url-to-options#2

Open

ronag removed the blocked PRs that are blocked by other issues or PRs. label Aug 15, 2020

ronag requested changes Aug 15, 2020

View reviewed changes

lpinca mentioned this pull request Jan 6, 2021

Special Characters in WS Authorization websockets/ws#1831

Closed

1 task

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

lewgordon mentioned this pull request Jul 8, 2021

http: decodes url.username and url.password for authorization header #39310

Closed

szmarczak mentioned this pull request Jul 29, 2022

Library erroneously URI-encodes usersname and password sindresorhus/got#2093

Closed

2 tasks

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

ferronrsmith mentioned this pull request Sep 18, 2023

Password contains @，Iunable to connect[BUG] elasticsearch-dump/elasticsearch-dump#991

Closed

mcollina closed this Jan 7, 2024

	return decodeURIComponent(str).replace(':', '%3A').replace('/', '%2F');
	return decodeURIComponent(str).replace(colonRegEx, '%3A').replace(forwardSlashRegEx, '%2F');

http: decode username and password before encoding #31450

http: decode username and password before encoding #31450

Conversation

addaleax commented Jan 21, 2020

Checklist

mcollina commented Jan 21, 2020

addaleax commented Jan 21, 2020

lpinca commented Jan 22, 2020

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izonder Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jan 23, 2020

addaleax commented Jan 23, 2020

Trott commented Jan 25, 2020

jasnell commented Jan 27, 2020

izonder commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 29, 2020

ronag commented Jun 21, 2020

addaleax commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Jul 28, 2020

ronag commented Aug 15, 2020

ronag commented Aug 15, 2020 • edited Loading

ronag left a comment

Choose a reason for hiding this comment

sindresorhus commented Sep 16, 2020

ronag commented Dec 27, 2020

szmarczak commented Mar 20, 2021

Uzlopak commented Jan 6, 2024

izonder Jan 22, 2020 •

edited

Loading

ronag commented Aug 15, 2020 •

edited

Loading