Skip to content

Fixed an issue where redirects to socket path-based servers from any server was always allowed #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 22, 2020
Merged

Fixed an issue where redirects to socket path-based servers from any server was always allowed #259

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71d7f7f
Added tests that ensure redirects to unix socket paths from a regular…
dimitribouniol Jun 13, 2020
9db6474
Fixed an issue where redirects to socket path-based servers from any …
dimitribouniol Jun 13, 2020
1725ab4
Merge branch 'master' into redirect-validation
artemredkin Jun 22, 2020
d5087bf
Merge branch 'master' into redirect-validation
artemredkin Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions Sources/AsyncHTTPClient/HTTPHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ extension HTTPClient {
/// UNIX Domain Socket HTTP request.
case unixSocket(_ scheme: UnixScheme)

private static var hostSchemes = ["http", "https"]
private static var unixSchemes = ["unix", "http+unix", "https+unix"]
private static var hostRestrictedSchemes: Set = ["http", "https"]
private static var allSupportedSchemes: Set = ["http", "https", "unix", "http+unix", "https+unix"]

init(forScheme scheme: String) throws {
switch scheme {
Expand Down Expand Up @@ -158,12 +158,14 @@ extension HTTPClient {
}
}

func supports(scheme: String) -> Bool {
func supportsRedirects(to scheme: String?) -> Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why has this become optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly to simplify the call site. I can change that to

guard let scheme = url.scheme, self.request.kind.supportsRedirects(to: scheme) {...

if you’d prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, this works too, just wanted to be sure I understood it.

guard let scheme = scheme?.lowercased() else { return false }

switch self {
case .host:
return Kind.hostSchemes.contains(scheme)
return Kind.hostRestrictedSchemes.contains(scheme)
case .unixSocket:
return Kind.unixSchemes.contains(scheme)
return Kind.allSupportedSchemes.contains(scheme)
}
}
}
Expand Down Expand Up @@ -1049,7 +1051,7 @@ internal struct RedirectHandler<ResponseType> {
return nil
}

guard self.request.kind.supports(scheme: self.request.scheme) else {
guard self.request.kind.supportsRedirects(to: url.scheme) else {
return nil
}

Expand Down
6 changes: 6 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,12 @@ internal final class HttpBinHandler: ChannelInboundHandler {
headers.add(name: "Location", value: "/redirect/infinite1")
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
case "/redirect/target":
var headers = self.responseHeaders
let targetURL = req.headers["X-Target-Redirect-URL"].first ?? ""
headers.add(name: "Location", value: targetURL)
self.resps.append(HTTPResponseBuilder(status: .found, headers: headers))
return
case "/percent%20encoded":
if req.method != .GET {
self.resps.append(HTTPResponseBuilder(status: .methodNotAllowed))
Expand Down
89 changes: 89 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTPClientTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,95 @@ class HTTPClientTests: XCTestCase {

response = try localClient.get(url: self.defaultHTTPBinURLPrefix + "redirect/https?port=\(httpsBin.port)").wait()
XCTAssertEqual(response.status, .ok)

XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpSocketPath in
XCTAssertNoThrow(try TemporaryFileHelpers.withTemporaryUnixDomainSocketPathName { httpsSocketPath in
let socketHTTPBin = HTTPBin(bindTarget: .unixDomainSocket(httpSocketPath))
let socketHTTPSBin = HTTPBin(ssl: true, bindTarget: .unixDomainSocket(httpsSocketPath))
defer {
XCTAssertNoThrow(try socketHTTPBin.shutdown())
XCTAssertNoThrow(try socketHTTPSBin.shutdown())
}

// From HTTP or HTTPS to HTTP+UNIX should fail to redirect
var targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
var request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

var response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .found)
XCTAssertEqual(response.headers.first(name: "Location"), targetURL)

request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .found)
XCTAssertEqual(response.headers.first(name: "Location"), targetURL)

// From HTTP or HTTPS to HTTPS+UNIX should also fail to redirect
targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
request = try Request(url: self.defaultHTTPBinURLPrefix + "redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .found)
XCTAssertEqual(response.headers.first(name: "Location"), targetURL)

request = try Request(url: "https://localhost:\(httpsBin.port)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .found)
XCTAssertEqual(response.headers.first(name: "Location"), targetURL)

// ... while HTTP+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed
targetURL = self.defaultHTTPBinURLPrefix + "ok"
request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "https://localhost:\(httpsBin.port)/ok"
request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
request = try Request(url: "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

// ... and HTTPS+UNIX to HTTP, HTTPS, or HTTP(S)+UNIX should succeed
targetURL = self.defaultHTTPBinURLPrefix + "ok"
request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "https://localhost:\(httpsBin.port)/ok"
request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "http+unix://\(httpSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)

targetURL = "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/ok"
request = try Request(url: "https+unix://\(httpsSocketPath.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!)/redirect/target", method: .GET, headers: ["X-Target-Redirect-URL": targetURL], body: nil)

response = try localClient.execute(request: request).wait()
XCTAssertEqual(response.status, .ok)
})
})
}

func testHttpHostRedirect() {
Expand Down