ref(tracing): Ignore third party baggage entries from incoming requests #5319

Lms24 · 2022-06-27T11:51:58Z

This PR makes changes to how we handle third party content in the baggage Http header: On incoming requests we no longer parse and put non-sentry- baggage entries into the Baggage object but we ignore them and instead store an empty third party string. The reason for this change is that - as we discussed and agreed upon - the Sentry SDK should not be responsible for propagating other vendors' data. If users want to propagate that data as well on the service where the Sentry SDK is installed, they should add the other vendors' propagation mechanisms. In fact, this is the only way we can ensure that baggage content that should not be propagated further is really stopped.

It's important to note that this only concerns incoming 3rd party baggage items. If other vendors (or the users themselves) add a baggage header to the outgoing request before the Sentry SDK adds its baggage header, we continue to merge the two headers such that the 3rd party baggage as well as this SDK's DSC entries are still in the final header. In (the edge) case that this 3rd party baggage header already contains sentry- entries, they will be ignored and overwritten with this SDK's sentry- entries.

With this PR, our DSC propagation behaviour conforms with our DSC specification.

In addition of these changes, this PR also contains a few small cleanups and improvements:

removed isBaggageEmpty helper function (+ tests) as it was not used anymore except for in tests
simplified mutability check: Because of how we parse incoming baggage headers, we can make the check slightly simpler than the pseudo code in the spec. Reviewer(s): Would appreciate a careful look here. Happy to discuss changing this.
adjusted tests to new 3rd party handling
added node integration tests to check for correct handling of baggage header merging in case another vendor injects a baggage header before the Sentry SDK

fixes #5297

was not used anymore (except in tests)

…ge headers

github-actions · 2022-06-27T11:59:58Z

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.3 KB (added)
@sentry/browser - ES5 CDN Bundle (minified)	59.72 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	17.9 KB (added)
@sentry/browser - ES6 CDN Bundle (minified)	52.67 KB (added)
@sentry/browser - Webpack (gzipped + minified)	19.67 KB (added)
@sentry/browser - Webpack (minified)	64.04 KB (added)
@sentry/react - Webpack (gzipped + minified)	19.69 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified)	43.85 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	25.63 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	23.9 KB (added)

Lms24 · 2022-06-27T11:57:31Z

packages/nextjs/src/utils/withSentry.ts

-              metadata: { baggage: baggage },
+              metadata: { baggage },


Just to cut down a few bytes....

Lms24 · 2022-06-27T11:58:30Z

packages/tracing/src/transaction.ts

-    // In case, we poulated the DSC, we have update the stored one on the transaction.
-    if (existingBaggage !== finalBaggage) {
-      this.metadata.baggage = finalBaggage;
-    }
+    // Update the baggage stored on the transaction.
+    this.metadata.baggage = finalBaggage;


IMO, it's simpler this way and saves us some bytes

Lms24 · 2022-06-27T12:05:02Z

packages/utils/src/baggage.ts

-  if (!isSentryBaggageEmpty(baggage) || (sentryTraceHeader && isSentryBaggageEmpty(baggage))) {
-    setBaggageImmutable(baggage);
-  }
+
+  // Because we are always creating a Baggage object by calling `parseBaggageHeader` above
+  // (either a filled one or an empty one, even if we didn't get a `baggage` header),
+  // we only need to check if we have a sentry-trace header or not. As soon as we have it,
+  // we set baggage immutable. In case we don't get a sentry-trace header, we can assume that
+  // this SDK is the head of the trace and thus we still permit mutation at this time.
+  sentryTraceHeader && setBaggageImmutable(baggage);
+


