mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-07 10:06:21 +00:00
fix(cli,core): close the remaining canary review findings
Six findings from review, none behaviour-critical on their own but three of them quietly corrupt the data the rollout is judged by. Endpoint no longer serves bucketSeed (studioServer.ts). Studio gets its canary answers from the injected decisions map now, so nothing needed the seed over HTTP — and an unauthenticated local endpoint is a strictly worse place for it than a script scoped to Studio's own document. The endpoint itself predates this PR and still serves distinctId, so it also gains a Host guard: a remote page can rebind its hostname to 127.0.0.1 and read the response as same-origin, but the request still carries THAT hostname, which is what makes it refusable. predecessorFound no longer reports corruption as a fresh install. It returned null for both "file absent" and "file unreadable", so a partial disk write looked like a new machine — understating recoverable churn, the one thing the field measures. Now distinguishes absent from corrupt and emits install_state_file_corrupt alongside. A mangled markerAt no longer discards a salvageable bucketSeed. markerAt is only a timestamp and can be restamped; the seed cannot be recovered, and losing it silently re-rolls the install's cohort. The seed backfill no longer ignores its write result. An unwritable ~/.hyperframes meant a different seed every invocation with no diagnostic, and made the field's own "backfilled once" docstring false. Warns once per process with the underlying error. FNV-1a's ASCII constraint is now explicit rather than incidental. It hashes UTF-16 code units while reference FNV-1a is byte-oriented, so the two agree only on ASCII; the registry's kebab-case assertion is what makes non-ASCII unreachable, and both ends now say so. Not a live bug — names are kebab-case and units are UUIDs. de-parallel-router is pinned at 0%. The registry is data, so a ramp is a one-line edit with no review surface, and its own description says to ramp only alongside the circuit breaker. Tests: 8 new (corruption vs absence, seed salvage, backfill write failure, 17 host-guard cases, registry pin). One existing test asserted predecessorFound: false on corruption — that was the bug, updated with a note. Fault injection: restoring the old corrupt handling fails 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
98b23a8850
commit
31361b8e5b
@@ -18,7 +18,7 @@ import {
|
||||
import { VERSION as version } from "../version.js";
|
||||
import {
|
||||
buildStudioHeadScripts,
|
||||
resolveCliBucketSeed,
|
||||
isLoopbackHost,
|
||||
resolveCliTelemetryDistinctId,
|
||||
} from "./telemetryIdentity.js";
|
||||
import { emitStudioRenderComplete, emitStudioRenderError } from "./studioRenderTelemetry.js";
|
||||
@@ -655,11 +655,23 @@ export function createStudioServer(options: StudioServerOptions): StudioServer {
|
||||
// clients that can't rely on the injected global. Returns the CLI's anonymous
|
||||
// distinct id (no PII) so the browser session can join the CLI's PostHog
|
||||
// person, or `{ distinctId: null }` when CLI telemetry is disabled.
|
||||
//
|
||||
// Deliberately does NOT serve `bucketSeed`. Studio gets its canary answers
|
||||
// from the injected `window.__HF_CLI_CANARY_DECISIONS` (booleans, not the
|
||||
// value cohorts derive from), so nothing needs the seed over HTTP — and an
|
||||
// unauthenticated local endpoint is a strictly worse place for it than an
|
||||
// inline script scoped to Studio's own document.
|
||||
//
|
||||
// Host-guarded against DNS rebinding: a remote page can point a hostname it
|
||||
// controls at 127.0.0.1 and read this response as same-origin. Pinning the
|
||||
// Host header to a loopback name means such a request (which carries the
|
||||
// attacker's hostname) is refused. Same-origin Studio traffic always
|
||||
// presents the bound loopback host.
|
||||
app.get("/api/telemetry-identity", (c) => {
|
||||
return c.json({
|
||||
distinctId: resolveCliTelemetryDistinctId(),
|
||||
bucketSeed: resolveCliBucketSeed(),
|
||||
});
|
||||
if (!isLoopbackHost(c.req.header("host"))) {
|
||||
return c.json({ error: "forbidden" }, 403);
|
||||
}
|
||||
return c.json({ distinctId: resolveCliTelemetryDistinctId() });
|
||||
});
|
||||
|
||||
app.get("/api/events", (c) => {
|
||||
|
||||
@@ -20,8 +20,12 @@ vi.mock("../telemetry/canary.js", () => ({
|
||||
canaryDecisionsForStudio: () => canaryDecisions(),
|
||||
}));
|
||||
|
||||
const { resolveCliTelemetryDistinctId, buildCliIdentityScript, buildStudioHeadScripts } =
|
||||
await import("./telemetryIdentity.js");
|
||||
const {
|
||||
resolveCliTelemetryDistinctId,
|
||||
buildCliIdentityScript,
|
||||
buildStudioHeadScripts,
|
||||
isLoopbackHost,
|
||||
} = await import("./telemetryIdentity.js");
|
||||
|
||||
describe("resolveCliTelemetryDistinctId", () => {
|
||||
beforeEach(() => {
|
||||
@@ -167,3 +171,34 @@ describe("buildStudioHeadScripts", () => {
|
||||
expect(buildStudioHeadScripts(ENV_SCRIPT)).toBe(ENV_SCRIPT);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isLoopbackHost (DNS-rebinding guard on the identity endpoint)", () => {
|
||||
it.each([
|
||||
"localhost",
|
||||
"localhost:5173",
|
||||
"127.0.0.1",
|
||||
"127.0.0.1:5173",
|
||||
"127.1.2.3",
|
||||
"[::1]",
|
||||
"[::1]:5173",
|
||||
"LOCALHOST:5173",
|
||||
])("accepts the loopback host %s", (host) => {
|
||||
expect(isLoopbackHost(host)).toBe(true);
|
||||
});
|
||||
|
||||
it.each([
|
||||
undefined,
|
||||
"",
|
||||
// The rebinding case: an attacker hostname resolving to 127.0.0.1 still
|
||||
// arrives with ITS name in Host, which is what makes this catchable.
|
||||
"evil.example.com",
|
||||
"evil.example.com:5173",
|
||||
"127.0.0.1.evil.com",
|
||||
"notlocalhost",
|
||||
"localhost.evil.com",
|
||||
"192.168.1.10",
|
||||
"0.0.0.0",
|
||||
])("rejects %s", (host) => {
|
||||
expect(isLoopbackHost(host)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -43,7 +43,7 @@ export function resolveCliTelemetryDistinctId(): string | null {
|
||||
* and editor would split one user across cohorts. Same telemetry gate as the
|
||||
* distinct id: seeding is part of the identity stitch, not a separate channel.
|
||||
*/
|
||||
export function resolveCliBucketSeed(): string | null {
|
||||
function resolveCliBucketSeed(): string | null {
|
||||
try {
|
||||
if (!telemetryShouldTrack()) return null;
|
||||
const seed = readConfig().bucketSeed;
|
||||
@@ -53,6 +53,30 @@ export function resolveCliBucketSeed(): string | null {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Is this request's `Host` a loopback name the studio server could have been
|
||||
* reached on directly?
|
||||
*
|
||||
* Guards the identity endpoint against DNS rebinding: an attacker-controlled
|
||||
* page can resolve its own hostname to 127.0.0.1 and read the response as
|
||||
* same-origin, but the request still carries THAT hostname in `Host`. Genuine
|
||||
* same-origin Studio traffic always presents the bound loopback host.
|
||||
*
|
||||
* A bare `[::1]`/`localhost`/dotted-quad check rather than a full parse: the
|
||||
* port is irrelevant (any port on loopback is us), and anything exotic enough
|
||||
* to miss here should be refused rather than guessed at.
|
||||
*/
|
||||
export function isLoopbackHost(host: string | undefined): boolean {
|
||||
if (!host) return false;
|
||||
// Strip the port. IPv6 literals are bracketed (`[::1]:1234`), so take the
|
||||
// bracketed part when present and only split on ":" otherwise.
|
||||
const bracketed = /^\[([^\]]+)\]/.exec(host);
|
||||
const hostname = (bracketed ? bracketed[1] : host.split(":")[0])?.toLowerCase() ?? "";
|
||||
if (hostname === "localhost" || hostname === "::1" || hostname === "0:0:0:0:0:0:0:1") return true;
|
||||
// 127.0.0.0/8 — the whole loopback block, not just 127.0.0.1.
|
||||
return /^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostname);
|
||||
}
|
||||
|
||||
// JSON.stringify does not escape "<" or "/". Escaping both means no
|
||||
// "</script>" (or "</…") sequence can form in the emitted value, so it can
|
||||
// never terminate the inline <script> or open a new tag. (The values are
|
||||
|
||||
Reference in New Issue
Block a user