Unify local subdomain with builds subdomain#33
Conversation
|
Claude finished @calvinstudebaker's task in 2m 56s —— View job Code ReviewGeneralizes the dev-app's host handling from a single 🔴
|
|
Claude finished @calvinstudebaker's task in 3m 20s —— View job Code Review
Overall this is a clean, well-scoped refactor. Swapping a per-game FindingsShould fix
Worth considering (optional)
Nits
Nothing blocking. The two should-fix items are doc/comment regressions, not behavior bugs. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| "uploadDir": "/abs/path", | ||
| "gameSubdomain": "<gameId>.local.wavedashcdn.com", | ||
| "gameSubdomain": "<gameCloudId>-<userHash>.local.wavedashcdn.com", |
There was a problem hiding this comment.
The field name and example value here are now inconsistent with the code. The Rust side (src/dev/launcher.rs:40) serializes local_host_suffix → localHostSuffix (camelCase), and main.ts:57 reads localHostSuffix. Also, the value isn't the full game subdomain anymore — it's just the suffix (e.g. local.wavedashcdn.com). The wildcard expansion happens in applyChromeSwitches and trustLocalCertFor, not in this field.
| "gameSubdomain": "<gameCloudId>-<userHash>.local.wavedashcdn.com", | |
| "localHostSuffix": "local.wavedashcdn.com", |
Also worth noting (separate from this PR but adjacent): the surrounding text on line 84 still says "CLI → dev-app over stdin" and line 103 says "Closing stdin from the CLI side signals the dev-app to quit", but the IPC switched to a temp config file + --parent-pid= watchdog (see main.ts:8-13 and launcher.rs:5-13). Worth a follow-up to fix the rest of the section while you're here.
| /** | ||
| * Headers every response on the game subdomain must carry. Mirrors the | ||
| * production play worker (`play/src/server/index.ts`). | ||
| * | ||
| * - `Origin-Agent-Cluster: ?1` is sticky per origin per BrowsingContextGroup, | ||
| * so a single response without it locks the origin into site-keyed mode for | ||
| * the rest of the chromium session and breaks `crossOriginIsolated`. | ||
| * - `Cross-Origin-Resource-Policy: cross-origin` is required because the | ||
| * mainsite (which sets COEP=require-corp) is on a different site than the | ||
| * playsite (wavedash.com vs wavedashcdn.com, and likewise in dev). | ||
| * Headers every response on the game subdomain must carry. | ||
| */ |
There was a problem hiding this comment.
The deletion here drops useful context that isn't actually local-vs-prod-specific — the explanation for why each header is set still applies. In particular the Origin-Agent-Cluster: ?1 note is load-bearing: it's a per-origin, per-BCG sticky flag, and one response missing it locks the origin into site-keyed mode for the rest of the chromium session. That's the kind of thing future-you (or someone else touching this) needs to know before removing or reordering a header.
Suggest restoring the body of the comment and just adjusting the lead sentence to drop the "game subdomain" framing:
| /** | |
| * Headers every response on the game subdomain must carry. Mirrors the | |
| * production play worker (`play/src/server/index.ts`). | |
| * | |
| * - `Origin-Agent-Cluster: ?1` is sticky per origin per BrowsingContextGroup, | |
| * so a single response without it locks the origin into site-keyed mode for | |
| * the rest of the chromium session and breaks `crossOriginIsolated`. | |
| * - `Cross-Origin-Resource-Policy: cross-origin` is required because the | |
| * mainsite (which sets COEP=require-corp) is on a different site than the | |
| * playsite (wavedash.com vs wavedashcdn.com, and likewise in dev). | |
| * Headers every response on the game subdomain must carry. | |
| */ | |
| /** | |
| * Headers every response on the local server must carry. Mirrors the | |
| * production play worker (`play/src/server/index.ts`). | |
| * | |
| * - `Origin-Agent-Cluster: ?1` is sticky per origin per BrowsingContextGroup, | |
| * so a single response without it locks the origin into site-keyed mode for | |
| * the rest of the chromium session and breaks `crossOriginIsolated`. | |
| * - `Cross-Origin-Resource-Policy: cross-origin` is required because the | |
| * mainsite (which sets COEP=require-corp) is on a different site than the | |
| * playsite (wavedash.com vs wavedashcdn.com, and likewise in dev). | |
| */ |
| // SNI is honored by https.createServer, so req.headers.host carries the | ||
| // browser-facing host (the game subdomain). We use that to build the | ||
| // proxy URL for passthrough paths. | ||
| const hostHeader = req.headers.host ?? config.gameSubdomain; | ||
| // browser-facing host (the game subdomain). chromium's host-rules MAP | ||
| // always sets it; we reject anything missing rather than guessing. | ||
| const hostHeader = req.headers.host; | ||
| if (!hostHeader) { | ||
| res.statusCode = 400; | ||
| res.end("Missing Host header"); | ||
| return; | ||
| } | ||
| // Strip the optional `:port` so the proxy origin and logs use the bare host. | ||
| const originHost = hostHeader.split(":")[0]; |
There was a problem hiding this comment.
Two thoughts here:
-
Stale framing: the comment still says "the game subdomain" (singular). With the wildcard MAP, it's now "any host under
localHostSuffix". Worth re-wording so the next reader doesn't get confused. -
No validation that
hostHeaderis actually underlocalHostSuffix: the value flows intoproxyToOriginas the upstreamhost(line 429). In practice the only path here is via chromium's host-rules MAP (single-label wildcard), so the host is constrained — but if anything ever reaches the server outside that path (a stray fetch from another local process, a misconfigured rule, etc.), this becomes a small SSRF: arbitraryHost: evil.com→ request goes toevil.com:443with the inbound headers. Cookie scoping should protect against credential leakage from chromium, but a cheap defense-in-depth would be:if (!originHost.endsWith(`.${config.localHostSuffix}`)) { res.statusCode = 400; res.end("Unexpected Host"); return; }
Up to you whether that's worth the line — fine to leave as-is given the local-only blast radius, but the assumption is worth making explicit either in code or in the comment.
| const dotSuffix = `.${localHostSuffix}`; | ||
| s.setCertificateVerifyProc((request, callback) => { | ||
| if (request.hostname === gameSubdomain) { | ||
| if (request.hostname.endsWith(dotSuffix)) { |
There was a problem hiding this comment.
Minor: endsWith(dotSuffix) accepts multi-level subdomains (a.b.local.wavedashcdn.com), but both the chromium MAP *.<suffix>:443 rule and the wildcard SAN cert only cover single-label (a.local.wavedashcdn.com). The asymmetry is benign today because the host-rules rule is what actually routes traffic here, so multi-level hosts never reach this proc — but pinning the check to single-label keeps the trust surface aligned with what host-rules can deliver, and reads as more intentional:
s.setCertificateVerifyProc((request, callback) => {
if (request.hostname.endsWith(dotSuffix)) {
const label = request.hostname.slice(0, -dotSuffix.length);
if (label.length > 0 && !label.includes(".")) {
callback(0);
return;
}
}
callback(-3);
});Optional — happy to leave as-is if you'd rather not add the extra check.
|
Claude finished @calvinstudebaker's task in 2m 0s —— View job Code Review (re-review of
|
1 participant
No need to special case local domain
Both builds domain and local domain use gcid and user hash as the subdomain