fix(authz): reject non-JSON POSTs instead of skipping authorization

[SashaMIT]

Problem

shouldSkipInitialAuthorization skipped authorization for POSTs whose Content-Type isn't application/json:

if r.Method != http.MethodPost || !strings.HasPrefix(r.Header.Get("Content-Type"), "application/json") {
    return true   // skip authz, call next handler
}

But skipping authz doesn't stop the request. The transparent proxy still forwards the body verbatim (its own batch guard runs "independently of Content-Type ... so a batch smuggled under a non-JSON content type cannot reach the backend" — the codebase already treats this smuggling class as in-scope), and MCP backends parse JSON-RPC without checking Content-Type. Net effect: POST /mcp with Content-Type: text/plain and a tools/call body executes with no Cedar evaluation at all.

Fix

A non-JSON POST no longer skips; it falls through to the existing parsed-request check, which rejects it with 400 Invalid or malformed MCP request (no parsed MCP request exists for it by construction). JSON POSTs and non-POST methods are unchanged.

Tests

Added TestMiddlewareRejectsNonJSONPost: a text/plain POST carrying a tools/call body must not reach the inner handler even under a permit-everything Cedar policy. Full pkg/authz suite passes.

Made with Cursor

Made with Cursor

[jhrozek]

Had a close look at this. The vulnerability is real and correctly diagnosed: I traced it through parser.go:225-246 (skips parsing on non-JSON, so nothing lands in context), the old skip here, and transparent_proxy.go:585-626 which forwards the body regardless of Content-Type. A tools/call under text/plain did reach the backend with no Cedar evaluation. git log says the skip has been there since at least 29a2c67 in March, so it's a latent gap rather than a recent regression. Direction of the fix is right and it's fail-closed.

One thing that will bite immediately: the test file isn't gofmt-clean, and no checks have run on this branch yet, so the lint failure isn't visible in the PR status.

The substantive one is a question about placement rather than about the logic, left inline on the comment block. Short version: the guard only exists when a Cedar policy is configured, and a few sibling middlewares that fail open on a missing parsed request run outer to authz, so their coverage here comes from chain position rather than anything explicit. Fine by me if that's a follow-up, I just want the remaining paths written down somewhere.

Also worth checking whether the newly-rejected requests deserve a release note. A client sending no Content-Type, or an uppercased one, now gets a 400 where it used to work, and 'Invalid or malformed MCP request' won't point anyone at their headers.

Rest is smaller stuff.

[jhrozek]

This block isn't sorted, so gofmt -l pkg/authz/ flags the file and the lint job will fail on it. strings belongs between os and testing; task lint-fix will move it.

No checks have reported on this branch yet, so this doesn't show up in the PR status right now.

[jhrozek]

Question about where this belongs rather than a problem with the logic.

The guard lives in the authz middleware, which is only added to the chain when a Cedar policy is configured (pkg/runner/middleware.go:213). Three other middlewares pass through when there's no parsed request in context, and all three run outer to authz:

• pkg/webhook/validating/middleware.go:95
• pkg/webhook/mutating/middleware.go:86
• pkg/ratelimit/middleware.go:131

So on a workload with --tools=deploy plus a validating webhook and no Cedar policy, a text/plain POST calling an allowlisted tool still skips the webhook's deny decision and the rate limiter, and reaches the backend. When authz is configured the new 400 stops it before any of that matters, which means those controls end up covered by chain position rather than by anything anyone declared.

The batch guard you cite in the description was deliberately put at the executor for what reads like this exact reason (streamable_proxy.go:549-553: "independent of middleware presence, ordering, and Content-Type"). Would rejecting in handlePost alongside it make more sense? For what it's worth I don't think it can go in ParsingMiddleware, or stay here, on the transparent proxy path: that mux registers the chain on catch-all / (transparent_proxy.go:1341), so a containerised backend's own non-MCP POST routes sit behind it and would start 400ing.

Entirely happy for this to be a follow-up issue if you'd rather keep the PR narrow. Mainly want the remaining paths known rather than assumed closed.

For the record, two things I checked that are not affected: the --tools filter reads and unmarshals the raw body regardless of Content-Type (tool_filter.go:249-263), and audit logs the request either way (audit/auditor.go:204-268).

