chore(http): refuse redirects on spec-defined fetches, parse max-age strictly

internal/clientmetadata/clientmetadata.go

@@ -29,6 +29,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -248,7 +249,22 @@ func (p *Provider) fetchViaClient(ctx context.Context, clientID string, client *
 	}
 	req.Header.Set("Accept", "application/json")
 
-	resp, err := client.Do(req)
+	// Refuse redirects, matching the JWKS and OIDC-discovery fetches. The
+	// document must be AT the client_id URL, so a hop away from it is a document
+	// for a different identifier, and SafeHTTPClient pins the dial to the
+	// validated IP — a redirect elsewhere would re-issue the request against
+	// that same address carrying someone else's Host header rather than
+	// reaching the named host at all.
+	//
+	// Set on a COPY, not on the caller's client: fetch() builds a fresh one per
+	// request, but SetHTTPClientForTest injects a client the test owns, and
+	// mutating a caller's value is a side effect regardless of which path is
+	// hotter. The copy costs nothing and keeps both paths on the same policy, so
+	// the test seam actually exercises what production does.
+	noRedirect := *client
+	noRedirect.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }
+
+	resp, err := noRedirect.Do(req)
 	if err != nil {
 		return nil, 0, fmt.Errorf("could not fetch client metadata document")
 	}
@@ -322,8 +338,9 @@ func cacheTTL(header string) time.Duration {
 	for _, part := range strings.Split(header, ",") {
 		part = strings.ToLower(strings.TrimSpace(part))
 		if v, ok := strings.CutPrefix(part, "max-age="); ok {
-			var secs int
-			if _, err := fmt.Sscanf(v, "%d", &secs); err == nil && secs > 0 {
+			// strconv.Atoi, not fmt.Sscanf: Sscanf stops at the first non-digit
+			// and still reports success, so "max-age=60junk" parsed as 60.
+			if secs, err := strconv.Atoi(v); err == nil && secs > 0 {
 				ttl = time.Duration(secs) * time.Second
 			}
 		}

internal/clientmetadata/clientmetadata_test.go

@@ -164,6 +164,15 @@ func TestCacheTTLIsClamped(t *testing.T) {
 		{"max-age=31536000", maxCacheTTL},
 		{"public, max-age=600", 600 * time.Second},
 		{"MAX-AGE=600", 600 * time.Second},
+		// fmt.Sscanf stopped at the first non-digit and still reported success,
+		// so these parsed as 600 / 60 rather than being rejected. The clamp made
+		// every outcome safe, which is why it went unnoticed — a header this
+		// malformed should still fall back to the floor rather than be half-read.
+		{"max-age=600junk", minCacheTTL},
+		{"max-age=600 junk", minCacheTTL},
+		{"max-age=", minCacheTTL},
+		{"max-age=-5", minCacheTTL},
+		{"max-age=abc", minCacheTTL},
 	}
 	for _, tc := range cases {
 		t.Run(tc.header, func(t *testing.T) {
@@ -301,6 +310,56 @@ func TestFetchValidatesDocumentContent(t *testing.T) {
 	})
 }
 
+// TestFetchRefusesRedirects pins the redirect policy.
+//
+// The document must be AT the client_id URL — that identity binding is the whole
+// mechanism CIMD rests on — so a hop away from it serves a document for a
+// different identifier. Following one is also useless in production: the
+// SSRF-hardened client pins the dial to the IP validated for the ORIGINAL host,
+// so a redirect elsewhere re-issues the request against that same address
+// carrying a foreign Host header instead of reaching the named host.
+//
+// The redirect target here serves a perfectly valid document for its own URL, so
+// nothing but the redirect policy itself can make this fail.
+func TestFetchRefusesRedirects(t *testing.T) {
+	var targetURL string
+	target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = fmt.Fprintf(w, `{"client_id":%q,"client_name":"Moved Client","redirect_uris":["https://app.example.com/cb"]}`, targetURL)
+	}))
+	defer target.Close()
+	targetURL = target.URL + "/moved.json"
+
+	redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, targetURL, http.StatusFound)
+	}))
+	defer redirector.Close()
+
+	_, _, err := testProvider(t).fetchViaClient(
+		context.Background(), redirector.URL+"/client.json", redirector.Client())
+	require.Error(t, err, "a redirected document MUST NOT resolve")
+	// The 302 is surfaced as the final response rather than followed.
+	assert.Contains(t, err.Error(), "status 302")
+}
+
+// TestFetchViaClientDoesNotMutateTheCallersClient guards the mechanism behind
+// TestFetchRefusesRedirects. The policy is applied to a copy, because the
+// injected-client seam (SetHTTPClientForTest) hands in a client the caller owns.
+// Setting the field in place would work and still be a side effect on someone
+// else's value.
+func TestFetchViaClientDoesNotMutateTheCallersClient(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	client := srv.Client()
+	require.Nil(t, client.CheckRedirect, "precondition: the caller's client has no redirect policy")
+
+	_, _, _ = testProvider(t).fetchViaClient(context.Background(), srv.URL+"/c.json", client)
+
+	assert.Nil(t, client.CheckRedirect, "fetchViaClient MUST NOT mutate the client it was handed")
+}
+
 // TestIsMetadataClientIDForExcludesTheReservedClient guards against a
 // configuration-triggered change of identity source.
 //

