🔒 Restrict WebSocket Origins to Mitigate CSWSH#15
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
⚡ Benchmark ResultsTo add benchmarks, create functions starting with |
There was a problem hiding this comment.
Pull request overview
This PR hardens the daemon’s WebSocket endpoint against Cross-Site WebSocket Hijacking (CSWSH) by replacing permissive origin acceptance with configurable origin restrictions that flow from environment config into the server.
Changes:
- Add
AllowedOriginsto configuration (config.Environment) with environment defaults and an ldflags override. - Wire
AllowedOriginsthrough daemon startup intoserver.Config, and apply it viawebsocket.AcceptOptions.OriginPatterns(removingInsecureSkipVerify: true). - Add WebSocket origin enforcement tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/server/server.go | Uses configured origin patterns during WebSocket Accept and removes insecure verification bypass. |
| internal/config/config.go | Introduces AllowedOrigins (env + ldflags override) and sets environment defaults. |
| internal/daemon/daemon.go | Passes environment AllowedOrigins into the WebSocket server configuration. |
| internal/server/server_test.go | Adds tests to validate allowed vs. disallowed origins and a same-origin scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Override allowed origins from ldflags if provided | ||
| if AllowedOrigins != "" { | ||
| cfg.AllowedOrigins = strings.Split(AllowedOrigins, ",") |
There was a problem hiding this comment.
Parsing AllowedOrigins from the ldflag with strings.Split leaves leading/trailing whitespace intact and can introduce empty entries (e.g., trailing comma). This can make valid origins fail to match unexpectedly and makes configuration fragile. Consider splitting, trimming spaces on each entry, and filtering out empty strings before assigning cfg.AllowedOrigins (optionally validate patterns and log/ignore invalid ones).
| cfg.AllowedOrigins = strings.Split(AllowedOrigins, ",") | |
| rawOrigins := strings.Split(AllowedOrigins, ",") | |
| cleanOrigins := make([]string, 0, len(rawOrigins)) | |
| for _, origin := range rawOrigins { | |
| trimmed := strings.TrimSpace(origin) | |
| if trimmed == "" { | |
| continue | |
| } | |
| cleanOrigins = append(cleanOrigins, trimmed) | |
| } | |
| if len(cleanOrigins) > 0 { | |
| cfg.AllowedOrigins = cleanOrigins | |
| } |
| // Case A: Connection from Allowed Origin | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
|
|
||
| opts := &websocket.DialOptions{ | ||
| HTTPHeader: http.Header{ | ||
| "Origin": []string{"http://good.com"}, | ||
| }, | ||
| } | ||
|
|
||
| conn, _, err := websocket.Dial(ctx, u, opts) | ||
| if err != nil { | ||
| t.Fatalf("Connection from good.com failed: %v", err) | ||
| } | ||
| conn.Close(websocket.StatusNormalClosure, "") | ||
|
|
||
| // Case B: Connection from Disallowed Origin | ||
| optsBad := &websocket.DialOptions{ | ||
| HTTPHeader: http.Header{ | ||
| "Origin": []string{"http://evil.com"}, | ||
| }, | ||
| } | ||
|
|
||
| _, _, err = websocket.Dial(ctx, u, optsBad) | ||
| if err == nil { | ||
| t.Fatalf("Connection from evil.com succeeded (should fail)") | ||
| } |
There was a problem hiding this comment.
The negative-origin assertions only check that websocket.Dial returns an error; a timeout/cancellation or other handshake failure would also satisfy this, which can mask regressions. To make the security behavior explicit and reduce flakiness, use a fresh context per dial and assert the failure is specifically an origin rejection (e.g., inspect the HTTP response status / close status / error type returned by the dial).
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
🧹 🔒 Restrict WebSocket Origins to Mitigate CSWSH
🎯 What
OriginPatterns: ["*"]).AllowedOriginssetting inconfig.Environmentandserver.Config.internal/server/server.goto use the configured allowed origins inOriginPatternsand removedInsecureSkipVerify: true.internal/daemon/daemon.goto pass the configuration to the server.remoteenvironment (localhost and file:// only) while keepinglocalpermissive for development (but overridable).AllowedOriginsvariable injectable via ldflags for deployment flexibility.💡 Why
✅ Verification
internal/server/server_test.gothat:evil.com) are rejected.go testlocally with a mockposterdependency and confirmed the test passes with the fix and fails without it.✨ Result
remote) default to a safer configuration.AllowedOriginsldflag (comma-separated list).PR created automatically by Jules for task 7606051191821757871 started by @adcondev