React SDK - Session Recorder - Original fetch args is modified causing application to crash

[samuellawerentz]

Bug description

We use the React SDK to initialize posthog. We have a fetch interceptor which modifies the request before the API call is made.

After integrating with posthog, the fetch API is already modified and the args are not passed properly, because of this our interceptor is not able to function properly. The issue is with the recorder.js file.

TLDR; you are modifying the original fetch and not forwarding all the args in this format ...args . And hence our fetch interceptor which expects two args, receive only one causing the app to crash.

How to reproduce

1. Load react posthog
2. modify the fetch API like window.fetch = ...
3. App crashes

Please let me know, how to resolve this or what is the next step.

I'm attaching a screenshot of the recorder.js file for reference, where you are overriding the default fetch.

I have already sent an email, but was not able to track it.

Debug info

No response

[pauldambra]

@samuellawerentz if I understand you correctly changing to using these two args would work for you?

(we'd have to test that doesn't have any other unexpected effect but it seems reasonable - cc @daibhin)

[samuellawerentz]

This is our code

const originalFetch = window.fetch

export const fetchInterceptor = () => {
  window.fetch = async (...args) => {
    const href = decodeURI(args[0]) // <- It breaks around here, as we are expecting a string as the first arg

    // Replace region with the appropriate region from the state
    if (href?.includes('{{region}}')) {
      ... some logic ...
      args[0] = new URL(href.replace('region', region)
    }

    const response = await originalFetch(...args)
    return response
  }
}

[samuellawerentz]

Yes, the first arg we expect it to be URL

[pauldambra]

Ah... "It breaks around here, as we are expecting a string as the first arg"

the first arg of fetch is not guaranteed to be a string

from: https://developer.mozilla.org/en-US/docs/Web/API/Window/fetch#resource

so we probably both have not ideal behaviour here :)

just for clarity for the future traveller
the req that we pass into fetch is constructed from the args we receive. so we're not altering the args in posthog. but it is fair that the original fetch isn't called in quite the way it would have been - but the two calls are equivalent

[pauldambra]

so , yes, when we wrap fetch a url+init becomes a request... which is in retrospect probably not as transparent to code as maybe we should be

but any other dependency could make a valid Fetch call and your interceptor would see a Request object as the first arg and so would fail too

[pauldambra]

we have to go really carefully with changing the wrapper
so I can't guarantee a quick change here @samuellawerentz

are you ok in the meantime checking if args[0] is a string or a request and reading the href out of the request if necessary...

it's an immediate fix for you and i'll update here when we've changed our behaviour (but it'll make your interceptor more robust regardless since someone else could make a valid fetch where args[0] is not a string)

[samuellawerentz]

Yes, you're right, but we haven't faced a crash until now due to fetch api receiving invalid args.

Sure, For now, I can do a manual check if the first object is string/object then act accordingly.

But I have a question, why can't you forward the original params back to the fetch api?

Thank you @pauldambra for the time!

[pauldambra]

But I have a question, why can't you forward the original params back to the fetch api?

Only two reasons... I'm out of the office at the moment is the most important for me 🤣
We've been running this for around 12 months and a lot of folk are using it, so where normally i'd be a little cavalier and change this - because it feels like an improvement worth making to me - i just want to be a little purposeful and make sure that the change is safe.

I definitely want to address this though! It just won't be as quick as we normally aim for, sorry!

[samuellawerentz]

Hi @pauldambra we are not able to proceed with the fix that we decided, because it is not working.

are you ok in the meantime checking if args[0] is a string or a request and reading the href out of the request if necessary...

The problem is, the first arg to our interceptor always comes as a Request object. This is because recorder.js converts all our fetch calls into Request object before it reaches our interceptor.

We invoke fetch(url, options) -> Posthog recorder.js converts to fetch(request) -> Our interceptor receives fetch(request)

Now as you can see, the first arg is always Request Object when it comes to our interceptor.

I'm not sure if we can modify the url inside a request object as it is only readonly.
We tried creating a new request out of the existing one

let href = ''
// Retrieve the URL
if (typeof args[0] === 'string') 
  href = decodeURI(args[0])
else if (args[0] instanceof Request) 
  href = decodeURI(args[0].url)

// Business logic
href?.replace('{{region}}', region)

// Set the URL
if (typeof args[0] === 'string') args[0] = href
else if (args[0] instanceof Request) 
  args[0] = new Request(href, args[0]) // -> This is not possible, getting error here

const response = await originalFetch(...args)
return response

So we got hit with this error in Chrome - Uncaught TypeError: Failed to construct 'Request': The 'duplex' member must be specified for a request with a streaming body

Safari seemed to work fine. Upon researching we came across this thread -whatwg/fetch#1486

Looks like duplication of a request is not straight forward. Came across this fix by the ky library for the same issue - https://github.com/sindresorhus/ky/pull/451/files

So can u let us know what to do next? Or if you have any simple workarounds? Right now we have reverted our changes and we are not able to use Posthog in produciton.

Or let me know an approximate timeline when this will be taken up, so i can share it with my team. Thanks!

[pauldambra]

i'm unexpectedly out of the office at the moment... i've opened #1351 to show the change we probably need to make is small - but i don't have my work head on so might be missing something

@daibhin are you ok to pick this up?

@samuellawerentz if you're using pnpm to add posthog then you could pnpm patch until this is picked up

[pauldambra]

(I'm 99.999% sure that change is fine just mindful i've not checked it at all 😊)

[daibhin]

Totally! Thanks for the PR @pauldambra - I can take it from here.

I'm also not in work mode this evening but will look at merging it tomorrow morning

[samuellawerentz]

Thank you @pauldambra @daibhin!

[samuellawerentz]

Hi @daibhin just checking if theres an update. Thanks!

[daibhin]

Hey @samuellawerentz, Paul is back today so I will catch up with him about #1351

[pauldambra]

That PR was merged as version 1.156.0. So, this should be fixed for you now 🤞

[pauldambra]

Ah, no, we're reverting PostHog/posthog#24471

I think that change makes the error I was worrying about, that if the first argument to fetch is a request object and we read the body then we pass along a request that a receiver can't use

Will need to go way more carefully if we change this again, wrapping fetch is useful and filled with footguns

[samuellawerentz]

Oh okay.. Yes, handling fetch is quite risky.. Please keep me updated.. Thank you!