[jhrozek]

HasPrefix is case sensitive, but media types aren't (RFC 9110 8.3.1), so Content-Type: Application/JSON now gets a 400. Before this change it took the skip path, so it was unauthorised but working; now it's broken. Fail-closed so not a security problem, but it is a behaviour change for anyone not sending lowercase, and the same goes for any header shape HasPrefix can't model.

tool_filter.go:349-365 already solves this with mime.ParseMediaType plus strings.EqualFold if you want the in-repo pattern.

Worth noting parser.go:232 carries the identical case-sensitive check, so changing only this side would leave the two disagreeing about what counts as JSON.

[jhrozek]

This says "refusing here", but the function doesn't refuse anything, it returns false. The actual rejection is the parsedRequest == nil branch at line 208, and that branch's own comment attributes the 400 to "a malformed request or missing parsing middleware", neither of which is what happened. Nothing there records that it's now load-bearing for security.

That's what I'd worry about long term: passing through on a nil parsed request is the established pattern elsewhere in this chain (ratelimit and both webhook middlewares do exactly that). Someone harmonising this with them, or chasing a false-positive 400 report from some non-MCP POST path, could flip line 208 to next.ServeHTTP and reopen the hole with no test failure outside pkg/authz, while this comment still says it's closed.

An explicit early return in the middleware body would put the refusal where the comment claims it is. Failing that, a line at 208 saying non-JSON POSTs land there deliberately and mustn't be passed through.

[jhrozek]

Where this lands is the generic 400 Invalid or malformed MCP request at line 211, sent as plain text. The body isn't malformed though, the media type is unsupported, so 415 would be the accurate status, and every other rejection in this file goes out as a JSON-RPC error through handleUnauthorized / mcp.WriteJSONRPCError. A client that expects a JSON-RPC envelope may surface a parse error rather than the real reason.

The audit side is the part I'd actually push on. With no ParsedMCPRequest in context, mcpMethodFor returns "" (audit/auditor.go:341), determineEventType falls back to a generic EventTypeHTTPRequest (:364), and determineOutcome maps 400 to "failure" rather than "denied", since that only covers 401/403 (:276). So someone sweeping tools/call under text/plain is now correctly blocked but produces events with no method, no tool and no denial classification, indistinguishable from ordinary client errors. A block that can't be alerted on is worth a fair bit less, given the whole point of the change is closing a security hole.

No 415 exists anywhere in the repo today, so if that's deliberate house style, ignore the status half and just take the audit part.

[jhrozek]

Good call using a permit-everything policy here. That's what makes the test meaningful, since a deny policy couldn't distinguish "authz never ran" from an ordinary denial.

Three cases I'd add, mostly because the refactor moved the SSE check behind the Content-Type check:

• application/json; charset=utf-8 carrying a valid tools/call, asserting it still authorises and reaches the handler. Most common real-world header, and the thing most likely to break if this check gets reworked later.
• POST with no Content-Type at all. That now gets a 400 where it previously passed through, so worth pinning so it's clearly intended rather than incidental.
• A POST to a path ending /sse. The skip at line 132 is only reachable by JSON POSTs now, so a non-JSON POST there 400s instead of skipping. Low impact in practice, since httpsse registers /sse GET-only, but it's reachable via the transparent proxy's catch-all and it's an unmentioned side effect of the reordering.

TestMiddlewareWithGETRequest already covers the non-POST half of the split, so that one's fine.

[SashaMIT]

Thanks jhrozek, all addressed in the new head. gofmt fixed (import sort). The refusal is now an explicit early return in the middleware body with a comment recording it is load-bearing, and the nil-parsed-request branch notes non-JSON POSTs are covered upstream. Media-type matching now uses mime.ParseMediaType + EqualFold per RFC 9110, and I updated parser.go's identical check so the two cannot disagree. Blocked sweeps now classify as audit denials rather than generic 400 failures, so they are alertable. Tests added for the charset variant, missing Content-Type, the /sse path, and an uppercase Application/JSON. On placement: agreed, the follow-up should document the remaining paths (webhook + ratelimit outer to authz on a no-Cedar workload); I will open that issue once this lands. On the release note: yes, a client sending no Content-Type or a non-lowercase one now gets a 400, worth a line in the changelog.