From 373930f0f790dcd578a0070446e61e01d9a0c373 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:42:22 -0300 Subject: [PATCH 1/7] Add custom redirect handler support to RedirectConfiguration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds RedirectConfiguration.custom(_:), letting callers intercept every redirect-eligible response and decide whether/how to follow it instead of being limited to disallow/follow(max:allowCycles:). The candidate request handed to the handler has already gone through the same method/header rewrite rules `.follow` applies (POST->GET on 303, stripping Authorization/Cookie/Origin/Proxy-Authorization cross-origin), so callers only need to make further adjustments — e.g. stripping additional sensitive headers before a cross-host redirect is followed. Wired into the Swift Concurrency execute(_:deadline:logger:) family only; the delegate-based execute(request:delegate:...) API fails fast with .invalidRedirectConfiguration since it has no HTTPClientRequest to hand the handler. Co-Authored-By: Claude Sonnet 5 --- .../AsyncAwait/HTTPClient+execute.swift | 112 +++++++++++++----- Sources/AsyncHTTPClient/HTTPClient.swift | 67 ++++++++++- Sources/AsyncHTTPClient/RedirectState.swift | 35 ++++++ .../AsyncAwaitEndToEndTests.swift | 99 ++++++++++++++++ .../HTTPClientTests.swift | 18 +++ .../SwiftConfigurationTests.swift | 4 +- 6 files changed, 304 insertions(+), 31 deletions(-) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index a1047fa85..e2a162342 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -46,7 +46,7 @@ extension HTTPClient { request, deadline: deadline, logger: logger ?? Self.loggingDisabled, - redirectState: RedirectState(self.configuration.redirectConfiguration.mode, initialURL: request.url) + redirectMode: self.configuration.redirectConfiguration.mode ) } } @@ -88,10 +88,11 @@ extension HTTPClient { _ request: HTTPClientRequest, deadline: NIODeadline, logger: Logger, - redirectState: RedirectState? + redirectMode: HTTPClient.Configuration.RedirectConfiguration.Mode ) async throws -> HTTPClientResponse { var currentRequest = request - var currentRedirectState = redirectState + var currentRedirectState = RedirectState(redirectMode, initialURL: request.url) + var customRedirectCount = 0 var history: [HTTPClientRequestResponse] = [] // this loop is there to follow potential redirects @@ -122,39 +123,94 @@ extension HTTPClient { return response }() - guard var redirectState = currentRedirectState else { - // a `nil` redirectState means we should not follow redirects + switch redirectMode { + case .disallow: return response - } - guard - let redirectURL = response.headers.extractRedirectTarget( + case .follow: + guard var redirectState = currentRedirectState else { + // a `nil` redirectState means we should not follow redirects + return response + } + + guard + let redirectURL = response.headers.extractRedirectTarget( + status: response.status, + originalURL: preparedRequest.url, + originalScheme: preparedRequest.poolKey.scheme + ) + else { + // response does not want a redirect + return response + } + + // validate that we do not exceed any limits or are running circles + try redirectState.redirect(to: redirectURL.absoluteString) + currentRedirectState = redirectState + + let newRequest = currentRequest.followingRedirect( + from: preparedRequest.url, + to: redirectURL, status: response.status, - originalURL: preparedRequest.url, - originalScheme: preparedRequest.poolKey.scheme + config: redirectState.config ) - else { - // response does not want a redirect - return response - } - // validate that we do not exceed any limits or are running circles - try redirectState.redirect(to: redirectURL.absoluteString) - currentRedirectState = redirectState + guard newRequest.body.canBeConsumedMultipleTimes else { + // we already send the request body and it cannot be send again + return response + } - let newRequest = currentRequest.followingRedirect( - from: preparedRequest.url, - to: redirectURL, - status: response.status, - config: redirectState.config - ) + currentRequest = newRequest - guard newRequest.body.canBeConsumedMultipleTimes else { - // we already send the request body and it cannot be send again - return response - } + case .custom(let handler): + guard + let redirectURL = response.headers.extractRedirectTarget( + status: response.status, + originalURL: preparedRequest.url, + originalScheme: preparedRequest.poolKey.scheme + ) + else { + // response does not want a redirect + return response + } - currentRequest = newRequest + // Pre-build the request the same way `.follow` would, applying the standard + // method/header rewrite rules, so the handler only needs to make further + // adjustments rather than reimplement those rules itself. `max`/`allowCycles` + // are irrelevant here: only the `retainHTTPMethodAndBodyOn30{1,2}` flags feed + // into this transformation, and there's no built-in limit in `.custom` mode. + let candidateRequest = currentRequest.followingRedirect( + from: preparedRequest.url, + to: redirectURL, + status: response.status, + config: .init( + max: 0, + allowCycles: true, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ) + + let responseHead = HTTPResponseHead( + version: response.version, + status: response.status, + headers: response.headers + ) + + switch handler(candidateRequest, responseHead, customRedirectCount) { + case .doNotFollow: + return response + + case .follow(let newRequest): + guard newRequest.body.canBeConsumedMultipleTimes else { + // we already send the request body and it cannot be send again + return response + } + + customRedirectCount += 1 + currentRequest = newRequest + } + } } } diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index cc8792497..4aeff4d49 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -759,6 +759,23 @@ public final class HTTPClient: Sendable { ] ) + if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *), + case .custom = self.configuration.redirectConfiguration.mode + { + // `.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` and are + // only wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. + logger.debug( + "`.custom` redirect configuration is not supported by the delegate-based execute API, failing request" + ) + return Task.failedTask( + eventLoop: taskEL, + error: HTTPClientError.invalidRedirectConfiguration, + logger: logger, + tracing: tracing, + makeOrGetFileIOThreadPool: self.makeOrGetFileIOThreadPool + ) + } + let failedTask: Task? = self.state.withLockedValue { state -> (Task?) in switch state { case .upAndRunning: @@ -1312,11 +1329,14 @@ extension HTTPClient.Configuration { /// Specifies redirect processing settings. public struct RedirectConfiguration: Sendable { - enum Mode: Hashable { + enum Mode { /// Redirects are not followed. case disallow /// Redirects are followed with a specified limit. case follow(FollowConfiguration) + /// Redirects are handed to a caller-supplied handler. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + case custom(CustomRedirectHandler) } /// Configuration for following redirects. @@ -1353,6 +1373,38 @@ extension HTTPClient.Configuration { } } + /// The result of a ``CustomRedirectHandler`` deciding whether — and how — to follow a redirect. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + public enum RedirectDecision: Sendable { + /// Follow the redirect using the given request. + case follow(HTTPClientRequest) + /// Do not follow the redirect; the response that triggered it is returned as-is. + case doNotFollow + } + + /// A handler invoked whenever a response indicates a redirect (a 3xx status with a `Location` + /// header), giving the caller the chance to inspect, modify, or refuse the redirect before it is + /// sent. + /// + /// `redirectRequest` has already gone through the same method/header rewrite rules `.follow` + /// would apply (converting `POST` to `GET` on a 303, stripping `Authorization`/`Cookie`/`Origin`/ + /// `Proxy-Authorization` on cross-origin redirects) — the handler only needs to make further + /// adjustments, not reimplement those rules from scratch. + /// + /// - Parameters: + /// - redirectRequest: The request that would be sent to follow the redirect. + /// - response: The head of the response that triggered the redirect. + /// - redirectCount: How many redirects have already been followed for this logical request. + /// There is no built-in limit for `.custom` mode — the handler is responsible for enforcing + /// its own policy (e.g. refusing past a maximum count) to avoid infinite redirect loops. + /// - Returns: Whether — and with what request — to follow the redirect. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + public typealias CustomRedirectHandler = @Sendable ( + _ redirectRequest: HTTPClientRequest, + _ response: HTTPResponseHead, + _ redirectCount: Int + ) -> RedirectDecision + var mode: Mode init() { @@ -1397,6 +1449,19 @@ extension HTTPClient.Configuration { public static func follow(configuration: FollowConfiguration) -> RedirectConfiguration { .init(configuration: .follow(configuration)) } + + /// Redirects are handed to a caller-supplied handler, which decides whether and how to follow + /// each one. See ``CustomRedirectHandler``. + /// + /// - warning: There is no built-in redirect-count or cycle limit for this mode — use the + /// `redirectCount` passed to the handler to enforce your own policy. + /// - note: Only supported by the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. + /// Using `.custom` with the delegate-based `execute(request:delegate:...)` API fails with + /// ``HTTPClientError/invalidRedirectConfiguration``. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + public static func custom(_ handler: @escaping CustomRedirectHandler) -> RedirectConfiguration { + .init(configuration: .custom(handler)) + } } /// Connection pool configuration. diff --git a/Sources/AsyncHTTPClient/RedirectState.swift b/Sources/AsyncHTTPClient/RedirectState.swift index 9665c03d4..bfd4d89b1 100644 --- a/Sources/AsyncHTTPClient/RedirectState.swift +++ b/Sources/AsyncHTTPClient/RedirectState.swift @@ -22,6 +22,37 @@ import struct Foundation.URL typealias RedirectMode = HTTPClient.Configuration.RedirectConfiguration.Mode +// `Mode` can't derive `Equatable`/`Hashable` because `.custom` carries a closure. `.custom` values have +// no meaningful notion of equality, so — like `NaN` — a `.custom` value is never equal to any other +// value, including another `.custom`; this is consistent (if vacuously so) with the `Hashable` +// requirement that equal values hash equally. +extension HTTPClient.Configuration.RedirectConfiguration.Mode: Equatable { + static func == (lhs: Self, rhs: Self) -> Bool { + switch (lhs, rhs) { + case (.disallow, .disallow): + return true + case (.follow(let lhsConfig), .follow(let rhsConfig)): + return lhsConfig == rhsConfig + default: + return false + } + } +} + +extension HTTPClient.Configuration.RedirectConfiguration.Mode: Hashable { + func hash(into hasher: inout Hasher) { + switch self { + case .disallow: + hasher.combine(0) + case .follow(let config): + hasher.combine(1) + hasher.combine(config) + case .custom: + hasher.combine(2) + } + } +} + struct RedirectState { var config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration @@ -42,6 +73,10 @@ extension RedirectState { return nil case .follow(let config): self.init(config: config, visited: [initialURL]) + case .custom: + // `.custom` redirects are handled entirely by the caller-supplied handler; there is no + // count/cycle state for `RedirectState` to track. + return nil } } } diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 78dec4296..b2c28b796 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -13,6 +13,7 @@ //===----------------------------------------------------------------------===// import Logging +import NIOConcurrencyHelpers import NIOCore import NIOFoundationCompat import NIOHTTP1 @@ -768,6 +769,104 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } } + // MARK: - Custom redirect handler + + func testCustomRedirectHandlerCanRewriteRedirectRequest() { + XCTAsyncTest { + let bin = HTTPBin(.http1_1(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + let port = bin.port + var config = HTTPClient.Configuration() + config.redirectConfiguration = .custom { redirectRequest, response, redirectCount in + XCTAssertEqual(response.status, .found) + XCTAssertEqual(redirectCount, 0) + // The candidate request has already gone through the standard rewrite rules: it + // should already point at the `Location` from the /redirect/302 response. + XCTAssertEqual(redirectRequest.url, "http://localhost:\(port)/ok") + + var rewritten = redirectRequest + rewritten.url = "http://localhost:\(port)/echo-uri" + return .follow(rewritten) + } + + let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + let request = HTTPClientRequest(url: "http://localhost:\(port)/redirect/302") + guard + let response = await XCTAssertNoThrowWithResult( + try await client.execute(request, deadline: .now() + .seconds(10)) + ) + else { return } + + XCTAssertEqual(response.status, .ok) + XCTAssertEqual(response.headers.first(name: "X-Calling-URI"), "/echo-uri") + XCTAssertEqual( + response.history.map(\.request.url), + ["http://localhost:\(port)/redirect/302", "http://localhost:\(port)/echo-uri"] + ) + } + } + + func testCustomRedirectHandlerCanRefuseRedirect() { + XCTAsyncTest { + let bin = HTTPBin(.http1_1(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + var config = HTTPClient.Configuration() + config.redirectConfiguration = .custom { _, _, _ in .doNotFollow } + + let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + let request = HTTPClientRequest(url: "http://localhost:\(bin.port)/redirect/302") + guard + let response = await XCTAssertNoThrowWithResult( + try await client.execute(request, deadline: .now() + .seconds(10)) + ) + else { return } + + // The handler refused the redirect, so the 302 itself is the final response. + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.history.count, 1) + } + } + + func testCustomRedirectHandlerReceivesIncreasingRedirectCountAndCanBoundLoops() { + XCTAsyncTest { + let bin = HTTPBin(.http1_1(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + let observedCounts = NIOLockedValueBox<[Int]>([]) + var config = HTTPClient.Configuration() + config.redirectConfiguration = .custom { redirectRequest, _, redirectCount in + observedCounts.withLockedValue { $0.append(redirectCount) } + guard redirectCount < 3 else { + return .doNotFollow + } + return .follow(redirectRequest) + } + + let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + // /redirect/infinite1 <-> /redirect/infinite2 bounce forever. `.custom` mode has no + // built-in redirect limit (unlike `.follow`), so this exercises the handler enforcing + // its own bound using the `redirectCount` it's handed. + let request = HTTPClientRequest(url: "http://localhost:\(bin.port)/redirect/infinite1") + guard + let response = await XCTAssertNoThrowWithResult( + try await client.execute(request, deadline: .now() + .seconds(10)) + ) + else { return } + + XCTAssertEqual(response.status, .found) + XCTAssertEqual(observedCounts.withLockedValue { $0 }, [0, 1, 2, 3]) + XCTAssertEqual(response.history.count, 4) + } + } + func testShutdown() { XCTAsyncTest { let client = makeDefaultHTTPClient() diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 75371d437..74890932a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -448,6 +448,24 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertEqual("1234", data.data) } + func testCustomRedirectConfigurationFailsWithDelegateBasedExecute() throws { + // `.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` and are only + // wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs; the + // delegate-based `execute(request:delegate:...)` API (exercised here via `.get`) should fail + // fast rather than silently ignore the configured handler. + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .custom { _, _, _ in .doNotFollow } + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + XCTAssertThrowsError(try localClient.get(url: self.defaultHTTPBinURLPrefix + "ok").wait()) { + XCTAssertEqual($0 as? HTTPClientError, .invalidRedirectConfiguration) + } + } + func testHttpRedirect() throws { let httpsBin = HTTPBin(.http1_1(ssl: true)) let localClient = HTTPClient( diff --git a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift index e80d49ed8..74eb9ad9c 100644 --- a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift +++ b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift @@ -67,7 +67,7 @@ struct HTTPClientConfigurationPropsTests { #expect(follow.allowCycles) #expect(follow.retainHTTPMethodAndBodyOn301) #expect(follow.retainHTTPMethodAndBodyOn302) - case .disallow: + case .disallow, .custom: Issue.record("Unexpected value") } @@ -130,7 +130,7 @@ struct HTTPClientConfigurationPropsTests { switch config.redirectConfiguration.mode { case .disallow: break - case .follow: + case .follow, .custom: Issue.record("Unexpected value") } } From 26f4fc38f7b689817aa7d7d37d2d1dc7e47db145 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:04:01 -0300 Subject: [PATCH 2/7] Rework custom redirect handling as a pluggable HTTPClientRedirectStrategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the bare-closure `.custom(_:)` from the previous commit with HTTPClientRedirectStrategy, a protocol callers can conform their own types to (not just closures), addressing the two gaps a maintainer flagged on the upstream issue thread: pluggable strategies as actual types, and access to more than a bare redirect count — the strategy now receives the full per-request history alongside the candidate request and response. - HTTPClientRedirectContext bundles redirectRequest/response/history/ redirectCount into one value instead of four positional parameters. - HTTPClientRedirectStrategy.redirectDecision(for:) can throw, so a strategy can fail the whole execute() call with a custom error instead of only following/refusing. - RedirectConfiguration.strategy(_:) is the primary entry point; .custom(_:) remains as a closure-based convenience over it via an internal ClosureRedirectStrategy adapter. - Mode.custom renamed to Mode.strategy to match. Still scoped to the Swift Concurrency execute(_:deadline:logger:) family only; the delegate-based API continues to fail fast with .invalidRedirectConfiguration. Co-Authored-By: Claude Sonnet 5 --- .../AsyncAwait/HTTPClient+execute.swift | 21 ++-- .../HTTPClientRedirectStrategy.swift | 96 +++++++++++++++++ Sources/AsyncHTTPClient/HTTPClient.swift | 68 ++++-------- Sources/AsyncHTTPClient/RedirectState.swift | 14 +-- .../AsyncAwaitEndToEndTests.swift | 101 +++++++++++++++--- .../HTTPClientTests.swift | 2 +- .../SwiftConfigurationTests.swift | 4 +- 7 files changed, 229 insertions(+), 77 deletions(-) create mode 100644 Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRedirectStrategy.swift diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index e2a162342..2ba78eaab 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -162,7 +162,7 @@ extension HTTPClient { currentRequest = newRequest - case .custom(let handler): + case .strategy(let strategy): guard let redirectURL = response.headers.extractRedirectTarget( status: response.status, @@ -175,10 +175,10 @@ extension HTTPClient { } // Pre-build the request the same way `.follow` would, applying the standard - // method/header rewrite rules, so the handler only needs to make further + // method/header rewrite rules, so the strategy only needs to make further // adjustments rather than reimplement those rules itself. `max`/`allowCycles` // are irrelevant here: only the `retainHTTPMethodAndBodyOn30{1,2}` flags feed - // into this transformation, and there's no built-in limit in `.custom` mode. + // into this transformation, and there's no built-in limit in `.strategy` mode. let candidateRequest = currentRequest.followingRedirect( from: preparedRequest.url, to: redirectURL, @@ -191,13 +191,18 @@ extension HTTPClient { ) ) - let responseHead = HTTPResponseHead( - version: response.version, - status: response.status, - headers: response.headers + let context = HTTPClientRedirectContext( + redirectRequest: candidateRequest, + response: HTTPResponseHead( + version: response.version, + status: response.status, + headers: response.headers + ), + history: history, + redirectCount: customRedirectCount ) - switch handler(candidateRequest, responseHead, customRedirectCount) { + switch try strategy.redirectDecision(for: context) { case .doNotFollow: return response diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRedirectStrategy.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRedirectStrategy.swift new file mode 100644 index 000000000..508b2ff79 --- /dev/null +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRedirectStrategy.swift @@ -0,0 +1,96 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2026 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import NIOHTTP1 + +/// A pluggable strategy for deciding whether — and how — to follow HTTP redirects, used via +/// ``HTTPClient/Configuration/RedirectConfiguration/strategy(_:)``. +/// +/// Unlike `.disallow`/`.follow(max:allowCycles:)`, a strategy gets a chance to inspect every +/// redirect-eligible response before it's followed: adjust the outgoing request, refuse the redirect +/// outright, or fail the whole request with a custom error. +/// +/// A single strategy instance is stored on ``HTTPClient/Configuration`` and reused for every request +/// that client makes, including concurrently — if your strategy holds mutable state (e.g. an audit +/// log, a shared allow-list), synchronize it yourself (an `actor`, or a class using a lock). +/// Per-request state doesn't need that: ``HTTPClientRedirectContext/history`` already carries +/// everything tracked so far for the *current* logical request, so most policies (host allow-listing, +/// loop bounds, auditing) can be implemented statelessly by reading it fresh on each call. +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +public protocol HTTPClientRedirectStrategy: Sendable { + /// Decide whether — and how — to follow a redirect. + /// + /// - Parameter context: Everything known about the redirect so far. See + /// ``HTTPClientRedirectContext``. + /// - Returns: Whether — and with what request — to follow the redirect. + /// - Throws: To fail the whole `execute(...)` call with a custom error instead of following the + /// redirect or returning the response that triggered it. + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision +} + +/// Everything a ``HTTPClientRedirectStrategy`` is handed to decide whether — and how — to follow one +/// redirect. +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +public struct HTTPClientRedirectContext: Sendable { + /// The request that would be sent to follow the redirect. It has already gone through the same + /// method/header rewrite rules `.follow` would apply (converting `POST` to `GET` on a 303, + /// stripping `Authorization`/`Cookie`/`Origin`/`Proxy-Authorization` on cross-origin redirects) — + /// you only need to make further adjustments, not reimplement those rules from scratch. + public var redirectRequest: HTTPClientRequest + + /// The head of the response that triggered the redirect. + public var response: HTTPResponseHead + + /// Every request/response pair sent so far for this logical request, oldest first, including the + /// one that produced ``response``. This is the same data that ends up in + /// ``HTTPClientResponse/history`` on the final response. + public var history: [HTTPClientRequestResponse] + + /// How many redirects have already been followed for this logical request (equivalently, + /// `history.count - 1`). There is no built-in limit for `.strategy`/`.custom` mode — enforce your + /// own policy (e.g. refusing past a maximum count) to avoid infinite redirect loops. + public var redirectCount: Int + + public init( + redirectRequest: HTTPClientRequest, + response: HTTPResponseHead, + history: [HTTPClientRequestResponse], + redirectCount: Int + ) { + self.redirectRequest = redirectRequest + self.response = response + self.history = history + self.redirectCount = redirectCount + } +} + +/// The result of a ``HTTPClientRedirectStrategy`` deciding whether — and how — to follow a redirect. +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +public enum HTTPClientRedirectDecision: Sendable { + /// Follow the redirect using the given request. + case follow(HTTPClientRequest) + /// Do not follow the redirect; the response that triggered it is returned as-is. + case doNotFollow +} + +/// Adapts a closure to ``HTTPClientRedirectStrategy``, backing +/// ``HTTPClient/Configuration/RedirectConfiguration/custom(_:)``. +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +struct ClosureRedirectStrategy: HTTPClientRedirectStrategy { + let handler: @Sendable (HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision + + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision { + try self.handler(context) + } +} diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 4aeff4d49..df9e70194 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -760,12 +760,12 @@ public final class HTTPClient: Sendable { ) if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *), - case .custom = self.configuration.redirectConfiguration.mode + case .strategy = self.configuration.redirectConfiguration.mode { - // `.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` and are - // only wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. + // `.strategy`/`.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` + // and are only wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. logger.debug( - "`.custom` redirect configuration is not supported by the delegate-based execute API, failing request" + "`.strategy` redirect configuration is not supported by the delegate-based execute API, failing request" ) return Task.failedTask( eventLoop: taskEL, @@ -1334,9 +1334,9 @@ extension HTTPClient.Configuration { case disallow /// Redirects are followed with a specified limit. case follow(FollowConfiguration) - /// Redirects are handed to a caller-supplied handler. + /// Redirects are handed to a pluggable ``HTTPClientRedirectStrategy``. @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - case custom(CustomRedirectHandler) + case strategy(any HTTPClientRedirectStrategy) } /// Configuration for following redirects. @@ -1373,38 +1373,6 @@ extension HTTPClient.Configuration { } } - /// The result of a ``CustomRedirectHandler`` deciding whether — and how — to follow a redirect. - @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - public enum RedirectDecision: Sendable { - /// Follow the redirect using the given request. - case follow(HTTPClientRequest) - /// Do not follow the redirect; the response that triggered it is returned as-is. - case doNotFollow - } - - /// A handler invoked whenever a response indicates a redirect (a 3xx status with a `Location` - /// header), giving the caller the chance to inspect, modify, or refuse the redirect before it is - /// sent. - /// - /// `redirectRequest` has already gone through the same method/header rewrite rules `.follow` - /// would apply (converting `POST` to `GET` on a 303, stripping `Authorization`/`Cookie`/`Origin`/ - /// `Proxy-Authorization` on cross-origin redirects) — the handler only needs to make further - /// adjustments, not reimplement those rules from scratch. - /// - /// - Parameters: - /// - redirectRequest: The request that would be sent to follow the redirect. - /// - response: The head of the response that triggered the redirect. - /// - redirectCount: How many redirects have already been followed for this logical request. - /// There is no built-in limit for `.custom` mode — the handler is responsible for enforcing - /// its own policy (e.g. refusing past a maximum count) to avoid infinite redirect loops. - /// - Returns: Whether — and with what request — to follow the redirect. - @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - public typealias CustomRedirectHandler = @Sendable ( - _ redirectRequest: HTTPClientRequest, - _ response: HTTPResponseHead, - _ redirectCount: Int - ) -> RedirectDecision - var mode: Mode init() { @@ -1450,17 +1418,27 @@ extension HTTPClient.Configuration { .init(configuration: .follow(configuration)) } - /// Redirects are handed to a caller-supplied handler, which decides whether and how to follow - /// each one. See ``CustomRedirectHandler``. + /// Redirects are handed to a pluggable strategy, which decides whether and how to follow each + /// one. See ``HTTPClientRedirectStrategy``. /// /// - warning: There is no built-in redirect-count or cycle limit for this mode — use the - /// `redirectCount` passed to the handler to enforce your own policy. + /// `redirectCount`/`history` passed to the strategy to enforce your own policy. /// - note: Only supported by the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. - /// Using `.custom` with the delegate-based `execute(request:delegate:...)` API fails with - /// ``HTTPClientError/invalidRedirectConfiguration``. + /// Using `.strategy`/`.custom` with the delegate-based `execute(request:delegate:...)` API + /// fails with ``HTTPClientError/invalidRedirectConfiguration``. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + public static func strategy(_ strategy: any HTTPClientRedirectStrategy) -> RedirectConfiguration { + .init(configuration: .strategy(strategy)) + } + + /// Convenience over ``strategy(_:)`` for a policy that doesn't need its own type: redirects are + /// handed to `handler`, which decides whether and how to follow each one. See + /// ``HTTPClientRedirectContext`` for what `handler` receives. @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - public static func custom(_ handler: @escaping CustomRedirectHandler) -> RedirectConfiguration { - .init(configuration: .custom(handler)) + public static func custom( + _ handler: @escaping @Sendable (HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision + ) -> RedirectConfiguration { + .strategy(ClosureRedirectStrategy(handler: handler)) } } diff --git a/Sources/AsyncHTTPClient/RedirectState.swift b/Sources/AsyncHTTPClient/RedirectState.swift index bfd4d89b1..466b4af6a 100644 --- a/Sources/AsyncHTTPClient/RedirectState.swift +++ b/Sources/AsyncHTTPClient/RedirectState.swift @@ -22,10 +22,10 @@ import struct Foundation.URL typealias RedirectMode = HTTPClient.Configuration.RedirectConfiguration.Mode -// `Mode` can't derive `Equatable`/`Hashable` because `.custom` carries a closure. `.custom` values have -// no meaningful notion of equality, so — like `NaN` — a `.custom` value is never equal to any other -// value, including another `.custom`; this is consistent (if vacuously so) with the `Hashable` -// requirement that equal values hash equally. +// `Mode` can't derive `Equatable`/`Hashable` because `.strategy` carries an existential. `.strategy` +// values have no meaningful notion of equality, so — like `NaN` — a `.strategy` value is never equal +// to any other value, including another `.strategy`; this is consistent (if vacuously so) with the +// `Hashable` requirement that equal values hash equally. extension HTTPClient.Configuration.RedirectConfiguration.Mode: Equatable { static func == (lhs: Self, rhs: Self) -> Bool { switch (lhs, rhs) { @@ -47,7 +47,7 @@ extension HTTPClient.Configuration.RedirectConfiguration.Mode: Hashable { case .follow(let config): hasher.combine(1) hasher.combine(config) - case .custom: + case .strategy: hasher.combine(2) } } @@ -73,8 +73,8 @@ extension RedirectState { return nil case .follow(let config): self.init(config: config, visited: [initialURL]) - case .custom: - // `.custom` redirects are handled entirely by the caller-supplied handler; there is no + case .strategy: + // `.strategy` redirects are handled entirely by the caller-supplied strategy; there is no // count/cycle state for `RedirectState` to track. return nil } diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index b2c28b796..f7d627d02 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -769,7 +769,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } } - // MARK: - Custom redirect handler + // MARK: - Pluggable redirect strategies func testCustomRedirectHandlerCanRewriteRedirectRequest() { XCTAsyncTest { @@ -778,14 +778,15 @@ final class AsyncAwaitEndToEndTests: XCTestCase { let port = bin.port var config = HTTPClient.Configuration() - config.redirectConfiguration = .custom { redirectRequest, response, redirectCount in - XCTAssertEqual(response.status, .found) - XCTAssertEqual(redirectCount, 0) + config.redirectConfiguration = .custom { context in + XCTAssertEqual(context.response.status, .found) + XCTAssertEqual(context.redirectCount, 0) + XCTAssertEqual(context.history.count, 1) // The candidate request has already gone through the standard rewrite rules: it // should already point at the `Location` from the /redirect/302 response. - XCTAssertEqual(redirectRequest.url, "http://localhost:\(port)/ok") + XCTAssertEqual(context.redirectRequest.url, "http://localhost:\(port)/ok") - var rewritten = redirectRequest + var rewritten = context.redirectRequest rewritten.url = "http://localhost:\(port)/echo-uri" return .follow(rewritten) } @@ -815,7 +816,7 @@ final class AsyncAwaitEndToEndTests: XCTestCase { defer { XCTAssertNoThrow(try bin.shutdown()) } var config = HTTPClient.Configuration() - config.redirectConfiguration = .custom { _, _, _ in .doNotFollow } + config.redirectConfiguration = .custom { _ in .doNotFollow } let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) defer { XCTAssertNoThrow(try client.syncShutdown()) } @@ -840,20 +841,21 @@ final class AsyncAwaitEndToEndTests: XCTestCase { let observedCounts = NIOLockedValueBox<[Int]>([]) var config = HTTPClient.Configuration() - config.redirectConfiguration = .custom { redirectRequest, _, redirectCount in - observedCounts.withLockedValue { $0.append(redirectCount) } - guard redirectCount < 3 else { + config.redirectConfiguration = .custom { context in + XCTAssertEqual(context.history.count, context.redirectCount + 1) + observedCounts.withLockedValue { $0.append(context.redirectCount) } + guard context.redirectCount < 3 else { return .doNotFollow } - return .follow(redirectRequest) + return .follow(context.redirectRequest) } let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) defer { XCTAssertNoThrow(try client.syncShutdown()) } - // /redirect/infinite1 <-> /redirect/infinite2 bounce forever. `.custom` mode has no - // built-in redirect limit (unlike `.follow`), so this exercises the handler enforcing - // its own bound using the `redirectCount` it's handed. + // /redirect/infinite1 <-> /redirect/infinite2 bounce forever. `.custom`/`.strategy` mode + // has no built-in redirect limit (unlike `.follow`), so this exercises the handler + // enforcing its own bound using the `redirectCount` it's handed. let request = HTTPClientRequest(url: "http://localhost:\(bin.port)/redirect/infinite1") guard let response = await XCTAssertNoThrowWithResult( @@ -867,6 +869,77 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } } + /// A real `HTTPClientRedirectStrategy` conformance (not a closure), proving strategies are + /// genuinely pluggable types — and using `history` (not just a count) to detect that a redirect + /// target has already been visited, the way `.follow(allowCycles: false)` does internally. + private struct VisitedURLCycleDetectingStrategy: HTTPClientRedirectStrategy { + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision { + let visited = Set(context.history.map(\.request.url)) + guard !visited.contains(context.redirectRequest.url) else { + return .doNotFollow + } + return .follow(context.redirectRequest) + } + } + + func testRedirectStrategyTypeDetectsCyclesUsingHistory() { + XCTAsyncTest { + let bin = HTTPBin(.http1_1(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + var config = HTTPClient.Configuration() + config.redirectConfiguration = .strategy(VisitedURLCycleDetectingStrategy()) + + let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + // infinite1 -> infinite2 -> infinite1 (already visited, refused). + let request = HTTPClientRequest(url: "http://localhost:\(bin.port)/redirect/infinite1") + guard + let response = await XCTAssertNoThrowWithResult( + try await client.execute(request, deadline: .now() + .seconds(10)) + ) + else { return } + + XCTAssertEqual(response.status, .found) + XCTAssertEqual( + response.history.map(\.request.url), + [ + "http://localhost:\(bin.port)/redirect/infinite1", + "http://localhost:\(bin.port)/redirect/infinite2", + ] + ) + } + } + + private struct RedirectRefusedError: Error, Equatable {} + + private struct ThrowingRedirectStrategy: HTTPClientRedirectStrategy { + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision { + throw RedirectRefusedError() + } + } + + func testRedirectStrategyCanThrowToFailTheRequest() { + XCTAsyncTest { + let bin = HTTPBin(.http1_1(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + var config = HTTPClient.Configuration() + config.redirectConfiguration = .strategy(ThrowingRedirectStrategy()) + + let client = HTTPClient(eventLoopGroupProvider: .singleton, configuration: config) + defer { XCTAssertNoThrow(try client.syncShutdown()) } + + let request = HTTPClientRequest(url: "http://localhost:\(bin.port)/redirect/302") + await XCTAssertThrowsError( + try await client.execute(request, deadline: .now() + .seconds(10)) + ) { + XCTAssertEqual($0 as? RedirectRefusedError, RedirectRefusedError()) + } + } + } + func testShutdown() { XCTAsyncTest { let client = makeDefaultHTTPClient() diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 74890932a..5ae14994e 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -456,7 +456,7 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { let localClient = HTTPClient( eventLoopGroupProvider: .shared(self.clientGroup), configuration: HTTPClient.Configuration( - redirectConfiguration: .custom { _, _, _ in .doNotFollow } + redirectConfiguration: .custom { _ in .doNotFollow } ) ) defer { XCTAssertNoThrow(try localClient.syncShutdown()) } diff --git a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift index 74eb9ad9c..f2f1f8a98 100644 --- a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift +++ b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift @@ -67,7 +67,7 @@ struct HTTPClientConfigurationPropsTests { #expect(follow.allowCycles) #expect(follow.retainHTTPMethodAndBodyOn301) #expect(follow.retainHTTPMethodAndBodyOn302) - case .disallow, .custom: + case .disallow, .strategy: Issue.record("Unexpected value") } @@ -130,7 +130,7 @@ struct HTTPClientConfigurationPropsTests { switch config.redirectConfiguration.mode { case .disallow: break - case .follow, .custom: + case .follow, .strategy: Issue.record("Unexpected value") } } From 92febeb6bbd32a420aece71434e446341e3cbac6 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:16:48 -0300 Subject: [PATCH 3/7] Re-trigger CI now that beta's swift-ci.yaml points at a valid ref The previous two CI runs on this branch failed before any job started ("reference to workflow should be either a valid branch, tag, or commit") because beta's swift-ci.yaml referenced a since-deleted request-dl/.github branch. That's now fixed on beta (f360eae); this empty commit just re-triggers the pull_request check with no code changes. Co-Authored-By: Claude Sonnet 5 From c1ecceb1f6dcbe63327c4734f103bb7393167154 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:44:25 -0300 Subject: [PATCH 4/7] Fix API-breakage-check build failure: enum case can't be @available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swift disallows @available on an enum case that carries an associated value, so `Mode.strategy(any HTTPClientRedirectStrategy)` failed to compile under the API digester's build (which surfaces this check that normal `swift build` doesn't enforce). Store the payload as `any Sendable` instead — the same type-erasure trick already used for `Configuration._tracer` — and downcast at the one call site that needs the concrete protocol. Co-Authored-By: Claude Sonnet 5 --- .../AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift | 3 ++- Sources/AsyncHTTPClient/HTTPClient.swift | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index 2ba78eaab..38982fdc7 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -162,7 +162,8 @@ extension HTTPClient { currentRequest = newRequest - case .strategy(let strategy): + case .strategy(let anyStrategy): + let strategy = anyStrategy as! any HTTPClientRedirectStrategy guard let redirectURL = response.headers.extractRedirectTarget( status: response.status, diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index df9e70194..301973538 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -1335,8 +1335,12 @@ extension HTTPClient.Configuration { /// Redirects are followed with a specified limit. case follow(FollowConfiguration) /// Redirects are handed to a pluggable ``HTTPClientRedirectStrategy``. - @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) - case strategy(any HTTPClientRedirectStrategy) + /// + /// Stored as `any Sendable` (erasure trick so this case doesn't need to be marked + /// `@available`, which Swift disallows on enum cases with associated values) — always an + /// `any HTTPClientRedirectStrategy` underneath, since `.strategy(_:)`/`.custom(_:)` are the + /// only way to construct one. + case strategy(any Sendable) } /// Configuration for following redirects. From 94e6572dbe67cfc88ce6930ceb805c56ac7e6920 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:16:27 -0300 Subject: [PATCH 5/7] Support .strategy redirect configuration on the delegate-based execute API `.strategy`/`.custom` redirect handlers were only wired up for the Concurrency `execute(_:deadline:logger:)` family; the delegate-based `execute(request:delegate:...)` API fast-failed with `.invalidRedirectConfiguration` instead. That's fine on its own, but a caller whose whole pipeline is built on the delegate-based API (streaming responses through a custom `HTTPClientResponseDelegate`, as request-dl-nio's `Internals.Client` does) had no way to use `.strategy` at all -- not even to make the same follow/rewrite/refuse decisions `.follow` already supports there. `RedirectState` becomes an enum (`.follow`/`.strategy`) instead of a struct that only ever modeled `.follow`, and `RedirectHandler.redirect` now builds a `HTTPClientRedirectContext` and calls the configured strategy for `.strategy` too, converting between the delegate API's legacy `HTTPClient.Request`/`Body` and the Concurrency API's `HTTPClientRequest`/`Body` (`RedirectStrategyLegacyBridge.swift`) -- reusing a strategy's untouched `redirectRequest.body` verbatim via a new internal `Body.Mode.legacyBody` case, and lossless synchronous draining for `.bytes`/`.byteBuffer` bodies a strategy replaces it with. `.doNotFollow` is not supported on this path: honoring it would mean resuming normal response delivery for a response the delegate-based state machine already committed to treating as a redirect candidate, which that state machine has no route back from (`.follow`'s own redirect-limit/cycle errors fail the whole task the same way, they never resume delivery either). It fails with `.invalidRedirectConfiguration` instead, same as before this change, with a doc comment pointing at the Concurrency API for callers that need it. Co-Authored-By: Claude Sonnet 5 --- .../AsyncAwait/HTTPClient+execute.swift | 8 +- .../HTTPClientRequest+Prepared.swift | 12 ++ .../AsyncAwait/HTTPClientRequest.swift | 17 +++ Sources/AsyncHTTPClient/HTTPClient.swift | 22 ++- Sources/AsyncHTTPClient/HTTPHandler.swift | 134 ++++++++++++++++-- Sources/AsyncHTTPClient/RedirectState.swift | 82 ++++++++--- .../RedirectStrategyLegacyBridge.swift | 92 ++++++++++++ Sources/AsyncHTTPClient/RequestBag.swift | 6 +- Sources/AsyncHTTPClient/TracingSupport.swift | 2 + .../HTTPClientTests.swift | 65 ++++++++- 10 files changed, 392 insertions(+), 48 deletions(-) create mode 100644 Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index 38982fdc7..8fc0b3759 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -128,7 +128,7 @@ extension HTTPClient { return response case .follow: - guard var redirectState = currentRedirectState else { + guard case .follow(var followState)? = currentRedirectState else { // a `nil` redirectState means we should not follow redirects return response } @@ -145,14 +145,14 @@ extension HTTPClient { } // validate that we do not exceed any limits or are running circles - try redirectState.redirect(to: redirectURL.absoluteString) - currentRedirectState = redirectState + try followState.redirect(to: redirectURL.absoluteString) + currentRedirectState = .follow(followState) let newRequest = currentRequest.followingRedirect( from: preparedRequest.url, to: redirectURL, status: response.status, - config: redirectState.config + config: followState.config ) guard newRequest.body.canBeConsumedMultipleTimes else { diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift index 3e4482292..518c5873a 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift @@ -114,6 +114,14 @@ extension HTTPClientRequest.Prepared.Body { ) case .byteBuffer(let byteBuffer): self = .byteBuffer(byteBuffer) + case .legacyBody: + // Only ever produced by the delegate-based redirect-strategy bridge + // (`RedirectStrategyLegacyBridge.swift`), which converts it back into a + // `HTTPClient.Body` (`asLegacyBody()`) rather than routing it through the + // Concurrency API's own request preparation. + fatalError( + "`.legacyBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asLegacyBody()` instead." + ) #if UnstableHTTPAPIsSupport case .httpClientRequestBody(let length, let requestBody): self = .httpClientRequestBody(length, requestBody) @@ -132,6 +140,10 @@ extension RequestBodyLength { self = .known(Int64(buffer.readableBytes)) case .sequence(let length, _, _), .asyncSequence(let length, _): self = length + case .legacyBody: + fatalError( + "`.legacyBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asLegacyBody()` instead." + ) #if UnstableHTTPAPIsSupport case .httpClientRequestBody(let length, _): self = length diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift index 17563a122..c8832e68e 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift @@ -101,6 +101,17 @@ extension HTTPClientRequest { ) case byteBuffer(ByteBuffer) + /// Wraps a legacy ``HTTPClient/Body`` verbatim, untouched. + /// + /// Exists solely so the delegate-based `execute(request:delegate:...)` API can offer a + /// redirect-eligible request's body to a ``HTTPClientRedirectStrategy`` (which is typed + /// in terms of this Concurrency-API `Body`, not the legacy one) without re-encoding it -- + /// see `RedirectStrategyLegacyBridge.swift`. `canBeConsumedMultipleTimes` is conservatively + /// `false`: a legacy `Body`'s `stream` closure isn't guaranteed replayable. Never produced + /// by any public factory, and never asked to iterate -- it is unwrapped back into a + /// `HTTPClient.Body` (`asLegacyBody()`) before it would ever need to stream. + case legacyBody(HTTPClient.Body) + #if UnstableHTTPAPIsSupport case httpClientRequestBody( length: RequestBodyLength, @@ -398,6 +409,7 @@ extension Optional where Wrapped == HTTPClientRequest.Body { case .byteBuffer: return true case .sequence(_, let canBeConsumedMultipleTimes, _): return canBeConsumedMultipleTimes case .asyncSequence: return false + case .legacyBody: return false #if UnstableHTTPAPIsSupport case .httpClientRequestBody: return false // TODO: I think this should be TRUE #endif @@ -441,6 +453,11 @@ extension HTTPClientRequest.Body: AsyncSequence { return .init(storage: .byteBuffer(makeCompleteBody(AsyncIterator.allocator))) case .byteBuffer(let byteBuffer): return .init(storage: .byteBuffer(byteBuffer)) + case .legacyBody: + // Only ever produced by the delegate-based redirect-strategy bridge, which unwraps + // it back into a `HTTPClient.Body` (`asLegacyBody()`) instead of ever asking this + // AsyncSequence conformance to iterate it. + fatalError("`.legacyBody` is never iterated -- it is unwrapped via `asLegacyBody()` instead.") #if UnstableHTTPAPIsSupport case .httpClientRequestBody: fatalError("Unimplemented") diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 301973538..3037103ed 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -759,13 +759,17 @@ public final class HTTPClient: Sendable { ] ) - if #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *), - case .strategy = self.configuration.redirectConfiguration.mode + if case .strategy = self.configuration.redirectConfiguration.mode, + #unavailable(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0) { - // `.strategy`/`.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` - // and are only wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. + // `.strategy(_:)`/`.custom(_:)` require this same availability floor to construct, so + // this is unreachable in practice -- kept as defense in depth rather than silently + // falling back to "no redirects are followed" (what `RedirectState.init?` returning + // `nil` here would otherwise mean). On an available OS, `.strategy` is now handled by + // `RedirectHandler` alongside `.follow`, driving the strategy through the + // delegate-based path via `RedirectStrategyLegacyBridge.swift`. logger.debug( - "`.strategy` redirect configuration is not supported by the delegate-based execute API, failing request" + "`.strategy` redirect configuration requires a newer OS than this process is running on, failing request" ) return Task.failedTask( eventLoop: taskEL, @@ -1572,6 +1576,7 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { case invalidDNSOverridesConfiguration case invalidLocalAddress case invalidProxyConfiguration + case redirectStrategyBodyNotSupported case internalStateFailure(file: String, line: UInt) } @@ -1669,6 +1674,9 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { return "Invalid local address" case .invalidProxyConfiguration: return "The proxy configuration is not valid" + case .redirectStrategyBodyNotSupported: + return + "A redirect strategy returned a streaming request body over the delegate-based execute API, which can't drain it at redirect-decision time. Return a `.bytes`/`.byteBuffer` body, reuse `context.redirectRequest` unchanged, or use the Concurrency `execute(_:deadline:logger:)` API instead." case .internalStateFailure(let file, let line): return "An internal state failure has occurred (File: \(file), line: \(line)). Please open an issue with a reproducer if possible" @@ -1778,6 +1786,10 @@ public struct HTTPClientError: Error, Equatable, CustomStringConvertible { /// The proxy configuration is not valid. public static let invalidProxyConfiguration = HTTPClientError(code: .invalidProxyConfiguration) + /// A ``HTTPClientRedirectStrategy`` returned a request body the delegate-based + /// `execute(request:delegate:...)` API can't drain at redirect-decision time. + public static let redirectStrategyBodyNotSupported = HTTPClientError(code: .redirectStrategyBodyNotSupported) + /// A state machine has reached an unsupported state, that wasn't considered when implementing. public static func internalStateFailure(file: String = #fileID, line: UInt = #line) -> HTTPClientError { HTTPClientError(code: .internalStateFailure(file: file, line: line)) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 432563d15..be4cdd1b0 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -1068,13 +1068,27 @@ internal struct RedirectHandler { } func redirect( + head: HTTPResponseHead, + to redirectURL: URL, + promise: EventLoopPromise + ) -> HTTPClient.Task? { + switch self.redirectState { + case .follow(let followState): + return self.redirectByFollowing(followState, status: head.status, to: redirectURL, promise: promise) + case .strategy(let anyStrategyState): + return self.redirectByStrategy(anyStrategyState, head: head, to: redirectURL, promise: promise) + } + } + + private func redirectByFollowing( + _ followState: RedirectState.Follow, status: HTTPResponseStatus, to redirectURL: URL, promise: EventLoopPromise ) -> HTTPClient.Task? { do { - var redirectState = self.redirectState - try redirectState.redirect(to: redirectURL.absoluteString) + var followState = followState + try followState.redirect(to: redirectURL.absoluteString) let (method, headers, body) = transformRequestForRedirect( from: request.url, @@ -1083,7 +1097,7 @@ internal struct RedirectHandler { body: self.request.body, to: redirectURL, status: status, - config: self.redirectState.config + config: followState.config ) let newRequest = try HTTPClient.Request( @@ -1093,20 +1107,120 @@ internal struct RedirectHandler { body: body ) - let newTask = self.execute(newRequest, redirectState) + return self.launch(newRequest, .follow(followState), promise: promise) + } catch { + promise.fail(error) + return nil + } + } - newTask.futureResult.whenComplete { result in - promise.futureResult.eventLoop.execute { - promise.completeWith(result) - } - } + /// Drives a `.strategy` redirect configuration through the delegate-based path, mirroring + /// `HTTPClient.executeAndFollowRedirectsIfNeeded(_:deadline:logger:redirectMode:)`'s own + /// `.strategy` case (`HTTPClient+execute.swift`) as closely as the two paths' different + /// request/response types allow -- see `RedirectStrategyLegacyBridge.swift` for the + /// conversion between them. + /// + /// `.doNotFollow` is not supported here: honoring it would mean resuming normal response + /// delivery (`didReceiveHead`/`didReceiveBodyPart`/`didFinishRequest`) for a response the + /// state machine already committed to treating as a redirect candidate, which the + /// delegate-based path's state machine has no path for -- `.follow`'s own error cases + /// (redirect limit/cycle) fail the whole task the same way, they never resume delivery + /// either. Fails with ``HTTPClientError/invalidRedirectConfiguration`` instead; use the + /// Concurrency `execute(_:deadline:logger:)` API if a strategy needs to decline a redirect + /// and still see the response that triggered it. + private func redirectByStrategy( + _ anyStrategyState: any Sendable, + head: HTTPResponseHead, + to redirectURL: URL, + promise: EventLoopPromise + ) -> HTTPClient.Task? { + guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { + // Unreachable in practice -- see `RedirectState.init?`'s own guard for why. + promise.fail(HTTPClientError.invalidRedirectConfiguration) + return nil + } - return newTask + let strategyState = anyStrategyState as! RedirectState.Strategy + + do { + let (method, headers, body) = transformRequestForRedirect( + from: request.url, + method: self.request.method, + headers: self.request.headers, + body: self.request.body, + to: redirectURL, + status: head.status, + // Matches the Concurrency API's own `.strategy` loop: `max`/`allowCycles` don't + // apply to `.strategy` mode (there's no built-in limit), only the + // `retainHTTPMethodAndBodyOn30{1,2}` flags feed into this rewrite. + config: .init( + max: 0, + allowCycles: true, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ) + + let candidateRequest = try HTTPClient.Request( + url: redirectURL, + method: method, + headers: headers, + body: body, + tlsConfiguration: self.request.tlsConfiguration + ) + + let history = + strategyState.history + [ + HTTPClientRequestResponse( + request: HTTPClientRequest(legacy: self.request), + responseHead: head + ) + ] + + let context = HTTPClientRedirectContext( + redirectRequest: HTTPClientRequest(legacy: candidateRequest), + response: head, + history: history, + redirectCount: strategyState.redirectCount + ) + + switch try strategyState.strategy.redirectDecision(for: context) { + case .doNotFollow: + promise.fail(HTTPClientError.invalidRedirectConfiguration) + return nil + + case .follow(let newRequest): + let newLegacyRequest = try newRequest.asLegacyRequest() + + let newStrategyState = RedirectState.Strategy( + strategy: strategyState.strategy, + history: history, + redirectCount: strategyState.redirectCount + 1 + ) + + return self.launch(newLegacyRequest, .strategy(newStrategyState), promise: promise) + } } catch { promise.fail(error) return nil } } + + private func launch( + _ newRequest: HTTPClient.Request, + _ newRedirectState: RedirectState, + promise: EventLoopPromise + ) -> HTTPClient.Task { + let newTask = self.execute(newRequest, newRedirectState) + + newTask.futureResult.whenComplete { result in + promise.futureResult.eventLoop.execute { + promise.completeWith(result) + } + } + + return newTask + } } extension RequestBodyLength { diff --git a/Sources/AsyncHTTPClient/RedirectState.swift b/Sources/AsyncHTTPClient/RedirectState.swift index 466b4af6a..5490aa0eb 100644 --- a/Sources/AsyncHTTPClient/RedirectState.swift +++ b/Sources/AsyncHTTPClient/RedirectState.swift @@ -53,11 +53,17 @@ extension HTTPClient.Configuration.RedirectConfiguration.Mode: Hashable { } } -struct RedirectState { - var config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration - - /// All visited URLs. - private var visited: [String] +/// Tracks how the delegate-based `execute(request:delegate:...)` API should handle the next +/// redirect for one logical request, for whichever mode `HTTPClient.Configuration +/// .RedirectConfiguration.Mode` was configured with. +enum RedirectState { + case follow(Follow) + + /// Always a `Strategy` value underneath -- type-erased as `any Sendable` for the same reason + /// `Mode.strategy` itself is: `Strategy` is only available on OSes new enough for Swift + /// Concurrency, and Swift disallows `@available` on an enum case with an associated value. See + /// `Mode.strategy`'s own doc comment. + case strategy(any Sendable) } extension RedirectState { @@ -71,30 +77,66 @@ extension RedirectState { switch configuration { case .disallow: return nil + case .follow(let config): - self.init(config: config, visited: [initialURL]) - case .strategy: - // `.strategy` redirects are handled entirely by the caller-supplied strategy; there is no - // count/cycle state for `RedirectState` to track. - return nil + self = .follow(Follow(config: config, visited: [initialURL])) + + case .strategy(let anyStrategy): + guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { + // `.strategy(_:)`/`.custom(_:)` require this same availability floor to + // construct, so this is unreachable in practice. Falling back to "don't track + // redirect state" would silently degrade to "no redirects are ever followed" + // instead -- `_execute`'s own availability guard is what actually surfaces this + // as `.invalidRedirectConfiguration` rather than a silent behavior change. + return nil + } + + let strategy = anyStrategy as! any HTTPClientRedirectStrategy + self = .strategy(Strategy(strategy: strategy, history: [], redirectCount: 0)) } } } extension RedirectState { - /// Call this method when you are about to do a redirect to the given `redirectURL`. - /// This method records that URL into `self`. - /// - Parameter redirectURL: the new URL to redirect the request to - /// - Throws: if it reaches the redirect limit or detects a redirect cycle if and `allowCycles` is false - mutating func redirect(to redirectURL: String) throws { - guard self.visited.count <= config.max else { - throw HTTPClientError.redirectLimitReached + struct Follow: Sendable { + var config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration + + /// All visited URLs. + private var visited: [String] + + fileprivate init(config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration, visited: [String]) + { + self.config = config + self.visited = visited } - guard config.allowCycles || !self.visited.contains(redirectURL) else { - throw HTTPClientError.redirectCycleDetected + /// Call this method when you are about to do a redirect to the given `redirectURL`. + /// This method records that URL into `self`. + /// - Parameter redirectURL: the new URL to redirect the request to + /// - Throws: if it reaches the redirect limit or detects a redirect cycle if and `allowCycles` is false + mutating func redirect(to redirectURL: String) throws { + guard self.visited.count <= config.max else { + throw HTTPClientError.redirectLimitReached + } + + guard config.allowCycles || !self.visited.contains(redirectURL) else { + throw HTTPClientError.redirectCycleDetected + } + self.visited.append(redirectURL) } - self.visited.append(redirectURL) + } +} + +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +extension RedirectState { + struct Strategy: Sendable { + let strategy: any HTTPClientRedirectStrategy + + /// Every request/response pair sent so far for this logical request, oldest first. + var history: [HTTPClientRequestResponse] + + /// How many redirects have already been followed for this logical request. + var redirectCount: Int } } diff --git a/Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift b/Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift new file mode 100644 index 000000000..b248b80bd --- /dev/null +++ b/Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift @@ -0,0 +1,92 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2026 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +import NIOCore + +#if canImport(FoundationEssentials) +import struct FoundationEssentials.URL +#else +import struct Foundation.URL +#endif + +/// Converts between the legacy `HTTPClient.Request`/`HTTPClient.Body` the delegate-based +/// `execute(request:delegate:...)` API is built on, and the `HTTPClientRequest`/`HTTPClientRequest +/// .Body` a ``HTTPClientRedirectStrategy`` is offered and returns -- so a `.strategy` redirect +/// configuration, which is only natively wired up for the Swift Concurrency `execute(_:deadline: +/// logger:)` family, can also drive the delegate-based path's own redirect following (see +/// `RedirectHandler` in `HTTPHandler.swift`). +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +extension HTTPClientRequest { + /// Builds the request a strategy is offered from the legacy request the delegate-based path + /// was about to follow a redirect with. + /// + /// `localAddress` has no equivalent field on `HTTPClient.Request`, so a strategy that sets it + /// on the request it returns has no effect when reissued through `asLegacyRequest()` -- there + /// is nowhere on the legacy side to put it. + init(legacy request: HTTPClient.Request) { + self.init(url: request.url.absoluteString) + self.method = request.method + self.headers = request.headers + self.body = request.body.map { HTTPClientRequest.Body(.legacyBody($0)) } + self.tlsConfiguration = request.tlsConfiguration + } + + /// Reissues a strategy's `.follow(_:)` decision through the legacy, delegate-based path. + /// + /// - Throws: Whatever `HTTPClient.Request.init(url:method:headers:body:tlsConfiguration:)` + /// throws for an invalid `url`, or ``HTTPClientError/redirectStrategyBodyNotSupported`` if + /// `body` is a streaming representation (`.stream`/`AsyncSequence`-backed, or the unstable + /// upload-writer body) that didn't originate from `init(legacy:)` -- those can only be drained + /// by actually opening a connection and writing to it, which nothing at redirect-decision + /// time is in a position to do. A strategy that only returns `context.redirectRequest` + /// unchanged, or replaces its body with `.bytes`/`.byteBuffer`, never hits this. + func asLegacyRequest() throws -> HTTPClient.Request { + try HTTPClient.Request( + url: self.url, + method: self.method, + headers: self.headers, + body: self.body.map { try $0.asLegacyBody() }, + tlsConfiguration: self.tlsConfiguration + ) + } +} + +@available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) +extension HTTPClientRequest.Body { + fileprivate func asLegacyBody() throws -> HTTPClient.Body { + switch self.mode { + case .legacyBody(let body): + // The common case: a strategy that didn't touch `redirectRequest.body` at all, so + // this is still the exact body `init(legacy:)` wrapped -- reused verbatim, no + // re-encoding, no loss of whatever streaming behavior it originally had. + return body + + case .byteBuffer(let buffer): + return .byteBuffer(buffer) + + case .sequence(_, _, let makeCompleteBody): + // `makeCompleteBody` is a plain synchronous closure (unlike `.asyncSequence`), so + // this is a lossless, non-streaming drain -- not an approximation. + return .byteBuffer(makeCompleteBody(ByteBufferAllocator())) + + case .asyncSequence: + throw HTTPClientError.redirectStrategyBodyNotSupported + + #if UnstableHTTPAPIsSupport + case .httpClientRequestBody: + throw HTTPClientError.redirectStrategyBodyNotSupported + #endif + } + } +} diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index 1122aa8e0..5b5da966b 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -294,7 +294,7 @@ final class RequestBag: Sendabl case .redirect(let executor, let handler, let head, let newURL): self.loopBoundState.value.redirectTask = handler.redirect( - status: head.status, + head: head, to: newURL, promise: self.task.promise ) @@ -320,7 +320,7 @@ final class RequestBag: Sendabl case .redirect(let executor, let handler, let head, let newURL): self.loopBoundState.value.redirectTask = handler.redirect( - status: head.status, + head: head, to: newURL, promise: self.task.promise ) @@ -359,7 +359,7 @@ final class RequestBag: Sendabl case .redirect(let handler, let head, let newURL): self.loopBoundState.value.redirectTask = handler.redirect( - status: head.status, + head: head, to: newURL, promise: self.task.promise ) diff --git a/Sources/AsyncHTTPClient/TracingSupport.swift b/Sources/AsyncHTTPClient/TracingSupport.swift index ca7cae82c..24240d6a4 100644 --- a/Sources/AsyncHTTPClient/TracingSupport.swift +++ b/Sources/AsyncHTTPClient/TracingSupport.swift @@ -42,6 +42,8 @@ struct TracingSupport { Int(length) case .byteBuffer(let byteBuffer): byteBuffer.readableBytes + case .legacyBody(let legacyBody): + legacyBody.contentLength.map(Int.init) case .asyncSequence(.unknown, _), .sequence(.unknown, _, _), nil: nil #if UnstableHTTPAPIsSupport diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 5ae14994e..f80f17b61 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -448,11 +448,12 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertEqual("1234", data.data) } - func testCustomRedirectConfigurationFailsWithDelegateBasedExecute() throws { - // `.custom` redirect handlers operate on `HTTPClientRequest`/`HTTPClientResponse` and are only - // wired up for the Swift Concurrency `execute(_:deadline:logger:)` family of APIs; the - // delegate-based `execute(request:delegate:...)` API (exercised here via `.get`) should fail - // fast rather than silently ignore the configured handler. + func testCustomRedirectConfigurationDoesNotAffectNonRedirectingRequestsOverDelegateBasedExecute() throws { + // `.custom`/`.strategy` only kick in once the delegate-based `execute(request:delegate:...)` + // API (exercised here via `.get`) actually encounters a redirect-eligible response -- + // configuring one doesn't reject every request outright, matching the Concurrency + // `execute(_:deadline:logger:)` API, which never consults its redirect mode for a request + // that never redirects either. let localClient = HTTPClient( eventLoopGroupProvider: .shared(self.clientGroup), configuration: HTTPClient.Configuration( @@ -461,11 +462,63 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { ) defer { XCTAssertNoThrow(try localClient.syncShutdown()) } - XCTAssertThrowsError(try localClient.get(url: self.defaultHTTPBinURLPrefix + "ok").wait()) { + let response = try localClient.get(url: self.defaultHTTPBinURLPrefix + "ok").wait() + XCTAssertEqual(response.status, .ok) + } + + func testCustomRedirectHandlerCanRefuseRedirectOverDelegateBasedExecute() throws { + // Resuming normal delivery of the response that triggered the redirect (returning it + // as-is, matching what `.doNotFollow` does for the Concurrency API) isn't supported by + // the delegate-based path's response-delivery state machine, which -- once it treats a + // response as a redirect candidate -- otherwise only ever redirects or fails the whole + // task, the same as `.follow`'s own redirect-limit/cycle errors. So `.doNotFollow` fails + // the task instead. + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .custom { _ in .doNotFollow } + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + XCTAssertThrowsError( + try localClient.get(url: "http://localhost:\(self.defaultHTTPBin.port)/redirect/302").wait() + ) { XCTAssertEqual($0 as? HTTPClientError, .invalidRedirectConfiguration) } } + func testCustomRedirectHandlerCanRewriteRedirectRequestOverDelegateBasedExecute() throws { + let port = self.defaultHTTPBin.port + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .custom { context in + XCTAssertEqual(context.response.status, .found) + XCTAssertEqual(context.redirectCount, 0) + XCTAssertEqual(context.history.count, 1) + // The candidate request has already gone through the standard rewrite rules: + // it should already point at the `Location` from the /redirect/302 response. + XCTAssertEqual(context.redirectRequest.url, "http://localhost:\(port)/ok") + + var rewritten = context.redirectRequest + rewritten.url = "http://localhost:\(port)/echo-uri" + return .follow(rewritten) + } + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + let response = try localClient.get(url: "http://localhost:\(port)/redirect/302").wait() + + XCTAssertEqual(response.status, .ok) + XCTAssertEqual(response.headers.first(name: "X-Calling-URI"), "/echo-uri") + XCTAssertEqual( + response.history.map(\.request.url.absoluteString), + ["http://localhost:\(port)/redirect/302", "http://localhost:\(port)/echo-uri"] + ) + } + func testHttpRedirect() throws { let httpsBin = HTTPBin(.http1_1(ssl: true)) let localClient = HTTPClient( From 897cf09adb4edf71ea55bbafda91d9a6b5f3fc31 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:25:56 -0300 Subject: [PATCH 6/7] Rename "legacy" -> "delegate" in the redirect-strategy bridge AsyncHTTPClient doesn't call HTTPClient.Request/execute(request:delegate:...) "legacy" anywhere -- it's the older of the two execute APIs, but it isn't deprecated and isn't being phased out, so labeling it that way in the new bridge code (file name, the Body.Mode case, doc comments) was misleading. Renamed throughout to name it by what it actually is: the delegate-based API, as opposed to the Concurrency one. Co-Authored-By: Claude Sonnet 5 --- .../HTTPClientRequest+Prepared.swift | 12 +++--- .../AsyncAwait/HTTPClientRequest.swift | 22 +++++------ Sources/AsyncHTTPClient/HTTPClient.swift | 2 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 10 ++--- ...t => RedirectStrategyDelegateBridge.swift} | 37 ++++++++++--------- Sources/AsyncHTTPClient/TracingSupport.swift | 4 +- 6 files changed, 44 insertions(+), 43 deletions(-) rename Sources/AsyncHTTPClient/{RedirectStrategyLegacyBridge.swift => RedirectStrategyDelegateBridge.swift} (69%) diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift index 518c5873a..7fad583e9 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift @@ -114,13 +114,13 @@ extension HTTPClientRequest.Prepared.Body { ) case .byteBuffer(let byteBuffer): self = .byteBuffer(byteBuffer) - case .legacyBody: + case .delegateBody: // Only ever produced by the delegate-based redirect-strategy bridge - // (`RedirectStrategyLegacyBridge.swift`), which converts it back into a - // `HTTPClient.Body` (`asLegacyBody()`) rather than routing it through the + // (`RedirectStrategyDelegateBridge.swift`), which converts it back into a + // `HTTPClient.Body` (`asDelegateBody()`) rather than routing it through the // Concurrency API's own request preparation. fatalError( - "`.legacyBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asLegacyBody()` instead." + "`.delegateBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asDelegateBody()` instead." ) #if UnstableHTTPAPIsSupport case .httpClientRequestBody(let length, let requestBody): @@ -140,9 +140,9 @@ extension RequestBodyLength { self = .known(Int64(buffer.readableBytes)) case .sequence(let length, _, _), .asyncSequence(let length, _): self = length - case .legacyBody: + case .delegateBody: fatalError( - "`.legacyBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asLegacyBody()` instead." + "`.delegateBody` never reaches `HTTPClientRequest.Prepared` -- it is unwrapped via `asDelegateBody()` instead." ) #if UnstableHTTPAPIsSupport case .httpClientRequestBody(let length, _): diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift index c8832e68e..d6ee28cf0 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest.swift @@ -101,16 +101,16 @@ extension HTTPClientRequest { ) case byteBuffer(ByteBuffer) - /// Wraps a legacy ``HTTPClient/Body`` verbatim, untouched. + /// Wraps a delegate-based-API ``HTTPClient/Body`` verbatim, untouched. /// /// Exists solely so the delegate-based `execute(request:delegate:...)` API can offer a /// redirect-eligible request's body to a ``HTTPClientRedirectStrategy`` (which is typed - /// in terms of this Concurrency-API `Body`, not the legacy one) without re-encoding it -- - /// see `RedirectStrategyLegacyBridge.swift`. `canBeConsumedMultipleTimes` is conservatively - /// `false`: a legacy `Body`'s `stream` closure isn't guaranteed replayable. Never produced - /// by any public factory, and never asked to iterate -- it is unwrapped back into a - /// `HTTPClient.Body` (`asLegacyBody()`) before it would ever need to stream. - case legacyBody(HTTPClient.Body) + /// in terms of this Concurrency-API `Body`, not that one) without re-encoding it -- see + /// `RedirectStrategyDelegateBridge.swift`. `canBeConsumedMultipleTimes` is conservatively + /// `false`: a delegate-API `Body`'s `stream` closure isn't guaranteed replayable. Never + /// produced by any public factory, and never asked to iterate -- it is unwrapped back + /// into a `HTTPClient.Body` (`asDelegateBody()`) before it would ever need to stream. + case delegateBody(HTTPClient.Body) #if UnstableHTTPAPIsSupport case httpClientRequestBody( @@ -409,7 +409,7 @@ extension Optional where Wrapped == HTTPClientRequest.Body { case .byteBuffer: return true case .sequence(_, let canBeConsumedMultipleTimes, _): return canBeConsumedMultipleTimes case .asyncSequence: return false - case .legacyBody: return false + case .delegateBody: return false #if UnstableHTTPAPIsSupport case .httpClientRequestBody: return false // TODO: I think this should be TRUE #endif @@ -453,11 +453,11 @@ extension HTTPClientRequest.Body: AsyncSequence { return .init(storage: .byteBuffer(makeCompleteBody(AsyncIterator.allocator))) case .byteBuffer(let byteBuffer): return .init(storage: .byteBuffer(byteBuffer)) - case .legacyBody: + case .delegateBody: // Only ever produced by the delegate-based redirect-strategy bridge, which unwraps - // it back into a `HTTPClient.Body` (`asLegacyBody()`) instead of ever asking this + // it back into a `HTTPClient.Body` (`asDelegateBody()`) instead of ever asking this // AsyncSequence conformance to iterate it. - fatalError("`.legacyBody` is never iterated -- it is unwrapped via `asLegacyBody()` instead.") + fatalError("`.delegateBody` is never iterated -- it is unwrapped via `asDelegateBody()` instead.") #if UnstableHTTPAPIsSupport case .httpClientRequestBody: fatalError("Unimplemented") diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 3037103ed..50277d514 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -767,7 +767,7 @@ public final class HTTPClient: Sendable { // falling back to "no redirects are followed" (what `RedirectState.init?` returning // `nil` here would otherwise mean). On an available OS, `.strategy` is now handled by // `RedirectHandler` alongside `.follow`, driving the strategy through the - // delegate-based path via `RedirectStrategyLegacyBridge.swift`. + // delegate-based path via `RedirectStrategyDelegateBridge.swift`. logger.debug( "`.strategy` redirect configuration requires a newer OS than this process is running on, failing request" ) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index be4cdd1b0..322ae719e 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -1117,7 +1117,7 @@ internal struct RedirectHandler { /// Drives a `.strategy` redirect configuration through the delegate-based path, mirroring /// `HTTPClient.executeAndFollowRedirectsIfNeeded(_:deadline:logger:redirectMode:)`'s own /// `.strategy` case (`HTTPClient+execute.swift`) as closely as the two paths' different - /// request/response types allow -- see `RedirectStrategyLegacyBridge.swift` for the + /// request/response types allow -- see `RedirectStrategyDelegateBridge.swift` for the /// conversion between them. /// /// `.doNotFollow` is not supported here: honoring it would mean resuming normal response @@ -1172,13 +1172,13 @@ internal struct RedirectHandler { let history = strategyState.history + [ HTTPClientRequestResponse( - request: HTTPClientRequest(legacy: self.request), + request: HTTPClientRequest(delegateRequest: self.request), responseHead: head ) ] let context = HTTPClientRedirectContext( - redirectRequest: HTTPClientRequest(legacy: candidateRequest), + redirectRequest: HTTPClientRequest(delegateRequest: candidateRequest), response: head, history: history, redirectCount: strategyState.redirectCount @@ -1190,7 +1190,7 @@ internal struct RedirectHandler { return nil case .follow(let newRequest): - let newLegacyRequest = try newRequest.asLegacyRequest() + let newDelegateRequest = try newRequest.asDelegateRequest() let newStrategyState = RedirectState.Strategy( strategy: strategyState.strategy, @@ -1198,7 +1198,7 @@ internal struct RedirectHandler { redirectCount: strategyState.redirectCount + 1 ) - return self.launch(newLegacyRequest, .strategy(newStrategyState), promise: promise) + return self.launch(newDelegateRequest, .strategy(newStrategyState), promise: promise) } } catch { promise.fail(error) diff --git a/Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift b/Sources/AsyncHTTPClient/RedirectStrategyDelegateBridge.swift similarity index 69% rename from Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift rename to Sources/AsyncHTTPClient/RedirectStrategyDelegateBridge.swift index b248b80bd..ddcb34cea 100644 --- a/Sources/AsyncHTTPClient/RedirectStrategyLegacyBridge.swift +++ b/Sources/AsyncHTTPClient/RedirectStrategyDelegateBridge.swift @@ -20,7 +20,7 @@ import struct FoundationEssentials.URL import struct Foundation.URL #endif -/// Converts between the legacy `HTTPClient.Request`/`HTTPClient.Body` the delegate-based +/// Converts between the `HTTPClient.Request`/`HTTPClient.Body` the delegate-based /// `execute(request:delegate:...)` API is built on, and the `HTTPClientRequest`/`HTTPClientRequest /// .Body` a ``HTTPClientRedirectStrategy`` is offered and returns -- so a `.strategy` redirect /// configuration, which is only natively wired up for the Swift Concurrency `execute(_:deadline: @@ -28,35 +28,36 @@ import struct Foundation.URL /// `RedirectHandler` in `HTTPHandler.swift`). @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) extension HTTPClientRequest { - /// Builds the request a strategy is offered from the legacy request the delegate-based path - /// was about to follow a redirect with. + /// Builds the request a strategy is offered from the `HTTPClient.Request` the delegate-based + /// path was about to follow a redirect with. /// /// `localAddress` has no equivalent field on `HTTPClient.Request`, so a strategy that sets it - /// on the request it returns has no effect when reissued through `asLegacyRequest()` -- there - /// is nowhere on the legacy side to put it. - init(legacy request: HTTPClient.Request) { + /// on the request it returns has no effect when reissued through `asDelegateRequest()` -- + /// there is nowhere on that side to put it. + init(delegateRequest request: HTTPClient.Request) { self.init(url: request.url.absoluteString) self.method = request.method self.headers = request.headers - self.body = request.body.map { HTTPClientRequest.Body(.legacyBody($0)) } + self.body = request.body.map { HTTPClientRequest.Body(.delegateBody($0)) } self.tlsConfiguration = request.tlsConfiguration } - /// Reissues a strategy's `.follow(_:)` decision through the legacy, delegate-based path. + /// Reissues a strategy's `.follow(_:)` decision through the delegate-based path. /// /// - Throws: Whatever `HTTPClient.Request.init(url:method:headers:body:tlsConfiguration:)` /// throws for an invalid `url`, or ``HTTPClientError/redirectStrategyBodyNotSupported`` if /// `body` is a streaming representation (`.stream`/`AsyncSequence`-backed, or the unstable - /// upload-writer body) that didn't originate from `init(legacy:)` -- those can only be drained - /// by actually opening a connection and writing to it, which nothing at redirect-decision - /// time is in a position to do. A strategy that only returns `context.redirectRequest` - /// unchanged, or replaces its body with `.bytes`/`.byteBuffer`, never hits this. - func asLegacyRequest() throws -> HTTPClient.Request { + /// upload-writer body) that didn't originate from `init(delegateRequest:)` -- those can only + /// be drained by actually opening a connection and writing to it, which nothing at + /// redirect-decision time is in a position to do. A strategy that only returns + /// `context.redirectRequest` unchanged, or replaces its body with `.bytes`/`.byteBuffer`, + /// never hits this. + func asDelegateRequest() throws -> HTTPClient.Request { try HTTPClient.Request( url: self.url, method: self.method, headers: self.headers, - body: self.body.map { try $0.asLegacyBody() }, + body: self.body.map { try $0.asDelegateBody() }, tlsConfiguration: self.tlsConfiguration ) } @@ -64,12 +65,12 @@ extension HTTPClientRequest { @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) extension HTTPClientRequest.Body { - fileprivate func asLegacyBody() throws -> HTTPClient.Body { + fileprivate func asDelegateBody() throws -> HTTPClient.Body { switch self.mode { - case .legacyBody(let body): + case .delegateBody(let body): // The common case: a strategy that didn't touch `redirectRequest.body` at all, so - // this is still the exact body `init(legacy:)` wrapped -- reused verbatim, no - // re-encoding, no loss of whatever streaming behavior it originally had. + // this is still the exact body `init(delegateRequest:)` wrapped -- reused verbatim, + // no re-encoding, no loss of whatever streaming behavior it originally had. return body case .byteBuffer(let buffer): diff --git a/Sources/AsyncHTTPClient/TracingSupport.swift b/Sources/AsyncHTTPClient/TracingSupport.swift index 24240d6a4..07396372a 100644 --- a/Sources/AsyncHTTPClient/TracingSupport.swift +++ b/Sources/AsyncHTTPClient/TracingSupport.swift @@ -42,8 +42,8 @@ struct TracingSupport { Int(length) case .byteBuffer(let byteBuffer): byteBuffer.readableBytes - case .legacyBody(let legacyBody): - legacyBody.contentLength.map(Int.init) + case .delegateBody(let delegateBody): + delegateBody.contentLength.map(Int.init) case .asyncSequence(.unknown, _), .sequence(.unknown, _, _), nil: nil #if UnstableHTTPAPIsSupport From 1e83a6e2e7a6344e9ee4fa8416badbb326778d35 Mon Sep 17 00:00:00 2001 From: brennobemoura <37243584+brennobemoura@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:05:38 -0300 Subject: [PATCH 7/7] Make .doNotFollow work on the delegate-based execute API too Previously, a .strategy redirect configuration deciding .doNotFollow on the delegate-based execute(request:delegate:...) API always failed the task instead of delivering the response that triggered the redirect -- the state machine (RequestBag+StateMachine.swift) decided whether a response was a redirect candidate, and either discarded its body after counting (<=3KB) or cancelled the connection before reading any of it (known >3KB), before the strategy's own decision was ever computed. RedirectHandler.earlyStrategyDecision(head:) now asks the strategy the moment the response head arrives, before any body byte is touched -- HTTPClientRedirectContext never carries a body, so nothing about it is needed at that point. `.doNotFollow` then simply proceeds like any ordinary, non-redirect-candidate response, regardless of size. A `.follow(_:)` decision is carried on a copy of the handler (`precomputed`) into the existing size-based buffer-or-cancel mechanism, which still reuses the HTTP/1.1 connection for small bodies exactly as before -- this only changes *when* the strategy is asked, not what happens once it decides to follow. The strategy call had to move outside receiveResponseHead's own mutating access to the state machine: it's arbitrary caller code that can synchronously reenter the task (e.g. call task.cancel()), which would otherwise overlap two accesses to the same storage and trap under Swift's exclusivity enforcement. RequestBag.receiveResponseHead0 now computes the decision first and hands the state machine only the answer. Co-Authored-By: Claude Sonnet 5 --- Sources/AsyncHTTPClient/HTTPClient.swift | 10 +- Sources/AsyncHTTPClient/HTTPHandler.swift | 193 ++++++++++++------ .../RequestBag+StateMachine.swift | 99 +++++++-- Sources/AsyncHTTPClient/RequestBag.swift | 10 +- .../HTTPClientTestUtils.swift | 13 ++ .../HTTPClientTests.swift | 71 ++++++- .../RequestBagTests.swift | 155 ++++++++++++++ 7 files changed, 448 insertions(+), 103 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 50277d514..0bcca2fca 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -1431,9 +1431,13 @@ extension HTTPClient.Configuration { /// /// - warning: There is no built-in redirect-count or cycle limit for this mode — use the /// `redirectCount`/`history` passed to the strategy to enforce your own policy. - /// - note: Only supported by the Swift Concurrency `execute(_:deadline:logger:)` family of APIs. - /// Using `.strategy`/`.custom` with the delegate-based `execute(request:delegate:...)` API - /// fails with ``HTTPClientError/invalidRedirectConfiguration``. + /// - note: Supported by both the Swift Concurrency `execute(_:deadline:logger:)` family and + /// the delegate-based `execute(request:delegate:...)` API. On the latter, `.follow(_:)` + /// reissues through the delegate API's own request/body types, and a body your strategy + /// replaces with a new streaming (`.stream`/`AsyncSequence`-backed) representation throws + /// ``HTTPClientError/redirectStrategyBodyNotSupported`` -- nothing at redirect-decision + /// time is in a position to drain one. `context.redirectRequest` unchanged, or a + /// `.bytes`/`.byteBuffer` replacement, works either way. @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) public static func strategy(_ strategy: any HTTPClientRedirectStrategy) -> RedirectConfiguration { .init(configuration: .strategy(strategy)) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 322ae719e..8a9d3f784 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -1059,6 +1059,18 @@ internal struct RedirectHandler { let redirectState: RedirectState let execute: (HTTPClient.Request, RedirectState) -> HTTPClient.Task + /// Set only on a copy returned from `earlyStrategyDecision(head:)`, carrying what that + /// already decided so `redirect(head:to:promise:)` -- called later, once the state machine + /// gets around to it -- reissues/fails accordingly instead of asking the strategy again. + /// `.follow` mode never sets this; its decision (redirect-limit/cycle check) has nothing to + /// precompute and stays exactly as cheap to make late as early. + var precomputed: Precomputed? + + enum Precomputed { + case launch(HTTPClient.Request, RedirectState) + case fail(Error) + } + func redirectTarget(status: HTTPResponseStatus, responseHeaders: HTTPHeaders) -> URL? { responseHeaders.extractRedirectTarget( status: status, @@ -1067,77 +1079,48 @@ internal struct RedirectHandler { ) } - func redirect( - head: HTTPResponseHead, - to redirectURL: URL, - promise: EventLoopPromise - ) -> HTTPClient.Task? { - switch self.redirectState { - case .follow(let followState): - return self.redirectByFollowing(followState, status: head.status, to: redirectURL, promise: promise) - case .strategy(let anyStrategyState): - return self.redirectByStrategy(anyStrategyState, head: head, to: redirectURL, promise: promise) - } + /// What `earlyStrategyDecision(head:)` decided, so `receiveResponseHead` (`RequestBag + /// +StateMachine.swift`) can act on it without re-deriving anything. + enum EarlyStrategyDecision { + /// `.follow` mode, or `head` isn't redirect-eligible at all -- proceed exactly as + /// before this existed. + case notApplicable + /// The response that triggered this should be delivered as the final response, same as + /// any ordinary (non-redirect-candidate) response -- nothing about its body has been + /// touched yet, so this works regardless of size. + case doNotFollow + /// Either what to launch, or what error to fail with -- both already computed, both + /// requiring the same redirect-eligible response's body to be drained-or-cancelled the + /// same way `.follow` does, hence going through the same `redirectURL`-tagged path. + case decided(RedirectHandler, URL) } - private func redirectByFollowing( - _ followState: RedirectState.Follow, - status: HTTPResponseStatus, - to redirectURL: URL, - promise: EventLoopPromise - ) -> HTTPClient.Task? { - do { - var followState = followState - try followState.redirect(to: redirectURL.absoluteString) - - let (method, headers, body) = transformRequestForRedirect( - from: request.url, - method: self.request.method, - headers: self.request.headers, - body: self.request.body, - to: redirectURL, - status: status, - config: followState.config - ) - - let newRequest = try HTTPClient.Request( - url: redirectURL, - method: method, - headers: headers, - body: body - ) + /// Computes a `.strategy` redirect configuration's decision for `head`, before any response + /// body byte has been read. Safe to call regardless of body size or whether it's even known + /// (`Content-Length` present, absent, or huge) -- `HTTPClientRedirectContext` never carries + /// a body, so nothing here needs one. (Adding a body field to that type later would break + /// this assumption.) + /// + /// - Important: `strategy.redirectDecision(for:)` is arbitrary caller code that may + /// synchronously reenter this task (e.g. call `task.cancel()`). Call this from outside any + /// exclusive access to the state machine that will go on to consume the result -- + /// `RequestBag.receiveResponseHead0` calls this first and hands the state machine only the + /// already-computed answer, never the handler itself, for exactly this reason. + func earlyStrategyDecision(head: HTTPResponseHead) -> EarlyStrategyDecision { + guard case .strategy(let anyStrategyState) = self.redirectState else { + return .notApplicable + } - return self.launch(newRequest, .follow(followState), promise: promise) - } catch { - promise.fail(error) - return nil + guard let redirectURL = self.redirectTarget(status: head.status, responseHeaders: head.headers) else { + return .notApplicable } - } - /// Drives a `.strategy` redirect configuration through the delegate-based path, mirroring - /// `HTTPClient.executeAndFollowRedirectsIfNeeded(_:deadline:logger:redirectMode:)`'s own - /// `.strategy` case (`HTTPClient+execute.swift`) as closely as the two paths' different - /// request/response types allow -- see `RedirectStrategyDelegateBridge.swift` for the - /// conversion between them. - /// - /// `.doNotFollow` is not supported here: honoring it would mean resuming normal response - /// delivery (`didReceiveHead`/`didReceiveBodyPart`/`didFinishRequest`) for a response the - /// state machine already committed to treating as a redirect candidate, which the - /// delegate-based path's state machine has no path for -- `.follow`'s own error cases - /// (redirect limit/cycle) fail the whole task the same way, they never resume delivery - /// either. Fails with ``HTTPClientError/invalidRedirectConfiguration`` instead; use the - /// Concurrency `execute(_:deadline:logger:)` API if a strategy needs to decline a redirect - /// and still see the response that triggered it. - private func redirectByStrategy( - _ anyStrategyState: any Sendable, - head: HTTPResponseHead, - to redirectURL: URL, - promise: EventLoopPromise - ) -> HTTPClient.Task? { guard #available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) else { - // Unreachable in practice -- see `RedirectState.init?`'s own guard for why. - promise.fail(HTTPClientError.invalidRedirectConfiguration) - return nil + // Unreachable in practice -- see `RedirectState.init?`'s own guard for why. Once the + // state machine eventually calls `redirect(head:to:promise:)` on this (unmodified) + // handler, `redirectByStrategy` fails it the same way `_execute`'s own top-level + // guard would have. + return .notApplicable } let strategyState = anyStrategyState as! RedirectState.Strategy @@ -1186,8 +1169,7 @@ internal struct RedirectHandler { switch try strategyState.strategy.redirectDecision(for: context) { case .doNotFollow: - promise.fail(HTTPClientError.invalidRedirectConfiguration) - return nil + return .doNotFollow case .follow(let newRequest): let newDelegateRequest = try newRequest.asDelegateRequest() @@ -1198,14 +1180,89 @@ internal struct RedirectHandler { redirectCount: strategyState.redirectCount + 1 ) - return self.launch(newDelegateRequest, .strategy(newStrategyState), promise: promise) + var decided = self + decided.precomputed = .launch(newDelegateRequest, .strategy(newStrategyState)) + return .decided(decided, redirectURL) } + } catch { + var decided = self + decided.precomputed = .fail(error) + return .decided(decided, redirectURL) + } + } + + /// Reissues or fails according to whatever `earlyStrategyDecision(head:)` already decided + /// (`.strategy` mode), or makes `.follow` mode's decision now, for the first time (there is + /// nothing to precompute there -- the redirect-limit/cycle check is exactly as cheap late as + /// early, and always ends in "redirect" or "fail the task", never "delivered normally"). + func redirect( + head: HTTPResponseHead, + to redirectURL: URL, + promise: EventLoopPromise + ) -> HTTPClient.Task? { + if let precomputed = self.precomputed { + switch precomputed { + case .launch(let newRequest, let newRedirectState): + return self.launch(newRequest, newRedirectState, promise: promise) + case .fail(let error): + promise.fail(error) + return nil + } + } + + switch self.redirectState { + case .follow(let followState): + return self.redirectByFollowing(followState, status: head.status, to: redirectURL, promise: promise) + case .strategy: + return self.redirectByStrategy(promise: promise) + } + } + + private func redirectByFollowing( + _ followState: RedirectState.Follow, + status: HTTPResponseStatus, + to redirectURL: URL, + promise: EventLoopPromise + ) -> HTTPClient.Task? { + do { + var followState = followState + try followState.redirect(to: redirectURL.absoluteString) + + let (method, headers, body) = transformRequestForRedirect( + from: request.url, + method: self.request.method, + headers: self.request.headers, + body: self.request.body, + to: redirectURL, + status: status, + config: followState.config + ) + + let newRequest = try HTTPClient.Request( + url: redirectURL, + method: method, + headers: headers, + body: body + ) + + return self.launch(newRequest, .follow(followState), promise: promise) } catch { promise.fail(error) return nil } } + /// Reached only when `earlyStrategyDecision(head:)` couldn't run the strategy at all -- + /// i.e. an OS below the availability floor `.strategy(_:)`/`.custom(_:)` require to + /// construct in the first place. Unreachable in practice; see `RedirectState.init?`'s own + /// guard for why. Every other path through `.strategy` mode is decided by + /// `earlyStrategyDecision(head:)` at head-received time and carried here via + /// `precomputed`, above, before this method would ever be reached. + private func redirectByStrategy(promise: EventLoopPromise) -> HTTPClient.Task? { + promise.fail(HTTPClientError.invalidRedirectConfiguration) + return nil + } + private func launch( _ newRequest: HTTPClient.Request, _ newRedirectState: RedirectState, diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index 8f5cd37ce..f49734028 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -328,11 +328,32 @@ extension RequestBag.StateMachine { case redirect(HTTPRequestExecutor, RedirectHandler, HTTPResponseHead, URL) } + /// The redirect handler awaiting this request's response head, if any -- for computing a + /// `.strategy` configuration's decision (`RedirectHandler.earlyStrategyDecision(head:)`) + /// *before* calling `receiveResponseHead`, since that call is arbitrary caller code that may + /// reenter this task synchronously, which `receiveResponseHead`'s exclusive access to + /// `self.state` cannot tolerate. + var redirectHandlerAwaitingResponseHead: RedirectHandler? { + guard case .executing(_, _, .initialized(let handler)) = self.state else { + return nil + } + return handler + } + /// The response head has been received. /// - /// - Parameter head: The response' head + /// - Parameters: + /// - head: The response' head + /// - earlyDecision: What `redirectHandlerAwaitingResponseHead?.earlyStrategyDecision(head:)` + /// already decided for a `.strategy` redirect configuration, computed by the caller + /// before this call (see that method's own doc comment for why it can't be computed in + /// here). `.notApplicable` for `.follow` mode, no configured handler, or a non-redirect + /// response -- proceeds exactly as if this parameter didn't exist. /// - Returns: Whether the response should be forwarded to the delegate. Will be `false` if the request follows a redirect. - mutating func receiveResponseHead(_ head: HTTPResponseHead) -> ReceiveResponseHeadAction { + mutating func receiveResponseHead( + _ head: HTTPResponseHead, + earlyDecision: RedirectHandler.EarlyStrategyDecision + ) -> ReceiveResponseHeadAction { switch self.state { case .initialized, .queued, .deadlineExceededWhileQueued: preconditionFailure("How can we receive a response, if the request hasn't started yet.") @@ -341,26 +362,40 @@ extension RequestBag.StateMachine { preconditionFailure("If we receive a response, we must not have received something else before") } - if let redirectHandler = redirectHandler, - let redirectURL = redirectHandler.redirectTarget( - status: head.status, - responseHeaders: head.headers - ) - { - // If we will redirect, we need to consume the response's body ASAP, to be able to - // reuse the existing connection. We will consume a response body, if the body is - // smaller than 3kb. - switch head.contentLength { - case .some(0...(HTTPClient.maxBodySizeRedirectResponse)), .none: - self.state = .redirected(executor, redirectHandler, 0, head, redirectURL) - return .signalBodyDemand(executor) - case .some: - self.state = .finished(error: HTTPClientError.cancelled) - return .redirect(executor, redirectHandler, head, redirectURL) - } - } else { + switch earlyDecision { + case .doNotFollow: + // Nothing about the body has been touched -- deliver this response exactly like + // any ordinary, non-redirect-candidate one, regardless of its size. self.state = .executing(executor, requestState, .buffering(.init(), next: .askExecutorForMore)) return .forwardResponseHead(head) + + case .decided(let decidedHandler, let redirectURL): + return self.redirectOrForward( + executor: executor, + requestState: requestState, + redirectHandler: decidedHandler, + redirectURL: redirectURL, + head: head + ) + + case .notApplicable: + if let redirectHandler = redirectHandler, + let redirectURL = redirectHandler.redirectTarget( + status: head.status, + responseHeaders: head.headers + ) + { + return self.redirectOrForward( + executor: executor, + requestState: requestState, + redirectHandler: redirectHandler, + redirectURL: redirectURL, + head: head + ) + } else { + self.state = .executing(executor, requestState, .buffering(.init(), next: .askExecutorForMore)) + return .forwardResponseHead(head) + } } case .redirected: preconditionFailure("This state can only be reached after we have received a HTTP head") @@ -373,6 +408,30 @@ extension RequestBag.StateMachine { } } + /// Shared by `.follow` mode (which has always decided by the time this is reached) and a + /// `.strategy` mode `.follow(_:)` decision (already computed early, carried on + /// `redirectHandler`): reads up to `HTTPClient.maxBodySizeRedirectResponse` of body to try to + /// reuse the connection, or cancels immediately for a known-larger body. + private mutating func redirectOrForward( + executor: HTTPRequestExecutor, + requestState: RequestStreamState, + redirectHandler: RedirectHandler, + redirectURL: URL, + head: HTTPResponseHead + ) -> ReceiveResponseHeadAction { + // If we will redirect, we need to consume the response's body ASAP, to be able to + // reuse the existing connection. We will consume a response body, if the body is + // smaller than 3kb. + switch head.contentLength { + case .some(0...(HTTPClient.maxBodySizeRedirectResponse)), .none: + self.state = .redirected(executor, redirectHandler, 0, head, redirectURL) + return .signalBodyDemand(executor) + case .some: + self.state = .finished(error: HTTPClientError.cancelled) + return .redirect(executor, redirectHandler, head, redirectURL) + } + } + enum ReceiveResponseBodyAction { case none case forwardResponsePart(ByteBuffer) diff --git a/Sources/AsyncHTTPClient/RequestBag.swift b/Sources/AsyncHTTPClient/RequestBag.swift index 5b5da966b..6d596d5a6 100644 --- a/Sources/AsyncHTTPClient/RequestBag.swift +++ b/Sources/AsyncHTTPClient/RequestBag.swift @@ -284,8 +284,16 @@ final class RequestBag: Sendabl self.delegate.didVisitURL(task: self.task, self.loopBoundState.value.request, head) self.loopBoundState.value.endRequestSpan(response: head) + // Computed here, outside the state machine's exclusive access to its own storage -- + // `earlyStrategyDecision(head:)` runs arbitrary caller code (`redirectDecision(for:)`) + // that may synchronously reenter this task (e.g. `task.cancel()`), which the `mutating + // func receiveResponseHead` call below cannot tolerate overlapping with. + let earlyDecision = + self.loopBoundState.value.state.redirectHandlerAwaitingResponseHead? + .earlyStrategyDecision(head: head) ?? .notApplicable + // runs most likely on channel eventLoop - switch self.loopBoundState.value.state.receiveResponseHead(head) { + switch self.loopBoundState.value.state.receiveResponseHead(head, earlyDecision: earlyDecision) { case .none: break diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index ea80cee6e..770f5e65a 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -1037,6 +1037,19 @@ internal final class HTTPBinHandler: ChannelInboundHandler { headers.add(name: "Location", value: targetURL) self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return + case "/redirect/302-with-body": + // `size` lets a test exercise a redirect-eligible response whose body is + // announced (`Content-Length`) as larger than `HTTPClient + // .maxBodySizeRedirectResponse` (3KB) -- none of the other `/redirect/*` + // endpoints here can produce that, since they're all bodyless. + let size = Int(self.value(for: "size", from: urlComponents.query ?? "")) ?? 4096 + var headers = self.responseHeaders + headers.add(name: "location", value: "/ok") + headers.replaceOrAdd(name: "content-length", value: "\(size)") + var builder = HTTPResponseBuilder(status: .found, headers: headers) + builder.body = ByteBuffer(repeating: UInt8(ascii: "x"), count: size) + self.resps.append(builder) + return case "/percent%20encoded": if req.method != .GET { self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed)) diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index f80f17b61..680e42d18 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -467,12 +467,10 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { } func testCustomRedirectHandlerCanRefuseRedirectOverDelegateBasedExecute() throws { - // Resuming normal delivery of the response that triggered the redirect (returning it - // as-is, matching what `.doNotFollow` does for the Concurrency API) isn't supported by - // the delegate-based path's response-delivery state machine, which -- once it treats a - // response as a redirect candidate -- otherwise only ever redirects or fails the whole - // task, the same as `.follow`'s own redirect-limit/cycle errors. So `.doNotFollow` fails - // the task instead. + // `.doNotFollow` is decided the moment the response head arrives + // (`RedirectHandler.earlyStrategyDecision(head:)`), before any byte of its body has been + // read -- so the 302 itself becomes the final response, delivered exactly like any + // ordinary (non-redirect-candidate) response, same as the Concurrency API. let localClient = HTTPClient( eventLoopGroupProvider: .shared(self.clientGroup), configuration: HTTPClient.Configuration( @@ -481,11 +479,10 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { ) defer { XCTAssertNoThrow(try localClient.syncShutdown()) } - XCTAssertThrowsError( - try localClient.get(url: "http://localhost:\(self.defaultHTTPBin.port)/redirect/302").wait() - ) { - XCTAssertEqual($0 as? HTTPClientError, .invalidRedirectConfiguration) - } + let response = try localClient.get(url: "http://localhost:\(self.defaultHTTPBin.port)/redirect/302").wait() + + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.history.count, 1) } func testCustomRedirectHandlerCanRewriteRedirectRequestOverDelegateBasedExecute() throws { @@ -519,6 +516,58 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { ) } + func testCustomRedirectHandlerCanRefuseRedirectWithLargeBodyOverDelegateBasedExecute() throws { + // End-to-end version of `RequestBagTests + // .testRedirectStrategyDoesNotFollowWithLargeAnnouncedBodyDeliversResponseNormally`: + // `/redirect/302-with-body` announces a body bigger than `HTTPClient + // .maxBodySizeRedirectResponse` (3KB), which used to be cancelled before any byte was + // read, before the strategy even got a chance to decide. + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .custom { _ in .doNotFollow } + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + let response = try localClient.get( + url: "http://localhost:\(self.defaultHTTPBin.port)/redirect/302-with-body?size=8192" + ).wait() + + XCTAssertEqual(response.status, .found) + XCTAssertEqual(response.history.count, 1) + XCTAssertEqual(response.body?.readableBytes, 8192) + } + + func testCustomRedirectHandlerOverDelegateBasedExecuteInvokesStrategyExactlyOncePerRedirect() throws { + // The end-to-end guard against `earlyStrategyDecision(head:)` and the later + // `redirect(head:to:promise:)` both invoking the strategy for the same redirect: if + // `precomputed` weren't threaded through correctly, this would observe duplicate/repeated + // counts instead of a clean [0, 1, 2]. + let port = self.defaultHTTPBin.port + let observedCounts = NIOLockedValueBox<[Int]>([]) + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .custom { context in + observedCounts.withLockedValue { $0.append(context.redirectCount) } + guard context.redirectCount < 2 else { + return .doNotFollow + } + return .follow(context.redirectRequest) + } + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + let response = try localClient.get( + url: "http://localhost:\(port)/redirect/infinite1" + ).wait() + + XCTAssertEqual(response.status, .found) + XCTAssertEqual(observedCounts.withLockedValue { $0 }, [0, 1, 2]) + } + func testHttpRedirect() throws { let httpsBin = HTTPBin(.http1_1(ssl: true)) let localClient = HTTPClient( diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index 297e81704..3061cd042 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -964,6 +964,161 @@ final class RequestBagTests: XCTestCase { XCTAssertTrue(redirectTriggered) } + /// A `.strategy` redirect configuration deciding `.doNotFollow` for a redirect-eligible + /// response whose body is announced (`Content-Length`) as larger than `HTTPClient + /// .maxBodySizeRedirectResponse` (3KB) -- the case that, before `earlyStrategyDecision(head:)` + /// existed, was cancelled before any byte of the body was ever read, since the state machine + /// used to decide "redirect or not" only by size, not by asking the strategy first. Now the + /// strategy is asked immediately at head time, before touching the body at all, so + /// `.doNotFollow` delivers this response exactly like `testRaceBetweenConnectionCloseAndDemandMoreData`'s + /// plain, non-redirect one -- regardless of size. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + func testRedirectStrategyDoesNotFollowWithLargeAnnouncedBodyDeliversResponseNormally() { + let embeddedEventLoop = EmbeddedEventLoop() + defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } + let logger = Logger(label: "test") + + var maybeRequest: HTTPClient.Request? + XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "https://swift.org")) + guard let request = maybeRequest else { return XCTFail("Expected to have a request") } + + struct RefusingStrategy: HTTPClientRedirectStrategy { + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision { + .doNotFollow + } + } + + let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop) + var maybeRequestBag: RequestBag? + XCTAssertNoThrow( + maybeRequestBag = try RequestBag( + request: request, + eventLoopPreference: .delegate(on: embeddedEventLoop), + task: .init(eventLoop: embeddedEventLoop, logger: logger), + redirectHandler: .init( + request: request, + redirectState: RedirectState( + .strategy(RefusingStrategy()), + initialURL: request.url.absoluteString + )!, + execute: { _, _ in + XCTFail("`.doNotFollow` must not redirect") + return HTTPClient.Task( + eventLoop: embeddedEventLoop, + logger: logger + ) + } + ), + connectionDeadline: .now() + .seconds(30), + requestOptions: .forTests(), + delegate: delegate + ) + ) + guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } + + let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) + executor.runRequest(bag) + + let responseHead = HTTPResponseHead( + version: .http1_1, + status: .found, + headers: ["content-length": "\(4 * 1024)", "location": "https://swift.org/sswg"] + ) + bag.receiveResponseHead(responseHead) + XCTAssertFalse(executor.isCancelled) + XCTAssertFalse(executor.signalledDemandForResponseBody) + XCTAssertNoThrow(try XCTUnwrap(delegate.backpressurePromise).succeed(())) + XCTAssertTrue(executor.signalledDemandForResponseBody) + executor.resetResponseStreamDemandSignal() + + XCTAssertEqual(delegate.hitDidReceiveBodyPart, 0) + bag.receiveResponseBodyParts([ByteBuffer(repeating: 0, count: 4 * 1024)]) + XCTAssertFalse(executor.isCancelled) + XCTAssertEqual(delegate.hitDidReceiveBodyPart, 1) + XCTAssertNoThrow(try XCTUnwrap(delegate.backpressurePromise).succeed(())) + executor.resetResponseStreamDemandSignal() + + bag.receiveResponseEnd([], trailers: nil) + XCTAssertEqual(delegate.hitDidReceiveResponse, 1) + + XCTAssertFalse(executor.isCancelled) + XCTAssertEqual(delegate.receivedHead?.status, .found) + } + + /// `strategy.redirectDecision(for:)` is arbitrary caller code, and a strategy that + /// synchronously reaches back into its own task (e.g. to abort rather than redirect) is a + /// realistic thing to write. `earlyStrategyDecision(head:)` must be safe to call this way -- + /// specifically, it must be computed *before* `RequestBag`'s exclusive access to its own + /// state machine storage is taken, or a reentrant `task.cancel()` from inside the strategy + /// traps under Swift's exclusivity enforcement instead of just failing the task. + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + func testRedirectStrategyReentrantlyCancellingTheTaskDoesNotTrap() { + let embeddedEventLoop = EmbeddedEventLoop() + defer { XCTAssertNoThrow(try embeddedEventLoop.syncShutdownGracefully()) } + let logger = Logger(label: "test") + + var maybeRequest: HTTPClient.Request? + XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "https://swift.org")) + guard let request = maybeRequest else { return XCTFail("Expected to have a request") } + + let task = HTTPClient.Task(eventLoop: embeddedEventLoop, logger: logger) + + struct SelfCancellingStrategy: HTTPClientRedirectStrategy { + let task: HTTPClient.Task + + func redirectDecision(for context: HTTPClientRedirectContext) throws -> HTTPClientRedirectDecision { + self.task.cancel() + return .doNotFollow + } + } + + let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop) + var maybeRequestBag: RequestBag? + XCTAssertNoThrow( + maybeRequestBag = try RequestBag( + request: request, + eventLoopPreference: .delegate(on: embeddedEventLoop), + task: task, + redirectHandler: .init( + request: request, + redirectState: RedirectState( + .strategy(SelfCancellingStrategy(task: task)), + initialURL: request.url.absoluteString + )!, + execute: { _, _ in + XCTFail("Should not redirect") + return HTTPClient.Task( + eventLoop: embeddedEventLoop, + logger: logger + ) + } + ), + connectionDeadline: .now() + .seconds(30), + requestOptions: .forTests(), + delegate: delegate + ) + ) + guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") } + + let executor = MockRequestExecutor(eventLoop: embeddedEventLoop) + executor.runRequest(bag) + + // Must not trap. The strategy cancels `task` from inside `redirectDecision(for:)`, which + // reenters this same task while `earlyStrategyDecision(head:)` is being computed -- + // proving it runs outside the state machine's own exclusive access. + bag.receiveResponseHead( + .init( + version: .http1_1, + status: .found, + headers: ["content-length": "\(4 * 1024)", "location": "https://swift.org/sswg"] + ) + ) + + XCTAssertThrowsError(try task.futureResult.wait()) { + XCTAssertEqual($0 as? HTTPClientError, .cancelled) + } + } + func testWeDontLeakTheRequestIfTheRequestWriterWasCapturedByAPromise() { final class LeakDetector: Sendable {}