feat: enforce VK tool-grant boundary on caller-provided x-bf-mcp-include-tools lists via pruneMCPIncludeToolsFromContext

[Pratham-Mishra04]

Summary

Caller-provided MCP include-tools lists (set via the x-bf-mcp-include-tools header) could previously reference tools that the virtual key does not grant, effectively allowing callers to expand their own tool access beyond what the key permits. This PR enforces that a caller-provided list can only narrow the virtual key's tool grant, never expand it.

Changes

• Introduced pruneMCPIncludeToolsFromContext, which reads a caller-provided include-tools list from the context, intersects it with the tools the virtual key actually grants, and writes the pruned result back to the context.
• Caller wildcards (e.g. sentry-*) are kept verbatim only when the virtual key itself is unrestricted for that client; otherwise they are expanded to the key's specific grants to prevent the wildcard from being interpreted downstream as "all tools for this client."
• Auto-injection of the computed tool list is now skipped when a caller-provided list was present (even if it pruned to empty), replacing the previous check that only skipped injection when the context key was unset.
• Pruning is applied in both runPreRequestRouting and PreRequestHook, replacing the earlier pattern of checking for an existing context value before injecting.
• A wrong-type context value fails closed, pruning to a deny-all empty list rather than passing through.
• Added a dedicated test file covering: no caller list, disallowed tools dropped, granted tools kept, specific tools under unrestricted grants, wildcard passthrough vs. narrowing, ungranted client wildcards, empty header opt-out, deduplication, AllowOnAllVirtualKeys clients, explicit empty config overriding allow-all, multi-client pruning, and wrong-type fail-closed behavior.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./plugins/governance/...

The new test file prunemcpincludetools_test.go covers all pruning scenarios. Verify that:

• A caller requesting a tool not in the virtual key's grant receives an empty (deny-all) list.
• A caller requesting a subset of granted tools receives only that subset.
• A caller wildcard is narrowed to the key's specific grants when the key is not itself unrestricted for that client.
• Auto-injection does not overwrite a caller-provided list, even when the pruned result is empty.

Breaking changes

• Yes
• No

Security considerations

This change closes a privilege escalation path where a caller could use the x-bf-mcp-include-tools header to request tools beyond what their virtual key grants. The pruning logic ensures the effective tool set is always bounded by the virtual key's configured permissions, and any ambiguous or malformed input fails closed to a deny-all state.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

Summary by CodeRabbit

• New Features

  • Caller-provided MCP include-tools lists are now pruned to only the tools allowed for the active virtual key; auto-injection of tools occurs only when callers provide no list and auto-injection isn’t disabled.

• Tests

  • Added comprehensive tests covering pruning behavior, wildcards, deduplication, opt-out cases, multi-client scenarios, and fail-closed handling.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Pratham-Mishra04]

• tests: add PreRequestHook MCP tool-stamping unit tests for governance plugin #4298
• fix: stamp virtual key tool allowlist when include-clients filter bypasses autoInjectDisabled #4297
• feat: enforce VK tool-grant boundary on caller-provided x-bf-mcp-include-tools lists via pruneMCPIncludeToolsFromContext #4296 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

📝 Walkthrough

Walkthrough

Prunes caller-provided MCP include-tools to the virtual key's allowed grants via a new helper; refactors include-tools computation for consistent snapshots; integrates pruning into routing and pre-request hook; adds tests covering wildcards, dedup, allow-on-all, multi-client, and edge cases.

Changes

MCP Include-Tools Pruning

Layer / File(s)	Summary
Core pruning function and computation snapshot plugins/governance/main.go	Adds pruneMCPIncludeToolsFromContext to read caller schemas.MCPContextKeyIncludeTools, compute VK-allowed patterns via a snapshotting computeMCPIncludeToolsWith, prune/expand/narrow/deduplicate entries, write the pruned list back, and return whether a caller list existed.
Routing and PreRequestHook integration plugins/governance/main.go	Calls pruning in runPreRequestRouting (line 431–442) and PreRequestHook (line 1224–1233); auto-injection of VK include-tools now occurs only when no caller list exists and auto-injection isn't disabled.
Test helpers and foundational test cases plugins/governance/prunemcpincludetools_test.go	Adds newCtxWithIncludeTools and includeToolsFromCtx helpers and tests for no-caller-list, disallowed-tool drop, granted-tool retention, specific-tool under unrestricted VK, and wildcard preservation when VK is unrestricted.
Complex and edge-case tests plugins/governance/prunemcpincludetools_test.go	Tests wildcard narrowing/dropping for ungranted clients, empty-header opt-out, deduplication across wildcard and specific requests, allow-on-all-virtual-keys behavior and explicit-empty override, multi-client pruning, and wrong-type fail-closed handling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• maximhq/bifrost#3933: Related work on governance MCP include-tools handling and routing/refactor that touches the same computation/injection paths.

Suggested reviewers

• akshaydeo
• danpiths

Poem

🐰 I nibble lists the callers bring,
Wildcards trimmed to what VKs sing.
I prune and dedupe, tidy each line,
Inject only when none are mine —
A rabbit's hop keeps MCPs fine.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically summarizes the main change: enforcing virtual key tool-grant boundaries on caller-provided include-tools lists via the new pruneMCPIncludeToolsFromContext function.
Description check	✅ Passed	The description follows the template structure, includes all critical sections (Summary, Changes, Type of change, Affected areas, How to test, Breaking changes, Security considerations, Checklist), and provides comprehensive detail about the security fix and implementation.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 06-11-fix_governance_mcp_tool_filter_fixes

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Pratham-Mishra04]

@coderabbitai full-review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff and key file contents
gh pr view 4296 --json files,title,body

echo "=== main.go changes ==="
gh api repos/maximhq/bifrost/pulls/4296/files | jq -r '.[] | select(.filename | contains("governance/main.go")) | .patch'

echo "=== new test file ==="
gh api repos/maximhq/bifrost/pulls/4296/files | jq -r '.[] | select(.filename | contains("prunemcpincludetools_test.go")) | .patch'

echo "=== full file list ==="
gh api repos/maximhq/bifrost/pulls/4296/files | jq -r '.[].filename'

Length of output: 18013

---

🏁 Script executed:

#!/bin/bash
# Read current full contents of the modified files
cat plugins/governance/main.go

Length of output: 70978

---

🏁 Script executed:

#!/bin/bash
cat plugins/governance/prunemcpincludetools_test.go 2>/dev/null || echo "File not found in current branch"

# Also check related schemas
rg -n "MCPContextKeyIncludeTools" --type=go -A2 -B2

# Check computeMCPIncludeTools
rg -n "computeMCPIncludeTools" --type=go -A20 -B2

Length of output: 28462

---

---

@Pratham-Mishra04 Here is the full review of PR #4296.

---

Review of feat: enforce caller x-bf-mcp-include-tools can only narrow VK tool grant

Summary

The PR correctly addresses a privilege escalation path and the overall approach is sound. The logic for wildcard narrowing, AllowOnAllVirtualKeys passthrough, wrong-type fail-closed, and deduplication is correct and well-tested. A few issues are worth addressing before merge.

---

Issues

1. 🔴 computeMCPIncludeTools is called twice per request (hot path)

In both runPreRequestRouting (Line 431) and PreRequestHook (Line 1215), the call to pruneMCPIncludeToolsFromContext already invokes computeMCPIncludeTools internally. When no caller-provided list exists (the common case), the auto-inject branch calls it a second time:

// First call — inside pruneMCPIncludeToolsFromContext
callerProvided := p.pruneMCPIncludeToolsFromContext(ctx, virtualKey)

