Refuse OIDC redirect downgrades; stop logging provider error text

The provider HTTP client now has a redirect policy: every redirect target
must pass secureurl.Check, and an https:// request never follows a
redirect to plain http://, so a 307/308 from the token endpoint cannot
resend the client secret in plaintext. Transport errors from the code
exchange no longer include the (possibly provider-supplied) URL.

The callback's provider-error path logs a fixed category instead of the
raw error and error_description query values.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/auth/auth.go

@@ -91,7 +91,7 @@ func New(ctx context.Context, cfg Config, sessions SessionStarter, logger *slog.
 		return nil, err
 	}
 
-	client := &http.Client{Timeout: providerTimeout}
+	client := &http.Client{Timeout: providerTimeout, CheckRedirect: checkRedirect}
 	provider, err := oidc.NewProvider(oidc.ClientContext(ctx, client), cfg.Issuer)
 	if err != nil {
 		return nil, fmt.Errorf("auth: OIDC discovery: %w", err)
@@ -149,6 +149,26 @@ func checkEndpoint(name, raw string) error {
 	return nil
 }
 
+// maxRedirects matches net/http's default redirect limit.
+const maxRedirects = 10
+
+// checkRedirect is the provider client's redirect policy. On a 307 or 308 to
+// the same host Go resends the body and Authorization header, which carry the
+// code and client secret, so every target must pass secureurl.Check and an
+// https:// request never follows a redirect to plain http://, even loopback.
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= maxRedirects {
+		return fmt.Errorf("stopped after %d redirects", maxRedirects)
+	}
+	if err := secureurl.Check(req.URL); err != nil {
+		return fmt.Errorf("redirect refused: %w", err)
+	}
+	if via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme != "https" {
+		return errors.New("redirect refused: https:// to plain http:// downgrade")
+	}
+	return nil
+}
+
 // Mount registers GET LoginPath and GET CallbackPath on r.
 func (a *Auth) Mount(r chi.Router) {
 	r.Get(LoginPath, a.login)
@@ -203,8 +223,10 @@ func (a *Auth) callback(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "sign-in state mismatch; start again", http.StatusBadRequest)
 		return
 	}
-	if e := q.Get("error"); e != "" {
-		a.logger.WarnContext(ctx, "identity provider refused sign-in", "error", e, "description", q.Get("error_description"))
+	if q.Get("error") != "" {
+		// error and error_description are provider-supplied text; only the
+		// fixed category is logged.
+		a.logger.WarnContext(ctx, "identity provider returned an authorization error")
 		http.Error(w, "sign-in was refused", http.StatusUnauthorized)
 		return
 	}
@@ -268,9 +290,9 @@ func exchangeError(err error) error {
 	case errors.As(err, &retrieve):
 		return fmt.Errorf("exchange code: token endpoint rejected the request (HTTP %d)", retrieve.Response.StatusCode)
 	case errors.As(err, &transport):
-		// Request failures never carry the response; the URL is the token
-		// endpoint from discovery.
-		return fmt.Errorf("exchange code: token request failed: %w", transport)
+		// Request failures never carry the response. The URL is dropped: after
+		// a refused redirect it is the provider-supplied Location.
+		return fmt.Errorf("exchange code: token request failed: %s: %w", transport.Op, transport.Err)
 	default:
 		return errors.New("exchange code: token endpoint returned an unusable response")
 	}

internal/auth/auth_test.go

@@ -21,6 +21,7 @@ import (
 	"slices"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -571,15 +572,38 @@ func TestCallbackTamperedLoginCookie(t *testing.T) {
 	}
 }
 
+// The provider's error and error_description are untrusted text and never
+// reach the logs; only the fixed category does.
 func TestCallbackProviderError(t *testing.T) {
 	t.Parallel()
+	const (
+		errorMarker       = "provider-error-marker"
+		descriptionMarker = "description-marker-" + testClientSecret
+	)
 	h := newHarness(t)
 	l := h.login(t, LoginPath)
-	q := url.Values{"error": {"access_denied"}, "state": {l.query.Get("state")}}
+	q := url.Values{
+		"error":             {errorMarker},
+		"error_description": {descriptionMarker},
+		"state":             {l.query.Get("state")},
+	}
 	resp := h.get(CallbackPath+"?"+q.Encode(), l.cookie)
 	if resp.StatusCode != http.StatusUnauthorized {
 		t.Errorf("callback status = %d, want 401", resp.StatusCode)
 	}
+	assertLoginCookieCleared(t, resp)
+	if n := len(h.sessions.started()); n != 0 {
+		t.Errorf("started %d sessions, want 0", n)
+	}
+	logs := h.logs.String()
+	for _, marker := range []string{errorMarker, descriptionMarker} {
+		if strings.Contains(logs, marker) {
+			t.Errorf("logs contain provider text %q: %s", marker, logs)
+		}
+	}
+	if !strings.Contains(logs, "identity provider returned an authorization error") {
+		t.Errorf("logs = %q, want the authorization error category", logs)
+	}
 }
 
 func TestCallbackSessionError(t *testing.T) {
@@ -775,3 +799,103 @@ func TestNewRejectsRemoteHTTPEndpoint(t *testing.T) {
 		})
 	}
 }
