mirror of
https://github.com/heygen-com/hyperframes.git
synced 2026-09-07 10:06:21 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3f69a2c635
commit
6f0df2640b
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<typeof trackRenderComplete>[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),
|
||||
|
||||
@@ -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() });
|
||||
});
|
||||
|
||||
|
||||
@@ -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<string>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string> {
|
||||
const names = new Set<string>();
|
||||
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) });
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user