From f81ab0162e2add6c278518de8dc237450b5f8d93 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 18:35:22 -0700 Subject: [PATCH] fix(cli,studio): close the four R3 blocking gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — SPA route bypassed the DNS-rebinding guard. Guarding only /api/telemetry-identity left the catch-all as an open side door: a rebound origin could fetch `/` and read __HF_CLI_DISTINCT_ID and __HF_CLI_BUCKET_SEED straight out of the returned HTML. The SPA response now applies the same isLoopbackHost() check; an untrusted Host still gets a working Studio, just with no identity, seed or decisions injected. Route-level regression added. P1 — a CLI cohort roll could override Studio's own opt-out. decideStudioCanary() adopted the injected decision before checking isOptedOut(), so CLI-telemetry-on plus Studio-opted-out still enrolled Studio. A bare boolean could not express the difference between a deliberate override and an ordinary cohort roll, so the injected map now carries provenance ({ enabled, forced }). Forced wins outright — it is the documented escalation channel and must behave the same on both surfaces — while a percentage roll now loses to this profile's opt-out. Full interaction matrix tested. P1 — the legacy studio:* path sat outside both contracts. utils/studioTelemetry.ts shipped its own opt-out key and its own send loop, so the documented hyperframes-studio:telemetryDisabled did not silence it and its events carried no cohort assignment. It now honours both keys (the legacy one stays, so nobody already opted out is quietly re-enabled) and mixes in canaryEventProperties(), making "every telemetry event carries the assignment" actually true. P2 — partial salvage could drop a tripped breaker. salvageInstallState() discarded the whole record when markerAt and bucketSeed were both unusable, taking deParallelRouterTrialFired with it and re-enrolling a machine whose router already failed. All three fields are now independently salvageable. Docs: canary-rollouts.mdx said "disabling telemetry disables the reporting, not the enrolment" — exactly backwards since the opt-out gate landed. Corrected; checked for other copies, none. Tests: 13 new (4 opt-out precedence, 4 legacy-path opt-out and canary props, 3 route-level host guard, 2 breaker salvage). Fault injection: each of the four fixes reverted independently fails its own tests (2 CLI + 1 Studio + 2 Studio). Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 4 +- packages/cli/src/server/studioServer.test.ts | 54 ++++++++++++++ packages/cli/src/server/studioServer.ts | 12 ++- .../cli/src/server/telemetryIdentity.test.ts | 16 ++-- packages/cli/src/server/telemetryIdentity.ts | 4 +- packages/cli/src/telemetry/canary.ts | 26 ++++++- packages/cli/src/telemetry/config.test.ts | 29 ++++++++ packages/cli/src/telemetry/config.ts | 12 +-- packages/studio/src/telemetry/canary.test.ts | 54 +++++++++++--- packages/studio/src/telemetry/canary.ts | 58 ++++++++++----- .../studio/src/utils/studioTelemetry.test.ts | 74 +++++++++++++++++++ packages/studio/src/utils/studioTelemetry.ts | 17 ++++- 12 files changed, 313 insertions(+), 47 deletions(-) create mode 100644 packages/studio/src/utils/studioTelemetry.test.ts diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index 5d11c0c6e..cda386b60 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -80,7 +80,9 @@ tooling, so whoever operates the telemetry backend can split any metric by cohort with **nothing configured server-side** — while the decision itself still happens locally and offline, which the render path requires. (Assignments ride the same anonymous, opt-out telemetry pipeline as every -other event; disabling telemetry disables the reporting, not the enrolment.) +other event — and disabling telemetry disables the **enrolment**, not just the +reporting: an opted-out install is never bucketed at all. See the note at the +top of this page.) Two details worth knowing: diff --git a/packages/cli/src/server/studioServer.test.ts b/packages/cli/src/server/studioServer.test.ts index 466317aff..cb5ce853c 100644 --- a/packages/cli/src/server/studioServer.test.ts +++ b/packages/cli/src/server/studioServer.test.ts @@ -57,3 +57,57 @@ describe("createStudioServer autoProxy plumbing", () => { expect(server.adapter.autoProxy).toBe(true); }); }); + +describe("host guarding on identity-bearing responses", () => { + const dirs: string[] = []; + let server: StudioServer | undefined; + + function tmpProject(): string { + const dir = mkdtempSync(join(tmpdir(), "hf-studio-host-test-")); + dirs.push(dir); + return dir; + } + + afterEach(() => { + server?.watcher.close(); + server = undefined; + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + // A rebound origin can point its own hostname at 127.0.0.1 and read + // responses as same-origin. Guarding only /api/telemetry-identity left the + // SPA route as an open side door: fetching `/` returned the same distinct + // id and bucket seed inline in the HTML. + it("omits identity injection from the SPA response for a hostile Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/", { headers: { host: "evil.example.com" } }); + const html = await res.text(); + expect(html).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(html).not.toContain("__HF_CLI_BUCKET_SEED"); + expect(html).not.toContain("__HF_CLI_CANARY_DECISIONS"); + // Studio still loads — only the identity block is withheld. (The env + // script is empty here: it only emits with VITE_STUDIO_* vars set.) + expect(res.status).toBe(200); + expect(html).toContain(""); + }); + + it("refuses the identity endpoint for a hostile Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/api/telemetry-identity", { + headers: { host: "evil.example.com" }, + }); + expect(res.status).toBe(403); + expect(await res.text()).not.toContain('distinctId":"'); + }); + + it("serves the identity endpoint on a loopback Host", async () => { + server = createStudioServer({ projectDir: tmpProject() }); + const res = await server.app.request("/api/telemetry-identity", { + headers: { host: "127.0.0.1:5173" }, + }); + expect(res.status).toBe(200); + // The seed is no longer served here at all — Studio gets decisions + // injected instead, so nothing needs it over HTTP. + expect(Object.keys((await res.json()) as object)).toEqual(["distinctId"]); + }); +}); diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 742bee1d2..210ce82f7 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -814,7 +814,17 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // Inject before the studio bundle runs. Identity script first (see // buildStudioHeadScripts) so the CLI distinct id is on `window` by the time // telemetry init reads it. - const headScript = buildStudioHeadScripts(buildRuntimeEnvScript()); + // + // Host-guarded for the same reason /api/telemetry-identity is, and it has + // to be checked HERE too: guarding only the endpoint leaves this route as + // an open side door, since a rebound origin can simply fetch `/` and read + // the same distinct id and seed out of the returned HTML. Untrusted Host + // still gets a working Studio — it just gets the env script alone, with no + // identity, no seed, and no canary decisions. + const trustedHost = isLoopbackHost(c.req.header("host")); + const headScript = trustedHost + ? buildStudioHeadScripts(buildRuntimeEnvScript()) + : buildRuntimeEnvScript(); if (headScript) { html = html.replace("", `${headScript}`); } diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 98728555b..4599fa3b3 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -8,7 +8,7 @@ const shouldTrack = vi.fn(); const readConfig = vi.fn(); // Pinned rather than using the real registry, so these string assertions // don't move every time a canary is added, ramped, or retired. -const canaryDecisions = vi.fn<() => Record>(); +const canaryDecisions = vi.fn<() => Record>(); vi.mock("../telemetry/client.js", () => ({ shouldTrack: (...args: unknown[]) => shouldTrack(...args), @@ -99,10 +99,11 @@ describe("buildCliIdentityScript", () => { // stops Studio evaluating independently and enrolling anyway. it("still publishes canary decisions when telemetry is off, but no identity", () => { shouldTrack.mockReturnValue(false); - canaryDecisions.mockReturnValue({ "de-parallel-router": false }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: false, forced: false } }); const script = buildCliIdentityScript(); expect(script).toBe( - '', + "', ); expect(script).not.toContain("__HF_CLI_DISTINCT_ID"); expect(script).not.toContain("__HF_CLI_BUCKET_SEED"); @@ -111,17 +112,20 @@ describe("buildCliIdentityScript", () => { it("publishes decisions alongside the identity when telemetry is on", () => { shouldTrack.mockReturnValue(true); readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); - canaryDecisions.mockReturnValue({ "de-parallel-router": true }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: true, forced: true } }); expect(buildCliIdentityScript()).toBe( '', + "window.__HF_CLI_CANARY_DECISIONS=" + + '{"de-parallel-router":{"enabled":true,"forced":true}};', ); }); it("escapes a canary name that tries to close the script tag", () => { shouldTrack.mockReturnValue(false); - canaryDecisions.mockReturnValue({ "