fix(tracing): Baggage parsing fails when input is not of type string

[Lms24]

This PR attempts to fix a bug reported in #5250 where the input parameter for parseBaggageString was not a string and hence the string.split function call would throw an error. This PR now checks for the type of the input and makes the function

• return an empty baggage object if the input is neither a string, nor an array. In this case we log a warning to get more information on what is actually passed in.
• parse the baggage string if the input is a string
• join and then split the array if the input is an array

Additionally, the parser now ignores empty or blank baggage entries (e.g. foo=bar, ,,alice=bob).

This is far from a proper, complete bugfix because we're missing an actual reproduction of the exact error but we got similar errors when experimenting with a Node/Express test app that would yield an array of baggage items rather than a string. However, it has the potential to fix it, or at least keep an error from emerging. Additionally, it makes the parser a little more robust.

Added some tests to describe/test how the parser now handles different input values.

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.35 KB (added)
@sentry/browser - ES5 CDN Bundle (minified)	59.85 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.15 KB (added)
@sentry/browser - ES6 CDN Bundle (minified)	53.49 KB (added)
@sentry/browser - Webpack (gzipped + minified)	19.92 KB (added)
@sentry/browser - Webpack (minified)	64.85 KB (added)
@sentry/react - Webpack (gzipped + minified)	19.95 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified)	43.98 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.57 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.08 KB (added)

[lforst]

Change looks great!

For my understanding: What this change means for the future is, that if any requests are incoming, that contain multiple baggage headers, our SDK will take care of combining them into one singular outgoing header. Correct?

I thought about this for a bit, because at first it seemed a bit invasive, however, we need to do this for sentry tracing to work + we're actually being a good citizen because we're cleaning up after other vendors. Also we're adhering to spec so it should totally be fine if others adhere to the spec too.

[lforst]

I find this somehow very satisfying. :)

[Lms24]

What this change means for the future is, that if any requests are incoming, that contain multiple baggage headers, our SDK will take care of combining them into one singular outgoing header. Correct?

Yes and no. I tried to reproduce having incoming requests with multiple baggage headers and what I've found out was that Express apparently automatically combines them into one header (which is what the standard permits) . (Only reproduced this with Express, might of course be different for other frameworks in which case the parsing function should take care of the multiple headers as well).

The one case where I could reproduce having an array of baggage headers instead of a string was when I simulated having another party add a baggage header with an array of elements to an outgoing request before we add our baggage header to that request. In this case our mergeAndSerializeBaggage function would parse that 3rd party's baggage header and crash if it was not a string. The changes in this PR should take care of parsing that header, in case it really is an array. Else it would log a warning which hopefully helps us understand what's really going on.

EDIT: I'll add this paragraph to the PR description for future readers

[lforst]

Awesome thanks for clarifying!

[lforst]

Btw, can we remove this type cast here now?

sentry-javascript/packages/node/src/integrations/http.ts

Line 126 in afe1000

const headerBaggageString = requestOptions.headers && (requestOptions.headers.baggage as string);

[Lms24]

Good point. Would like to do that but after trying, TS still screams at me because a header item could be of types string, string[], number, undefined. So we'd have to somehow handle a number type as well. Which overall is possible but I doubt that a baggage header would only have a number as content. WDYT is the better way here? Handle the number or expand the type cast to string | string[]?

[lforst]

Ah right. Hmm, if I were to make a decision I would do the following (kinda a lot of work though):

• Create a header type (called Header just to illustrate this) somewhere which is the following: string | string[] | number | null (null because http module returns null for getHeader when it's not there)
• Set mergeAndSerializeBaggage signature to (incomingBaggage?: Baggage, incomingBaggageHeader?: Header) => string
• Rename parseBaggageString to parseBaggageHeader and change its signature to (baggageHeaderValue: Header) => Baggage
• Fix up any type errors that may result from this

I get it though that this would kinda explode the scope of this PR. I guess for now, casting to string | string[] is perfectly reasonable.

If you think what I'm suggesting above makes sense we can do this in a follow-up PR.

[Lms24]

I think it's a good idea and I had a similar thought in mind (although just not wrapping it with a type which is for sure nicer). I don't even think that it's out of the scope for this PR because in this very PR I changed the types already to accept and handle string arrays, so why not add the number in as well.
I'll give it a try to see how much work it really is. In case it's a lot, I'll leave it with the extended type cast and we address it later.