security: add authentication and origin validation to WebSocket #414
Conversation
Add session authentication and origin header validation to the WebSocket endpoint to prevent unauthorized access. - Add checkWebSocketOrigin() for origin header validation - Add AuthenticatedWebSocketHandler() requiring valid session - Update main.go to use authenticated handler - Support ALLOWED_ORIGIN and FRONTEND_ORIGIN_DEV env vars - Allow localhost in development mode - Log rejected connections for security monitoring Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
backend/controllers/websocket.go
Outdated
|
|
||
| // If no ALLOWED_ORIGIN configured, check if origin matches the request host | ||
| host := r.Host | ||
| if strings.Contains(origin, host) { |
There was a problem hiding this comment.
might be insecure,can parse the origin somehow and then compare hostname equality, not substrings.
backend/controllers/websocket.go
Outdated
| } | ||
|
|
||
| // In development, allow localhost origins | ||
| if os.Getenv("ENV") != "production" { |
There was a problem hiding this comment.
I think by default we should get the ENV var at the top, as a string, which should be set to "development" by default. This check might break and is harder to read in general
Addresses review feedback: 1. Security fix: Replace insecure substring check with proper hostname comparison. Previously `strings.Contains(origin, host)` could be bypassed by an attacker using "malicious-example.com" to match "example.com". Now parses the origin URL and compares hostnames exactly. 2. Add getEnv() helper that returns "development" by default, making the environment check clearer and more maintainable. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Summary
Security Issue Addressed
WebSocket Without Authentication (Low) - Previously, the /ws WebSocket endpoint:
CheckOrigin: func(r *http.Request) bool { return true })Changes
backend/controllers/websocket.go:checkWebSocketOrigin()for Origin header validationAuthenticatedWebSocketHandler()that requires valid sessionWebSocketHandlerfor backward compatibility (deprecated)ALLOWED_ORIGINandFRONTEND_ORIGIN_DEVenv varsbackend/main.go:AuthenticatedWebSocketHandler(store)instead ofWebSocketHandlerWebSocket Security Layers
With this PR:
Test plan
🤖 Generated with Claude Code