Duplicated values in Sentry Baggage header cause eventual 431 "Request header fields too large" error on HTTP fetch

etnichols commented 1 month ago

Is there an existing issue for this?

[X] I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
[X] I have reviewed the documentation https://docs.sentry.io/
[X] I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/nextjs

SDK Version

7.112.2

Framework Version

13.5.6

Link to Sentry event

n/a

SDK Setup

// sentry.client.config.ts
// This file configures the initialization of Sentry on the client.
// The config you add here will be used whenever a users loads a page in their browser.
// https://docs.sentry.io/platforms/javascript/guides/nextjs/

import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: process.env.NEXT_PUBLIC_SENTRY_DSN,

  // Adjust this value in production, or use tracesSampler for greater control
  tracesSampleRate: process.env.NEXT_PUBLIC_SENTRY_SAMPLE_RATE
    ? parseFloat(process.env.NEXT_PUBLIC_SENTRY_SAMPLE_RATE)
    : 0.2,

  // Setting this option to true will print useful information to the console while you're setting up Sentry.
  debug: false,

  replaysOnErrorSampleRate: 1.0,

  // This sets the sample rate to be 10%. You may want this to be 100% while
  // in development and sample at a lower rate in production
  replaysSessionSampleRate: 0.1,

  // You can remove this option if you're not planning to use the Sentry Session Replay feature:
  integrations: [
    Sentry.replayIntegration({
      // Additional Replay configuration goes in here, for example:
      maskAllText: true,
      blockAllMedia: true,
    }),
  ],

  // Only enable sentry in production with feature flag on.
  enabled: process.env.NODE_ENV === 'production' && process.env.ENABLE_SENTRY === 'true',
});

// sentry.edge.config.ts
// This file configures the initialization of Sentry for edge features (middleware, edge routes, and so on).
// The config you add here will be used whenever one of the edge features is loaded.
// Note that this config is unrelated to the Vercel Edge Runtime and is also required when running locally.
// https://docs.sentry.io/platforms/javascript/guides/nextjs/

import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: process.env.SENTRY_DSN,

  // Adjust this value in production, or use tracesSampler for greater control
  tracesSampleRate: process.env.SENTRY_SAMPLE_RATE
    ? parseFloat(process.env.SENTRY_SAMPLE_RATE)
    : 0.2,

  // Setting this option to true will print useful information to the console while you're setting up Sentry.
  debug: false,

  // Only enable sentry in production with feature flag on.
  enabled: process.env.NODE_ENV === 'production' && process.env.ENABLE_SENTRY === 'true',
});

// sentry.server.config.ts
// This file configures the initialization of Sentry on the server.
// The config you add here will be used whenever the server handles a request.
// https://docs.sentry.io/platforms/javascript/guides/nextjs/

import * as Sentry from '@sentry/nextjs';

Sentry.init({
  dsn: process.env.SENTRY_DSN,

  // Adjust this value in production, or use tracesSampler for greater control
  tracesSampleRate: process.env.SENTRY_SAMPLE_RATE
    ? parseFloat(process.env.SENTRY_SAMPLE_RATE)
    : 0.2,

  // Setting this option to true will print useful information to the console while you're setting up Sentry.
  debug: false,

  // uncomment the line below to enable Spotlight (https://spotlightjs.com)
  // spotlight: process.env.NODE_ENV === 'development',

  // Only enable sentry in production with feature flag on.
  enabled: process.env.NODE_ENV === 'production' && process.env.ENABLE_SENTRY === 'true',
});

Steps to Reproduce

create a simple next app which uses rewrites to proxy requests to some external API. Call this /my/api/route
use the sentry configs described above
create a button that performs client side fetch calls to this /my/api/route every time click.
observe the "Baggage" headers attached to the client fetch calls, and how duplicate values are continually added on subsequent requests.

Expected Result

Non-duplicated Sentry Baggage header.

Actual Result

App is experiencing the exact same issue as described in https://github.com/getsentry/sentry-javascript/issues/11500. Each subsequent request made from our NextJS client includes a duplicated entry under the Baggage header. For example, we have an /account/settings page that fires off 3 requests during load:

request 1

request 2

request 3

this pattern continues until the one of the fetch calls recieves back a 431 "Request header fields too large" response, which usually results in app hanging until hard refresh.