+
+// A token endpoint on https:// that redirects the code exchange to plain
+// http:// must not receive a follow-up request: Go would resend the client
+// secret in the Authorization header. The plaintext target is loopback, so
+// only the downgrade rule stops it.
+func TestCallbackRefusesTokenRedirectToHTTP(t *testing.T) {
+	t.Parallel()
+	for _, status := range []int{http.StatusTemporaryRedirect, http.StatusPermanentRedirect} {
+		t.Run(http.StatusText(status), func(t *testing.T) {
+			t.Parallel()
+			var plaintextHits atomic.Int32
+			plaintext := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				plaintextHits.Add(1)
+				http.Error(w, "unreachable", http.StatusTeapot)
+			}))
+			t.Cleanup(plaintext.Close)
+			var tlsHits atomic.Int32
+			tlsToken := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				tlsHits.Add(1)
+				http.Redirect(w, r, plaintext.URL+"/token", status)
+			}))
+			t.Cleanup(tlsToken.Close)
+
+			p := newFakeProvider(t)
+			p.discoveryOverrides = map[string]any{"token_endpoint": tlsToken.URL + "/token"}
+			sessions := &memorySessions{}
+			logs := &syncBuffer{}
+			a, err := New(context.Background(), Config{
+				Issuer:       p.issuer(),
+				ClientID:     testClientID,
+				ClientSecret: testClientSecret,
+				PublicURL:    testPublicURL,
+				SecretKey:    []byte("server-secret-key"),
+			}, sessions, slog.New(slog.NewTextHandler(logs, nil)))
+			if err != nil {
+				t.Fatal(err)
+			}
+			// Trust the test server's certificate; the redirect policy stays.
+			a.client.Transport = tlsToken.Client().Transport
+			r := chi.NewRouter()
+			a.Mount(r)
+			h := &harness{provider: p, sessions: sessions, router: r, logs: logs}
+			l := h.login(t, LoginPath)
+
+			resp := h.callback(l, l.query.Get("state"), "unused")
+
+			if resp.StatusCode != http.StatusUnauthorized {
+				t.Errorf("callback status = %d, want 401", resp.StatusCode)
+			}
+			if n := tlsHits.Load(); n != 1 {
+				t.Errorf("https token endpoint hit %d times, want 1", n)
+			}
+			if n := plaintextHits.Load(); n != 0 {
+				t.Errorf("plaintext endpoint hit %d times, want 0", n)
+			}
+			if n := len(sessions.started()); n != 0 {
+				t.Errorf("started %d sessions, want 0", n)
+			}
+			if got := logs.String(); !strings.Contains(got, "downgrade") {
+				t.Errorf("logs = %q, want the downgrade refusal", got)
+			}
+		})
+	}
+}
+
+func TestCheckRedirect(t *testing.T) {
+	t.Parallel()
+	request := func(raw string) *http.Request {
+		req, err := http.NewRequest(http.MethodPost, raw, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return req
+	}
+	tests := []struct {
+		from, to string
+		ok       bool
+	}{
+		{"https://auth.example.com/token", "https://auth2.example.com/token", true},
+		{"http://127.0.0.1:9000/token", "http://localhost:9001/token", true},
+		{"http://127.0.0.1:9000/token", "https://auth.example.com/token", true},
+		{"https://auth.example.com/token", "http://auth.example.com/token", false},
+		{"https://127.0.0.1:9000/token", "http://127.0.0.1:9001/token", false},
+		{"http://127.0.0.1:9000/token", "http://auth.example.com/token", false},
+		{"https://auth.example.com/token", "ftp://auth.example.com/token", false},
+	}
+	for _, tc := range tests {
+		err := checkRedirect(request(tc.to), []*http.Request{request(tc.from)})
+		if (err == nil) != tc.ok {
+			t.Errorf("redirect %s -> %s: error = %v, want ok=%v", tc.from, tc.to, err, tc.ok)
+		}
+	}
+	via := make([]*http.Request, maxRedirects)
+	for i := range via {
+		via[i] = request("https://auth.example.com/token")
+	}
+	if err := checkRedirect(request("https://auth.example.com/token"), via); err == nil {
+		t.Errorf("redirect after %d hops: error = nil, want limit", maxRedirects)
+	}
+}