Repository navigation
fix(web): replace wildcard CORS with origin/Host validation; wire board SSE events - #51
Merged
Merged
Conversation
Once Run returned (source closed or ctx done) the hub had no closed state, so a late Subscribe registered a channel nothing would ever write to or close. Reconnecting SSE clients subscribed to the dead hub and hung on keepalives forever, silently frozen. Subscribe now hands out an already-closed channel once Run has exited, and Closed() lets handlers refuse new streams outright. Constraint: Subscribe signature must stay (int, <-chan T) for existing callers Rejected: error return from Subscribe | breaks both stream handlers and cmd callers for no added signal Confidence: high Scope-risk: narrow
…CORS withCORS set Access-Control-Allow-Origin: * on every plain REST/SSE route with no Host or Origin validation, so any web page could read the full orchestrator state, stream /api/v1/events, and drive POST/PATCH mutations (stop agents, create/modify board issues) against the localhost dashboard. The dashboard is served by this same server, so cross-origin access is unnecessary by default: requests now pass only with no Origin header (curl/CLI), a same-origin Origin, or a loopback Origin, and the Host header must name a loopback address or the configured listen host to block DNS rebinding. Validated origins are echoed exactly (with Vary: Origin) instead of *. Operators serving the dashboard from another origin can allowlist it (or "*") via CONTRABASS_ALLOWED_ORIGINS. Also hardens the same surface: http.Server gains ReadHeaderTimeout to bound slowloris clients, board create/update bodies are capped at 1 MiB via http.MaxBytesReader (parity with the streamable HTTP cap), and /api/v1/events returns 503 once the hub has shut down so EventSource clients surface the outage instead of reconnecting into a silent stream. Updated tests that pinned the removed behavior: the "*" grant assertions and requests carrying httptest's default example.com Host, which the rebinding guard now rejects by design. Constraint: same-origin dashboard and no-Origin CLI clients must keep working unauthenticated Constraint: WriteTimeout must stay unset so SSE streams are not severed Rejected: reuse isAllowedLoopbackExactOrigin | its RemoteAddr loopback check breaks operators who deliberately bind non-localhost Rejected: reject only state-changing routes | wildcard grant also let foreign origins read state snapshots and event streams Confidence: high Scope-risk: moderate Directive: keep CONTRABASS_ALLOWED_ORIGINS parsing exact-origin only; pattern matching would reopen the bypass this fixes Not-tested: IPv6 bracketed Host/Origin variants beyond net.ParseIP loopback handling
SetEventSink had no production caller, so publishEvent silently no-oped and the board_issue_created/updated/moved events emitted by the HTTP board handlers never reached the hub. A second dashboard tab (or any stream subscriber) showed a stale Kanban board until a full reload. Both server construction sites now pass the hub's source channel as the sink. Confidence: high Scope-risk: narrow Not-tested: no cmd-level regression test; the publish path itself is covered by internal/web board handler tests
The event forwarder in run() closed webEvents once orch.Events() ended. With the server now publishing board mutations into the same channel via SetEventSink, shutdown raced: ctx cancel ends orch.Run, the forwarder closes webEvents, while graceful Shutdown gives in-flight handlers up to 5s — a POST/PATCH reaching publishEvent after the close panics with "send on closed channel" (select/default does not guard a closed channel). Extract the forwarder into forwardOrchestratorEvents, which never closes the sink; the hub already tears down subscribers via ctx. Constraint: publishEvent and the forwarder are concurrent writers to one channel, so neither writer may close it Rejected: recover() inside publishEvent | hides the race instead of removing it Rejected: routing publishEvent through a new hub broadcast API | larger surface change; team_root relies on the raw sink contract Confidence: high Scope-risk: narrow Directive: webEvents must stay open for the process lifetime; hub shutdown is driven by ctx, never by closing the source Not-tested: natural orchestrator completion without ctx cancel now leaves SSE streams open until process exit (benign; process is exiting)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 2/3 — security hardening.
HIGH — wildcard CORS with no Host/Origin validation on state-changing REST and SSE. The dashboard is same-origin, so the wildcard was pure attack surface: any web page could drive the local API. Now: Host header validated (DNS-rebinding guard), no-Origin (curl/CLI) allowed, same-origin + loopback allowed, everything else rejected 403. Escape hatch:
CONTRABASS_ALLOWED_ORIGINS(exact origins, or*to opt out). Exact CORS headers replace the wildcard.Also:
SetEventSinkfinally has a caller — board mutations now broadcast to SSE clients (the review caught a send-on-closed-channel shutdown race in the first wiring; fixed in the follow-up commit). ReadHeaderTimeout on the HTTP server, MaxBytesReader on board handlers, hub.Subscribe after Run exit returns a closed channel instead of a dead one.Adversarial review: 1 blocking (the shutdown race) → fixed and re-verified.
go test ./internal/web/... ./internal/hub/... ./cmd/...green after rebase.