Parse the host argument once and validate the endpoint - #634
Open
hashhar wants to merge 9 commits into
Open
Conversation
Connection and TrinoRequest stored the argument as given and compared it against the lowercase constants so a differently cased scheme silently skipped the TLS check and the port inference. Neither rejected a scheme that is not http or https.
The host URL used to win silently.
urlparse reads a scheme-less argument as a path so a bare hostname yielded no hostname and the code fell back to the raw string. Brackets, ports and paths were then added on top of that fallback which built unusable URLs for an IPv6 literal, for a path in the host and for a bare host:port. Prefixing // when the argument carries no scheme makes urlsplit read every accepted form as an authority so one parse handles all cases. A path and credentials are rejected rather than carried - neither ever reached the wire even before this change. Hostnames now come back lowercased, which also fixes the coordinator comparison in SpooledSegment that silently dropped custom headers for a mixed-case host.
The port argument was never validated so an out-of-range value built an unusable URL. Check the resolved port so one check covers a port from either argument. A port in the host that contradicts the port argument now raises, matching how a contradicting scheme is treated. It used to win silently.
_assert_valid_host reimplemented the parsing that _parse_host now owns. Call it and layer the stricter policy on top because this host goes into a URL that already has its own scheme and port.
| try: | ||
| scheme = trino.client.normalize_http_scheme(host_scheme) | ||
| except ValueError: | ||
| raise ValueError( |
Contributor
There was a problem hiding this comment.
A bit awkward. Maybe have normalize_http_scheme() accept an optional context argument and, if provided, use it to add "in {context!r}" to the error message?
Comment on lines
+36
to
+37
| missing port from the scheme. Wrap an IPv6 literal in brackets when it carries | ||
| a port, as in `[::1]:8080`. |
Contributor
There was a problem hiding this comment.
To me it reads as if it referred to IPv6 literal. Maybe
Suggested change
| missing port from the scheme. Wrap an IPv6 literal in brackets when it carries | |
| a port, as in `[::1]:8080`. | |
| missing port from the scheme. Wrap an IPv6 literal in brackets when a port | |
| is also present, as in `[::1]:8080`. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Connectionparsed thehostargument by hand on top of aurlparsecall that returns no hostname for a scheme-less value. An IPv6 literal, a barehost:portand a host carrying a path each built a malformed request URL that failed at connect time with an error naming neither the argument nor the problem.The
http_schemeandportarguments were not validated either. A wrongly cased scheme, a scheme or port contradicting one given inhost, and a port outside the valid range all went through unchecked.Parse the argument once and reject what cannot be represented. The scheme and port rules move out of
ConnectionandTrinoRequestinto helpers both call.A path in
hostis now rejected rather than carried. The port landed after the path, so no such setup ever reached the wire intact. The gateway-prefix use case behind it is real and still unserved. A dedicatedpath_prefixargument is the shape to add if we want it.Release notes
( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: