fix(broker): sanitize OAuth callback error and token-endpoint failures (MCP-1045)

The T12 security review (Codex, request_changes) found two paths that leak
authorization-server-controlled strings into logs and responses, violating the
FR-029/SC-005 'no secret in logs/responses/audit' invariant:

1. The credential connect callback coerced the raw ?error= value only for the
   audit sink, but still logged it verbatim and reflected it into the browser
   redirect's credential_error param. Now the sanitized label is used on all
   three sinks (audit, log, redirect).

2. OAuthConnector.postToken returned the raw token-endpoint response body in its
   error, which is logged on the connect and refresh paths. A hostile/misconfigured
   AS could embed access/refresh tokens or echoed input there. postToken now
   surfaces only the HTTP status plus an allowlisted RFC 6749 §5.2 error code,
   mirroring the RFC 8693 exchanger's sanitizedError.

Adds regression tests covering the redirect/log leak and the token-body leak.

Related #694, Related MCP-1045

Author: Dumbris

docs/features/idp-token-storage.md

@@ -133,7 +133,12 @@ which validates the one-time `state`, exchanges the code, and persists the
 per-user credential (encrypted, `obtained_via=connect_flow`). The credential is
 always stored under the **initiating** user, so the callback cannot be used to
 write into another user's record. The browser then lands on `/ui/` with a
-`credential_connected` / `credential_error` query flag.
+`credential_connected` / `credential_error` query flag. The `credential_error`
+value is always a coerced, secret-free label (e.g. `access_denied`,
+`authorization_denied`); the raw, authorization-server-controlled error string is
+never logged or reflected back into the redirect, and token-endpoint failures
+surface only the HTTP status plus an allowlisted OAuth error code — never the raw
+response body (FR-029 / SC-005).
 
 ## Operational notes
 

internal/serveredition/api/credential_handlers.go

@@ -278,13 +278,15 @@ func (h *CredentialHandlers) callback(w http.ResponseWriter, r *http.Request) {
 	state := q.Get("state")
 
 	// Authorization-server-side error (e.g. user denied consent).
-	// The AS error code is coerced to a fixed label for the audit sink so the
-	// activity log never captures a raw upstream error string (FR-029).
+	// The raw asErr query value is fully AS-controlled and may embed secrets or
+	// echoed input, so the coerced label is the ONLY form ever written to the
+	// audit sink, the application log, or reflected back into the browser
+	// redirect (FR-029 / SC-005).
 	if asErr := q.Get("error"); asErr != "" {
 		auditReason := sanitizeCallbackError(asErr)
 		_ = conn.Deny(state, auditReason)
-		h.logger.Infow("brokered credential connect denied", "server", srv.Name, "reason", asErr)
-		http.Redirect(w, r, credentialConnectRedirect(srv.Name, asErr), http.StatusFound)
+		h.logger.Infow("brokered credential connect denied", "server", srv.Name, "reason", auditReason)
+		http.Redirect(w, r, credentialConnectRedirect(srv.Name, auditReason), http.StatusFound)
 		return
 	}
 

internal/serveredition/api/credential_handlers_test.go

@@ -408,6 +408,43 @@ func TestCredentialsCallback_Denied_UnknownErrorCoerced(t *testing.T) {
 	assert.Equal(t, broker.AuditOutcomeFailure, ev.Outcome)
 }
 
+// TestCredentialsCallback_Denied_RedirectSanitized proves the callback's
+// browser redirect never reflects a raw, AS-controlled error string. The
+// upstream authorization server fully controls the ?error= query value, so a
+// hostile/misconfigured AS could embed secrets or echoed input there; the
+// redirect's credential_error must carry only the coerced label (FR-029/SC-005).
+func TestCredentialsCallback_Denied_RedirectSanitized(t *testing.T) {
+	store := credTestStore(t)
+	srv := brokerHTTPServer("connect-srv", config.AuthBrokerModeOAuthConnect)
+	sink := &testRecordingSink{}
+	h := NewCredentialHandlers(store, []*config.ServerConfig{srv}, sink, zap.NewNop().Sugar())
+	r := credRouter(h, defaultAuthContext())
+
+	// Begin a flow to register a state.
+	connReq := httptest.NewRequest(http.MethodGet, "/api/v1/user/credentials/connect-srv/connect", http.NoBody)
+	connReq.Host = "gw.example.com"
+	connW := httptest.NewRecorder()
+	r.ServeHTTP(connW, connReq)
+	require.Equal(t, http.StatusFound, connW.Code)
+	loc, _ := url.Parse(connW.Header().Get("Location"))
+	state := loc.Query().Get("state")
+
+	const leak = "leaked_secret_AKIAIOSFODNN7EXAMPLE"
+	cbURL := "/api/v1/user/credentials/connect-srv/callback?error=" + url.QueryEscape(leak) + "&state=" + url.QueryEscape(state)
+	cbReq := httptest.NewRequest(http.MethodGet, cbURL, http.NoBody)
+	cbW := httptest.NewRecorder()
+	r.ServeHTTP(cbW, cbReq)
+	require.Equal(t, http.StatusFound, cbW.Code)
+
+	redirect, err := url.Parse(cbW.Header().Get("Location"))
+	require.NoError(t, err)
+	gotErr := redirect.Query().Get("credential_error")
+	assert.Equal(t, "authorization_denied", gotErr,
+		"redirect must carry the coerced label, not the raw upstream error")
+	assert.NotContains(t, cbW.Header().Get("Location"), leak,
+		"redirect must not reflect the raw AS-controlled error string")
+}
+
 func TestCredentials_Unauthenticated(t *testing.T) {
 	store := credTestStore(t)
 	srv := brokerHTTPServer("shared-gh", config.AuthBrokerModeTokenExchange)

internal/serveredition/broker/oauth_connector.go

@@ -331,7 +331,7 @@ func (c *OAuthConnector) postToken(ctx context.Context, form url.Values) (*oauth
 		return nil, fmt.Errorf("oauth connector: read token response: %w", err)
 	}
 	if resp.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("oauth connector: token endpoint returned %d: %s", resp.StatusCode, strings.TrimSpace(string(body)))
+		return nil, sanitizedTokenEndpointError(resp.StatusCode, body)
 	}
 
 	var tok oauthTokenResponse
@@ -344,6 +344,35 @@ func (c *OAuthConnector) postToken(ctx context.Context, form url.Values) (*oauth
 	return &tok, nil
 }
 
+// rfc6749TokenErrorCodes is the closed set of error codes a token endpoint may
+// legitimately return per RFC 6749 §5.2. Any value outside this set is treated
+// as untrusted AS-controlled free text (which could echo secrets or caller
+// input) and is dropped rather than surfaced.
+var rfc6749TokenErrorCodes = map[string]struct{}{
+	"invalid_request":        {},
+	"invalid_client":         {},
+	"invalid_grant":          {},
+	"unauthorized_client":    {},
+	"unsupported_grant_type": {},
+	"invalid_scope":          {},
+}
+
+// sanitizedTokenEndpointError maps a non-200 token-endpoint response onto a safe
+// error that names only the HTTP status and, when present, an allowlisted OAuth
+// error code. The raw response body and error_description are deliberately
+// dropped: a malicious or misconfigured authorization server can embed access
+// tokens, refresh tokens, client details, or echoed request data there, and
+// that error string is logged on the connect and refresh paths (FR-029 /
+// SC-005). This mirrors the RFC 8693 exchanger's sanitizedError.
+func sanitizedTokenEndpointError(status int, body []byte) error {
+	var te tokenErrorResponse
+	_ = json.Unmarshal(body, &te)
+	if _, ok := rfc6749TokenErrorCodes[te.ErrorCode]; ok {
+		return fmt.Errorf("oauth connector: token endpoint returned %d, error %q", status, te.ErrorCode)
+	}
+	return fmt.Errorf("oauth connector: token endpoint returned %d", status)
+}
+
 // credentialFromToken maps a token response into a stored UpstreamCredential.
 // fallbackRefresh is used when the response omits a refresh token (so a
 // non-rotating AS does not drop the user's refresh capability).

internal/serveredition/broker/oauth_connector_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -366,6 +367,53 @@ func TestOAuthConnector_Complete_TokenEndpointError(t *testing.T) {
 	}
 }
 