// Second call — when callerProvided == false (common case)
if !callerProvided && !autoInjectDisabled {
    if tools := p.computeMCPIncludeTools(virtualKey); tools != nil {

computeMCPIncludeTools iterates over virtualKey.MCPConfigs and calls GetMCPClientsAllowingAllVirtualKeys() on every call — this hits both call sites.

Suggested fix: Return the computed tools from pruneMCPIncludeToolsFromContext so callers can reuse them:

// pruneMCPIncludeToolsFromContext signature change
func (p *GovernancePlugin) pruneMCPIncludeToolsFromContext(
    ctx *schemas.BifrostContext,
    virtualKey *configstoreTables.TableVirtualKey,
) (callerProvided bool, vkTools []string) {
    existing := ctx.Value(schemas.MCPContextKeyIncludeTools)
    vkTools = p.computeMCPIncludeTools(virtualKey) // compute once
    if existing == nil {
        return false, vkTools
    }
    // ... pruning logic using vkTools ...
    ctx.SetValue(schemas.MCPContextKeyIncludeTools, pruned)
    return true, vkTools
}

// Call sites become:
callerProvided, vkTools := p.pruneMCPIncludeToolsFromContext(ctx, virtualKey)
if !callerProvided && !autoInjectDisabled {
    if len(vkTools) > 0 {
        ctx.SetValue(schemas.MCPContextKeyIncludeTools, vkTools)
    }
}

---

2. 🟡 GetMCPClientsAllowingAllVirtualKeys() called independently inside pruneMCPIncludeToolsFromContext

At Lines 851–854, the function fetches allowAllClients independently for the isMCPToolAllowedByVKWith call, even though computeMCPIncludeTools (called just above it) already fetched it internally. This means GetMCPClientsAllowingAllVirtualKeys() is invoked 2–3 times per request. Fix 1 above resolves this as well if allowAllClients is threaded through.

---

3. 🟡 No nil guard on virtualKey inside pruneMCPIncludeToolsFromContext

pruneMCPIncludeToolsFromContext calls computeMCPIncludeTools(virtualKey) at Line 845 without checking for a nil virtualKey. If virtualKey is nil, computeMCPIncludeTools will panic on for _, vkMcpConfig := range virtualKey.MCPConfigs.

The two call sites do guard with if virtualKey != nil { ... } today, but nothing prevents a future call site from missing that guard. A defensive check inside the function itself is warranted:

func (p *GovernancePlugin) pruneMCPIncludeToolsFromContext(ctx *schemas.BifrostContext, virtualKey *configstoreTables.TableVirtualKey) bool {
    existing := ctx.Value(schemas.MCPContextKeyIncludeTools)
    if existing == nil {
        return false
    }
    if virtualKey == nil {
        // Treat as no VK grants — prune everything to deny-all
        ctx.SetValue(schemas.MCPContextKeyIncludeTools, []string{})
        return true
    }
    // ... rest of the function

---

4. 🟢 Minor: computeMCPIncludeTools returns []string{} (empty, non-nil) but the auto-inject guard checks tools != nil

if tools := p.computeMCPIncludeTools(virtualKey); tools != nil {
    ctx.SetValue(schemas.MCPContextKeyIncludeTools, tools)
}

computeMCPIncludeTools always initializes executeOnlyTools := make([]string, 0) and never returns nil, so the != nil guard never short-circuits. This is not a bug, but either document the guarantee in computeMCPIncludeTools's godoc, or change the guard to len(tools) > 0 for clarity and accuracy.

---

Test Coverage Assessment

The 13 tests in prunemcpincludetools_test.go cover the scenarios well. A few additional cases worth considering:

• nil virtualKey passed directly to pruneMCPIncludeToolsFromContext — currently would panic (see Issue 3).
• Tool name containing -* that is not a client wildcard (e.g., "some-weird-*-tool") — strings.CutSuffix(pattern, "-*") would return "some-weird-*-tool" with isWildcard = false, so it falls to isMCPToolAllowedByVKWith. This seems correct but worth a test.
• TestPruneMCPIncludeTools_AllowOnAllVirtualKeysClient: The test passes buildVKNoMCPConfigs() — it would be worth also testing an AllowOnAllVirtualKeys client where the VK has an empty MCPConfig override for that client (this is partially covered by TestPruneMCPIncludeTools_ExplicitEmptyConfigOverridesAllowAll but the AllowOnAllVirtualKeys-specific interaction with non-empty specific override is not).

---

Overall

The security fix is correct and the new pruneMCPIncludeToolsFromContext logic handles all the subtle edge cases (wildcard narrowing, fail-closed wrong types, deduplication) properly. The main actionable items are the double computeMCPIncludeTools call (perf) and the nil-VK defensive guard (safety). Everything else is low priority.

[Pratham-Mishra04]

@coderabbitai

This suggestion is solving a problem that doesn't exist, and the sketch as written would reintroduce the leak we just fixed. Two issues:

1. There's no duplicate computation today. The two computeMCPIncludeTools calls are on mutually exclusive paths:

func (p *GovernancePlugin) pruneMCPIncludeToolsFromContext(...) bool {
existing := ctx.Value(schemas.MCPContextKeyIncludeTools)
if existing == nil {
return false // ← returns BEFORE computing anything
}
...
vkTools := p.computeMCPIncludeTools(virtualKey) // only on the prune path

When a caller list exists, prune computes once and the inject branch is skipped (!includeToolsProvided is false). When it doesn't, prune returns before computing and the inject branch computes once. Exactly one computation per request either way. The suggested version actually makes it worse: it computes eagerly even on the path where nothing is stamped at all (auto-inject off, no filters) — wasted work on every such request.

1. The len(vkTools) > 0 guard is a regression. computeMCPIncludeTools never returns nil — for a VK with no MCP grants it returns an empty list, and stamping that empty list is load-bearing: it's the deny-all that stops core from injecting anything when an include-clients header has opted the request into injection. With len(vkTools) > 0, that case leaves the ctx key unset → core opts in via include-clients → every tool of the named client leaks. That's precisely the bug from your curl, and TestPreRequestHookMCP_NoGrants_IncludeClients_StampsDenyAll exists to catch it — this change would fail that test. (The sketch also drops the || includeClientsPresent condition, though I assume that's just sketching against the older call site.)

If this came from a review bot, I'd resolve it with that explanation rather than apply it. If what you like about it is the signature — making the data flow explicit instead of the boolean — a corrected version would be:

includeToolsProvided, vkTools := p.pruneMCPIncludeToolsFromContext(ctx, virtualKey)
...
if !includeToolsProvided && (!autoInjectDisabled || includeClientsPresent) {
ctx.SetValue(schemas.MCPContextKeyIncludeTools, vkTools) // unconditional — empty = deny-all
}

with prune computing vkTools up front. That's behaviorally safe but trades the lazy computation for a slightly cleaner call site. My recommendation is to leave the code as is — say the word if you'd like the corrected variant applied instead.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Pratham-Mishra04 You're right on both counts — I apologize for the incorrect analysis.

1. No duplicate computation: I missed the early-return before computeMCPIncludeTools is invoked inside pruneMCPIncludeToolsFromContext. The two call sites are genuinely mutually exclusive — one computation per request regardless of path. My suggestion would have made it worse by computing eagerly on every request.

2. len(vkTools) > 0 is a regression: The empty-list stamp is the deny-all mechanism. Skipping the stamp when the list is empty would leave the context key unset, allowing core to inject all tools for a named client via include-clients — exactly the leak the PR is fixing. TestPreRequestHookMCP_NoGrants_IncludeClients_StampsDenyAll is the right guard for this.

The code as-is is correct. I'm withdrawing the review comment.

🐇 (ears-down moment)

[greptile-apps]

Confidence Score: 5/5

Safe to merge. The change tightens an access-control boundary and fails closed; the refactor to computeMCPIncludeToolsWith gives both the wildcard and per-tool checks a single consistent snapshot per request.

The pruning logic covers all four relevant paths — specific tools, wildcard expansion, AllowOnAllVirtualKeys fallback, and wrong-type fail-closed — and the tests exercise each of them. The TOCTOU concern from the previous review thread is directly addressed by the computeMCPIncludeToolsWith refactor. Both call sites guard with virtualKey != nil before calling the new function, and the large-payload early return ensures no double-pruning occurs.

No files require special attention.

Important Files Changed

Filename	Overview
plugins/governance/main.go	Adds pruneMCPIncludeToolsFromContext to enforce VK grant boundaries on caller-provided tool lists; refactors computeMCPIncludeTools into computeMCPIncludeToolsWith to accept a pre-fetched snapshot; updates both runPreRequestRouting and PreRequestHook to prune before auto-injection.
plugins/governance/prunemcpincludetools_test.go	New dedicated test file covering all pruning scenarios: no caller list, disallowed drop, granted keep, specific-under-unrestricted, wildcard passthrough vs. narrowing, ungranted wildcard, empty opt-out, dedup, AllowOnAllVirtualKeys, explicit-empty override, multi-client, and wrong-type fail-closed.

_{Reviews (2): Last reviewed commit: "fix: governance mcp tool filter fixes" | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/governance/prunemcpincludetools_test.go`:
- Around line 31-187: Add two low-priority edge-case tests for
pruneMCPIncludeToolsFromContext: one that passes a nil virtualKey to verify it
fails closed (returns true and results in an empty include-tools ctx) to
document the defensive contract, and another that uses a client name containing
multiple dashes (e.g., "sentry-mcp") with a wildcard request like "sentry-mcp-*"
to ensure the suffix-cutting logic preserves the multi-dash prefix; reuse
helpers newPluginWithInMemoryStore, newCtxWithIncludeTools,
buildVKWithMCPConfigs or construct a TableVirtualKey as in
TestPruneMCPIncludeTools_MultipleClients, and assert behavior via
includeToolsFromCtx and the boolean return of pruneMCPIncludeToolsFromContext.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e0911dc6-7da9-40de-bc1e-9bf59a42fc5e

📥 Commits

Reviewing files that changed from the base of the PR and between 9c77480 and 1ee84a3.

📒 Files selected for processing (2)

• plugins/governance/main.go
• plugins/governance/prunemcpincludetools_test.go

[akshaydeo]

Merge activity

• Jun 11, 4:21 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jun 11, 4:22 PM UTC: @akshaydeo merged this pull request with Graphite.