From 5e2a9432f142416de7d3d72f0d6b08ad999a23e4 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 23:03:20 -0700 Subject: [PATCH] fix(cli,studio): close the five R4 blocking gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — the required Test lane was red, and it was my test. The hostile-Host SPA case asserted a 200, which only holds when packages/studio/dist is built: true on a dev box, false in CI, so it passed locally and failed there. The Host split moved into a pure buildStudioHeadScriptsForHost() and is asserted directly; the route test no longer depends on build state. Verified by running the CLI suite with the bundle moved aside — 2330 pass. P1 — studio:* still bypassed most privacy controls. It honoured two localStorage keys but not navigator.doNotTrack, VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode or API-key eligibility, and canary enrolment honoured a different single control. New telemetry/policy.ts is the one answer to "may this profile be measured", consumed by both transports and by enrolment. It imports only ./config, so no cycle with the modules that import it. Each control is asserted individually. P1 — LAN/remote preview lost the authoritative decisions. Withholding the whole head script for any non-loopback Host also dropped the safe {enabled, forced} map, sending a supported HYPERFRAMES_PREVIEW_HOST= 0.0.0.0 Studio back to re-deriving. Identity injection is now gated separately from decision injection: identity is loopback-only, decisions always publish. P1 — the breaker latch was neither authoritative nor truthfully persisted. syncInstallState swallowed its own failures so writeConfigWithResult always reported ok, and reads took the flag only from config.json. The latch is now merged into every effective read, which makes install-state authoritative and closes both the failed-mirror and stale-concurrent-writer paths; the write additionally reports mirrored: false rather than swallowing. P2 — public contracts. canary-rollouts.mdx said a config wipe loses the breaker (it does not) and documented the superseded {name: boolean} map with unconditional CLI precedence; both corrected, with the precedence ladder written out and the override exception stated explicitly. PR body rewritten — it still named ~/.local/state, claimed state survives deleting ~/.hyperframes, and carried stale counts. Tests: 2330 CLI (bundle absent), 3151 Studio, 24 core. Fault injection: reverting each fix alone fails 5 CLI / 5 Studio. Co-Authored-By: Claude Opus 5 (1M context) --- docs/contributing/canary-rollouts.mdx | 43 ++++++--- packages/cli/src/server/studioServer.test.ts | 21 +---- packages/cli/src/server/studioServer.ts | 16 ++-- .../cli/src/server/telemetryIdentity.test.ts | 45 +++++++++ packages/cli/src/server/telemetryIdentity.ts | 29 +++++- packages/cli/src/telemetry/config.test.ts | 59 ++++++++++++ packages/cli/src/telemetry/config.ts | 69 +++++++++++--- packages/studio/src/telemetry/canary.test.ts | 14 +++ packages/studio/src/telemetry/canary.ts | 4 +- packages/studio/src/telemetry/client.ts | 44 +-------- packages/studio/src/telemetry/policy.test.ts | 81 ++++++++++++++++ packages/studio/src/telemetry/policy.ts | 93 +++++++++++++++++++ .../studio/src/utils/studioTelemetry.test.ts | 30 +++--- packages/studio/src/utils/studioTelemetry.ts | 21 ++--- 14 files changed, 448 insertions(+), 121 deletions(-) create mode 100644 packages/studio/src/telemetry/policy.test.ts create mode 100644 packages/studio/src/telemetry/policy.ts diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index cda386b60..c224b2f0b 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -116,9 +116,11 @@ during the window*, not the instantaneous rate. Two practical consequences: far tighter than a 60-day one. - **The percentage is not the safety mechanism.** It bounds *initial* exposure and decays from there. Per-render verification and per-install circuit - breakers are what actually bound harm — and note that a breaker's state - lives in the same config file as the id, so a wipe loses both and the - install can re-enrol into a path that already failed it. + breakers are what actually bound harm. A breaker's tripped state lives in + `install-state.json`, separate from `config.json` and merged back in on every + read, so a `config.json` re-mint cannot re-enrol an install into a path that + already failed it. Deleting `~/.hyperframes` clears both, deliberately — + that is the user's reset. For *measurement* — "is this feature better?" — churn is harmless: re-bucketing is random, so it adds noise, not bias. It is specifically the blast-radius @@ -223,14 +225,33 @@ bucketing unit is a dedicated seed in `install-state.json`, inherited across telemetry id and never emitted. It does not outlive `~/.hyperframes`. **A CLI-launched Studio adopts the CLI's decisions rather than re-deriving -them.** The CLI publishes `window.__HF_CLI_CANARY_DECISIONS` — a plain -`{ name: boolean }` map — and Studio takes it as authoritative over its own -seed, URL override and the registry percentage. Re-deriving cannot agree in -the cases that matter: telemetry off (the CLI resolves `telemetry_opt_out`, -but Studio's opt-out is a separate localStorage flag it cannot see), an -`HF_CANARY_*` override (env vars never cross into the browser), or no seed -injected (Studio falls back to a different unit, so a different bucket). One -render spanning both surfaces must not run half-enrolled. +them.** The CLI publishes `window.__HF_CLI_CANARY_DECISIONS` — a +`{ name: { enabled, forced } }` map. Re-deriving cannot agree in the cases +that matter: telemetry off (the CLI resolves `telemetry_opt_out`, but Studio's +opt-out is a separate localStorage flag it cannot see), an `HF_CANARY_*` +override (env vars never cross into the browser), or no seed injected (Studio +falls back to a different unit, so a different bucket). One render spanning +both surfaces must not run half-enrolled. + +`forced` carries the provenance, and the precedence follows from it — highest +first: + +1. A **forced** CLI decision (`HF_CANARY_*`). Wins outright, including over + this profile's opt-out, exactly as a local URL override does. +2. A local `?hf_canary_*=` override, same reasoning. +3. **This profile's telemetry opt-out.** Checked before any percentage + decision: the two surfaces have independent opt-outs, and CLI telemetry + being on says nothing about whether this browser profile agreed to be + measured. A cohort roll must never enrol an opted-out profile. +4. The CLI's percentage decision. +5. Local evaluation (standalone Studio, or a canary the CLI did not publish). + +Identity is treated differently from decisions. `__HF_CLI_DISTINCT_ID` and +`__HF_CLI_BUCKET_SEED` are injected only for a loopback `Host` — a page that +rebinds its hostname to `127.0.0.1` would otherwise read them as same-origin. +The decisions map is not identifying, so it is published regardless, which +keeps a LAN preview (`HYPERFRAMES_PREVIEW_HOST=0.0.0.0`) in agreement with the +CLI instead of silently re-deriving. The decisions map is published even when telemetry is off — that is the case it exists for. It is safe to expose where the seed is not: booleans about diff --git a/packages/cli/src/server/studioServer.test.ts b/packages/cli/src/server/studioServer.test.ts index cb5ce853c..f11d0dee8 100644 --- a/packages/cli/src/server/studioServer.test.ts +++ b/packages/cli/src/server/studioServer.test.ts @@ -74,22 +74,11 @@ describe("host guarding on identity-bearing responses", () => { 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(""); - }); + // NOTE: the SPA-injection branch itself is covered in telemetryIdentity.test.ts + // via buildStudioHeadScriptsForHost. It cannot be asserted here: this route + // only reaches the injection branch when packages/studio/dist is built, + // which is true locally and false in the CI test lane, so a route-level + // assertion on the returned HTML passes on a dev box and fails in CI. it("refuses the identity endpoint for a hostile Host", async () => { server = createStudioServer({ projectDir: tmpProject() }); diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 210ce82f7..e026679c4 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -17,7 +17,7 @@ import { } from "./runtimeSource.js"; import { VERSION as version } from "../version.js"; import { - buildStudioHeadScripts, + buildStudioHeadScriptsForHost, isLoopbackHost, resolveCliTelemetryDistinctId, } from "./telemetryIdentity.js"; @@ -818,13 +818,13 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { // 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(); + // the same distinct id and seed out of the returned HTML. + // + // Only IDENTITY is withheld from an untrusted Host. The canary decisions + // map still goes out — it is non-identifying, and a LAN/remote Studio + // (`HYPERFRAMES_PREVIEW_HOST=0.0.0.0`) needs it to stay in agreement with + // the CLI. See buildStudioHeadScriptsForHost. + const headScript = buildStudioHeadScriptsForHost(buildRuntimeEnvScript(), c.req.header("host")); 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 4599fa3b3..83ea864c1 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -25,6 +25,7 @@ const { buildCliIdentityScript, buildStudioHeadScripts, isLoopbackHost, + buildStudioHeadScriptsForHost, } = await import("./telemetryIdentity.js"); describe("resolveCliTelemetryDistinctId", () => { @@ -206,3 +207,47 @@ describe("isLoopbackHost (DNS-rebinding guard on the identity endpoint)", () => expect(isLoopbackHost(host)).toBe(false); }); }); + +describe("buildStudioHeadScriptsForHost — Host split", () => { + const ENV = ""; + + beforeEach(() => { + shouldTrack.mockReset(); + readConfig.mockReset(); + canaryDecisions.mockReset(); + shouldTrack.mockReturnValue(true); + readConfig.mockReturnValue({ anonymousId: "machine-uuid", bucketSeed: "seed-uuid" }); + canaryDecisions.mockReturnValue({ "de-parallel-router": { enabled: true, forced: false } }); + }); + + it("publishes identity and decisions on a loopback Host", () => { + const head = buildStudioHeadScriptsForHost(ENV, "127.0.0.1:5173"); + expect(head).toContain("__HF_CLI_DISTINCT_ID"); + expect(head).toContain("__HF_CLI_BUCKET_SEED"); + expect(head).toContain("__HF_CLI_CANARY_DECISIONS"); + }); + + // DNS rebinding: identity must not be readable from a hostile origin. + it("withholds identity and seed from a hostile Host", () => { + const head = buildStudioHeadScriptsForHost(ENV, "evil.example.com"); + expect(head).not.toContain("__HF_CLI_DISTINCT_ID"); + expect(head).not.toContain("__HF_CLI_BUCKET_SEED"); + }); + + // ...but the decisions map is NOT identifying, and withholding it would send + // a supported LAN preview (HYPERFRAMES_PREVIEW_HOST=0.0.0.0) back to + // re-deriving locally and disagreeing with the CLI. + it.each(["evil.example.com", "192.168.1.10:5173", "my-dev-box.local:5173", undefined])( + "still publishes canary decisions for non-loopback Host %s", + (host) => { + const head = buildStudioHeadScriptsForHost(ENV, host); + expect(head).toContain("__HF_CLI_CANARY_DECISIONS"); + expect(head).not.toContain("__HF_CLI_DISTINCT_ID"); + }, + ); + + it("always keeps the env script, whatever the Host", () => { + expect(buildStudioHeadScriptsForHost(ENV, "evil.example.com")).toContain("__HF_STUDIO_ENV__"); + expect(buildStudioHeadScriptsForHost(ENV, "localhost")).toContain("__HF_STUDIO_ENV__"); + }); +}); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index ad53b54a7..f8d027274 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -119,10 +119,16 @@ function resolveCliCanaryDecisions(): Record | null { * or browser history. Empty string only when there is nothing at all to * publish. */ -export function buildCliIdentityScript(): string { +export function buildCliIdentityScript(options: { includeIdentity?: boolean } = {}): string { + const { includeIdentity = true } = options; const parts: string[] = []; - const cliId = resolveCliTelemetryDistinctId(); + // Identity is the only part gated on a trusted Host. The decisions map below + // is not identifying, and withholding it would push a LAN/remote Studio + // (`HYPERFRAMES_PREVIEW_HOST=0.0.0.0`, an explicitly supported mode) back to + // re-deriving locally — reopening exactly the CLI/Studio disagreement this + // whole mechanism exists to close. + const cliId = includeIdentity ? resolveCliTelemetryDistinctId() : null; if (cliId) { parts.push(`window.__HF_CLI_DISTINCT_ID=${encodeInlineScriptValue(cliId)};`); const seed = resolveCliBucketSeed(); @@ -154,6 +160,21 @@ export function buildCliIdentityScript(): string { * ordering in one pure, tested function guards against a future `` inject * silently landing ahead of the identity script and reintroducing a boot race. */ -export function buildStudioHeadScripts(envScript: string): string { - return `${buildCliIdentityScript()}${envScript}`; +export function buildStudioHeadScripts( + envScript: string, + options: { includeIdentity?: boolean } = {}, +): string { + return `${buildCliIdentityScript(options)}${envScript}`; +} + +/** + * The `` scripts for a request, given its `Host`. + * + * The Host split lives here rather than in the route so it is testable + * without a Studio bundle on disk — the route's own test can only reach the + * injection branch when `packages/studio/dist` happens to be built, which is + * true locally and false in the CI test lane. + */ +export function buildStudioHeadScriptsForHost(envScript: string, host: string | undefined): string { + return buildStudioHeadScripts(envScript, { includeIdentity: isLoopbackHost(host) }); } diff --git a/packages/cli/src/telemetry/config.test.ts b/packages/cli/src/telemetry/config.test.ts index 9f0dd1c29..09b42ff46 100644 --- a/packages/cli/src/telemetry/config.test.ts +++ b/packages/cli/src/telemetry/config.test.ts @@ -494,3 +494,62 @@ describe("a tripped breaker survives even total marker+seed corruption", () => { expect(readConfig().stateFileCorrupt).toBe(true); }); }); + +describe("the breaker latch is authoritative from install-state", () => { + let readConfig: typeof import("./config.js").readConfig; + let writeConfigWithResult: typeof import("./config.js").writeConfigWithResult; + let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH; + let STATE_PATH: typeof import("./config.js").STATE_PATH; + + beforeEach(async () => { + fsState.files.clear(); + vi.resetModules(); + ({ readConfig, writeConfigWithResult, CONFIG_PATH, STATE_PATH } = await import("./config.js")); + }); + + // The stale-writer race: a concurrent process rewrites config.json from a + // snapshot taken before the breaker fired, clearing the flag there. Without + // merging the latch on read, the next process re-enrols a machine whose + // router already failed. + it("rehydrates a latched breaker when config.json says otherwise", () => { + fsState.files.set( + STATE_PATH, + JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", deParallelRouterTrialFired: true }), + ); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed" }), + ); + expect(readConfig().deParallelRouterTrialFired).toBe(true); + }); + + it("does not invent a latch when neither store has one", () => { + fsState.files.set(STATE_PATH, JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z" })); + fsState.files.set( + CONFIG_PATH, + JSON.stringify({ telemetryEnabled: true, anonymousId: "id", bucketSeed: "seed" }), + ); + expect(readConfig().deParallelRouterTrialFired).toBeUndefined(); + }); + + it("reports mirrored:false when the state write fails but config.json lands", async () => { + const config = readConfig(); + const fs = await import("node:fs"); + const { __resetInstallStateSyncForTests } = await import("./config.js"); + __resetInstallStateSyncForTests(); + fsState.files.delete(STATE_PATH); + vi.mocked(fs.writeFileSync).mockImplementation((path, content) => { + if (String(path).startsWith(STATE_PATH)) throw new Error("EACCES"); + fsState.files.set(String(path), String(content)); + }); + + config.deParallelRouterTrialFired = true; + // The config write still succeeds — a failed mirror must never break it — + // but the caller can now see the safety fact reached only one store. + expect(writeConfigWithResult(config)).toEqual({ ok: true, mirrored: false }); + + vi.mocked(fs.writeFileSync).mockImplementation((path, content) => { + fsState.files.set(String(path), String(content)); + }); + }); +}); diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index ffbbb365f..30eb92451 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -147,6 +147,29 @@ function backfillBucketSeed(config: HyperframesConfig): void { if (!write.ok) warnSeedBackfillFailed(write.error); } +// The latch is monotonic — once tripped it never untrips — so one read per +// process is enough, and readConfig is hot (every command, every render). +let latchFromStateFile: boolean | undefined; + +/** + * Is the breaker latched according to install-state? + * + * Merged into EVERY effective read, which is what makes install-state + * authoritative for the latch rather than merely a mirror of config.json. + * Without this a failed mirror, or a stale concurrent writer that rewrites + * config.json from a pre-trip snapshot, leaves state latched and config + * false — and the next process happily re-enrols a machine whose router + * already failed. Verifying the write is not enough on its own; the read has + * to prefer the safety fact. + */ +function installStateLatchedFired(): boolean { + if (latchFromStateFile === undefined) { + const state = readInstallState(); + latchFromStateFile = isInstallState(state) && state.deParallelRouterTrialFired === true; + } + return latchFromStateFile; +} + /** Narrow the parse result to a usable record. */ function isInstallState(value: InstallState | InstallStateMiss): value is InstallState { return typeof value !== "string"; @@ -249,18 +272,27 @@ function nextInstallState( return next; } -function syncInstallState(config: HyperframesConfig): void { +/** @returns false if the mirror could not be written this call. */ +/** The write half, split out to keep the memo bookkeeping legible. */ +function applyInstallState(config: HyperframesConfig, wantFired: boolean): void { + const read = readInstallState(); + const state = isInstallState(read) ? read : null; + const next = nextInstallState(state, config); + if (next !== null) writeInstallState(next); + stateMarkerSynced = true; + stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; + if (wantFired) latchFromStateFile = true; +} + +function syncInstallState(config: HyperframesConfig): boolean { const wantFired = config.deParallelRouterTrialFired === true; - if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return; + if (stateMarkerSynced && (stateFiredSynced || !wantFired)) return true; try { - const read = readInstallState(); - const state = isInstallState(read) ? read : null; - const next = nextInstallState(state, config); - if (next !== null) writeInstallState(next); - stateMarkerSynced = true; - stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; + applyInstallState(config, wantFired); + return true; } catch { // Leave the memo unset so a later write retries. + return false; } } @@ -504,7 +536,10 @@ export function readConfig(): HyperframesConfig { // non-boolean/non-number JSON value (e.g. the STRING "false", which is // truthy in JS) for these two fields specifically, since they're read // with a bare truthy check at the call site (review finding). - deParallelRouterTrialFired: parsed.deParallelRouterTrialFired === true ? true : undefined, + // `|| installStateLatchedFired()` — a latch recorded in install-state + // wins over an untripped config.json, never the reverse. + deParallelRouterTrialFired: + parsed.deParallelRouterTrialFired === true || installStateLatchedFired() ? true : undefined, deParallelRouterTrialRenderCount: typeof parsed.deParallelRouterTrialRenderCount === "number" ? parsed.deParallelRouterTrialRenderCount @@ -575,7 +610,11 @@ export function writeConfig(config: HyperframesConfig): boolean { return writeConfigWithResult(config).ok; } -export type ConfigWriteResult = { ok: true } | { ok: false; error: string }; +export type ConfigWriteResult = + // `mirrored: false` means config.json landed but the install-state mirror + // did not. Not a write failure — the latch is merged back in on read — but + // callers persisting a tripped breaker may want to retry or warn. + { ok: true; mirrored?: false } | { ok: false; error: string }; /** * Persist config and retain the failure reason for user-facing commands that @@ -589,9 +628,13 @@ export function writeConfigWithResult(config: HyperframesConfig): ConfigWriteRes renameSync(tmpFile, CONFIG_FILE); cachedConfig = { ...config }; // Mirror into the install-state file (marker + tripped breaker) so no - // breaker write site has to remember to do it. - syncInstallState(config); - return { ok: true }; + // breaker write site has to remember to do it. A failed mirror does NOT + // fail the config write — config.json landed, and the latch is merged + // back in on read — but it is reported rather than swallowed so a caller + // persisting a tripped breaker can tell the safety fact only reached one + // of the two stores. + const mirrored = syncInstallState(config); + return mirrored ? { ok: true } : { ok: true, mirrored: false }; } catch (error) { // Non-fatal — telemetry should never break the CLI return { ok: false, error: normalizeErrorMessage(error) }; diff --git a/packages/studio/src/telemetry/canary.test.ts b/packages/studio/src/telemetry/canary.test.ts index c6bc0c5d0..802a4aaf2 100644 --- a/packages/studio/src/telemetry/canary.test.ts +++ b/packages/studio/src/telemetry/canary.test.ts @@ -6,6 +6,14 @@ import { evaluateCanary } from "@hyperframes/core/canary"; // Pin the registry: real entries move as rollouts ramp, and these tests are // about the BINDING (does the browser supply the right three inputs?), not // about whichever canaries happen to be live today. +// The policy reads import.meta.env.DEV, which vitest sets true — without +// this every case would resolve to telemetry_opt_out. Controlled explicitly +// so each test states the privacy posture it is exercising. +const policyState = { allowed: true }; +vi.mock("./policy", () => ({ + browserTelemetryAllowed: () => policyState.allowed, +})); + vi.mock("@hyperframes/core/canary-registry", async () => { const actual = await vi.importActual( "@hyperframes/core/canary-registry", @@ -43,6 +51,7 @@ function setSearch(search: string): void { } beforeEach(() => { + policyState.allowed = true; localStorage.clear(); sessionStorage.clear(); setSearch(""); @@ -198,6 +207,7 @@ describe("telemetry opt-out is canary opt-out", () => { const OPT_OUT_KEY = "hyperframes-studio:telemetryDisabled"; it("does not enrol an opted-out browser profile", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); // on-everywhere is at 100% — it would be on for everyone otherwise. expect(resolveCanary("on-everywhere")).toEqual({ @@ -207,17 +217,20 @@ describe("telemetry opt-out is canary opt-out", () => { }); it("never buckets an opted-out profile — no cohort is assigned at all", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); expect(resolveCanary("on-everywhere").bucket).toBeUndefined(); }); it("still honours an explicit URL override", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); setSearch("?hf_canary_off_everywhere=on"); expect(resolveCanary("off-everywhere")).toEqual({ enabled: true, reason: "forced_on" }); }); it("reports every canary as false when opted out", () => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); expect(canaryEventProperties()).toEqual({ "$feature/canary-on-everywhere": "false", @@ -281,6 +294,7 @@ describe("CLI-launched Studio adopts the CLI's decisions", () => { // opt-outs, and CLI telemetry being on says nothing about this profile. describe("precedence against Studio's own opt-out", () => { beforeEach(() => { + policyState.allowed = false; localStorage.setItem(OPT_OUT_KEY, "1"); }); diff --git a/packages/studio/src/telemetry/canary.ts b/packages/studio/src/telemetry/canary.ts index a8ef6a644..8c90f8088 100644 --- a/packages/studio/src/telemetry/canary.ts +++ b/packages/studio/src/telemetry/canary.ts @@ -41,7 +41,7 @@ import { } from "@hyperframes/core/canary"; import { CANARIES, findCanary } from "@hyperframes/core/canary-registry"; import { resolveStudioDistinctId } from "./distinctId"; -import { isOptedOut } from "./config"; +import { browserTelemetryAllowed } from "./policy"; import { safeSessionStorage } from "../utils/safeStorage"; /** `my-feature` → `hf_canary_my_feature`, the query param and storage key. */ @@ -212,7 +212,7 @@ function decideStudioCanary(name: string): CanaryDecision { const override = readOverride(definition.name); if (override === undefined) { - if (isOptedOut()) return { enabled: false, reason: "telemetry_opt_out" }; + if (!browserTelemetryAllowed()) return { enabled: false, reason: "telemetry_opt_out" }; if (fromCli !== undefined) return cohortOutcome(fromCli.enabled); } diff --git a/packages/studio/src/telemetry/client.ts b/packages/studio/src/telemetry/client.ts index f43aa3486..511571bb4 100644 --- a/packages/studio/src/telemetry/client.ts +++ b/packages/studio/src/telemetry/client.ts @@ -4,7 +4,8 @@ // All calls are fire-and-forget; telemetry must never break the studio UI. // --------------------------------------------------------------------------- -import { getAnonymousId, hasShownNotice, isOptedOut, markNoticeShown } from "./config"; +import { getAnonymousId, hasShownNotice, markNoticeShown } from "./config"; +import { browserTelemetryAllowed } from "./policy"; import { getBrowserSystemMeta } from "./system"; import { canaryEventProperties } from "./canary"; @@ -25,46 +26,11 @@ let eventQueue: QueuedEvent[] = []; let flushTimer: ReturnType | null = null; let telemetryEnabled: boolean | null = null; -function isDoNotTrackOn(): boolean { - return typeof navigator !== "undefined" && navigator.doNotTrack === "1"; -} - -function isApiKeyConfigured(): boolean { - return POSTHOG_API_KEY.startsWith("phc_"); -} - -// VITE_HYPERFRAMES_NO_TELEMETRY mirrors the CLI's HYPERFRAMES_NO_TELEMETRY=1 -// opt-out so HeyGen's own dev/CI builds can suppress telemetry from the studio -// bundle the same way. Vite injects it at build time. Match the CLI's -// affirmative privacy-control spellings. -// `import.meta.env` may be undefined in non-Vite bundlers (Next.js Turbopack). -function isBuildTimeOptOut(): boolean { - try { - const v = import.meta.env.VITE_HYPERFRAMES_NO_TELEMETRY as string | undefined; - return v !== undefined && ["1", "true", "yes", "on"].includes(v.trim().toLowerCase()); - } catch { - return false; - } -} - -// `import.meta.env.DEV` is true under `vite dev` / `vite preview`. Auto-suppress -// so developers running `hyperframes preview` don't pollute production telemetry. -function isViteDevMode(): boolean { - try { - return import.meta.env.DEV === true; - } catch { - return false; - } -} - export function shouldTrack(): boolean { if (telemetryEnabled !== null) return telemetryEnabled; - telemetryEnabled = - isApiKeyConfigured() && - !isBuildTimeOptOut() && - !isViteDevMode() && - !isOptedOut() && - !isDoNotTrackOn(); + // Delegated to telemetry/policy.ts so this transport, the older `studio:*` + // transport, and canary enrolment cannot drift apart again. + telemetryEnabled = browserTelemetryAllowed(); return telemetryEnabled; } diff --git a/packages/studio/src/telemetry/policy.test.ts b/packages/studio/src/telemetry/policy.test.ts new file mode 100644 index 000000000..e11bbbfae --- /dev/null +++ b/packages/studio/src/telemetry/policy.test.ts @@ -0,0 +1,81 @@ +// @vitest-environment happy-dom +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// Each control the browser telemetry policy enforces, asserted individually. +// This is the SSOT that `telemetry/client.ts`, `utils/studioTelemetry.ts` and +// canary enrolment all consult, so a gap here is a gap in all three — which is +// how `navigator.doNotTrack` and Vite dev mode came to suppress one transport +// but not the other, nor enrolment. + +const DOCUMENTED_OPT_OUT = "hyperframes-studio:telemetryDisabled"; +const LEGACY_OPT_OUT = "hf-studio-telemetry-opt-out"; + +describe("browserTelemetryAllowed", () => { + let browserTelemetryAllowed: typeof import("./policy").browserTelemetryAllowed; + + beforeEach(async () => { + localStorage.clear(); + vi.resetModules(); + // vitest sets import.meta.env.DEV; the policy suppresses under it, so the + // baseline has to be an explicitly production-like env. + vi.stubEnv("DEV", false); + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", ""); + Object.defineProperty(navigator, "doNotTrack", { value: null, configurable: true }); + ({ browserTelemetryAllowed } = await import("./policy")); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("allows telemetry with no control set", () => { + expect(browserTelemetryAllowed()).toBe(true); + }); + + it("refuses when the documented localStorage key is set", () => { + localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); + + // Anyone already opted out this way must never be quietly re-enabled by the + // move to the documented key. + it("refuses when the legacy localStorage key is set", () => { + localStorage.setItem(LEGACY_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it("refuses when navigator.doNotTrack is on", () => { + Object.defineProperty(navigator, "doNotTrack", { value: "1", configurable: true }); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it("refuses under Vite dev mode", async () => { + vi.stubEnv("DEV", true); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(false); + }); + + it.each(["1", "true", "yes", "on", " ON "])( + "refuses when VITE_HYPERFRAMES_NO_TELEMETRY=%s", + async (value) => { + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", value); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(false); + }, + ); + + it("ignores an unset or unrelated VITE_HYPERFRAMES_NO_TELEMETRY value", async () => { + vi.stubEnv("VITE_HYPERFRAMES_NO_TELEMETRY", "0"); + vi.resetModules(); + ({ browserTelemetryAllowed } = await import("./policy")); + expect(browserTelemetryAllowed()).toBe(true); + }); + + it("is not memoized — a mid-session opt-out takes effect immediately", () => { + expect(browserTelemetryAllowed()).toBe(true); + localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); + expect(browserTelemetryAllowed()).toBe(false); + }); +}); diff --git a/packages/studio/src/telemetry/policy.ts b/packages/studio/src/telemetry/policy.ts new file mode 100644 index 000000000..89f42ff71 --- /dev/null +++ b/packages/studio/src/telemetry/policy.ts @@ -0,0 +1,93 @@ +// --------------------------------------------------------------------------- +// Browser telemetry policy — the single answer to "may this profile be +// measured?", shared by every transport and by canary enrolment. +// +// This exists because the answer was previously duplicated and the copies had +// drifted: `telemetry/client.ts` enforced five controls, the older +// `utils/studioTelemetry.ts` transport enforced one (its own localStorage +// key), and canary evaluation enforced a different one. So a profile with +// `navigator.doNotTrack` set, or a Vite dev build, still emitted `studio:*` +// events AND could be bucketed into a rollout — under controls the public +// docs say disable both. +// +// Deliberately imports only `./config` (localStorage helpers). Nothing here +// may import a transport or the canary module: both of those import this, and +// the whole point is one definition with no cycle. +// --------------------------------------------------------------------------- + +import { isOptedOut } from "./config"; + +// Write-only PostHog project key, safe to embed in client code. Duplicated +// from client.ts intentionally — the eligibility check must not drag the +// transport (and its queue/timer state) into modules that only need the +// policy. +const POSTHOG_API_KEY = "phc_zjjbX0PnWxERXrMHhkEJWj9A9BhGVLRReICgsfTMmpx"; + +/** Legacy opt-out key predating `telemetry/config.ts`. Still honoured so + * anyone already opted out is never quietly re-enabled. */ +const LEGACY_OPT_OUT_KEY = "hf-studio-telemetry-opt-out"; + +function isLegacyOptedOut(): boolean { + try { + return localStorage.getItem(LEGACY_OPT_OUT_KEY) === "1"; + } catch { + return false; + } +} + +function isDoNotTrackOn(): boolean { + return typeof navigator !== "undefined" && navigator.doNotTrack === "1"; +} + +function isApiKeyConfigured(): boolean { + return POSTHOG_API_KEY.startsWith("phc_"); +} + +// VITE_HYPERFRAMES_NO_TELEMETRY mirrors the CLI's HYPERFRAMES_NO_TELEMETRY=1 +// opt-out so HeyGen's own dev/CI builds can suppress telemetry from the studio +// bundle the same way. Vite injects it at build time. Match the CLI's +// affirmative privacy-control spellings. +// `import.meta.env` may be undefined in non-Vite bundlers (Next.js Turbopack). +function isBuildTimeOptOut(): boolean { + try { + const v = import.meta.env.VITE_HYPERFRAMES_NO_TELEMETRY as string | undefined; + return v !== undefined && ["1", "true", "yes", "on"].includes(v.trim().toLowerCase()); + } catch { + return false; + } +} + +// `import.meta.env.DEV` is true under `vite dev` / `vite preview`. Auto-suppress +// so developers running `hyperframes preview` don't pollute production telemetry. +function isViteDevMode(): boolean { + try { + return import.meta.env.DEV === true; + } catch { + return false; + } +} + +/** + * May this browser profile be measured at all? + * + * Governs BOTH sending events and canary enrolment. Enrolment is part of + * measurement, not separate from it: a profile that reports nothing cannot be + * compared against anyone, so bucketing it changes that user's code path for + * no signal. The one documented exception is an explicit `HF_CANARY_*` / + * `?hf_canary_*=` override, which callers apply before consulting this. + * + * Not memoized — `isOptedOut()` reads localStorage, which a user can flip in + * DevTools mid-session, and the per-call cost is a couple of property reads. + * Callers that must stay stable within a session memoize their own result + * (canary decisions do; the transports intentionally do not). + */ +export function browserTelemetryAllowed(): boolean { + return ( + isApiKeyConfigured() && + !isBuildTimeOptOut() && + !isViteDevMode() && + !isOptedOut() && + !isLegacyOptedOut() && + !isDoNotTrackOn() + ); +} diff --git a/packages/studio/src/utils/studioTelemetry.test.ts b/packages/studio/src/utils/studioTelemetry.test.ts index a50b0bbb6..b7eb42cf6 100644 --- a/packages/studio/src/utils/studioTelemetry.test.ts +++ b/packages/studio/src/utils/studioTelemetry.test.ts @@ -6,18 +6,24 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; // canary work established: the documented opt-out did not silence it, and its // events carried no cohort assignment. These pin both. -const DOCUMENTED_OPT_OUT = "hyperframes-studio:telemetryDisabled"; -const LEGACY_OPT_OUT = "hf-studio-telemetry-opt-out"; - vi.mock("../telemetry/canary", () => ({ canaryEventProperties: () => ({ "$feature/canary-test-one": "true" }), })); +// One shared policy now governs this transport, telemetry/client.ts and +// canary enrolment. Exercised directly here so each case names the control +// under test rather than relying on ambient import.meta.env. +const policyState = { allowed: true }; +vi.mock("../telemetry/policy", () => ({ + browserTelemetryAllowed: () => policyState.allowed, +})); + describe("studioTelemetry — shared opt-out and canary properties", () => { let trackStudioEvent: typeof import("./studioTelemetry").trackStudioEvent; let fetchMock: ReturnType; beforeEach(async () => { + policyState.allowed = true; localStorage.clear(); vi.resetModules(); vi.useFakeTimers(); @@ -40,19 +46,15 @@ describe("studioTelemetry — shared opt-out and canary properties", () => { return parsed.batch ?? []; } - it("honours the documented opt-out key", async () => { - // Previously only the legacy key was checked, so a user who opted out the - // documented way kept emitting every `studio:*` event. - localStorage.setItem(DOCUMENTED_OPT_OUT, "1"); - trackStudioEvent("thing_happened"); - expect(await sentEvents()).toHaveLength(0); - }); - - it("still honours the legacy opt-out key", async () => { - // Anyone already opted out this way must not be quietly re-enabled. - localStorage.setItem(LEGACY_OPT_OUT, "1"); + // Every control the shared policy enforces — documented key, legacy key, + // navigator.doNotTrack, VITE_HYPERFRAMES_NO_TELEMETRY, Vite dev mode, API + // key eligibility. Before the policy was shared this transport honoured + // only the legacy key, so all of the others still emitted `studio:*`. + it("sends nothing when the shared policy refuses", async () => { + policyState.allowed = false; trackStudioEvent("thing_happened"); expect(await sentEvents()).toHaveLength(0); + expect(fetchMock).not.toHaveBeenCalled(); }); it("attaches canary assignments to every event", async () => { diff --git a/packages/studio/src/utils/studioTelemetry.ts b/packages/studio/src/utils/studioTelemetry.ts index 83b4fe2ed..373127d0c 100644 --- a/packages/studio/src/utils/studioTelemetry.ts +++ b/packages/studio/src/utils/studioTelemetry.ts @@ -1,5 +1,5 @@ import { resolveStudioDistinctId } from "../telemetry/distinctId"; -import { isOptedOut } from "../telemetry/config"; +import { browserTelemetryAllowed } from "../telemetry/policy"; import { canaryEventProperties } from "../telemetry/canary"; // PostHog public ingest key — write-only, safe to ship in the client bundle @@ -29,21 +29,14 @@ function getDistinctId(): string { } /** - * Honours BOTH opt-out keys. - * - * This path predates telemetry/config.ts and shipped its own key, so the - * documented `hyperframes-studio:telemetryDisabled` was silently ignored here - * — `studio:*` events kept flowing for anyone who opted out the documented - * way. The legacy key stays honoured so nobody who already opted out gets - * quietly re-enabled by this fix. + * This path predates telemetry/config.ts and enforced only its own + * localStorage key, so `navigator.doNotTrack`, VITE_HYPERFRAMES_NO_TELEMETRY, + * Vite dev mode and the documented `hyperframes-studio:telemetryDisabled` all + * failed to silence `studio:*` events. Now one shared policy governs every + * transport — including the legacy key, which it still honours. */ function isEnabled(): boolean { - if (isOptedOut()) return false; - try { - return localStorage.getItem("hf-studio-telemetry-opt-out") !== "1"; - } catch { - return true; - } + return browserTelemetryAllowed(); } function getSessionProperties(): EventProperties {