fix(server): proxy routes forward WebSocket upgrades (rewrite Origin) (#257)

[adnaan]

Summary

Fixes #257. routes: type: proxy loaded HTML fine but every WebSocket upgrade returned 403, so the embedded LiveTemplate client silently fell back to HTTP polling — invisible because isReady() stays true under fallback.

Root cause (confirmed from primary source, not inferred): the issue title ("doesn't forward WebSocket upgrades") misdiagnoses it. Go's stdlib httputil.ReverseProxy forwards WS upgrades natively (Go 1.12+; repo is on 1.26). The real bug: the Director rewrites req.Host to the upstream but left the Origin header pointing at the proxy's domain. The upstream (a LiveTemplate app) runs createSecureOriginChecker, whose production default requires Origin == scheme://Host — so the handshake mismatched and gorilla/websocket returned 403.

Fix

Rewrite the Origin header to the upstream origin inside the proxy Director, gated on the WebSocket upgrade headers (new isWebSocketUpgrade helper) so non-WS CORS behavior is untouched. This is the standard reverse-proxy pattern (nginx proxy_set_header Origin). One change point in newProxyRoute covers both config-declared routes: type: proxy and embed-lvt auto-routes.

Tests (each proven to fail without the fix, pass with it)

• TestProxyRoute_WebSocketCrossOrigin — Go integration reproducing the cross-origin 403 against a strict same-origin upstream.
• TestProxyRoute_RewritesOriginOnlyForWS — asserts Origin is rewritten for WS, left unchanged for plain HTTP.
• TestProxyRoute_WebSocketUpgradeE2E — chromedp E2E asserting a 101 handshake + WS frame via CDP (deliberately not isReady(), which masks the bug), capturing all four diagnostic channels: console logs, server logs, WS frames, rendered HTML.

Verification

• CI gate locally: go build ./... clean; go test -race -tags=ci -skip='E2E|e2e' ./... → all packages pass.
• New E2E + related embed-lvt E2E pass under Docker Chrome.
• gofmt/vet clean; introduces no new lint findings.

Scope

Tight per triage. The new WS-origin E2E partially addresses #143. #144 (configurable CSP) is the inverse scenario (tinkerdown's own /ws behind a proxy) and stays separate.

🤖 Generated with Claude Code

[claude]

Code Review

PR: fix(server): proxy routes forward WebSocket upgrades (rewrite Origin) (#257)

---

Overview

The fix is correct and tightly scoped. The root cause diagnosis is accurate: httputil.ReverseProxy does handle WS upgrades natively in Go 1.12+, but the Director's req.Host rewrite left the Origin header pointing at the proxy — causing a strict same-origin upstream to 403 the handshake. The one-line fix (rewrite Origin only for WS upgrades) is the right approach and follows the same pattern as nginx's proxy_set_header Origin.

---

What's Good

• Surgical scope. The fix is confined to a single code path (newProxyRoute's Director) with no collateral changes.
• isWebSocketUpgrade is correct. Scanning comma-separated Connection tokens with strings.SplitSeq (Go 1.23+) and case-insensitive comparison handles real browser headers properly.
• Origin guard != "" is the right touch — skips the rewrite if no Origin was sent, e.g. from non-browser WS clients.
• Test pyramid is solid. Three tests at three distinct layers: unit (RewritesOriginOnlyForWS), integration (CrossOrigin against a live httptest upstream), and E2E (chromedp + CDP). Each is proven to fail without the fix.
• E2E assertion target is intentionally robust. The test checks the 101 handshake + received frame via CDP — not isReady(), which would mask the bug via the HTTP fallback. The diagnostic dump (all four channels) is a good pattern for CI triage.

---

Issues & Suggestions

Minor — waitForStatus silently swallows the timeout (proxy_ws_e2e_test.go:196)

return nil // let the assertions report the actual state

On timeout the function returns nil, so chromedp.Run succeeds. The test then fails at the strings.Contains(status, wsMarker) assertion — but the failure message will be #status = "connecting", with no indication that the real problem was the 5 s wait expiring. In practice this will confuse CI debugging.

Suggestion: return a sentinel error when the deadline passes; the assertions still run (because you can return the error without using t.Fatal), but the chromedp log will also show the timeout:

return fmt.Errorf("timed out after %v waiting for %q prefix on %s", timeout, wantPrefix, sel)

(The runErr != nil block already t.Fatalfs with the full chromedp error, so this surfaces cleanly.)

Minor — log.Prefix() not saved/restored in E2E setup

prevOut := log.Writer()
prevFlags := log.Flags()
log.SetOutput(&tdLog)
t.Cleanup(func() { log.SetOutput(prevOut); log.SetFlags(prevFlags) })

log.Prefix() is not captured or restored. If another test in the package sets a non-empty prefix before this test runs, the prefix leaks. Harmless in practice given the serial E2E execution noted in the comment, but worth completing for correctness:

prevPrefix := log.Prefix()
t.Cleanup(func() { log.SetOutput(prevOut); log.SetFlags(prevFlags); log.SetPrefix(prevPrefix) })

Nit — comment slightly overstates the edge case

// (An upstream that allow-lists the proxy's own origin rather than
// its host would still reject; that config is uncommon.)

This is true but the inverse is also worth acknowledging: an upstream that uses the Origin header for authorization decisions (not just same-origin checking) will now see the upstream's own origin on every proxied WS request, silently bypassing those checks. That's expected transparent-proxy behaviour, but makes the security contract explicit in the comment, e.g.:

This hides the real browser origin from the upstream; upstreams that use Origin for authorization (not just same-origin checking) should not be fronted by this proxy without additional authentication.

---

Security

The rewrite is correct and standard for reverse proxies. The isWebSocketUpgrade gate means non-WS CORS is untouched, which prevents inadvertently laundering cross-origin XHR/fetch requests. No new attack surface introduced.

---

Summary

The fix is correct, the tests are well-designed, and the PR description is exemplary. The two minor points above (timeout surfacing in waitForStatus, restoring log.Prefix) are low-risk but worth a quick pass before merge.

Verdict: Approve with minor suggestions.

[claude]

Code Review

Overview

This PR fixes a real and well-diagnosed bug: the proxy Director rewrote req.Host to the upstream but left Origin pointing at the proxy domain, causing the upstream strict same-origin WebSocket check to 403. The fix is minimal, targeted, and follows the standard reverse-proxy pattern. Test coverage is excellent — unit, integration, and E2E all proven to fail before the fix.

---

Code Quality — proxy_routes.go

• upstreamOrigin computed outside the closure — avoids re-allocating on every request.
• Double-gating the rewrite (isWebSocketUpgrade and Origin != "") is the right shape; plain HTTP CORS is untouched.
• strings.SplitSeq — uses Go 1.24+ iterator-based split. Fine for Go 1.26, but worth verifying go.mod minimum version if ever tightened.
• req.Header.Get("Upgrade") returns only the first value. Valid WS upgrades always send exactly one Upgrade: websocket, so this is not a practical bug.
• Security comment in the Director is correct and appreciated. Suggestion: surface the "hides real browser origin" contract in user-facing config documentation (YAML comments or config reference), not just inline source, so operators understand the auth implications without reading Go.

---

Tests

TestProxyRoute_WebSocketCrossOrigin — solid regression. The gorilla CheckOrigin directly mirrors the livetemplate production checker; faithful reproduction of the bug.

TestProxyRoute_RewritesOriginOnlyForWS — good unit coverage of the gating logic. Minor nit: a comment on wantUpstreamOrigin := upstream.URL noting that httptest.NewServer returns a bare scheme://host URL (no trailing path) would clarify why this equals the expected upstream origin.

TestProxyRoute_WebSocketUpgradeE2E

• Four-channel diagnostic capture is thorough; acceptance signal (101 + frame, not isReady()) correctly avoids the masking issue this bug caused in production.
• waitForStatus does not respect context cancellation — if ctx is cancelled the time.Sleep loop runs to deadline. Low priority (only affects how timeouts surface, not correctness). A select on ctx.Done() / ticker.C would fix it.
• mu.Lock() held for the full diagnostic/assertion block is safe (goroutine only briefly holds the lock), but worth a comment for future readers.

---

Potential Issues / Risks

Area	Finding	Severity
Security docs	"Hides real browser origin" is code-comment-only; config reference should warn operators	Low
waitForStatus ctx handling	Polling loop ignores cancellation; benign in practice	Low
Multi-value Upgrade header	Header.Get returns first value only; not a real-world WS issue	Negligible

---

Summary

Correct, well-scoped fix with sound root-cause analysis and excellent test coverage. Main actionable item: add a user-facing documentation note about the Origin-rewrite security contract for proxy routes. The waitForStatus context-cancellation issue is a minor follow-up.

[claude]

Code Review

Overview

This is a clean, tightly-scoped fix for the WebSocket 403 in proxied routes. The root-cause analysis is correct: httputil.ReverseProxy already forwards WS upgrades natively, but rewriting req.Host without rewriting Origin breaks the upstream's scheme://Host equality check. The fix — rewriting Origin only on WS upgrades — is the standard pattern and is properly gated and documented.

---

Correctness

isWebSocketUpgrade — correctly handles the multi-token Connection header (e.g. keep-alive, Upgrade) using case-insensitive token scanning. strings.SplitSeq is Go 1.24+; the repo is on 1.26 so this is fine.

upstreamOrigin construction — u.Scheme + "://" + u.Host is correct: net/url.URL.Host already includes the port when non-standard, matching what the Origin header spec requires.

Guard condition — isWebSocketUpgrade(req) && req.Header.Get("Origin") != "" correctly skips non-browser clients that omit Origin.

---

Nit: req.Header.Get("Connection") reads only the first header line

Get returns the first value. In theory a request could carry multiple Connection header lines; http.Header.Values scans all of them. In practice browsers never split it, so no real impact — but the robust form is:

for _, line := range req.Header.Values("Connection") {
    for tok := range strings.SplitSeq(line, ",") {
        if strings.EqualFold(strings.TrimSpace(tok), "upgrade") {
            return true
        }
    }
}

Not a blocker.

---

Minor: Director comment is slightly over-explained

The new block repeats the security contract already in the docs note added by this PR. One sentence referencing the docs would suffice. Keeping the longer form is fine too.

---

Tests

Three tests, each with a distinct job:

Test	What it proves
TestProxyRoute_WebSocketCrossOrigin	Integration: cross-origin 403 is gone, echo works
TestProxyRoute_RewritesOriginOnlyForWS	Unit: Origin rewritten for WS, untouched for plain HTTP
TestProxyRoute_WebSocketUpgradeE2E	Browser: 101 handshake + frame via CDP, not the masked isReady() signal

The E2E waitForStatus helper correctly avoids time.Sleep races. The four-channel diagnostic dump on any failure is excellent practice.

One small observation in authorProxyFixture: YAML config is built with string concatenation. For httptest URLs (http://127.0.0.1:<port>) this is safe; worth noting if the helper is ever reused with user-supplied upstream URLs.

---

Security

The trade-off is correctly identified and documented in both the code comment and the new docs note. The isWebSocketUpgrade gate ensures non-WS CORS is untouched.

---

Summary

Well-executed. The root-cause write-up is accurate, the implementation is minimal, and the tests specifically reproduce the failure mode rather than just asserting the happy path. The req.Header.Values nit is the only thing worth a follow-up; everything else is correct or cosmetic. LGTM.

[claude]

Code Review — fix(server): proxy routes forward WebSocket upgrades (rewrite Origin) (#257)

Overall: Well-scoped, accurately diagnosed fix with solid test coverage. The root cause analysis is correct and the change is minimal. A few items below worth addressing or acknowledging.

---

What works well

• Root cause is correct. The PR description correctly identifies that stdlib httputil.ReverseProxy handles WS upgrade forwarding natively (Go 1.12+); the actual bug is the Origin/Host mismatch that gorilla's CheckOrigin rejects. Good investigative work.
• Minimal blast radius. The rewrite is gated on both isWebSocketUpgrade and a non-empty Origin, so non-WS CORS behaviour is completely untouched.
• Test pyramid is solid. Three distinct levels: unit (RewritesOriginOnlyForWS), integration (WebSocketCrossOrigin), and real-browser E2E (WebSocketUpgradeE2E). Each catches a different failure mode.
• E2E signal is correct. Asserting on the 101 handshake status + a received WS frame via CDP — rather than isReady() — directly targets the masked regression. Good choice.
• Security contract documented in both the Director closure and the user-facing docs. The note about Origin-as-authorization is the right thing to call out.

---

Issues / concerns

1. strings.SplitSeq requires Go 1.24+ (minor, but worth noting)

for tok := range strings.SplitSeq(line, ",") {

strings.SplitSeq is a rangefunc iterator added in Go 1.24. The PR says the repo is on Go 1.26 so it builds, but if the go.mod minimum ever drops below 1.24 this would silently break at the build stage rather than at the call site. A short comment above isWebSocketUpgrade noting the Go version dependency would make it visible to future maintainers.

2. Missing test case: WS upgrade with empty Origin

The guard req.Header.Get("Origin") != "" is correct — a WS dial with no Origin header should not have one injected. But TestProxyRoute_RewritesOriginOnlyForWS doesn't exercise that branch. A third sub-case would lock in the invariant:

// WS upgrade with no Origin header — must not inject one.
noOriginReq := httptest.NewRequest(http.MethodGet, "http://docs.example.com/proxy/ws", nil)
noOriginReq.Header.Set("Upgrade", "websocket")
noOriginReq.Header.Set("Connection", "Upgrade")
noOriginRec := httptest.NewRecorder()
pr.handler.ServeHTTP(noOriginRec, noOriginReq)
if got := noOriginRec.Header().Get("X-Saw-Origin"); got != "" {
    t.Errorf("no-Origin WS: upstream saw Origin %q, want empty", got)
}

Not a blocker, but this edge case is real — some dialers omit Origin on same-host WS connections.

3. Global log redirect in the E2E test is a latent parallelism hazard

log.SetOutput(&tdLog)
t.Cleanup(func() { log.SetOutput(prevOut); ... })

The comment says tests run serially — that's fine for today. But if a future author adds t.Parallel() to any E2E test, or the binary is run with -parallel N, this global mutation will race. Consider capturing logs via a custom *log.Logger passed into the tinkerdown server, rather than redirecting the package-level global. At minimum, a comment like // SERIAL — never add t.Parallel() here would warn future authors.

4. Upgrade-without-Connection edge case (informational)

isWebSocketUpgrade correctly requires both Upgrade: websocket and a Connection header containing the upgrade token (RFC 7230 §6.7). A non-compliant client that sends only Upgrade: websocket without Connection: upgrade would return false — which is correct per spec. No test covers this, but the behaviour is right, so this is informational only.

---

Nits

• The alignment-only diff in TestProxyRouteMatches (trimming trailing spaces in table entries) is harmless but adds noise to git blame. Fine to keep if the project auto-formats.
• conn.SetReadDeadline(time.Now().Add(2 * time.Second)) in TestProxyRoute_WebSocketCrossOrigin is correct; if the project uses t.Context() (Go 1.21+) elsewhere for deadline propagation, consistency would be nice — but not a meaningful issue.

---

Summary

The fix is correct and well-targeted. Items 1 and 2 above are low-effort improvements (a comment and one extra test case). Item 3 warrants at least a // SERIAL guard comment. The PR is in good shape to merge once those are resolved or acknowledged.

[claude]

Code Review

Good diagnosis and a well-scoped fix. The root cause identification (Origin header mismatch, not missing WS forwarding) is confirmed correct — httputil.ReverseProxy has handled upgrades since Go 1.12, and the repo is on 1.26. A few observations below.

---

isWebSocketUpgrade (proxy_routes.go)

The implementation is correct per RFC 7230: Connection may arrive as multiple lines of comma-separated tokens, so iterating req.Header.Values and splitting on , is the right approach. Using strings.SplitSeq (Go 1.24+ range-over-func iterator, fine for a 1.26 module) is a nice touch.

Minor: The function doesn't check req.Method == http.MethodGet. RFC 6455 §4.1 requires GET, so a non-GET request with Upgrade: websocket headers set would pass this check and have its Origin rewritten unnecessarily. In practice the upstream will reject it, but a method guard would make the helper's intent more precise and self-documenting.

---

Origin rewrite in the Director (proxy_routes.go:68)

if isWebSocketUpgrade(req) && req.Header.Get("Origin") != "") {
    req.Header.Set("Origin", upstreamOrigin)
}

The != "" guard is correct — it avoids injecting an Origin where the dialer sent none (same-host clients, some non-browser clients). The upstreamOrigin computation (u.Scheme + "://" + u.Host) handles both default-port and explicit-port upstreams correctly since url.Parse normalises the Host field.

---

Security documentation

Both the inline comment and the embed-lvt.md note clearly state the contract: the upstream sees its own origin, not the real browser origin. This is the right level of disclosure for a reverse-proxy-level rewrite. ✓

---

Test coverage

TestProxyRoute_WebSocketCrossOrigin — directly reproduces the #257 scenario with a gorilla CheckOrigin that mirrors createSecureOriginChecker's production default. Good.

TestProxyRoute_RewritesOriginOnlyForWS — the three sub-cases (WS with origin, plain HTTP with origin, WS without origin) cover all branches of the gate condition. The technique of echoing the seen Origin via X-Saw-Origin is race-free since ServeHTTP returns only after the response is written. ✓

TestProxyRoute_WebSocketUpgradeE2E — the explicit rejection of isReady() as the acceptance signal is the key design decision here and it's correct; isReady() staying true under HTTP fallback is exactly what masked this in production. Using CDP EventWebSocketHandshakeResponseReceived to assert 101 and EventWebSocketFrameReceived to assert frame delivery gives a crisp, unforgeable signal.

One thing to flag: the waitForStatus helper returns nil on seeing "WS_ERROR", which lets chromedp.Run succeed even on the error path. The intent is that the assertion block below will catch it — and it does — but a reader scanning just waitForStatus could miss this subtlety. A short comment at the WS_ERROR branch explaining "settle early so assertions report the root cause" would help.

authorProxyFixture builds YAML via string concatenation with upstreamURL interpolated bare:

"    upstream: \"" + upstreamURL + "\"\n"

httptest.NewServer always returns http://127.0.0.1:PORT so there's no injection risk here, but since this is a test helper that future tests might copy, it might be worth using fmt.Sprintf with %q or a yaml.Marshal call for hygiene.

---

Misc

The alignment fix in the existing table-driven test (removing double-space alignment) is a cosmetic change that's fine but unrelated to the fix — worth noting in case the diff reviewer is confused about why table formatting changed.

---

Summary: The fix is correct and minimal. The security contract is well-documented. Tests are genuinely regression-resistant (not just coverage-padding). The two items above (req.Method guard, WS_ERROR comment in waitForStatus) are nits — nothing blocking. Ready to merge pending those.

[claude]

Code Review

Summary: This PR fixes a 403 on proxied WebSocket upgrades by rewriting the Origin header to the upstream origin inside the proxy Director, gated strictly on WS upgrade requests. The root cause analysis (Go's stdlib ReverseProxy forwards WS natively; the real bug is Origin ≠ scheme://Host after host rewriting) is correct.

Overall this is a well-scoped, well-documented fix. A few observations below.

---

Correctness

• isWebSocketUpgrade logic is RFC-compliant. Guarding on GET method, Upgrade: websocket (case-insensitive via EqualFold), and scanning every token in every Connection header line (comma-split + trim) correctly implements RFC 6455 §4.1. No issues.
• Origin != "" guard prevents injecting an Origin header where the client omitted one — correct.
• upstreamOrigin computation (u.Scheme + "://" + u.Host) is correct for both http/https; browsers send https://host as the Origin for wss:// connections, so the scheme mapping is right.
• strings.SplitSeq (Go 1.24+) is noted in comments and consistent with the module's go 1.26 requirement.

---

Potential Issues

1. E2E test: global log.SetOutput mutation is fragile

// proxy_ws_e2e_test.go:57-60
log.SetOutput(&tdLog)
t.Cleanup(func() { log.SetOutput(prevOut); ... })

The comment warns against t.Parallel(), but the guard is only advisory. If this file is ever run alongside another test that also mutates the global logger (or if a future developer adds t.Parallel() without noticing the comment), there's a data race. Consider using a build tag, a testing.T.Setenv-style helper, or asserting non-parallel with a mutex sentinel to make this structural rather than documentary-only.

2. waitForStatus swallows WS_ERROR silently

if strings.HasPrefix(txt, wantPrefix) || strings.HasPrefix(txt, "WS_ERROR") {
    return nil // no error returned
}

Exiting without an error means chromedp.Run succeeds on WS_ERROR, and the test only fails later at the #status assertion. The failure message will read #status = "WS_ERROR", want ... which is clear enough, but returning a descriptive error here would produce a faster, more localized failure without having to read the diagnostic dump to understand why chromedp.Run succeeded but assertions failed.

3. Sec-WebSocket-Key not checked in isWebSocketUpgrade

The current three-condition check (GET + Upgrade: websocket + Connection: Upgrade token) is sufficient for detecting legitimate handshakes from well-behaved clients. However, an adversarial HTTP/1.1 request could satisfy all three without being a real WebSocket handshake. Since the consequence of a false positive is just rewriting the Origin header on a non-WS request (not a security bypass), this is low severity — but worth noting. Adding a Sec-WebSocket-Key != "" check would make the intent explicit and match RFC 6455 §4.1 more fully.

---

Test Coverage

The three-tier test strategy (integration → unit gate → E2E) is well-structured:

• TestProxyRoute_WebSocketCrossOrigin — reproduces the actual bug scenario end-to-end at the Go level. ✅
• TestProxyRoute_RewritesOriginOnlyForWS — unit-level proof of the gating logic including the no-origin case. ✅
• TestProxyRoute_WebSocketUpgradeE2E — real-browser coverage with the right acceptance signal (101 + frame, not isReady()). ✅

The noOriginReq sub-case in the unit test is a good addition — explicitly verifies no Origin injection for clients that omit it.

---

Security

The PR is appropriately transparent about the security contract:

• The inline comment (// Security contract: ...) and the docs note (embed-lvt.md) both explain that Origin rewriting hides the real browser origin from the upstream.
• Non-WS CORS is explicitly untouched.
• The scoping to WS upgrades only limits blast radius significantly compared to a blanket Origin rewrite.

One minor doc nit: the security note in embed-lvt.md says "should not be fronted by a proxy route without additional authentication" — it might be worth clarifying that this advice applies specifically to upstreams that use Origin for authorization (not same-origin CSRF protection), since the preceding sentence already states this but readers skimming may miss the qualification.

---

Cosmetic

The alignment-only change to TestProxyRouteMatches table entries (extra space removed) is harmless but unrelated to the fix — could have gone in a separate commit to keep the diff focused. Minor.

---

Verdict

Approve with suggestions. The fix is correct, minimal, and well-tested. Items 1 and 2 above are worth addressing before merge; item 3 is optional hardening.