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

Honor auth option #1734

Merged
merged 5 commits into from
Apr 11, 2020
Merged

Honor auth option #1734

merged 5 commits into from
Apr 11, 2020

Conversation

enekid
Copy link
Contributor

@enekid enekid commented Apr 10, 2020

Description

The WebSocket client documentation states that all standard http.request() options are valid.

Any other option allowed in http.request() or https.request().

But if you create a new WebSocket client, the auth property in the options object parameter is ignored. And the auth property is a valid http.request() options

Reproducible in:

  • version: all versions

Steps to reproduce:

  1. Create a Websocket client with an auth option:
const auth = 'user:password';
const ws = new WebSocket('ws://localhost', {auth});

Expected result:

The server should receive the auth header. Eg: Authorization: Basic dXNlcjpwYXNzd29yZA==

Actual result:

No auth header is sent.

Fix

When the opts object gets initialized, the auth property is set to undefined, overwriting the value passed in the options object.

A little reordering of the properties in the initialization seems to fix the bug.

@lpinca
Copy link
Member

lpinca commented Apr 11, 2020

Thanks but this is by design. The auth option should not override the userinfo in the connection URL.

const ws = new WebSocket('ws://foo:bar@example.com/', { auth: 'baz:qux' });

foo:bar should win.

This is why other options like path, port, host, etc are also set to undefined. The values read from the parsed connection URL should not be changed by http.request() options.

@enekid
Copy link
Contributor Author

enekid commented Apr 11, 2020

Hi!

The userinfo will always win!

In the initAsClient function, after initiating the opts object with the options object,

  const opts = {
    protocolVersion: protocolVersions[1],
//...
    auth: undefined, // this line could even be deleted (?)
    ...options,
//...
  };

it's checked if there is userInfo in the parsedUrl, and if so, the auth property would be overwritten

  if (parsedUrl.username || parsedUrl.password) {
    opts.auth = `${parsedUrl.username}:${parsedUrl.password}`;
  }

@lpinca
Copy link
Member

lpinca commented Apr 11, 2020

Ok, in that case I'm fine with this.

lib/websocket.js Outdated Show resolved Hide resolved
test/websocket.test.js Outdated Show resolved Hide resolved
test/websocket.test.js Outdated Show resolved Hide resolved
test/websocket.test.js Outdated Show resolved Hide resolved
@lpinca lpinca merged commit eaf2ad1 into websockets:master Apr 11, 2020
@lpinca
Copy link
Member

lpinca commented Apr 11, 2020

Thank you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants