[#988] Reverse-proxy dashboard token allowlist (v2.5.1 hotfix)#989
Conversation
The #968 loopback-only gate on /api/session-token 403s a dashboard served through an on-box authenticated reverse proxy (nginx forwards a public Host/Origin), so the terminal WS never gets a token and closes 1006. Add opt-in config.trusted_dashboard_hosts: when the socket is loopback AND the forwarded Host/Origin are both in the allowlist, /api/session-token and the WS upgrade accept the request. Empty/unset (default) → exactly #968's loopback-only behavior, no change. A foreign/un-allowlisted Host or Origin still gets 403 — DNS-rebinding protection intact. Tests (server/proxyDashboardAllowlist.test.js) cover: trusted host → token 200 + WS opens; foreign host → token 403 + WS 403; loopback unchanged; partial spoof (only Host or only Origin trusted) → 403. wsPtyAuth.test.js (no-allowlist case) still passes. docs/troubleshooting.md documents the authenticated-nginx + allowlist setup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: FAIL
#988 requires an untrusted/foreign Host or Origin to still get 403 on both /api/session-token and WS; the token endpoint does, but the WS origin check still accepts matched non-allowlisted Host+Origin.
Checked (evidence)
- Structural gate: PR body contains filled
## EPIC Alignmentand## Self-Verificationsections; no UI fidelity table required. - Ticket/epic context:
gh api repos/realproject7/quadwork/issues/988+issues/967→ contract is no DNS-rebinding weakening and no regression of working deployments. - Live diff reviewed:
gh pr diff 989at headccffe682e3c4a2b7cd0b8adec4a1c83ea84c397c. - Riskiest part of this diff:
server/index.jsWS/PTY auth path; not acceptable because a non-allowlisted same-host WS still passes origin validation. - Kill-list: scanned all items — hit listed in Findings.
- CI:
gh pr checks 989→test pass 54s.
Findings
- [high] WS still accepts a foreign same-host origin outside
trusted_dashboard_hosts- File:
server/index.js:209 - Why it fails: after
isTrustedProxyRequest(req)returns false, the legacyu.host === req.headers.hostfallback still allowsHost: evil.example+Origin: https://evil.examplethroughisAllowedWsOrigin; with a valid session token the upgrade proceeds past the 403 gate atserver/index.js:1700. That contradicts #988's required invariant and the PR's security claim that unallowlisted Host/Origin still get WS 403. The new test covers foreign Origin with loopback Host atserver/proxyDashboardAllowlist.test.js:158, but not the matched foreign Host+Origin case. - Do instead: make
/ws/terminalaccept only loopback origins orisTrustedProxyRequest(req)for this path, or otherwise gate the same-host fallback behind an explicit trusted config; add a regression test forHost: evil.example+Origin: https://evil.example+ valid token expecting WS 403.
- File:
Decision
Requesting changes because the central security invariant for this hotfix is not actually enforced on the WS upgrade path. If retaining the same-host token path is intentional, escalate #988 to @Head because the ticket acceptance criteria and PR security section need to be changed before approval.
|
APPROVE at Core-risky #968 WS/PTY auth path reviewed with the security invariants front of mind. The change is opt-in and strictly additive: a foreign Host/Origin still 403s on both Checked (evidence)
No blocking issues. Nice work isolating the exception behind a loopback-socket + dual-allowlist gate. |
REQUEST CHANGES: isAllowedWsOrigin's #968 same-host fallback (u.host === req.headers.host) fired even for a LOOPBACK socket carrying a non-loopback, non-allowlisted Host+Origin — so a forged/foreign matched pair (untrusted on-box proxy, DNS-rebinding page) with a valid token slipped through, violating #988's "foreign Host/Origin → WS 403" invariant. Gate the fallback on a genuine DIRECT (non-loopback) connection: a loopback socket with a non-loopback Host means the request arrived via an on-box reverse proxy, which must be explicitly allowlisted (isTrustedProxyRequest, checked first). Preserves #968 local + direct tailnet/LAN paths and strengthens DNS-rebinding protection at the WS layer. Adds the requested regression test: matched foreign Host+Origin + valid token → WS 403 (not opened). Full suite still 61 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
APPROVE at @re1's REQUEST CHANGES was correct and I'd missed it: the #968 same-host fallback ( Checked (evidence)
Prior-SHA note: my earlier APPROVE at |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Epic Alignment: PASS
Delta ccffe682e3c4a2b7cd0b8adec4a1c83ea84c397c..777d9857a1a0a4c8be6ddc0d3c8167f6a2234d2d now enforces #988's Host/Origin 403 invariant while preserving the #967 WS auth contract.
Checked (evidence)
- Re-review delta:
gh api repos/realproject7/quadwork/compare/ccffe682e3c4a2b7cd0b8adec4a1c83ea84c397c...777d985→ changed onlyserver/index.jsandserver/proxyDashboardAllowlist.test.js. - Prior finding resolved: loopback reverse-proxy/same-host fallback is now gated by
!isLocalhost(ip)atserver/index.js:218, after allowlisted proxy handling. - Regression coverage: matched foreign
Host: evil.example+Origin: http://evil.example+ valid token expects WS 403 atserver/proxyDashboardAllowlist.test.js:170. - Riskiest part of this diff: preserving direct tailnet/LAN same-host WS access while rejecting loopback proxy spoofing; acceptable because the fallback is now non-loopback only.
- Kill-list: scanned new ranges — clean.
- CI:
gh pr checks 989→test pass 48s.
Findings
None.
Decision
The requested security fix is in place and covered by the exact regression test. Approving PR #989 at 777d9857a1a0a4c8be6ddc0d3c8167f6a2234d2d.
Closes #988
Summary
P0 hotfix for v2.5.1 — a #968 regression locked the operator's nginx-proxied VPS dashboard out of every terminal.
The VPS dashboard is served by nginx (
p7.quadwork.xyz→127.0.0.1:8400,proxy_set_header Host $host) behind HTTP Basic Auth. #968's/api/session-tokenguard (isLocalTokenRequest) requires socket-IP andHostandOriginto all be loopback. Through the proxy the forwardedHost/Originis the public domain, so the endpoint 403s → the dashboard can't fetch the session token → every terminal WS closes1006.Fix
Add an opt-in operator allowlist
config.trusted_dashboard_hosts(e.g.["p7.quadwork.xyz"]). NewisTrustedProxyRequest(req)returns true only when:Hostand (when the browser sends one)Originhostnames are both in the allowlist./api/session-tokenand the WS upgrade (isAllowedWsOrigin) now accept such requests. The allowlist config is read once at boot (likeport/session_token).isTrustedProxyRequestshort-circuits tofalsewhen the set is empty.src/lib/sessionToken.ts) already fetches/api/session-tokenrelative to the page origin and attaches the token to the WS, so once the endpoint returns 200 the proxied path works with no frontend change.Security invariants (not weakened)
HostorOrigin(DNS-rebind page, untrusted proxy, tailnet name) is never in the allowlist → still 403 on/api/session-tokenand on the WS upgrade. DNS-rebinding protection intact.HostandOriginmust be allowlisted — a partial spoof (only one trusted) is rejected.EPIC Alignment
Ref EPIC #967 (Phase 3 hardening). This restores the #968 (EPIC #967 Phase 1) WS/PTF auth surface for the operator's authenticated reverse-proxy deployment without relaxing its DNS-rebinding guarantees — the allowlist is a scoped, opt-in exception layered on top of the existing loopback gate, not a replacement for it.
Self-Verification
Automated —
server/proxyDashboardAllowlist.test.js(new, 13 assertions, auto-discovered byrun-tests.js) boots the REAL server as a subprocess on a throwaway port with a temp HOME whoseconfig.jsonsetstrusted_dashboard_hosts: ["p7.quadwork.xyz"], then drives the actualserver.on("upgrade")handler +/api/session-tokenover the wire:server/wsPtyAuth.test.js(boots with an empty config; 19 passed, still green).Full suite green:
npm test→ 61 passed, 0 failed, 2 skipped (the 2 skips are pre-existing Jest-style, out of #836 scope).Reverse-proxy path — live-verified by booting the real server on a throwaway port (
8477, temp HOME, allowlist configured) and curling the exact headers nginx forwards:docs/troubleshooting.mdupdated with the nginx Basic-Auth +trusted_dashboard_hostssetup (incl. the requiredHost/Origin/Upgradeproxy headers) and a prominent note that the proxy must be authenticated.cc @re1 @re2 — please review on current @Head.