diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift index bbf8c948c..b77cb9527 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClient+execute.swift @@ -140,7 +140,8 @@ extension HTTPClient { let newRequest = currentRequest.followingRedirect( from: preparedRequest.url, to: redirectURL, - status: response.status + status: response.status, + config: redirectState.config ) guard newRequest.body.canBeConsumedMultipleTimes else { diff --git a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift index e57406e2b..03cd8e464 100644 --- a/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift +++ b/Sources/AsyncHTTPClient/AsyncAwait/HTTPClientRequest+Prepared.swift @@ -124,7 +124,8 @@ extension HTTPClientRequest { func followingRedirect( from originalURL: URL, to redirectURL: URL, - status: HTTPResponseStatus + status: HTTPResponseStatus, + config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration ) -> HTTPClientRequest { let (method, headers, body) = transformRequestForRedirect( from: originalURL, @@ -132,7 +133,8 @@ extension HTTPClientRequest { headers: self.headers, body: self.body, to: redirectURL, - status: status + status: status, + config: config ) var newRequest = HTTPClientRequest(url: redirectURL.absoluteString) newRequest.method = method diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index 5beba8da6..1aa37fb7e 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -1269,13 +1269,54 @@ extension HTTPClient.Configuration { /// Redirects are not followed. case disallow /// Redirects are followed with a specified limit. - case follow(max: Int, allowCycles: Bool) + case follow(FollowConfiguration) + } + + /// Configuration for following redirects. + public struct FollowConfiguration: Hashable, Sendable { + /// The maximum number of allowed redirects. + public var max: Int + /// Whether cycles are allowed. + public var allowCycles: Bool + /// Whether to retain the HTTP method and body when following a 301 redirect on a POST request. + /// This should be false as per the fetch spec, but may be true according to RFC 9110. + /// This does not affect non-POST requests. + public var retainHTTPMethodAndBodyOn301: Bool + /// Whether to retain the HTTP method and body when following a 302 redirect on a POST request. + /// This should be false as per the fetch spec, but may be true according to RFC 9110. + /// This does not affect non-POST requests. + public var retainHTTPMethodAndBodyOn302: Bool + + /// Create a new ``FollowConfiguration`` + /// - Parameters: + /// - max: The maximum number of allowed redirects. + /// - allowCycles: Whether cycles are allowed. + /// - retainHTTPMethodAndBodyOn301: Whether to retain the HTTP method and body when following a 301 redirect on a POST request. This should be false as per the fetch spec, but may be true according to RFC 9110. This does not affect non-POST requests. + /// - retainHTTPMethodAndBodyOn302: Whether to retain the HTTP method and body when following a 302 redirect on a POST request. This should be false as per the fetch spec, but may be true according to RFC 9110. This does not affect non-POST requests. + public init( + max: Int, + allowCycles: Bool, + retainHTTPMethodAndBodyOn301: Bool, + retainHTTPMethodAndBodyOn302: Bool + ) { + self.max = max + self.allowCycles = allowCycles + self.retainHTTPMethodAndBodyOn301 = retainHTTPMethodAndBodyOn301 + self.retainHTTPMethodAndBodyOn302 = retainHTTPMethodAndBodyOn302 + } } var mode: Mode init() { - self.mode = .follow(max: 5, allowCycles: false) + self.mode = .follow( + .init( + max: 5, + allowCycles: false, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ) } init(configuration: Mode) { @@ -1293,7 +1334,21 @@ extension HTTPClient.Configuration { /// /// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector. public static func follow(max: Int, allowCycles: Bool) -> RedirectConfiguration { - .init(configuration: .follow(max: max, allowCycles: allowCycles)) + .follow( + configuration: .init( + max: max, + allowCycles: allowCycles, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ) + } + + /// Redirects are followed. + /// + /// - Parameter: configuration: Configure how redirects are followed. + public static func follow(configuration: FollowConfiguration) -> RedirectConfiguration { + .init(configuration: .follow(configuration)) } } diff --git a/Sources/AsyncHTTPClient/HTTPClientConfiguration+SwiftConfiguration.swift b/Sources/AsyncHTTPClient/HTTPClientConfiguration+SwiftConfiguration.swift index 8adb22d70..3015dfadd 100644 --- a/Sources/AsyncHTTPClient/HTTPClientConfiguration+SwiftConfiguration.swift +++ b/Sources/AsyncHTTPClient/HTTPClientConfiguration+SwiftConfiguration.swift @@ -66,6 +66,8 @@ extension HTTPClient.Configuration.RedirectConfiguration { /// - `mode` (string, optional, default: "follow"): Redirect handling mode ("follow" or "disallow"). /// - `maxRedirects` (int, optional, default: 5): Maximum allowed redirects when mode is "follow". /// - `allowCycles` (bool, optional, default: false): Allow cyclic redirects when mode is "follow". + /// - `retainHTTPMethodAndBodyOn301` (bool, optional, default: false): Whether to retain the HTTP method and body when following a 301 redirect on a POST request. This should be false as per the fetch spec, but may be true according to RFC 9110. This does not affect non-POST requests. + /// - `retainHTTPMethodAndBodyOn302` (bool, optional, default: false): Whether to retain the HTTP method and body when following a 302 redirect on a POST request. This should be false as per the fetch spec, but may be true according to RFC 9110. This does not affect non-POST requests. /// /// - Throws: `HTTPClientError.invalidRedirectConfiguration` if mode is specified but invalid. public init(configReader: ConfigReader) throws { @@ -77,7 +79,16 @@ extension HTTPClient.Configuration.RedirectConfiguration { if mode == "follow" { let maxRedirects = configReader.int(forKey: "maxRedirects", default: 5) let allowCycles = configReader.bool(forKey: "allowCycles", default: false) - self = .follow(max: maxRedirects, allowCycles: allowCycles) + let retainHTTPMethodAndBodyOn301 = configReader.bool(forKey: "retainHTTPMethodAndBodyOn301", default: false) + let retainHTTPMethodAndBodyOn302 = configReader.bool(forKey: "retainHTTPMethodAndBodyOn302", default: false) + self = .follow( + configuration: .init( + max: maxRedirects, + allowCycles: allowCycles, + retainHTTPMethodAndBodyOn301: retainHTTPMethodAndBodyOn301, + retainHTTPMethodAndBodyOn302: retainHTTPMethodAndBodyOn302 + ) + ) } else if mode == "disallow" { self = .disallow } else { diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 940cdf40f..9c7becb0b 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -1076,7 +1076,8 @@ internal struct RedirectHandler { headers: self.request.headers, body: self.request.body, to: redirectURL, - status: status + status: status, + config: self.redirectState.config ) let newRequest = try HTTPClient.Request( diff --git a/Sources/AsyncHTTPClient/RedirectState.swift b/Sources/AsyncHTTPClient/RedirectState.swift index 95de2d508..4e14712d6 100644 --- a/Sources/AsyncHTTPClient/RedirectState.swift +++ b/Sources/AsyncHTTPClient/RedirectState.swift @@ -19,14 +19,10 @@ import struct Foundation.URL typealias RedirectMode = HTTPClient.Configuration.RedirectConfiguration.Mode struct RedirectState { - /// number of redirects we are allowed to follow. - private var limit: Int + var config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration /// All visited URLs. private var visited: [String] - - /// if true, `redirect(to:)` will throw an error if a cycle is detected. - private let allowCycles: Bool } extension RedirectState { @@ -40,8 +36,8 @@ extension RedirectState { switch configuration { case .disallow: return nil - case .follow(let maxRedirects, let allowCycles): - self.init(limit: maxRedirects, visited: [initialURL], allowCycles: allowCycles) + case .follow(let config): + self.init(config: config, visited: [initialURL]) } } } @@ -52,11 +48,11 @@ extension RedirectState { /// - 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 <= limit else { + guard self.visited.count <= config.max else { throw HTTPClientError.redirectLimitReached } - guard allowCycles || !self.visited.contains(redirectURL) else { + guard config.allowCycles || !self.visited.contains(redirectURL) else { throw HTTPClientError.redirectCycleDetected } self.visited.append(redirectURL) @@ -111,13 +107,16 @@ func transformRequestForRedirect( headers requestHeaders: HTTPHeaders, body requestBody: Body?, to redirectURL: URL, - status responseStatus: HTTPResponseStatus + status responseStatus: HTTPResponseStatus, + config: HTTPClient.Configuration.RedirectConfiguration.FollowConfiguration ) -> (HTTPMethod, HTTPHeaders, Body?) { let convertToGet: Bool if responseStatus == .seeOther, requestMethod != .HEAD { convertToGet = true - } else if responseStatus == .movedPermanently || responseStatus == .found, requestMethod == .POST { - convertToGet = true + } else if responseStatus == .movedPermanently, requestMethod == .POST { + convertToGet = !config.retainHTTPMethodAndBodyOn301 + } else if responseStatus == .found, requestMethod == .POST { + convertToGet = !config.retainHTTPMethodAndBodyOn302 } else { convertToGet = false } diff --git a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift index 56a08b852..43430b85c 100644 --- a/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift +++ b/Tests/AsyncHTTPClientTests/AsyncAwaitEndToEndTests.swift @@ -1019,6 +1019,95 @@ final class AsyncAwaitEndToEndTests: XCTestCase { } } } + + // MARK: - POST to GET conversion on redirects + + @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) + private func _testPostConvertedToGetOnRedirect( + statusPath: String, + expectedStatus: HTTPResponseStatus + ) { + XCTAsyncTest { + let bin = HTTPBin(.http2(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + let client = makeDefaultHTTPClient() + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:)) + + var request = HTTPClientRequest(url: "https://localhost:\(bin.port)\(statusPath)") + request.method = .POST + request.body = .bytes(ByteBuffer(string: "test body")) + + guard + let response = await XCTAssertNoThrowWithResult( + try await client.execute(request, deadline: .now() + .seconds(10), logger: logger) + ) + else { return } + + XCTAssertEqual(response.status, .ok) + XCTAssertEqual(response.history.count, 2, "Expected 2 entries in history for \(statusPath)") + XCTAssertEqual( + response.history[0].request.method, + .POST, + "Original request should be POST for \(statusPath)" + ) + XCTAssertEqual( + response.history[1].request.method, + .GET, + "Redirected request should be converted to GET for \(statusPath)" + ) + XCTAssertEqual( + response.history[0].responseHead.status, + expectedStatus, + "Expected \(expectedStatus) for \(statusPath)" + ) + } + } + + func testPostConvertedToGetOn301Redirect() { + self._testPostConvertedToGetOnRedirect( + statusPath: "/redirect/301", + expectedStatus: .movedPermanently + ) + } + + func testPostConvertedToGetOn302Redirect() { + self._testPostConvertedToGetOnRedirect( + statusPath: "/redirect/302", + expectedStatus: .found + ) + } + + func testPostConvertedToGetOn303Redirect() { + self._testPostConvertedToGetOnRedirect( + statusPath: "/redirect/303", + expectedStatus: .seeOther + ) + } + + func testGetMethodUnchangedOnRedirect() { + XCTAsyncTest { + let bin = HTTPBin(.http2(compress: false)) + defer { XCTAssertNoThrow(try bin.shutdown()) } + let client = makeDefaultHTTPClient() + defer { XCTAssertNoThrow(try client.syncShutdown()) } + let logger = Logger(label: "HTTPClient", factory: StreamLogHandler.standardOutput(label:)) + + // Test that non-POST methods remain unchanged + let requestGet = HTTPClientRequest(url: "https://localhost:\(bin.port)/redirect/302") + + guard + let responseGet = await XCTAssertNoThrowWithResult( + try await client.execute(requestGet, deadline: .now() + .seconds(10), logger: logger) + ) + else { return } + + XCTAssertEqual(responseGet.status, .ok) + XCTAssertEqual(responseGet.history.count, 2) + XCTAssertEqual(responseGet.history[0].request.method, .GET, "Original request should be GET") + XCTAssertEqual(responseGet.history[1].request.method, .GET, "Redirected request should remain GET") + } + } } struct AnySendableSequence: @unchecked Sendable { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 689b4358e..2dcce3e12 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -984,11 +984,21 @@ internal final class HTTPBinHandler: ChannelInboundHandler { } self.resps.append(HTTPResponseBuilder(status: .ok, responseBodyIsRequestBodyByteCount: true)) return + case "/redirect/301": + var headers = self.responseHeaders + headers.add(name: "location", value: "/ok") + self.resps.append(HTTPResponseBuilder(status: .movedPermanently, headers: headers)) + return case "/redirect/302": var headers = self.responseHeaders headers.add(name: "location", value: "/ok") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return + case "/redirect/303": + var headers = self.responseHeaders + headers.add(name: "location", value: "/ok") + self.resps.append(HTTPResponseBuilder(status: .seeOther, headers: headers)) + return case "/redirect/https": let port = self.value(for: "port", from: urlComponents.query!) var headers = self.responseHeaders diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 054cf3487..959e0f939 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -4575,6 +4575,75 @@ final class HTTPClientTests: XCTestCaseHTTPClientTestsBaseClass { XCTAssertEqual($0 as? HTTPClientError, .connectTimeout) } } + + private func _testPostConvertedToGetOnRedirect( + statusPath: String, + expectedStatus: HTTPResponseStatus, + retainHTTPMethodAndBodyOn301: Bool, + retainHTTPMethodAndBodyOn302: Bool, + expectRetain: Bool + ) throws { + let bin = HTTPBin(.http1_1()) + defer { XCTAssertNoThrow(try bin.shutdown()) } + + let localClient = HTTPClient( + eventLoopGroupProvider: .shared(self.clientGroup), + configuration: HTTPClient.Configuration( + redirectConfiguration: .follow( + configuration: .init( + max: 10, + allowCycles: false, + retainHTTPMethodAndBodyOn301: retainHTTPMethodAndBodyOn301, + retainHTTPMethodAndBodyOn302: retainHTTPMethodAndBodyOn302 + ) + ) + ) + ) + defer { XCTAssertNoThrow(try localClient.syncShutdown()) } + + let request = try HTTPClient.Request( + url: "http://localhost:\(bin.port)\(statusPath)", + method: .POST, + headers: HTTPHeaders(), + body: .string("test body") + ) + let response = try localClient.execute(request: request).wait() + XCTAssertEqual(response.status, .ok) + guard response.history.count == 2 else { + return XCTFail("Expected 2 entries in history for \(statusPath)") + } + XCTAssertEqual(response.history[0].request.method, .POST) + if expectRetain { + XCTAssertEqual(response.history[1].request.method, .POST) + } else { + XCTAssertEqual(response.history[1].request.method, .GET) + } + XCTAssertEqual(response.history[0].responseHead.status, expectedStatus) + } + + func testPostConvertedToGetOn301Redirect() throws { + for retainHTTPMethodAndBody in [true, false] { + try _testPostConvertedToGetOnRedirect( + statusPath: "/redirect/301", + expectedStatus: .movedPermanently, + retainHTTPMethodAndBodyOn301: retainHTTPMethodAndBody, + retainHTTPMethodAndBodyOn302: false, + expectRetain: retainHTTPMethodAndBody + ) + } + } + + func testPostConvertedToGetOn302Redirect() throws { + for retainHTTPMethodAndBody in [true, false] { + try _testPostConvertedToGetOnRedirect( + statusPath: "/redirect/302", + expectedStatus: .found, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: retainHTTPMethodAndBody, + expectRetain: retainHTTPMethodAndBody + ) + } + } } final class CountingDebugInitializerUtil: Sendable { diff --git a/Tests/AsyncHTTPClientTests/RequestBagTests.swift b/Tests/AsyncHTTPClientTests/RequestBagTests.swift index 51621c7a6..a3ae5017c 100644 --- a/Tests/AsyncHTTPClientTests/RequestBagTests.swift +++ b/Tests/AsyncHTTPClientTests/RequestBagTests.swift @@ -731,7 +731,14 @@ final class RequestBagTests: XCTestCase { redirectHandler: .init( request: request, redirectState: RedirectState( - .follow(max: 5, allowCycles: false), + .follow( + .init( + max: 5, + allowCycles: false, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ), initialURL: request.url.absoluteString )!, execute: { request, _ in @@ -819,7 +826,14 @@ final class RequestBagTests: XCTestCase { redirectHandler: .init( request: request, redirectState: RedirectState( - .follow(max: 5, allowCycles: false), + .follow( + .init( + max: 5, + allowCycles: false, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ), initialURL: request.url.absoluteString )!, execute: { request, _ in @@ -881,7 +895,14 @@ final class RequestBagTests: XCTestCase { redirectHandler: .init( request: request, redirectState: RedirectState( - .follow(max: 5, allowCycles: false), + .follow( + .init( + max: 5, + allowCycles: false, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ), initialURL: request.url.absoluteString )!, execute: { request, _ in diff --git a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift index 8e9c97276..6bb796f6c 100644 --- a/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift +++ b/Tests/AsyncHTTPClientTests/SwiftConfigurationTests.swift @@ -29,6 +29,8 @@ struct HTTPClientConfigurationPropsTests { "redirect.mode": "follow", "redirect.maxRedirects": 10, "redirect.allowCycles": true, + "redirect.retainHTTPMethodAndBodyOn301": true, + "redirect.retainHTTPMethodAndBodyOn302": true, "timeout.connectionMs": 5000, "timeout.readMs": 30000, @@ -51,9 +53,11 @@ struct HTTPClientConfigurationPropsTests { #expect(config.dnsOverride["example.com"] == "192.168.1.1") switch config.redirectConfiguration.mode { - case .follow(let max, let allowCycles): - #expect(max == 10) - #expect(allowCycles) + case .follow(let follow): + #expect(follow.max == 10) + #expect(follow.allowCycles) + #expect(follow.retainHTTPMethodAndBodyOn301) + #expect(follow.retainHTTPMethodAndBodyOn302) case .disallow: Issue.record("Unexpected value") } @@ -245,7 +249,17 @@ struct HTTPClientConfigurationPropsTests { let configReader = ConfigReader(provider: testProvider) let config = try HTTPClient.Configuration(configReader: configReader) - #expect(config.redirectConfiguration.mode == .follow(max: 5, allowCycles: false)) + #expect( + config.redirectConfiguration.mode + == .follow( + .init( + max: 5, + allowCycles: false, + retainHTTPMethodAndBodyOn301: false, + retainHTTPMethodAndBodyOn302: false + ) + ) + ) } @Test @@ -255,13 +269,25 @@ struct HTTPClientConfigurationPropsTests { "redirect.mode": "follow", "redirect.maxRedirects": 3, "redirect.allowCycles": true, + "redirect.retainHTTPMethodAndBodyOn301": true, + "redirect.retainHTTPMethodAndBodyOn302": false, ]) let configReader = ConfigReader(provider: testProvider) let config = try HTTPClient.Configuration(configReader: configReader) - #expect(config.redirectConfiguration.mode == .follow(max: 3, allowCycles: true)) + #expect( + config.redirectConfiguration.mode + == .follow( + .init( + max: 3, + allowCycles: true, + retainHTTPMethodAndBodyOn301: true, + retainHTTPMethodAndBodyOn302: false + ) + ) + ) } @Test