RedHatInsights / insights-chrome

Chroming for Insights apps
MIT License
28 stars 128 forks source link

RTK Query mutation/POST request headers are missing #2614

Open lucasgarfield opened 11 months ago

lucasgarfield commented 11 months ago

I am not sure whether the issue I am encountering is caused by insights-chrome or not, but it seems to be very similar to issue #2461. This is related to https://github.com/RedHatInsights/image-builder-frontend. I am using the library RTK Query to make requests to other services on console.redhat.com.

My RTK Query mutation hook (which is their terminology for a hook that makes a POST request) is missing the "content-type": "application/json" header. Our backend rejects the request as it is missing this header.

RTK Query mutation's use a small wrapper around fetch() and I am wondering whether insights-chrome has a polyfill for fetch that is somehow causing the headers to get dropped/lost. This has been to known to happen before.***

I noticed reviewing #2465 there seems to be a wrapper around fetch... specifically here: https://github.com/RedHatInsights/insights-chrome/pull/2465/commits/13f3a2febf01b7a48d0920130c869dada76a39e6#diff-1a0b511d1eb317649220c672597fd6a2ab6e384023efde9f4ccf05d30fe4a2b8R106

Do all fetch requests use this wrapper? If the answer is no - are there any other wrappers around fetch?

If this wrapper (or another wrapper) is affecting fetch(), is it possible that it is causing the missing headers? The first argument in the chrome-insights fetch wrapper I found is a string, but I believe that the RTK Query mutation expects for the first argument to be a Request class instance. This is in the fetch specification: https://fetch.spec.whatwg.org/#requestinfo

I would be happy to look into this issue a bit more on my own but I need some guidance at this point on how to continue. Thanks!

*** Here is an example of a similar issue, the bug was fixed in this PR after some back and forth with the RTK Query maintainer: https://github.com/supertokens/supertokens-website/issues/116

Hyperkid123 commented 11 months ago

RTK Query mutation's use a small wrapper around fetch() and I am wondering whether insights-chrome has a polyfill for fetch that is somehow causing the headers to get dropped/lost. This has been to known to happen before.***

The wrapper is most likely the reason why the headers are not being added. Chrome is using the native version of fetch. We will not adopt any polyfill or any other implementation of fetch because it can cause issues for the rest of the platform.

You will have to add the auth header yourself to the RTK fetch wrapper.

lucasgarfield commented 11 months ago

I'm certainly not asking for insights-chrome to adopt a polyfill or other implementation of fetch just for me... that would really be outrageous. :sweat_smile:

Rather, I believe that the fetch wrapper in insights-chrome is dropping the headers for the reasons that the RTK Query maintainer describes in this comment: https://github.com/supertokens/supertokens-website/issues/116#issuecomment-1094272646

In iqeEnablements.ts there is the following code:

  window.fetch = function fetchReplacement(path = '', options, ...rest) {
    const tid = Math.random().toString(36);
    const additionalHeaders: any = spreadAdditionalHeaders(options);

    const prom = oldFetch.apply(this, [
      path,
      {
        ...(options || {}),
        headers: {
          // If app wants to set its own Auth header it can do so
          ...(checkOrigin(path) && libJwt?.()?.jwt.isAuthenticated() && { Authorization: `Bearer ${libJwt?.()?.jwt.getEncodedToken()}` }),
          ...additionalHeaders,
        },
      },
      ...rest,
    ]);

I believe that this code does not conform to the fetch specification (https://fetch.spec.whatwg.org/#requestinfo). If path is a Request object (https://developer.mozilla.org/en-US/docs/Web/API/Request) that contains headers (which is the case for the POST request from the RTKQ mutation), then the headers provided in the second argument to oldFetch.apply() will actually overwrite them completely instead of merging them.

I haven't had time to work on a PR yet to prove this, but this is my current working theory... I will try to get a PR to confirm soon.

lucasgarfield commented 11 months ago

I believe my theory was correct, I've managed to get the correct headers from my RTKQ mutation after implementing these changes: #2615