fix: credentials follow a redirect only back to the exact origin

97b898f gave X-Environment-Key the treatment I believed Go already gave
Authorization. Both halves of that were wrong. Go's rule is laxer than I
described — shouldCopyHeaderOnRedirect permits any subdomain of the
initial host, so api.example → evil.api.example keeps Authorization —
and my own host-only comparison ignored the scheme, so
https://api.example → http://api.example re-sent both secrets in
cleartext on the same host.

checkRedirect now strips Authorization as well as X-Environment-Key
whenever the target is not the exact origin the request began at: same
host, and no downgrade away from https (an upgrade to it is fine). The
policy moved out of the client literal so it can be tested directly —
subdomains and scheme downgrades are awkward to stage against httptest.

Both new protections are mutation-verified. That caught a real defect in
the first version of the test: it looped over the production
secretHeaders slice, so shrinking that slice shrank what the test
checked and the Authorization protection was never actually asserted.
The header names are now literal in the test.

Addresses Themis review on #43 (httpx/client.go:62).

beep boop

Author: khvn26

internal/httpx/client.go

@@ -12,6 +12,7 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptrace"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -54,24 +55,52 @@ func New(userAgent string) *http.Client {
 		rt = &tracer{base: rt, out: os.Stderr}
 	}
 	return &http.Client{
-		Transport: &transport{base: rt, userAgent: userAgent},
-		// Go strips Authorization when a redirect crosses hosts, but copies
-		// custom headers verbatim — and X-Environment-Key can carry a
-		// server-side (ser.) secret. Give it the same treatment. Setting
-		// CheckRedirect replaces the default policy, so the ten-hop cap
-		// comes with it.
-		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-			if len(via) >= 10 {
-				return errors.New("stopped after 10 redirects")
-			}
-			if req.URL.Host != via[0].URL.Host {
-				req.Header.Del("X-Environment-Key")
-			}
-			return nil
-		},
+		Transport:     &transport{base: rt, userAgent: userAgent},
+		CheckRedirect: checkRedirect,
 	}
 }
 
+// maxRedirects mirrors the cap in Go's default policy, which setting
+// CheckRedirect replaces.
+const maxRedirects = 10
+
+// secretHeaders never travel to an origin the request did not start at.
+// Authorization carries the Admin credential; X-Environment-Key can carry a
+// server-side (ser.) environment key, also a secret.
+var secretHeaders = []string{"Authorization", "X-Environment-Key"}
+
+// checkRedirect strips credentials when a redirect leaves the origin the
+// request began at. Go's own rule is laxer in two ways: it forwards
+// Authorization to any subdomain of the initial host (shouldCopyHeaderOnRedirect
+// → isDomainOrSubdomain, so api.example → evil.api.example keeps it), and it
+// applies to Authorization only, copying custom headers verbatim. It also
+// ignores the scheme, so https → http on the same host would re-send both
+// secrets in cleartext. Require the same host and no downgrade instead.
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= maxRedirects {
+		return errors.New("stopped after 10 redirects")
+	}
+	if !sameOrigin(via[0].URL, req.URL) {
+		for _, h := range secretHeaders {
+			req.Header.Del(h)
+		}
+	}
+	return nil
+}
+
+// sameOrigin reports whether credentials may follow a redirect from → to:
+// the same host (exactly — not a subdomain), over the same scheme or an
+// upgrade to https, never a downgrade away from it.
+func sameOrigin(from, to *url.URL) bool {
+	if !strings.EqualFold(from.Host, to.Host) {
+		return false
+	}
+	if strings.EqualFold(from.Scheme, "https") && !strings.EqualFold(to.Scheme, "https") {
+		return false
+	}
+	return true
+}
+
 // envBool reads a boolean switch from the environment: presence alone is not
 // truth, so FLAGSMITH_DEBUG=0 leaves tracing off. Duplicated rather than
 // shared because this package deliberately imports nothing internal; the

internal/httpx/client_test.go

@@ -329,3 +329,71 @@ func TestRedirectStripsEnvironmentKeyAcrossHosts(t *testing.T) {
 		t.Errorf("X-Environment-Key = %q after a same-host redirect, want it kept", v)
 	}
 }
+
+// Credentials follow a redirect only back to the exact origin the request
+// started at. Go's own rule is laxer — it forwards Authorization to any
+// subdomain and ignores the scheme — and it never protects custom headers,
+// so X-Environment-Key needs this too.
+func TestCheckRedirectStripsSecrets(t *testing.T) {
+	cases := []struct {
+		name     string
+		from, to string
+		wantKept bool
+	}{
+		{"same origin", "https://api.example/a", "https://api.example/b", true},
+		{"upgrade to https is fine", "http://api.example/a", "https://api.example/b", true},
+		{"scheme downgrade on the same host", "https://api.example/a", "http://api.example/b", false},
+		{"different host", "https://api.example/a", "https://evil.example/b", false},
+		{"subdomain of the original host", "https://api.example/a", "https://evil.api.example/b", false},
+		{"parent of the original host", "https://api.example/a", "https://example/b", false},
+		{"different port", "https://api.example/a", "https://api.example:8443/b", false},
+		{"host case is not a difference", "https://API.example/a", "https://api.example/b", true},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			// Given a redirect from → to, carrying both secret headers
+			from, err := http.NewRequest(http.MethodGet, c.from, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			to, err := http.NewRequest(http.MethodGet, c.to, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			// Literal names, not secretHeaders: the test must fail if a
+			// header is dropped from the production list.
+			for _, h := range []string{"Authorization", "X-Environment-Key"} {
+				to.Header.Set(h, "secret")
+			}
+
+			// When
+			if err := checkRedirect(to, []*http.Request{from}); err != nil {
+				t.Fatal(err)
+			}
+
+			// Then both headers survive together, or neither does
+			for _, h := range []string{"Authorization", "X-Environment-Key"} {
+				if kept := to.Header.Get(h) != ""; kept != c.wantKept {
+					t.Errorf("%s kept = %v, want %v", h, kept, c.wantKept)
+				}
+			}
+		})
+	}
+}
+
+func TestCheckRedirectCapsHops(t *testing.T) {
+	// Given a chain already at the cap
+	req, err := http.NewRequest(http.MethodGet, "https://api.example/", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	via := make([]*http.Request, maxRedirects)
+	for i := range via {
+		via[i] = req
+	}
+
+	// When / Then — setting CheckRedirect replaces Go's cap, so ours must hold
+	if err := checkRedirect(req, via); err == nil {
+		t.Error("err = nil, want the redirect chain to stop")
+	}
+}