Skip to content

Commit 0ce2811

Browse files
reyortiz3claude
andauthored
Merge commit from fork
The streamable proxy matched backend responses to waiting requests by a key built from the session ID and the client-chosen JSON-RPC id. Two in-flight requests in one session with the same id shared a key, so the second registration overwrote the first and a response could be delivered to the wrong request, bypassing the authz list filter. Mint a random per-request token in createWaiter and use it as the upstream correlation key instead. The client's original id is still restored on the reply through idRestore. Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
1 parent 9d0b2e4 commit 0ce2811

4 files changed

Lines changed: 102 additions & 43 deletions

File tree

‎pkg/transport/proxy/streamable/dispatcher.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ func (p *HTTPProxy) rejectServerRequestToBackend(m *jsonrpc2.Request) {
191191
}
192192

193193
// routeResponseToWaiter delivers resp to the waiter channel correlated by its
194-
// composite request ID (compositeKey(sessID, idKey)), if one is registered.
194+
// composite request ID (the composite key), if one is registered.
195195
func (p *HTTPProxy) routeResponseToWaiter(resp *jsonrpc2.Response) {
196196
if !resp.ID.IsValid() {
197197
slog.Warn("received invalid message that is not a valid response",
@@ -200,7 +200,7 @@ func (p *HTTPProxy) routeResponseToWaiter(resp *jsonrpc2.Response) {
200200
}
201201

202202
rawID := resp.ID.Raw()
203-
// Composite-only routing: responses must carry composite ID (sessID|idKey)
203+
// Composite-only routing: responses must carry the composite key minted by createWaiter
204204
sID, ok := rawID.(string)
205205
if !ok {
206206
slog.Warn("non-string response id (expected composite string); dropping",

‎pkg/transport/proxy/streamable/dispatcher_test.go‎

Lines changed: 65 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,9 @@ func TestDispatchResponses_ResponseRoutesToWaiter(t *testing.T) {
3333
t.Parallel()
3434
proxy := newDispatcherTestProxy(t)
3535

36-
waitCh, cleanup := proxy.createWaiter("sess-1", jsonrpc2.StringID("req-1"))
36+
waitCh, ck, cleanup, err := proxy.createWaiter("sess-1", jsonrpc2.StringID("req-1"))
37+
require.NoError(t, err)
3738
defer cleanup()
38-
ck := compositeKey("sess-1", idKeyFromID(jsonrpc2.StringID("req-1")))
3939

4040
resp, err := jsonrpc2.NewResponse(jsonrpc2.StringID(ck), "result", nil)
4141
require.NoError(t, err)
@@ -50,6 +50,69 @@ func TestDispatchResponses_ResponseRoutesToWaiter(t *testing.T) {
5050
}
5151
}
5252

53+
// TestDoRequest_SameSessionSameIDDoesNotCrossResponses is the regression test
54+
// for GHSA-5cgx-5frr-fgv5: a client chooses its own JSON-RPC id, so two
55+
// in-flight requests in one session may carry the same id. Each must receive
56+
// the backend response to its own request (never the other's), and each must
57+
// see its original id restored.
58+
func TestDoRequest_SameSessionSameIDDoesNotCrossResponses(t *testing.T) {
59+
t.Parallel()
60+
proxy := newDispatcherTestProxy(t)
61+
62+
methods := []string{"tools/list", "ping"}
63+
type outcome struct {
64+
method string
65+
msg jsonrpc2.Message
66+
err error
67+
}
68+
outcomes := make(chan outcome, len(methods))
69+
for _, method := range methods {
70+
go func() {
71+
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
72+
defer cancel()
73+
req := &jsonrpc2.Request{ID: jsonrpc2.Int64ID(1), Method: method}
74+
msg, err := proxy.doRequest(ctx, "sess-1", req)
75+
outcomes <- outcome{method: method, msg: msg, err: err}
76+
}()
77+
}
78+
79+
// Fake backend: collect both in-flight requests, then answer them in the
80+
// reverse of arrival order, tagging each result with the request's method.
81+
var inflight []*jsonrpc2.Request
82+
for len(inflight) < len(methods) {
83+
select {
84+
case msg := <-proxy.messageCh:
85+
req, ok := msg.(*jsonrpc2.Request)
86+
require.True(t, ok)
87+
inflight = append(inflight, req)
88+
case <-time.After(2 * time.Second):
89+
t.Fatal("timed out waiting for both requests to reach the backend")
90+
}
91+
}
92+
for i := len(inflight) - 1; i >= 0; i-- {
93+
resp, err := jsonrpc2.NewResponse(inflight[i].ID, inflight[i].Method, nil)
94+
require.NoError(t, err)
95+
proxy.responseCh <- resp
96+
}
97+
98+
for range methods {
99+
select {
100+
case got := <-outcomes:
101+
require.NoError(t, got.err, "request %q must not time out or fail", got.method)
102+
resp, ok := got.msg.(*jsonrpc2.Response)
103+
require.True(t, ok)
104+
assert.JSONEq(t, `"`+got.method+`"`, string(resp.Result),
105+
"request %q received another request's response", got.method)
106+
assert.Equal(t, jsonrpc2.Int64ID(1), resp.ID, "original client id must be restored")
107+
case <-time.After(5 * time.Second):
108+
t.Fatal("timed out waiting for doRequest to return")
109+
}
110+
}
111+
112+
assert.Zero(t, syncMapLen(&proxy.waiters), "waiters must be cleaned up")
113+
assert.Zero(t, syncMapLen(&proxy.idRestore), "idRestore must be cleaned up")
114+
}
115+
53116
// TestDispatchResponses_UnknownCompositeKeyResponseIsDropped verifies a
54117
// *jsonrpc2.Response whose composite ID has no registered waiter is dropped
55118
// without blocking or panicking.

‎pkg/transport/proxy/streamable/streamable_proxy.go‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,13 @@ type HTTPProxy struct {
8989
// Keys are URL path prefixes. Set via WithPrefixHandlers.
9090
prefixHandlers map[string]http.Handler
9191

92-
// Waiters keyed by compositeKey(sessID, idKey) -> one-shot channel for response delivery.
93-
// The composite key MUST be unique per concurrent request; sharing it across requests
94-
// (e.g. with sessID="" for sessionless requests) silently overwrites entries and crosses
95-
// response payloads between unrelated clients. See resolveSessionForRequest.
92+
// Waiters keyed by compositeKey(sessID, per-request token) -> one-shot channel for response
93+
// delivery. The composite key MUST be unique per concurrent request; sharing it across
94+
// requests silently overwrites entries and crosses response payloads between requests.
95+
// createWaiter guarantees this by minting the token itself, because neither the session
96+
// ID (empty for sessionless requests) nor the client-chosen JSON-RPC ID is unique.
9697
waiters sync.Map // map[string]chan jsonrpc2.Message
97-
// Keyed by the same compositeKey(sessID, idKey); stores the original client JSON-RPC ID
98+
// Keyed by the same composite key; stores the original client JSON-RPC ID
9899
// to restore before replying. Same uniqueness requirement as `waiters`.
99100
idRestore sync.Map // map[string]jsonrpc2.ID
100101

@@ -1026,10 +1027,10 @@ func (p *HTTPProxy) restoreResponseID(resp *jsonrpc2.Response, ck string) (jsonr
10261027
func (p *HTTPProxy) sendCorrelatedRequest(
10271028
sessID string, req *jsonrpc2.Request,
10281029
) (waitCh chan jsonrpc2.Message, ck string, cleanup func(), err error) {
1029-
key := idKeyFromID(req.ID)
1030-
ck = compositeKey(sessID, key)
1031-
1032-
waitCh, cleanup = p.createWaiter(sessID, req.ID)
1030+
waitCh, ck, cleanup, err = p.createWaiter(sessID, req.ID)
1031+
if err != nil {
1032+
return nil, "", nil, err
1033+
}
10331034

10341035
proxiedMsg, err := encodeRequestWithID(req, ck)
10351036
if err != nil {
@@ -1099,11 +1100,12 @@ func (p *HTTPProxy) ensureSession(r *http.Request, id string) error {
10991100
// in-process routing token (not registered with sessionManager). Sessionless
11001101
// routing tokens MUST be unique per request: sharing one (e.g. the empty string,
11011102
// or a stale/foreign Mcp-Session-Id) across concurrent sessionless requests with
1102-
// the same JSON-RPC id collapses them onto the same compositeKey(sessID, idKey)
1103-
// and overwrites entries in the waiters / idRestore sync.Maps, leaking one
1103+
// the same JSON-RPC id could otherwise share routing state and leak one
11041104
// client's response payload to another. This is a confidentiality bug, not a
11051105
// performance issue -- do not collapse the token, and do not let a Modern
1106-
// request fall through to the session-lookup path below.
1106+
// request fall through to the session-lookup path below. (The waiter key
1107+
// itself is additionally made unique per request by createWaiter, so two
1108+
// requests in one session reusing an id are isolated too.)
11071109
//
11081110
// Writes HTTP errors on failure and returns error to stop handling.
11091111
func (p *HTTPProxy) resolveSessionForRequest(
@@ -1652,10 +1654,21 @@ func resolveRequestTimeout() time.Duration {
16521654
return defaultRequestTimeout
16531655
}
16541656

1655-
// createWaiter registers a waiter channel for the given request ID and returns cleanup fn.
1656-
func (p *HTTPProxy) createWaiter(sessID string, id jsonrpc2.ID) (chan jsonrpc2.Message, func()) {
1657-
key := idKeyFromID(id)
1658-
ck := compositeKey(sessID, key)
1657+
// createWaiter registers a waiter channel for a request and returns the channel,
1658+
// the composite key (ck) the request must carry upstream, and a cleanup fn.
1659+
//
1660+
// ck is minted here from a per-request random token, never from the
1661+
// client-chosen JSON-RPC id: a client may have several requests with the same id
1662+
// in flight in one session, and a key derived from the id would let them
1663+
// overwrite each other's waiter and receive each other's responses (including
1664+
// responses the authz response filter never saw). The client's original id is
1665+
// kept in idRestore and put back on the reply.
1666+
func (p *HTTPProxy) createWaiter(sessID string, id jsonrpc2.ID) (chan jsonrpc2.Message, string, func(), error) {
1667+
token, err := uuid.NewRandom()
1668+
if err != nil {
1669+
return nil, "", nil, fmt.Errorf("generate request correlation token: %w", err)
1670+
}
1671+
ck := compositeKey(sessID, token.String())
16591672
// store original client id to restore before replying
16601673
p.idRestore.Store(ck, id)
16611674

@@ -1666,5 +1679,5 @@ func (p *HTTPProxy) createWaiter(sessID string, id jsonrpc2.ID) (chan jsonrpc2.M
16661679
p.waiters.Delete(ck)
16671680
p.idRestore.Delete(ck)
16681681
}
1669-
return ch, cleanup
1682+
return ch, ck, cleanup, nil
16701683
}

‎pkg/transport/proxy/streamable/utils.go‎

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -135,28 +135,11 @@ func writeJSONRPC(w http.ResponseWriter, msg jsonrpc2.Message) error {
135135
return err
136136
}
137137

138-
// idKeyFromID returns a stable string key for a jsonrpc2.ID using its raw value.
139-
// We prefix with type to avoid collisions between numeric and string IDs.
140-
func idKeyFromID(id jsonrpc2.ID) string {
141-
raw := id.Raw()
142-
switch v := raw.(type) {
143-
case string:
144-
return "s:" + v
145-
case float64:
146-
// JSON numbers decode to float64
147-
return fmt.Sprintf("n:%v", v)
148-
case json.Number:
149-
return "n:" + v.String()
150-
case nil:
151-
return "nil"
152-
default:
153-
return fmt.Sprintf("%T:%v", v, v)
154-
}
155-
}
156-
157-
// compositeKey builds a stable composite key from session ID and request ID key.
158-
func compositeKey(sessID, idKey string) string {
159-
return sessID + "|" + idKey
138+
// compositeKey builds the upstream correlation key from the session ID and a
139+
// per-request token. The token must be unique per in-flight request (see
140+
// createWaiter); it must never be derived from the client-chosen JSON-RPC id.
141+
func compositeKey(sessID, requestToken string) string {
142+
return sessID + "|" + requestToken
160143
}
161144

162145
// extractStringParam returns params[key] as a string, and whether it was

0 commit comments

Comments
 (0)