+// TestOAuthConnector_Complete_TokenEndpointError_NoBodyLeak proves that a
+// non-200 token-endpoint response body — which an upstream AS controls and could
+// stuff with access/refresh tokens or echoed input — never reaches the error
+// returned to (and logged by) callers. Only the HTTP status and an allowlisted
+// OAuth error code may surface (FR-029 / SC-005).
+func TestOAuthConnector_Complete_TokenEndpointError_NoBodyLeak(t *testing.T) {
+	const leak = "super-secret-access-token-AKIAIOSFODNN7EXAMPLE"
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusBadRequest)
+		// A hostile AS reflecting secrets/free-text in both the body and the
+		// error_description field.
+		_, _ = w.Write([]byte(`{"error":"` + leak + `","error_description":"` + leak + `","access_token":"` + leak + `"}`))
+	}))
+	t.Cleanup(srv.Close)
+
+	store := newConnectorTestStore(t)
+	c, _ := NewOAuthConnector(store, connectorTestConfig(srv.URL), zap.NewNop(), nil)
+
+	_, state, _ := c.BuildAuthorizationURL("user-alice")
+	_, err := c.Complete(context.Background(), state, "code")
+	if err == nil {
+		t.Fatal("expected error when token endpoint returns 400")
+	}
+	if strings.Contains(err.Error(), leak) {
+		t.Errorf("error leaks upstream response body: %q", err.Error())
+	}
+	// A non-allowlisted error code must be dropped entirely (only the status remains).
+	if !strings.Contains(err.Error(), "400") {
+		t.Errorf("error should name the HTTP status: %q", err.Error())
+	}
+}
+
+// TestOAuthConnector_TokenEndpointError_AllowlistedCode confirms a standard
+// RFC 6749 §5.2 error code does pass through (so operators still get a useful
+// reason), while everything else is dropped.
+func TestOAuthConnector_TokenEndpointError_AllowlistedCode(t *testing.T) {
+	allow := sanitizedTokenEndpointError(http.StatusBadRequest, []byte(`{"error":"invalid_grant"}`))
+	if !strings.Contains(allow.Error(), "invalid_grant") {
+		t.Errorf("allowlisted code should surface: %q", allow.Error())
+	}
+	drop := sanitizedTokenEndpointError(http.StatusBadRequest, []byte(`{"error":"AKIAIOSFODNN7EXAMPLE"}`))
+	if strings.Contains(drop.Error(), "AKIAIOSFODNN7EXAMPLE") {
+		t.Errorf("non-allowlisted code must be dropped: %q", drop.Error())
+	}
+}
+
 func TestNewOAuthConnector_Validation(t *testing.T) {
 	store := newConnectorTestStore(t)
 	base := connectorTestConfig("https://idp/token")