fix: Don't override sentry-trace and baggage headers (#3540)

Don't override sentry-trace and baggage headers in a http request. Cross platform SDKs may set this headers and we're overriding it.
getsentry · Jan 10, 2024 · d02d953 · d02d953
1 parent 9901d21
commit d02d953
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@
 - Use correct rendered frames timestamp for TTID/TTFD and app start (#3531)
 - Move header reference out of "extern C" (#3538)
 - Missing transactions when not calling `reportFullyDisplayed` (#3477)
+- Don't override `sentry-trace` and `baggage` headers (#3540)
 
 ## 8.17.2
 

diff --git a/Sources/Sentry/SentryBaggage.m b/Sources/Sentry/SentryBaggage.m
@@ -34,11 +34,6 @@ - (instancetype)initWithTraceId:(SentryId *)traceId
     return self;
 }
 
-- (NSString *)toHTTPHeader
-{
-    return [self toHTTPHeaderWithOriginalBaggage:nil];
-}
-
 - (NSString *)toHTTPHeaderWithOriginalBaggage:(NSDictionary *_Nullable)originalBaggage
 {
     NSMutableDictionary *information

diff --git a/Sources/Sentry/SentryNetworkTracker.m b/Sources/Sentry/SentryNetworkTracker.m
@@ -239,19 +239,23 @@ - (void)addBaggageHeader:(SentryBaggage *)baggage
     NSString *baggageHeader = @"";
 
     if (baggage != nil) {
-        baggageHeader =
-            [baggage toHTTPHeaderWithOriginalBaggage:
-                         [SentrySerialization
-                             decodeBaggage:sessionTask.currentRequest
-                                               .allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]]];
+        NSDictionary *originalBaggage = [SentrySerialization
+            decodeBaggage:sessionTask.currentRequest.allHTTPHeaderFields[SENTRY_BAGGAGE_HEADER]];
+
+        if (originalBaggage[@"sentry-trace_id"] == nil) {
+            baggageHeader = [baggage toHTTPHeaderWithOriginalBaggage:originalBaggage];
+        }
     }
 
     // First we check if the current request is mutable, so we could easily add a new
     // header. Otherwise we try to change the current request for a new one with the extra
     // header.
     if ([sessionTask.currentRequest isKindOfClass:[NSMutableURLRequest class]]) {
         NSMutableURLRequest *currentRequest = (NSMutableURLRequest *)sessionTask.currentRequest;
-        [currentRequest setValue:traceHeader.value forHTTPHeaderField:SENTRY_TRACE_HEADER];
+
+        if ([currentRequest valueForHTTPHeaderField:SENTRY_TRACE_HEADER] == nil) {
+            [currentRequest setValue:traceHeader.value forHTTPHeaderField:SENTRY_TRACE_HEADER];
+        }
 
         if (baggageHeader.length > 0) {
             [currentRequest setValue:baggageHeader forHTTPHeaderField:SENTRY_BAGGAGE_HEADER];
@@ -265,7 +269,9 @@ - (void)addBaggageHeader:(SentryBaggage *)baggage
         if ([sessionTask respondsToSelector:setCurrentRequestSelector]) {
             NSMutableURLRequest *newRequest = [sessionTask.currentRequest mutableCopy];
 
-            [newRequest setValue:traceHeader.value forHTTPHeaderField:SENTRY_TRACE_HEADER];
+            if ([newRequest valueForHTTPHeaderField:SENTRY_TRACE_HEADER] == nil) {
+                [newRequest setValue:traceHeader.value forHTTPHeaderField:SENTRY_TRACE_HEADER];
+            }
 
             if (baggageHeader.length > 0) {
                 [newRequest setValue:baggageHeader forHTTPHeaderField:SENTRY_BAGGAGE_HEADER];

diff --git a/Sources/Sentry/include/SentryBaggage.h b/Sources/Sentry/include/SentryBaggage.h
@@ -63,7 +63,6 @@ static NSString *const SENTRY_BAGGAGE_HEADER = @"baggage";
                      sampleRate:(nullable NSString *)sampleRate
                         sampled:(nullable NSString *)sampled;
 
-- (NSString *)toHTTPHeader;
 - (NSString *)toHTTPHeaderWithOriginalBaggage:(NSDictionary *_Nullable)originalBaggage;
 
 @end

diff --git a/...s/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerIntegrationTests.swift b/...s/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerIntegrationTests.swift
@@ -164,7 +164,7 @@ class SentryNetworkTrackerIntegrationTests: XCTestCase {
             self.assertNetworkError(error)
             let response = String(data: data ?? Data(), encoding: .utf8) ?? ""
 
-            let expectedBaggageHeader = transaction.traceContext.toBaggage().toHTTPHeader()
+            let expectedBaggageHeader = transaction.traceContext.toBaggage().toHTTPHeader(withOriginalBaggage: nil)
             XCTAssertEqual(expectedBaggageHeader, response)
 
             expect.fulfill()

diff --git a/Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerTests.swift
@@ -565,9 +565,21 @@ class SentryNetworkTrackerTests: XCTestCase {
         let transaction = startTransaction() as! SentryTracer
         sut.urlSessionTaskResume(task)
 
-        let expectedBaggageHeader = transaction.traceContext.toBaggage().toHTTPHeader()
+        let expectedBaggageHeader = transaction.traceContext.toBaggage().toHTTPHeader(withOriginalBaggage: nil)
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["baggage"] ?? "", expectedBaggageHeader)
     }
+
+    func testDontOverrideBaggageHeader() {
+        let sut = fixture.getSut()
+        let task = createDataTask {
+            var request = $0
+            request.setValue("sentry-trace_id=something", forHTTPHeaderField: "baggage")
+            return request
+        }
+        sut.urlSessionTaskResume(task)
+
+        XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["baggage"] ?? "", "sentry-trace_id=something")
+    }
 
     func testTraceHeader() {
         let sut = fixture.getSut()
@@ -580,6 +592,18 @@ class SentryNetworkTrackerTests: XCTestCase {
         let expectedTraceHeader = networkSpan.toTraceHeader().value()
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", expectedTraceHeader)
     }
+
+    func testDontOverrideTraceHeader() {
+        let sut = fixture.getSut()
+        let task = createDataTask {
+            var request = $0
+            request.setValue("test", forHTTPHeaderField: "sentry-trace")
+            return request
+        }
+        sut.urlSessionTaskResume(task)
+
+        XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", "test")
+    }
 
     func testDefaultHeadersWhenDisabled() {
         let sut = fixture.getSut()
@@ -591,7 +615,7 @@ class SentryNetworkTrackerTests: XCTestCase {
 
         let expectedTraceHeader = SentrySDK.currentHub().scope.propagationContext.traceHeader.value()
         let traceContext = SentryTraceContext(trace: SentrySDK.currentHub().scope.propagationContext.traceId, options: self.fixture.options, userSegment: self.fixture.scope.userObject?.segment)
-        let expectedBaggageHeader = traceContext.toBaggage().toHTTPHeader()
+        let expectedBaggageHeader = traceContext.toBaggage().toHTTPHeader(withOriginalBaggage: nil)
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["baggage"] ?? "", expectedBaggageHeader)
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", expectedTraceHeader)
     }
@@ -603,7 +627,7 @@ class SentryNetworkTrackerTests: XCTestCase {
 
         let expectedTraceHeader = SentrySDK.currentHub().scope.propagationContext.traceHeader.value()
         let traceContext = SentryTraceContext(trace: SentrySDK.currentHub().scope.propagationContext.traceId, options: self.fixture.options, userSegment: self.fixture.scope.userObject?.segment)
-        let expectedBaggageHeader = traceContext.toBaggage().toHTTPHeader()
+        let expectedBaggageHeader = traceContext.toBaggage().toHTTPHeader(withOriginalBaggage: nil)
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["baggage"] ?? "", expectedBaggageHeader)
         XCTAssertEqual(task.currentRequest?.allHTTPHeaderFields?["sentry-trace"] ?? "", expectedTraceHeader)
     }
@@ -897,9 +921,12 @@ class SentryNetworkTrackerTests: XCTestCase {
         XCTAssertEqual(duration, expectedDuration)
     }
 
-    func createDataTask(method: String = "GET") -> URLSessionDataTaskMock {
+    func createDataTask(method: String = "GET", modifyRequest: ((URLRequest) -> (URLRequest))? = nil) -> URLSessionDataTaskMock {
         var request = URLRequest(url: SentryNetworkTrackerTests.fullUrl)
         request.httpMethod = method
+        if let modifyRequest = modifyRequest {
+           request = modifyRequest(request)
+        }
         return URLSessionDataTaskMock(request: request)
     }
 

diff --git a/Tests/SentryTests/Transaction/SentryBaggageTests.swift b/Tests/SentryTests/Transaction/SentryBaggageTests.swift
@@ -3,20 +3,14 @@ import Sentry
 import XCTest
 
 class SentryBaggageTests: XCTestCase {
-    func test_baggageToHeader() {
-        let header = SentryBaggage(trace: SentryId.empty, publicKey: "publicKey", releaseName: "release name", environment: "teste", transaction: "transaction", userSegment: "test user", sampleRate: "0.49", sampled: "true").toHTTPHeader()
-
-        XCTAssertEqual(header, "sentry-environment=teste,sentry-public_key=publicKey,sentry-release=release%20name,sentry-sample_rate=0.49,sentry-sampled=true,sentry-trace_id=00000000000000000000000000000000,sentry-transaction=transaction,sentry-user_segment=test%20user")
-    }
-
     func test_baggageToHeader_AppendToOriginal() {
         let header = SentryBaggage(trace: SentryId.empty, publicKey: "publicKey", releaseName: "release name", environment: "teste", transaction: "transaction", userSegment: "test user", sampleRate: "0.49", sampled: "true").toHTTPHeader(withOriginalBaggage: ["a": "a", "sentry-trace_id": "to-be-overwritten"])
 
         XCTAssertEqual(header, "a=a,sentry-environment=teste,sentry-public_key=publicKey,sentry-release=release%20name,sentry-sample_rate=0.49,sentry-sampled=true,sentry-trace_id=00000000000000000000000000000000,sentry-transaction=transaction,sentry-user_segment=test%20user")
     }
 
     func test_baggageToHeader_onlyTrace_ignoreNils() {
-        let header = SentryBaggage(trace: SentryId.empty, publicKey: "publicKey", releaseName: nil, environment: nil, transaction: nil, userSegment: nil, sampleRate: nil, sampled: nil).toHTTPHeader()
+        let header = SentryBaggage(trace: SentryId.empty, publicKey: "publicKey", releaseName: nil, environment: nil, transaction: nil, userSegment: nil, sampleRate: nil, sampled: nil).toHTTPHeader(withOriginalBaggage: nil)
 
         XCTAssertEqual(header, "sentry-public_key=publicKey,sentry-trace_id=00000000000000000000000000000000")
     }

diff --git a/scripts/no-changes-in-high-risk-files.sh b/scripts/no-changes-in-high-risk-files.sh
@@ -5,7 +5,7 @@ set -euo pipefail
 
 ACTUAL=$(shasum -a 256 ./Sources/Sentry/SentryNSURLSessionTaskSearch.m ./Sources/Sentry/SentryNetworkTracker.m ./Sources/Sentry/SentryUIViewControllerSwizzling.m ./Sources/Sentry/SentryNSDataSwizzling.m ./Sources/Sentry/SentrySubClassFinder.m ./Sources/Sentry/SentryCoreDataSwizzling.m ./Sources/Sentry/SentrySwizzleWrapper.m ./Sources/Sentry/include/SentrySwizzle.h ./Sources/Sentry/SentrySwizzle.m)
 EXPECTED="819d5ca5e3db2ac23c859b14c149b7f0754d3ae88bea1dba92c18f49a81da0e1  ./Sources/Sentry/SentryNSURLSessionTaskSearch.m
-2f9ea6984ff32b53fc03c386b648f4f1bdf8737ce69050d25ce6d1307f8d1672  ./Sources/Sentry/SentryNetworkTracker.m
+bee886f6eaa3ec0ba999fe0dbe38537a47cb328a3432b908231326a65a9fb2d3  ./Sources/Sentry/SentryNetworkTracker.m
 40f476800b32cf885dba9ac3de75d93b6f536e819fe8e51d071b4610c879b416  ./Sources/Sentry/SentryUIViewControllerSwizzling.m
 e95e62ec7363984f20c78643bb7d992a41a740f97e1befb71525ac34caf88b37  ./Sources/Sentry/SentryNSDataSwizzling.m
 cc3849725bd1733515c71742872bed94ca47d2c115ef9d8c98383eae2e171925  ./Sources/Sentry/SentrySubClassFinder.m