Share OAuthClientProvider across URL strategies#267
Draft
Conversation
Review Summary by QodoAdd interactive OAuth support for remote MCP servers
WalkthroughsDescription• Add --enable-oauth CLI flag for interactive OAuth authentication • Implement browser-based OAuth flow with local callback server on port 3030 • Add InteractiveTokenStorage for persisting tokens per-server in ~/.mcp-scan-oauth/ • Fix FileTokenStorage.set_tokens() and set_client_info() stubs • Wire OAuth through SSE and HTTP transport paths • Add 26 unit tests covering OAuth, token storage, and CLI integration Diagramflowchart LR
CLI["CLI --enable-oauth flag"]
CLI --> InspectArgs["InspectArgs.enable_oauth"]
InspectArgs --> InspectClient["inspect_client"]
InspectClient --> InspectExt["inspect_extension"]
InspectExt --> CheckServer["check_server"]
CheckServer --> GetClient["get_client"]
GetClient --> OAuthProvider["OAuthClientProvider"]
OAuthProvider --> Storage["InteractiveTokenStorage"]
Storage --> Callback["OAuth callback server"]
Callback --> Browser["Browser authorization"]
File Changes2. src/agent_scan/inspect.py
|
Code Review by Qodo
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![qodo-merge-etso[bot]](https://avatars.githubusercontent.com/u/12959162?s=60&v=4){kind=small}
Comment on lines
+21
to
+28
| async def open_browser_to_authorize(authorization_url: str) -> None: | ||
| """Open the authorization URL in the user's default browser. | ||
|
|
||
| Args: | ||
| authorization_url: The OAuth authorization URL to open. | ||
| """ | ||
| logger.info("Opening browser for OAuth authorization: %s", authorization_url) | ||
| webbrowser.open(authorization_url) |
There was a problem hiding this comment.
1. Logs authorization_url value 📘 Rule violation ⛨ Security
open_browser_to_authorize() logs the full OAuth authorization_url, which can include sensitive parameters (e.g., state) and should not be written to logs. This risks leaking secrets via log collection or shared terminals.
Agent Prompt
## Issue description
`open_browser_to_authorize()` logs the full OAuth authorization URL, which may contain sensitive query parameters.
## Issue Context
OAuth authorization URLs commonly include `state` and other parameters that should not be logged.
## Fix Focus Areas
- src/agent_scan/oauth.py[21-28]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+31
to
+107
| async def wait_for_oauth_callback( | ||
| host: str = "localhost", | ||
| port: int = 3030, | ||
| timeout: float = 300.0, | ||
| ) -> tuple[str, str | None]: | ||
| """Start a temporary HTTP server and wait for the OAuth callback. | ||
|
|
||
| Uses a threaded HTTP server so that synchronous callers (e.g. | ||
| ``urllib.request.urlopen``) do not block the async event loop. | ||
|
|
||
| Args: | ||
| host: The hostname to bind the callback server to. | ||
| port: The port to bind the callback server to. | ||
| timeout: Maximum seconds to wait for the callback before raising TimeoutError. | ||
|
|
||
| Returns: | ||
| A tuple of (code, state). state may be None if not provided. | ||
|
|
||
| Raises: | ||
| OAuthCallbackError: If the callback request is missing the ``code`` parameter. | ||
| TimeoutError: If no callback is received within the timeout period. | ||
| """ | ||
| loop = asyncio.get_running_loop() | ||
| result_future: asyncio.Future[tuple[str, str | None]] = loop.create_future() | ||
|
|
||
| class _CallbackHandler(BaseHTTPRequestHandler): | ||
| def do_GET(self) -> None: # noqa: N802 | ||
| parsed = urlparse(self.path) | ||
| if parsed.path != "/callback": | ||
| self.send_response(404) | ||
| self.end_headers() | ||
| return | ||
|
|
||
| params = parse_qs(parsed.query) | ||
| code_list = params.get("code") | ||
| state_list = params.get("state") | ||
|
|
||
| code = code_list[0] if code_list else None | ||
| state = state_list[0] if state_list else None | ||
|
|
||
| if code is None: | ||
| self.send_response(400) | ||
| self.send_header("Content-Type", "text/plain") | ||
| self.end_headers() | ||
| self.wfile.write(b"Missing 'code' parameter") | ||
| if not result_future.done(): | ||
| loop.call_soon_threadsafe( | ||
| result_future.set_exception, | ||
| OAuthCallbackError("OAuth callback missing required 'code' parameter"), | ||
| ) | ||
| return | ||
|
|
||
| self.send_response(200) | ||
| self.send_header("Content-Type", "text/plain") | ||
| self.end_headers() | ||
| self.wfile.write(b"Authorization successful. You can close this window.") | ||
| if not result_future.done(): | ||
| loop.call_soon_threadsafe(result_future.set_result, (code, state)) | ||
|
|
||
| def log_message(self, format: str, *args: object) -> None: | ||
| """Suppress default stderr logging from BaseHTTPRequestHandler.""" | ||
| pass | ||
|
|
||
| server = HTTPServer((host, port), _CallbackHandler) | ||
| server_thread = threading.Thread(target=server.serve_forever, daemon=True) | ||
| server_thread.start() | ||
|
|
||
| try: | ||
| return await asyncio.wait_for(asyncio.shield(result_future), timeout=timeout) | ||
| except asyncio.TimeoutError: | ||
| raise TimeoutError( | ||
| f"OAuth callback was not received within {timeout} seconds" | ||
| ) | ||
| finally: | ||
| server.shutdown() | ||
| server_thread.join(timeout=2) | ||
|
|
There was a problem hiding this comment.
2. wait_for_oauth_callback() too long 📘 Rule violation ☼ Reliability
wait_for_oauth_callback() exceeds the 40-line function length limit, making it harder to reason about and test in isolation. Refactoring into smaller helpers would improve maintainability and reduce future bug risk.
Agent Prompt
## Issue description
`wait_for_oauth_callback()` is a monolithic function well over 40 lines.
## Issue Context
The function currently defines an inner handler class, starts/stops a thread, and performs async waiting/timeout handling all in one block.
## Fix Focus Areas
- src/agent_scan/oauth.py[31-107]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+93
to
+98
| elif enable_oauth and isinstance(server_config, RemoteServer): | ||
| storage = InteractiveTokenStorage(server_url=server_config.url) | ||
| oauth_client_provider = build_oauth_client_provider( | ||
| server_url=server_config.url, | ||
| storage=storage, | ||
| ) |
There was a problem hiding this comment.
4. Oauth storage keyed by path 🐞 Bug ≡ Correctness
InteractiveTokenStorage uses server_config.url as its persistence key, but check_server() mutates server_config.url while probing /mcp and /sse variants. This causes tokens to be stored under different directories for the same logical server, leading to repeated authorization prompts and fragmented token state.
Agent Prompt
### Issue description
Interactive OAuth token persistence is keyed off `server_config.url`, but `check_server()` mutates that URL while trying different protocol/path combinations. This makes the token storage directory vary per attempt, so previously-authorized tokens often won’t be found.
### Issue Context
Users may be forced to re-authorize multiple times for the same server, and multiple per-variant token directories may be created.
### Fix Focus Areas
- Use a stable key for token storage (e.g., `scheme://host:port` from `urlparse(url)`), not the full mutable path.
- Alternatively, capture the original URL before probing and pass a separate stable `oauth_server_id`/`storage_key` into `get_client()`.
### Fix Focus Areas (code locations)
- src/agent_scan/mcp_client.py[80-98]
- src/agent_scan/mcp_client.py[236-276]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
iamcristi
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary