Skip to content

Commit 896dcff

Browse files
committed
Polish GHSA-r98x-p6m8-xcrv fix
Three small follow-ups to c7517ab: 1. Hoist the duplicate `./` guard above the if/elsif. The guard is a no-op when neither base_uri nor persistent is set, so it's safe to factor out — and it lets the two-branch comment collapse into one. 2. Strengthen the persistent regression test. Asserting only on `req.uri.host` hides an intermediate where make_request_uri returns a URI with host "example.com." (trailing dot), which only normalises away because HTTP::URI#normalize_host strips trailing dots. Pin the full URI (`origin` + `to_s`) so a future change to normalize_host that lets the dot leak through is caught here. 3. CHANGELOG accuracy. The persistent branch never called `URI#merge` — it was string concatenation. Distinguish the two branches and reference RFC 3986 §5.2 for the underlying rule.
1 parent c7517ab commit 896dcff

3 files changed

Lines changed: 19 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1111

1212
- `HTTP::Request::Builder#make_request_uri` now rejects protocol-relative inputs
1313
(`//host/path`) when resolving against a configured `base_uri` or `persistent`
14-
origin. Previously such inputs flowed into `URI#merge` and replaced the base
15-
authority, allowing an attacker who controlled the path argument to redirect
16-
the request to an arbitrary host and leak any connection-scoped headers
14+
origin. Previously such inputs were treated as network-path references per
15+
RFC 3986 §5.2 and replaced the base authority — via `URI#join` on the
16+
`base_uri` branch and via string concatenation on the `persistent` branch —
17+
allowing an attacker who controlled the path argument to redirect the
18+
request to an arbitrary host and leak any connection-scoped headers
1719
(`HTTP.auth(...)`, etc.). See `GHSA-r98x-p6m8-xcrv` for details.
1820

1921
## [6.0.3] - 2026-04-20

lib/http/request/builder.rb

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,17 @@ def wrap(request)
8383
def make_request_uri(uri)
8484
uri = uri.to_s
8585

86+
# A leading "//" makes the input a protocol-relative (network-path)
87+
# reference per RFC 3986 §4.2 / §5.2. On the base_uri branch it would
88+
# replace the base authority via URI#join; on the persistent branch
89+
# naive concatenation produces "scheme://persistent//evil/path" which
90+
# HTTP::URI.parse normalises into scheme://evil/path. Prepend "./" so
91+
# it resolves as a normal relative path under the configured base.
92+
uri = "./#{uri}" if (@options.base_uri? || @options.persistent?) && uri.start_with?("//")
93+
8694
if @options.base_uri? && uri !~ HTTP_OR_HTTPS_RE
87-
# A leading "//" makes the input a protocol-relative (network-path)
88-
# reference per RFC 3986 §4.2 / §5.2. Passing it to base.join /
89-
# URI#merge would replace the base authority with the input's host,
90-
# turning a base_uri-scoped request into one to an arbitrary host.
91-
# Prepend "./" so it resolves as a normal relative path under base.
92-
uri = "./#{uri}" if uri.start_with?("//")
9395
uri = resolve_against_base(uri)
9496
elsif @options.persistent? && uri !~ HTTP_OR_HTTPS_RE
95-
# Same hazard against the persistent-host concatenation: a leading
96-
# "//" produces "scheme://persistent//evil/path" which HTTP::URI.parse
97-
# normalises into scheme://evil/path. Force a relative-path prefix.
98-
uri = "./#{uri}" if uri.start_with?("//")
9997
uri = "#{@options.persistent}#{uri}"
10098
end
10199

test/http/request/builder_test.rb

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,7 +660,12 @@ def test_build_with_persistent_and_protocol_relative_does_not_override_host
660660
builder = build_builder(persistent: "http://example.com")
661661
req = builder.build(:get, "//evil.com/leak")
662662

663-
assert_equal "example.com", req.uri.host
663+
# Pin the full URI: concatenation produces "http://example.com.//..."
664+
# whose host is `example.com.`; HTTP::URI#normalize_host strips the
665+
# trailing dot. Asserting on origin/to_s guards against a future
666+
# change to normalize_host that would let the dot leak through.
667+
assert_equal "http://example.com", req.uri.origin
668+
assert_equal "http://example.com///evil.com/leak", req.uri.to_s
664669
refute_equal "evil.com", req.uri.host
665670
end
666671

0 commit comments

Comments
 (0)