Skip to content

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

Open
vishal-bala wants to merge 2 commits into
mainfrom
fix/mcp-auth-scope-prologue
Open

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

Conversation

@vishal-bala

@vishal-bala vishal-bala commented Sep 2, 2026

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

Note

Medium Risk
Changes authentication gating behavior for MCP tools when auth is enabled; misconfiguration now blocks tools rather than running ungated, which is safer but could surface new errors in production if wiring regresses.

Overview
Hardens MCP tool scope enforcement so auth cannot be bypassed silently when server wiring is wrong, and centralizes read/write scope checks at the tool entry points.

When auth is enabled but auth_config is missing on the server, ensure_tool_scope now raises an internal error and refuses to run the tool instead of treating a missing config like “no scope configured.” Scope names are resolved via new ensure_read_scope / ensure_write_scope helpers (using strict attribute lookup on MCPAuthConfig), replacing duplicated three-line prologues in list-indexes, search, profiles, and upsert-records. Unit tests cover helper resolution, fail-closed unreachable config, and loud failure when config field names are renamed.

Reviewed by Cursor Bugbot for commit 586d9d4. Bugbot is set up for automated code reviews on this repo. Configure here.

…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.
@vishal-bala vishal-bala added the auto:patch Increment the patch version when merged label Sep 3, 2026
vishal-bala added a commit that referenced this pull request Sep 9, 2026
…ache (#725)

## Motivation

The LangCache integration suite fails semi-randomly in the Service Tests
job, on pull requests that touch nothing related to LangCache, and a
different test fails each time. A reproducible defect fails the same
test every run, so the pattern itself points at a shared resource, not
at any one branch: #716 saw `test_store_and_check_async` fail on `assert
[]`, its stored entry gone; #718 saw two different tests fail, and a
third on re-run.

Two things combine to cause it.
`TestLangCacheSemanticCacheIntegrationWithAttributes` flushed the entire
managed cache, through `delete()`, `clear()` and `aclear()`, and the
suite runs under `pytest -n auto`, so a flush on one xdist worker wiped
entries another worker had stored moments earlier. The fixtures then
bind to a single `cache_id` from repo secrets, which every CI run
reaches concurrently: pull requests, fork pull requests, pushes to main,
and the nightly cron. Runs on separate branches therefore flushed each
other.

## Changes

### Whole-cache flushes are removed, and blocked from returning

`test_delete_and_clear_alias` is deleted and the trailing `aclear()` is
removed from `test_async_delete_variants`. An autouse fixture patches
`delete`, `adelete`, `clear` and `aclear` to fail the test, so the rule
is now mechanical, not a convention recorded in a docstring.

This trades away live coverage of the flush endpoint, which is a
constraint and not a preference: no cache shared with other runs can
safely be flushed, and a dedicated flushable `cache_id` would collide
identically once two runs used it. The wrappers themselves are four
one-line calls into the SDK and keep their mocked unit coverage; the
flush HTTP path is deliberately untested.

### Every write is scoped and expiring

A per-test `scope` token, from `uuid4().hex[:12]`, is threaded into
every prompt, response and attribute value, so no test can observe or
delete another's entries. Every entry carries a 60-second TTL, which
lets the shared caches drain now that nothing flushes them. The TTL is
passed per call, not set once on the fixtures, because `store()` ignores
a constructor TTL entirely. That defect is filed separately, and the
module docstring records it so the repetition is not tidied away into
the fixtures.

### The TTL-expiry tests no longer depend on result ranking

`num_results` is a client-side slice: `_build_search_kwargs` never sends
`max_results`, and the installed SDK documents its own default as one
result. Raising it therefore buys no headroom against a concurrent run's
semantically identical prompt competing for that single slot. Filtering
on a scope-unique attribute does, because the service can only return
entries matching it.

```python
# Before: reads back whichever single entry the service ranks first.
hits = langcache_with_attrs.check(prompt=prompt, num_results=5)

# After: the result set is provably this test's own.
hits = langcache_with_attrs.check(prompt=prompt, attributes=metadata)
```

Both tests also move from a two-second TTL to five, with the sleep from
three seconds to six. Two seconds had to cover a store round trip and a
search round trip against a shared managed service, so the assertion
that the entry exists was racing its own TTL.

Secondary changes:

- Delete-by-attribute assertions tighten from `>= 1` to the number of
entries actually stored, which scope-unique attributes make knowable.
- Scope tokens join punctuation-heavy attribute values with `_` rather
than `-`, which survives percent-encoding and is a RediSearch text
separator.
- The unit-test SDK mock is autospecced, so a renamed method or a
changed signature fails there instead of being auto-vivified.
- Two unit tests were indented into another test and so were never
collected. Dedenting them adds both to the run, and the file now
collects 26, having also lost a name-checking test that autospec
subsumes.
- CONTRIBUTING.md gains a paragraph on namespacing writes to shared,
stateful external services.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Test-only and documentation changes; no production LangCache client
behavior is modified.
> 
> **Overview**
> Stops flaky **LangCache** integration failures caused by many workers
and CI jobs sharing the same managed `cache_id`.
> 
> **Whole-cache flush is removed from live tests** and enforced with an
autouse fixture that makes `delete`/`clear` (sync and async) fail if
called. Integration cases that flushed the cache are dropped; flush
behavior stays covered only in **unit** mocks (now **autospecced**
against the real SDK). Two previously nested unit tests are dedented so
they actually run.
> 
> **Writes are isolated**: per-test `scope` tokens in
prompts/responses/attributes, **60s TTL** on routine stores, and
assertions that look for *your* scoped row in `hits` (not `hits[0]`).
TTL-expiry tests use **scoped attribute filters**, longer TTL/sleep, and
tighter delete-by-attribute counts.
> 
> **CONTRIBUTING.md** adds guidance to namespace data and never flush
shared external services, pointing at the integration module docstring.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
68d378d. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
@vishal-bala
vishal-bala marked this pull request as ready for review September 10, 2026 07:38
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