security: restrict CORS to localhost origin only - #65
Conversation
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
roborev: Combined Review (
|
The CORS middleware previously set Access-Control-Allow-Origin: *, allowing any website to make cross-origin requests to the API. This means a malicious website could silently read all session data, trigger insight generation, configure GitHub tokens, and publish sessions as public gists — all without user awareness. Fix: Replace wildcard with origin validation against the configured host:port. Both 127.0.0.1 and localhost variants are allowed when binding to either loopback address, since browsers treat them as distinct origins. Also adds Vary: Origin header for correct HTTP caching behavior when the response depends on the request origin. Tests updated and expanded: - TestCORSHeaders: verifies matching origin is reflected - TestCORSRejectsUnknownOrigin: verifies foreign origins get no header - TestCORSAllowsLocalhost: verifies localhost alias works - TestCORSPreflight: updated with Origin header - TestCORSAllowMethods: updated with Origin header
Address two issues identified during security review: 1. When binding to 0.0.0.0 (all interfaces), browsers access the server via 127.0.0.1 or localhost — not 0.0.0.0. The CORS allowlist now treats 0.0.0.0 and :: as "allow loopback origins" so cross-origin requests from the SPA still work. 2. Vary: Origin is now set unconditionally on /api/ responses to prevent caching issues where a proxy caches a response without CORS headers and serves it to a legitimate origin. New tests: TestCORSBindAllInterfaces, TestCORSVaryAlwaysSet
Address issues from multi-agent security review: 1. CSRF protection: Mutating requests (POST/PUT/PATCH/DELETE) and OPTIONS preflights from unrecognized origins now return 403 Forbidden instead of executing. This prevents <form>-based CSRF attacks that bypass CORS preflight. 2. IPv6 origin formatting: Use net.JoinHostPort to correctly produce [::1]:port bracket notation. Add ::1 to the loopback allowlist for 0.0.0.0 and :: bind-all cases. 3. Port 80 normalization: Browsers omit :80 from the Origin header for default HTTP port. When port is 80, both "http://host:80" and "http://host" are now in the allowlist. Note: DNS rebinding via Host header spoofing remains a separate concern that requires Host header validation middleware — tracked as a follow-up, not addressed in this CORS-focused change. New tests: TestCORSBlocksMutatingFromUnknownOrigin, TestCORSAllowsMutatingFromKnownOrigin, TestCORSPreflightRejectsBadOrigin. Updated: TestCORSBindAllInterfaces adds [::1] origin check.
Address high/medium findings from multi-agent security review: HIGH — DNS rebinding defense: Add hostCheckMiddleware that validates r.Host against expected loopback values before processing /api/ requests. An attacker's domain resolving to 127.0.0.1 will carry the attacker's domain as the Host header, which is now rejected with 403. MEDIUM — Empty Origin CSRF bypass: Mutating requests (POST/PUT/PATCH/DELETE) now require a non-empty Origin that matches the allowlist. Previously, originAllowed treated empty Origin as trusted, allowing CSRF via contexts that omit the header. MEDIUM — IPv6 port 80 formatting: httpOrigin() now brackets IPv6 literals in the portless form (http://[::1] not http://::1) to match browser behavior. Tests: 13 CORS/Host tests pass. Full server suite passes. Updated middleware_test.go to use real listener port for Host allowlist compatibility.
Browsers send Host: [::1] (with brackets) for IPv6 on port 80. buildAllowedHosts was storing bare ::1, causing 403 rejections.
When the server binds to all interfaces (0.0.0.0/::), the user explicitly chose network exposure. Skip Host header validation and accept any non-empty Origin in this mode so LAN clients connecting via the machine's real IP are not rejected. Loopback-only binding (127.0.0.1/localhost/::1) retains strict Host and Origin validation as before. Also add PUT and PATCH to Access-Control-Allow-Methods to match the methods that isMutating() treats as state-changing, preventing preflight failures if those methods are used by future endpoints.
roborev: Combined Review (
|
|
I'm taking over this PR, will rebase and push changes here, thank you for starting this! |
8934452 to
445bd2e
Compare
roborev: Combined Review (
|
## Summary - Replace `Access-Control-Allow-Origin: *` with origin validation against the server's configured host:port - Both `127.0.0.1` and `localhost` variants are accepted when binding to either loopback address - `0.0.0.0` and `::` (bind-all) are treated as "allow loopback origins" since browsers never send `Origin: http://0.0.0.0:*` - `Vary: Origin` set unconditionally on all `/api/` responses to prevent proxy caching issues - 5 new tests, 2 updated tests — all 7 CORS tests pass, full server suite passes ## Security Impact Previously, any website could silently make cross-origin requests to the agentsview API while it was running. This allowed: - Reading all AI coding session content (code, conversations, file paths) - Triggering insight generation (spawning CLI subprocesses) - Configuring GitHub tokens and publishing sessions as public gists The fix restricts CORS to only the origin matching the server's own address. ## Changes - `internal/server/server.go`: `corsMiddleware` now takes an `allowedOrigins` map and validates the `Origin` header. New `buildAllowedOrigins` helper derives the set from config, with special handling for `0.0.0.0`/`::` bind-all. - `internal/server/server_test.go`: Updated existing CORS tests to send Origin headers. Added `TestCORSRejectsUnknownOrigin`, `TestCORSAllowsLocalhost`, `TestCORSBindAllInterfaces`, `TestCORSVaryAlwaysSet`. ## Test plan - [x] `TestCORSHeaders` — matching origin is reflected - [x] `TestCORSRejectsUnknownOrigin` — foreign origins get no CORS header - [x] `TestCORSAllowsLocalhost` — localhost alias works when bound to 127.0.0.1 - [x] `TestCORSBindAllInterfaces` — loopback origins work when bound to 0.0.0.0 - [x] `TestCORSVaryAlwaysSet` — Vary: Origin present even for disallowed origins - [x] `TestCORSPreflight` — OPTIONS returns 204 - [x] `TestCORSAllowMethods` — Allow-Methods header present - [x] Full `go test ./internal/server/` passes --------- Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
Summary
Access-Control-Allow-Origin: *with origin validation against the server's configured host:port127.0.0.1andlocalhostvariants are accepted when binding to either loopback address0.0.0.0and::(bind-all) are treated as "allow loopback origins" since browsers never sendOrigin: http://0.0.0.0:*Vary: Originset unconditionally on all/api/responses to prevent proxy caching issuesSecurity Impact
Previously, any website could silently make cross-origin requests to the agentsview API while it was running. This allowed:
The fix restricts CORS to only the origin matching the server's own address.
Changes
internal/server/server.go:corsMiddlewarenow takes anallowedOriginsmap and validates theOriginheader. NewbuildAllowedOriginshelper derives the set from config, with special handling for0.0.0.0/::bind-all.internal/server/server_test.go: Updated existing CORS tests to send Origin headers. AddedTestCORSRejectsUnknownOrigin,TestCORSAllowsLocalhost,TestCORSBindAllInterfaces,TestCORSVaryAlwaysSet.Test plan
TestCORSHeaders— matching origin is reflectedTestCORSRejectsUnknownOrigin— foreign origins get no CORS headerTestCORSAllowsLocalhost— localhost alias works when bound to 127.0.0.1TestCORSBindAllInterfaces— loopback origins work when bound to 0.0.0.0TestCORSVaryAlwaysSet— Vary: Origin present even for disallowed originsTestCORSPreflight— OPTIONS returns 204TestCORSAllowMethods— Allow-Methods header presentgo test ./internal/server/passes