Skip to content

fix(mcp): fail closed when the tool scope gate cannot reach its auth config - #716

Draft
vishal-bala wants to merge 1 commit into
mainfrom
fix/mcp-auth-scope-prologue
Draft

fix(mcp): fail closed when the tool scope gate cannot reach its auth config#716
vishal-bala wants to merge 1 commit into
mainfrom
fix/mcp-auth-scope-prologue

Conversation

@vishal-bala

Copy link
Copy Markdown
Collaborator

Motivation

The MCP scope gate can be disabled by a rename, silently. Four tool wrappers each carry an identical three-line prologue that reads auth_config off the server, pulls a scope name from it, and hands that to ensure_tool_scope. Every step of that chain tolerates a miss: getattr(server, "auth_config", None) yields None, the conditional expression then yields a None scope, and ensure_tool_scope returns early on a None scope. So renaming the server's auth_config attribute stops read and write scopes being enforced on every tool at once, with no error, no warning and no failing test.

The prologue is also redundant. ensure_tool_scope performs the same getattr lookup itself, so the caller's copy exists only to name which scope field applies.

Changes

The scope name is resolved inside auth.py

ensure_read_scope(server) and ensure_write_scope(server) replace the prologue at all four call sites. Each resolves its own scope field from the server and delegates to ensure_tool_scope, so a wrapper no longer needs to know that a read tool reads read_scope.

        auth_config = getattr(server, "auth_config", None)
        read_scope = auth_config.read_scope if auth_config is not None else None
        ensure_tool_scope(server, read_scope)

becomes

        ensure_read_scope(server)

The field lookup is a bare getattr(auth_config, attribute) with no default, so renaming a field on MCPAuthConfig raises AttributeError rather than resolving to None and turning the gate into a no-op.

An unreachable auth config now fails closed

ensure_tool_scope checks _auth_enabled first and, when auth is wired, treats a missing auth_config as an internal inconsistency rather than a reason to skip the check:

    if not getattr(server, "_auth_enabled", False):
        return

    auth_config = getattr(server, "auth_config", None)
    if auth_config is None:
        raise RedisVLMCPError(..., code=MCPErrorCode.INTERNAL_ERROR, retryable=False)

The tokenless-request exit is unchanged and still returns early. That one is correct: an authenticated HTTP transport rejects tokenless requests before a tool body runs, so a missing token means stdio, where there is no scope to check.

Secondary changes

  • Four call sites reduced to one line each: tools/search.py, tools/upsert.py, tools/list_indexes.py, tools/profiles.py. No wrapper reads an auth_config attribute any more.
  • Four tests in tests/unit/test_mcp/test_auth_scope.py covering scope resolution, the auth-disabled no-op, the unreachable-config failure and the renamed-field failure.

Notes

Both new guards were mutation-checked: reverting the fail-closed raise alone fails exactly one test, and weakening the bare getattr to a defaulted one fails exactly one other. Neither guard is decorative.

ensure_tool_scope keeps its signature and stays public, because it is the right entry point for a caller that has a scope name in hand rather than a server to resolve one from. The new helpers are the preferred call-site form and its docstring says so.

Next Steps

  1. Verify the MCP suites:
uv run pytest tests/unit/test_mcp tests/integration/test_mcp -q

…config

Four tool wrappers carried an identical three-line prologue that read
auth_config off the server, pulled a scope name from it, and passed that
to ensure_tool_scope. Every step tolerated a miss, so renaming the
server's auth_config attribute resolved a None scope and made the gate
return early on every tool at once, silently.

Replace the prologue with ensure_read_scope/ensure_write_scope, which
resolve the scope field inside auth.py via an undefaulted getattr, and
make ensure_tool_scope raise when auth is enabled but its config is
unreachable. The tokenless-request exit is unchanged: an authenticated
HTTP transport rejects those before a tool body runs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:patch Increment the patch version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant