Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6521 +/- ##
=======================================
Coverage 79.28% 79.28%
=======================================
Files 802 802
Lines 81245 81253 +8
=======================================
+ Hits 64415 64422 +7
- Misses 16825 16826 +1
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review this, and thanks for sticking with it! Separating client responses from requests that failed to parse makes sense for the proxy authorization middleware.
There are two things we need to address before merging: the response check accepts invalid JSON-RPC envelopes, and the regression test doesn't establish that we're fixing the vMCP path described in #5009. I've left the details inline.
We've also changed the parser on main since this was written, and both production files now conflict. Please keep the newer admission checks when rebasing. We should be able to use the already validated message to identify responses rather than decode the body again.
I don't think we should add a pending-request table to the authorization middleware. Correlation belongs to the transport/session implementation. The 202 requirement is conditional on accepting the response, though, so please adjust the comment that says the transport is required to accept it.
A few contribution details to tidy up as well: commits 24049c75d, f2f74710f, and 327f0d372 are missing their Signed-off-by trailers. Please remove the fix(authz): title prefix and fill in the change-type and test-plan sections from the PR template. The green CI run is useful verification evidence; there's no need to repeat the same suite locally for this review.
Once the parser check and the affected-path coverage are sorted out, we can reassess whether this closes the original issue or should be scoped to the proxy behavior it fixes.
| if err != nil { | ||
| return false | ||
| } | ||
| _, ok := msg.(*jsonrpc2.Response) |
There was a problem hiding this comment.
Could we base this on a validated response envelope? The pinned jsonrpc2.DecodeMessage returns *jsonrpc2.Response for {"jsonrpc":"2.0","id":1} even though it has neither result nor error. It also accepts an envelope containing both. That means these invalid messages get the response marker and pass through authorization. The truncated-JSON test doesn't catch this because the JSON itself is valid.
This isn't evidence of a method-bearing request bypass, but it does broaden admission beyond the valid responses we're trying to allow. Current main already has the stricter mcp.DecodeMessage in ParsingMiddleware. After rebasing, please derive the marker from that already validated message and drop this second decode. Add regression cases for missing/both result and error so we keep that boundary intact.
| var handlerCalled bool | ||
| handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| handlerCalled = true | ||
| w.WriteHeader(http.StatusAccepted) |
There was a problem hiding this comment.
This proves the response reaches the next handler, but the stub supplies the 202 we're asserting. It doesn't show that the affected transport accepts the response or completes a pending server-initiated request.
There's a scope question here too: at this revision, vMCP no longer installs pkg/authz.Middleware. pkg/vmcp/server/server.go:703-713 describes the move to core admission and the SDK call gate, and pkg/vmcp/cli/serve.go:387-390 notes that the legacy authz middleware is ignored. So this is a useful proxy regression, but it doesn't establish closure of the vMCP issue.
Could you identify the currently affected deployment path and cover its actual transport/session flow? If the original vMCP behavior has already changed, let's say that in the PR and narrow the closure claim. For the middleware-level coverage, please also use a denying policy to show that valid responses pass while protected requests still fail.
Valid JSON-RPC responses are not authorization requests, but the middleware rejected them when no ParsedMCPRequest was available. Reuse the message already validated by ParsingMiddleware to identify responses, then leave session and response handling to the transport. Keep malformed envelopes rejected and retain Cedar authorization for method-bearing requests. Signed-off-by: King Star <mcxin.y@gmail.com>
4eb7c9c to
90c5c25
Compare
jstar0
changed the title
|
I traced the current supported paths after the review. The vMCP Given that, this change does not fix #5009 in the current vMCP path, and the only production HTTPProxy path I found has no supported server-request response flow for this middleware change to enable. I’m closing this PR rather than keep an unsupported scope. I’ll leave #5009 open for confirmation against a current build. |
2 participants
Summary
ParsingMiddlewarevalidates each POST withmcp.DecodeMessage, but only stores parsed method-bearing requests. A valid JSON-RPC response therefore looked like an unparsed body topkg/authz.Middlewareand received HTTP 400 before transport session handling.resultanderror, rejected byDecodeMessage.Related to #5009; this PR does not close #5009. The current vMCP
New/Servepath uses core admission rather thanpkg/authz.Middleware, and its real Legacy elicitation round-trip is covered byTestForwarding_Elicitation_RealBackend. This change is scoped to the HTTP authorization middleware used with the streamable proxy.Type of change
Test plan
task testwas run; see full-suite note below)task test-e2e)task lint-fix)Focused race tests passed for
pkg/authz,pkg/mcp, andpkg/transport/proxy/streamable.task lint-fixpassed with 0 issues.The full
task testsuite was run but did not complete cleanly in this environment. Remaining failures were unrelated to the changed packages: some tests require a container runtime, and network-error tests received HTTP 503 responses from the network proxy. No failures were reported in the three focused packages.Does this introduce a user-facing change?
Yes. With authorization enabled on the streamable proxy path, validated JSON-RPC client responses reach transport session handling instead of being rejected as malformed. Requests still receive normal Cedar decisions, and invalid envelopes remain rejected.
Special notes for reviewers
ParsingMiddlewarevalidates POST bodies independent ofContent-Type; this change follows that admission model and adds no MIME-type gate.