internal/events/events.go

@@ -215,6 +215,15 @@ func (p *provider) deliver(ctx context.Context, log *zerolog.Logger, webhook *sc
 // allow-list, DNS-rebinding host pinning, TLS SNI) is unchanged either way. Mirrors
 // internal/http_handlers/oauth_sso.go's ssoHTTPClient, kept independent so the two
 // escape hatches can never relax each other.
+//
+// Redirects are deliberately still FOLLOWED here, unlike the CIMD document and
+// backchannel-logout fetches which refuse them. Those two retrieve a
+// spec-defined resource that must be AT the URL named; a webhook endpoint is an
+// operator's own URL, and a same-host path redirect is followed today with the
+// final status recorded in WebhookLog.HttpStatus. Refusing them would change
+// that recorded status for existing deployments to close nothing: SafeHTTPClient
+// pins the dial to the validated IP, so a redirect cannot reach a different host
+// regardless. Do not "make this consistent" without a compatibility note.
 func webhookHTTPClient(ctx context.Context, rawURL string, timeout time.Duration, allowPrivate bool) (*http.Client, error) {
 	if allowPrivate {
 		return validators.SafeHTTPClientAllowPrivate(ctx, rawURL, timeout)

internal/service/admin_webhooks.go

@@ -363,6 +363,12 @@ func (p *provider) TestEndpoint(ctx context.Context, meta RequestMetadata, param
 	if skipSSRF {
 		client = &http.Client{Timeout: testEndpointHTTPTimeout}
 	} else {
+		// Redirects are deliberately still followed here — this must behave
+		// exactly like a real delivery, and delivery follows them (see
+		// events.webhookHTTPClient for why, and why the CIMD and
+		// backchannel-logout fetches differ). A "test endpoint" that applied a
+		// stricter policy than delivery would report a failure the live webhook
+		// would not have.
 		client, err = validators.SafeHTTPClient(ctx, params.Endpoint, testEndpointHTTPTimeout)
 		if err != nil {
 			log.Debug().Err(err).Str("endpoint", params.Endpoint).Msg("endpoint URL rejected by SSRF filter")

internal/token/backchannel_logout.go

@@ -86,6 +86,13 @@ func (p *provider) NotifyBackchannelLogout(ctx context.Context, uri string, cfg
 	if err != nil {
 		return fmt.Errorf("backchannel logout SSRF check: %w", err)
 	}
+	// Refuse redirects. Go re-issues a 302'd POST as a GET with no body, so an RP
+	// whose backchannel_logout_uri redirects was already receiving a bodyless GET
+	// and silently not processing the logout — this turns that into a visible
+	// non-2xx rather than a success that did nothing. SafeHTTPClient also pins the
+	// dial to the validated IP, so a redirect could never reach the named host
+	// anyway. Same policy as the JWKS and OIDC-discovery fetches.
+	client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse }
 	resp, err := client.Do(req)
 	if err != nil {
 		return err