fix: reject cross-origin endpoint events in SSE client - #958
Conversation
6fc1be3 to
fa252df
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new security boundary isn’t fully covered by tests (notably the “same host but different port” origin-mismatch case), which increases regression risk for a security-sensitive change.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| fun `full url endpoint with a different origin is rejected`() = runTest { | ||
| // Given | ||
| val sseUrl = "http://example.com/api/mcp/sse" | ||
|
|
||
| // And |
devcrocod
left a comment
devcrocod
left a comment
There was a problem hiding this comment.
there are a few critical comments, please take a look
also note that ktlint failed on ci
| * The comparison uses the authority string, so default ports are ignored (e.g. `http://host:80` | ||
| * and `http://host` are considered the same origin). | ||
| */ | ||
| private fun Url.hasSameOrigin(): Boolean = protocolWithAuthority == origin |
There was a problem hiding this comment.
A raw string is compared, which can also include UserAndPassword. This can cause rejection in two cases, when credentials are present or when there is a case mismatch.
Also, why is this moved to a separate function? It is not used anywhere else, and it also increases complexity
| val endpointOrigin = Url(eventData) | ||
| if (!endpointOrigin.hasSameOrigin()) { | ||
| val error = IllegalArgumentException( | ||
| "Endpoint origin ${endpointOrigin.protocolWithAuthority} does not match connection origin $origin", |
There was a problem hiding this comment.
credentials might leak into error messages
There was a problem hiding this comment.
I can't find a test for port mismatch, which seems like a frequent use case
In the HTTP+SSE transport, the server's
endpointevent tells the client where to POST subsequent JSON-RPC messages. The client currently accepts a fullhttp(s)://URL as-is, so a compromised server could redirect all subsequent traffic — including auth headers — to an attacker-controlled host. The TypeScript and Python clients already reject endpoints whose origin differs from the connection origin.This change rejects full-URL endpoints whose origin (scheme, host, and port, with default ports normalized) does not match the SSE connection's origin: the endpoint future completes exceptionally and the transport fails to start. Relative and root-relative paths keep their existing resolution, and same-origin full URLs keep their existing behavior.