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

Support username:password in NodeClientRequest #438

Closed
Tracked by #2517
mikicho opened this issue Sep 21, 2023 · 7 comments · Fixed by #440
Closed
Tracked by #2517

Support username:password in NodeClientRequest #438

mikicho opened this issue Sep 21, 2023 · 7 comments · Fixed by #440
Assignees

Comments

@mikicho
Copy link
Contributor

mikicho commented Sep 21, 2023

The request fails when we send auth option to http.request:

http.request({
  hostname: 'example.test',
  auth: 'username:password',
  path: '/',
})

logs

20:33:47:990 [http:request GET http://username:password@example.test/] constructing ClientRequest using options: {"url":"http://username:password@example.test/","requestOptions":{"hostname":"example.test","auth":"username:password","path":"/","protocol":"http:","method":"GET","agent":{"_events":{},"_eventsCount":2,"defaultPort":80,"protocol":"http:","options":{"noDelay":true,"path":null},"requests":{},"sockets":{"example.test:80:":[{"connecting":true,"_hadError":false,"_parent":null,"_host":"example.test","_closeAfterHandlingError":false,"_readableState":{"objectMode":false,"highWaterMark":16384,"buffer":{"head":null,"tail":null,"length":0},"length":0,"pipes":[],"flowing":null,"ended":false,"endEmitted":false,"reading":false,"constructed":true,"sync":true,"needReadable":false,"emittedReadable":false,"readableListening":false,"resumeScheduled":false,"errorEmitted":false,"emitClose":false,"autoDestroy":true,"destroyed":false,"errored":null,"closed":false,"closeEmitted":false,"defaultEncoding":"utf8","awaitDrainWriters":null,"multiAwaitDrain":false,"readingMore":false,"dataEmitted":false,"decoder":null,"encoding":null},"_events":{},"_eventsCount":6,"_writableState":{"objectMode":false,"highWaterMark":16384,"finalCalled":false,"needDrain":false,"ending":false,"ended":false,"finished":false,"destroyed":false,"decodeStrings":false,"defaultEncoding":"utf8","length":0,"writing":false,"corked":0,"sync":true,"bufferProcessing":false,"writecb":null,"writelen":0,"afterWriteTickInfo":null,"buffered":[],"bufferedIndex":0,"allBuffers":true,"allNoop":true,"pendingcb":0,"constructed":true,"prefinished":false,"errorEmitted":false,"emitClose":false,"autoDestroy":true,"errored":null,"closed":false,"closeEmitted":false},"allowHalfOpen":false,"_sockname":null,"_pendingData":null,"_pendingEncoding":"","server":null,"_server":null}]},"freeSockets":{},"keepAliveMsecs":1000,"keepAlive":false,"maxSockets":null,"maxFreeSockets":256,"scheduling":"lifo","maxTotalSockets":null,"totalSocketCount":1},"_defaultAgent":{"_events":{},"_eventsCount":2,"defaultPort":80,"protocol":"http:","options":{"keepAlive":true,"scheduling":"lifo","timeout":5000,"noDelay":true,"path":null},"requests":{},"sockets":{},"freeSockets":{},"keepAliveMsecs":1000,"keepAlive":true,"maxSockets":null,"maxFreeSockets":256,"scheduling":"lifo","maxTotalSockets":null,"totalSocketCount":0}}}

I think we should add some logic into the createRequest function
https://github.com/mswjs/interceptors/blob/main/src/interceptors/ClientRequest/NodeClientRequest.ts#L141

I think I can open a PR if you think I'm correct and we should fix the createRequest function

@kettanaito
Copy link
Member

kettanaito commented Sep 21, 2023

Hi, @mikicho. Thanks for proposing this.

Can you please tell me more about the expected behavior here? Do you wish to see the username/password represented as the Basic authentication header on the constructed Request instance? I do believe we grab the values from the options and forward them onto the ClientRequest instance as of now.

The request fails when we send auth option to http.request:

You must be referring to some third-party request module. auth isn't a supported property on the http.ClientRequest constructor. Instead, Node.js uses username and password properties:

http.request({
  method: 'POST',
  host: 'localhost',
  port: 12345,
  username: 'john',
  password: 'supersecret123'
})

@kettanaito kettanaito changed the title Add supprot for username:password in NodeClientRequest Support username:password in NodeClientRequest Sep 21, 2023
@mikicho
Copy link
Contributor Author

mikicho commented Sep 21, 2023

Can you please tell me more about the expected behavior here?

I think this is a bug:

const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')
const http = require('http')

const interceptor = new ClientRequestInterceptor({
  name: 'my-interceptor',
})
interceptor.apply();
interceptor.on('request', async ({request, a}) => {
  await new Promise(r => setTimeout(r, 700));
  throw new Error('error')
});

const req = http.request(
  {
    host: 'example.test',
    path: '/wrong-path',
    auth: 'user:pass'
  },
)
req.end() // TypeError: Request cannot be constructed from a URL that includes credentials: http://user:pass@example.test/wrong-path

Do you wish to see the username/password represented...

Probably yes.

You must be referring to some third-party request module...

It's one of the http.request options: https://nodejs.org/api/http.html#httprequesturl-options-callback

@kettanaito
Copy link
Member

Oh, I've overlooked that one 🤦 Thanks for pointing out. I wonder why this errors.

@mikicho
Copy link
Contributor Author

mikicho commented Sep 21, 2023

The error is from the createRequest function:
image

@kettanaito
Copy link
Member

kettanaito commented Sep 21, 2023

I see. So we need to omit the credentials from the URL itself and instead represent them in the Authorization request header. I think that's doable! I think this implementation can consist of multiple steps.

  1. Do not propagate username and password onto the NodeClientRequest.url. From the source of it, we don't do that, but it doesn't hurt to check.
  2. In createRequest, check request.username and request.password, and translate them into the Authorization header.

If you have time, I'd be thankful if you opened a pull request and we iterated on this together. Starting with some integration test would also be a good point.

@mikicho
Copy link
Contributor Author

mikicho commented Sep 21, 2023

on it

@kettanaito
Copy link
Member

Released: v0.25.4 🎉

This has been released in v0.25.4!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

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 a pull request may close this issue.

2 participants