mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
523 stars 118 forks source link

ClientRequestInterceptor incorrectly encodes basic authentication header #564

Closed mdesousa closed 2 months ago

mdesousa commented 2 months ago

Our application uses Axios to send basic authentication requests to a 3rd party application. This integration works ok until we add the ClientRequestInterceptor (for logging purposes). The interceptor seems to apply url encoding when it should not.

Authorization header without interceptor (after base64 decoding): user@xyz.com:password Authorization header with interceptor (after base64 decoding): user%40xyz.com:password

kettanaito commented 2 months ago

Hi, @mdesousa! Thanks for reporting this. Looks like a bug to me.

kettanaito commented 2 months ago

@mdesousa, can you please share how you make the Axios request?

mdesousa commented 2 months ago

hi @kettanaito , thanks for the help (and for the great library 👍 ) we are using basic authentication as documented here

  auth: {
    username: 'user@xyz.com',
    password: 's00pers3cret'
  }
kettanaito commented 2 months ago

@mdesousa, is the auth.username intended to be in a@b format? xyz.com looks suspiciously like a hostname. The docs you've shared doesn't feature such an example but it may be implied (let me know if it is).

mdesousa commented 2 months ago

the username is an email address, e.g. joe@gmail.com. the axios docs are generic, but it's common for many web services to use an email as the username.

kettanaito commented 2 months ago

Can you please point me to some docs (or better a spec) that mentions that email addresses are permitted as the Basic auth usernames? I couldn't find any mention of that.

Here's a reference I found: https://stackoverflow.com/a/6718568/2754939. It talks that if you decide to use an email as the username for Basic auth, it has to be escaped. In fact, Interceptors doesn't do this explicitly so I assume this is done either by Node.js or even Axios to comply with the RFC.

mdesousa commented 2 months ago

@kettanaito don't have docs that specifically mention emails. I can say that we are running into this issue while connecting to the Jira Cloud API. They use basic authentication where the username is an email address. The integration with Axios (without the interceptors) works without problems but fails when we add the interceptor.

mdesousa commented 2 months ago

@kettanaito also about that reference to RFC 3986: i think that RFC is talking about a different scenario, where user info is included as part of the url. That is not the case for basic authentication... axios encodes as base 64 and puts the content in the Authorization header. That's referenced in RFC 7617.

kettanaito commented 2 months ago

The integration with Axios (without the interceptors) works without problems but fails when we add the interceptor.

I suspect this is due to Interceptor coercing your request options to a URL instance, which escapes the auth by default (which, per the RFC, seems like the right thing to do).

That is not the case for basic authentication... axios encodes as base 64 and puts the content in the Authorization header.

Oh, that's a huge oversight on my part! For some reason, I thought we are talking about the auth-in-URL (I think the username email threw me off). Let me give this test another try...

kettanaito commented 2 months ago

@mdesousa, I've added a test for this use case in https://github.com/mswjs/interceptors/pull/565 but it's passing. Could you please take a look at that pull request and let me know how the way I use Axios in the test differs from your usage? It preserves the auth.username as-is, even if it's an email address.

mdesousa commented 2 months ago

@kettanaito thanks for the test. i cloned locally and ran it and i see it passing. but when i try the same test in a minimalistic repo, it fails !?!? can you try it out by cloning this repo?

mdesousa commented 2 months ago

hi @kettanaito , did a little bit of digging into the differences in the unit test vs. the repo. the test from the repo is calling the NodeClientRequest constructor with a url argument, while the unit test is not. this affects the logic below inside createRequest:

  if (clientRequest.url.username || clientRequest.url.password) {
    const auth = `${clientRequest.url.username || ''}:${clientRequest.url.password || ''}`
    headers.set('Authorization', `Basic ${btoa(auth)}`)

    // Remove the credentials from the URL since you cannot
    // construct a Request instance with such a URL.
    clientRequest.url.username = ''
    clientRequest.url.password = ''
  }

the repo test goes inside this if, and we confirmed that the username and password were uri encoded. the unit test that you wrote skips the if so there is no problem there.

seems that this is due to a difference in the arguments passed to interceptorHttpRequest... the unit test that is passing (false positive) has an Authorization header already populated and encoded correctly. the one that is failing (true negative) does not have an Authorization header. it has an auth property set to foo@bar.com:secret123. that is getting encoded inside normalizeClientRequestArgs which results in the url object below. this is where the username and password are being url encoded.

{
    href: 'http://foo%40bar.com:secret123@127.0.0.1:53469/books',
    origin: 'http://127.0.0.1:53469',
    protocol: 'http:',
    username: 'foo%40bar.com',
    password: 'secret123',
    host: '127.0.0.1:53469',
    hostname: '127.0.0.1',
    port: '53469',
    pathname: '/books',
    search: '',
    searchParams: URLSearchParams {},
    hash: ''
  }
mdesousa commented 2 months ago

@kettanaito found the reason why the unit test was not reproducing the issue: Axios was defaulting to the xhr adapter instead of http. When the adapter is set to http the test files. I have created a pull request with a proposed fix. Can you please review and merge if it looks correct? Thanks.

kettanaito commented 2 months ago

Closed by #567

kettanaito commented 2 months ago

Released: v0.29.1 🎉

This has been released in v0.29.1!

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.