From 51eb938bc78b405d558cd26131a233c250cd355f Mon Sep 17 00:00:00 2001 From: haoxiang Yan <108142374+Yanhaoxi@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:48:54 +0800 Subject: [PATCH] fix(authz): commit recorded status before flushing in ResponseFilteringWriter ResponseFilteringWriter is write-behind: Write buffers the body and WriteHeader only records the status, with the real write and filter happening in FlushAndFilter. Its non-2xx passthrough branch is safe only because a fetch-based MCP client gates list delivery on response.ok (200-299), so a non-2xx list body is never consumed. In the production transparent-proxy path (httputil.ReverseProxy with FlushInterval: -1), the proxy calls Flush() while copying the backend response. The first Flush() on a fresh net/http writer commits the headers with an implicit WriteHeader(200) before FlushAndFilter() runs. FlushAndFilter then takes the non-2xx passthrough branch from the recorded status (e.g. 500), its WriteHeader(500) is a no-op, and the unfiltered buffered list body is delivered under a fabricated 200 -- the #5257-class bypass on the non-2xx branch. Legitimate 4xx/5xx statuses are silently rewritten to 200 as well. Commit the recorded status (rfw.statusCode) in Flush() before flushing downstream, so the wire status reflects the real backend status. SSE (statusCode 200) is unaffected and later WriteHeader calls in FlushAndFilter become no-ops instead of corrupting the status. Adds a regression test over the production wiring (real HTTP server + ReverseProxy FlushInterval:-1) asserting a 500/404 list response reaches the client with the non-2xx status while 2xx responses are still filtered. The test fails without the fix (status rewritten to 200). --- pkg/authz/response_filter.go | 10 ++ .../response_filter_status_commit_test.go | 113 ++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 pkg/authz/response_filter_status_commit_test.go diff --git a/pkg/authz/response_filter.go b/pkg/authz/response_filter.go index 027a38261a..76cc0ddc51 100644 --- a/pkg/authz/response_filter.go +++ b/pkg/authz/response_filter.go @@ -158,9 +158,19 @@ func (rfw *ResponseFilteringWriter) FlushAndFilter() error { // implicit WriteHeader(200), sending headers to the wire. If the stale // Content-Length is still present at that point, it's too late to remove it in // FlushAndFilter(). +// +// Commit the recorded status before the first flush. Without this, the implicit +// 200 would also rewrite a non-2xx backend status (e.g. 500) to 200 on the +// wire, defeating the non-2xx passthrough precondition in FlushAndFilter(): a +// fetch-based MCP client gates list delivery on response.ok, so an unfiltered +// list body would be delivered under a fabricated 200. Committing here keeps +// the wire status identical to the recorded backend status; SSE (statusCode +// 200) is unaffected and later WriteHeader calls in FlushAndFilter become +// no-ops instead of corrupting the status. func (rfw *ResponseFilteringWriter) Flush() { if flusher, ok := rfw.ResponseWriter.(http.Flusher); ok { rfw.ResponseWriter.Header().Del("Content-Length") + rfw.ResponseWriter.WriteHeader(rfw.statusCode) flusher.Flush() } } diff --git a/pkg/authz/response_filter_status_commit_test.go b/pkg/authz/response_filter_status_commit_test.go new file mode 100644 index 0000000000..a85d7d8805 --- /dev/null +++ b/pkg/authz/response_filter_status_commit_test.go @@ -0,0 +1,113 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// 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" +) + +// TestResponseFilteringWriter_Non2xxStatusPreservedOnWire reproduces the +// production transparent-proxy wiring (real HTTP server + httputil.ReverseProxy +// with FlushInterval:-1 + ResponseFilteringWriter) and asserts that a non-2xx +// backend status survives on the wire. Regression for the bypass where Flush() +// committed an implicit 200 before FlushAndFilter() ran, so the non-2xx +// passthrough branch (safe only when the client actually observes a non-2xx +// status) delivered an unfiltered list body under a fabricated 200. +func TestResponseFilteringWriter_Non2xxStatusPreservedOnWire(t *testing.T) { + t.Parallel() + + 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 mirrors the 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() + + do := func(query string) (*http.Response, []byte) { + resp, err := http.Get(frontend.URL + "/mcp" + query) + require.NoError(t, err) + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + return resp, body + } + + // 2xx list responses must still be filtered (admin_tool removed). + resp200, body200 := do("") + assert.Equal(t, http.StatusOK, resp200.StatusCode) + assert.Contains(t, string(body200), "weather") + assert.NotContains(t, string(body200), "admin_tool") + + // Non-2xx list responses must reach the client with the non-2xx status, so + // the passthrough precondition (client gates list delivery on response.ok) + // holds. + for _, code := range []int{http.StatusInternalServerError, http.StatusNotFound} { + resp, body := do(fmt.Sprintf("?code=%d", code)) + assert.Equalf(t, code, resp.StatusCode, + "non-2xx backend status %d was rewritten on the wire; the unfiltered list body would be delivered as 200", code) + // The passthrough branch is intentionally body-preserving for error + // responses; the security property is that the client sees the non-2xx + // status and does not deliver the body. + assert.Equal(t, string(backendBody), string(body)) + } +}