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

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")