Skip to content

feat(mcp): add a locked-filter merge primitive to search-records - #675

Draft
vishal-bala wants to merge 1 commit into
feat/mcp-custom-tool-profilesfrom
feat/mcp-locked-filter-merge
Draft

feat(mcp): add a locked-filter merge primitive to search-records#675
vishal-bala wants to merge 1 commit into
feat/mcp-custom-tool-profilesfrom
feat/mcp-locked-filter-merge

Conversation

@vishal-bala

Copy link
Copy Markdown
Collaborator

Stack position: 1 of 3. Base is the feat/mcp-custom-tool-profiles integration branch, not main — see the sequencing note at the bottom.

Groundwork for custom tool profiles, split out on its own because it is the security crux of that feature and deserves attention it would not get buried in a 1,900-line PR. Nothing calls the new parameters yet.

What it does

merge_locked_filter(locked, caller) AND-combines an author-locked filter expression with a caller-supplied one, so a caller can only narrow within the locked scope and never widen past it. That rests on two things: FilterExpression.__and__ parenthesizing the combination, so a caller's or/not nests inside the AND rather than reaching the top level; and every filter value staying inside its own clause, which the text escaping already on main provides.

A raw string caller filter is refused outright — strings skip the DSL's field validation and have no safe composition with an expression, since combining them means concatenation.

The backstop, and why it walks the rendering

_reject_escapable_filter sits behind that as a backstop, not the primary defense: a well-formed expression built from escaped values cannot escape, so anything it rejects means a value reached the query string raw.

It walks the rendered string rather than counting delimiters, because several things look like structure and are not:

  • \( is an escaped literal, not a group
  • a numeric range renders exclusive bounds as [(5 +inf], where ( is a marker
  • a tag clause scopes its alternatives in braces, so @category:{sports|health} is one clause rather than a union

Escape pairs are consumed rather than tested against the previous character, so \\| is not misread as a protected delimiter. Both directions of that were wrong in earlier drafts, which is the main reason this is its own PR.

limit_cap

Applied inside _validate_request rather than by the caller, because an omitted limit only resolves to the binding default at that point — capping just the explicit value would let the default sail past the cap. An explicit request above the cap is rejected; an omitted one is capped silently, since the caller never named a number.

Not exposed to the model

Both parameters are keyword-only on search_records and never reach the advertised MCP schema: register_search_tool registers an inner wrapper with its own fixed signature. I verified that rather than assuming it.

Verification

  • MCP unit tests: 276 passing
  • make check-types: clean
  • The backstop accepts every legitimate like pattern (multi-word AND, % fuzzy, wildcards, in-clause |) while keeping the locked clause intact on an injection attempt

Sequencing

This targets the integration branch so that main never receives a config surface that validates but enforces nothing — see the next PR in the stack for why that matters. The integration branch currently also carries #668; once that merges it rebases onto main and drops out.

Groundwork for custom tool profiles, landed on its own because it is the
security crux of that feature and deserves review attention that it would
not get buried in a larger change. Nothing calls the new parameters yet.

`merge_locked_filter(locked, caller)` AND-combines an author-locked filter
expression with a caller-supplied one so the caller can only narrow within
the locked scope and never widen past it. That rests on two things:
`FilterExpression.__and__` parenthesizing the combination, so a caller's
`or`/`not` nests inside the AND rather than reaching the top level; and
every filter value staying inside its own clause, which the text escaping
already on this branch provides.

A raw string caller filter is refused outright. Strings skip the DSL's
field validation and have no safe composition with an expression --
combining them means concatenation, where a crafted value can close the
locked group.

`_reject_escapable_filter` is a backstop behind that, not the primary
defense: a well-formed expression built from escaped values cannot escape,
so anything it rejects means a value reached the query string raw. It
walks the rendering rather than counting delimiters, because several
things look like structure and are not -- `\(` is an escaped literal, a
numeric range renders exclusive bounds as `[(5 +inf]` where `(` is a
marker, and a tag clause scopes its alternatives in braces, so
`@category:{sports|health}` is one clause rather than a union. Escape
pairs are consumed rather than tested against the previous character, so
`\\|` is not misread as a protected delimiter.

`limit_cap` bounds the result window. It is applied inside
`_validate_request` rather than by the caller because an omitted `limit`
only resolves to the binding default at that point -- capping just the
explicit value would let the default sail past the cap. An explicit
request above the cap is rejected; an omitted one is capped silently,
since the caller never named a number.

Both parameters are keyword-only on `search_records` and never reach the
model: `register_search_tool` registers an inner wrapper with its own
fixed signature, so the advertised MCP input schema is unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant