PR Review: OAuth Implementation for MCP (roboflow-mcp #51) — 2026-05-25

roboflow-mcp-51-20260525-static.md

PR Review: OAuth Implementation for MCP

Repository: roboflow/roboflow-mcp PR: #51 — OAuth Implementation for MCP Branch: mike/mcp-oauth → main Author: Mike-Medvedev (Michael Medvedev) Review Date: 2026-05-25 Generation Timestamp: 2026-05-25 15:20 America/Sao_Paulo Base Commit: e79d8aeb05cb7c2703dd62d572e48d53df058d1f Head Commit: ded89e9a0c784231df5392b911bbbfc5873bb06c Changed Files: 36 Additions / Deletions: +1464 / -323 PR URL: https://github.com/roboflow/roboflow-mcp/pull/51 Previous Review: roboflow-mcp-51-20260525-143021.md (2026-05-22, head d4145adf)

DISCLAIMER: Generated by Codex.

📋 Summary

This is a follow-up review of the same PR, now at ded89e9a (two new commits since d4145adf: a tightening commit 682b468 and a merge with main). All six issues from the previous review have been resolved. The PR now adds a well-structured OAuth 2.1 flow to the MCP server with proper credential caching, execution-time tool enforcement, and scope alignment.

Previous review status

#	Previous Finding	Status
1	⚠️ Duplicate uncached OAuth workspace lookups	✅ Resolved — custom CredentialWorkspaceCache with LRU bounds, TTL for OAuth, and singleflight deduplication replaces async_lru
2	⚠️ API-key-only tools hidden but not blocked at execution time	✅ Resolved — _raise_if_oauth_call_requires_api_key() in on_call_tool returns 403 before the tool body runs
3	⚠️ model:infer scope advertised but unreachable via OAuth	✅ Resolved — model:infer removed from MCP_REQUIRED_SCOPES
4	🤨 Blank Bearer headers bypass HTTP gate	✅ Resolved — _has_mcp_credential() now delegates to _bearer_token(), which validates non-empty token. Test added
5	🤨 Test cache reset depends on private async_lru internals	✅ Resolved — async-lru dependency removed entirely; custom cache has a public clear() method
6	🤨 get_oauth_compatible_tools() accepts unused _access_token	✅ Resolved — parameter removed

🏗️ Architectural Review

The architecture is now clean and well-layered. Transport-level auth lives in MCPAuthChallengeMiddleware (ASGI). Credential extraction and typing live in roboflow_mcp.auth. The workspace cache is a standalone, testable component in workspace_cache.py. The tool-level OAuth policy is enforced at two points — on_list_tools (catalog filtering) and on_call_tool (execution guard) — creating a consistent contract.

The biggest structural improvement is the elimination of the per-tool-call workspace lookup from logging. _request_log_fields() now records only credential_type, removing the duplicate uncached lookup that was the most significant performance issue in the previous review. Initialize logs still resolve workspace, so correlation is possible at session level.

flowchart LR
    Client["MCP client"] --> Challenge["MCPAuthChallengeMiddleware"]
    Challenge -->|"401 + RFC 9728 metadata"| Client
    Challenge -->|authenticated| FastMCP["FastMCP"]
    FastMCP --> Catalog["on_list_tools"]
    Catalog --> Filter{"OAuth?"}
    Filter -->|yes| Reduced["OAuth-compatible subset"]
    Filter -->|no| Full["All tools"]
    FastMCP --> Call["on_call_tool"]
    Call --> Guard["_raise_if_oauth_call_requires_api_key"]
    Guard -->|"403 if OAuth + API-key-only"| Client
    Guard -->|pass| Tool["tool handler"]
    Tool --> Cache["CredentialWorkspaceCache"]
    Cache -->|"cache hit or singleflight"| WS["workspace slug"]
    Tool --> Platform["Roboflow Platform API"]

Loading

🐛 Code Quality Issues

1. 🤨 on_list_tools catches bare Exception from get_credential()

src/roboflow_mcp/middleware.py:53-55:

try:
    credential = get_credential()
except Exception:
    return tools

The expected failures are ClientRequestError (no credentials) and RuntimeError (no HTTP request context). Catching bare Exception silently swallows unexpected errors (e.g., AttributeError from a bug in get_credential) and returns the full unfiltered tool catalog. This could mask a broken credential path while appearing to work.

This doesn't block merge because the tool body will still enforce auth, but it weakens the signal for future debugging.

Suggested fix — narrow the catch to match the _raise_if_oauth_call_requires_api_key pattern:

try:
    credential = get_credential()
except (ClientRequestError, RuntimeError):
    return tools

2. 🤨 Tool call log events no longer include workspace

src/roboflow_mcp/middleware.py:171-182 — _request_log_fields() previously resolved workspace for every tool call event. It now records only credential_type. This was the correct call when workspace lookups were uncached and duplicated. But now that CredentialWorkspaceCache makes lookups cheap (cache hit on second call), workspace can be added back without the performance cost that motivated removing it.

Suggested fix — add workspace back via the now-cached lookup:

async def _request_log_fields() -> dict[str, object]:
    try:
        request = get_http_request()
    except RuntimeError:
        return {}

    fields: dict[str, object] = {}
    user_agent = request.headers.get("user-agent")
    if user_agent:
        fields["user_agent"] = user_agent

    try:
        credential = get_credential()
        fields["credential_type"] = credential_type(credential)
        fields["workspace"] = await get_workspace(credential)
    except MCPVisibleError as error:
        fields["credential_error_code"] = error.code
    except Exception as error:
        fields["credential_error_type"] = type(error).__name__

    return fields

This is safe because:

• The first get_workspace() call in the tool body populates the cache.
• Even if _request_log_fields runs first, the singleflight ensures only one network call happens.
• After the first request for a given credential, all subsequent lookups are pure cache hits (instant for API keys, TTL-bounded for OAuth).

3. 🤨 Minor: variable naming inconsistency across tool handlers

Some tools were updated to rename api_key → credential (e.g., batch.py, devices.py), while others kept the api_key variable name even though the value now comes from get_credential() (e.g., projects.py:26, models.py:117, workflows.py:31). The code is functionally correct — the variable holds whichever credential was provided — but the mixed naming makes it less obvious that these tools support both OAuth and API key paths.

Not a blocker; the parameter name on rf_api.* functions is still api_key for backwards compatibility, so the local variable name is somewhat forced. Worth a follow-up rename of the api_key parameter in rf_api.* to credential for consistency.

💡 Recommendations

1. Narrow the on_list_tools exception catch to (ClientRequestError, RuntimeError) (see finding #1).
2. Add workspace back to tool call logs — the cache makes it free now (see finding #2).
3. In a follow-up, rename api_key → credential across rf_api.* function signatures and tool handler locals for vocabulary consistency (see finding #3).

🧪 Tests

Test coverage has improved substantially since the previous review:

Well covered:

• test_oauth_auth.py — protected-resource metadata, Bearer challenge, credential precedence, Bearer-vs-query-param formatting, blank Bearer rejection.
• test_tool_catalog.py — OAuth-vs-API-key catalog split, execution-time blocking of API-key-only tools via on_call_tool, scope set assertion, complete tool classification check.
• test_client_cache.py — OAuth token caching with TTL, LRU eviction, singleflight deduplication (both at cache level and through client.get_workspace).
• test_projects_and_data_tools.py — OAuth Bearer header in upload URLs, credential formatting for both auth types.
• test_versions_models_workflows_meta.py — OAuth Bearer formatting for models_train.

Previously noted gaps now closed:

• ✅ OAuth client calling API-key-only tool gets clean 403 at call_tool time.
• ✅ Blank Authorization: Bearer case is tested.
• ✅ Workspace cache behavior is thoroughly tested including TTL expiry and concurrent deduplication.

Remaining minor gap:

• No test for the on_list_tools behavior when get_credential() throws an unexpected exception type (related to finding #1).

⚖️ Verdict

✅ Approve

All six blocking and non-blocking issues from the previous review have been addressed. The workspace cache is well-designed with LRU bounds, TTL for OAuth tokens, and singleflight deduplication. The execution-time enforcement for API-key-only tools is clean. The scope set now matches the reachable OAuth surface. The async-lru dependency is gone along with its test hacks. Test coverage is strong across the critical paths.

The three remaining findings are all 🤨 minor — none affects correctness or merge safety. Ship it.