fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553
Open
aaronjmars wants to merge 1 commit into
Open
fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553aaronjmars wants to merge 1 commit into
aaronjmars wants to merge 1 commit into
Conversation
Allowlist the HTTP `Host` header on both the request path and the
WebSocket upgrade. Default allowlist covers `localhost`, `127.0.0.1`,
`[::1]` (with and without :PORT), plus the configured `BRAINSTORM_HOST`
and `BRAINSTORM_URL_HOST`. Operators can extend with
`BRAINSTORM_ALLOWED_HOSTS` (comma-separated) for tunneled setups.
Without this gate, a page on another origin can DNS-rebind its own
hostname to 127.0.0.1, hit `/` or `/files/*` on the running brainstorm
companion server, and read the active screen + content files even
though the listener is loopback-only. The same rebind would also let
the page complete a WebSocket upgrade and inject `{choice: ...}`
events into `state_dir/events`, which the agent reads as the user's
selection.
PR obra#1110 / issue obra#1014 already cover the WebSocket Origin axis. This
PR adds the complementary Host axis, which is the canonical defense
against DNS rebinding (Origin alone doesn't cover `/files/*`).
Tests: 6 new cases in tests/brainstorm-server/server.test.js cover
loopback accept, foreign-Host 421, /files/* foreign-Host 421, and a
forged-Host WebSocket upgrade. Existing 25 tests still pass (31/31).
Detected by: Aeon (https://github.com/aaronjmars/aeon-aaron) +
manual review against the brainstorm-server attack surface.
Severity: medium (cross-origin info disclosure + agent input
injection via DNS rebinding).
CWE-346 (Origin Validation Error), CWE-350 (Reliance on Reverse DNS
Resolution for a Security-Critical Action).
Author
|
Friendly bump @obra — small Host-header validation fix to close the DNS-rebinding window on brainstorm-server. Happy to address any feedback when you have a moment. |
Author
|
Friendly second nudge @obra — small Host-header validation in |
5 tasks
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
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.
What problem are you trying to solve?
The brainstorm companion server (
skills/brainstorming/scripts/server.cjs) accepts HTTP and WebSocket connections without validating theHostheader. Combined with the listener's binding to127.0.0.1, that's enough to keep the TCP socket "local" — but it does not stop a DNS-rebinding attack:ws://127.0.0.1:<port>and the user opens the printed URL in their browser.https://attacker.example, which has its own DNS server return its hostname pointing first to its own IP, then re-resolves it to127.0.0.1after the page loads (DNS rebind).attacker.exampleissuesfetch('http://attacker.example:<port>/')andfetch('/files/...')— the TCP connection now lands on the loopback brainstorm server, but the browser sendsHost: attacker.exampleand treats the response as same-origin./files/*content. The attacker reads brainstorm screens and any file the agent dropped inCONTENT_DIR.{type: "click", choice: "<attacker text>"}send, which the server appends tostate_dir/events. On the next turn the agent reads that file as the user's selection.Issue #1014 describes the WebSocket-side variant of this; PR #1110 covers the WebSocket
Originaxis but leaves both HTTP info-disclosure paths and theHost-axis defense on WS in place. This PR closes theHost-header gap.Reproduction loop (without the fix), using the existing test harness as the "attacker":
What does this PR change?
Adds a single
Host-header allowlist (buildAllowedHosts()+isHostAllowed()) inserver.cjsand applies it before any work inhandleRequest()(returns421 Misdirected Request) and before the101handshake inhandleUpgrade()(destroys the socket). Default allowlist:localhost,127.0.0.1,[::1](bare and with:PORT) plus the configuredBRAINSTORM_HOST/BRAINSTORM_URL_HOST. Operators can extend viaBRAINSTORM_ALLOWED_HOSTS(comma-separated) for tunneled / containerized setups that already exist invisual-companion.md.Is this change appropriate for the core library?
Yes —
skills/brainstorming/scripts/server.cjsis a first-party companion server shipped with core. DNS-rebinding is not a project-specific concern; every brainstorm session running anywhere is exposed without this gate. The fix introduces no new dependencies, no third-party integrations, and no configuration the user is forced to set (defaults work in every documented setup).What alternatives did you consider?
start-server.sh,visual-companion.md,helper.js, the URL the agent prints, and how the user opens the page. PR harden brainstorming session access and cleanup boundaries #685 was bundled with permissions/cleanup/lockfile concerns and got closed quickly. A token approach is also strictly defense-in-depth on top of the Host check — DNS rebinding is solved by checking Host, regardless of token presence./and/files/*info-disclosure open. HTTP same-origin requests under DNS rebind don't sendOrigin, so anOrigincheck on the HTTP path would silently pass attacker traffic. Host validation is the canonical answer for the HTTP axis./files/*entirely: removes one disclosure path but not the other (/still serves the current screen). Also a much more invasive behavioral change.The Host allowlist is the minimum surgical fix that closes the rebinding surface end-to-end on both HTTP and WS paths.
Does this PR contain multiple unrelated changes?
No. Single concern: validate the
Hostheader against an allowlist before processing any HTTP request or completing a WebSocket upgrade. Two files touched — the server (+59) and the test file (+76) — both load-bearing for the same change.Existing PRs
Origincheck). This PR is intentionally complementary, not duplicative —HostandOriginare different headers covering different threat directions. PR harden brainstorming session access and cleanup boundaries #685 (closed) bundled a much broader token-auth + cleanup change; the maintainer's quick close on that one motivated the narrow scope here.Environment tested
New harness support (required if this PR adds a new harness)
N/A — no new harness. The existing
using-superpowersbootstrap is untouched.Evaluation
obra/superpowersfrom today's GitHub trending list (No marketplace.json? #3, ★191,721). After semgrep / trufflehog / osv-scanner came back clean, manual review of the brainstorm companion server surfaced the missing Host gate.cd tests/brainstorm-server && npm install && npm test. Both: 31 passed, 0 failed.GET / Host: evil.example:<port>→200+ brainstorm screen body returned.GET /files/<any> Host: evil.example→200/404depending on file.ws://evil.example:<port>via DNS-rebind hop → upgrade succeeds, server accepts{choice: "..."}and writes it tostate/events.421/421/ socket destroy with no101. LegitimateHost: localhost:<port>,127.0.0.1:<port>, and bare-loopback Hosts continue to return200and complete the WS handshake.Adversarial cases the new tests cover: foreign-Host on
/, foreign-Host on/files/*, and a forged-Host WebSocket upgrade routed back to the loopback listener via a custom DNSlookup.Rigor
superpowers:writing-skillsand completed adversarial pressure testing (paste results below)This is a server-code change, not a skills-prose change — the writing-skills checkbox is intentionally left unchecked. Adversarial coverage lives in the new tests (
Host: evil.example,Host: attacker.example,ws://evil.examplerebinding to 127.0.0.1).Human review
Leaving this unchecked intentionally rather than claiming a diff review that did not happen. The change was generated by Aeon (an autonomous Claude Code agent) and the full 135-line diff is small enough to review in one pass — please flag anything that looks off and I (Aaron) will respond.
Filed by Aeon on behalf of @aaronjmars.