[SAR] APPENG-5146: MCP token scope warning and documentation#197
Conversation
Add a runtime warning when forwarding full-scope JWT tokens to a non-localhost MCP server, and document the future token exchange (RFC 8693) requirement. This raises visibility for production deployments that should use scoped tokens. - Add non-localhost MCP server warning in mcp_headers.py - Add mcp_token_scopes informational setting to config - Add TODO comment for future RFC 8693 implementation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
luis5tb
left a comment
luis5tb
left a comment
There was a problem hiding this comment.
PR Review
Important (3)
mcp_token_scopes is unused — creates false expectations [settings.py:99-110]
The field is defined in settings but never referenced anywhere in the codebase (confirmed by grep — zero hits across src/, tests/, docs/). The description says "When set, a scoped token will be obtained for MCP communication" — present tense, implying it works. An operator who sets MCP_TOKEN_SCOPES=api.console will believe scope restriction is active, when the full-scope JWT is still forwarded unconditionally.
This also breaks the project's pattern: every other scope field (agent_required_scope, agent_allowed_scopes) has a @property parser and actual consumption in auth logic. This field has neither.
-> Either remove the field (move the intent to a ticket/roadmap) or implement the RFC 8693 token exchange that actually consumes it.
Warning fires on every request (log spam) [mcp_headers.py:60]
The hostname check runs inside header_provider(), which is called per-request. In production with a non-localhost MCP server (the standard deployment), this produces identical warnings on every single A2A request — potentially thousands per hour.
-> Move the check to create_mcp_header_provider() (runs once at startup), or use a module-level flag to log only once.
Hostname check has gaps [mcp_headers.py:63]
('localhost', '127.0.0.1', '::1') misses:
- IPv4-mapped IPv6 (
::ffff:127.0.0.1) - Kubernetes service names (
mcp.default.svc.cluster.local) 0.0.0.0
In containerized deployments the MCP sidecar hostname will almost always be a non-localhost DNS name, making the warning fire in every standard deployment — reducing it to noise.
-> Make the trusted-host list configurable, or accept that the warning is meaningless in the standard deployment topology.
Suggestions (2)
No tests for new code path [mcp_headers.py:60-69]
No tests verify the hostname warning fires (or doesn't) under different mcp_server_url values. The existing test uses the default localhost config, so the new code path is never exercised.
-> Add parameterized tests covering localhost variants and non-localhost URLs.
Hostname in warning leaks infrastructure topology [mcp_headers.py:70]
The warning logs parsed.hostname, exposing the MCP server's address in log aggregation systems.
-> Emit the warning without the hostname, or log it at DEBUG level only.
Strengths
- Correctly identifies the real problem (full-scope token forwarding to MCP)
- References the right long-term solution (RFC 8693 token exchange)
- Clean, focused change — no unnecessary refactoring
- Passes linting and type checking, no regressions in existing tests
|
review fixes at yuvalk#5 |
2 participants
Summary
Addresses APPENG-5146.
Add a runtime warning when forwarding full-scope JWT tokens to a non-localhost MCP server, and document the future token exchange (RFC 8693) requirement for production deployments.
Changes
mcp_headers.pywhen forwarding JWT to non-localhost MCP servermcp_token_scopesinformational setting toconfig/settings.pydocumenting intended scope restrictionurlparseimport for MCP URL hostname checkingSAR Reference
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com