Skip to content
New issue

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

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Passing username from URL object to http.clientRequest without decoding #31439

Closed
izonder opened this issue Jan 21, 2020 · 1 comment
Closed
Labels
http Issues or PRs related to the http subsystem.

Comments

@izonder
Copy link

izonder commented Jan 21, 2020

  • Version: v8.x.x+
  • Platform: any
  • Subsystem: any

Description

Passing username with "unsafe" symbols (e.g. @) to URL object causes wrongly computed Basic-Authorization header string.

Pre-requisites

The next code looks good enough (Node.js CLI):

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

This should result in:

URL {
  href: 'http://test%40test:123456@localhost/',
  origin: 'http://localhost',
  protocol: 'http:',
  username: 'test%40test',
  password: '123456',
  host: 'localhost',
  hostname: 'localhost',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

The field username turned to percent-encoded as mentioned in the documentation (https://nodejs.org/api/url.html#url_url_username). According to the composed URI in the field href it's working as expected.

Expected behavior

Reference calls via cURL will look like:

curl -u 'test@test:123456' -v 'http://localhost/'
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 80 (#0)
* Server auth using Basic with user 'test@test'
> GET / HTTP/1.1
> Host: localhost
> Authorization: Basic dGVzdEB0ZXN0OjEyMzQ1Ng==
> User-Agent: curl/7.58.0
> Accept: */*

curl -v 'http://test%40test:123456@localhost/'
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 80 (#0)
* Server auth using Basic with user 'test@test'
> GET / HTTP/1.1
> Host: localhost
> Authorization: Basic dGVzdEB0ZXN0OjEyMzQ1Ng==
> User-Agent: curl/7.58.0
> Accept: */*

Decoding the header Authorization: Basic dGVzdEB0ZXN0OjEyMzQ1Ng== results to test@test:123456 as expected.

Actual behavior

Again try to make the same call from Node.js CLI:

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

const http = require('http');
console.log(http.get(url, () => {}).outputData);

That will output something like:

[
  {
    data: 'GET /profile HTTP/1.1\r\n' +
      'Host: localhost\r\n' +
      'Authorization: Basic dGVzdCU0MHRlc3Q6MTIzNDU2\r\n' +
      'Connection: close\r\n' +
      '\r\n',
    encoding: 'latin1',
    callback: [Function: bound onFinish]
  }
]

Decoding Authorization header results to test%40test:123456, which is wrong.

Expectation

When http.request(<URL>) grabs a value from href or username fields, it should sanitize and decode values before composing Authorization header.

-or-

WHATWG-URL should keep raw username and provide it like:

URL {
  href: 'http://test%40test:123456@localhost/', # here encoded values
  origin: 'http://localhost',
  protocol: 'http:',
  username: 'test@test', # here should be raw value
  password: '123456',
  host: 'localhost',
  hostname: 'localhost',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

References

@addaleax addaleax added whatwg-url Issues and PRs related to the WHATWG URL implementation. http Issues or PRs related to the http subsystem. and removed whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 21, 2020
@addaleax addaleax added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 21, 2020
addaleax added a commit to addaleax/node that referenced this issue Jan 21, 2020
@jasnell
Copy link
Member

jasnell commented May 29, 2020

The whatwg url impl is doing the right thing here. The issue is in the urlToOptions function

lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 8, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 8, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 8, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 12, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 12, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 14, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
lewgordon pushed a commit to lewgordon/node that referenced this issue Jul 15, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: nodejs#31439
andrei-cdl added a commit to compassdigital/basic-auth that referenced this issue Aug 9, 2021
due to a bug in node.js some clients will end up encoding the value
before base64 which results in the incorrect value being parsed.
ref: nodejs/node#31439
andrei-cdl added a commit to compassdigital/basic-auth that referenced this issue Aug 9, 2021
due to a bug in node.js some clients will end up encoding the value
before base64 which results in the incorrect value being parsed.
ref: nodejs/node#31439
danielleadams pushed a commit that referenced this issue Aug 16, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: #31439

PR-URL: #39310
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Sep 4, 2021
This change properly decodes the url.username and url.password for
the authorization header constructed from the URL object for
http(s) requests.

Fixes: #31439

PR-URL: #39310
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
3 participants