fix: remove hardcoded * allowed origin for sse#3054
Conversation
Yuan325
force-pushed
the
remove-sse-access-control
branch
from
37bf148 to
09af442
Compare
kurtisvg
left a comment
kurtisvg
left a comment
There was a problem hiding this comment.
Can we ensure we add a regression test?
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the hardcoded 'Access-Control-Allow-Origin' header from the SSE handler. Feedback was provided to further improve the SSE response headers for better security and compatibility, including adding 'no-transform' to Cache-Control, setting 'X-Accel-Buffering' and 'X-Content-Type-Options', and ensuring the 'Connection' header is only set for HTTP/1.1 requests.
Yuan325
force-pushed
the
remove-sse-access-control
branch
from
09af442 to
8b2af71
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Addr() method to the Server struct and removes the hardcoded wildcard Access-Control-Allow-Origin header from the SSE handler to support configurable CORS. It also adds comprehensive security tests for origin and host validation. The review feedback identifies several issues in the newly added tests, including goroutine leaks caused by unbuffered and unread error channels when starting the test server, and potential panics due to unhandled errors from http.NewRequest calls.
Yuan325
force-pushed
the
remove-sse-access-control
branch
from
56fdbd1 to
f66d3da
Compare
|
🧨 Preview deployments removed. Cloudflare Pages environments for |
3 participants
Remove the hardcoded "Access-Control-Allow-Origin" header in sseHandler. This are only affecting users that are using SSE via MCP specs from v2024-11-05.
Tested with MCP inspector that removing this does not break SSE connection. Added regression test for checking security with allowedOrigin and allowedHost values.
🛠️ Fixes #3053