Feat/oauth resolution - #452
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36116019 | Triggered | Bearer Token | 81047b5 | tests/unit/test_redact.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
A potential secret has been detected in this pull request. Please reference the following documentation for next steps:
- How to Triage a Finding
- How to Handle a False Positive
- Secret Leakage Runbook - use only when the finding is a confirmed true positive.
Click on the hyperlinked GitGuardian ID in the table above for more information on the detection. Team leaders can review ignore approval requests via the GitGuardian ID as well.
If you have any questions, please contact your ProdSec partner.
Not your repo? Get access to the GitGuardian issue by following these steps.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
PR Summary by QodoAdd native MCP OAuth and single-server scan targeting
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
1.
|
![qodo-merge-etso[bot]](https://avatars.githubusercontent.com/u/12959162?s=60&v=4){kind=small}
| await ensure_fresh_token(store, url) | ||
| return OAuthClientProvider( |
There was a problem hiding this comment.
7. Sdk bypasses refresh guards 🐞 Bug ⛨ Security
After ensure_fresh_token refuses an insecure endpoint or redirect, _resolve_scan_oauth_provider still returns the SDK OAuth provider with the stored refresh token and client secret; the store's own documentation confirms that provider can then perform its own unguarded refresh. This bypasses both hardening controls and can send long-lived credentials over plaintext or through a redirect.
Agent Prompt
## Issue description
The scan returns an OAuth provider containing refresh credentials even when the proactive refresh security checks refuse the endpoint, allowing the SDK refresh path to bypass HTTPS and redirect protections.
## Issue Context
The provider must not receive usable refresh credentials/client secrets unless every token request it can make is subject to the same endpoint and redirect policy.
## Fix Focus Areas
- src/agent_scan/mcp_client.py[67-85]
- src/agent_scan/oauth_store.py[426-434]
- src/agent_scan/oauth_store.py[453-484]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Fixed in 67bb08d: PersistentTokenStorage.get_tokens()/get_client_info() now withhold the refresh token and client secret from the SDK provider whenever the stored token endpoint fails is_secure_token_url — the same check ensure_fresh_token already applies — so the SDK's own internal refresh can no longer bypass that guard. The access token still flows through, so the connection attempt itself is unaffected. Covered by test_persistent_storage_withholds_refresh_credentials_for_insecure_endpoint.
There was a problem hiding this comment.
Correction to my reply above: 67bb08d closes the insecure-endpoint half only — when entry.token_url fails is_secure_token_url, the SDK now gets no refresh token at all, so it can't attempt a refresh in that case.
It does not close the redirect half this finding also names. If the stored token_url is secure but that endpoint responds to the SDK's own refresh POST with a 307/308, the SDK sends that request through the scan path's httpx.AsyncClient(..., follow_redirects=True) (mcp_client.py's streamablehttp_client_without_session) and would follow the redirect and resend the refresh token/client secret — the same risk ensure_fresh_token sets follow_redirects=False to avoid for its own refresh POST. That gap is pre-existing (documented in ensure_fresh_token's docstring as "not covered here and is tracked as a follow-up," predating this PR) and remains open; my fix didn't address it. Leaving this thread open rather than marking it fully resolved.
- oauth_flow.py: add missing type: ignore on the monkey-patched HTTPServer.callback_received access - cli.py: cast --server-type to its Literal type and annotate unresolved_paths so mypy resolves the single-server-scan branch - mcp_client.py: ruff-format line reflow - test_redact.py: replace a JWT-shaped fake bearer token fixture (GitGuardian flagged it as a real secret) with a non-JWT-shaped one - test_oauth_store.py: guard the 0600 permission assertion with the same win32 check its sibling tests already use (NTFS has no POSIX mode bits) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- oauth_store.py, oauth_flow.py: normalize_server_url and _transport_strategy sliced a transport suffix off the raw URL string, mangling any query string (e.g. ?tenant=acme). _transport_strategy now delegates to normalize_server_url instead of duplicating the logic. - redact.py: bearer-token redaction only matched "Bearer"/"bearer"; auth schemes are case-insensitive per RFC 7235 s2.1, so "BEARER" or "BeArEr" tokens could leak past the redaction boundary. - pipelines.py: filter_clients_to_server kept every entry matching --server NAME across all clients/config files instead of the first occurrence, so a duplicated name could scan multiple distinct servers. Now matches discover_servers_by_name's first-occurrence-wins policy. - cli.py: mcp-auth always exited 0, even on auth failure or an unknown server name, so scripts couldn't detect failure. It now returns/ propagates a real exit status. - oauth_store.py: PersistentTokenStorage handed the SDK's OAuthClientProvider a live refresh token and client secret even when ensure_fresh_token had refused to refresh against an insecure token endpoint, letting the SDK's own unguarded refresh bypass that protection. It now withholds both when the stored endpoint isn't HTTPS/loopback. - oauth_flow.py: added success/failure-path tests for authenticate_server, previously untested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Two more findings from @qodo-merge-etso's summary that didn't get their own inline threads:
Replies to the 8 items that did get inline threads are posted there. Summary: 5 real bugs fixed with regression tests (query-string mangling in 2 places, case-sensitive bearer-token redaction, duplicate-server-name targeting, |
Thanks for the detailed disposition. The query-string fix is reflected as finding 9 (implemented), including both The line-count objections remain findings 10, 11, and 12 (active). Given the cited precedent and the functions’ orchestration responsibilities, leaving them unchanged is consistent with the repository’s established practice. Your summary also indicates the SDK refresh-guard bypass and missing authentication tests were fixed; the current findings index still shows finding 4 and finding 6 as implemented, so those are accounted for. No further action is needed from me based on this update. |
|
@SH4DY , the security finding is actually from the test for fixing case sensitivity on bearer tokens. Pre-existing. |
SH4DY
left a comment
SH4DY
left a comment
There was a problem hiding this comment.
Overall as discussed on the call: Please check if you can easily store the refresh tokens encrypted instead of plaintext on file.
There was a problem hiding this comment.
@SH4DY , token encrypted. Decryption key is currently in the same location. As discussed, a mechanism for this to be implemented separately
| *path* only, so a query string or fragment (e.g. ``?tenant=acme``) is | ||
| preserved rather than sliced off along with the suffix. | ||
| """ | ||
| split = urlsplit(url) |
There was a problem hiding this comment.
normalize_server_url strips /mcp and /sse to build the store key, so https://host/mcp, https://host/sse and https://host/ all share one credential. I authenticated only /mcp and a different service at /sse on the same origin received the bearer. Key on the full URL and handle probe aliasing by trying candidate keys instead.
| """ | ||
| store = OAuthTokenStore() | ||
| entry = store.get(url) | ||
| if entry is None and token is not None: |
There was a problem hiding this comment.
seeding the store from --mcp-oauth-tokens-path silently copies a caller-supplied access + refresh token into ~/.mcp-scan/oauth-tokens.json, this needs to be documented in docs/
| setup_scan_parser(mcp_auth_parser, add_files=False) | ||
| add_target_arguments(mcp_auth_parser, positional=True, include_type=False) | ||
| mcp_auth_parser.add_argument( | ||
| "--all-unauthenticated", action="store_true", help="Authenticate every discovered remote MCP server" |
There was a problem hiding this comment.
Whats the intention of this flag? The name and help don't really match
| ) | ||
| else: | ||
| logger.warning("Authenticated %s but no token endpoint was discovered", url) | ||
| return AuthResult(ok=True, server_url=normalize_server_url(url)) |
There was a problem hiding this comment.
returns ok=True whenever the MCP session initializes, so a server needing no auth prints : authenticated with nothing written to the store. Gate success on a token having been persisted.
Let unattended scans authenticate to OAuth-protected remote MCP servers, and add an `mcp-auth` command for the one-time interactive authorization. Removes the manual mcp-remote + hand-assembled-token-file workaround. - oauth_store.py (new): read-write token store at ~/.mcp-scan/oauth-tokens.json (0600), keyed by normalized server URL, with proactive refresh + write-back so a token authenticated once survives across unattended scan invocations. - oauth_flow.py (new) + `mcp-auth` CLI command: browser + 127.0.0.1 loopback OAuth authorization; persists the token for scans to consume. The only interactive path. - mcp_client.py: scan path is a non-interactive consumer of the store over both HTTP and SSE transports; never prompts, degrades to auth_failed when there is no valid credential. - redact.py: scrub bearer tokens from uploaded server_output (defense in depth). Credentials stay on the machine; nothing new is sent to the platform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- create the token file 0600 before writing, not after (was 0644 during write) - stop the debug helper printing access/refresh tokens and client secrets - do not follow redirects on the token exchange (httpx re-sends the body on 307/308) - require HTTPS or loopback for the token endpoint before sending a refresh token
- oauth_flow.py: add missing type: ignore on the monkey-patched HTTPServer.callback_received access - cli.py: cast --server-type to its Literal type and annotate unresolved_paths so mypy resolves the single-server-scan branch - mcp_client.py: ruff-format line reflow - test_redact.py: replace a JWT-shaped fake bearer token fixture (GitGuardian flagged it as a real secret) with a non-JWT-shaped one - test_oauth_store.py: guard the 0600 permission assertion with the same win32 check its sibling tests already use (NTFS has no POSIX mode bits) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- oauth_store.py, oauth_flow.py: normalize_server_url and _transport_strategy sliced a transport suffix off the raw URL string, mangling any query string (e.g. ?tenant=acme). _transport_strategy now delegates to normalize_server_url instead of duplicating the logic. - redact.py: bearer-token redaction only matched "Bearer"/"bearer"; auth schemes are case-insensitive per RFC 7235 s2.1, so "BEARER" or "BeArEr" tokens could leak past the redaction boundary. - pipelines.py: filter_clients_to_server kept every entry matching --server NAME across all clients/config files instead of the first occurrence, so a duplicated name could scan multiple distinct servers. Now matches discover_servers_by_name's first-occurrence-wins policy. - cli.py: mcp-auth always exited 0, even on auth failure or an unknown server name, so scripts couldn't detect failure. It now returns/ propagates a real exit status. - oauth_store.py: PersistentTokenStorage handed the SDK's OAuthClientProvider a live refresh token and client secret even when ensure_fresh_token had refused to refresh against an insecure token endpoint, letting the SDK's own unguarded refresh bypass that protection. It now withholds both when the stored endpoint isn't HTTPS/loopback. - oauth_flow.py: added success/failure-path tests for authenticate_server, previously untested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… process's environment
- utils.py: a set-but-empty env var (e.g. AUTH_HEADER=) now counts as
unset too, so it warns and leaves the placeholder unexpanded instead
of silently substituting a blank credential.
- utils.py: restore the docstring's --verbose qualifier -- default
logging suppresses the warning, so "fails loudly" overstated it.
- consent.py: the interactive consent prompt now shows a whole-value
${NAME} placeholder literally instead of masking it as ***, since the
placeholder itself isn't a secret -- it discloses which of the user's
own env vars a server is about to read before they approve it.
- docs/scanning.md: promote the new section out of the version-specific
data-sharing list, document that a scanned config's env block can now
pull arbitrary variables from the scanning user's shell (not just
values written on disk), and note there's no escape syntax for a
literal ${NAME}.
- test_mcp_client.py: assert args stay unexpanded (only env is), the
one previously-unpinned scope boundary from the plan.
- test_consent.py: new, covers _render_env_redacted's masked vs.
placeholder-literal branches (this module had no tests before).
67bb08d to
6058c4d
Compare
3 participants
Title: Native OAuth support for remote MCP servers, single-server targeting, and credential hardening
Summary
Adds native OAuth credential handling so unattended scans can authenticate to OAuth-protected remote MCP servers, replacing the manual mcp-remote + hand-assembled-token-file workaround. Includes a new mcp-auth command for one-time interactive authorization (browser + loopback flow), a persistent token store at ~/.mcp-scan/oauth-tokens.json with proactive refresh, and bearer-token redaction on uploaded server output.
Adds --server, --url, and --server-type to scan/inspect for targeting exactly one MCP server (skipping every other server and all skills), plus prefixed positionals (streamable-https:, npm:, pypi:, oci:, …) for scanning a server directly from a URL or package with no config file needed.
Hardens the OAuth token store: token files are now created 0600 before writing (previously 0644 during the write window), the debug helper no longer prints access/refresh tokens or client secrets, the token exchange no longer follows redirects (avoids httpx re-sending the request body on 307/308), and refresh tokens are only sent to HTTPS or loopback endpoints.
Test plan
mcp-auth completes the browser/loopback flow and persists a usable token
scan --server NAME / scan --url URL --server-type http target a single server and skip skills
scan streamable-https:host/path resolves without a config file
Unattended scan picks up and refreshes a previously stored OAuth token
Token file permissions are 0600 immediately on creation
Debug helper output contains no tokens/secrets
uv run pytest passes (new coverage in test_oauth_store.py, test_debug_mcp_auth.py, test_single_server_scan.py, test_mcp_client.py, test_cli_parsing.py)