Non-2xx list responses are delivered as HTTP 200 unfiltered

[Yanhaoxi]

Bug description

ResponseFilteringWriter filters 2xx list responses (tools/list, prompts/list, resources/list, find_tool) against Cedar policies before forwarding. Its non-2xx passthrough branch (FlushAndFilter, pkg/authz/response_filter.go:81-85) is safe only on the assumption that the client actually observes a non-2xx status and therefore does not deliver the body (response.ok). That assumption is broken in the production transparent-proxy path:

• The authz middleware hands the real ResponseWriter to a httputil.ReverseProxy with FlushInterval:-1 (pkg/transport/proxy/transparent/transparent_proxy.go:1220-1221), which calls ResponseFilteringWriter.Flush() after every chunk it copies.
• Write/WriteHeader on the writer only buffer/record (response_filter.go:61-68), so the first thing to touch the underlying writer is Flush() (response_filter.go:161-166). The first Flush() on a net/http response writer commits the headers with an implicit WriteHeader(200) — the writer's own comment at :155-160 says so.
• FlushAndFilter() then reads the recorded statusCode (e.g. 500) and takes the non-2xx passthrough branch, where WriteHeader(500) is a no-op (headers already committed) and the unfiltered buffered body is written out.

Result: a backend answering a list method with any 3xx–5xx status (especially 4xx/5xx) delivers the full list to a response.ok-gated client as HTTP 200 — the disguised-result bypass family of #5257, on the non-2xx branch. In the same transparent-proxy path, where ReverseProxy invokes Flush() before FlushAndFilter(), every legitimate 4xx/5xx backend status is also rewritten to 200 (functional breakage: clients cannot distinguish success from failure at the HTTP layer).

Steps to reproduce

Verified on latest main (cfba5800, also present in the v0.42.1 release) with Go 1.26.5. The test below drives the relevant production wiring — a real HTTP server, ResponseFilteringWriter, and httputil.ReverseProxy with FlushInterval: -1 — with a Cedar authorizer that permits only weather. (The writer is constructed directly rather than via AuthorizationMiddleware; the exercised behavior is identical.)

// SPDX-License-Identifier: Apache-2.0

package authz

import (
	"context"
	"encoding/json"
	"fmt"
	"io"
	"net/http"
	"net/http/httptest"
	"net/http/httputil"
	"net/url"
	"testing"

	"github.com/golang-jwt/jwt/v5"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"golang.org/x/exp/jsonrpc2"

	"github.com/stacklok/toolhive-core/mcpcompat/mcp"
	"github.com/stacklok/toolhive/pkg/auth"
	"github.com/stacklok/toolhive/pkg/authz/authorizers/cedar"
	mcpparser "github.com/stacklok/toolhive/pkg/mcp"
)

func TestRepro_Non2xxListSmuggledAs200(t *testing.T) {
	authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{
		Policies:     []string{`permit(principal, action == Action::"call_tool", resource == Tool::"weather");`},
		EntitiesJSON: `[]`,
	}, "")
	require.NoError(t, err)

	backendResult := mcp.ListToolsResult{Tools: []mcp.Tool{
		{Name: "weather", Description: "Get weather information"},
		{Name: "calculator", Description: "Perform calculations"},
		{Name: "admin_tool", Description: "Administrative operations"},
	}}
	resultData, err := json.Marshal(backendResult)
	require.NoError(t, err)
	backendRPCResponse := &jsonrpc2.Response{ID: jsonrpc2.Int64ID(1), Result: json.RawMessage(resultData)}
	backendBody, err := jsonrpc2.EncodeMessage(backendRPCResponse)
	require.NoError(t, err)

	// Backend returns the same list body with a caller-chosen status code.
	backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		code := http.StatusOK
		if v := r.URL.Query().Get("code"); v != "" {
			fmt.Sscanf(v, "%d", &code)
		}
		w.Header().Set("Content-Type", "application/json")
		w.WriteHeader(code)
		_, _ = w.Write(backendBody)
	}))
	defer backend.Close()
	backendURL, _ := url.Parse(backend.URL)

	// Frontend: authz-middleware + transparent-proxy wiring.
	frontend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
			Subject: "user123", Name: "Test User",
			Claims: jwt.MapClaims{"sub": "user123", "name": "Test User"},
		}}
		ctx := auth.WithIdentity(r.Context(), identity)
		parsed := &mcpparser.ParsedMCPRequest{Method: string(mcp.MethodToolsList), ID: float64(1)}
		ctx = context.WithValue(ctx, mcpparser.MCPRequestContextKey, parsed)
		r = r.WithContext(ctx)

		filteringWriter := NewResponseFilteringWriter(w, authorizer, r, string(mcp.MethodToolsList), nil, nil)
		proxy := httputil.NewSingleHostReverseProxy(backendURL)
		proxy.FlushInterval = -1 // production transparent proxy
		proxy.ServeHTTP(filteringWriter, r)
		require.NoError(t, filteringWriter.FlushAndFilter())
	}))
	defer frontend.Close()

	// Control: a 200 list response IS filtered (admin_tool removed).
	resp200, _ := http.Get(frontend.URL + "/mcp")
	body200, _ := io.ReadAll(resp200.Body)
	resp200.Body.Close()
	require.Equal(t, http.StatusOK, resp200.StatusCode)
	assert.Contains(t, string(body200), "weather")
	assert.NotContains(t, string(body200), "admin_tool", "control failed: 2xx list must be filtered")

	// Bypass: the same list answered with 500 is delivered as 200, unfiltered.
	resp500, _ := http.Get(frontend.URL + "/mcp?code=500")
	body500, _ := io.ReadAll(resp500.Body)
	resp500.Body.Close()

	t.Logf("BUG: backend returned 500, client received HTTP %d", resp500.StatusCode)
	t.Logf("BUG: unfiltered body byte-equal to backend body: %v", string(body500) == string(backendBody))

	assert.Equal(t, http.StatusOK, resp500.StatusCode,
		"BUG: non-2xx backend status silently rewritten to 200 (Flush() committed implicit 200 before FlushAndFilter)")
	assert.Equal(t, string(backendBody), string(body500),
		"BUG: non-2xx list body delivered unchanged")
	assert.Contains(t, string(body500), "admin_tool",
		"BUG: denied tool reached the client, bypassing the authz response filter")
}