This is arguably a critical change here because it does not cover one edge case (which I don't know if it ever exists): We receive an incoming request without a sentry-trace header but with a baggage header that contains sentry- entries. In such a situation, this change would keep the baggage mutable, where previously it would have been set immutable.
WDYT, should we handle this edge case or do we assume that we won't receive this combination of headers?

I personally can't think of a situation when this would occur because all SDKs already send sentry-trace headers. I therefore made this simplification here. Happy to revisit though!

The inverse situation (no baggage but a sentry-trace header) is of course handled correctly.

In such a situation, this change would keep the baggage mutable, where previously it would have been set immutable.

Technically we shouldn't need to handle this edge case, but I think we should just in case. I think this is the correct behaviour. Let's update the develop docs to account for this?

but I think we should just in case

@lforst and I also just discussed this again and I just handled it in 795dd25.

Let's update the develop docs to account for this

Sure, I'm all for it. We should try to find a conclusion as to what this means though. If we get a baggage header with sentry- entries but no sentry-trace header,

Are we the head of trace or not?

What happened before (i.e. in the SDK that propagated the baggage header to us) and where could something like this come from?

And most importantly, is it okay to say, we just freeze baggage and propagate it downstream?

Would we in this case get sentry-trace_id with that baggage header and if yes, why don't we get sentry-trace in that situation?

Could this come from a third party that (intentionally) sends sentry- entries in baggage s.t. our DS data is corrupted?

Not saying we have to have definitive answers to all of them but to me these are questions we should ask if this ever happens

lforst · 2022-06-27T16:15:36Z

packages/utils/src/baggage.ts

+/**
+ * Parse a baggage header from a string or a string array and return a Baggage object
+ *
+ * If @param includeThirdPartyEntries is set to true, thir party baggage entries are added to the Baggage object


Suggested change

* If @param includeThirdPartyEntries is set to true, thir party baggage entries are added to the Baggage object

* If @param includeThirdPartyEntries is set to true, third party baggage entries are added to the Baggage object

Whoops, forgot to commit this. Opened #5322

Lms24 added 5 commits June 27, 2022 11:09

ignore 3rd party entries baggage in incoming requests

7a1bc93

fix (integration) tests

9359d3f

remove isBaggageEmpty helper function

e9d1c2a

was not used anymore (except in tests)

add node integration tests to test correct merging of 3rd party bagga…

55e0243

…ge headers

re-add accidentally deleted yarn script

2409063

Lms24 commented Jun 27, 2022

View reviewed changes

fix serverless unit tests

1dc038f

Lms24 requested review from AbhiPrasad and lforst June 27, 2022 12:08

Lms24 added this to the Dynamic Sampling milestone Jun 27, 2022

change mutability condition in parseBaggageSetMutability

795dd25

lforst approved these changes Jun 27, 2022

View reviewed changes

Lms24 merged commit 3e07e20 into master Jun 28, 2022

Lms24 deleted the lms-baggage-third-party-baggage-cleanup branch June 28, 2022 08:14

Lms24 mentioned this pull request Jun 28, 2022

chore(doc): Fix typo in JSDoc of parseBaggageHeader #5322

Merged

Lms24 self-assigned this Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ref(tracing): Ignore third party baggage entries from incoming requests #5319

ref(tracing): Ignore third party baggage entries from incoming requests #5319

Uh oh!

Lms24 commented Jun 27, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Jun 27, 2022 •

edited

Loading

Uh oh!

Lms24 Jun 27, 2022

Uh oh!

Lms24 Jun 27, 2022

Uh oh!

Lms24 Jun 27, 2022 •

edited

Loading

Uh oh!

AbhiPrasad Jun 27, 2022

Uh oh!

Lms24 Jun 27, 2022 •

edited

Loading

Uh oh!

Lms24 Jun 27, 2022 •

edited

Loading

Uh oh!

lforst Jun 27, 2022

Uh oh!

Lms24 Jun 28, 2022

Uh oh!

Uh oh!

	* If @param includeThirdPartyEntries is set to true, thir party baggage entries are added to the Baggage object
	* If @param includeThirdPartyEntries is set to true, third party baggage entries are added to the Baggage object

Uh oh!

ref(tracing): Ignore third party baggage entries from incoming requests #5319

ref(tracing): Ignore third party baggage entries from incoming requests #5319

Uh oh!

Conversation

Lms24 commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

Lms24 Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbhiPrasad Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lforst Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

Lms24 Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lms24 commented Jun 27, 2022 •

edited

Loading

github-actions bot commented Jun 27, 2022 •

edited

Loading

Lms24 Jun 27, 2022 •

edited

Loading

Lms24 Jun 27, 2022 •

edited

Loading

Lms24 Jun 27, 2022 •

edited

Loading