From aad0eaa67b07170b0faf5220a3ef25e632c5161d Mon Sep 17 00:00:00 2001 From: Lakhan Samani Date: Sat, 15 Aug 2026 15:25:45 +0530 Subject: [PATCH] chore(http): refuse redirects on spec-defined fetches, parse max-age strictly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four outbound fetches build an SSRF-hardened client; only two set a redirect policy. The pinned dialer already makes a redirect unable to reach a new host, so this is consistency, not a hole — but a policy present at half the call sites is one relaxed dialer away from being one. Applied to the two that retrieve a spec-defined resource which must be AT the URL named: - backchannel_logout: Go re-issues a 302'd POST as a bodyless GET, so a redirecting RP was already not receiving the logout. Now visible. - clientmetadata: a hop away from the client_id URL serves a document for a different identifier. Already rejected incidentally by the client_id-must-match-its-URL check; now rejected deterministically, matching the sibling JWKS fetch. Set on a copy of the client rather than in place: fetchViaClient is also the injected-test-client seam, and mutating a caller's value is a side effect whichever path is hotter. The copy keeps production and the test seam on one policy. Left following redirects, with the reason recorded at both sites: webhook delivery and the admin test-endpoint probe. Those URLs are the operator's own, a same-host path redirect is followed today and its final status recorded in WebhookLog.HttpStatus, and changing that would alter recorded behaviour for existing deployments to close nothing. Separately, cacheTTL used fmt.Sscanf, which stops at the first non-digit and still reports success — "max-age=600junk" parsed as 600. strconv.Atoi rejects it. The clamp already made every outcome safe, so no live defect. --- internal/clientmetadata/clientmetadata.go | 23 +++++++- .../clientmetadata/clientmetadata_test.go | 59 +++++++++++++++++++ internal/events/events.go | 9 +++ internal/service/admin_webhooks.go | 6 ++ internal/token/backchannel_logout.go | 7 +++ 5 files changed, 101 insertions(+), 3 deletions(-) diff --git a/internal/clientmetadata/clientmetadata.go b/internal/clientmetadata/clientmetadata.go index 45b19136c..a819b3983 100644 --- a/internal/clientmetadata/clientmetadata.go +++ b/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 } } diff --git a/internal/clientmetadata/clientmetadata_test.go b/internal/clientmetadata/clientmetadata_test.go index 9627c4026..0b00a1331 100644 --- a/internal/clientmetadata/clientmetadata_test.go +++ b/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. // diff --git a/internal/events/events.go b/internal/events/events.go index 76ac00abb..44aba49f2 100644 --- a/internal/events/events.go +++ b/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) diff --git a/internal/service/admin_webhooks.go b/internal/service/admin_webhooks.go index e26cb9133..6edc1f2b2 100644 --- a/internal/service/admin_webhooks.go +++ b/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") diff --git a/internal/token/backchannel_logout.go b/internal/token/backchannel_logout.go index 49f269c1c..d88651b1f 100644 --- a/internal/token/backchannel_logout.go +++ b/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