From efe6b5b6f626723d980a0ce72a4cf081b149f0ea Mon Sep 17 00:00:00 2001 From: Iain Smith Date: Mon, 13 Apr 2020 20:13:04 +0100 Subject: [PATCH 1/2] Enable clients to call URLs that include %2F as an escaped backslash Previously `percentEncodedPath` was using `path.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed)` which converts %2F to a literal '/'. This prevented users fetching URLs like https://api.travis-ci.org/repo/github/rails%2Frails which use %2F as part of a path segment. Migrating to `URLComponents(url: self, resolvingAgainstBaseURL: false)?.percentEncodedPath` has the desired behaviour for the couple of test cases that exist. Updated the test server to switch on the `percentEncodedPath` so it's easier to understand the desired behaviour. --- Sources/AsyncHTTPClient/HTTPHandler.swift | 2 +- .../HTTPClientTestUtils.swift | 18 ++++++++++++------ .../HTTPClientTests+XCTest.swift | 1 + .../AsyncHTTPClientTests/HTTPClientTests.swift | 12 ++++++++++++ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index b77ab02d1..5ac7ad667 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -464,7 +464,7 @@ extension URL { if self.path.isEmpty { return "/" } - return self.path.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? self.path + return URLComponents(url: self, resolvingAgainstBaseURL: false)?.percentEncodedPath ?? self.path } var pathHasTrailingSlash: Bool { diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift index 65e675721..765839f93 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift @@ -394,8 +394,8 @@ internal final class HttpBinHandler: ChannelInboundHandler { switch self.unwrapInboundIn(data) { case .head(let req): self.parseAndSetOptions(from: req) - let url = URL(string: req.uri)! - switch url.path { + let urlComponents = URLComponents(string: req.uri)! + switch urlComponents.percentEncodedPath { case "/": var headers = HTTPHeaders() headers.add(name: "X-Is-This-Slash", value: "Yes") @@ -429,13 +429,13 @@ internal final class HttpBinHandler: ChannelInboundHandler { self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return case "/redirect/https": - let port = self.value(for: "port", from: url.query!) + let port = self.value(for: "port", from: urlComponents.query!) var headers = HTTPHeaders() headers.add(name: "Location", value: "https://localhost:\(port)/ok") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return case "/redirect/loopback": - let port = self.value(for: "port", from: url.query!) + let port = self.value(for: "port", from: urlComponents.query!) var headers = HTTPHeaders() headers.add(name: "Location", value: "http://127.0.0.1:\(port)/echohostheader") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) @@ -450,8 +450,14 @@ internal final class HttpBinHandler: ChannelInboundHandler { headers.add(name: "Location", value: "/redirect/infinite1") self.resps.append(HTTPResponseBuilder(status: .found, headers: headers)) return - // Since this String is taken from URL.path, the percent encoding has been removed - case "/percent encoded": + case "/percent%20encoded": + if req.method != .GET { + self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed)) + return + } + self.resps.append(HTTPResponseBuilder(status: .ok)) + return + case "/percent%2Fencoded/hello": if req.method != .GET { self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed)) return diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift index 0ffeb42a6..8937682a0 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests+XCTest.swift @@ -37,6 +37,7 @@ extension HTTPClientTests { ("testHttpRedirect", testHttpRedirect), ("testHttpHostRedirect", testHttpHostRedirect), ("testPercentEncoded", testPercentEncoded), + ("testPercentEncodedBackslash", testPercentEncodedBackslash), ("testMultipleContentLengthHeaders", testMultipleContentLengthHeaders), ("testStreaming", testStreaming), ("testRemoteClose", testRemoteClose), diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift index 986194761..f58170205 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientTests.swift @@ -218,6 +218,18 @@ class HTTPClientTests: XCTestCase { XCTAssertEqual(.ok, response.status) } + func testPercentEncodedBackslash() throws { + let httpBin = HTTPBin() + let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) + defer { + XCTAssertNoThrow(try httpClient.syncShutdown(requiresCleanClose: true)) + XCTAssertNoThrow(try httpBin.shutdown()) + } + + let response = try httpClient.get(url: "http://localhost:\(httpBin.port)/percent%2Fencoded/hello").wait() + XCTAssertEqual(.ok, response.status) + } + func testMultipleContentLengthHeaders() throws { let httpClient = HTTPClient(eventLoopGroupProvider: .createNew) defer { From 3950c9598d4edd2d6fe8c8222a4825760c281754 Mon Sep 17 00:00:00 2001 From: Iain Smith Date: Tue, 14 Apr 2020 12:27:54 +0100 Subject: [PATCH 2/2] Remove redundant path manipulation related to trailing slashes. See - efe6b5b6f626723d980a0ce72a4cf081b149f0ea --- Sources/AsyncHTTPClient/HTTPHandler.swift | 22 ------------------- .../HTTPClientInternalTests.swift | 3 +++ 2 files changed, 3 insertions(+), 22 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift index 5ac7ad667..22069731f 100644 --- a/Sources/AsyncHTTPClient/HTTPHandler.swift +++ b/Sources/AsyncHTTPClient/HTTPHandler.swift @@ -467,30 +467,8 @@ extension URL { return URLComponents(url: self, resolvingAgainstBaseURL: false)?.percentEncodedPath ?? self.path } - var pathHasTrailingSlash: Bool { - if #available(OSX 10.11, iOS 9.0, tvOS 9.0, watchOS 2.0, *) { - return self.hasDirectoryPath - } else { - // Most platforms should use `self.hasDirectoryPath`, but on older darwin platforms - // we have this approximation instead. - let url = self.absoluteString - - var pathEndIndex = url.index(before: url.endIndex) - if let queryIndex = url.firstIndex(of: "?") { - pathEndIndex = url.index(before: queryIndex) - } else if let fragmentIndex = url.suffix(from: url.firstIndex(of: "@") ?? url.startIndex).lastIndex(of: "#") { - pathEndIndex = url.index(before: fragmentIndex) - } - - return url[pathEndIndex] == "/" - } - } - var uri: String { var uri = self.percentEncodedPath - if self.pathHasTrailingSlash, uri != "/" { - uri += "/" - } if let query = self.query { uri += "?" + query diff --git a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift index 9288814c6..f175246c0 100644 --- a/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTPClientInternalTests.swift @@ -286,6 +286,9 @@ class HTTPClientInternalTests: XCTestCase { let request11 = try Request(url: "https://someserver.com/some%20path") XCTAssertEqual(request11.url.uri, "/some%20path") + + let request12 = try Request(url: "https://someserver.com/some%2Fpathsegment1/pathsegment2") + XCTAssertEqual(request12.url.uri, "/some%2Fpathsegment1/pathsegment2") } func testChannelAndDelegateOnDifferentEventLoops() throws {