From c36c2f04241ab3dafd88ad944490895889ee2f86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ot=C3=A1vio=20Lima?= Date: Mon, 8 Dec 2025 18:32:00 +0800 Subject: [PATCH] Fix: URLSessionHTTPClient only move file if response code is within OK range --- .../HTTPClient/URLSessionHTTPClient.swift | 6 ++ .../URLSessionHTTPClientTests.swift | 70 ++++++++++++++++++- 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift b/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift index a12c352f725..af2c73bda9d 100644 --- a/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift +++ b/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift @@ -307,6 +307,12 @@ private final class DownloadTaskManager: NSObject, URLSessionDownloadDelegate { do { let path = try AbsolutePath(validating: location.path) + // Only proceed to move the file if status code is within 200-299 range. + if let response = downloadTask.response as? HTTPURLResponse, + response.statusCode < 200 || response.statusCode >= 300 { + throw HTTPClientError.badResponseStatusCode(response.statusCode) + } + // Always using synchronous `localFileSystem` here since `URLSession` requires temporary `location` // to be moved from synchronously. Otherwise the file will be immediately cleaned up after returning // from this delegate method. diff --git a/Tests/BasicsTests/URLSessionHTTPClientTests.swift b/Tests/BasicsTests/URLSessionHTTPClientTests.swift index 275836d46b7..8082c1da9a4 100644 --- a/Tests/BasicsTests/URLSessionHTTPClientTests.swift +++ b/Tests/BasicsTests/URLSessionHTTPClientTests.swift @@ -494,7 +494,8 @@ final class URLSessionHTTPClientTest: XCTestCase { let completionExpectation = XCTestExpectation(description: "completion") let url = URL("https://downloader-tests.com/testServerError.zip") - var request = LegacyHTTPClient.Request.download(url: url, fileSystem: localFileSystem, destination: temporaryDirectory.appending("download")) + let destination = temporaryDirectory.appending("download") + var request = LegacyHTTPClient.Request.download(url: url, fileSystem: localFileSystem, destination: destination) request.options.validResponseCodes = [200] httpClient.execute( @@ -508,6 +509,8 @@ final class URLSessionHTTPClientTest: XCTestCase { XCTFail("unexpected success") case .failure(let error): XCTAssertEqual(error as? HTTPClientError, HTTPClientError.badResponseStatusCode(500)) + // Verify that no file was created at the destination for bad response codes + XCTAssertFalse(localFileSystem.exists(destination), "Destination file should not exist for bad response codes") } completionExpectation.fulfill() } @@ -998,13 +1001,14 @@ final class URLSessionHTTPClientTest: XCTestCase { try await testWithTemporaryDirectory { temporaryDirectory in let url = URL("https://async-downloader-tests.com/testServerError.zip") + let destination = temporaryDirectory.appending("download") var options = HTTPClientRequest.Options() options.validResponseCodes = [200] let request = HTTPClient.Request.download( url: url, options: options, fileSystem: localFileSystem, - destination: temporaryDirectory.appending("download") + destination: destination ) MockURLProtocol.onRequest(request) { request in @@ -1022,6 +1026,68 @@ final class URLSessionHTTPClientTest: XCTestCase { XCTFail("unexpected success") } catch { XCTAssertEqual(error as? HTTPClientError, HTTPClientError.badResponseStatusCode(500)) + // Verify that no file was created at the destination for bad response codes + XCTAssertFalse(localFileSystem.exists(destination), "Destination file should not exist for bad response codes") + } + } + } + + func testAsyncDownloadServerErrorAndResponseBody() async throws { + #if !os(macOS) + // URLSession Download tests can only run on macOS + // as re-libs-foundation's URLSessionTask implementation which expects the temporaryFileURL property to be on the request. + // and there is no way to set it in a mock + // https://github.com/apple/swift-corelibs-foundation/pull/2593 tries to address the latter part + try XCTSkipIf(true, "test is only supported on macOS") + #endif + let configuration = URLSessionConfiguration.default + configuration.protocolClasses = [MockURLProtocol.self] + let urlSession = URLSessionHTTPClient(configuration: configuration) + let httpClient = HTTPClient(implementation: urlSession.execute) + + try await testWithTemporaryDirectory { temporaryDirectory in + let url = URL("https://async-downloader-tests.com/testServerErrorWithBody.zip") + let destination = temporaryDirectory.appending("download") + var options = HTTPClientRequest.Options() + options.validResponseCodes = [200] + let request = HTTPClient.Request.download( + url: url, + options: options, + fileSystem: localFileSystem, + destination: destination + ) + + // Create an error response body (e.g., JSON error message) + let errorJson = Data(""" + { + "errors" : [ { + "status" : 500, + "message" : "Internal Server Error" + } ] + } + """.utf8) + + MockURLProtocol.onRequest(request) { request in + MockURLProtocol.sendResponse(statusCode: 500, headers: ["Content-Type": "application/json", "Content-Length": "\(errorJson.count)"], for: request) + // Send error response body - this should be downloaded to temp file but then deleted + MockURLProtocol.sendData(errorJson, for: request) + MockURLProtocol.sendCompletion(for: request) + } + + do { + _ = try await httpClient.execute( + request, + progress: { _, _ in + // Progress may be called as data is downloaded, even for error responses + // This is expected behavior + } + ) + XCTFail("unexpected success") + } catch { + XCTAssertEqual(error as? HTTPClientError, HTTPClientError.badResponseStatusCode(500)) + // Verify that no file was created at the destination for bad response codes + // even though a response body was sent + XCTAssertFalse(localFileSystem.exists(destination), "Destination file should not exist for bad response codes, even when response body is present") } } }