fix(broker): sanitize OAuth callback error & token-endpoint failures (MCP-1045)#739
Conversation
…s (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
Deploying mcpproxy-docs with
|
| Latest commit: |
4cd64be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2f3271f9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp1045-oauth-error-leak.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27859650989 --repo smart-mcp-proxy/mcpproxy-go
|
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gatekeeper approval — review verdict: ACCEPT (by KimiReviewer, model-diverse fallback).
This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of KimiReviewer — the model-diverse reviewer-fallback reviewer of record (verdict lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.
Auto-approved per Model B (MCP-1249) + reviewer-fallback (MCP-3066).
2 participants
Summary
Addresses the MCP-1045 / T12 security review verdict (Codex
request_changes) on the credential-broker (spec 074). Two paths leaked authorization-server-controlled strings into logs/responses, violating the FR-029 / SC-005 no secret in logs/responses/audit invariant. PR #694 was already merged, so this is the follow-up fix; the Paperclip comment remains the review-of-record.Finding 1 — connect-callback raw error leak
internal/serveredition/api/credential_handlers.gocoerced the raw?error=query value only for the audit sink, but still:reason), andcredential_errorparam.Both now use the sanitized label (
sanitizeCallbackError). An upstream AS fully controls this value and could embed secrets or echoed input.Finding 2 — token-endpoint raw body leak
internal/serveredition/broker/oauth_connector.go::postTokenreturned the raw token-endpoint response body in its error, which is logged on the connect path (credential_handlers.go) and the refresh path (credential_resolver.go). A hostile/misconfigured AS can embed access/refresh tokens, client details, or echoed request data there.postTokennow returns only the HTTP status plus an allowlisted RFC 6749 §5.2 OAuth error code (via newsanitizedTokenEndpointError), mirroring the RFC 8693 exchanger's existingsanitizedError. Non-allowlisted codes and the raw body/error_descriptionare dropped.Tests (TDD — written failing first)
TestCredentialsCallback_Denied_RedirectSanitized— redirectcredential_errorcarries the coerced label, never the raw AS string.TestOAuthConnector_Complete_TokenEndpointError_NoBodyLeak— a token-stuffed error body never reaches the returned error.TestOAuthConnector_TokenEndpointError_AllowlistedCode— allowlisted codes pass through; others are dropped.Verification
go test -tags server ./internal/serveredition/broker ./internal/serveredition/api ./internal/transport✅go test -tags server -race ./internal/serveredition/broker ./internal/serveredition/api✅go build -tags server ./cmd/mcpproxyandgo build ./cmd/mcpproxy✅golangci-lint --build-tags server).Docs
docs/features/idp-token-storage.mdupdated to state the connect error flag is a coerced, secret-free label.Related #694