diff --git a/scripts/regressions/head-and-get-redirect-budgets-disagree-by-one-hop.sh b/scripts/regressions/head-and-get-redirect-budgets-disagree-by-one-hop.sh new file mode 100755 index 00000000..8d7548ee --- /dev/null +++ b/scripts/regressions/head-and-get-redirect-budgets-disagree-by-one-hop.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# Regression: the HEAD walk must not resolve a chain the download cannot follow. +# +# The two redirect loops carried independent budgets written in different +# units: the download's cap counted redirects (3), the HEAD walk's counted +# requests (5, i.e. 4 redirects). A cask URL sitting behind exactly 4 +# redirects therefore classified successfully - possibly as `.pkg`, raising +# the system-wide `sudo installer -target /` prompt - and then always failed +# at download. +# +# There is now one budget and one place that enforces it, reached by all three +# walks through a shared hop decision. The fixture tests are the honest gate: +# they express every chain length in terms of that budget. The source +# preconditions guard the HEAD walk from growing a second one. +# +# No network, no temp state, well under 30s. + +set -euo pipefail + +ROOT=$(cd "$(dirname "$0")/../.." && pwd) +cd "$ROOT" + +SRC="src/net/client.zig" + +if grep -Eqs -- "max_head_(requests|redirects)" "$SRC"; then + echo "FAIL: the HEAD walk carries a redirect budget of its own again" >&2 + exit 1 +fi + +guards=$(grep -Ec -- "hops >= max_redirects" "$SRC") +if [[ "$guards" -ne 1 ]]; then + echo "FAIL: the redirect budget is enforced in $guards places, not one" >&2 + exit 1 +fi + +# Always rebuild so the binaries reflect current source. Zig's cache makes a +# no-op rebuild cheap. +if ! zig build test-bin >/dev/null 2>&1; then + echo "FAIL: could not build the test binaries (zig build test-bin)" >&2 + exit 1 +fi + +BIN="$ROOT/zig-out/test-bin/net_redirect_auth_test" +OUT=$("$BIN" 2>&1) && STATUS=0 || STATUS=$? +if [[ "$STATUS" -ne 0 ]]; then + echo "FAIL: net_redirect_auth_test - the HEAD walk and the download disagree on redirect depth" >&2 + printf '%s\n' "$OUT" | grep -iE "failed|leaked|panic" >&2 || true + exit 1 +fi + +echo "PASS: the HEAD walk resolves exactly the chains the download can follow" diff --git a/scripts/regressions/head-resolve-cap-exhaustion-returns-unfetched-url.sh b/scripts/regressions/head-resolve-cap-exhaustion-returns-unfetched-url.sh index 028521c9..5c178833 100755 --- a/scripts/regressions/head-resolve-cap-exhaustion-returns-unfetched-url.sh +++ b/scripts/regressions/head-resolve-cap-exhaustion-returns-unfetched-url.sh @@ -1,17 +1,17 @@ #!/usr/bin/env bash # Regression: the manual HEAD redirect loop must fail when it runs out of hops. # -# The loop capped its walk at `max_head_redirects`. When the last response -# inside the cap was still a redirect, the loop swapped `final_url` to the next -# hop and fell out of the `for`, returning that URL as resolved - nothing had -# ever requested it. Falling out and breaking out converged on the same -# `return resolved`, so cap exhaustion was indistinguishable from a terminal -# response. The cask installer then classified the artifact type from that +# When the last response inside the cap was still a redirect, the loop swapped +# `final_url` to the next hop and fell out, returning that URL as resolved - +# nothing had ever requested it. Falling out and breaking out converged on the +# same `return resolved`, so cap exhaustion was indistinguishable from a +# terminal response. The walk now takes every hop from a shared decision that +# errors on exhaustion, before there is a url to adopt. The cask installer then classified the artifact type from that # URL and could raise a `sudo installer -target /` prompt on the strength of # a URL malt never contacted. Both GET loops already error out here. # -# The fixture test is the honest gate. The source precondition guards the -# one-line `for`-`else` from drifting back out. +# The fixture test is the honest gate. The source precondition guards the walk +# from resolving its own hops again. # # No network, no temp state, well under 30s. @@ -22,7 +22,7 @@ cd "$ROOT" SRC="src/net/client.zig" -if ! grep -Eqs -- "^[[:space:]]*\} else return error\.TooManyHttpRedirects;" "$SRC"; then +if ! grep -Eqs -- "nextRedirectHop\(uri, status, response\.head\.location, hops\)\) orelse break" "$SRC"; then echo "FAIL: the HEAD loop still returns an un-fetched url when it runs out of hops" >&2 exit 1 fi diff --git a/scripts/regressions/head-resolved-follows-https-to-http-downgrade.sh b/scripts/regressions/head-resolved-follows-https-to-http-downgrade.sh index 484b8dff..04ba9ae9 100755 --- a/scripts/regressions/head-resolved-follows-https-to-http-downgrade.sh +++ b/scripts/regressions/head-resolved-follows-https-to-http-downgrade.sh @@ -13,7 +13,8 @@ # # The fix routes all three loops through one `nextHopUrl` helper that resolves # the location against the current base and refuses the downgrade in one place, -# so the rule cannot drift between them again. +# so the rule cannot drift between them again. The loops now reach it through +# the shared hop decision, leaving it a single call site. # # Presenting a real https origin needs a TLS fixture, so the guard is judged # through the colocated inline unit tests (`lib_tests`). This script builds and @@ -45,7 +46,11 @@ if grep -Fqs -- "replaceFinalUrl(loc)" "$SRC"; then echo "FAIL: headResolved still follows Location without a scheme check" >&2 exit 1 fi -if [[ "$(grep -Fc -- "nextHopUrl(uri, loc)" "$SRC")" -ne 3 ]]; then +if [[ "$(grep -Fc -- "self.nextHopUrl(base, loc)" "$SRC")" -ne 1 ]]; then + echo "FAIL: the scheme-checking hop resolver is bypassed or duplicated" >&2 + exit 1 +fi +if [[ "$(grep -Fc -- "self.nextRedirectHop(uri, status, response.head.location, hops)" "$SRC")" -ne 3 ]]; then echo "FAIL: not all three redirect loops resolve their hop through the helper" >&2 exit 1 fi diff --git a/src/net/client.zig b/src/net/client.zig index 396126ed..7b02431a 100644 --- a/src/net/client.zig +++ b/src/net/client.zig @@ -610,8 +610,6 @@ pub const HttpClient = struct { } }; - pub const max_head_redirects = 5; - /// Conditional GET — sends `If-None-Match: ` when `if_none_match` /// is non-null and returns a `ConditionalResponse` that surfaces the /// server's ETag plus a `not_modified` flag when the server answered @@ -661,7 +659,8 @@ pub const HttpClient = struct { // Every exit below returns an error rather than what the walk reached // so far: a partial walk is indistinguishable from a resolved url, and // the caller picks a cask's artifact type from it. - for (0..max_head_redirects) |_| { + var hops: usize = 0; + while (true) : (hops += 1) { const uri = std.Uri.parse(resolved.final_url) catch return error.RequestFailed; var req = self.client.request(.HEAD, uri, .{ @@ -681,15 +680,10 @@ pub const HttpClient = struct { } const status: u16 = @intFromEnum(response.head.status); - if (!isFollowableRedirect(status)) break; - - const loc = response.head.location orelse return error.HttpRedirectLocationMissing; - const next = try self.nextHopUrl(uri, loc); + const next = (try self.nextRedirectHop(uri, status, response.head.location, hops)) orelse break; defer self.allocator.free(next); try resolved.replaceFinalUrl(next); - // No `break` above means every hop redirected, so the cap ran out - // mid-walk and `final_url` names a url nothing ever requested. - } else return error.TooManyHttpRedirects; + } return resolved; } @@ -935,9 +929,11 @@ pub const HttpClient = struct { }; } - /// Max redirects followed on a credentialed GET — matches stdlib's default - /// so download depth is unchanged by taking over redirect handling. - const max_get_redirects: usize = 3; + /// Redirect budget for every walk - matches stdlib's default so download + /// depth is unchanged by taking over redirect handling. The HEAD walk that + /// classifies a cask shares it, so it cannot resolve a chain the download + /// would reject. + pub const max_redirects: usize = 3; const GetOutcome = struct { status: u16, @@ -982,6 +978,24 @@ pub const HttpClient = struct { return next; } + /// The redirect decision every walk shares, so the rules cannot drift + /// between them. A missing `Location` and a spent budget both error here + /// rather than reading as terminal - either one would otherwise hand back + /// a url nothing ever requested as if it were resolved. Null means + /// terminal; caller owns the url. + fn nextRedirectHop( + self: *HttpClient, + base: std.Uri, + status: u16, + location: ?[]const u8, + hops: usize, + ) !?[]const u8 { + if (!isFollowableRedirect(status)) return null; + const loc = location orelse return error.HttpRedirectLocationMissing; + if (hops >= max_redirects) return error.TooManyHttpRedirects; + return try self.nextHopUrl(base, loc); + } + /// Release a request whose response carries no body. stdlib's `deinit` /// decides by method alone, so on a bodiless status it drains until the /// peer closes — on a keep-alive 304 that is a full idle timeout. @@ -1032,12 +1046,7 @@ pub const HttpClient = struct { var response = try req.receiveHead(&redirect_buf); const status: u16 = @intFromEnum(response.head.status); - if (isFollowableRedirect(status)) { - // A redirect without a Location is malformed — fail loud rather - // than hand the redirect's body back to the caller as a "success". - const loc = response.head.location orelse return error.HttpRedirectLocationMissing; - if (hops >= max_get_redirects) return error.TooManyHttpRedirects; - const next = try self.nextHopUrl(uri, loc); + if (try self.nextRedirectHop(uri, status, response.head.location, hops)) |next| { errdefer self.allocator.free(next); if (!credsSurviveRedirect(current, next)) live_creds = &.{}; // Hop committed: no fallible op past here, so the errdefers @@ -1202,14 +1211,13 @@ pub const HttpClient = struct { var response = req.receiveHead(&redirect_buf) catch return error.RequestFailed; const status: u16 = @intFromEnum(response.head.status); - if (isFollowableRedirect(status)) { - const loc = response.head.location orelse return error.HttpRedirectInvalid; - if (hops >= max_get_redirects) return error.TooManyHttpRedirects; - const next = self.nextHopUrl(uri, loc) catch |e| switch (e) { - error.OutOfMemory => return error.OutOfMemory, - error.TlsDowngradeRefused => return error.TlsDowngradeRefused, - else => return error.HttpRedirectInvalid, - }; + const hop = self.nextRedirectHop(uri, status, response.head.location, hops) catch |e| switch (e) { + error.OutOfMemory => return error.OutOfMemory, + error.TlsDowngradeRefused => return error.TlsDowngradeRefused, + error.TooManyHttpRedirects => return error.TooManyHttpRedirects, + else => return error.HttpRedirectInvalid, + }; + if (hop) |next| { errdefer self.allocator.free(next); if (!credsSurviveRedirect(current, next)) live_creds = &.{}; req.deinit(); diff --git a/tests/net_redirect_auth_test.zig b/tests/net_redirect_auth_test.zig index 3349eef4..ceb1fa9c 100644 --- a/tests/net_redirect_auth_test.zig +++ b/tests/net_redirect_auth_test.zig @@ -420,7 +420,7 @@ test "headResolved reports an exhausted redirect walk instead of an un-fetched u // The walk sends exactly one request per hop in the cap pre- and post-fix, // so the hop thread always drains and the join never hangs. - const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_head_redirects }); + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 1 }); var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); @@ -438,10 +438,10 @@ test "headResolved reports an exhausted redirect walk instead of an un-fetched u try std.testing.expectError(error.TooManyHttpRedirects, result); } -test "headResolved resolves a chain that uses the hop cap exactly" { - // Guards against over-correcting: the last hop inside the cap may still be - // the terminal response. This passes on the pre-fix loop too - it pins a - // legal chain that must keep resolving if the budget is ever retuned. +test "headResolved resolves a chain as long as the download can follow" { + // Guards against over-correcting: a chain the download would follow to the + // end must still classify. Expressed in the download's budget so retuning + // it moves both loops together. var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); defer threaded.deinit(); const io = threaded.io(); @@ -457,9 +457,9 @@ test "headResolved resolves a chain that uses the hop cap exactly" { .listener = &l1, .redirect_to = loc, .status = .found, - .redirects_left = client.HttpClient.max_head_redirects - 1, + .redirects_left = client.HttpClient.max_redirects, }; - const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_head_redirects }); + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 1 }); var url_buf: [64]u8 = undefined; const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); @@ -477,3 +477,169 @@ test "headResolved resolves a chain that uses the hop cap exactly" { try std.testing.expectEqualStrings(loc, resolved.final_url); } + +test "headResolved refuses a chain one hop longer than the download can follow" { + // The window this closes: classifying a cask - possibly as `.pkg`, which + // raises the sudo installer prompt - from a chain the download then + // rejects. One hop past the download's budget must never resolve. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/loop", .{p1}); + var hop1 = Hop{ + .io = io, + .listener = &l1, + .redirect_to = loc, + .status = .found, + .redirects_left = client.HttpClient.max_redirects + 1, + }; + // Serves one request more than the walk may spend, so a loop that follows + // the extra hop reaches a terminal 200 and resolves instead of hanging. + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 2 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + + const result = http.headResolved(url); + http.deinit(); + knock(io, p1); + t1.join(); + + if (result) |r| { + var resolved = r; + resolved.deinit(); + } else |_| {} + try std.testing.expectError(error.TooManyHttpRedirects, result); +} + +test "a download follows a chain the full length of the shared redirect budget" { + // The other half of the invariant: the HEAD walk is only allowed to resolve + // what the download reaches, so the download must actually reach it. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/loop", .{p1}); + var hop1 = Hop{ + .io = io, + .listener = &l1, + .redirect_to = loc, + .status = .found, + .redirects_left = client.HttpClient.max_redirects, + }; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 1 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + + const result = http.getWithHeaders(url, &.{}, null, .transport_only); + http.deinit(); + knock(io, p1); + t1.join(); + + const resp = try result; + defer resp.allocator.free(resp.body); + + try std.testing.expectEqual(@as(u16, 200), resp.status); + try std.testing.expectEqualStrings(blob_body, resp.body); +} + +test "a streaming download follows a chain the full length of the shared redirect budget" { + // The bottle path streams rather than buffers, and it walks redirects on + // its own; without this it is the only walk whose hop following is untested. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/loop", .{p1}); + var hop1 = Hop{ + .io = io, + .listener = &l1, + .redirect_to = loc, + .status = .found, + .redirects_left = client.HttpClient.max_redirects, + }; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, client.HttpClient.max_redirects + 1 }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + + var sink: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer sink.deinit(); + + const status = http.getToWriter(url, &.{}, &sink.writer, null); + http.deinit(); + knock(io, p1); + t1.join(); + + try std.testing.expectEqual(@as(u16, 200), try status); + try std.testing.expectEqualStrings(blob_body, sink.writer.buffered()); +} + +test "a streaming download names an over-long chain rather than calling it malformed" { + // The streaming walk maps hop failures into its own error set, where an + // exhausted budget is one `else` arm away from being reported as a + // malformed redirect - a different fault with a different remedy. + var threaded: std.Io.Threaded = .init(std.testing.allocator, .{}); + defer threaded.deinit(); + const io = threaded.io(); + + var l1 = try bindIp4(io); + defer l1.deinit(io); + const p1 = l1.socket.address.getPort(); + + var loc_buf: [64]u8 = undefined; + const loc = try std.fmt.bufPrint(&loc_buf, "http://127.0.0.1:{d}/loop", .{p1}); + var hop1 = Hop{ + .io = io, + .listener = &l1, + .redirect_to = loc, + .status = .found, + }; + // A pre-body failure is retried, so the fixture has to outlast every + // attempt: each spends the whole budget before giving up. + const attempts = 4; + const t1 = try std.Thread.spawn(.{}, serveCount, .{ &hop1, attempts * (client.HttpClient.max_redirects + 1) }); + + var url_buf: [64]u8 = undefined; + const url = try std.fmt.bufPrint(&url_buf, "http://127.0.0.1:{d}/start", .{p1}); + + var inner: std.http.Client = .{ .allocator = std.testing.allocator, .io = io }; + var http = client.HttpClient.initWith(&inner, io, std.process.Environ.empty, std.testing.allocator); + + var sink: std.Io.Writer.Allocating = .init(std.testing.allocator); + defer sink.deinit(); + + const status = http.getToWriter(url, &.{}, &sink.writer, null); + http.deinit(); + knock(io, p1); + t1.join(); + + try std.testing.expectError(error.TooManyHttpRedirects, status); + try std.testing.expectEqualStrings("", sink.writer.buffered()); +}