From 4cd64be6a65e6ef575aa8c9733ab598e6fb61040 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Sat, 20 Jun 2026 07:04:04 +0300 Subject: [PATCH] fix(broker): sanitize OAuth callback error and token-endpoint failures (MCP-1045) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/features/idp-token-storage.md | 7 ++- .../serveredition/api/credential_handlers.go | 10 ++-- .../api/credential_handlers_test.go | 37 ++++++++++++++ .../serveredition/broker/oauth_connector.go | 31 +++++++++++- .../broker/oauth_connector_test.go | 48 +++++++++++++++++++ 5 files changed, 127 insertions(+), 6 deletions(-) diff --git a/docs/features/idp-token-storage.md b/docs/features/idp-token-storage.md index 79aefef2..92759f31 100644 --- a/docs/features/idp-token-storage.md +++ b/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 diff --git a/internal/serveredition/api/credential_handlers.go b/internal/serveredition/api/credential_handlers.go index d2dfb908..6605e986 100644 --- a/internal/serveredition/api/credential_handlers.go +++ b/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 } diff --git a/internal/serveredition/api/credential_handlers_test.go b/internal/serveredition/api/credential_handlers_test.go index f1c6c818..bd120c79 100644 --- a/internal/serveredition/api/credential_handlers_test.go +++ b/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) diff --git a/internal/serveredition/broker/oauth_connector.go b/internal/serveredition/broker/oauth_connector.go index 2b6c5476..91480c4a 100644 --- a/internal/serveredition/broker/oauth_connector.go +++ b/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). diff --git a/internal/serveredition/broker/oauth_connector_test.go b/internal/serveredition/broker/oauth_connector_test.go index 982a1bc1..e018d086 100644 --- a/internal/serveredition/broker/oauth_connector_test.go +++ b/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")