From 6f0df2640b942e422b964ae564022f4874889788 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 1 Aug 2026 17:21:41 -0700 Subject: [PATCH] fix(cli,studio,core): close five R5 telemetry and canary findings - A long-lived preview cached its telemetry posture in two places (readConfig and shouldTrack). Running `telemetry disable` in another terminal left it resolving canaries and injecting the CLI id for hours. Both caches are now dropped together at a request boundary. - Studio minted and shipped a telemetry id for every render regardless of the browser profile's opt-out, and the server emitted the outcome under CLI policy, which cannot see localStorage or DNT. The browser now sends an explicit telemetryOptOut, distinct from an old client's omission. - Any non-empty HYPERFRAMES_PREVIEW_HOST disabled the DNS-rebinding guard, so even a loopback bind accepted a hostile Host. The guard now holds for loopback binds and, on a LAN bind, admits only names this machine answers on. - sunsetAfter had no reader of the current date. A scheduled workflow runs scripts/check-canary-sunset.ts weekly, so a failure lands on the rollout's owner rather than on an unrelated PR author. - The install-state seed memo outlived `rm -rf ~/.hyperframes`, resurrecting a cleared cohort. Removed; it only saved a read on a readConfig cache miss. Docs updated for the Host rule and the 100% exclusion carve-out. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/canary-sunset.yml | 26 ++++ docs/contributing/canary-rollouts.mdx | 42 +++++-- .../src/server/studioRenderTelemetry.test.ts | 25 ++++ .../cli/src/server/studioRenderTelemetry.ts | 13 ++ packages/cli/src/server/studioServer.ts | 5 + .../cli/src/server/telemetryIdentity.test.ts | 54 +++++++- packages/cli/src/server/telemetryIdentity.ts | 119 +++++++++++++++--- packages/cli/src/telemetry/client.ts | 17 +++ packages/cli/src/telemetry/config.ts | 21 ++-- packages/core/src/canary.test.ts | 9 +- .../studio-server/src/routes/render.test.ts | 49 ++++++++ packages/studio-server/src/routes/render.ts | 5 + packages/studio-server/src/types.ts | 7 ++ .../src/components/renders/useRenderQueue.ts | 19 ++- .../renders/useRenderQueueTelemetry.test.tsx | 104 +++++++++++++++ scripts/check-canary-sunset.ts | 36 ++++++ 16 files changed, 504 insertions(+), 47 deletions(-) create mode 100644 .github/workflows/canary-sunset.yml create mode 100644 packages/studio/src/components/renders/useRenderQueueTelemetry.test.tsx create mode 100644 scripts/check-canary-sunset.ts diff --git a/.github/workflows/canary-sunset.yml b/.github/workflows/canary-sunset.yml new file mode 100644 index 000000000..6b3d4b753 --- /dev/null +++ b/.github/workflows/canary-sunset.yml @@ -0,0 +1,26 @@ +# Canary sunset governance. +# +# Deliberately NOT a PR gate. The check reads the current date, so as a PR gate +# it would fail builds for authors who touched nothing related, on a calendar +# date, with no fix available to them. On a schedule the failure lands on the +# rollout's owner instead, which is who can actually ramp it to 100 and delete +# the guard. +name: Canary sunset + +on: + schedule: + # Mondays at 15:10 UTC, shortly after the CodeQL run. + - cron: "10 15 * * 1" + workflow_dispatch: + +jobs: + sunset: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: oven-sh/setup-bun@v2 + with: + bun-version: latest + # No install/build: the script imports the registry source directly and + # bun runs TypeScript natively. + - run: bun scripts/check-canary-sunset.ts diff --git a/docs/contributing/canary-rollouts.mdx b/docs/contributing/canary-rollouts.mdx index c224b2f0b..d1cb779da 100644 --- a/docs/contributing/canary-rollouts.mdx +++ b/docs/contributing/canary-rollouts.mdx @@ -54,8 +54,15 @@ call site. **3. Ramp it** by editing `percentage` in a patch release: `0 → 5 → 25 → 100`. **4. Delete it** once it is at 100 and holding — both the registry entry and -the branch it guarded. `sunsetAfter` exists to force this: a test fails once -the date passes. +the branch it guarded. `sunsetAfter` exists to force this: the scheduled +**Canary sunset** workflow runs weekly and fails once the date passes, naming +the rollout and its owner. + +It is a scheduled job rather than a PR check on purpose. A current-date +assertion in the unit suite would redden builds for authors who changed +nothing related, on a calendar date, with no fix available to them — so the +failure is routed to the person who can actually ramp or remove the canary. +Run it yourself with `bun scripts/check-canary-sunset.ts`. ## Overriding @@ -247,11 +254,16 @@ first: 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. +`__HF_CLI_BUCKET_SEED` are injected only for a `Host` this server can vouch +for — a page that rebinds its hostname to `127.0.0.1` would otherwise read +them as same-origin. That means loopback by default, and on an explicit LAN +bind (`HYPERFRAMES_PREVIEW_HOST=0.0.0.0`) a `Host` naming an address or +hostname this machine actually answers on. Setting the variable does not +disable the check; a hostile `Host` is refused in either mode. + 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. +keeps a LAN preview 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 @@ -283,12 +295,18 @@ An explicit `HF_CANARY_` (or `?hf_canary_=` in Studio) override still wins — that is a deliberate local choice, and it stays the way to exercise a canary with telemetry off. -**It fails closed.** No install id, an unregistered name, or a CI machine all -resolve to *not enrolled*. A canary exists to bound blast radius, so "we -don't know who this is" must never mean "enrol everyone". CI is excluded -because its config is regenerated per run, so its ids are ephemeral and would -hop cohorts between runs — an explicit override still reaches it, which is -how you test a canary in CI. +**It fails closed below 100%.** No install id, an unregistered name, or a CI +machine all resolve to *not enrolled*. A canary exists to bound blast radius, +so "we don't know who this is" must never mean "enrol everyone". CI is +excluded because its config is regenerated per run, so its ids are ephemeral +and would hop cohorts between runs — an explicit override still reaches it, +which is how you test a canary in CI. + +At exactly 100% those exclusions stop applying, deliberately. 100% is the step +before deleting the entry and the guard, so anything still resolving false +there would take the new path for the FIRST time at deletion, unstaged — and +CI and seedless installs are precisely the populations a dashboard cannot see, +so "100% and holding" would look green while they were never exercised. **Decisions are stable and memoized.** The same install always resolves the same way, and the answer is fixed for the life of a process — a render that diff --git a/packages/cli/src/server/studioRenderTelemetry.test.ts b/packages/cli/src/server/studioRenderTelemetry.test.ts index 07d126214..db89492f7 100644 --- a/packages/cli/src/server/studioRenderTelemetry.test.ts +++ b/packages/cli/src/server/studioRenderTelemetry.test.ts @@ -312,4 +312,29 @@ describe("studioRenderTelemetry", () => { expect(p.observabilityExtractVfrPreflightCount).toBe(6); }); }); + + // The browser profile's opt-out is invisible to the CLI's own policy, so + // without an explicit signal these fired for an opted-out user — attributed + // to the install id, which is worse than attributing them correctly. + describe("browser telemetry opt-out", () => { + it("emits nothing for a render whose browser opted out", () => { + emitStudioRenderComplete({ ...opts, telemetryOptOut: true }, 5000, fullPerf); + emitStudioRenderError( + { ...opts, telemetryOptOut: true }, + 1200, + "encode", + new Error("boom"), + undefined, + ); + expect(trackRenderComplete).not.toHaveBeenCalled(); + expect(trackRenderError).not.toHaveBeenCalled(); + }); + + // An older client sends no flag at all. That is not consent withdrawn, and + // treating it as such would silently drop every pre-upgrade render. + it.each([undefined, false])("still emits when telemetryOptOut is %s", (flag) => { + emitStudioRenderComplete({ ...opts, telemetryOptOut: flag }, 5000, fullPerf); + expect(trackRenderComplete).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/packages/cli/src/server/studioRenderTelemetry.ts b/packages/cli/src/server/studioRenderTelemetry.ts index f6d44d5a0..1feb70e6f 100644 --- a/packages/cli/src/server/studioRenderTelemetry.ts +++ b/packages/cli/src/server/studioRenderTelemetry.ts @@ -24,6 +24,17 @@ export interface StudioRenderOpts { // outcome joins their studio_session_start / studio_render_start events. // Undefined for older studio clients → falls back to the install anonymousId. distinctId?: string; + /** + * The browser profile that triggered this render has telemetry disabled. + * + * The CLI's own policy cannot see a localStorage opt-out or `DoNotTrack` in + * someone else's browser, so without this the server happily emitted + * render_complete / render_error for a user who had opted out — the events + * merely landed on the install id instead of theirs, which is worse, not + * better. Explicit `true` only: an old client sends nothing here, and that + * is not consent withdrawn. + */ + telemetryOptOut?: boolean; } type RenderCompleteProps = Parameters[0]; @@ -103,6 +114,7 @@ export function emitStudioRenderError( // user-supplied worker count (the producer picks its default), so on early // failures we genuinely don't know one. The CLI side has the value from // `options.workers` even before `job.perfSummary` exists; studio doesn't. + if (opts.telemetryOptOut === true) return; trackRenderError({ fps: fpsToNumber(opts.fps), quality: opts.quality, @@ -122,6 +134,7 @@ export function emitStudioRenderComplete( elapsedMs: number, perf: RenderPerfSummary | undefined, ): void { + if (opts.telemetryOptOut === true) return; trackRenderComplete({ durationMs: elapsedMs, fps: fpsToNumber(opts.fps), diff --git a/packages/cli/src/server/studioServer.ts b/packages/cli/src/server/studioServer.ts index 8b5e7c10a..fced4070f 100644 --- a/packages/cli/src/server/studioServer.ts +++ b/packages/cli/src/server/studioServer.ts @@ -19,6 +19,7 @@ import { VERSION as version } from "../version.js"; import { buildStudioHeadScriptsForHost, identityAllowed, + refreshTelemetryPosture, resolveCliTelemetryDistinctId, } from "./telemetryIdentity.js"; import { emitStudioRenderComplete, emitStudioRenderError } from "./studioRenderTelemetry.js"; @@ -671,6 +672,10 @@ export function createStudioServer(options: StudioServerOptions): StudioServer { if (!identityAllowed(c.req.header("host"))) { return c.json({ error: "forbidden" }, 403); } + // Same request-boundary refresh the head-script route does: this endpoint + // is polled by a long-lived Studio tab, so a cached posture here outlives + // an opt-out run in another terminal just as visibly. + refreshTelemetryPosture(); return c.json({ distinctId: resolveCliTelemetryDistinctId() }); }); diff --git a/packages/cli/src/server/telemetryIdentity.test.ts b/packages/cli/src/server/telemetryIdentity.test.ts index 169f9ceb2..7e0d126d9 100644 --- a/packages/cli/src/server/telemetryIdentity.test.ts +++ b/packages/cli/src/server/telemetryIdentity.test.ts @@ -1,3 +1,4 @@ +import { hostname, networkInterfaces } from "node:os"; import { afterEach, describe, expect, it, vi, beforeEach } from "vitest"; // CLI → Studio telemetry identity seeding (Layer 1). Verifies the server only @@ -253,6 +254,23 @@ describe("buildStudioHeadScriptsForHost — Host split", () => { }); }); +/** + * Non-loopback names this machine answers to. Computed, not hardcoded: the + * rule under test is "an address/name this host actually has", so a literal + * like `192.168.1.10` would pass only by accident on one developer's laptop. + */ +function localHostCandidates(): string[] { + const names = new Set(); + for (const entries of Object.values(networkInterfaces())) { + for (const entry of entries ?? []) { + if (!entry.internal && entry.family === "IPv4") names.add(entry.address); + } + } + const self = hostname().split(".")[0]; + if (self !== undefined && self !== "") names.add(`${self}.local`); + return [...names]; +} + describe("identityAllowed — loopback-bound vs explicitly LAN-bound", () => { const original = process.env["HYPERFRAMES_PREVIEW_HOST"]; @@ -285,16 +303,44 @@ describe("identityAllowed — loopback-bound vs explicitly LAN-bound", () => { }); // The mode this regressed: browsing your own LAN-exposed Studio lost the - // CLI stitch entirely, so the same human became two PostHog persons. - it.each(["0.0.0.0:3000", "192.168.1.10:3000", "my-dev-box.local:3000"])( + // CLI stitch entirely, so the same human became two PostHog persons. The + // names come from this machine, because that is now the actual rule — + // a hardcoded `192.168.1.10` asserted only that the check was absent. + it.each(["0.0.0.0:3000", ...localHostCandidates().map((n) => `${n}:3000`)])( "allows %s once the operator opted into LAN exposure", (host) => { expect(identityAllowed(host)).toBe(true); }, ); - it("still allows loopback in that mode", () => { - expect(identityAllowed("localhost:3000")).toBe(true); + // Setting the env var opted into LAN exposure, NOT into handing identity + // to whatever name a rebinding page invents. This is the hole: the old + // rule returned true for every one of these. + it.each(["evil.example.com", "127.0.0.1.evil.com", "attacker.test:3000", undefined])( + "still refuses hostile Host %s", + (host) => { + expect(identityAllowed(host)).toBe(false); + }, + ); + + it("refuses a LAN address this machine does not answer on", () => { + expect(identityAllowed("203.0.113.7:3000")).toBe(false); + }); + }); + + // A loopback bind exposes nothing, so the Host check stays a live rebinding + // mitigation — previously ANY non-empty value disabled it wholesale. + describe("bound to loopback explicitly", () => { + beforeEach(() => { + process.env["HYPERFRAMES_PREVIEW_HOST"] = "127.0.0.1"; + }); + + it.each(["localhost:5173", "127.0.0.1"])("still allows %s", (host) => { + expect(identityAllowed(host)).toBe(true); + }); + + it.each(["evil.example.com", "192.168.1.10:3000"])("still refuses %s", (host) => { + expect(identityAllowed(host)).toBe(false); }); }); }); diff --git a/packages/cli/src/server/telemetryIdentity.ts b/packages/cli/src/server/telemetryIdentity.ts index a518a8708..1e6d09b42 100644 --- a/packages/cli/src/server/telemetryIdentity.ts +++ b/packages/cli/src/server/telemetryIdentity.ts @@ -16,8 +16,12 @@ // server's heavy render dependencies (@hyperframes/producer, engine, …). // --------------------------------------------------------------------------- -import { readConfig } from "../telemetry/config.js"; -import { shouldTrack as telemetryShouldTrack } from "../telemetry/client.js"; +import { hostname, networkInterfaces } from "node:os"; +import { readConfig, readConfigFresh } from "../telemetry/config.js"; +import { + resetTelemetryPostureCache, + shouldTrack as telemetryShouldTrack, +} from "../telemetry/client.js"; import { canaryDecisionsForStudio, type CliCanaryDecision } from "../telemetry/canary.js"; /** @@ -178,28 +182,111 @@ export function buildStudioHeadScripts( * injection branch when `packages/studio/dist` happens to be built, which is * true locally and false in the CI test lane. */ +/** Hostname of a `Host` header, port and IPv6 brackets removed. */ +function hostnameOf(host: string | undefined): string { + if (!host) return ""; + const bracketed = /^\[([^\]]+)\]/.exec(host); + return (bracketed ? bracketed[1] : host.split(":")[0])?.toLowerCase() ?? ""; +} + +/** + * Names this machine legitimately answers to on a wildcard bind: every + * interface address, plus its own hostname and the `.local` mDNS form people + * actually type. Fail closed — a throw yields an empty set, which denies. + */ +function localNames(): Set { + const names = new Set(); + try { + for (const entries of Object.values(networkInterfaces())) { + for (const entry of entries ?? []) names.add(entry.address.toLowerCase()); + } + const self = hostname().toLowerCase(); + if (self !== "") { + names.add(self); + // `my-box` is reachable as `my-box.local`, and a `my-box.lan.example` + // FQDN is reachable by its short form. Register both directions. + names.add(`${self}.local`); + const short = self.split(".")[0]; + if (short !== undefined && short !== "") { + names.add(short); + names.add(`${short}.local`); + } + } + } catch { + /* fail closed */ + } + return names; +} + +/** + * Wildcard binds answer on every local name; a specific bind on itself only. + * + * This is the part that was missing: the previous rule accepted ANY Host once + * the env var was set, so a rebinding page could name itself `evil.example.com` + * and still be handed the CLI identity. + */ +function hostMatchesBind(host: string | undefined, bind: string): boolean { + const requested = hostnameOf(host); + if (requested === "") return false; + const bound = bind.toLowerCase(); + if (requested === bound) return true; + return bound === "0.0.0.0" || bound === "::" || bound === "*" + ? localNames().has(requested) + : false; +} + /** * May this request receive the CLI's identity (distinct id + bucket seed)? * - * Two regimes, because the server binds loopback by DEFAULT and exposes the - * LAN only when an operator sets `HYPERFRAMES_PREVIEW_HOST` (portUtils.ts, - * F-001): + * The server binds loopback by DEFAULT and exposes the LAN only when an + * operator sets `HYPERFRAMES_PREVIEW_HOST` (portUtils.ts, F-001). A loopback + * `Host` is always fine: whatever reached us came via loopback, and the only + * interesting attacker is a rebinding browser page — which the Host check + * catches, because a browser cannot forge `Host`. * - * - **Loopback-bound (default).** Anything reaching us came via loopback, so - * the only interesting attacker is a rebinding browser page — which the - * Host check catches, because a browser cannot forge `Host`. - * - **Explicitly LAN-bound.** The operator opted into exposing this server, - * and the Host header is trivially forgeable by any non-browser client, so - * the check buys nothing. Withholding identity there only broke the - * CLI-to-Studio stitch for the supported mode: the user browses - * `http://0.0.0.0:3000` or the machine's LAN IP, `isLoopbackHost` says no, - * and Studio mints a second anonymous person for the same human. + * For anything else the bind decides: + * + * - **Unset, or bound to loopback.** No LAN exposure was requested, so the + * Host check is a live rebinding mitigation and a non-loopback Host is + * refused. Previously ANY non-empty value disabled the check, so even + * `HYPERFRAMES_PREVIEW_HOST=127.0.0.1` — which exposes nothing — turned the + * loopback service from hostile-Host refusal into accept-everything. + * - **Bound to a LAN address.** The operator opted into exposure, so identity + * has to reach the LAN name the user browses or the CLI-to-Studio stitch + * breaks and Studio mints a second person for the same human. But it is + * still checked: the Host must name an address this machine actually + * answers on, not any attacker-chosen name a rebinding page supplies. */ export function identityAllowed(host: string | undefined): boolean { - const lanBound = (process.env["HYPERFRAMES_PREVIEW_HOST"] ?? "").trim() !== ""; - return lanBound || isLoopbackHost(host); + if (isLoopbackHost(host)) return true; + const bind = (process.env["HYPERFRAMES_PREVIEW_HOST"] ?? "").trim(); + if (bind === "" || isLoopbackHost(bind)) return false; + return hostMatchesBind(host, bind); +} + +/** + * Re-read the persisted telemetry preference for this request. + * + * Both caches have to go, together. `readConfig()` memoizes the parsed config + * and `shouldTrack()` memoizes its own boolean on top of it, so clearing + * either alone still yields the stale answer: the canary layer asks + * `readConfig()`, the identity layer asks `shouldTrack()`, and they would + * disagree mid-refresh. A long-lived `hyperframes preview` otherwise keeps + * serving pre-opt-out decisions and injecting the CLI id for hours after + * `hyperframes telemetry disable` runs in another terminal. + * + * Fail-silent and once per request — a config re-read, not a hot path. + */ +export function refreshTelemetryPosture(): void { + try { + readConfigFresh(); + resetTelemetryPostureCache(); + } catch { + /* telemetry must never break the preview server */ + } } export function buildStudioHeadScriptsForHost(envScript: string, host: string | undefined): string { + refreshTelemetryPosture(); return buildStudioHeadScripts(envScript, { includeIdentity: identityAllowed(host) }); } diff --git a/packages/cli/src/telemetry/client.ts b/packages/cli/src/telemetry/client.ts index 1c9a577c5..0cb1d34aa 100644 --- a/packages/cli/src/telemetry/client.ts +++ b/packages/cli/src/telemetry/client.ts @@ -38,6 +38,23 @@ export function shouldTrack(): boolean { return telemetryEnabled; } +/** + * Drop the cached posture so the next `shouldTrack()` re-reads the persisted + * preference. + * + * The memo is right for a CLI command — one process, one answer, and the + * question is asked per event. It is wrong for `hyperframes preview`, which + * lives for hours: run `hyperframes telemetry disable` in another terminal and + * this process kept the old answer indefinitely, still resolving canaries and + * still injecting the CLI id into every page load. Callers that serve requests + * refresh at a request boundary; see `refreshTelemetryPosture` in + * server/telemetryIdentity.ts, which invalidates this and the config cache + * together so the two cannot disagree. + */ +export function resetTelemetryPostureCache(): void { + telemetryEnabled = null; +} + /** * Queue a telemetry event. Non-blocking, fail-silent. * Enriches the event with system metadata, then hands it to the transport diff --git a/packages/cli/src/telemetry/config.ts b/packages/cli/src/telemetry/config.ts index df587663e..0b23eef9c 100644 --- a/packages/cli/src/telemetry/config.ts +++ b/packages/cli/src/telemetry/config.ts @@ -179,16 +179,20 @@ function installStateLatchedFired(): boolean { return latchedFiredSeen; } -// Same shape as the latch memo, and same reason for existing: readConfig is -// hot. A seed never changes once recorded, so the positive is cacheable. -let seedFromStateFile: string | undefined; - -/** The seed install-state has recorded for this machine, if any. */ +/** + * The seed install-state has recorded for this machine, if any. + * + * Deliberately NOT memoized, unlike the latch above. This is only reached on a + * `readConfig` cache miss, so a memo saved one `readFileSync` per re-parse — + * and cost the documented full reset: a long-lived preview that had read seed + * A, observed `rm -rf ~/.hyperframes`, and minted B would still be handed the + * cached A, resurrecting the cohort the user just cleared. The latch memo is + * different on purpose: it caches only `true`, and a breaker that survives a + * reset fails safe. + */ function installStateSeed(): string | undefined { - if (seedFromStateFile !== undefined) return seedFromStateFile; const state = readInstallState(); - if (isInstallState(state) && state.bucketSeed !== undefined) seedFromStateFile = state.bucketSeed; - return seedFromStateFile; + return isInstallState(state) ? state.bucketSeed : undefined; } /** Narrow the parse result to a usable record. */ @@ -309,7 +313,6 @@ function applyInstallState(config: HyperframesConfig, wantFired: boolean): void stateMarkerSynced = true; stateFiredSynced = wantFired || state?.deParallelRouterTrialFired === true; if (wantFired) latchedFiredSeen = true; - if (next?.bucketSeed !== undefined) seedFromStateFile = next.bucketSeed; } function syncInstallState(config: HyperframesConfig): boolean { diff --git a/packages/core/src/canary.test.ts b/packages/core/src/canary.test.ts index 03da853ca..19ef6a71e 100644 --- a/packages/core/src/canary.test.ts +++ b/packages/core/src/canary.test.ts @@ -313,9 +313,12 @@ describe("registry", () => { // Deliberately NOT `overdueCanaries()` with the ambient date. That assertion // reads wall-clock time, so it turns the entire @hyperframes/core suite red // on a calendar date for every unrelated PR — a broken build nobody caused - // and whose fix is unrelated to the change under test. The registry's own - // freshness is enforced by the pinned dates below plus the sunset REPORT, - // which is advisory rather than a gate. + // and whose fix is unrelated to the change under test. + // + // Enforcement against the CURRENT date is real, it just is not here: the + // scheduled `Canary sunset` workflow runs `scripts/check-canary-sunset.ts` + // weekly and fails on the rollout's owner rather than on a passing author. + // These two tests cover the pinned-date and boundary logic it depends on. it("every canary carries a parseable sunset date in the future at authoring time", () => { const authored = new Date("2026-07-31T00:00:00Z"); for (const c of CANARIES) { diff --git a/packages/studio-server/src/routes/render.test.ts b/packages/studio-server/src/routes/render.test.ts index b54987401..40183da19 100644 --- a/packages/studio-server/src/routes/render.test.ts +++ b/packages/studio-server/src/routes/render.test.ts @@ -602,6 +602,55 @@ describe("POST /projects/:id/render — telemetryDistinctId forwarding", () => { } }); + // Explicit suppression, forwarded so the CLI can honour a browser opt-out + // it has no other way to observe. + it("forwards an explicit telemetryOptOut", async () => { + const spy = vi.fn(); + const { app, cleanup } = buildApp(spy); + try { + const res = await app.request("http://localhost/projects/demo/render", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + fps: 30, + quality: "standard", + format: "mp4", + telemetryOptOut: true, + }), + }); + expect(res.status).toBe(200); + expect(spy.mock.calls[0][0].telemetryOptOut).toBe(true); + } finally { + cleanup(); + } + }); + + // An old client omits the flag, and a non-boolean is not a signal either. + // Defaulting those to "opted out" would silently drop every pre-upgrade + // render outcome. + it.each([undefined, false, "true"])( + "treats telemetryOptOut %s as not opted out", + async (flag) => { + const spy = vi.fn(); + const { app, cleanup } = buildApp(spy); + try { + await app.request("http://localhost/projects/demo/render", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + fps: 30, + quality: "standard", + format: "mp4", + telemetryOptOut: flag, + }), + }); + expect(spy.mock.calls[0][0].telemetryOptOut).toBe(false); + } finally { + cleanup(); + } + }, + ); + it("ignores a non-string telemetryDistinctId", async () => { const spy = vi.fn(); const { app, cleanup } = buildApp(spy); diff --git a/packages/studio-server/src/routes/render.ts b/packages/studio-server/src/routes/render.ts index 2b7f58450..bc0c6de27 100644 --- a/packages/studio-server/src/routes/render.ts +++ b/packages/studio-server/src/routes/render.ts @@ -66,6 +66,10 @@ export function registerRenderRoutes(api: Hono, adapter: StudioApiAdapter): void // Browser telemetry id, so the server-emitted render outcome is // attributed to the user who triggered the render (joinable funnel). telemetryDistinctId?: string; + // Explicit "this browser profile opted out" flag. Distinct from simply + // omitting the id: an OLD client omits it too, and that case falls back + // to the install anonymousId. Only an explicit `true` suppresses. + telemetryOptOut?: boolean; // Composition-variable overrides ({variableId: value}), injected as // window.__hfVariables — same channel as `hyperframes render --variables`. variables?: Record; @@ -126,6 +130,7 @@ export function registerRenderRoutes(api: Hono, adapter: StudioApiAdapter): void variables, distinctId: typeof body.telemetryDistinctId === "string" ? body.telemetryDistinctId : undefined, + telemetryOptOut: body.telemetryOptOut === true, }); (jobState as RenderJobState & { createdAt: number }).createdAt = Date.now(); renderJobs.set(jobId, jobState as RenderJobState & { createdAt: number }); diff --git a/packages/studio-server/src/types.ts b/packages/studio-server/src/types.ts index f9223ece0..07bb5cffc 100644 --- a/packages/studio-server/src/types.ts +++ b/packages/studio-server/src/types.ts @@ -144,6 +144,13 @@ export interface StudioApiAdapter { fps: import("@hyperframes/core").Fps; quality: string; jobId: string; + /** + * The triggering browser profile has telemetry disabled (localStorage + * opt-out, DNT, dev build...). The CLI cannot observe any of that, so the + * browser has to say so — without it the server emitted render outcomes + * for a user who had opted out, under the CLI's own policy. + */ + telemetryOptOut?: boolean; /** * Optional output resolution preset. See `resolveDeviceScaleFactor` in * the producer for the integer-scale + aspect + HDR constraints. diff --git a/packages/studio/src/components/renders/useRenderQueue.ts b/packages/studio/src/components/renders/useRenderQueue.ts index 7928950fc..aae5b5aca 100644 --- a/packages/studio/src/components/renders/useRenderQueue.ts +++ b/packages/studio/src/components/renders/useRenderQueue.ts @@ -2,6 +2,7 @@ import { useState, useEffect, useCallback, useRef, useMemo } from "react"; import type { CanvasResolution } from "@hyperframes/parsers"; import { trackStudioRenderStart } from "../../telemetry/events"; import { getAnonymousId } from "../../telemetry/config"; +import { browserTelemetryAllowed } from "../../telemetry/policy"; import { generateId } from "../../utils/generateId"; export interface RenderJob { @@ -157,16 +158,28 @@ export function useRenderQueue(projectId: string | null) { resolution?: string; composition?: string; variables?: Record; - telemetryDistinctId: string; + telemetryDistinctId?: string; + telemetryOptOut?: boolean; } = { fps, quality, format, + }; + // The id is MINTED by getAnonymousId(), so calling it unconditionally + // created a telemetry identity for a profile that had opted out — and + // then shipped it to the server. The server's own policy cannot see this + // browser's localStorage or DoNotTrack, so it has to be told: an + // explicit `telemetryOptOut` suppresses the render outcome, which + // omitting the id alone does NOT (an old client omits it too, and that + // falls back to the install id). + if (browserTelemetryAllowed()) { // So the server-emitted render_complete/render_error is attributed to // this browser user (same id studio_* events use), making the render // funnel joinable. Matches studio_render_start fired just above. - telemetryDistinctId: getAnonymousId(), - }; + body.telemetryDistinctId = getAnonymousId(); + } else { + body.telemetryOptOut = true; + } if (resolution && resolution !== "auto") body.resolution = resolution; if (composition) body.composition = composition; if (opts.variables && Object.keys(opts.variables).length > 0) { diff --git a/packages/studio/src/components/renders/useRenderQueueTelemetry.test.tsx b/packages/studio/src/components/renders/useRenderQueueTelemetry.test.tsx new file mode 100644 index 000000000..c1625ad90 --- /dev/null +++ b/packages/studio/src/components/renders/useRenderQueueTelemetry.test.tsx @@ -0,0 +1,104 @@ +// @vitest-environment happy-dom + +// The render request body is the only place the browser can tell the CLI that +// this profile opted out. The CLI's own policy cannot see localStorage or +// DoNotTrack in someone else's browser, so if the body says nothing, the +// server emits render_complete / render_error anyway — attributed to the +// install id rather than the user's, which is worse than attributing it +// correctly. + +import { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const policyState = { allowed: true }; +const mintCalls = vi.fn(() => "browser-user-123"); + +vi.mock("../../telemetry/policy", () => ({ + browserTelemetryAllowed: () => policyState.allowed, +})); +vi.mock("../../telemetry/config", () => ({ + getAnonymousId: () => mintCalls(), +})); +vi.mock("../../telemetry/events", () => ({ + trackStudioRenderStart: vi.fn(), +})); + +const { useRenderQueue } = await import("./useRenderQueue"); + +Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + +let root: Root | null = null; + +/** Body of the POST the hook makes when a render is started. */ +async function startRenderBody(): Promise> { + const fetchMock = vi.fn(async (_url: string, _init?: RequestInit) => + Promise.resolve( + new Response(JSON.stringify({ jobId: "j1", status: "rendering" }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ), + ); + vi.stubGlobal("fetch", fetchMock); + vi.stubGlobal( + "EventSource", + class { + close(): void {} + addEventListener(): void {} + }, + ); + + let api: ReturnType | null = null; + function Harness(): null { + api = useRenderQueue("demo"); + return null; + } + const host = document.createElement("div"); + document.body.append(host); + root = createRoot(host); + act(() => { + root?.render(); + }); + await act(async () => { + await api?.startRender({ fps: 30, quality: "standard", format: "mp4" }); + }); + + const post = fetchMock.mock.calls.find(([, init]) => init?.method === "POST"); + const body = post?.[1]?.body; + if (body === undefined || body === null) throw new Error("hook made no POST with a body"); + return JSON.parse(String(body)) as Record; +} + +beforeEach(() => { + policyState.allowed = true; + mintCalls.mockClear(); +}); + +afterEach(() => { + if (root) act(() => root?.unmount()); + root = null; + document.body.innerHTML = ""; + vi.unstubAllGlobals(); +}); + +describe("render request telemetry fields", () => { + it("sends the browser id when this profile allows telemetry", async () => { + const body = await startRenderBody(); + expect(body["telemetryDistinctId"]).toBe("browser-user-123"); + expect(body["telemetryOptOut"]).toBeUndefined(); + }); + + it("mints no id and says so explicitly when the profile opted out", async () => { + policyState.allowed = false; + const body = await startRenderBody(); + // Minting is itself the leak: getAnonymousId() CREATES and persists an + // identity, so calling it for an opted-out profile is wrong even if the + // value were never sent. + expect(mintCalls).not.toHaveBeenCalled(); + expect(body["telemetryDistinctId"]).toBeUndefined(); + // Omission alone would be read as an old client and fall back to the + // install id — the flag is what actually suppresses the server event. + expect(body["telemetryOptOut"]).toBe(true); + }); +}); diff --git a/scripts/check-canary-sunset.ts b/scripts/check-canary-sunset.ts new file mode 100644 index 000000000..6649c21d4 --- /dev/null +++ b/scripts/check-canary-sunset.ts @@ -0,0 +1,36 @@ +#!/usr/bin/env bun +/** + * Sunset governance for the canary registry. + * + * Every canary declares a `sunsetAfter` date. Without something that reads the + * CURRENT date, that field is a comment: a rollout can sit at 10% forever and + * nothing notices. The registry's own unit tests deliberately do NOT check it + * against wall-clock time, because a date-triggered failure there turns the + * whole core suite red on an unrelated PR whose author cannot fix it. + * + * So the check lives here and runs on a schedule instead. A failure names an + * expired rollout and belongs to its owner, not to whoever pushed that day. + * + * Run: `bun scripts/check-canary-sunset.ts` + */ +import { CANARIES, overdueCanaries } from "../packages/core/src/canaryRegistry.js"; + +const overdue = overdueCanaries(new Date()); + +if (overdue.length === 0) { + console.log(`✓ ${CANARIES.length} canary/canaries registered, none past sunset.`); + process.exit(0); +} + +console.error(`✗ ${overdue.length} canary/canaries are past their sunset date:\n`); +for (const name of overdue) { + const canary = CANARIES.find((c) => c.name === name); + console.error( + ` ${name} — sunset ${canary?.sunsetAfter ?? "?"}, ${canary?.percentage ?? "?"}%, owner ${canary?.owner ?? "?"}`, + ); +} +console.error( + "\nRamp it to 100 and delete the guard, roll it back, or move the date with a" + + "\nreason. See docs/contributing/canary-rollouts.mdx.", +); +process.exit(1);