fix(net): stop classifying casks from redirect chains the download would reject

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"

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

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

src/net/client.zig

@@ -610,8 +610,6 @@ pub const HttpClient = struct {
         }
     };
 
-    pub const max_head_redirects = 5;
-
     /// Conditional GET — sends `If-None-Match: <etag>` 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();