feat(mcp): add the security core for auth-claim tenant injection - #738
Draft
vishal-bala wants to merge 1 commit into
Draft
vishal-bala wants to merge 1 commit into
vishal-bala wants to merge 1 commit into
Conversation
Phase 2 of custom MCP tools scopes every query a profile runs to the tenant
carried in the caller's verified token. This is the first of two slices: the
three functions that carry the security property, with no configuration model
and no server wiring, so all of it is unit-testable.
`resolve_injected_claim` reads one claim and accepts only a single non-empty
unpadded string. It sits directly below `authorization_values`, which reads
`access_token.claims` under the opposite rules -- that reader space-splits a
string and coerces list members, because an absent scope only ever denies,
whereas a widened value here grants. Keeping the two adjacent, each commented
with why it differs from its neighbour, is what stops them being unified.
The type check is the load-bearing one. `_formatted_tag_value` escapes each
element before joining them with `|`, so a list claim renders a genuine
cross-tenant union that no character scan of the output can tell from a
legitimate one -- the `|` is structure rather than content. The scalar `|`
check stays as a backstop for the character class changing again.
`build_injected_filter` builds one tag equality per entry, ANDs them, and
refuses the whole request when any single claim is unusable. Its match-all
check runs on each clause alone, before combining: an intersection elides a
`*` operand, so a check on the combined expression would pass while the tenant
clause had silently vanished.
`validate_inject_against_schema` fails startup when an injected field is
absent, is not a tag, or is NOINDEX. A NOINDEX field is the worst of the three
because it returns nothing and so looks like correct scoping.
`_is_match_all_filter` moves from `redisvl/index/index.py` to
`redisvl/query/filter.py` as public `is_match_all_filter`, with its one call
site updated. It exists rather than a bare `str(expr) != "*"` because an
un-initialized `FilterExpression` raises on render; that case now has a test.
Injected fields are tag fields only. Text equality is an exact-phrase match
and text tokenizes on punctuation, so `@t:("acme-corp")` also matches
`acme-corp-eu` -- widening, and `no_stem` does not fix it.
Every guard was mutation-checked by reverting it alone. One did not fail any
test: the match-all check is unreachable behind the claim reader's own empty
check, so it is now pinned by a test that substitutes that reader, which is
exactly the future it insures against.
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
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.
What this changes
Phase 2 of custom MCP tools lets a profile scope every query to the tenant carried in the caller's verified token, so a multi-tenant deployment can expose one
search_customer_kbtool instead of trusting the model to remember a filter. This is the first of two slices: the three functions that carry the security property, with no configuration model and no server wiring. Nothing in this PR is reachable from a running server yet, which is deliberate -- it means all of it is unit-testable, and the slice that adds the YAML surface adds no new security logic.The guarantee, in the words the tests prove: a client presenting a validly-signed token cannot make the model widen or escape the tenant scope carried in that token. The trust boundary is the identity provider, not the MCP client.
Three functions land in
redisvl/mcp/auth.py:resolve_injected_claimreads one claim off the current request's token and accepts only a single non-empty, unpadded string. It sits directly belowauthorization_values, which readsaccess_token.claimsunder the opposite rules -- that reader space-splits a string and coerces list members, because an absent scope only ever denies, whereas a widened value here grants. Keeping them adjacent, each commented with why it differs from its neighbour, is what stops someone unifying them in a year and reintroducing the hazard.build_injected_filterbuilds one tag equality per entry, ANDs them, and refuses the whole request when any single claim is unusable rather than narrowing by the entries that did resolve.validate_inject_against_schemafails startup when an injected field is absent, is not a tag, or is declaredNOINDEX._is_match_all_filteralso moves fromredisvl/index/index.pytoredisvl/query/filter.pyas publicis_match_all_filter, with its one call site updated, because the injection path needs it too.Why the type check is the load-bearing one
Taghas escaped|inside a single value since 0.27.1, so a scalar claim can no longer smuggle a union. A list claim still can:_formatted_tag_valueescapes each element before joining them with|, which is what makes the documented list-OR form work.The second form is indistinguishable from a legitimate union by inspecting the rendered output, so a character scan cannot close it. Rejecting a non-
strclaim on type is what does. The scalar|check stays as defence in depth against that character class changing again -- it has already changed once.The canary test asserts both rendering properties directly rather than asserting which characters are in which set, so it keeps its meaning the next time the class moves.
Why the match-all check runs before combining, not after
Tag(f) == ""renders as the match-all*, andformat_expressionelides a*operand. So a check on the combined expression is vacuous:The tenant clause is gone and the query still looks scoped. The guard therefore runs on each injected clause by itself. It reuses
is_match_all_filterrather than a secondstr(expr) != "*"comparison, because a defaultFilterExpressionraises on render and the helper already catches that.That rendering behaviour is deliberate and pinned by the nullable-filter tests in
tests/unit/test_filter.py. It is not changed here; the guard belongs at the MCP boundary.Why tag fields only
Text equality renders as an exact-phrase query,
@t:("acme-corp"), and text tokenizes on punctuation -- so the phrase also matchesacme-corp-eu. That is widening, andno_stemdoes not fix it. #721 closed the separate clause-breakout hole inText, but tokenization is untouched by that fix and rules text out on its own.Numeric is excluded for cost rather than safety: a second validation path, a second rendering path, a second empty-render case, an explicit
boolexclusion becauseboolsubclassesint, and an open question about string-typed"42"claims, for no acceptance criterion this phase carries. The configuration key is the same either way, so numeric can be added later with no change for operators.This narrows the approved proposal, which permits "a tag, or a text/numeric with equality". The proposal sentence is the artefact that should change.
Why the refusal names the claim
A misspelled claim name is otherwise an opaque permanent failure. The disclosure costs nothing: a JWT is signed, not encrypted, so a client holding a valid token can already read its own claim names, and an unauthenticated client never reaches this code because the HTTP layer rejects it first.
Testing
tests/unit/test_mcp/test_auth_claim_injection.pyis new and hermetic -- 29 tests, no Redis, no network. It extends the monkeypatch pattern already intest_auth_scope.py.Absent, misspelled,
None, empty, whitespace-only, left- and right-padded,list,dict,bool,intand|-bearing claims each raiseFORBIDDENwith no expression built. A tokenless request raises rather than returning early, which is the one place injection must invert the scope gate's behaviour:ensure_tool_scopereads a missing token as "stdio, so no gate applies", and the same exit reached here would attach no tenant clause at all.Every guard was mutation-checked by reverting it alone and confirming exactly the tests asserting it fail. One did not fail anything: the match-all check is unreachable behind the claim reader's own emptiness check. Rather than delete it or leave it as an untested comment, it is now pinned by a test that substitutes the claim reader -- which is precisely the future it insures against, a reader that admits a value
Tagrenders as the wildcard.A padded claim is refused rather than normalized to its stripped value, because two tenants named
acmeandacmewould otherwise collapse onto one.Full unit suite: 1692 passed, 1 skipped. MCP and bulk-operation integration suites: 89 passed, 2 skipped.
mypyclean on all three changed modules.Release Notes
Internal only; no user-facing behaviour changes in this PR.
redisvl.query.filter.is_match_all_filteris now public -- it returnsTruefor a filter that would match every document, includingNone,"*", and an un-initializedFilterExpression.Next steps
Stack 02 adds the configuration surface (
lock.injectwithfrom: claim), the profile wiring that pre-folds the injected expression into the locked filter, the startup refusal when authentication is off or configured under stdio, schema hints that exclude the injected field, and the documentation with its three operator preconditions.Stack created with GitHub Stacks CLI • Give Feedback 💬