Using "@sentry/nextjs": "^7.112.2" for our Next app.

Taking a look at the logic that @lforst mentioned in https://github.com/getsentry/sentry-javascript/issues/11500#issuecomment-2047136837, I suspect this is a bug with the construction of the sentry baggage headers, specifically either here or here.

In both cases, doing a deduplication of existing key-value pairs might resolve the issue:

function deduplicateHeaderValues(headerString) {
  // Split the string into an array of key-value pairs
  const pairsArray = headerString.split(',');

  // Create a Map to store unique key-value pairs
  const uniquePairs = new Map();

  // Iterate over the pairs array
  pairsArray.forEach(pair => {
    const [key, value] = pair.split('=');
    // Store the pair in the Map, overwriting any duplicate keys
    uniquePairs.set(key, value);
  });

  // Convert the Map back to a string of key-value pairs
  const deduplicatedString = Array.from(uniquePairs).map(pair => pair.join('=')).join(',');

  return deduplicatedString;
}

// Example usage:
const headerString = 'sentry-environment=vercel-production,sentry-release=44628bc0cc9159d525751a0da2c82fa54d01124a,sentry-public_key=ca2f84b9f6e4d93d9c70dd70f4ad9bbe,sentry-trace_id=559215e003254fe5abecdb9dd001cd2e,sentry-environment=vercel-production,sentry-release=44628bc0cc9159d525751a0da2c82fa54d01124a,sentry-public_key=ca2f84b9f6e4d93d9c70dd70f4ad9bbe,sentry-trace_id=559215e003254fe5abecdb9dd001cd2e,sentry-environment=vercel-production,sentry-release=44628bc0cc9159d525751a0da2c82fa54d01124a,sentry-public_key=ca2f84b9f6e4d93d9c70dd70f4ad9bbe,sentry-trace_id=559215e003254fe5abecdb9dd001cd2e';

const deduplicatedHeaderString = deduplicateHeaderValues(headerString);
console.log(deduplicatedHeaderString);

lforst commented 1 month ago

@etnichols can you share more about the request? Can you verify that the request is indeed made as a fetch request and not XHR? What does the code making the request look like? Thanks!

etnichols commented 1 month ago

Thanks for the reply @lforst!

Can you verify that the request is indeed made as a fetch request and not XHR?

Confirmed, here is an example snippet of our fetch code used:

/* User Routes */

const fetchCurrentUser = (options) => fetch(`${BASE_URL}/api/user`, {
  credentials: 'include',
  cache: 'no-store',
})
  .then((response) => (response.status === 401 ? (window.location.href = '/login') : response))
  .then((response) => response.json());

I can provide a HAR file too if that would be helpful.

etnichols commented 1 month ago

.HAR file from our dev environment is here: app.tryinteract.dev-sentry-baggage-issue.har.zip

Lms24 commented 1 month ago

I wonder if there is some other library involved that patches fetch, for example to somehow retain request headers across fetch requests. To me, this looks like for some reason (parts of) the request are re-used which, as you pointed out in our source code, we don't account for at the moment.

lforst commented 1 month ago

I also don't really see how this can happen with the code @etnichols shared without a third party interfering. @etnichols would you mind providing a minimal reproduction? Thanks.

etnichols commented 1 month ago

While creating the min repro (https://github.com/etnichols/sentry-baggage-min-repro) we found the root cause: re-using a const object for the fetch options.

❌ passes same object reference to every instantiation of fetchCurrentUser, which causes Sentry code to continually append Baggage headers on each call:

const FETCH_OPTIONS = {
  credentials: 'include',
  method: 'GET',
};

const fetchCurrentUser = (options) =>
  fetch(`/api/user`, FETCH_OPTIONS).then((response) => response.json());

✅ spread operator creates a new object for each instantiation, no duplicated Baggage headers.

const FETCH_OPTIONS = {
  credentials: 'include',
  method: 'GET',
};

const fetchCurrentUser = (options) =>
  fetch(`/api/user`, { ...FETCH_OPTIONS }).then((response) => response.json());

While you could still consider adding de-dupe logic to the Header ctor code, this is 100% user error 🐒 apologies for the confusion.

lforst commented 1 month ago

I honestly don't consider this a user error! We should make it harder to run into this probably by cloning the object or smth.

getsentry / sentry-javascript