Keep virtual server filtering scoped - #1105
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe router stops reinjecting client-supplied ChangesVirtual Server Scope Bypass Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
review-effort/large
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/code-style.yaml (1)
17-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winConcurrency group violates coding guidelines.
The group must use
${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}per coding guidelines. Current pattern lacks PR number isolation, causing incorrect run cancellation across different PRs.🔧 Required fix
concurrency: - group: code-style-${{ github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true🤖 Prompt for 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. In @.github/workflows/code-style.yaml around lines 17 - 19, Replace the current concurrency group declaration so it uses the required workflow+PR-or-ref pattern; specifically update the concurrency.group value (the "concurrency" block with keys group and cancel-in-progress) to use the expression ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} instead of code-style-${{ github.ref }} so runs are isolated per workflow and PR.Sources: Coding guidelines, Learnings
🤖 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.
Outside diff comments:
In @.github/workflows/code-style.yaml:
- Around line 17-19: Replace the current concurrency group declaration so it
uses the required workflow+PR-or-ref pattern; specifically update the
concurrency.group value (the "concurrency" block with keys group and
cancel-in-progress) to use the expression ${{ github.workflow }}-${{
github.event.pull_request.number || github.ref }} instead of code-style-${{
github.ref }} so runs are isolated per workflow and PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24e44d17-d122-4482-b359-bf1a1c5063c3
📒 Files selected for processing (1)
.github/workflows/code-style.yaml
coderabbitai
Bot
added
review-effort/medium
jasonmadigan
left a comment
jasonmadigan
left a comment
There was a problem hiding this comment.
virtual server validation and fail-closed behaviour look correct. one issue: x-mcp-authorized is still re-injected from client-supplied headers (line 868). #1082 flagged this as a bypass risk -- the value should come from validated session state, not from what the client sent. the VS half is fixed properly, but the authorized header needs the same treatment before this fully closes #1082.
|
Thanks @jasonmadigan ! I've removed the re-injection entirely since Authorino sets it on the request anyway, so it still reaches the broker without needing to be copied from client headers. |
a1643d3 to
e23c448
Compare
e23c448 to
f89c0fe
Compare
|
Hey @jasonmadigan ! just following up on this PR. I've addressed the review feedback and all checks are passing. Would appreciate another look when you have time. Thanks! |
|
@manik3160 please resolve conflicts |
f89c0fe to
e0106a2
Compare
|
Sure @Patryk-Stefanski ! |
Patryk-Stefanski
left a comment
Patryk-Stefanski
left a comment
There was a problem hiding this comment.
The core fixes are correct — failing closed on missing virtual server and stopping the re-injection of client-supplied headers on the broker path are both the right calls.
One blocking issue before this can merge, plus two minor nits.
Blocking — x-mcp-authorized accidentally removed from client stripping
clientStrippedHeaders no longer includes mcpAuthorizedHeader:
var clientStrippedHeaders = []string{mcpVirtualServerHeader, mcpVerifiedSubHeader}The PR's intent was to stop the router re-injecting x-mcp-authorized — correct. But it also stopped stripping the client-supplied value in the headers phase, which is a separate concern and should stay.
The consequence: if a client sends any value in x-mcp-authorized, Authorino appends its wristband JWT rather than overwriting, so the broker receives 2 values. parseAuthorizedCapabilitiesJWT requires exactly 1 value (internal/broker/filtered_tools_handler.go:130) and returns an error when it gets 2. The broker then returns an empty tool list — even when enforceCapabilityFilter is false. Any client that sends this header gets silently empty tools.
Fix — add mcpAuthorizedHeader back to clientStrippedHeaders:
var clientStrippedHeaders = []string{mcpAuthorizedHeader, mcpVirtualServerHeader, mcpVerifiedSubHeader}Stripping the client value in the headers phase and not re-injecting it in the body phase are both independently correct.
Nit — validVirtualServerHeader returns 400 when RoutingConfig is nil
A nil RoutingConfig is a server error, not a bad client request. The 400 status code is misleading. Since HandleRequestHeaders already dereferences RoutingConfig unconditionally and would panic first, this path is unreachable in practice — but if the guard is there for safety, a 500 would be more accurate.
Nit — nil vs empty slice inconsistency in applyVirtualServerFilter
On error the function returns []mcp.Tool{} (non-nil empty slice), but when a VS exists and no tools match, filtered is nil. Both mean "no tools" and callers using len() are unaffected, but it's worth making consistent — use var filtered []mcp.Tool and return that on error too.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1082 by tightening how virtual server scoping is enforced across the router→broker path: preventing re-use of client-supplied internal filter headers after Envoy strips them, and ensuring virtual-server-based tool filtering fails closed when a referenced virtual server doesn’t exist.
Changes:
- Router: stops re-injecting broker-only internal headers from the client header snapshot; validates
x-mcp-virtualserverbefore forwarding it to the broker passthrough path. - Broker: changes virtual server filtering to return an empty tool set when the virtual server lookup fails (fail-closed instead of fail-open).
- Tests/CI: updates e2e/unit tests for the stricter behavior and adds retry logic for Go tool installs in the code-style workflow; bumps KIND version.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/user_specific_list_test.go | Creates a real MCPVirtualServer and retries client creation to match new virtual-server validation. |
| tests/e2e/tool_discovery_test.go | Retries initialize/notify-initialized sequence to reduce flakes under stricter header validation. |
| tests/e2e/happy_path_test.go | Retries client creation when using X-Mcp-Virtualserver header in happy-path flows. |
| tests/e2e/auth_policy_test.go | Retries initialize flow for AuthPolicy + virtual-server cases. |
| internal/mcp-router/request_handlers.go | Switches stripped-header set usage and adds virtual server header validation before broker passthrough. |
| internal/mcp-router/request_handlers_test.go | Adds coverage to ensure internal headers aren’t re-injected and validates behavior for unknown virtual servers. |
| internal/mcp-router/headers.go | Introduces clientStrippedHeaders (currently identical to internalOnlyHeaders). |
| internal/broker/filtered_tools_handler.go | Makes virtual server filtering return no tools when the virtual server cannot be found. |
| internal/broker/filtered_tools_handler_test.go | Updates expectations to verify fail-closed behavior for missing virtual servers. |
| build/tools.mk | Bumps KIND version used by CI tooling. |
| .github/workflows/code-style.yaml | Adds a retry wrapper around Go tool installs for CI robustness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f8b82d9 to
9642b75
Compare
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
The previous commit validating virtual server headers accidentally dropped the re-injection of the x-mcp-authorized header in HandleNoneToolCall. Because of this, auth-based filtering was broken as the broker never received the authorization header to filter tools. This commit restores the re-injection of x-mcp-authorized and updates the unit test that incorrectly asserted it should be absent. Signed-off-by: Manik <maniksingh3606@gmail.com>
Fixes 'ERROR: unknown containerd config version: 4 (supported versions: 2 and 3)' which occurs in GitHub Actions runner with newer containerd versions by updating kind from v0.29.0 to v0.32.0. Signed-off-by: Manik <maniksingh3606@gmail.com>
The router now strictly validates the X-Mcp-Virtualserver header and returns a 400 Bad Request if the virtual server isn't found. When tests create an MCPVirtualServer and immediately make a request, the router might not have received the updated config from the controller yet. This wraps the initial client connection in an Eventually block to ensure we wait for the router configuration to sync before failing. Signed-off-by: Manik <maniksingh3606@gmail.com>
The router now strictly validates the X-Mcp-Virtualserver header and returns a 400 Bad Request if the virtual server isn't found. The security test was using a fake 'test/vs' header, which caused the router to reject the request. This updates the test to create a valid MCPVirtualServer and wait for it to sync before testing header forwarding. Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
Signed-off-by: Manik <maniksingh3606@gmail.com>
9642b75 to
7c3c96a
Compare
|
Thanks @Patryk-Stefanski ! fixed all the points |
Signed-off-by: Manik <maniksingh3606@gmail.com>
7c3c96a to
604ffb7
Compare
…headers in broker This reverts the addition of x-mcp-authorized to clientStrippedHeaders in the router, as doing so breaks E2E tests which inject this header directly from the client. Instead, the broker now safely accepts multiple headers and uses the last one, protecting against Authorino wristband duplication without breaking client testability. Signed-off-by: Manik <maniksingh3606@gmail.com>
There was a problem hiding this comment.
mcpAuthorizedHeader is still missing from clientStrippedHeaders — flagged in #1105 (review) as a blocking issue.
var clientStrippedHeaders = []string{mcpVirtualServerHeader, mcpVerifiedSubHeader}This means a client-supplied x-mcp-authorized value is no longer stripped in the headers phase. The fix is one line:
var clientStrippedHeaders = []string{mcpAuthorizedHeader, mcpVirtualServerHeader, mcpVerifiedSubHeader}Stopping re-injection in the body phase and stripping in the headers phase are two independent concerns — both need to be in place.
Fixes #1082
This stops the router from re-adding client-supplied internal filter headers on the broker passthrough path after Envoy strips them.
It also makes tool filtering fail closed when
x-mcp-virtualserverpoints to a missing virtual server, so the broker returns no tools instead of the full unfiltered list.Tests run:
Summary by CodeRabbit
Bug Fixes
Tests
Chores