Skip to content

fix: prevent JSON.stringify on non-JSON payloads#37

Closed
bulgakovk wants to merge 4 commits intomasterfrom
kb/fix/do-not-json-stringify
Closed

fix: prevent JSON.stringify on non-JSON payloads#37
bulgakovk wants to merge 4 commits intomasterfrom
kb/fix/do-not-json-stringify

Conversation

@bulgakovk
Copy link

@bulgakovk bulgakovk commented Sep 29, 2025

In some scenarios (like this one) we want to use non application/json payload. JSON.stringify breaks the payload in that case by adding escape characters

Testing

  • CI

@bulgakovk bulgakovk self-assigned this Sep 29, 2025
@mvayngrib mvayngrib requested a review from ChALkeR September 29, 2025 20:36
...opts.headers,
}
// If a consumer has set the non-json content type, we don't stringify the body.
const shouldStringify = headers['Content-Type'] === 'application/json'
Copy link

@mvayngrib mvayngrib Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • theoretically there could be others like application/vnd.api+json but in practice maybe we don't have them
  • should we do a case insensitive check on the header name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do have vendor specific content-types in at least one case I know of. I don't think the WorldpayClient in that case uses @exodus/fetch, but better to assume that it can happen

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see three theoretical options to solve the problem for the Sentry client:

  1. Provide a custom fetch implementation for the error-tracking module to prevent the default JSON.stringify behavior. At a quick glance, I didn't find any existing patterns in the code for doing that, so not sure if it's the right direction.
  2. Attach the sentry content-type on a proxy side and serialize the payload back to remove escape characters. This sounds too fragile and complex IMO.
  3. Make this fetch implementation handle the sentry specific header separately and avoid JSON.stringify for it. That feels like a solid compromise, added in c78222f

should we do a case insensitive check on the header name?

Given consumers need to be aware of the mechanism that prevents default JSON.stringify behavior, maybe let's force them to pass the header in a specific case? Just to keep this simple.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we stringify for application/json and startsWith('application/') && endsWith('+json')? What does that miss?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that miss?

Hard to say, perhaps need to look for every possible content type in all repositories =\

If we don't want the vendor headers to be hardcoded in this library, could we do something simple like

const shouldStringify = headers['Content-Type'].includes('json')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to inject the prevenJsonStringifyContentTypes set via a config though?

Copy link
Contributor

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. this is touching deprecated & frozen fetchival impl. A replacement is in experimental, which comes with breaking changes
  2. does the code in question even need fetchival at all? it might be time to design a better api

@bulgakovk bulgakovk closed this Oct 2, 2025
@bulgakovk bulgakovk deleted the kb/fix/do-not-json-stringify branch October 2, 2025 15:53
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.

5 participants