Run:

go test -v ./pkg/authz -run '^TestRepro_Non2xxListSmuggledAs200$' -count=1

Verified output (the superfluous response.WriteHeader call lines are the no-op WriteHeader(500) after Flush() already committed the headers as 200):

=== RUN   TestRepro_Non2xxListSmuggledAs200
http: superfluous response.WriteHeader call from github.com/stacklok/toolhive/pkg/authz.(*ResponseFilteringWriter).processJSONResponse (response_filter.go:199)
http: superfluous response.WriteHeader call from github.com/stacklok/toolhive/pkg/authz.(*ResponseFilteringWriter).FlushAndFilter (response_filter.go:82)
    response_filter_non2xx_bypass_test.go:123: BUG: backend returned 500, client received HTTP 200
    response_filter_non2xx_bypass_test.go:124: BUG: unfiltered body byte-equal to backend body: true
    response_filter_non2xx_bypass_test.go:133: client received body: {"jsonrpc":"2.0","id":1,"result":{"tools":[{"annotations":{},"description":"Get weather information","inputSchema":{},"name":"weather"},{"annotations":{},"description":"Perform calculations","inputSchema":{},"name":"calculator"},{"annotations":{},"description":"Administrative operations","inputSchema":{},"name":"admin_tool"}]}}
--- PASS: TestRepro_Non2xxListSmuggledAs200 (0.00s)

The same bypass applies to any 3xx–5xx status (especially 4xx/5xx) for the list-class methods; with a plain JSON-RPC error body (no result) the list does not leak, but in the transparent-proxy path the status is still rewritten to 200.

Expected behavior

The non-2xx passthrough is only safe if the client actually observes the non-2xx status (FlushAndFilter's own comment at response_filter.go:73-80, and the existing test "non-2xx error response passes through unfiltered" at response_filter_test.go:2042-2047 which asserts the original status is preserved). The wire status must therefore reflect the recorded backend status: a 500 list response must reach the client as 500 (client discards it), and a legitimate 4xx/5xx must not be rewritten to 200.

Actual behavior

In the production transparent-proxy chain, Flush() (invoked by ReverseProxy with FlushInterval:-1 while copying the response) commits the headers with an implicit 200 before FlushAndFilter() runs. FlushAndFilter() then takes the non-2xx passthrough branch based on the recorded 500, its WriteHeader(500) is a no-op, and the unfiltered buffered body is delivered as HTTP 200. Verified above: admin_tool (denied by policy) reaches the client.

Environment (if relevant)

• Go version: go1.26.5
• ToolHive version: cfba5800 (latest main), also present in the v0.42.1 release
• Affected: pkg/authz/response_filter.go (Flush :161-166; non-2xx passthrough :81-85); production trigger pkg/transport/proxy/transparent/transparent_proxy.go:1220-1221, wrapped by pkg/authz/middleware.go:262-268 (list methods) and :364-366 (find_tool). Existing coverage only reproduces the "flush before FlushAndFilter" ordering on a 2xx (TestResponseFilteringWriter_ContentLengthMismatch, response_filter_test.go:612-619); the non-2xx tests use a bare httptest.NewRecorder with no flush interaction.

Additional context

• Reporting precedent: same bug family as the publicly-filed authz: SSE response filter leaks unfiltered tools/list on undecodable / non-Response data lines (bypasses cedar) #5257 (authz response filter bypass), and as BOM-prefixed list responses bypass authz response filtering #6301 (BOM prefix). Both were root causes in body content recognition; this one is a separate root cause in the status-commit ordering (deferred write vs early streaming flush), not covered by either.
• Threat model: an upstream/malicious backend (or ordinary tooling) needs only mark a list response non-2xx (e.g. 500) to bypass filtering and have the full tools/prompts/resources list delivered to the client as 200. The filter exists precisely to defend the client against such backends.
• Suggested fix:
  1. Root cause — fix in (*ResponseFilteringWriter).Flush() (response_filter.go:161-166): after deleting Content-Length, commit the recorded status before flushing downstream, i.e. rfw.ResponseWriter.WriteHeader(rfw.statusCode) followed by flusher.Flush(). The wire status then reflects the real backend status instead of the implicit 200; SSE (statusCode 200) is unaffected and the non-2xx passthrough precondition is restored.
  2. Defense in depth (optional, not the root fix): in the non-2xx passthrough branch (response_filter.go:81-85), fail closed when the body still carries a JSON-RPC result (reuse carriesResult/sseCarriesResult) instead of trusting the status code alone.