Fixed an issue where redirects to socket path-based servers from any server was always allowed #259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
Lukasa
reviewed
Jun 16, 2020
| @@ -162,12 +162,14 @@ extension HTTPClient { | |||
| } | |||
| } | |||
|
|
|||
| func supports(scheme: String) -> Bool { | |||
| func supportsRedirects(to scheme: String?) -> Bool { | |||
There was a problem hiding this comment.
Why has this become optional?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nah, this works too, just wanted to be sure I understood it.
dimitribouniol
force-pushed
the
redirect-validation
branch
from
3810060 to
6c87c34
Compare
|
Thanks! Happy to contribute where I can 🙂 |
dimitribouniol
force-pushed
the
redirect-validation
branch
from
6c87c34 to
79190aa
Compare
|
Rebased to keep up to date with master. |
… HTTP server are disallowed. Motivation: Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230. Modifications: - Added a method to HTTPBin to redirect to the specified target. - Added failing tests that perform redirects from a regular server to a socket-based server and vice versa. Result: Failing tests that show that the existing redirect checks were inadequate.
…server was always allowed. Motivation: An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme. Modifications: - Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do. - Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now. - Check the destination URL's scheme rather than the current URL's scheme when validating a redirect. Result: Closes swift-server#230
dimitribouniol
force-pushed
the
redirect-validation
branch
from
79190aa to
9db6474
Compare
|
Rebased to keep up to date with master. This is failing with: ... which I don't believe is related to these changes (first time I've run across it) |
|
Yep, this is test failure is related to #260 |
|
@swift-server-bot test this please |
artemredkin
approved these changes
Jun 22, 2020
|
@swift-server-bot test this please |
|
Thank you, @dimitribouniol ! |
|
Thank you! 😁 |
artemredkin
added a commit
to artemredkin/async-http-client
that referenced
this pull request
Jun 23, 2020
…server was always allowed (swift-server#259) * Added tests that ensure redirects to unix socket paths from a regular HTTP server are disallowed. Motivation: Currently, redirects to any supported URL scheme will always be allowed, despite code being in place to seemingly prevent it. See swift-server#230. Modifications: - Added a method to HTTPBin to redirect to the specified target. - Added failing tests that perform redirects from a regular server to a socket-based server and vice versa. Result: Failing tests that show that the existing redirect checks were inadequate. * Fixed an issue where redirects to socket path-based servers from any server was always allowed. Motivation: An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme. Modifications: - Refactored `hostSchemes`/`unixSchemes` to `hostRestrictedSchemes`/`allSupportedSchemes`, which better describes what they do. - Refactored `Request.supports()` to `Request.supportsRedirects(to:)` since it is only used by Redirects now. - Check the destination URL's scheme rather than the current URL's scheme when validating a redirect. Result: Closes swift-server#230 Co-authored-by: Artem Redkin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
An arbitrary HTTP(S) server should not be able to trigger redirects, and thus activity, to a local socket-path based server, though the opposite may be a valid scenario. Currently, requests in either direction are allowed since the checks don't actually check the destination scheme.
Modifications:
hostSchemes/unixSchemestohostRestrictedSchemes/allSupportedSchemes, which better describes what they do.Request.supports()toRequest.supportsRedirects(to:)since it is only used by Redirects now.Result:
Closes #230