Accept CONNECT and absolute-form request-targets (RFC 9112 §3.2) - #923
Open
CodyPubNub wants to merge 3 commits into
Open
Accept CONNECT and absolute-form request-targets (RFC 9112 §3.2)#923CodyPubNub wants to merge 3 commits into
CodyPubNub wants to merge 3 commits into
Conversation
As of http 1.4.1, PathAndQuery rejects any input that is not origin-form
or "*" (RFC 9112 §3.2), so set_raw_path() — which routed every target
through Uri::builder().path_and_query() — fails to parse CONNECT and
absolute-form request-targets. pingora-http declares http = "1", so a
routine dependency bump silently breaks every forward-proxy and CONNECT
request. (Before 1.4.1 the same call mis-parsed absolute-form, folding
scheme+authority into the path so route matching failed — broken either
way.)
- absolute-form ("http://host/path", §3.2.2): a server MUST accept it.
Parse as a full Uri, keeping scheme+authority available to callers,
and re-serialize origin-form on the wire (raw_path() derives from
path_and_query()). An empty path is "/" (§3.2.1); the no-path-with-
query shape ("http://host?q") is canonicalized to "/?q" on the stored
Uri so a later rebuild (e.g. the H1->H2 :path) stays valid.
- authority-form ("host:443", §3.2.3): accepted only for CONNECT and
only as uri-host ":" port — a scheme, path, query, missing port, or
userinfo are rejected. The target is re-serialized verbatim so the
existing CONNECT tunneling path (allow_connect_method_proxying) runs.
- raw_path() no longer unwraps path_and_query(), and set_raw_path()
clears any stale fallback on entry so a reused header cannot serialize
a previous target.
Origin-form and asterisk-form keep their fast path; malformed targets
still error. The non-UTF-8 lossy branch is untouched.
Verified by request-target unit tests and end-to-end re-serialization
through http_req_header_to_wire().
Resolves cloudflare#909
Resolve conflict in pingora-http/src/lib.rs: - raw_path(): main's 0c08149 independently added an authority fallback for the same panic this branch fixes. Keep the branch version, which is the stricter superset: it gates the authority fallback on CONNECT (RFC 9112 §3.2.3 admits authority-form only there) and returns "/" rather than empty for a Uri with no path-and-query, since origin-form requires at least "/" (§3.2.1). Main's test_authority_form_raw_path is kept and passes against it. - Drop the DerefMut import removed by main's 4d7061b, keeping the Error import this branch's request-target validation needs. - Both test blocks are additive; keep all of them.
- Parse request targets before updating RequestHeader so a rejected mutation cannot discard the existing raw CONNECT or non-UTF-8 target. - Validate CONNECT uri-host syntax, including IPv6 and IPvFuture, because http::Uri accepts malformed bracketed authorities. - Exercise absolute-form and authority-form through the stock H1 parser, and run the existing CONNECT proxy tests without patched_http1.
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
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.
Resolves #909. Alternative to #912.
Summary
GET http://host/path.CONNECT host:porttargets.Uri, while forwardingabsolute-form to an origin as origin-form.
RequestHeaderunchanged whenset_raw_path()rejects a replacement./for non-CONNECT URIs without a path-and-query, and limit theauthority fallback to CONNECT.
Why
RequestHeader::set_raw_path()previously sent every request target throughUri::builder().path_and_query(). Starting withhttp1.4.1, that builderrejects targets other than origin-form or
*. This makes valid HTTP/1absolute-form and CONNECT authority-form requests fail before application code
runs.
RFC 9112 §3.2.2 requires servers to accept absolute-form. RFC 9112 §3.2.3 defines CONNECT authority-form as
uri-host ":" port.Behavior
raw_path()/ origin wire/path?q/path/path?q***http://host/path?q/pathhost/path?qhttp://host/host/http://host?q/host/?qhost:443with CONNECT""host:443host:443Malformed CONNECT targets are rejected, including targets with a scheme, path,
query, userinfo, missing or invalid port, empty host, malformed IP-literal, or
invalid reg-name characters.
Implementation
parse_request_target()computes the newUriand raw fallback beforeupdating either field, so errors leave the existing header intact.
Uri. Scheme and authority remainavailable to callers; the stored path-and-query is normalized for later H1
or H2 forwarding.
serialization and are validated as
uri-host ":" port.hosts accept the
http::Uri-representable subset of reg-name.No new fields are added. The common origin-form path does not perform
authority validation.
Interaction with current main
0c08149independently fixed the sameraw_path()panic by falling back to theUriauthority unconditionally. This branch retains that fallback for CONNECT and intentionally narrows it for other methods, which return/instead. Authority-form is valid only for CONNECT (§3.2.3); origin-form requires at least/(§3.2.1). The upstreamtest_authority_form_raw_pathtest is kept and passes unchanged.Tests
pingora-httpunit tests cover all four request-target forms, malformedabsolute and authority targets, IPv6, IPvFuture, reg-name, and atomic failure.
pingora-coretests drive absolute-form and CONNECT through the real stockHTTP/1 downstream parser using mock IO, including a malformed
CONNECT [gg]:443that is rejected at read time — validation fires on actualinbound H1 traffic, not just on constructed headers.
CONNECT.
patched_http1feature.receives
GET /absolute?q=1 HTTP/1.1.Local verification:
cargo test -p pingora-http --lib: 52 passedcargo test -p pingora-core --lib request_target: 4 passedcargo test -p pingora-proxy --lib: 22 passedcargo test -p pingora-proxy --test test_basic --no-run: cleancargo check --workspace: cleancargo fmt --all -- --check: cleangit diff --check: cleanThe three CONNECT/absolute-form proxy integrations compile locally but require the external OpenResty fixture and have not run locally. Stock-parser behavior is covered locally by the
pingora-coremock-IO tests; the integrations will execute in CI.Difference from #912
/, rather than re-emittingthe absolute URI as the origin request target.
http://host?qis normalized to/?qon the storedUri, so a later H2:pathrebuild is valid.http::Uriaccepts but RFC 9112authority-form does not.
Out of scope
Hostfrom the absolute-form authority.origin.
*.http::Urirejects before host validation.