fix: Avoid mutating live network requests

CHANGELOG.md

@@ -18,6 +18,7 @@
 - Reduce memory usage when storing envelopes with large attachments (#8649)
 - Fix incorrect `duration` sent for active sessions (#8612)
   - Session `duration` is now set only when the session ends. Active sessions (including on error increments) no longer emit a bogus `duration`.
+- Fix a race caused by mutating `URLSessionTask.currentRequest` during trace header propagation (#8650)
 - Fix a race that could prevent consecutive app hangs from being reported (#8627)
 - Fix malformed itms-services URL in SentryDistribution updater (#8567)
 

Sources/Sentry/SentryTracePropagation.m

@@ -38,25 +38,18 @@ + (void)addBaggageHeader:(nullable SentryBaggage *)baggage
         }
     }
 
-    if ([request isKindOfClass:[NSMutableURLRequest class]]) {
-        NSMutableURLRequest *mutableRequest = (NSMutableURLRequest *)request;
-        [SentryTracePropagation addHeaderFieldsToRequest:mutableRequest
+    // CFNetwork may read currentRequest concurrently, so never mutate it in place.
+    SEL setCurrentRequestSelector = NSSelectorFromString(@"setCurrentRequest:");
+    if ([sessionTask respondsToSelector:setCurrentRequestSelector]) {
+        NSMutableURLRequest *newRequest = [request mutableCopy];
+        [SentryTracePropagation addHeaderFieldsToRequest:newRequest
                                              traceHeader:traceHeader
                                            baggageHeader:baggageHeader
                                     propagateTraceparent:propagateTraceparent];
-    } else {
-        SEL setCurrentRequestSelector = NSSelectorFromString(@"setCurrentRequest:");
-        if ([sessionTask respondsToSelector:setCurrentRequestSelector]) {
-            NSMutableURLRequest *newRequest = [request mutableCopy];
-            [SentryTracePropagation addHeaderFieldsToRequest:newRequest
-                                                 traceHeader:traceHeader
-                                               baggageHeader:baggageHeader
-                                        propagateTraceparent:propagateTraceparent];
 
-            void (*func)(id, SEL, id param)
-                = (void *)[sessionTask methodForSelector:setCurrentRequestSelector];
-            func(sessionTask, setCurrentRequestSelector, newRequest);
-        }
+        void (*func)(id, SEL, id param)
+            = (void *)[sessionTask methodForSelector:setCurrentRequestSelector];
+        func(sessionTask, setCurrentRequestSelector, newRequest);
     }
 }
 

Tests/SentryTests/Integrations/Performance/Network/SentryNetworkTrackerIntegrationTestServerTests.swift

@@ -11,10 +11,7 @@ class SentryNetworkTrackerIntegrationTestServerTests: XCTestCase {
 
     override func tearDown() {
         super.tearDown()
-        // Closing the SDK uninstalls all integrations, which disables the network tracker and its
-        // URLSession swizzling. Without this, the swizzled callbacks of one test leak into the next
-        // one and create spans on the wrong transaction.
-        SentrySDK.close()
+        clearTestState()
     }
 
     func testGetRequest_SpanCreatedAndBaggageHeaderAdded() throws {
@@ -93,6 +90,85 @@ class SentryNetworkTrackerIntegrationTestServerTests: XCTestCase {
         XCTAssertEqual(expectedTraceHeader, response)
     }
 
+    func testDownloadRequest_CompareSentryTraceHeader() throws {
+        // -- Arrange --
+        try ensureTestServerIsRunning()
+        let testTraceURL = try XCTUnwrap(URL(string: "http://localhost:8081/echo-sentry-trace"))
+        startSDK()
+        let transaction = try XCTUnwrap(
+            SentrySDK.startTransaction(
+                name: "Test Transaction",
+                operation: "TEST",
+                bindToScope: true
+            ) as? SentryTracer
+        )
+        let requestCompleted = expectation(description: "Download request completed")
+        // Cancelling the task in defer can trigger the completion handler again.
+        requestCompleted.assertForOverFulfill = false
+        var response: String?
+        let session = URLSession(configuration: URLSessionConfiguration.default)
+        let task = session.downloadTask(with: testTraceURL) { location, _, error in
+            self.assertNetworkError(error)
+            defer { requestCompleted.fulfill() }
+
+            guard let location else {
+                return XCTFail("Expected download location")
+            }
+
+            do {
+                response = String(data: try Data(contentsOf: location), encoding: .utf8)
+            } catch {
+                XCTFail("Failed to read download response: \(error)")
+            }
+        }
+        defer { task.cancel() }
+
+        // -- Act --
+        task.resume()
+        wait(for: [requestCompleted], timeout: 10)
+
+        // -- Assert --
+        let children = Dynamic(transaction).children as [SentrySpanInternal]?
+        let networkSpan = try XCTUnwrap(children?.first)
+        XCTAssertEqual(networkSpan.toTraceHeader().value(), response)
+    }
+
+    func testUploadRequest_CompareSentryTraceHeader() throws {
+        // -- Arrange --
+        try ensureTestServerIsRunning()
+        let testTraceURL = try XCTUnwrap(URL(string: "http://localhost:8081/echo-sentry-trace"))
+        startSDK()
+        let transaction = try XCTUnwrap(
+            SentrySDK.startTransaction(
+                name: "Test Transaction",
+                operation: "TEST",
+                bindToScope: true
+            ) as? SentryTracer
+        )
+        let requestCompleted = expectation(description: "Upload request completed")
+        // Cancelling the task in defer can trigger the completion handler again.
+        requestCompleted.assertForOverFulfill = false
+        var response: String?
+        var request = URLRequest(url: testTraceURL)
+        request.httpMethod = "POST"
+        let session = URLSession(configuration: URLSessionConfiguration.default)
+        let task = session.uploadTask(with: request, from: Data("test".utf8)) { data, _, error in
+            self.assertNetworkError(error)
+            response = String(data: data ?? Data(), encoding: .utf8)
+            requestCompleted.fulfill()
+        }
+        defer { task.cancel() }
+
+        // -- Act --
+        task.resume()
+        wait(for: [requestCompleted], timeout: 10)
+
+        // -- Assert --
+        let children = Dynamic(transaction).children as [SentrySpanInternal]?
+        let networkSpan = try XCTUnwrap(children?.first)
+        XCTAssertEqual(networkSpan.toTraceHeader().value(), response)
+    }
+
     func testGetCaptureFailedRequestsEnabled() throws {
         try ensureTestServerIsRunning()
 

Tests/SentryTests/Integrations/Performance/Network/SentryTracePropagationTests.swift

@@ -170,11 +170,84 @@ final class SentryTracePropagationTests: XCTestCase {
         XCTAssertNil(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"))
     }
 
-    private func createSessionTask(method: String = "GET") throws -> URLSessionDownloadTaskMock {
+    func testAddBaggageHeader_whenCurrentRequestIsMutable_shouldNotMutateItInPlace() throws {
+        // -- Arrange --
+        let request = URLRequest(url: try XCTUnwrap(URL(string: "https://www.domain.com/api")))
+        let task = MutableRequestTaskMock(request: request)
+        let traceHeader = TraceHeader(
+            trace: SentryId(),
+            spanId: SpanId(),
+            sampled: .yes
+        )
+
+        // -- Act --
+        SentryTracePropagation.addBaggageHeader(
+            Baggage(),
+            traceHeader: traceHeader,
+            propagateTraceparent: true,
+            tracePropagationTargets: [try XCTUnwrap(NSRegularExpression(pattern: ".*"))],
+            toRequest: task
+        )
+
+        // -- Assert --
+        XCTAssertEqual(task.setCurrentRequestCallCount, 1)
+        XCTAssertNil(task.initialCurrentRequest.value(forHTTPHeaderField: "sentry-trace"))
+        XCTAssertEqual(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"), traceHeader.value())
+    }
+
+    // MARK: - Trace propagation for download and upload tasks (issue #8519)
+
+    func testAddBaggageHeader_DownloadTask_AddsTraceHeaders() throws {
+        // -- Arrange --
+        let url = try XCTUnwrap(URL(string: "https://www.domain.com/api"))
+        let task = URLSessionDownloadTaskMock(request: URLRequest(url: url))
+        let traceHeader = TraceHeader(
+            trace: SentryId(),
+            spanId: SpanId(),
+            sampled: .yes
+        )
+
+        // -- Act --
+        SentryTracePropagation.addBaggageHeader(
+            Baggage(),
+            traceHeader: traceHeader,
+            propagateTraceparent: true,
+            tracePropagationTargets: [try XCTUnwrap(NSRegularExpression(pattern: ".*"))],
+            toRequest: task
+        )
+
+        // -- Assert --
+        XCTAssertEqual(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"), traceHeader.value())
+    }
+
+    func testAddBaggageHeader_UploadTask_AddsTraceHeaders() throws {
+        // -- Arrange --
+        let url = try XCTUnwrap(URL(string: "https://www.domain.com/api"))
+        let task = URLSessionUploadTaskMock(request: URLRequest(url: url))
+        let traceHeader = TraceHeader(
+            trace: SentryId(),
+            spanId: SpanId(),
+            sampled: .yes
+        )
+
+        // -- Act --
+        SentryTracePropagation.addBaggageHeader(
+            Baggage(),
+            traceHeader: traceHeader,
+            propagateTraceparent: true,
+            tracePropagationTargets: [try XCTUnwrap(NSRegularExpression(pattern: ".*"))],
+            toRequest: task
+        )
+
+        // -- Assert --
+        XCTAssertEqual(task.currentRequest?.value(forHTTPHeaderField: "sentry-trace"), traceHeader.value())
+    }
+
+    private func createSessionTask(method: String = "GET") throws -> URLSessionDataTaskMock {
         let url = try XCTUnwrap(URL(string: "https://www.domain.com/api?query=value&query2=value2#fragment"))
         var request = URLRequest(url: url)
         request.httpMethod = method
-        return URLSessionDownloadTaskMock(request: request)
+        return URLSessionDataTaskMock(request: request)
     }
 
 }

Tests/SentryTests/Integrations/Performance/Network/URLSessionTaskMock.h

@@ -43,6 +43,8 @@ static int64_t const DATA_BYTES_SENT = 652;
 
 - (void)setResponse:(NSURLResponse *)response;
 
+- (void)setCurrentRequest:(NSURLRequest *)request;
+
 @end
 
 @interface URLSessionUploadTaskMock : NSURLSessionUploadTask <URLSessionTaskMock>
@@ -53,6 +55,8 @@ static int64_t const DATA_BYTES_SENT = 652;
 
 - (void)setResponse:(NSURLResponse *)response;
 
+- (void)setCurrentRequest:(NSURLRequest *)request;
+
 @end
 
 @interface URLSessionStreamTaskMock : NSURLSessionStreamTask <URLSessionTaskMock>
@@ -86,4 +90,11 @@ static int64_t const DATA_BYTES_SENT = 652;
 
 @end
 
+@interface MutableRequestTaskMock : URLSessionDataTaskMock
+
+@property (nonatomic, readonly) NSMutableURLRequest *initialCurrentRequest;
+@property (nonatomic, readonly) NSUInteger setCurrentRequestCallCount;
+
+@end
+
 NS_ASSUME_NONNULL_END

Tests/SentryTests/Integrations/Performance/Network/URLSessionTaskMock.m

@@ -96,6 +96,7 @@ - (instancetype)initWithRequest:(NSURLRequest *)request
 
 @implementation URLSessionDownloadTaskMock {
     NSURLRequest *_request;
+    NSURLRequest *_currentRequest;
     NSURLResponse *_response;
     NSURLSessionTaskState _state;
     NSError *_error;
@@ -127,7 +128,12 @@ - (NSError *)error
 
 - (NSURLRequest *)currentRequest
 {
-    return _request;
+    return _currentRequest;
+}
+
+- (void)setCurrentRequest:(NSURLRequest *)request
+{
+    _currentRequest = request;
 }
 
 - (NSURLResponse *)response
@@ -156,6 +162,7 @@ - (instancetype)initWithRequest:(NSURLRequest *)request
 {
     if (self = [super init]) {
         _request = request;
+        _currentRequest = [_request mutableCopy];
     }
     return self;
 }
@@ -164,6 +171,7 @@ - (instancetype)initWithRequest:(NSURLRequest *)request
 
 @implementation URLSessionUploadTaskMock {
     NSURLRequest *_request;
+    NSURLRequest *_currentRequest;
     NSURLResponse *_response;
     NSURLSessionTaskState _state;
     NSError *_error;
@@ -195,7 +203,12 @@ - (NSError *)error
 
 - (NSURLRequest *)currentRequest
 {
-    return _request;
+    return _currentRequest;
+}
+
+- (void)setCurrentRequest:(NSURLRequest *)request
+{
+    _currentRequest = request;
 }
 
 - (NSURLResponse *)response
@@ -224,6 +237,7 @@ - (instancetype)initWithRequest:(NSURLRequest *)request
 {
     if (self = [super init]) {
         _request = request;
+        _currentRequest = [_request mutableCopy];
     }
     return self;
 }
@@ -325,3 +339,37 @@ - (NSURLRequest *)currentRequest
 }
 
 @end
+
+@implementation MutableRequestTaskMock {
+    NSMutableURLRequest *_initialCurrentRequest;
+    NSUInteger _setCurrentRequestCallCount;
+}
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+- (instancetype)initWithRequest:(NSURLRequest *)request
+{
+    if (self = [super initWithRequest:request]) {
+        _initialCurrentRequest = (NSMutableURLRequest *)[super currentRequest];
+    }
+    return self;
+}
+#pragma clang diagnostic pop
+
+- (NSMutableURLRequest *)initialCurrentRequest
+{
+    return _initialCurrentRequest;
+}
+
+- (NSUInteger)setCurrentRequestCallCount
+{
+    return _setCurrentRequestCallCount;
+}
+
+- (void)setCurrentRequest:(NSURLRequest *)request
+{
+    _setCurrentRequestCallCount++;
+    [super setCurrentRequest:request];
+}
+
+@end

test-server/Sources/App/routes.swift

@@ -23,6 +23,15 @@ public func routes(_ app: Application) throws {
         return "(NO-HEADER)"
     }
 
+    app.post("echo-sentry-trace") { request -> String in
+        let trace_id = request.headers["sentry-trace"]
+        if let sentryTraceHeader = trace_id.first {
+            return sentryTraceHeader
+        }
+
+        return "(NO-HEADER)"
+    }
+
     app.get("http-client-error") { _ -> String in
         throw Abort(.badRequest)
     }