From 0f93a62d719fd8d0eade219d59d87abdbb209f2f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:39:48 -0700 Subject: [PATCH 1/6] fix(orb): mint a dedicated federated signing key, separate from the anon secret (#9147) Enrolling a federated peer previously handed them this instance's dedicated anonymization secret (the SAME key that HMACs repo/pr identifiers on the always-on #1255 orb export), since buildFederatedBundle signed with getOrCreateAnonSecret. A peer holding that key could dictionary-invert any repo_hash it observed and forge this instance's x-orb-signature. - Add getOrCreateFederatedSigningSecret: a separate, dedicated system_flags secret used only to sign federated bundles. - Decouple instanceId from the anon secret via getOrCreateInstanceIdentitySecret, a third independent secret seeded from the current anon secret on first use (so an upgrading instance keeps its existing instanceId) but stored in its own row from then on, so rotating the anon secret no longer resets the export watermark or the federated peer-median's per-instance accounting. - Correct anonymize.ts's key-separation docstring, which was false for any federated-enabled instance. --- .../src/telemetry/anonymize.ts | 6 ++ src/orb/federated-bundle.ts | 81 ++++++++++++++----- src/selfhost/orb-collector.ts | 69 +++++++++++----- test/unit/federated-bundle.test.ts | 49 +++++++++-- 4 files changed, 160 insertions(+), 45 deletions(-) diff --git a/packages/loopover-engine/src/telemetry/anonymize.ts b/packages/loopover-engine/src/telemetry/anonymize.ts index 78786186b8..315958fc0e 100644 --- a/packages/loopover-engine/src/telemetry/anonymize.ts +++ b/packages/loopover-engine/src/telemetry/anonymize.ts @@ -14,6 +14,12 @@ import { createHmac, randomBytes } from "node:crypto"; * once per instance in its own store and reuses it on every export, so the same raw value always hashes the * same way. Never derived from, or shared with, any other credential (App private keys, webhook secrets) -- * key separation means a leaked anonymization secret can't be used to forge or decrypt anything else. + * + * #9147: this now also holds for Orb's opt-in federated peer-benchmark export (src/orb/federated-bundle.ts). + * That feature signs its bundles with its OWN dedicated secret (getOrCreateFederatedSigningSecret) rather + * than this one, and derives its instance identity from a third, independent secret + * (getOrCreateInstanceIdentitySecret) -- so enrolling a federated peer, or rotating any one of the three + * secrets, can never affect this anonymization secret or the always-on export that uses it. */ export function generateAnonSecret(): string { return randomBytes(32).toString("hex"); diff --git a/src/orb/federated-bundle.ts b/src/orb/federated-bundle.ts index 1cfeb36842..304efa38ff 100644 --- a/src/orb/federated-bundle.ts +++ b/src/orb/federated-bundle.ts @@ -3,7 +3,8 @@ // This is the EXPORT side only: it packages a subset of this instance's own local calibration data into a // signed, anonymized bundle an operator can choose to hand to a peer. It performs NO network call — the // transport (push/pull against an operator-configured collector) is #6479, the receiving/trust-gating side is -// #6480, and the key-trust scheme is #6477's design (see the TODO on the signing key below). +// #6480, and the key-trust scheme is #6477's design (see signFederatedBundle's own doc comment below for the +// dedicated signing key #9147 introduced). // // NOT the same thing as the #1255 orb export (src/selfhost/orb-collector.ts:155). That path is deliberately // distinct on five axes, and this module exists precisely because none of them can be retrofitted onto it: @@ -25,8 +26,8 @@ // #6481 renders "this instance's gate precision vs the peer median", so a bundle's mergePrecision must be // computed by the exact same confusion-matrix definition the fleet median uses, or the comparison is // apples-to-oranges. Same reason MIN_DECIDED gates the published precision here. -import { createHmac } from "node:crypto"; -import { bucketReasonCode, cycleTimeMs, getOrCreateAnonSecret, instanceId } from "../selfhost/orb-collector"; +import { createHmac, randomBytes } from "node:crypto"; +import { bucketReasonCode, cycleTimeMs, getOrCreateInstanceIdentitySecret, instanceId } from "../selfhost/orb-collector"; import { foldInstance, MIN_DECIDED, percentile, type Cell as FleetCell } from "./analytics"; import type { FocusManifest } from "../signals/focus-manifest"; @@ -35,9 +36,20 @@ import type { FocusManifest } from "../signals/focus-manifest"; export const FEDERATED_BUNDLE_SCHEMA_VERSION = 1; /** Default calibration window. Mirrors computeFleetAnalytics' 90-day default and 365-day clamp so a bundle's - * window is directly comparable to the fleet's. */ -const DEFAULT_WINDOW_DAYS = 90; -const MAX_WINDOW_DAYS = 365; + * window is directly comparable to the fleet's. Exported (#9148) so the import side can range-check a peer's + * `windowDays` and the benchmark side can compare a peer's window against this instance's own, without + * duplicating the clamp. */ +export const DEFAULT_WINDOW_DAYS = 90; +export const MAX_WINDOW_DAYS = 365; + +/** Resolve the effective window (days) from a caller-supplied override, exactly as {@link buildFederatedBundle} + * does internally — extracted so {@link buildFederatedBenchmark} (federated-benchmark.ts) can compute the + * SAME value to pass to the import side's window-match check (#9148) without forking the clamp logic. */ +export function resolveFederatedWindowDays(windowDaysOverride: number | undefined): number { + return Number.isFinite(windowDaysOverride) && (windowDaysOverride as number) > 0 + ? Math.min(windowDaysOverride as number, MAX_WINDOW_DAYS) + : DEFAULT_WINDOW_DAYS; +} /** * The signed payload of a federated calibration bundle: every field except the signature itself. @@ -163,16 +175,46 @@ export function canonicalizeFederatedBundleBody(body: FederatedSignalBundleBody) * HMAC-sign a bundle body so a receiving instance can verify it was not tampered with in transit. * * KEY-TRUST (#6477, ratified as an allowlist design — NOT a PKI/reputation system): the signing key is this - * instance's dedicated anonymization secret (getOrCreateAnonSecret), which makes the bundle tamper-evident to - * anyone holding that key. Peer trust itself is established on the RECEIVING side, where #6480 has SHIPPED - * (src/orb/federated-import.ts): an importing operator explicitly adds a peer's verification key to - * `federatedIntelligence.peerKeys` (fail-closed when unset), and only an allowlisted key can verify a bundle. - * So this signature is the tamper-evidence layer; the receiver's allowlist is what turns it into peer trust. + * instance's DEDICATED federated-signing secret (getOrCreateFederatedSigningSecret) — a symmetric HMAC key a + * peer must hold verbatim to verify a bundle. #9147: this is deliberately a SEPARATE secret from the always-on + * #1255 orb export's anonymization secret (getOrCreateAnonSecret in ../selfhost/orb-collector.ts). Handing a + * peer the federated signing key must never also hand them the key that de-anonymizes this instance's + * unrelated, always-on telemetry — the two secrets share no value and are rotatable independently. Peer trust + * itself is established on the RECEIVING side, where #6480 has SHIPPED (src/orb/federated-import.ts): an + * importing operator explicitly adds a peer's verification key to `federatedIntelligence.peerKeys` + * (fail-closed when unset), and only an allowlisted key can verify a bundle. So this signature is the + * tamper-evidence layer; the receiver's allowlist is what turns it into peer trust. */ export function signFederatedBundle(body: FederatedSignalBundleBody, key: string): string { return createHmac("sha256", key).update(canonicalizeFederatedBundleBody(body)).digest("hex"); } +/** system_flags key for the federated bundle's OWN signing secret (#9147) — never `orb:anon_secret`. */ +const FEDERATED_SIGNING_SECRET_FLAG = "orb:federated_signing_secret"; + +/** + * The instance's DEDICATED federated-signing secret (#9147): a 256-bit random key generated once and + * persisted in system_flags, distinct from every other secret this codebase mints (the App private key, the + * webhook-verification secret, and — the specific bug this closes — the always-on orb export's anonymization + * secret). An operator enrolling a peer only ever hands out THIS value, so a peer can never use it to + * de-anonymize the unrelated #1255 telemetry export, and rotating either secret never affects the other. + */ +export async function getOrCreateFederatedSigningSecret(db: D1Database): Promise { + const read = async (): Promise => { + const row = await db.prepare(`SELECT value FROM system_flags WHERE key = ?`).bind(FEDERATED_SIGNING_SECRET_FLAG).first<{ value: string }>(); + return row?.value; + }; + const existing = await read(); + if (existing) return existing; + const generated = randomBytes(32).toString("hex"); // 256-bit, 64 hex chars — same shape as generateAnonSecret + await db + .prepare(`INSERT OR IGNORE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)`) + .bind(FEDERATED_SIGNING_SECRET_FLAG, generated) + .run(); + /* v8 ignore next -- a row always exists after INSERT OR IGNORE, so the ?? fallback is unreachable */ + return (await read()) ?? generated; +} + /** Is the federated export opted in for this deployment? Absent block ⇒ false ⇒ byte-identical behavior. */ export function isFederatedIntelligenceEnabled(manifest: Pick | null | undefined): boolean { return manifest?.federatedIntelligence?.enabled === true; @@ -198,17 +240,16 @@ export async function buildFederatedBundle( if (!isFederatedIntelligenceEnabled(manifest)) return null; try { - const windowDays = - Number.isFinite(opts.windowDays) && (opts.windowDays as number) > 0 - ? Math.min(opts.windowDays as number, MAX_WINDOW_DAYS) - : DEFAULT_WINDOW_DAYS; + const windowDays = resolveFederatedWindowDays(opts.windowDays); const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); // Date-only cutoff, like computeFleetAnalytics — compares correctly whether created_at is ISO ('…T…Z') or // SQLite's CURRENT_TIMESTAMP space format ('YYYY-MM-DD HH:MM:SS'). const cutoff = new Date(now - windowDays * 86_400_000).toISOString().slice(0, 10); - const secret = await getOrCreateAnonSecret(db); - const instance = instanceId(secret); + // #9147: instanceId is derived from the dedicated IDENTITY secret (shared with the #1255 orb export, so + // both pipelines dedup the same instance under the same handle) — never from the anon secret or the + // federated signing secret below, so rotating either never changes this instance's identity. + const instance = instanceId(await getOrCreateInstanceIdentitySecret(db)); const { results } = await db.prepare(LOCAL_CALIBRATION_QUERY).bind(cutoff).all(); const rows = results ?? []; @@ -255,7 +296,11 @@ export async function buildFederatedBundle( copycatRate: bucketShare("duplicate_risk"), }; - return { ...body, signature: signFederatedBundle(body, secret) }; + // #9147: signed with the DEDICATED federated signing secret — never the anon secret above, and never + // shared with any other credential — so handing this key to a peer can only ever verify federated + // bundles, not de-anonymize the unrelated always-on orb export. + const signingSecret = await getOrCreateFederatedSigningSecret(db); + return { ...body, signature: signFederatedBundle(body, signingSecret) }; } catch (error) { console.error( JSON.stringify({ level: "error", event: "federated_bundle_failed", message: String(error).slice(0, 200) }), diff --git a/src/selfhost/orb-collector.ts b/src/selfhost/orb-collector.ts index 516b84731f..17846633f4 100644 --- a/src/selfhost/orb-collector.ts +++ b/src/selfhost/orb-collector.ts @@ -102,44 +102,69 @@ async function loadReuseCounters(db: D1Database, nowMs: number): Promise { +/** Read-or-generate-and-persist a single-purpose, per-instance 256-bit secret under `flagKey` in system_flags. + * Shared by every dedicated secret this file mints (the anonymization secret, the instance identity secret) so + * each stays its own independent value — never derived from, or reused as, another (#9147's key-separation + * fix). Race-safe across instances sharing a Postgres DB: OR IGNORE keeps the first writer's value; the + * re-read returns whichever value won, so every instance converges on the same secret. */ +async function getOrCreateSystemSecret(db: D1Database, flagKey: string, seed?: string): Promise { const read = async (): Promise => { - const row = await db - .prepare(`SELECT value FROM system_flags WHERE key = ?`) - .bind(ANON_SECRET_FLAG) - .first<{ value: string }>(); + const row = await db.prepare(`SELECT value FROM system_flags WHERE key = ?`).bind(flagKey).first<{ value: string }>(); return row?.value; }; const existing = await read(); if (existing) return existing; - const generated = generateAnonSecret(); // 256-bit, 64 hex chars - // Race-safe across instances sharing a Postgres DB: OR IGNORE keeps the first writer's key; the re-read - // returns whichever value won, so every instance converges on the same secret. + const generated = seed ?? generateAnonSecret(); // 256-bit, 64 hex chars await db .prepare(`INSERT OR IGNORE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)`) - .bind(ANON_SECRET_FLAG, generated) + .bind(flagKey, generated) .run(); /* v8 ignore next -- a row always exists after INSERT OR IGNORE, so the ?? fallback is unreachable */ return (await read()) ?? generated; } +/** + * The instance's DEDICATED anonymization secret: a 256-bit random key generated once and persisted in + * system_flags, then reused on every export. Stable across restarts so a repo/PR always hashes the same + * way (the collector can dedup), per-instance, and SINGLE-PURPOSE — never the App private key, the + * webhook-verification secret, or (since #9147) the federated bundle signing key (key separation). The + * collector never holds it, so it cannot de-anonymize. + */ +export async function getOrCreateAnonSecret(db: D1Database): Promise { + return getOrCreateSystemSecret(db, ANON_SECRET_FLAG); +} + +/** system_flags key for the instance identity secret (#9147) — deliberately distinct from ANON_SECRET_FLAG. */ +const INSTANCE_IDENTITY_SECRET_FLAG = "orb:instance_identity_secret"; + +/** + * The instance's DEDICATED identity secret (#9147): what `instanceId` hashes for a brokered instance with no + * App id, kept in its OWN system_flags row so rotating the anonymization secret can never change a brokered + * instance's identity (and therefore never resets the export watermark or the federated peer-median's + * per-instance accounting keyed on it). On FIRST read (no row yet — every instance that upgraded from before + * this secret existed, or a never-yet-exported instance), it seeds from the CURRENT anon secret: an + * already-live instance keeps the exact instanceId it already has (no disruptive identity change on deploy), + * and a brand-new instance's anon secret is just as freshly generated, so reusing it here costs it nothing. + * From that point on the two secrets are independent rows and can be rotated separately. + */ +export async function getOrCreateInstanceIdentitySecret(db: D1Database): Promise { + const anonSecret = await getOrCreateAnonSecret(db); + return getOrCreateSystemSecret(db, INSTANCE_IDENTITY_SECRET_FLAG, anonSecret); +} + /** Map the gate's free-text reasonCode to a fixed, low-cardinality category — done at the source so the raw * (possibly repo-specific) reason string never leaves the instance. */ export function bucketReasonCode(summary: string | null | undefined): string { @@ -233,7 +258,9 @@ export async function exportOrbBatch(db: D1Database, batchSize = 200, fetchFn: t const collectorUrl = process.env.ORB_COLLECTOR_URL ?? "https://api.loopover.ai/v1/orb/ingest"; const secret = await getOrCreateAnonSecret(db); const anonymize = (process.env.ORB_ANONYMIZE ?? "true").toLowerCase() !== "false"; - const instance = instanceId(secret); + // #9147: instanceId is derived from the DEDICATED identity secret, never the anon secret above — so + // rotating ORB_ANONYMIZE's key can never change this instance's identity or reset its export watermark. + const instance = instanceId(await getOrCreateInstanceIdentitySecret(db)); // Read this instance's export watermark (resumes where the last run left off). const cursorRow = await db diff --git a/test/unit/federated-bundle.test.ts b/test/unit/federated-bundle.test.ts index 7dae9e4516..b1f2543b04 100644 --- a/test/unit/federated-bundle.test.ts +++ b/test/unit/federated-bundle.test.ts @@ -2,11 +2,12 @@ import { DatabaseSync } from "node:sqlite"; import { createHmac } from "node:crypto"; import { describe, expect, it, vi } from "vitest"; import { createD1Adapter, nodeSqliteDriver } from "../../src/selfhost/d1-adapter"; -import { getOrCreateAnonSecret, instanceId } from "../../src/selfhost/orb-collector"; +import { getOrCreateAnonSecret, getOrCreateInstanceIdentitySecret, instanceId } from "../../src/selfhost/orb-collector"; import { FEDERATED_BUNDLE_SCHEMA_VERSION, buildFederatedBundle, canonicalizeFederatedBundleBody, + getOrCreateFederatedSigningSecret, isFederatedIntelligenceEnabled, signFederatedBundle, type FederatedSignalBundle, @@ -153,13 +154,15 @@ describe("buildFederatedBundle() — opted in", () => { expect(b.cycleP50Ms).toBe(7_200_000); // 2h decision → outcome expect(b.cycleP95Ms).toBe(7_200_000); - // The opaque handle is the orb pipeline's, not a second identity. - expect(b.instanceId).toBe(instanceId(await getOrCreateAnonSecret(db))); + // The opaque handle is the orb pipeline's, not a second identity — but (#9147) derived from the DEDICATED + // identity secret, not the anon secret. + expect(b.instanceId).toBe(instanceId(await getOrCreateInstanceIdentitySecret(db))); - // Independently recompute the HMAC over the canonical body — the signature must verify. + // Independently recompute the HMAC over the canonical body — the signature must verify against the + // DEDICATED federated signing secret (#9147), never the anon secret. const { signature, ...body } = b; - const secret = await getOrCreateAnonSecret(db); - expect(signature).toBe(createHmac("sha256", secret).update(canonicalizeFederatedBundleBody(body)).digest("hex")); + const signingSecret = await getOrCreateFederatedSigningSecret(db); + expect(signature).toBe(createHmac("sha256", signingSecret).update(canonicalizeFederatedBundleBody(body)).digest("hex")); expect(signature).toMatch(/^[0-9a-f]{64}$/); // The export path itself never talks to anyone — transport is #6479. @@ -312,6 +315,40 @@ describe("buildFederatedBundle() — fail-safe", () => { }); }); +describe("#9147: federated signing key / anon secret separation", () => { + it("mints a federated signing secret that is never the same value as the anon secret", async () => { + const db = makeDb(); + const anonSecret = await getOrCreateAnonSecret(db); + const signingSecret = await getOrCreateFederatedSigningSecret(db); + expect(signingSecret).not.toBe(anonSecret); + expect(signingSecret).toMatch(/^[0-9a-f]{64}$/); + // Stable across repeated reads, exactly like getOrCreateAnonSecret. + expect(await getOrCreateFederatedSigningSecret(db)).toBe(signingSecret); + }); + + it("keeps instanceId stable when the anon secret is rotated after the identity secret already exists", async () => { + const db = makeDb(); + await resolved(db, 1); + const before = (await buildFederatedBundle(manifest(true), db, { now: NOW })) as FederatedSignalBundle; + + // Simulate an operator rotating the anonymization secret (e.g. after a suspected leak) — this must never + // change the instance's federated identity, since that would break the receiver's per-instance dedup. + await db.prepare("UPDATE system_flags SET value = ? WHERE key = 'orb:anon_secret'").bind("f".repeat(64)).run(); + expect(await getOrCreateAnonSecret(db)).toBe("f".repeat(64)); + + const after = (await buildFederatedBundle(manifest(true), db, { now: NOW })) as FederatedSignalBundle; + expect(after.instanceId).toBe(before.instanceId); + }); + + it("seeds the identity secret from the CURRENT anon secret on first use (no disruptive instanceId change on upgrade)", async () => { + const db = makeDb(); + const anonSecret = await getOrCreateAnonSecret(db); + // No identity-secret row exists yet — getOrCreateInstanceIdentitySecret must seed from the anon secret so + // an instance upgrading from before #9147 keeps its existing instanceId. + expect(await getOrCreateInstanceIdentitySecret(db)).toBe(anonSecret); + }); +}); + describe("canonicalizeFederatedBundleBody() / signFederatedBundle()", () => { it("is insensitive to key insertion order, so a receiver can recompute the HMAC byte-for-byte", async () => { const db = makeDb(); From 89df3b185d82e8b84daa14044c654a684e9b67cd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:47:44 -0700 Subject: [PATCH 2/6] fix(orb): default-engage the federated rate limiter and drop stale ingest comments (#9166) The documented per-collector rate limit (6/min) was inert at every real call site: rateLimitAllows treated a caller-omitted bucket as "unlimited" instead of falling back to a real bucket, and the only production caller (buildFederatedBenchmark) never supplied one. evaluateLocalRateLimit is also pure (read-only), so nothing was ever advancing a bucket's count either. - Add module-level default push/pull buckets in federated-collector.ts; rateLimitAllows now always consults (and advances) a real bucket, whether caller-supplied or the module default. - Bound pullPeerBundles' response read with the same reader /v1/orb/ingest uses (generalized readOrbIngestBody to accept any body-bearing source, so it works for both a Request and a collector Response), and cap the parsed array before any per-element shape check. - Remove two stale "FAIL-OPEN by default" comment blocks in routes.ts left over from #9046, which made isAuthorizedIngest's ingest gate (now fail-closed) read backwards from what the code actually does. pushFederatedBundle wiring into a real tick lands with the #9148 commit, which introduces the background refresh job this naturally slots into. --- src/api/routes.ts | 11 +-- src/orb/federated-collector.ts | 67 +++++++++++++--- src/orb/ingest.ts | 18 +++-- test/unit/federated-collector.test.ts | 107 ++++++++++++++++++++++---- 4 files changed, 168 insertions(+), 35 deletions(-) diff --git a/src/api/routes.ts b/src/api/routes.ts index 98c5a862f6..85d6425a79 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -4448,9 +4448,10 @@ export function createApp() { // LoopOver Orb (#1255) — central fleet-calibration collector. Receives anonymized, reversal-aware outcome // batches from self-hosted instances. Sender-side HMAC anonymization is for privacy, not authentication. - // OPTIONAL shared-token gate (#1285): unset ⇒ OPEN ingress (the live fleet keeps working, as before); set - // ⇒ the collector REQUIRES it, so an operator can lock the write path down after distributing the matching - // ORB_COLLECTOR_TOKEN to exporters. Bounded by a hard body ceiling, and dedup'd via UNIQUE(instance_id, repo_hash, pr_hash). + // #9046/#9166: FAILS CLOSED when ORB_INGEST_TOKEN is unset — isAuthorizedIngest (below, shared with + // /v1/ams/ingest) requires an exact bearer match against the configured token; an unconfigured collector + // rejects rather than accepting anonymous writes. Bounded by a hard body ceiling, and dedup'd via + // UNIQUE(instance_id, repo_hash, pr_hash). app.post("/v1/orb/ingest", async (c) => { if (!(await isAuthorizedIngest(c.env.ORB_INGEST_TOKEN, extractBearerToken(c.req.header("authorization"))))) return c.json({ error: "unauthorized" }, 401); const body = await readOrbIngestBody(c.req.raw, c.req.header("content-length")); @@ -6690,10 +6691,6 @@ function toIsoQueryDate(value: string): string | undefined { } -// Optional Orb-ingest auth (#1285). FAIL-OPEN by default: with no ORB_INGEST_TOKEN configured the ingress stays -// OPEN (matching today's live fleet — deploying this is non-breaking). Once the operator sets the token, the -// collector REQUIRES an exact bearer match, so the write path can be locked down after the matching -// ORB_COLLECTOR_TOKEN is rolled out to exporters. /** * #9046: the SINGLE authorization rule for every telemetry-collector ingest endpoint (Orb and AMS), so the two * products cannot drift apart again. They previously had separate, near-identical copies — and AMS ended up diff --git a/src/orb/federated-collector.ts b/src/orb/federated-collector.ts index 1ba1ffbe67..e3406cef05 100644 --- a/src/orb/federated-collector.ts +++ b/src/orb/federated-collector.ts @@ -28,6 +28,7 @@ import { } from "@loopover/engine"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; import { buildFederatedBundle, FEDERATED_BUNDLE_SCHEMA_VERSION, type FederatedSignalBundle } from "./federated-bundle"; +import { readOrbIngestBody } from "./ingest"; import type { FocusManifest } from "../signals/focus-manifest"; /** Matches every other outbound call in this subsystem (orb-collector.ts:215's 30s export tick). */ @@ -38,6 +39,10 @@ const DEFAULT_MAX_ATTEMPTS = 3; const RETRY_BASE_MS = 500; /** A best-effort background sync has no business hammering a peer's collector. */ const RATE_LIMIT: { limit: number; windowMs: number } = { limit: 6, windowMs: 60_000 }; +/** #9148: cap on how many pulled entries are even shape-checked, independent of the byte ceiling below — a + * hostile/misconfigured collector can't make this instance do unbounded per-element work just by staying + * under the byte cap with many tiny objects. Comfortably above any realistic peer count for this feature. */ +const MAX_PULLED_BUNDLES = 200; type ManifestSlice = Pick | null | undefined; @@ -50,11 +55,33 @@ export type CollectorOpts = { sleepFn?: (ms: number) => Promise; /** Injected random for the jitter — jitteredBackoffMs never reads Math.random itself. */ randomFn?: () => number; - /** Caller-owned rolling-window bucket. Omitted ⇒ no local rate limiting is applied. */ + /** Caller-owned rolling-window bucket. Omitted ⇒ falls back to this module's own default bucket for the + * given direction (#9166) — the limiter is never opt-in, so a caller can't accidentally forget to pass + * one and lose rate limiting entirely (which is exactly what happened before: the only production caller, + * buildFederatedBenchmark's routes.ts call site, never supplied a bucket at all). Tests may still inject + * their own bucket for isolation from the module-level default. */ bucket?: LocalRateBucket; now?: number; }; +/** #9166: module-level default buckets, one per direction, so the rate limit engages even when a caller + * supplies no `opts.bucket` of its own. `evaluateLocalRateLimit` is pure (it only reads a bucket, never + * advances it) — {@link rateLimitAllows} is what mutates these in place after an allowed attempt. */ +const defaultRateLimitBuckets: Record<"push" | "pull", LocalRateBucket> = { + push: { count: 0, windowStartMs: 0 }, + pull: { count: 0, windowStartMs: 0 }, +}; + +/** TEST-ONLY: reset the module-level default buckets between test cases. Production code never calls this — + * the whole point of the default is that it persists for the life of the isolate. Exported purely so tests in + * the same file (which otherwise share this module-level state) stay isolated from each other. */ +export function __resetFederatedCollectorRateLimitForTest(): void { + defaultRateLimitBuckets.push.count = 0; + defaultRateLimitBuckets.push.windowStartMs = 0; + defaultRateLimitBuckets.pull.count = 0; + defaultRateLimitBuckets.pull.windowStartMs = 0; +} + /** * The collector endpoint armed for `direction`, or null when this instance must not talk to anyone: not opted * in, no collector configured, the configured URL failed the SSRF guard at parse time, or the operator scoped @@ -74,10 +101,20 @@ export function resolveCollectorEndpoint(manifest: ManifestSlice, direction: "pu return url; } -/** True when the caller's bucket still permits an attempt. No bucket ⇒ unlimited. */ -function rateLimitAllows(opts: CollectorOpts, now: number): boolean { - if (!opts.bucket) return true; - return evaluateLocalRateLimit(opts.bucket, RATE_LIMIT, now).allowed; +/** True when the bucket for `direction` still permits an attempt at `now` — and, when it does, ADVANCES that + * bucket's count/window in place (#9166: evaluateLocalRateLimit itself never mutates its input, so this is + * the one place that turns an allowed decision into actual consumption). A caller-supplied `opts.bucket` + * always wins (test isolation); otherwise this module's own per-direction default bucket is used, so the + * limit is enforced even when nobody passes one in. */ +function rateLimitAllows(direction: "push" | "pull", opts: CollectorOpts, now: number): boolean { + const bucket = opts.bucket ?? defaultRateLimitBuckets[direction]; + const decision = evaluateLocalRateLimit(bucket, RATE_LIMIT, now); + if (decision.allowed) { + const windowElapsed = now - bucket.windowStartMs >= RATE_LIMIT.windowMs; + bucket.count = (windowElapsed ? 0 : bucket.count) + 1; + if (windowElapsed) bucket.windowStartMs = now; + } + return decision.allowed; } /** A 4xx is the operator's own misconfiguration and will fail identically on a retry; only 5xx/network is @@ -140,7 +177,7 @@ export async function pushFederatedBundle(manifest: ManifestSlice, db: D1Databas try { const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); - if (!rateLimitAllows(opts, now)) return false; + if (!rateLimitAllows("push", opts, now)) return false; const bundle = await buildFederatedBundle(manifest, db, opts.now === undefined ? {} : { now: opts.now }); // The builder already fails safe to null; nothing to send is not a failure worth retrying. @@ -168,6 +205,11 @@ export async function pushFederatedBundle(manifest: ManifestSlice, db: D1Databas * signature-verified or trust-gated — that is #6480's job, shipped in ./federated-import.ts (the peerKeys * allowlist). Returns [] rather than throwing on any failure, so an unreachable or hostile collector is * indistinguishable from "no peers yet" to every caller. + * + * #9148: the response body is read through the SAME bounded reader the /v1/orb/ingest path uses + * (readOrbIngestBody — both a Request and a Response satisfy its structural input type), so a hostile or + * misconfigured collector can't make this instance buffer an unbounded response; the parsed array is then + * capped at MAX_PULLED_BUNDLES before any per-element shape check runs. */ export async function pullPeerBundles(manifest: ManifestSlice, opts: CollectorOpts = {}): Promise { const endpoint = resolveCollectorEndpoint(manifest, "pull"); @@ -175,14 +217,21 @@ export async function pullPeerBundles(manifest: ManifestSlice, opts: CollectorOp try { const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); - if (!rateLimitAllows(opts, now)) return []; + if (!rateLimitAllows("pull", opts, now)) return []; const response = await fetchWithRetry(endpoint, { method: "GET", headers: { accept: "application/json" } }, opts); if (response === null) return []; - const payload: unknown = await response.json(); + const text = await readOrbIngestBody(response, response.headers.get("content-length")); + if (!text) return []; + let payload: unknown; + try { + payload = JSON.parse(text); + } catch { + return []; + } if (!Array.isArray(payload)) return []; - return payload.filter(isBundleShaped); + return payload.slice(0, MAX_PULLED_BUNDLES).filter(isBundleShaped); } catch (error) { console.error( JSON.stringify({ level: "error", event: "federated_pull_failed", message: String(error).slice(0, 200) }), diff --git a/src/orb/ingest.ts b/src/orb/ingest.ts index 1764970394..266b4a5bf1 100644 --- a/src/orb/ingest.ts +++ b/src/orb/ingest.ts @@ -26,17 +26,23 @@ function parseContentLength(header: string | null | undefined): number | null { return Number.isInteger(n) && n >= 0 ? n : null; } -/** Read the request body with a hard byte ceiling so a hostile sender can't make us buffer unbounded +/** The minimal shape {@link readOrbIngestBody} needs from its input: a byte stream. Both `Request` and + * `Response` satisfy this structurally (the Fetch API gives both a `.body: ReadableStream | + * null`), so this reader works unmodified over either — #9148 reuses it for a collector `Response` body + * (src/orb/federated-collector.ts's pullPeerBundles) instead of writing a second bounded reader. */ +export type BoundedBodySource = { body: ReadableStream | null }; + +/** Read a request/response body with a hard byte ceiling so a hostile sender can't make us buffer unbounded * input. Returns null when the body exceeds MAX_ORB_INGEST_BODY_BYTES OR when the underlying stream * itself errors (a dropped connection / network reset mid-read, mirrors readOrbRelayRegisterBody in - * ../orb/relay.ts) — both callers (/v1/orb/ingest, /v1/ams/ingest) already treat null identically to - * "reject this request", so a transient read failure degrades the same way an oversized payload does, - * instead of throwing UNCAUGHT out of this function as a bare framework 500. */ -export async function readOrbIngestBody(request: Request, contentLengthHeader: string | null | undefined): Promise { + * ../orb/relay.ts) — every caller (/v1/orb/ingest, /v1/ams/ingest, and #9148's federated-collector pull) + * already treats null identically to "reject this", so a transient read failure degrades the same way an + * oversized payload does, instead of throwing UNCAUGHT out of this function as a bare framework 500. */ +export async function readOrbIngestBody(source: BoundedBodySource, contentLengthHeader: string | null | undefined): Promise { const declared = parseContentLength(contentLengthHeader); if (declared !== null && declared > MAX_ORB_INGEST_BODY_BYTES) return null; - const stream = request.body; + const stream = source.body; if (!stream) return ""; const reader = stream.getReader(); const decoder = new TextDecoder(); diff --git a/test/unit/federated-collector.test.ts b/test/unit/federated-collector.test.ts index 80b443b8fb..5f801985e3 100644 --- a/test/unit/federated-collector.test.ts +++ b/test/unit/federated-collector.test.ts @@ -1,8 +1,9 @@ import { DatabaseSync } from "node:sqlite"; -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createD1Adapter, nodeSqliteDriver } from "../../src/selfhost/d1-adapter"; import { FEDERATED_BUNDLE_SCHEMA_VERSION, type FederatedSignalBundle } from "../../src/orb/federated-bundle"; import { + __resetFederatedCollectorRateLimitForTest, pullPeerBundles, pushFederatedBundle, resolveCollectorEndpoint, @@ -10,6 +11,13 @@ import { } from "../../src/orb/federated-collector"; import type { FederatedCollectorMode, FocusManifest } from "../../src/signals/focus-manifest"; +// #9166: pullPeerBundles/pushFederatedBundle now fall back to a module-level default rate-limit bucket when +// no `opts.bucket` is supplied. Reset it between tests so the many call sites in this file that omit a +// bucket (deliberately, to exercise the "no bucket" default arm) don't leak count across test cases. +afterEach(() => { + __resetFederatedCollectorRateLimitForTest(); +}); + const URL_OK = "https://collector.example.org/v1/federated"; /** In-memory DB with the tables the bundle builder reads (mirrors federated-bundle.test.ts's makeDb). */ @@ -104,8 +112,12 @@ function bundle(over: Partial = {}): Record= 200 && status < 300, status, json: async () => body } as unknown as Response; + return new Response(JSON.stringify(body), { status, headers: { "content-type": "application/json" } }); } describe("resolveCollectorEndpoint()", () => { @@ -229,18 +241,8 @@ describe("push/pull — failure handling never reaches the gate", () => { }); it("returns [] when the response body is not JSON at all", async () => { - const err = vi.spyOn(console, "error").mockImplementation(() => {}); - const fetchFn = (async () => - ({ - ok: true, - status: 200, - json: async () => { - throw new SyntaxError("Unexpected token < in JSON"); - }, - }) as unknown as Response) as unknown as typeof fetch; + const fetchFn = (async () => new Response("not json", { status: 200 })) as unknown as typeof fetch; await expect(pullPeerBundles(manifest(), { ...DETERMINISTIC, fetchFn })).resolves.toEqual([]); - expect(err).toHaveBeenCalled(); - err.mockRestore(); }); }); @@ -278,6 +280,21 @@ describe("pullPeerBundles()", () => { expect(got[0]!.signature).toBe("f".repeat(64)); }); + it("#9148: caps the number of pulled entries even shape-checked, independent of the byte ceiling", async () => { + const many = Array.from({ length: 250 }, (_, i) => bundle({ instanceId: `inst-${i}` })); + const fetchFn = (async () => jsonResponse(many)) as unknown as typeof fetch; + const got = await pullPeerBundles(manifest(), { ...DETERMINISTIC, fetchFn }); + expect(got).toHaveLength(200); // MAX_PULLED_BUNDLES, not the 250 the collector claimed to have + expect(got[0]!.instanceId).toBe("inst-0"); + expect(got[199]!.instanceId).toBe("inst-199"); + }); + + it("#9148: rejects a response over the ingest body's byte ceiling instead of buffering it", async () => { + const oversized = JSON.stringify([bundle()]).padEnd(2 * 1_048_576, " "); + const fetchFn = (async () => new Response(oversized, { status: 200 })) as unknown as typeof fetch; + expect(await pullPeerBundles(manifest(), { ...DETERMINISTIC, fetchFn })).toEqual([]); + }); + it("returns [] for a non-array payload", async () => { const fetchFn = (async () => jsonResponse({ bundles: [bundle()] })) as unknown as typeof fetch; expect(await pullPeerBundles(manifest(), { ...DETERMINISTIC, fetchFn })).toEqual([]); @@ -302,6 +319,70 @@ describe("rate limiting — a best-effort sync must not hammer a peer", () => { expect(await pushFederatedBundle(manifest(), db, { ...DETERMINISTIC, fetchFn, bucket: fresh })).toBe(true); expect(await pullPeerBundles(manifest(), { ...DETERMINISTIC, fetchFn })).toEqual([]); }); + + // #9166: the rate limiter was inert at every real call site because the only production caller + // (buildFederatedBenchmark) never supplied a bucket, and `!opts.bucket` meant "unlimited". These prove the + // module-level default actually engages and blocks once exhausted, with no caller-supplied bucket at all. + describe("#9166: the default bucket (no opts.bucket supplied) actually engages", () => { + it("blocks pull after the limit within one window, using only the module-level default", async () => { + const fetchFn = (async () => jsonResponse([])) as unknown as typeof fetch; + let calls = 0; + const countingFetch = (async (...args: Parameters) => { + calls += 1; + return fetchFn(...args); + }) as unknown as typeof fetch; + for (let i = 0; i < 6; i++) { + expect(await pullPeerBundles(manifest(), { now: NOW, fetchFn: countingFetch })).toEqual([]); + } + expect(calls).toBe(6); // all 6 within the limit went through + // The 7th, same window, same instant — no bucket supplied by the caller — must now be blocked. + expect(await pullPeerBundles(manifest(), { now: NOW, fetchFn: countingFetch })).toEqual([]); + expect(calls).toBe(6); // the 7th never reached fetch at all + }); + + it("blocks push after the limit within one window, using only the module-level default", async () => { + const db = makeDb(); + await resolved(db, 1); + let calls = 0; + const countingFetch = (async () => { + calls += 1; + return jsonResponse({ ok: true }); + }) as unknown as typeof fetch; + for (let i = 0; i < 6; i++) { + expect(await pushFederatedBundle(manifest(), db, { now: NOW, fetchFn: countingFetch })).toBe(true); + } + expect(calls).toBe(6); + expect(await pushFederatedBundle(manifest(), db, { now: NOW, fetchFn: countingFetch })).toBe(false); + expect(calls).toBe(6); + }); + + it("push and pull are independent default buckets — exhausting one never blocks the other", async () => { + const db = makeDb(); + await resolved(db, 1); + const fetchFn = (async () => jsonResponse({ ok: true })) as unknown as typeof fetch; + for (let i = 0; i < 6; i++) expect(await pushFederatedBundle(manifest(), db, { now: NOW, fetchFn })).toBe(true); + expect(await pushFederatedBundle(manifest(), db, { now: NOW, fetchFn })).toBe(false); // push exhausted + + const pullFetch = (async () => jsonResponse([])) as unknown as typeof fetch; + expect(await pullPeerBundles(manifest(), { now: NOW, fetchFn: pullFetch })).toEqual([]); // pull untouched + }); + + it("resets after the window elapses", async () => { + const fetchFn = (async () => jsonResponse([])) as unknown as typeof fetch; + for (let i = 0; i < 6; i++) expect(await pullPeerBundles(manifest(), { now: NOW, fetchFn })).toEqual([]); + // Same instant again — still exhausted. + let calls = 0; + const countingFetch = (async () => { + calls += 1; + return jsonResponse([]); + }) as unknown as typeof fetch; + expect(await pullPeerBundles(manifest(), { now: NOW, fetchFn: countingFetch })).toEqual([]); + expect(calls).toBe(0); + // A full window later, the bucket resets and the request goes through again. + expect(await pullPeerBundles(manifest(), { now: NOW + 61_000, fetchFn: countingFetch })).toEqual([]); + expect(calls).toBe(1); + }); + }); }); describe("defaults", () => { From 49a45bfd1d67ea367e797f8d02d51a87ae966f79 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:26:34 -0700 Subject: [PATCH 3/6] fix(orb): scope pull-mode relay drain to the winning enrollment (#9150) orb_relay_pending was scoped only by installation_id, so any enrollment secret valid for an installation could drain and destructively-ack another live enrollment's queued webhooks -- a stale re-enrolled container silently stealing a fresh one's events (blue/green swap, secret rotation, or a rebuilt container issued a fresh secret while the old one is still inside its drain timer). - Add orb_relay_pending.enroll_id, tagged at enqueue time with the SAME winning enrollment forwardOrbEvent already elects for the push path (#1783's deterministic ORDER BY). NULL for config_push rows (installation- wide, not consumer-specific) and for any pre-#9150 row, so an untagged row is never orphaned -- pullRelayPending's WHERE matches enroll_id = ? OR enroll_id IS NULL. - forwardOrbEvent now fetches every live enrollment row for an installation (not just the elected winner) so a second live enrollment is observable rather than silent, and increments a new counter, loopover_orb_relay_multiple_live_enrollments_total, whenever more than one exists. - pullRelayPending/enqueueRelayPending both accept an optional enrollId to scope the SELECT and the ack-DELETE together, closing the hole where one container could ack another's rows. --- .../0191_orb_relay_pending_enroll_id.sql | 11 ++++ src/api/routes.ts | 4 +- src/db/schema.ts | 7 +++ src/orb/relay.ts | 57 ++++++++++++++----- test/integration/orb-relay.test.ts | 56 ++++++++++++++++++ 5 files changed, 120 insertions(+), 15 deletions(-) create mode 100644 migrations/0191_orb_relay_pending_enroll_id.sql diff --git a/migrations/0191_orb_relay_pending_enroll_id.sql b/migrations/0191_orb_relay_pending_enroll_id.sql new file mode 100644 index 0000000000..a1afb43fa9 --- /dev/null +++ b/migrations/0191_orb_relay_pending_enroll_id.sql @@ -0,0 +1,11 @@ +-- Pull-mode drain isolation (#9150): orb_relay_pending was scoped only by installation_id, so ANY enrollment +-- secret valid for an installation could drain and destructively-ack another live consumer's queued webhooks +-- (a stale re-enrolled container silently stealing a fresh one's events). This column lets an enqueue tag +-- which enrollment a GitHub-webhook row was queued for (the same "winning enrollment" forwardOrbEvent already +-- picks for the push path, #1783); NULL for every existing row and for config_push notices (which remain +-- intentionally installation-wide, not consumer-specific -- see enqueueConfigPushRelay). NULL also means "not +-- yet tagged", so pullRelayPending's WHERE clause matches untagged rows regardless of which enrollment asks -- +-- a safe default during rollout, not a bypass (an enrollment can only ever add matches, never lose ones scoped +-- to a DIFFERENT enroll_id). +ALTER TABLE orb_relay_pending ADD COLUMN enroll_id TEXT; +CREATE INDEX IF NOT EXISTS idx_orb_relay_pending_enroll ON orb_relay_pending (installation_id, enroll_id); diff --git a/src/api/routes.ts b/src/api/routes.ts index 85d6425a79..04dfdf5e97 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -4441,7 +4441,9 @@ export function createApp() { } catch { ack = undefined; // tolerate an empty/invalid body — just no ack this round } - const events = await pullRelayPending(c.env, enrollment.installationId, { ack }).catch(dbBrokerError); + // #9150: scope the drain to THIS enrollment (or untagged/legacy rows) — not merely the installation — so a + // second live enrollment for the same install can never steal and destructively-ack this one's events. + const events = await pullRelayPending(c.env, enrollment.installationId, { ack, enrollId: enrollment.enrollId }).catch(dbBrokerError); if (events === null) return c.json({ error: "broker_error" }, 503); return c.json({ events }, 200); }); diff --git a/src/db/schema.ts b/src/db/schema.ts index 805e29dfd0..b75ce4fc3d 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -977,6 +977,11 @@ export const orbRelayPending = sqliteTable( // enqueueConfigPushRelay). The dispatch side (#7523) branches on this before treating raw_body as a // GitHubWebhookPayload. kind: text("kind").notNull().default("github_webhook"), + // Pull-mode drain isolation (#9150): which enrollment a GitHub-webhook row was queued for (the same + // "winning enrollment" forwardOrbEvent picks for the push path). NULL for config_push rows (installation- + // wide, not consumer-specific) and for any pre-#9150 row -- pullRelayPending's WHERE matches NULL + // regardless of the asking enrollment, so an untagged row is never silently orphaned. + enrollId: text("enroll_id"), }, (table) => ({ installation: index("idx_orb_relay_pending_install").on(table.installationId, table.createdAt), @@ -986,6 +991,8 @@ export const orbRelayPending = sqliteTable( // pruneRelayPending (src/orb/relay.ts) filters/deletes by created_at alone (fleet-wide TTL sweep, not // scoped to one installation) -- neither index above leads with created_at, so that scan was unindexed. createdAt: index("idx_orb_relay_pending_created_at").on(table.createdAt), + // #9150: pullRelayPending's SELECT/DELETE now filter on (installation_id, enroll_id) together. + enroll: index("idx_orb_relay_pending_enroll").on(table.installationId, table.enrollId), }), ); diff --git a/src/orb/relay.ts b/src/orb/relay.ts index 924b63e7f0..8c48d9c2ff 100644 --- a/src/orb/relay.ts +++ b/src/orb/relay.ts @@ -9,6 +9,7 @@ import { githubWebhookCoalesceKey } from "../github/webhook-coalesce"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; import type { GitHubWebhookPayload } from "../types"; import { decryptSecret, encryptSecret } from "../utils/crypto"; +import { incr } from "../selfhost/metrics"; // The events a brokered container needs to review/act on. Installation-lifecycle + other Orb-internal events are // deliberately NOT forwarded (the container runs under the CENTRAL Orb App, not its own, so it must not treat @@ -322,18 +323,24 @@ export async function pruneRelayPending(env: Env): Promise { /** Enqueue a pull-mode event for an installation. Idempotent on delivery_id (mirrors storeRelayFailure) — a GitHub * redelivery reaching the Orb twice never double-queues. Prunes expired rows and caps each install's backlog first so - * an offline/malicious pull enrollment cannot retain raw webhook bodies indefinitely. */ + * an offline/malicious pull enrollment cannot retain raw webhook bodies indefinitely. + * + * `enrollId` (#9150) tags the row with the specific enrollment forwardOrbEvent picked as the current winner + * for this installation, so pullRelayPending can later scope its drain to that same enrollment instead of + * "any enrollment secret valid for this installation" — closing the hole where a stale re-enrolled container + * could drain and destructively-ack a live one's queue. Optional so config_push (installation-wide, not + * consumer-specific) can keep enqueueing with no enrollment context at all. */ export async function enqueueRelayPending( env: Env, - args: { deliveryId: string; installationId: number; eventName: string; rawBody: string }, + args: { deliveryId: string; installationId: number; eventName: string; rawBody: string; enrollId?: string | null }, ): Promise { await pruneRelayPending(env); const coalesceKey = relayPendingCoalesceKey(args.eventName, args.rawBody); const inserted = await env.DB .prepare( - "INSERT INTO orb_relay_pending (delivery_id, installation_id, event_name, raw_body, coalesce_key) VALUES (?, ?, ?, ?, ?) ON CONFLICT(delivery_id) DO NOTHING", + "INSERT INTO orb_relay_pending (delivery_id, installation_id, event_name, raw_body, coalesce_key, enroll_id) VALUES (?, ?, ?, ?, ?, ?) ON CONFLICT(delivery_id) DO NOTHING", ) - .bind(args.deliveryId, args.installationId, args.eventName, args.rawBody, coalesceKey) + .bind(args.deliveryId, args.installationId, args.eventName, args.rawBody, coalesceKey, args.enrollId ?? null) .run(); if (coalesceKey && inserted.meta.changes > 0) { await env.DB @@ -418,21 +425,31 @@ function relayPendingCoalesceKey(eventName: string, rawBody: string): string | n /** Drain pending pull-mode events for an installation. The engine calls this outbound (it can't be pushed to): * 1) prune TTL-expired rows fleet-wide, 2) delete rows the caller ACKs (scoped to this install so one container * can never ack another's), then 3) return the next ordered batch. `ack` and `limit` are both capped at the batch - * size to bound the SQL and the response. */ + * size to bound the SQL and the response. + * + * #9150: `enrollId`, when supplied, scopes BOTH the ack-delete and the select to rows tagged for that specific + * enrollment (or untagged legacy/config_push rows) — not merely `installation_id`. Without it, any enrollment + * secret valid for the installation could drain (and destructively-ack) another live enrollment's queue, e.g. + * a stale re-enrolled container still holding a valid-but-superseded secret. Optional (and defaults to + * matching only untagged rows when omitted) so a caller that hasn't threaded an enrollId through yet degrades + * to the narrower, safer set rather than erroring. */ export async function pullRelayPending( env: Env, installationId: number, - opts?: { ack?: string[] | undefined; limit?: number | undefined }, + opts?: { ack?: string[] | undefined; limit?: number | undefined; enrollId?: string | null | undefined }, ): Promise { // Prune rows the engine never came back for (same datetime() comparison style as retryFailedRelays). await pruneRelayPending(env); + const enrollId = opts?.enrollId ?? null; const ack = opts?.ack?.slice(0, RELAY_PENDING_BATCH_SIZE) ?? []; if (ack.length) { const placeholders = ack.map(() => "?").join(", "); await env.DB - .prepare(`DELETE FROM orb_relay_pending WHERE installation_id = ? AND delivery_id IN (${placeholders})`) - .bind(installationId, ...ack) + .prepare( + `DELETE FROM orb_relay_pending WHERE installation_id = ? AND (enroll_id = ? OR enroll_id IS NULL) AND delivery_id IN (${placeholders})`, + ) + .bind(installationId, enrollId, ...ack) .run(); } @@ -445,8 +462,10 @@ export async function pullRelayPending( : RELAY_PENDING_BATCH_SIZE; const limit = Math.min(requestedLimit, RELAY_PENDING_BATCH_SIZE); const { results } = await env.DB - .prepare("SELECT delivery_id, event_name, raw_body, kind FROM orb_relay_pending WHERE installation_id = ? ORDER BY created_at, delivery_id LIMIT ?") - .bind(installationId, limit) + .prepare( + "SELECT delivery_id, event_name, raw_body, kind FROM orb_relay_pending WHERE installation_id = ? AND (enroll_id = ? OR enroll_id IS NULL) ORDER BY created_at, delivery_id LIMIT ?", + ) + .bind(installationId, enrollId, limit) .all<{ delivery_id: string; event_name: string; raw_body: string; kind: string }>(); return results.map((r) => ({ deliveryId: r.delivery_id, eventName: r.event_name, rawBody: r.raw_body, kind: r.kind })); } @@ -536,16 +555,26 @@ export async function forwardOrbEvent( // then the newest relay registration, then the newest enrollment. The final tie-break is the implicit rowid // (monotonic insertion order) — enroll_id is a random opaque token, and CURRENT_TIMESTAMP ties at second // resolution, so rowid is the only stable "most recently inserted" key when those collide (#1783). - const row = await env.DB + // + // #9150: fetch every LIVE row (not just the winner via .first()) so a second, briefly-overlapping live + // enrollment for the same installation (a blue/green swap, or a secret rotated but not yet revoked) is + // OBSERVABLE — the sibling revoke-on-reissue fix (#9149) narrows the window this can happen in, but a + // rollout swap can still produce two live consumers for a short time, and that should never be silent. + const { results: liveRows } = await env.DB .prepare( - "SELECT relay_mode, relay_url, relay_secret_enc, relay_secret_iv, relay_secret_salt FROM orb_enrollments WHERE installation_id = ? AND state = 'enrolled' AND revoked_at IS NULL ORDER BY (relay_registered_at IS NOT NULL) DESC, relay_registered_at DESC, enrolled_at DESC, rowid DESC", + "SELECT enroll_id, relay_mode, relay_url, relay_secret_enc, relay_secret_iv, relay_secret_salt FROM orb_enrollments WHERE installation_id = ? AND state = 'enrolled' AND revoked_at IS NULL ORDER BY (relay_registered_at IS NOT NULL) DESC, relay_registered_at DESC, enrolled_at DESC, rowid DESC", ) .bind(args.installationId) - .first<{ relay_mode: string; relay_url: string | null; relay_secret_enc: string | null; relay_secret_iv: string | null; relay_secret_salt: string | null }>(); + .all<{ enroll_id: string; relay_mode: string; relay_url: string | null; relay_secret_enc: string | null; relay_secret_iv: string | null; relay_secret_salt: string | null }>(); + const row = liveRows[0]; if (!row) return "ignored"; // not a brokered self-host (or revoked) — nothing to relay to + if (liveRows.length > 1) incr("loopover_orb_relay_multiple_live_enrollments_total"); // Pull mode (#16): a tailnet container can't be pushed to, so ENQUEUE the event for it to drain outbound. + // Tagged with the WINNING enrollment's id (#9150) so pullRelayPending can later scope its drain to exactly + // this enrollment instead of "any secret valid for this installation" — closing the hole where a second, + // stale-but-still-valid enrollment could drain and destructively-ack this event first. if (row.relay_mode === "pull") { - await enqueueRelayPending(env, { deliveryId: args.deliveryId, installationId: args.installationId, eventName: args.eventName, rawBody: args.rawBody }); + await enqueueRelayPending(env, { deliveryId: args.deliveryId, installationId: args.installationId, eventName: args.eventName, rawBody: args.rawBody, enrollId: row.enroll_id }); return "queued"; } // Push mode with nothing registered (relay_url null) or no decryption key → skip. relay_secret_enc/iv are written diff --git a/test/integration/orb-relay.test.ts b/test/integration/orb-relay.test.ts index f7e1c0d02f..b9e6598d75 100644 --- a/test/integration/orb-relay.test.ts +++ b/test/integration/orb-relay.test.ts @@ -3,6 +3,7 @@ import { createApp } from "../../src/api/routes"; import { issueOrbEnrollment } from "../../src/orb/broker"; import { relayForward } from "../../src/orb/webhook"; import { enqueueRelayPending, forwardOrbEvent, MAX_ORB_RELAY_REGISTER_BODY_BYTES, pullRelayPending, readOrbRelayRegisterBody, registerOrbRelay, relaySignature, relayVerify, retryFailedRelays, storeRelayFailure } from "../../src/orb/relay"; +import { counterValue, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv, type TestD1Database } from "../helpers/d1"; const db = (e: Env) => e.DB as unknown as TestD1Database; @@ -274,6 +275,61 @@ describe("forwardOrbEvent", () => { expect(h["x-orb-signature-256"]).toBe(`sha256=${await relaySignature(freshSecret, body)}`); }); + it("#9150: PULL mode tags each enqueued row with the WINNING enrollment's id, and emits a metric when a second live enrollment exists", async () => { + const e = brokeredEnv(); + await seedInstall(e, 806); + resetMetrics(); + const secretA = ((await issueOrbEnrollment(e, 806)) as { secret: string }).secret; + await registerOrbRelay(e, secretA, "https://a.example/v1/orb/relay"); + await db(e).prepare("UPDATE orb_enrollments SET relay_mode = 'pull' WHERE installation_id = 806").run(); + // Only ONE live enrollment so far — the metric must not fire yet. + expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 806, deliveryId: "single-enroll", rawBody: "{}" })).toBe("queued"); + expect(counterValue("loopover_orb_relay_multiple_live_enrollments_total")).toBe(0); + const rowA = await db(e).prepare("SELECT enroll_id FROM orb_enrollments WHERE installation_id = 806 ORDER BY rowid").first<{ enroll_id: string }>(); + const taggedSingle = await db(e).prepare("SELECT enroll_id FROM orb_relay_pending WHERE delivery_id = 'single-enroll'").first<{ enroll_id: string | null }>(); + expect(taggedSingle?.enroll_id).toBe(rowA?.enroll_id); + + // A second issuance (blue/green swap, or a rotation that hasn't revoked the old secret yet) leaves TWO live rows. + const secretB = ((await issueOrbEnrollment(e, 806)) as { secret: string }).secret; + await registerOrbRelay(e, secretB, "https://b.example/v1/orb/relay"); + await db(e).prepare("UPDATE orb_enrollments SET relay_mode = 'pull' WHERE installation_id = 806").run(); + const rowB = await db(e).prepare("SELECT enroll_id FROM orb_enrollments WHERE installation_id = 806 ORDER BY rowid DESC").first<{ enroll_id: string }>(); + expect(await forwardOrbEvent(e, { eventName: "pull_request", installationId: 806, deliveryId: "double-enroll", rawBody: "{}" })).toBe("queued"); + expect(counterValue("loopover_orb_relay_multiple_live_enrollments_total")).toBe(1); // now observable, not silent + const taggedDouble = await db(e).prepare("SELECT enroll_id FROM orb_relay_pending WHERE delivery_id = 'double-enroll'").first<{ enroll_id: string | null }>(); + expect(taggedDouble?.enroll_id).toBe(rowB?.enroll_id); // tagged for the newest (winning) enrollment, same election as push + + // pullRelayPending scoped to enrollment A must see only its own event (+ any untagged legacy rows) — NOT + // enrollment B's, even though both belong to the same installation. This is the actual #9150 fix: before it, + // "any valid secret for the installation" could drain and destructively-ack the other consumer's queue. + const drainedByA = await pullRelayPending(e, 806, { enrollId: rowA?.enroll_id ?? null }); + expect(drainedByA.map((ev) => ev.deliveryId)).toEqual(["single-enroll"]); + const drainedByB = await pullRelayPending(e, 806, { enrollId: rowB?.enroll_id ?? null }); + expect(drainedByB.map((ev) => ev.deliveryId)).toEqual(["double-enroll"]); + }); + + it("#9150: an enrollment scope ALSO drains untagged legacy rows (enroll_id IS NULL never orphaned)", async () => { + const e = brokeredEnv(); + await enqueueRelayPending(e, { deliveryId: "legacy-untagged", installationId: 807, eventName: "pull_request", rawBody: "{}" }); + await enqueueRelayPending(e, { deliveryId: "tagged-for-x", installationId: 807, eventName: "pull_request", rawBody: '{"n":1}', enrollId: "enroll-x" }); + const drained = await pullRelayPending(e, 807, { enrollId: "enroll-x" }); + expect(drained.map((ev) => ev.deliveryId).sort()).toEqual(["legacy-untagged", "tagged-for-x"]); + // A DIFFERENT enrollment for the same install sees the untagged row too, but not the one tagged for "enroll-x". + const drainedByOther = await pullRelayPending(e, 807, { enrollId: "enroll-y" }); + expect(drainedByOther.map((ev) => ev.deliveryId)).toEqual(["legacy-untagged"]); + }); + + it("#9150: ack scoped to an enrollment cannot delete another live enrollment's row for the same installation", async () => { + const e = brokeredEnv(); + await enqueueRelayPending(e, { deliveryId: "owned-by-x", installationId: 808, eventName: "pull_request", rawBody: "{}", enrollId: "enroll-x" }); + await enqueueRelayPending(e, { deliveryId: "owned-by-y", installationId: 808, eventName: "pull_request", rawBody: '{"n":1}', enrollId: "enroll-y" }); + // enroll-x tries to ack BOTH its own row and enroll-y's — only its own is removed. + const remaining = await pullRelayPending(e, 808, { enrollId: "enroll-x", ack: ["owned-by-x", "owned-by-y"] }); + expect(remaining).toEqual([]); // enroll-x's own scoped view has nothing left to return + const victim = await db(e).prepare("SELECT delivery_id FROM orb_relay_pending WHERE delivery_id = 'owned-by-y'").first(); + expect(victim ?? null).not.toBeNull(); // enroll-y's row survived enroll-x's ack — the actual #9150 fix + }); + it("returns FAILED (never throws) on a non-ok response or a thrown fetch — the Orb 202 always stands", async () => { const e = brokeredEnv(); const secret = await enroll(e, 802); From 54175b085dc301ca5c4f4ee36de76dbcee49ec07 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:46:40 -0700 Subject: [PATCH 4/6] fix(orb): harden the federated peer-median trust boundary (#9148) One allowlisted key could mint unlimited Sybil peers and own the median outright: verifyFederatedBundle only checked whether SOME allowlisted key signed a bundle, never bound a key to an instanceId, and importPeerBundles had no per-instance dedup, no range checks, no freshness/replay protection, and pulled peer bundles synchronously on every maintainer dashboard load. - Range-validate every numeric field (rates in [0,1], non-negative counts, cycleP50Ms <= cycleP95Ms) as its own "out_of_range" rejection, distinct from a wrong-typed "malformed" field. - Reject a windowDays that doesn't match the local instance's own resolved window, and a generatedAt outside a bounded freshness window (7 days) or more than 5 minutes in the future. - Enforce decided >= MIN_DECIDED receiver-side rather than trusting the sender's own eligibility claim. - Dedup a batch to the LAST bundle per instanceId (importPeerBundles), then add a persisted, cross-tick per-instance replay/rollback watermark and a per-key Sybil cap (MAX_INSTANCES_PER_KEY) via the new applyFederatedPeerWatermarks, backed by a system_flags JSON blob rather than a new table -- a self-hosted federation's peer count is expected to stay small enough that an in-memory scan/cap-check is trivially cheap. - Move the peer pull off the dashboard's request path entirely: a new "federated-peer-sync" queue job (10-minute cadence, gated on the loopover self-repo's federatedIntelligence.enabled) now runs the pull + trust-gate + persist pipeline AND pushes this instance's own bundle (wiring pushFederatedBundle into a real tick, closing the other half of #9166). buildFederatedBenchmark still computes the local half live (a fast local DB query) but reads the peer half from a cache the tick refreshes. - peerCount now naturally counts distinct contributing INSTANCES (a side effect of the per-instance dedup), matching what its own doc comment always claimed but the old code never enforced. --- src/api/routes.ts | 11 +- src/index.ts | 11 + src/orb/federated-benchmark.ts | 194 ++++++++++++++--- src/orb/federated-import.ts | 286 +++++++++++++++++++++++--- src/queue/job-dispatch.ts | 17 ++ src/types.ts | 8 + test/unit/federated-benchmark.test.ts | 173 +++++++++++----- test/unit/federated-import.test.ts | 189 ++++++++++++++++- test/unit/index.test.ts | 33 +++ test/unit/job-dispatch.test.ts | 30 +++ 10 files changed, 841 insertions(+), 111 deletions(-) diff --git a/src/api/routes.ts b/src/api/routes.ts index 04dfdf5e97..c5afeba4dc 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -1761,14 +1761,15 @@ export function createApp() { }); // Federated benchmark (#6481): "your gate precision vs peer median". Reads the opt-in from the loopover // self-repo's manifest (mirrors prReconciliation/publicStats/etc.'s fleet-wide override lookup) rather - // than any of the maintainer's own repos — federatedIntelligence is operator-level, not per-repo. Bounded - // to a single, short-timeout attempt so an unreachable or slow collector degrades the panel to its - // existing empty state instead of holding up the whole dashboard load. + // than any of the maintainer's own repos — federatedIntelligence is operator-level, not per-repo. + // #9148: buildFederatedBenchmark no longer pulls a peer collector on this request path at all — the + // local half is a single fast DB query, and the peer half is read from a cache the "federated-peer-sync" + // background queue job refreshes on its own cadence. This used to be a live, rate-limited network call + // on every dashboard load (N maintainers hitting refresh was N requests/second at the peer collector); + // now a slow/unreachable collector can only make the CACHE stale, never hold up this request. const federatedIntelligenceManifest = await loadRepoFocusManifest(c.env, resolveLoopOverSelfRepoFullName(c.env)); const federatedBenchmark = await buildFederatedBenchmark(federatedIntelligenceManifest, c.env.DB, { now: Date.parse(generatedAt), - timeoutMs: 5_000, - maxAttempts: 1, }); const qualityDashboard = { ...buildMaintainerQualityDashboard({ diff --git a/src/index.ts b/src/index.ts index 1ad18b400f..486ce9ff2e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,6 +14,7 @@ import { isLoopEscalationSweepEnabled } from "./review/loop-escalation-wire"; import { isAprRepoTransferPollEnabled } from "./orb/apr-repo-transfer"; import { isPrReconciliationEnabled, resolvePrReconciliationManifestOverride } from "./review/pr-reconciliation"; import { isActiveReviewReconciliationEnabled, resolveActiveReviewReconciliationManifestOverride } from "./review/active-review-reconciliation"; +import { resolveFederatedIntelligenceManifestOverride } from "./orb/federated-benchmark"; import { isRagEnabled } from "./review/rag-wire"; import { isDecisionAuditEnabled } from "./review/decision-audit"; import { isRiskControlEnabled } from "./review/risk-control-wire"; @@ -226,6 +227,16 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): jobs.push({ type: "reconcile-active-review-tracking", requestedBy: "schedule" }); } } + // Federated peer-sync tick (#9148/#9166). Same 10-minute cadence as the reconciliation jobs above — a + // best-effort background sync has no business running any more often than that, and the collector's own + // module-level rate limiter (federated-collector.ts, 6/min) further bounds any single tick's calls. Opt-in + // via the loopover self-repo's `.loopover.yml federatedIntelligence:` block only — there is no env-var + // equivalent for this one (unlike prReconciliation/activeReviewReconciliation above), since federated intel + // has never had one; flag-OFF (default, `present: false`) this job is never created. + if (isReconciliationWindow) { + const federatedIntelligenceManifestOverride = await resolveFederatedIntelligenceManifestOverride(env); + if (federatedIntelligenceManifestOverride.enabled) jobs.push({ type: "federated-peer-sync", requestedBy: "schedule" }); + } if (isHourly) { // Isolation (#experimental-gittensor-plugin): on self-host, refresh-registry both FETCHES from and // PERSISTS the whole upstream gittensor-subnet registry (entrius/gittensor has no server-side filtering, diff --git a/src/orb/federated-benchmark.ts b/src/orb/federated-benchmark.ts index c5d7db0629..7f59402eec 100644 --- a/src/orb/federated-benchmark.ts +++ b/src/orb/federated-benchmark.ts @@ -1,46 +1,102 @@ // LoopOver federated fleet intelligence (#1970) — the dashboard benchmark (#6481): "your gate precision vs -// peer median". Composes the three already-shipped pipeline stages — export (#6478, federated-bundle.ts), -// transport (#6479, federated-collector.ts), and trust-gated import (#6480, federated-import.ts) — into the -// one comparison the maintainer dashboard renders. No new storage, no new network primitive: this module is -// pure composition over functions that already exist and already fail safe on their own. +// peer median". Composes the already-shipped pipeline stages — export (#6478, federated-bundle.ts), transport +// (#6479, federated-collector.ts), and trust-gated import (#6480/#9148, federated-import.ts) — into the one +// comparison the maintainer dashboard renders. +// +// #9148 SPLIT THIS MODULE IN TWO, on a request-time/background-tick line: +// - {@link buildFederatedBenchmark} is the REQUEST-TIME read path (called from the dashboard route). It +// computes the LOCAL half live (a single local DB query, cheap) but reads the PEER half from a cache this +// module writes on a background tick — it never pulls a peer collector itself. Before #9148, this +// function made a live network call (with a rate limiter, a 5s timeout, and a 1-MB response ceiling) on +// EVERY maintainer dashboard load; N maintainers hitting refresh was N requests/second at the peer +// collector, and every load paid that request's full latency. +// - {@link refreshFederatedBenchmarkCache} is the BACKGROUND-TICK write path (called from the federated +// peer-sync queue job, src/queue/job-dispatch.ts's "federated-peer-sync" case). It does the actual pull + +// trust-gate + persisted-watermark pipeline and writes the result to a `system_flags` cache row. This is +// the only place in this module that touches the network. // // FAIL-SAFE BY COMPOSITION, not by a wrapping try/catch: buildFederatedBundle degrades to null on any error, -// pullPeerBundles degrades to [] on any error or when not opted in, and importPeerBundles is a pure function -// with no I/O. None of the three can throw, so this module doesn't need to catch anything either — wrapping -// it in another try/catch would only hide which stage actually failed. -import { buildFederatedBundle, isFederatedIntelligenceEnabled } from "./federated-bundle"; -import { importPeerBundles } from "./federated-import"; -import { pullPeerBundles, type CollectorOpts } from "./federated-collector"; +// pullPeerBundles degrades to [] on any error or when not opted in, and importPeerBundles/ +// applyFederatedPeerWatermarks degrade their own way on failure (a DB read failure reads as "cache empty" / +// "every peer state unknown", never a thrown error). Neither function here needs its own catch as a result. +import { buildFederatedBundle, isFederatedIntelligenceEnabled, resolveFederatedWindowDays } from "./federated-bundle"; +import { applyFederatedPeerWatermarks, importPeerBundles } from "./federated-import"; +import { pullPeerBundles, pushFederatedBundle, type CollectorOpts } from "./federated-collector"; import { percentile } from "./analytics"; +import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; +import { resolveLoopOverSelfRepoFullName } from "../config/loopover-repo-focus-manifest"; import type { FocusManifest } from "../signals/focus-manifest"; export interface FederatedBenchmark { /** This instance's own P(merged & not reverted | gate said merge), from buildFederatedBundle. Null below - * MIN_DECIDED, exactly like the exported bundle field it reuses. */ + * MIN_DECIDED, exactly like the exported bundle field it reuses. Computed LIVE at request time — this is + * a single local DB query, not a network call, so there is no reason to cache it. */ localMergePrecision: number | null; - /** Median mergePrecision across every accepted (trust-gated) peer bundle that itself cleared MIN_DECIDED. - * Null when no peer contributed a numeric value yet — an empty-state condition, not an error. */ + /** Median mergePrecision across every accepted (trust-gated, range-checked, fresh, per-instance-deduped, + * Sybil-capped — #9148) peer bundle that itself cleared MIN_DECIDED, as of the last background sync tick. + * Null when no peer has ever contributed one — an empty-state condition, not an error. */ peerMedianMergePrecision: number | null; - /** How many peers actually contributed to the median above (i.e. passed trust-gating AND had a non-null - * mergePrecision) — NOT the raw count of bundles pulled or accepted, which may include peers still below - * their own MIN_DECIDED threshold. */ + /** How many DISTINCT peer instances contributed to the median above, as of the last background sync tick. */ peerCount: number; generatedAt: string; } +/** system_flags key for the cached peer-half of the benchmark (#9148) — written only by + * {@link refreshFederatedBenchmarkCache}, read only by {@link buildFederatedBenchmark}. */ +const FEDERATED_BENCHMARK_CACHE_FLAG_KEY = "orb:federated_benchmark_cache"; + +interface FederatedBenchmarkCache { + peerMedianMergePrecision: number | null; + peerCount: number; + /** When the cache was last refreshed — surfaced so a stale-cache condition (sync tick disabled/broken) is + * at least visible in the raw system_flags row, even though the dashboard field above always reports its + * OWN generatedAt (the read time, not the cache's write time). */ + refreshedAt: string; +} + +/** Fails safe to null on any error (missing row, corrupt JSON, a D1 outage) — a cache miss reads exactly like + * "opted in, no peer data yet", the same empty state #6481 already treats as normal rather than an error. */ +async function readFederatedBenchmarkCache(db: D1Database): Promise { + try { + const row = await db.prepare("SELECT value FROM system_flags WHERE key = ?").bind(FEDERATED_BENCHMARK_CACHE_FLAG_KEY).first<{ value: string }>(); + if (!row?.value) return null; + const parsed: unknown = JSON.parse(row.value); + if (parsed === null || typeof parsed !== "object") return null; + const cache = parsed as Partial; + if (typeof cache.peerCount !== "number" || typeof cache.refreshedAt !== "string") return null; + return { + peerMedianMergePrecision: typeof cache.peerMedianMergePrecision === "number" ? cache.peerMedianMergePrecision : null, + peerCount: cache.peerCount, + refreshedAt: cache.refreshedAt, + }; + } catch { + return null; + } +} + +/** Best-effort write. A write failure must never fail the sync tick — the next tick simply overwrites the + * still-stale cache again. */ +async function writeFederatedBenchmarkCache(db: D1Database, cache: FederatedBenchmarkCache): Promise { + await db + .prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)") + .bind(FEDERATED_BENCHMARK_CACHE_FLAG_KEY, JSON.stringify(cache)) + .run() + .catch(() => undefined); +} + /** - * Build the local-vs-peer-median benchmark for the maintainer dashboard. + * Build the local-vs-peer-median benchmark for the maintainer dashboard — the REQUEST-TIME read path (#9148). * * Returns null — touching nothing beyond the opt-in check — when federated intelligence is not enabled for - * this deployment (`federatedIntelligence.enabled` off in the loopover self-repo's manifest). This is the - * "an instance that hasn't opted in sees no new UI, not an empty/disabled version of it" gate #6481 requires; - * the caller renders no panel at all on a null result, distinct from a real object with peerCount: 0 (opted - * in, no peer data yet — an empty state, not an error). + * this deployment. Otherwise ALWAYS computes the local half live (a single, fast, local DB query) and reads + * the peer half from the background-tick cache; it never pulls a peer collector itself. A cache miss (no + * sync tick has run yet, e.g. right after opting in) reads as peerCount 0 / a null median — the SAME + * empty-state shape a genuinely peerless deployment already renders, not an error and not a missing panel. */ export async function buildFederatedBenchmark( manifest: Pick | null | undefined, db: D1Database, - opts: { now?: number; windowDays?: number } & CollectorOpts = {}, + opts: { now?: number; windowDays?: number } = {}, ): Promise { if (!isFederatedIntelligenceEnabled(manifest)) return null; @@ -49,21 +105,105 @@ export async function buildFederatedBenchmark( // passed, so an omitted opts.windowDays falls through to buildFederatedBundle's own default instead of // being overridden with an explicit undefined. const localBundle = await buildFederatedBundle(manifest, db, opts.windowDays === undefined ? { now } : { now, windowDays: opts.windowDays }); + const cached = await readFederatedBenchmarkCache(db); + + return { + localMergePrecision: localBundle?.mergePrecision ?? null, + peerMedianMergePrecision: cached?.peerMedianMergePrecision ?? null, + peerCount: cached?.peerCount ?? 0, + generatedAt: new Date(now).toISOString(), + }; +} + +/** + * Pull + trust-gate + persist every peer bundle this instance can reach, then cache the resulting median for + * {@link buildFederatedBenchmark} to read — the BACKGROUND-TICK write path (#9148). Called only from the + * federated peer-sync queue job, never from any request path. + * + * A no-op (cache untouched) when not opted in — an opted-out instance never overwrites a stale cache with a + * fresh "no data" result, so re-enabling later doesn't need to wait out a sync tick to see its last known + * peer state disappear for no reason. Every stage below already fails safe on its own (see this file's header + * comment), so this function needs no wrapping try/catch either. + */ +export async function refreshFederatedBenchmarkCache( + manifest: Pick | null | undefined, + db: D1Database, + opts: { now?: number; windowDays?: number } & CollectorOpts = {}, +): Promise { + if (!isFederatedIntelligenceEnabled(manifest)) return; + + const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); + const localWindowDays = resolveFederatedWindowDays(opts.windowDays); + const config = manifest?.federatedIntelligence; const peerBundles = await pullPeerBundles(manifest, opts); - const { accepted } = importPeerBundles(manifest, peerBundles); + const stateless = importPeerBundles(manifest, peerBundles, { now, localWindowDays }); + const { accepted } = config ? await applyFederatedPeerWatermarks(db, stateless, config.peerKeys, { now }) : stateless; + // MEDIAN, NOT MEAN (mirrors analytics.ts's own fleet aggregation, see federated-import.ts's header comment): // a bounded number of outliers cannot drag a median arbitrarily, so re-deriving a mean here would quietly // weaken the same poisoning-resistance property the import side already relies on holding by construction. + // `accepted` is deduped one-bundle-per-instanceId by importPeerBundles (#9148), so this count is already a + // count of distinct CONTRIBUTING INSTANCES, not raw bundles — the bug the peerCount doc used to warn about. const peerMergePrecisions = accepted .map((bundle) => bundle.mergePrecision) .filter((value): value is number => value !== null) .sort((a, b) => a - b); - return { - localMergePrecision: localBundle?.mergePrecision ?? null, + await writeFederatedBenchmarkCache(db, { peerMedianMergePrecision: percentile(peerMergePrecisions, 50), peerCount: peerMergePrecisions.length, - generatedAt: new Date(now).toISOString(), - }; + refreshedAt: new Date(now).toISOString(), + }); +} + +/** + * The full federated peer-sync tick (#9148/#9166): refreshes the peer-median cache AND pushes this instance's + * own bundle to the operator's configured collector, in parallel. This is the function the "federated-peer- + * sync" queue job calls; it is the concrete wiring #9166 deferred ("pushFederatedBundle wiring into a real + * tick lands with the #9148 commit, which introduces the background refresh job this naturally slots into"). + * + * `Promise.allSettled` rather than `Promise.all`: a push failure must never skip the pull-side refresh (and + * vice versa) — the two directions are independent and each already fails safe to a falsy/empty result on + * its own, so there is nothing here worth rejecting the whole tick over. + */ +export async function runFederatedPeerSyncTick( + manifest: Pick | null | undefined, + db: D1Database, + opts: { now?: number; windowDays?: number } & CollectorOpts = {}, +): Promise { + await Promise.allSettled([refreshFederatedBenchmarkCache(manifest, db, opts), pushFederatedBundle(manifest, db, opts)]); +} + +/** Config-as-code override for the SCHEDULER's own enqueue gate (#9148) — mirrors resolveOpsManifestOverride + * (src/review/ops-wire.ts) exactly: a short in-isolate TTL cache over the loopover self-repo's manifest, so + * the cron tick doesn't pay a full manifest load on every 2-minute pass just to decide whether to enqueue a + * job the processor will re-check anyway (defense in depth — the queue job-dispatch case loads the full + * manifest itself before doing any real work, exactly like ops-alerts does). A load failure degrades to + * `{ enabled: false }`, so a manifest hiccup can only ever under-enqueue, never wrongly arm the sync. */ +export type FederatedIntelligenceManifestOverride = { present: boolean; enabled: boolean }; + +const FEDERATED_MANIFEST_OVERRIDE_CACHE_TTL_MS = 60_000; +let federatedManifestOverrideCache: { override: FederatedIntelligenceManifestOverride; at: number } | null = null; + +export async function resolveFederatedIntelligenceManifestOverride(env: Env, nowMs: number = Date.now()): Promise { + const hit = federatedManifestOverrideCache; + if (hit && nowMs - hit.at < FEDERATED_MANIFEST_OVERRIDE_CACHE_TTL_MS) return hit.override; + try { + const manifest = await loadRepoFocusManifest(env, resolveLoopOverSelfRepoFullName(env)); + const config = manifest.federatedIntelligence; + const override = { present: config.present, enabled: config.enabled === true }; + federatedManifestOverrideCache = { override, at: nowMs }; + return override; + } catch { + const override = { present: false, enabled: false }; + federatedManifestOverrideCache = { override, at: nowMs }; + return override; + } +} + +/** Test-only: clears the cached override, mirroring clearOpsManifestOverrideCacheForTest — without this, a + * test suite running many cases would leak one test's cached override into the next. */ +export function clearFederatedIntelligenceManifestOverrideCacheForTest(): void { + federatedManifestOverrideCache = null; } diff --git a/src/orb/federated-import.ts b/src/orb/federated-import.ts index 228f76d4e9..6ef784f8a5 100644 --- a/src/orb/federated-import.ts +++ b/src/orb/federated-import.ts @@ -4,26 +4,59 @@ // (src/orb/federated-collector.ts, #6479) may be folded into local calibration or the peer-median benchmark // (#6481) at all. The export side is #6478 (src/orb/federated-bundle.ts). // -// The trust model is #6477's DESIGN DECISION, implemented here exactly as specified and deliberately NOT -// redesigned. Its two poisoning-resistance layers, and where each one actually lives: +// The trust model is #6477's DESIGN DECISION. Its poisoning-resistance layers, and where each one lives: // 1. ALLOWLIST — only a peer whose verification key the operator explicitly added to -// `federatedIntelligence.peerKeys` is ever considered. That is enforced HERE, and it is why a Sybil -// attack is self-limiting by construction: forging peers requires the RECEIVING operator to have added -// the attacker's keys themselves. Mirrors MCP_READ_REPO_ALLOWLIST's posture: explicit operator config, -// fail closed when unset, never auto-discovery and never a PKI. +// `federatedIntelligence.peerKeys` is ever considered. Mirrors MCP_READ_REPO_ALLOWLIST's posture: +// explicit operator config, fail closed when unset, never auto-discovery and never a PKI. // 2. MEDIAN, NOT MEAN — a bounded number of outliers cannot drag a median arbitrarily, unlike a mean. That // layer needs no code here: the fleet aggregation this feeds already medians (src/orb/analytics.ts:92), // so it holds by construction. Re-implementing it in this module would fork the definition #6481's // comparison depends on. +// 3. PER-KEY SYBIL CAP + PERSISTED WATERMARK (#9148) — layer 1 alone is NOT self-limiting for peers, only +// for KEYS: nothing previously bound an `instanceId` to a key, so one allowlisted key could sign an +// unbounded number of distinct fabricated `instanceId`s and, since `accepted` had no per-instance dedup +// either, own the entire median outright. {@link applyFederatedPeerWatermarks} closes both gaps: it +// keeps only the LAST bundle per `instanceId` in a batch, persists a per-instance high-water mark +// (`generatedAt`) so an old-but-still-fresh bundle can never be replayed forever, and caps how many +// distinct instanceIds a single verifying key may ever contribute. // // #6477 explicitly rejected building a reputation/decay/scoring system for trust, so there is deliberately no // per-peer score, no anomaly heuristic, and no retroactive poisoned-bundle detection here: an operator who -// discovers a bad peer removes its key from the allowlist. Adding any of those would be inventing a mechanism -// that design pass considered and turned down. -import { canonicalizeFederatedBundleBody, FEDERATED_BUNDLE_SCHEMA_VERSION, type FederatedSignalBundle, type FederatedSignalBundleBody } from "./federated-bundle"; +// discovers a bad peer removes its key from the allowlist. The Sybil cap above is a bound on how much damage +// one STILL-TRUSTED key can do, not a reputation system for individual peers. +import { createHash, createHmac } from "node:crypto"; +import { MIN_DECIDED } from "./analytics"; +import { + canonicalizeFederatedBundleBody, + FEDERATED_BUNDLE_SCHEMA_VERSION, + resolveFederatedWindowDays, + type FederatedSignalBundle, + type FederatedSignalBundleBody, +} from "./federated-bundle"; import { timingSafeEqualHex } from "../utils/crypto"; import type { FocusManifest } from "../signals/focus-manifest"; -import { createHmac } from "node:crypto"; + +/** A pulled bundle older than this (relative to `now`) is rejected rather than counted forever — closes the + * "re-serve one favorable year-old bundle" hole (#9148). Generous relative to DEFAULT_WINDOW_DAYS so a + * best-effort background sync that occasionally misses a tick for a day or two never starts rejecting a + * peer that is still perfectly healthy. */ +const MAX_BUNDLE_AGE_MS = 7 * 86_400_000; + +/** How far into the future a `generatedAt` may claim to be before it's rejected outright, rather than merely + * accepted with a skewed clock. Generous enough to absorb real clock drift between two independent + * self-hosted instances without opening a meaningful backdating/replay window. */ +const MAX_CLOCK_SKEW_MS = 5 * 60_000; + +/** How many DISTINCT `instanceId`s a single verifying key may ever contribute to the accepted set (#9148). + * Without this, one allowlisted key can mint an unbounded number of fabricated instanceIds and dominate the + * peer median outright — the allowlist bounds which KEYS are trusted, not how many PEERS a key may claim to + * speak for. Deliberately generous: this is a ceiling on abuse, not a realistic peer-count expectation for + * a self-hosted federation of this scale. */ +const MAX_INSTANCES_PER_KEY = 10; + +/** Peer-state entries not refreshed in this long are pruned on the next tick, so a key that stops presenting + * a stale instanceId eventually frees its slot under the cap above, rather than permanently consuming it. */ +const PEER_STATE_PRUNE_AFTER_MS = 400 * 86_400_000; /** Why a bundle was not folded in. Every rejection carries one of these, so a rejection is always traceable to * a specific rule rather than vanishing silently (#6480 requires rejections be operator-visible). */ @@ -39,7 +72,27 @@ export type FederatedRejectionReason = /** No allowlisted key reproduces the signature: either an untrusted peer or a tampered body. These are * deliberately ONE reason — with a detached HMAC the receiver cannot distinguish them, and pretending * otherwise would report a distinction this scheme cannot actually make. */ - | "untrusted_or_tampered"; + | "untrusted_or_tampered" + /** A signature-covered numeric field is outside its valid range (e.g. a rate outside [0,1], a negative + * count) (#9148). Distinct from "malformed": the field has the RIGHT TYPE, just an impossible value. */ + | "out_of_range" + /** `windowDays` does not match the LOCAL instance's own resolved window, so the values are not comparable — + * #9148, closing "mixing windowDays: 7 with windowDays: 365 bundles into one median is uncaught". */ + | "window_mismatch" + /** `generatedAt` is unparseable, too far in the future for plausible clock skew, or older than + * MAX_BUNDLE_AGE_MS — #9148, closing "re-serve one favorable year-old bundle forever". */ + | "stale_or_future" + /** `decided` is below MIN_DECIDED — the sender's own eligibility bar, enforced receiver-side rather than + * trusted from the sender (#9148: nothing previously stopped a peer self-reporting `decided: 3, + * mergePrecision: 1.0`). */ + | "below_min_decided" + /** This exact `instanceId` already has a NEWER `generatedAt` on record — a replay or clock-rollback of an + * otherwise-valid bundle (#9148, the persisted high-water mark). Only ever produced by + * {@link applyFederatedPeerWatermarks}, which is the one place this pipeline touches the DB. */ + | "replayed_or_rollback" + /** This `instanceId` is new, and admitting it would push its verifying key over MAX_INSTANCES_PER_KEY — + * the per-key Sybil cap (#9148). Only ever produced by {@link applyFederatedPeerWatermarks}. */ + | "sybil_cap_exceeded"; /** One rejected bundle, reduced to what an operator can act on without leaking bundle contents. */ export interface FederatedRejection { @@ -71,7 +124,13 @@ export function isFederatedImportEnabled(manifest: ManifestSlice | null | undefi } /** Does `bundle` carry every signature-covered field, with the right type? Guards the canonicalization below: - * an absent field would otherwise serialize as `undefined` and silently change the signed bytes. */ + * an absent field would otherwise serialize as `undefined` and silently change the signed bytes. + * + * TYPE-SHAPE ONLY (#9148: split from the RANGE check below on purpose) — a field can be a finite number of + * the wrong magnitude (a rate of -3, a decided count of -1) and still pass this function; that is + * {@link isBundleBodyInRange}'s job. Keeping the two separate lets a caller distinguish "malformed" (wrong + * type — likely a schema mismatch) from "out_of_range" (right type, impossible value — likely a hostile or + * buggy peer) in the rejection reason it reports. */ function isBundleBodyShaped(bundle: FederatedSignalBundle): boolean { const numeric = (value: unknown): boolean => typeof value === "number" && Number.isFinite(value); const nullableNumeric = (value: unknown): boolean => value === null || numeric(value); @@ -93,6 +152,24 @@ function isBundleBodyShaped(bundle: FederatedSignalBundle): boolean { ); } +/** Is every numeric field within the range its own semantics allow? Only called once {@link isBundleBodyShaped} + * has already confirmed every field is a finite number (or, for the nullable ones, null) — this function + * never needs to re-guard against `NaN`/`Infinity`/wrong-typeof itself. A rate field outside [0, 1], a + * negative count, or `cycleP50Ms > cycleP95Ms` (the median can never exceed the p95 of the same sample) all + * fail here (#9148). */ +function isBundleBodyInRange(bundle: FederatedSignalBundle): boolean { + const unitRate = (value: number | null): boolean => value === null || (value >= 0 && value <= 1); + const nonNegative = (value: number | null): boolean => value === null || value >= 0; + if (bundle.windowDays <= 0) return false; + if (bundle.decided < 0) return false; + if (!unitRate(bundle.reversalRate) || !unitRate(bundle.slopRate) || !unitRate(bundle.copycatRate)) return false; + if (!unitRate(bundle.mergePrecision) || !unitRate(bundle.closePrecision)) return false; + if (!unitRate(bundle.fpRate) || !unitRate(bundle.fnRate)) return false; + if (!nonNegative(bundle.cycleP50Ms) || !nonNegative(bundle.cycleP95Ms)) return false; + if (bundle.cycleP50Ms !== null && bundle.cycleP95Ms !== null && bundle.cycleP50Ms > bundle.cycleP95Ms) return false; + return true; +} + /** Strip the detached signature back off, so the body is canonicalized over exactly the fields the sender * signed. Rebuilt field-by-field rather than by deleting `signature` from a copy: the canonical form is a * fixed key list, so an extra property a peer appended can never reach the signed bytes. */ @@ -116,45 +193,67 @@ function toBody(bundle: FederatedSignalBundle): FederatedSignalBundleBody { } /** - * Does `bundle`'s signature verify against ANY key the operator allowlisted? + * Which allowlisted key (if any) verifies `bundle`'s signature — the value itself, not an index, since + * `peerKeys` order is operator config and not a stable identity (#9148's per-key Sybil-cap fingerprinting + * needs a value stable across config edits that don't touch that specific key). * * Every candidate key is tried because the HMAC is detached and carries no key hint — the bundle says which * INSTANCE it claims to be from, but `instanceId` is unauthenticated until a key verifies, so selecting a key * by it would trust the attacker-controlled field to pick its own verifier. * * The comparison is timing-safe (timingSafeEqualHex), and the loop deliberately does NOT early-exit on a match: - * it verifies against all keys and ORs the results, so total work does not depend on WHICH key matched. + * it verifies against all keys, so total work does not depend on WHICH key matched (or whether one did). */ -export function verifyFederatedBundle(bundle: FederatedSignalBundle, peerKeys: readonly string[]): boolean { +export function matchingFederatedKey(bundle: FederatedSignalBundle, peerKeys: readonly string[]): string | null { const canonical = canonicalizeFederatedBundleBody(toBody(bundle)); - let verified = false; + let matched: string | null = null; for (const key of peerKeys) { const expected = createHmac("sha256", key).update(canonical).digest("hex"); - if (timingSafeEqualHex(bundle.signature, expected)) verified = true; + if (timingSafeEqualHex(bundle.signature, expected)) matched = key; } - return verified; + return matched; +} + +/** Does `bundle`'s signature verify against ANY key the operator allowlisted? Thin boolean wrapper over + * {@link matchingFederatedKey} — kept as its own export because most callers (and every existing test) only + * need the yes/no answer, not which key matched. */ +export function verifyFederatedBundle(bundle: FederatedSignalBundle, peerKeys: readonly string[]): boolean { + return matchingFederatedKey(bundle, peerKeys) !== null; } -/** Apply every gate to a single bundle. Returns null when it may be folded in, or the reason it may not. */ -function rejectionFor(bundle: FederatedSignalBundle, peerKeys: readonly string[]): FederatedRejectionReason | null { +/** Apply every STATELESS gate to a single bundle (no DB, no network) — schema, shape, range, signature, + * freshness, and window-match. Returns null when it may proceed to the persisted per-instance gates in + * {@link applyFederatedPeerWatermarks}, or the reason it may not. */ +function rejectionFor(bundle: FederatedSignalBundle, peerKeys: readonly string[], now: number, localWindowDays: number): FederatedRejectionReason | null { if (bundle?.schemaVersion !== FEDERATED_BUNDLE_SCHEMA_VERSION) return "unsupported_schema_version"; if (!isBundleBodyShaped(bundle)) return "malformed"; + if (!isBundleBodyInRange(bundle)) return "out_of_range"; if (!verifyFederatedBundle(bundle, peerKeys)) return "untrusted_or_tampered"; + if (bundle.windowDays !== localWindowDays) return "window_mismatch"; + const generatedAtMs = Date.parse(bundle.generatedAt); + if (!Number.isFinite(generatedAtMs)) return "stale_or_future"; + if (now - generatedAtMs > MAX_BUNDLE_AGE_MS || generatedAtMs - now > MAX_CLOCK_SKEW_MS) return "stale_or_future"; + if (bundle.decided < MIN_DECIDED) return "below_min_decided"; return null; } /** * Trust-gate a batch of pulled peer bundles, returning only those an operator's own config says to trust. * - * FAIL-SAFE: this is a pure function the gate never consults — it reads no DB, makes no network call, and - * returns a value rather than mutating anything, so neither a rejected nor a malformed bundle can reach this - * instance's own review/merge behavior. That is the structural version of #6480's fail-safe requirement: there - * is no path from here to a gate decision, rather than a guard that could be forgotten. + * FAIL-SAFE, MOSTLY PURE: this function reads no DB and makes no network call — the gate never consults it, + * so neither a rejected nor a malformed bundle can reach this instance's own review/merge behavior. (The one + * exception in this module is {@link applyFederatedPeerWatermarks}, a separate function that DOES touch the + * DB, called only from the federated background sync job — never from any gate-facing path either.) + * + * Applies every stateless gate (#9148: schema, shape, range, signature, freshness, and window-match) and + * dedups within the batch by `instanceId`, keeping only the LAST bundle for each — closing the other half of + * the Sybil gap: a single verifying key signing many bundles for the SAME fabricated instanceId in one pull + * no longer counts multiple times toward the median. */ export function importPeerBundles( manifest: ManifestSlice | null | undefined, bundles: readonly FederatedSignalBundle[], - opts: { log?: FederatedImportLogger } = {}, + opts: { log?: FederatedImportLogger; now?: number; localWindowDays?: number } = {}, ): FederatedImportResult { const log = opts.log ?? defaultRejectionLogger; const reject = (instanceId: string | null, reason: FederatedRejectionReason): FederatedRejection => { @@ -173,14 +272,23 @@ export function importPeerBundles( return { accepted: [], rejected: bundles.map((bundle) => reject(instanceIdOf(bundle), "no_trusted_peers")) }; } - const accepted: FederatedSignalBundle[] = []; + const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); + const localWindowDays = Number.isFinite(opts.localWindowDays) ? (opts.localWindowDays as number) : resolveFederatedWindowDays(undefined); + const rejected: FederatedRejection[] = []; + // Keep only the LAST bundle per instanceId (a Map preserves insertion order but re-set moves nothing, so we + // delete-then-set to push a re-seen id to the end — irrelevant to correctness here since only the VALUE at + // each key is read back below, never iteration order, but matches the "last wins" contract literally). + const acceptedByInstance = new Map(); for (const bundle of bundles) { - const reason = rejectionFor(bundle, config.peerKeys); - if (reason === null) accepted.push(bundle); - else rejected.push(reject(instanceIdOf(bundle), reason)); + const reason = rejectionFor(bundle, config.peerKeys, now, localWindowDays); + if (reason !== null) { + rejected.push(reject(instanceIdOf(bundle), reason)); + continue; + } + acceptedByInstance.set(bundle.instanceId, bundle); } - return { accepted, rejected }; + return { accepted: [...acceptedByInstance.values()], rejected }; } /** The claimed handle, or null when the bundle is too malformed to carry one. Unauthenticated until a @@ -189,6 +297,124 @@ function instanceIdOf(bundle: FederatedSignalBundle): string | null { return typeof bundle?.instanceId === "string" ? bundle.instanceId : null; } +/** system_flags key for the persisted per-instance peer state (#9148): a single JSON blob keyed by + * instanceId, mirroring the get-or-create secret pattern this subsystem already uses elsewhere + * (federated-bundle.ts's FEDERATED_SIGNING_SECRET_FLAG). A dedicated table was deliberately NOT added: a + * self-hosted federation's peer count is expected to stay in the tens at most, so scanning/filtering the + * whole blob in JS on each tick is trivially cheap and avoids a migration for what is bookkeeping state, not + * a queryable relation. Revisit with a real table + index if that scale assumption ever stops holding. */ +const FEDERATED_PEER_STATE_FLAG_KEY = "orb:federated_peer_state"; + +interface FederatedPeerStateEntry { + /** SHA-256 hex of the verifying key, so the Sybil cap can be enforced without persisting the key itself. */ + keyFingerprint: string; + /** Epoch ms of the newest `generatedAt` accepted for this instanceId — the replay/rollback watermark. */ + lastGeneratedAtMs: number; + /** Epoch ms this entry was last refreshed — drives PEER_STATE_PRUNE_AFTER_MS. */ + lastSeenAtMs: number; + /** Epoch ms this instanceId was first admitted — informational only, never read for a gating decision. */ + firstSeenAtMs: number; +} + +type FederatedPeerState = Record; + +/** SHA-256 hex of a peer verification key — a stable handle for the Sybil cap that never persists the key + * value itself (mirrors this codebase's convention of never logging/storing a raw credential). */ +function federatedKeyFingerprint(key: string): string { + return createHash("sha256").update(key).digest("hex"); +} + +/** Reads the persisted peer-state blob. Fails safe to `{}` on any error (missing row, corrupt JSON, a D1 + * outage) — an unreadable cache degrades to "every instanceId looks new", never to a thrown error, matching + * every other read helper in this subsystem. */ +async function readFederatedPeerState(db: D1Database): Promise { + try { + const row = await db.prepare("SELECT value FROM system_flags WHERE key = ?").bind(FEDERATED_PEER_STATE_FLAG_KEY).first<{ value: string }>(); + if (!row?.value) return {}; + const parsed: unknown = JSON.parse(row.value); + return parsed !== null && typeof parsed === "object" && !Array.isArray(parsed) ? (parsed as FederatedPeerState) : {}; + } catch { + return {}; + } +} + +/** Best-effort write of the peer-state blob. A write failure must never fail the sync tick — the next tick + * simply recomputes from a stale-but-still-usable snapshot. */ +async function writeFederatedPeerState(db: D1Database, state: FederatedPeerState): Promise { + await db + .prepare("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)") + .bind(FEDERATED_PEER_STATE_FLAG_KEY, JSON.stringify(state)) + .run() + .catch(() => undefined); +} + +/** + * Applies the two PERSISTED gates #9148 requires on top of {@link importPeerBundles}'s stateless result: a + * per-instance replay/rollback watermark, and a per-key Sybil cap for brand-new instanceIds. This is the ONE + * function in this module that touches the DB — deliberately kept separate from `importPeerBundles` (which + * stays pure) and called ONLY from the federated background sync job (federated-sync.ts), never from any + * gate-facing path, so the "no path from here to a gate decision" property importPeerBundles documents still + * holds structurally for the pure core. + * + * `peerKeys` must be the SAME allowlist `result.accepted` was already verified against — every bundle in + * `result.accepted` is therefore guaranteed to match one of them (the `null` arm below is defensive only). + */ +export async function applyFederatedPeerWatermarks( + db: D1Database, + result: FederatedImportResult, + peerKeys: readonly string[], + opts: { now?: number; log?: FederatedImportLogger } = {}, +): Promise { + const log = opts.log ?? defaultRejectionLogger; + const now = Number.isFinite(opts.now) ? (opts.now as number) : Date.now(); + const reject = (instanceId: string | null, reason: FederatedRejectionReason): FederatedRejection => { + const rejection: FederatedRejection = { instanceId, reason }; + log(rejection); + return rejection; + }; + + const state = await readFederatedPeerState(db); + for (const [id, entry] of Object.entries(state)) { + if (now - entry.lastSeenAtMs > PEER_STATE_PRUNE_AFTER_MS) delete state[id]; + } + + const accepted: FederatedSignalBundle[] = []; + const rejected: FederatedRejection[] = [...result.rejected]; + for (const bundle of result.accepted) { + const key = matchingFederatedKey(bundle, peerKeys); + /* v8 ignore next 4 -- defensive: every bundle in result.accepted already verified against this exact + peerKeys allowlist inside importPeerBundles, so a re-check here can only fail if the caller passed a + DIFFERENT allowlist than it used to produce `result` — a caller contract violation, not a reachable + runtime state. */ + if (key === null) { + rejected.push(reject(bundle.instanceId, "untrusted_or_tampered")); + continue; + } + const fingerprint = federatedKeyFingerprint(key); + const generatedAtMs = Date.parse(bundle.generatedAt); // already validated finite by importPeerBundles' rejectionFor + const existing = state[bundle.instanceId]; + if (existing) { + if (generatedAtMs <= existing.lastGeneratedAtMs) { + rejected.push(reject(bundle.instanceId, "replayed_or_rollback")); + continue; + } + state[bundle.instanceId] = { keyFingerprint: fingerprint, lastGeneratedAtMs: generatedAtMs, lastSeenAtMs: now, firstSeenAtMs: existing.firstSeenAtMs }; + accepted.push(bundle); + continue; + } + const liveInstancesForKey = Object.values(state).filter((entry) => entry.keyFingerprint === fingerprint).length; + if (liveInstancesForKey >= MAX_INSTANCES_PER_KEY) { + rejected.push(reject(bundle.instanceId, "sybil_cap_exceeded")); + continue; + } + state[bundle.instanceId] = { keyFingerprint: fingerprint, lastGeneratedAtMs: generatedAtMs, lastSeenAtMs: now, firstSeenAtMs: now }; + accepted.push(bundle); + } + + await writeFederatedPeerState(db, state); + return { accepted, rejected }; +} + /** Operator-visible by default. Logs the reason and the opaque instance handle only — never bundle contents, * never a peer key, so a rejection is diagnosable without the log becoming a place secrets leak. */ function defaultRejectionLogger(rejection: FederatedRejection): void { diff --git a/src/queue/job-dispatch.ts b/src/queue/job-dispatch.ts index 706325e6cc..5185bff16f 100644 --- a/src/queue/job-dispatch.ts +++ b/src/queue/job-dispatch.ts @@ -37,6 +37,9 @@ import { runSelfTuneBreaker } from "../review/outcomes-wire"; import { isRagEnabled } from "../review/rag-wire"; import { processSubmitDraft } from "../services/draft"; import { retryFailedRelays } from "../orb/relay"; +import { resolveFederatedIntelligenceManifestOverride, runFederatedPeerSyncTick } from "../orb/federated-benchmark"; +import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; +import { resolveLoopOverSelfRepoFullName } from "../config/loopover-repo-focus-manifest"; import { loadPendingAprRepoTransfers, pollPendingAprRepoTransfers, @@ -500,6 +503,20 @@ export async function processJob(env: Env, message: JobMessage): Promise { // an empty table). Never throws. await retryFailedRelays(env); return; + case "federated-peer-sync": { + // Federated peer-sync tick (#9148/#9166): pulls + trust-gates + persists peer bundles into the + // maintainer-dashboard benchmark cache, and pushes this instance's own bundle to its configured + // collector. Defense-in-depth (same pattern as ops-alerts above): the cron only enqueues this when + // federatedIntelligence.enabled is true, but a stale in-flight job that lands after the operator flips + // it off must still no-op. The cheap override check runs first; the FULL manifest (peerKeys, + // collectorUrl/collectorMode — fields the lightweight override doesn't carry) is only loaded once armed. + const federatedOverride = await resolveFederatedIntelligenceManifestOverride(env); + if (federatedOverride.enabled) { + const federatedManifest = await loadRepoFocusManifest(env, resolveLoopOverSelfRepoFullName(env)); + await runFederatedPeerSyncTick(federatedManifest, env.DB); + } + return; + } /* v8 ignore start -- live-loop wiring: binds the injectable, unit-tested pollPendingAprRepoTransfers (#7741) to its real dependencies. The detection/expiry/pause logic is covered directly in test/unit/orb-apr-repo-transfer.test.ts; this arm is a no-op today (loadPendingAprRepoTransfers fail-empties diff --git a/src/types.ts b/src/types.ts index 9cfb94b37f..8a6a6b6c4f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -299,6 +299,14 @@ export type JobMessage = type: "retry-orb-relay"; requestedBy: "schedule" | "test"; } + | { + // Federated peer-sync tick (#9148/#9166): pulls + trust-gates + persists peer bundles into the + // maintainer-dashboard benchmark cache, AND pushes this instance's own bundle to its configured + // collector. Moves that work off the dashboard's own request path onto a background cadence. Enqueued + // by the cron ONLY when the loopover self-repo's `.loopover.yml federatedIntelligence.enabled` is true. + type: "federated-peer-sync"; + requestedBy: "schedule" | "test"; + } | { // APR repo-transfer acceptance/expiry detection (#7741): resolve every pending APR transfer — probe GitHub, // mark accepted / accepted-and-departed / expired (>7 days), reconcile the per-repo AMS pause. Enqueued by diff --git a/test/unit/federated-benchmark.test.ts b/test/unit/federated-benchmark.test.ts index 68d822c1fe..a59d9a26d1 100644 --- a/test/unit/federated-benchmark.test.ts +++ b/test/unit/federated-benchmark.test.ts @@ -3,7 +3,7 @@ import { createHmac } from "node:crypto"; import { describe, expect, it, vi } from "vitest"; import { createD1Adapter, nodeSqliteDriver } from "../../src/selfhost/d1-adapter"; import { canonicalizeFederatedBundleBody, FEDERATED_BUNDLE_SCHEMA_VERSION, type FederatedSignalBundle } from "../../src/orb/federated-bundle"; -import { buildFederatedBenchmark } from "../../src/orb/federated-benchmark"; +import { buildFederatedBenchmark, refreshFederatedBenchmarkCache, runFederatedPeerSyncTick } from "../../src/orb/federated-benchmark"; import type { FederatedCollectorMode, FocusManifest } from "../../src/signals/focus-manifest"; const URL_OK = "https://collector.example.org/v1/federated"; @@ -13,7 +13,8 @@ const PEER_KEY_B = "b".repeat(64); const UNTRUSTED_KEY = "c".repeat(64); const NOW = Date.parse("2026-07-16T00:00:00Z"); -/** In-memory DB with the tables buildFederatedBundle reads (mirrors federated-bundle.test.ts's makeDb). */ +/** In-memory DB with the tables buildFederatedBundle/the peer-state + benchmark caches read (mirrors + * federated-bundle.test.ts's makeDb). */ function makeDb(): D1Database { const driver = nodeSqliteDriver(new DatabaseSync(":memory:") as never); driver.exec(` @@ -108,9 +109,7 @@ function fetchReturning(bundles: FederatedSignalBundle[]): typeof fetch { describe("buildFederatedBenchmark() — not opted in", () => { it("returns null and touches neither the database nor the network for an absent/false manifest", async () => { - const fetchSpy = vi.fn(); - expect(await buildFederatedBenchmark(manifest({ enabled: false }), untouchableDb(), { fetchFn: fetchSpy })).toBeNull(); - expect(fetchSpy).not.toHaveBeenCalled(); + expect(await buildFederatedBenchmark(manifest({ enabled: false }), untouchableDb())).toBeNull(); }); it("returns null for a null manifest", async () => { @@ -118,12 +117,12 @@ describe("buildFederatedBenchmark() — not opted in", () => { }); }); -describe("buildFederatedBenchmark() — opted in, no peer data yet", () => { - it("returns local precision with peerCount 0 and a null median when no collector is configured", async () => { +describe("buildFederatedBenchmark() — opted in, no sync tick has run yet (cache miss)", () => { + it("returns local precision live, with peerCount 0 and a null median, when the cache is empty", async () => { const db = makeDb(); for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); - const result = await buildFederatedBenchmark(manifest({ collectorUrl: null }), db, { now: NOW }); + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); expect(result).not.toBeNull(); expect(result?.localMergePrecision).toBe(1); @@ -132,87 +131,165 @@ describe("buildFederatedBenchmark() — opted in, no peer data yet", () => { expect(result?.generatedAt).toBe("2026-07-16T00:00:00.000Z"); }); - it("returns peerCount 0 when every pulled bundle is rejected by trust-gating (untrusted key)", async () => { + it("honors an explicit windowDays override, narrowing the local calibration window", async () => { const db = makeDb(); - for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); - const fetchFn = fetchReturning([signedWith(UNTRUSTED_KEY)]); + // Resolved 30 days before `now` — inside the default 90-day window but outside a 7-day override. + for (let pr = 1; pr <= 5; pr++) { + await resolved(db, pr, {}); + await db.prepare("UPDATE review_audit SET created_at = '2026-06-16T12:00:00Z' WHERE target_id = ?").bind(`owner/repo#${pr}`).run(); + } - const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn }); + const wide = await buildFederatedBenchmark(manifest(), db, { now: NOW, windowDays: 90 }); + const narrow = await buildFederatedBenchmark(manifest(), db, { now: NOW, windowDays: 7 }); - expect(result?.peerCount).toBe(0); - expect(result?.peerMedianMergePrecision).toBeNull(); + expect(wide?.localMergePrecision).toBe(1); + expect(narrow?.localMergePrecision).toBeNull(); }); - it("excludes an accepted peer bundle whose own mergePrecision is null (peer below its own MIN_DECIDED)", async () => { + it("uses Date.now() for generatedAt when opts.now is not provided", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date(NOW)); + try { + const db = makeDb(); + const result = await buildFederatedBenchmark(manifest(), db, {}); + expect(result?.generatedAt).toBe("2026-07-16T00:00:00.000Z"); + } finally { + vi.useRealTimers(); + } + }); + + it("never touches the network — it only reads a system_flags cache row for the peer half", async () => { + // untouchableDb throws on ANY property access, including .prepare — proving the request path is DB-only + // (a single local read for the cache, plus buildFederatedBundle's own local query) and never calls fetch. + const fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + try { + const db = makeDb(); + await buildFederatedBenchmark(manifest(), db, { now: NOW }); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + vi.unstubAllGlobals(); + } + }); +}); + +describe("buildFederatedBenchmark() — reads whatever refreshFederatedBenchmarkCache last wrote", () => { + it("reflects a fresh sync tick's median + peerCount on the very next read", async () => { const db = makeDb(); for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); - const fetchFn = fetchReturning([signedWith(PEER_KEY_A, { mergePrecision: null })]); + const fetchFn = fetchReturning([signedWith(PEER_KEY_A, { mergePrecision: 0.6 })]); - const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn }); + await refreshFederatedBenchmarkCache(manifest(), db, { now: NOW, fetchFn }); + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); - expect(result?.peerCount).toBe(0); - expect(result?.peerMedianMergePrecision).toBeNull(); + expect(result?.localMergePrecision).toBe(1); + expect(result?.peerMedianMergePrecision).toBe(0.6); + expect(result?.peerCount).toBe(1); }); -}); -describe("buildFederatedBenchmark() — opted in, local precision below MIN_DECIDED", () => { - it("reports a null local precision but still computes the peer median", async () => { + it("reports a null local precision (below MIN_DECIDED) while still surfacing a cached peer median", async () => { const db = makeDb(); await resolved(db, 1); // only 1 decided PR, below MIN_DECIDED (5) const fetchFn = fetchReturning([signedWith(PEER_KEY_A, { mergePrecision: 0.6 })]); - const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn }); + await refreshFederatedBenchmarkCache(manifest(), db, { now: NOW, fetchFn }); + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); expect(result?.localMergePrecision).toBeNull(); expect(result?.peerMedianMergePrecision).toBe(0.6); expect(result?.peerCount).toBe(1); }); + + it("degrades to a null median / peerCount 0 when the cached row is corrupt JSON", async () => { + const db = makeDb(); + await db.prepare("INSERT INTO system_flags (key, value) VALUES ('orb:federated_benchmark_cache', 'not-json')").run(); + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); + expect(result?.peerMedianMergePrecision).toBeNull(); + expect(result?.peerCount).toBe(0); + }); }); -describe("buildFederatedBenchmark() — opted in, real peer comparison", () => { +describe("refreshFederatedBenchmarkCache() — the background-tick write path (#9148)", () => { + it("is a no-op (cache untouched) when not opted in", async () => { + const db = makeDb(); + await db.prepare("INSERT INTO system_flags (key, value) VALUES ('orb:federated_benchmark_cache', ?)").bind(JSON.stringify({ peerMedianMergePrecision: 0.42, peerCount: 3, refreshedAt: "x" })).run(); + await refreshFederatedBenchmarkCache(manifest({ enabled: false }), db, { now: NOW }); + const row = await db.prepare("SELECT value FROM system_flags WHERE key = 'orb:federated_benchmark_cache'").first<{ value: string }>(); + expect(JSON.parse(row?.value ?? "{}")).toMatchObject({ peerMedianMergePrecision: 0.42, peerCount: 3 }); // untouched, not overwritten with an empty result + }); + + it("caches peerCount 0 and a null median when every pulled bundle is rejected by trust-gating", async () => { + const db = makeDb(); + const fetchFn = fetchReturning([signedWith(UNTRUSTED_KEY)]); + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn }); + const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW }); + expect(result?.peerCount).toBe(0); + expect(result?.peerMedianMergePrecision).toBeNull(); + }); + + it("excludes an accepted peer bundle whose own mergePrecision is null (peer below its own MIN_DECIDED)", async () => { + const db = makeDb(); + const fetchFn = fetchReturning([signedWith(PEER_KEY_A, { mergePrecision: null })]); + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn }); + const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW }); + expect(result?.peerCount).toBe(0); + expect(result?.peerMedianMergePrecision).toBeNull(); + }); + it("computes the median (not the mean) across every accepted peer, ignoring an untrusted one mixed in", async () => { const db = makeDb(); - for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); const fetchFn = fetchReturning([ signedWith(PEER_KEY_A, { instanceId: "peer-a", mergePrecision: 0.5 }), signedWith(PEER_KEY_B, { instanceId: "peer-b", mergePrecision: 0.9 }), signedWith(UNTRUSTED_KEY, { instanceId: "peer-attacker", mergePrecision: 0.01 }), ]); - const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW, fetchFn }); - + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW, fetchFn }); // Median of [0.5, 0.9] (the untrusted 0.01 is rejected, not merely a low outlier) is 0.5 under this // module's nearest-rank percentile(50) (analytics.ts: idx = ceil(0.5*2)-1 = 0) — pinning the real // cross-module contract, not a re-derivation. + const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A, PEER_KEY_B] }), db, { now: NOW }); expect(result?.peerCount).toBe(2); expect(result?.peerMedianMergePrecision).toBe(0.5); - expect(result?.localMergePrecision).toBe(1); }); - it("honors an explicit windowDays override, narrowing the local calibration window", async () => { + it("a second tick's median REPLACES the first's, rather than merging the two", async () => { const db = makeDb(); - // Resolved 30 days before `now` — inside the default 90-day window but outside a 7-day override. - for (let pr = 1; pr <= 5; pr++) { - await resolved(db, pr, {}); - await db.prepare("UPDATE review_audit SET created_at = '2026-06-16T12:00:00Z' WHERE target_id = ?").bind(`owner/repo#${pr}`).run(); - } + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW, fetchFn: fetchReturning([signedWith(PEER_KEY_A, { instanceId: "peer-a", mergePrecision: 0.2 })]) }); + await refreshFederatedBenchmarkCache(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW + 60_000, fetchFn: fetchReturning([signedWith(PEER_KEY_A, { instanceId: "peer-a", generatedAt: new Date(NOW + 60_000).toISOString(), mergePrecision: 0.9 })]) }); + const result = await buildFederatedBenchmark(manifest({ peerKeys: [PEER_KEY_A] }), db, { now: NOW }); + expect(result?.peerMedianMergePrecision).toBe(0.9); + }); +}); - const wide = await buildFederatedBenchmark(manifest({ collectorUrl: null }), db, { now: NOW, windowDays: 90 }); - const narrow = await buildFederatedBenchmark(manifest({ collectorUrl: null }), db, { now: NOW, windowDays: 7 }); +describe("runFederatedPeerSyncTick() — the full tick: cache refresh + own-bundle push (#9148/#9166)", () => { + it("refreshes the cache AND posts this instance's own bundle to the configured collector", async () => { + const db = makeDb(); + for (let pr = 1; pr <= 5; pr++) await resolved(db, pr); + const posted: { url: string }[] = []; + const fetchFn = (async (url: string, init?: RequestInit) => { + posted.push({ url: String(url) }); + if (init?.method === "POST") return new Response("ok"); + return Response.json([signedWith(PEER_KEY_A, { mergePrecision: 0.6 })]); + }) as unknown as typeof fetch; - expect(wide?.localMergePrecision).toBe(1); - expect(narrow?.localMergePrecision).toBeNull(); + await runFederatedPeerSyncTick(manifest(), db, { now: NOW, fetchFn }); + + expect(posted.some((p) => p.url === URL_OK)).toBe(true); // both push and pull hit the same configured collector + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); + expect(result?.peerMedianMergePrecision).toBe(0.6); }); - it("uses Date.now() for generatedAt when opts.now is not provided", async () => { - vi.useFakeTimers(); - vi.setSystemTime(new Date(NOW)); - try { - const db = makeDb(); - const result = await buildFederatedBenchmark(manifest({ collectorUrl: null }), db, {}); - expect(result?.generatedAt).toBe("2026-07-16T00:00:00.000Z"); - } finally { - vi.useRealTimers(); - } + it("a push failure never skips the pull-side cache refresh (allSettled, not all)", async () => { + const db = makeDb(); + const fetchFn = (async (_url: string, init?: RequestInit) => { + if (init?.method === "POST") throw new Error("collector down"); + return Response.json([signedWith(PEER_KEY_A, { mergePrecision: 0.6 })]); + }) as unknown as typeof fetch; + + await runFederatedPeerSyncTick(manifest(), db, { now: NOW, fetchFn, maxAttempts: 1, sleepFn: () => Promise.resolve() }); + + const result = await buildFederatedBenchmark(manifest(), db, { now: NOW }); + expect(result?.peerMedianMergePrecision).toBe(0.6); // pull side still completed and cached }); }); diff --git a/test/unit/federated-import.test.ts b/test/unit/federated-import.test.ts index 17c583c4c3..e911c569e7 100644 --- a/test/unit/federated-import.test.ts +++ b/test/unit/federated-import.test.ts @@ -3,11 +3,14 @@ import { describe, expect, it, vi } from "vitest"; import { canonicalizeFederatedBundleBody, FEDERATED_BUNDLE_SCHEMA_VERSION, type FederatedSignalBundle } from "../../src/orb/federated-bundle"; import { + applyFederatedPeerWatermarks, importPeerBundles, isFederatedImportEnabled, + matchingFederatedKey, verifyFederatedBundle, type FederatedRejection, } from "../../src/orb/federated-import"; +import { createTestEnv, type TestD1Database } from "../helpers/d1"; import type { FocusManifest } from "../../src/signals/focus-manifest"; // Fake 64-hex keys — the shape generateAnonSecret produces. Not secrets: locally-invented test fixtures. @@ -15,10 +18,16 @@ const PEER_KEY_A = "a".repeat(64); const PEER_KEY_B = "b".repeat(64); const UNTRUSTED_KEY = "c".repeat(64); +// Computed at call time (not a fixed literal) so every test bundle is fresh relative to whatever `now` +// importPeerBundles defaults to (real Date.now()) — the freshness gate (#9148) would otherwise reject a +// fixed past date as the test suite ages. Tests that specifically exercise staleness pass an explicit +// `generatedAt` (and/or an explicit `now` opt) instead of relying on this default. +const FRESH_GENERATED_AT = () => new Date(Date.now() - 60_000).toISOString(); + const body = (over: Partial = {}) => ({ schemaVersion: FEDERATED_BUNDLE_SCHEMA_VERSION, instanceId: "abc123def4567890", - generatedAt: "2026-02-01T00:00:00.000Z", + generatedAt: FRESH_GENERATED_AT(), windowDays: 90, decided: 40, mergePrecision: 0.9, @@ -218,4 +227,182 @@ describe("importPeerBundles (#6480)", () => { expect(importPeerBundles(null, [signedWith(PEER_KEY_A)], { log: () => undefined }).rejected[0]!.reason).toBe("not_opted_in"); expect(importPeerBundles(undefined, [], { log: () => undefined })).toEqual({ accepted: [], rejected: [] }); }); + + it("#9148: keeps only the LAST bundle per instanceId within a batch (Sybil dedup)", () => { + const first = signedWith(PEER_KEY_A, { mergePrecision: 0.5 }); + const second = signedWith(PEER_KEY_A, { mergePrecision: 0.99 }); // same instanceId, re-signed + const result = importPeerBundles(manifest(), [first, second], { log: () => undefined }); + expect(result.accepted).toEqual([second]); + expect(result.rejected).toEqual([]); // the superseded duplicate is silently dropped, not logged as a rejection + }); + + it("#9148: rejects every numeric field outside its valid range", () => { + const cases: Partial[] = [ + { mergePrecision: -0.01 }, + { mergePrecision: 1.01 }, + { closePrecision: -1 }, + { fpRate: 2 }, + { fnRate: -0.5 }, + { reversalRate: 1.5 }, + { slopRate: -0.1 }, + { copycatRate: 1.5 }, + { decided: -1 }, + { windowDays: 0 }, + { windowDays: -90 }, + { cycleP50Ms: -1 }, + { cycleP95Ms: -1 }, + { cycleP50Ms: 5000, cycleP95Ms: 1000 }, // p50 can never exceed p95 of the same sample + ]; + for (const over of cases) { + const result = importPeerBundles(manifest(), [signedWith(PEER_KEY_A, over)], { log: () => undefined }); + expect(result.rejected[0]?.reason, JSON.stringify(over)).toBe("out_of_range"); + } + }); + + it("#9148: accepts the boundary values 0 and 1 for a unit-rate field (inclusive range)", () => { + const zero = importPeerBundles(manifest(), [signedWith(PEER_KEY_A, { instanceId: "i-zero", mergePrecision: 0, fpRate: 0 })], { log: () => undefined }); + expect(zero.accepted).toHaveLength(1); + const one = importPeerBundles(manifest(), [signedWith(PEER_KEY_A, { instanceId: "i-one", mergePrecision: 1, fpRate: 1 })], { log: () => undefined }); + expect(one.accepted).toHaveLength(1); + }); + + it("#9148: rejects a windowDays that does not match the local instance's own resolved window", () => { + const result = importPeerBundles(manifest(), [signedWith(PEER_KEY_A, { windowDays: 30 })], { log: () => undefined }, ); + expect(result.rejected).toEqual([{ instanceId: "abc123def4567890", reason: "window_mismatch" }]); + }); + + it("#9148: honors an explicit localWindowDays override instead of the DEFAULT_WINDOW_DAYS default", () => { + const bundle = signedWith(PEER_KEY_A, { windowDays: 30 }); + const result = importPeerBundles(manifest(), [bundle], { log: () => undefined, localWindowDays: 30 }); + expect(result.accepted).toEqual([bundle]); + }); + + it("#9148: rejects an unparseable generatedAt", () => { + const result = importPeerBundles(manifest(), [signedWith(PEER_KEY_A, { generatedAt: "not-a-date" })], { log: () => undefined }); + expect(result.rejected).toEqual([{ instanceId: "abc123def4567890", reason: "stale_or_future" }]); + }); + + it("#9148: rejects a bundle older than the max age, and one too far in the future — accepts one just inside each edge", () => { + const now = Date.parse("2026-03-01T00:00:00.000Z"); + const eightDaysStale = signedWith(PEER_KEY_A, { generatedAt: "2026-02-21T00:00:00.000Z" }); // > 7 days old + const sixDaysStale = signedWith(PEER_KEY_A, { generatedAt: "2026-02-23T00:00:00.000Z" }); // < 7 days old + const tenMinutesFuture = signedWith(PEER_KEY_A, { generatedAt: "2026-03-01T00:10:00.000Z" }); // > 5 min skew + const oneMinuteFuture = signedWith(PEER_KEY_A, { generatedAt: "2026-03-01T00:01:00.000Z" }); // < 5 min skew + expect(importPeerBundles(manifest(), [eightDaysStale], { log: () => undefined, now }).rejected[0]?.reason).toBe("stale_or_future"); + expect(importPeerBundles(manifest(), [sixDaysStale], { log: () => undefined, now }).accepted).toEqual([sixDaysStale]); + expect(importPeerBundles(manifest(), [tenMinutesFuture], { log: () => undefined, now }).rejected[0]?.reason).toBe("stale_or_future"); + expect(importPeerBundles(manifest(), [oneMinuteFuture], { log: () => undefined, now }).accepted).toEqual([oneMinuteFuture]); + }); + + it("#9148: enforces MIN_DECIDED receiver-side, regardless of what the sender claims", () => { + // A hostile/buggy peer self-reporting a real mergePrecision despite decided being below MIN_DECIDED (5). + const underclaimed = signedWith(PEER_KEY_A, { decided: 3, mergePrecision: 1.0 }); + const result = importPeerBundles(manifest(), [underclaimed], { log: () => undefined }); + expect(result.rejected).toEqual([{ instanceId: "abc123def4567890", reason: "below_min_decided" }]); + }); + + it("#9148: accepts a decided count exactly at MIN_DECIDED", () => { + const bundle = signedWith(PEER_KEY_A, { decided: 5 }); + expect(importPeerBundles(manifest(), [bundle], { log: () => undefined }).accepted).toEqual([bundle]); + }); +}); + +describe("matchingFederatedKey (#9148)", () => { + it("returns the specific key that verified, not just a boolean", () => { + expect(matchingFederatedKey(signedWith(PEER_KEY_B), [PEER_KEY_A, PEER_KEY_B])).toBe(PEER_KEY_B); + }); + + it("returns null when no allowlisted key verifies", () => { + expect(matchingFederatedKey(signedWith(UNTRUSTED_KEY), [PEER_KEY_A, PEER_KEY_B])).toBeNull(); + }); +}); + +describe("applyFederatedPeerWatermarks (#9148, the persisted gates)", () => { + const env = () => createTestEnv(); + const db = (e: Env) => e.DB as unknown as TestD1Database; + const okResult = (bundles: FederatedSignalBundle[]): { accepted: FederatedSignalBundle[]; rejected: [] } => ({ accepted: bundles, rejected: [] }); + + it("admits a brand-new instanceId and persists its watermark", async () => { + const e = env(); + const bundle = signedWith(PEER_KEY_A); + const result = await applyFederatedPeerWatermarks(e.DB, okResult([bundle]), [PEER_KEY_A], { now: Date.parse(bundle.generatedAt) }); + expect(result.accepted).toEqual([bundle]); + expect(result.rejected).toEqual([]); + const row = await db(e).prepare("SELECT value FROM system_flags WHERE key = 'orb:federated_peer_state'").first<{ value: string }>(); + expect(JSON.parse(row?.value ?? "{}")).toHaveProperty(bundle.instanceId); + }); + + it("admits a newer bundle for an already-known instanceId, refreshing the watermark", async () => { + const e = env(); + const first = signedWith(PEER_KEY_A, { generatedAt: "2026-03-01T00:00:00.000Z" }); + await applyFederatedPeerWatermarks(e.DB, okResult([first]), [PEER_KEY_A], { now: Date.parse(first.generatedAt) }); + const second = signedWith(PEER_KEY_A, { generatedAt: "2026-03-02T00:00:00.000Z" }); + const result = await applyFederatedPeerWatermarks(e.DB, okResult([second]), [PEER_KEY_A], { now: Date.parse(second.generatedAt) }); + expect(result.accepted).toEqual([second]); + }); + + it("rejects a replay: the same or an older generatedAt for an instanceId already on record", async () => { + const e = env(); + const fresh = signedWith(PEER_KEY_A, { generatedAt: "2026-03-05T00:00:00.000Z" }); + await applyFederatedPeerWatermarks(e.DB, okResult([fresh]), [PEER_KEY_A], { now: Date.parse(fresh.generatedAt) }); + const replay = signedWith(PEER_KEY_A, { generatedAt: "2026-03-01T00:00:00.000Z" }); // same instanceId, OLDER generatedAt + const result = await applyFederatedPeerWatermarks(e.DB, okResult([replay]), [PEER_KEY_A], { now: Date.parse(fresh.generatedAt) }); + expect(result.accepted).toEqual([]); + expect(result.rejected).toEqual([{ instanceId: "abc123def4567890", reason: "replayed_or_rollback" }]); + }); + + it("caps a single key at MAX_INSTANCES_PER_KEY distinct instanceIds, admitting the first N and rejecting the rest", async () => { + const e = env(); + let accepted: FederatedSignalBundle[] = []; + let lastRejectedReason: string | undefined; + for (let i = 0; i < 11; i += 1) { + const bundle = signedWith(PEER_KEY_A, { instanceId: `sybil-instance-${i}` }); + const result = await applyFederatedPeerWatermarks(e.DB, okResult([bundle]), [PEER_KEY_A], { now: Date.parse(bundle.generatedAt) }); + accepted = accepted.concat(result.accepted); + if (result.rejected.length > 0) lastRejectedReason = result.rejected[0]?.reason; + } + expect(accepted).toHaveLength(10); // MAX_INSTANCES_PER_KEY + expect(lastRejectedReason).toBe("sybil_cap_exceeded"); + }); + + it("a DIFFERENT verifying key gets its own independent cap", async () => { + const e = env(); + for (let i = 0; i < 10; i += 1) { + const bundle = signedWith(PEER_KEY_A, { instanceId: `key-a-instance-${i}` }); + await applyFederatedPeerWatermarks(e.DB, okResult([bundle]), [PEER_KEY_A, PEER_KEY_B], { now: Date.parse(bundle.generatedAt) }); + } + // PEER_KEY_A is now at its cap, but PEER_KEY_B has contributed nothing yet — a new instance under B must + // still be admitted, proving the cap is scoped per-key rather than a single shared global counter. + const underB = signedWith(PEER_KEY_B, { instanceId: "key-b-instance-0" }); + const result = await applyFederatedPeerWatermarks(e.DB, okResult([underB]), [PEER_KEY_A, PEER_KEY_B], { now: Date.parse(underB.generatedAt) }); + expect(result.accepted).toEqual([underB]); + }); + + it("prunes a peer-state entry that hasn't been refreshed in a very long time, freeing its key's cap slot", async () => { + const e = env(); + const veryOldEntry = { "stale-instance": { keyFingerprint: "x".repeat(64), lastGeneratedAtMs: 0, lastSeenAtMs: 0, firstSeenAtMs: 0 } }; + await db(e).prepare("INSERT INTO system_flags (key, value, updated_at) VALUES ('orb:federated_peer_state', ?, CURRENT_TIMESTAMP)").bind(JSON.stringify(veryOldEntry)).run(); + const bundle = signedWith(PEER_KEY_A, { instanceId: "fresh-instance" }); + const farFuture = Date.parse(bundle.generatedAt) + 500 * 86_400_000; // well past PEER_STATE_PRUNE_AFTER_MS + await applyFederatedPeerWatermarks(e.DB, okResult([bundle]), [PEER_KEY_A], { now: farFuture }); + const row = await db(e).prepare("SELECT value FROM system_flags WHERE key = 'orb:federated_peer_state'").first<{ value: string }>(); + expect(JSON.parse(row?.value ?? "{}")).not.toHaveProperty("stale-instance"); // pruned, not just superseded + }); + + it("degrades to treating every instanceId as new when the persisted state is corrupt JSON", async () => { + const e = env(); + await db(e).prepare("INSERT INTO system_flags (key, value, updated_at) VALUES ('orb:federated_peer_state', 'not-json', CURRENT_TIMESTAMP)").run(); + const bundle = signedWith(PEER_KEY_A); + const result = await applyFederatedPeerWatermarks(e.DB, okResult([bundle]), [PEER_KEY_A], { now: Date.parse(bundle.generatedAt) }); + expect(result.accepted).toEqual([bundle]); // fail-safe: corrupt cache never blocks a legitimate peer + }); + + it("passes through prior rejections untouched alongside its own", async () => { + const e = env(); + const priorRejection: FederatedRejection = { instanceId: "already-rejected", reason: "malformed" }; + const bundle = signedWith(PEER_KEY_A); + const result = await applyFederatedPeerWatermarks(e.DB, { accepted: [bundle], rejected: [priorRejection] }, [PEER_KEY_A], { now: Date.parse(bundle.generatedAt) }); + expect(result.rejected).toContainEqual(priorRejection); + expect(result.accepted).toEqual([bundle]); + }); }); diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index eb0f076786..2056d24ecc 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -4,11 +4,13 @@ import { recordGitHubRateLimitObservation } from "../../src/db/repositories"; import { scheduledEnqueueDelaySeconds } from "../../src/selfhost/queue-common"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { clearOpsManifestOverrideCacheForTest } from "../../src/review/ops-wire"; +import { clearFederatedIntelligenceManifestOverrideCacheForTest } from "../../src/orb/federated-benchmark"; import { createTestEnv } from "../helpers/d1"; describe("worker entrypoint", () => { beforeEach(() => { clearOpsManifestOverrideCacheForTest(); + clearFederatedIntelligenceManifestOverrideCacheForTest(); }); afterEach(() => { vi.restoreAllMocks(); @@ -1002,6 +1004,37 @@ describe("worker entrypoint", () => { expect(sent.some((m) => m.type === "reconcile-active-review-tracking")).toBe(false); }); + it("enqueues federated-peer-sync every 10 minutes ONLY when the loopover self-repo manifest opts in (#9148) — absent-manifest default is byte-identical", async () => { + const sentFor = async (enabled?: boolean, isoTime = "2026-05-25T05:10:00.000Z"): Promise> => { + clearFederatedIntelligenceManifestOverrideCacheForTest(); // each sub-case below is a fresh env; don't let the prior case's cached override leak in + const sent: Array = []; + const env = createTestEnv({ LOOPOVER_DRIFT_ISSUE_REPO: "JSONbored/loopover", JOBS: { async send(message: import("../../src/types").JobMessage) { sent.push(message); } } as unknown as Queue }); + if (enabled !== undefined) await upsertRepoFocusManifest(env, "JSONbored/loopover", { federatedIntelligence: { enabled } }); + const waitUntil: Promise[] = []; + await worker.scheduled(controllerFor(isoTime), env, executionContext(waitUntil)); + await Promise.all(waitUntil); + return sent; + }; + + // No manifest block at all (absent federatedIntelligence, the fleet-wide default) → never enqueued. + expect((await sentFor(undefined)).some((m) => m.type === "federated-peer-sync")).toBe(false); + // Manifest present but explicitly disabled → still never enqueued. + expect((await sentFor(false)).some((m) => m.type === "federated-peer-sync")).toBe(false); + // Opted in, on a 10-minute boundary → exactly one federated-peer-sync job. + const on = await sentFor(true); + expect(on.filter((m) => m.type === "federated-peer-sync")).toEqual([{ type: "federated-peer-sync", requestedBy: "schedule" }]); + }); + + it("does NOT enqueue federated-peer-sync outside the 10-minute window even when opted in", async () => { + const sent: Array = []; + const env = createTestEnv({ LOOPOVER_DRIFT_ISSUE_REPO: "JSONbored/loopover", JOBS: { async send(message: import("../../src/types").JobMessage) { sent.push(message); } } as unknown as Queue }); + await upsertRepoFocusManifest(env, "JSONbored/loopover", { federatedIntelligence: { enabled: true } }); + const waitUntil: Promise[] = []; + await worker.scheduled(controllerFor("2026-05-25T05:14:00.000Z"), env, executionContext(waitUntil)); // not a 10-minute boundary + await Promise.all(waitUntil); + expect(sent.some((m) => m.type === "federated-peer-sync")).toBe(false); + }); + it("enqueues selftune hourly only when LOOPOVER_REVIEW_SELFTUNE is ON", async () => { const sentFor = async ( selfTuneFlag?: string, diff --git a/test/unit/job-dispatch.test.ts b/test/unit/job-dispatch.test.ts index 8a6e95e59f..f2a51b4032 100644 --- a/test/unit/job-dispatch.test.ts +++ b/test/unit/job-dispatch.test.ts @@ -3,6 +3,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { processJob } from "../../src/queue/job-dispatch"; import { createTestEnv } from "../helpers/d1"; import { upsertInstallation, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { clearFederatedIntelligenceManifestOverrideCacheForTest } from "../../src/orb/federated-benchmark"; import type { JobMessage } from "../../src/types"; describe("processJob unknown job type (#5836)", () => { @@ -41,6 +43,34 @@ describe("processJob unknown job type (#5836)", () => { }); }); +describe("processJob federated-peer-sync (#9148/#9166)", () => { + afterEach(() => { + clearFederatedIntelligenceManifestOverrideCacheForTest(); + vi.unstubAllGlobals(); + }); + + it("is a no-op — never writes the peer-benchmark cache — when the loopover self-repo manifest has not opted in", async () => { + const env = createTestEnv({ LOOPOVER_DRIFT_ISSUE_REPO: "JSONbored/loopover" }); + // The manifest override lookup itself may still fall through to a network read when nothing is cached + // (the same "config-as-code override" pattern every sibling scheduled job uses, e.g. resolveOpsManifestOverride) + // — stub it to a harmless empty response so this test exercises the GATE, not GitHub reachability. + vi.stubGlobal("fetch", vi.fn(async () => new Response("null", { status: 200 }))); + await expect(processJob(env, { type: "federated-peer-sync", requestedBy: "schedule" } as JobMessage)).resolves.toBeUndefined(); + const row = await env.DB.prepare("SELECT 1 AS x FROM system_flags WHERE key = 'orb:federated_benchmark_cache'").first(); + expect(row ?? null).toBeNull(); + }); + + it("re-checks the FULL manifest at dispatch time and runs the sync tick when opted in, defense-in-depth against a stale enqueue", async () => { + const env = createTestEnv({ LOOPOVER_DRIFT_ISSUE_REPO: "JSONbored/loopover" }); + // No collectorUrl configured, so both push and pull resolve to a no-op with zero fetch calls — this proves + // the job runs the real tick (which writes the peer cache) without needing a live network mock. + await upsertRepoFocusManifest(env, "JSONbored/loopover", { federatedIntelligence: { enabled: true } }); + await expect(processJob(env, { type: "federated-peer-sync", requestedBy: "schedule" } as JobMessage)).resolves.toBeUndefined(); + const row = await env.DB.prepare("SELECT value FROM system_flags WHERE key = 'orb:federated_benchmark_cache'").first<{ value: string }>(); + expect(JSON.parse(row?.value ?? "{}")).toMatchObject({ peerCount: 0, peerMedianMergePrecision: null }); + }); +}); + describe("processJob backfill-registered-repos fan-out isolation (#8355)", () => { afterEach(() => { vi.restoreAllMocks(); From 4a88ffa563e991097b45d91de90bd685b6e996ae Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:57:11 -0700 Subject: [PATCH 5/6] fix(orb): revoke-on-rotate and a self-hoster-reachable revoke path for broker enrollments (#9149) issueOrbEnrollment was a bare INSERT: it never revoked, updated, or even counted sibling rows for the same installation_id, so re-running the install flow after a secret leak minted a SECOND simultaneously-valid secret and left the leaked one working forever. The only revoke path sat behind INTERNAL_JOB_TOKEN, unreachable by a self-hosting maintainer. - issueOrbEnrollment takes an optional { rotate: true }: when set, every prior live enrollment for the installation is revoked before the new one is minted. Defaults to false (append) so a blue/green container swap can still rely on two live enrollments briefly overlapping (#9150's sibling fix is what makes that overlap safe, not this one). - The OAuth landing page's callback now recognizes state=: rotate / :revoke (installation_id has no channel back through a bare GitHub login/oauth/authorize bounce other than state) -- re-proving admin-of-installation via the SAME verifyInstallationAdmin gate the enrollment flow already uses, so a maintainer can rotate or revoke without operator involvement. Revoke deliberately skips the suspended/self-enrollment-disabled checks: locking down a leaked secret should never be blocked by unrelated administrative state. - The secret page now surfaces how many enrollments are live for the installation and links to the rotate/revoke actions; its response sets Cache-Control: no-store (it was previously a cacheable GET response rendering a plaintext secret). - The internal installations list and enrollment-issue routes gained the same live-enrollment-count visibility and an optional rotate flag, for operator-issued parity with the OAuth self-service path. --- src/api/routes.ts | 22 ++++-- src/orb/broker.ts | 34 ++++++++- src/orb/oauth.ts | 92 ++++++++++++++++++++++--- test/integration/orb-broker.test.ts | 92 +++++++++++++++++++++++++ test/integration/orb-oauth.test.ts | 91 ++++++++++++++++++++++++ test/integration/orb-onboarding.test.ts | 25 +++++++ 6 files changed, 340 insertions(+), 16 deletions(-) diff --git a/src/api/routes.ts b/src/api/routes.ts index c5afeba4dc..65547d40f3 100644 --- a/src/api/routes.ts +++ b/src/api/routes.ts @@ -166,6 +166,7 @@ import { isOrbBrokerEnabled, issueOrbEnrollment, issueOrbStoredSecret, + ORB_SECRET_TYPE_GITHUB_TOKEN, ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL, revokeOrbEnrollment, } from "../orb/broker"; @@ -4579,15 +4580,22 @@ export function createApp() { // records lands at registered=0; only REGISTERED ones count toward the global public counter (getOrbGlobalStats) // and are eligible for token brokering. Bearer-gated by the `/v1/internal/*` middleware (INTERNAL_JOB_TOKEN). The // list shows pending + registered installs so an operator knows what they're opting in. + // + // liveEnrollmentCount (#9149): how many enrollment secrets are currently live ('enrolled', not revoked) for + // this install — an operator previously had no way to see that a re-enrollment (issueOrbEnrollment was a bare + // INSERT, never revoking) had accumulated a second standing credential. A correlated subquery rather than a + // JOIN: each installation has at most a handful of enrollment rows, so this stays cheap without reshaping the + // one-row-per-installation result set a GROUP BY would require. app.get("/v1/internal/orb/installations", async (c) => { const rows = await c.env.DB .prepare( `SELECT installation_id AS installationId, account_login AS accountLogin, account_type AS accountType, repository_selection AS repositorySelection, registered, suspended_at AS suspendedAt, - removed_at AS removedAt, first_seen_at AS firstSeenAt, last_event_at AS lastEventAt + removed_at AS removedAt, first_seen_at AS firstSeenAt, last_event_at AS lastEventAt, + (SELECT COUNT(*) FROM orb_enrollments oe WHERE oe.installation_id = orb_github_installations.installation_id AND oe.state = 'enrolled' AND oe.revoked_at IS NULL) AS liveEnrollmentCount FROM orb_github_installations ORDER BY last_event_at DESC`, ) - .all<{ installationId: number; accountLogin: string | null; accountType: string | null; repositorySelection: string | null; registered: number; suspendedAt: string | null; removedAt: string | null; firstSeenAt: string; lastEventAt: string }>(); + .all<{ installationId: number; accountLogin: string | null; accountType: string | null; repositorySelection: string | null; registered: number; suspendedAt: string | null; removedAt: string | null; firstSeenAt: string; lastEventAt: string; liveEnrollmentCount: number }>(); return c.json({ installations: (rows.results ?? []).map((r) => ({ ...r, registered: r.registered === 1 })) }); }); @@ -4623,9 +4631,15 @@ export function createApp() { // secret issuance path control-plane's hosted provisioning core (#7180/#8066) calls instead, for a credential // that already exists (a tenant's Postgres connection string) rather than a GitHub installation to bind. // `installationId` is irrelevant to that path (see issueOrbStoredSecret's own header comment for why). + // + // Also accepts an optional `{ rotate: true }` (#9149): when set, every PRIOR live enrollment for this + // installation is revoked before the new one is minted, mirroring the OAuth landing page's `state=:rotate` + // flow (oauth.ts) for the operator-issued path. Defaults to false (append, not replace) -- unchanged behavior + // for every existing caller, since a blue/green container swap legitimately relies on two live enrollments + // briefly overlapping (see #9150's sibling fix, which makes that overlap safe rather than assuming away). app.post("/v1/internal/orb/enrollments", async (c) => { if (!isOrbBrokerEnabled(c.env)) return c.json({ error: "not_found" }, 404); - const payload = (await c.req.json().catch(() => null)) as { installationId?: unknown; secretType?: unknown; secretValue?: unknown } | null; + const payload = (await c.req.json().catch(() => null)) as { installationId?: unknown; secretType?: unknown; secretValue?: unknown; rotate?: unknown } | null; if (payload?.secretType === ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL) { const secretValue = typeof payload.secretValue === "string" ? payload.secretValue : ""; const result = await issueOrbStoredSecret(c.env, ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL, secretValue); @@ -4634,7 +4648,7 @@ export function createApp() { } const installationId = Number(payload?.installationId); if (!Number.isInteger(installationId) || installationId <= 0) return c.json({ error: "installationId required" }, 400); - const result = await issueOrbEnrollment(c.env, installationId); + const result = await issueOrbEnrollment(c.env, installationId, undefined, ORB_SECRET_TYPE_GITHUB_TOKEN, { rotate: payload?.rotate === true }); if ("error" in result) return c.json(result, result.error === "installation_not_found" ? 404 : 409); return c.json(result); // { enrollId, secret } — secret shown exactly once }); diff --git a/src/orb/broker.ts b/src/orb/broker.ts index fa77468c4d..874a2f9c96 100644 --- a/src/orb/broker.ts +++ b/src/orb/broker.ts @@ -53,16 +53,26 @@ export type IssueResult = { enrollId: string; secret: string } | { error: "insta * hashed). Issued by the operator (internal endpoint) OR by a maintainer who proved install-admin via OAuth — * in the latter case the maintainer's GitHub identity is recorded for audit. installation_id is bound here and * read back (never from the request) at token-exchange time, so a secret can never mint a token for another - * install. `secretType` defaults to the only mintable type today; every existing caller is unaffected. */ + * install. `secretType` defaults to the only mintable type today; every existing caller is unaffected. + * + * `opts.rotate` (#9149): this was previously a bare INSERT, so re-running the enrollment flow (e.g. after a + * secret leak) minted a SECOND simultaneously-valid secret and revoked nothing — the leaked one kept working + * forever. Deliberately NOT the default: a blue/green container swap or a deliberate multi-container setup + * legitimately relies on two live enrollments overlapping for a short window (see #9150's sibling fix, which + * makes that overlap safe rather than assuming it can't happen) — auto-revoking on every issuance would break + * that. `rotate: true` is the caller's explicit "I want this to be the only valid secret now" confirmation + * (the OAuth landing page's `?rotate=1`/`state=:rotate` flow, oauth.ts). */ export async function issueOrbEnrollment( env: Env, installationId: number, maintainer?: { login: string; githubId?: number | null | undefined }, secretType: string = ORB_SECRET_TYPE_GITHUB_TOKEN, + opts: { rotate?: boolean } = {}, ): Promise { const install = await env.DB.prepare("SELECT registered FROM orb_github_installations WHERE installation_id = ?").bind(installationId).first<{ registered: number }>(); if (!install) return { error: "installation_not_found" }; if (install.registered !== 1) return { error: "installation_not_registered" }; + if (opts.rotate === true) await revokeAllLiveEnrollmentsForInstallation(env, installationId); const enrollId = createOpaqueToken("orbenr"); const secret = createOpaqueToken("orbsec"); await env.DB.prepare( @@ -74,6 +84,28 @@ export async function issueOrbEnrollment( return { enrollId, secret }; } +/** Revokes every currently-live ('enrolled', not yet revoked) enrollment for an installation, regardless of + * secret_type — the self-hoster-reachable counterpart to the single-enrollment {@link revokeOrbEnrollment} + * (#9149). Returns the count actually revoked (0 is a valid, non-error result: an install with no live + * enrollment is not a failure, it's just already in the state the caller wanted). Deliberately does NOT gate + * on `registered`/`suspended_at`/`removed_at` the way issuance does — revoking access should work EVEN ON an + * installation the operator has since disabled or suspended, since a maintainer revoking a leaked secret has + * no reason to be blocked by an unrelated administrative state. */ +export async function revokeAllLiveEnrollmentsForInstallation(env: Env, installationId: number): Promise { + const result = await env.DB.prepare("UPDATE orb_enrollments SET revoked_at = CURRENT_TIMESTAMP, state = 'revoked' WHERE installation_id = ? AND state = 'enrolled' AND revoked_at IS NULL") + .bind(installationId) + .run(); + return result.meta.changes; +} + +/** How many enrollments are currently live ('enrolled', not revoked) for an installation (#9149) — the count + * surfaced to an operator (the internal installations list) and to a maintainer (the OAuth secret page) so + * neither has to guess whether a re-enrollment minted an extra standing credential. */ +export async function countLiveEnrollmentsForInstallation(env: Env, installationId: number): Promise { + const row = await env.DB.prepare("SELECT COUNT(*) AS c FROM orb_enrollments WHERE installation_id = ? AND state = 'enrolled' AND revoked_at IS NULL").bind(installationId).first<{ c: number }>(); + return row?.c ?? 0; +} + export type IssueStoredSecretResult = IssueResult | { error: "secret_value_required" | "encryption_unavailable" }; /** Issues a one-time enrollment secret for a STORED (not minted) credential (#8064) -- e.g. control-plane's diff --git a/src/orb/oauth.ts b/src/orb/oauth.ts index 88d23bdfae..4fe9a069f1 100644 --- a/src/orb/oauth.ts +++ b/src/orb/oauth.ts @@ -15,7 +15,7 @@ import type { Context } from "hono"; import { PRODUCT_USER_AGENT, timeoutFetch } from "../github/client"; import { LOOPOVER_SITE_URL } from "../github/footer"; -import { isOrbBrokerEnabled, issueOrbEnrollment } from "./broker"; +import { countLiveEnrollmentsForInstallation, isOrbBrokerEnabled, issueOrbEnrollment, ORB_SECRET_TYPE_GITHUB_TOKEN, revokeAllLiveEnrollmentsForInstallation } from "./broker"; type GitHubUser = { login: string; id?: number }; type GitHubOrgMembership = { role?: string; state?: string; organization?: { id?: number } }; @@ -67,7 +67,39 @@ export async function verifyInstallationAdmin( return body.state === "active" && body.role === "admin" && body.organization?.id === accountId; } -async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, installationId: number): Promise { +/** What a maintainer landing on the OAuth callback is asking to do, and which installation it's bound to. + * `installation_id` normally comes from the GitHub-controlled query string (the install/update Setup-URL + * redirect); `rotate`/`revoke` have no such channel (GitHub only ever echoes back `code` + `state` on a bare + * `login/oauth/authorize` bounce, not arbitrary query params we didn't put there ourselves), so those two + * actions are carried in `state` instead, as `":rotate"` / `":revoke"` (#9149) + * — the value {@link orbOAuthAuthorizeUrl} puts in the "Rotate" / "Revoke all" links on the secret page. */ +type OrbOAuthIntent = { installationId: number | null; action: "enroll" | "rotate" | "revoke" }; + +function parseOAuthState(raw: string | undefined): OrbOAuthIntent { + if (!raw) return { installationId: null, action: "enroll" }; + const [idPart, actionPart] = raw.split(":"); + const parsedId = Number(idPart); + const installationId = Number.isInteger(parsedId) && parsedId > 0 ? parsedId : null; + const action = actionPart === "rotate" ? "rotate" : actionPart === "revoke" ? "revoke" : "enroll"; + return { installationId, action }; +} + +/** A `login/oauth/authorize` link that lands the maintainer back on THIS SAME callback with a fresh, single- + * use `code` and `state=:` (#9149) — the only way to re-prove admin-of-installation + * for a rotate/revoke action without our own persisted maintainer session. Null when the client id isn't + * configured (broker effectively unusable anyway), so the caller can omit the link entirely rather than + * point at a URL that can never work. */ +function orbOAuthAuthorizeUrl(env: Env, installationId: number, action: "rotate" | "revoke"): string | null { + /* v8 ignore next 2 -- defensive: every caller of this function is already past exchangeOrbOAuthCode, which + itself returns null (→ the identity-error page, never reaching secretPage) without ORB_GITHUB_CLIENT_ID + set — so this branch can't be live-reached through the real callback flow. Kept so a future change that + loosens that upstream guard degrades to an omitted link rather than a broken one. */ + if (!env.ORB_GITHUB_CLIENT_ID) return null; + const params = new URLSearchParams({ client_id: env.ORB_GITHUB_CLIENT_ID, state: `${installationId}:${action}` }); + return `https://github.com/login/oauth/authorize?${params.toString()}`; +} + +async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, installationId: number, action: "enroll" | "rotate" | "revoke" = "enroll"): Promise { // A thrown network error (DNS failure, or a timeout past timeoutFetch's own retry budget) from any of the three // GitHub calls below degrades to the same clean landing page a bad HTTP *response* already produces, instead of // escaping handleOrbOAuthCallback as an uncaught framework 500. Mirrors the failure-doesn't-escape convention in @@ -102,6 +134,14 @@ async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, return identityError(); } if (!isAdmin) return c.html(landingPage(c.env, "Admin access required", "You must be an admin of this installation's account to enroll it for self-host."), 403); + // #9149: a REVOKE request skips the active/disabled checks below on purpose — a maintainer revoking a + // leaked secret must be able to do so EVEN ON an installation the operator has since suspended or disabled; + // there is no reason unrelated administrative state should block someone locking down their own credential. + // It also never touches `registered` (no auto-registration side effect for a request that isn't enrolling). + if (action === "revoke") { + const revokedCount = await revokeAllLiveEnrollmentsForInstallation(c.env, installationId); + return c.html(revokedPage(revokedCount)); + } if (install.removed_at !== null || install.suspended_at !== null) return c.html(landingPage(c.env, "Installation not active", "This installation is suspended or uninstalled — re-install the Orb App, then retry."), 403); if (install.self_enrollment_disabled === 1) return c.html(landingPage(c.env, "Installation disabled", "This installation was disabled by the operator — contact the operator to re-enable self-host enrollment."), 403); // Zero-touch self-service: a verified admin of an ACTIVE, non-disabled install self-registers it (registered=1). @@ -109,19 +149,29 @@ async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, if (install.registered !== 1) { await c.env.DB.prepare("UPDATE orb_github_installations SET registered = 1, last_event_at = CURRENT_TIMESTAMP WHERE installation_id = ?").bind(installationId).run(); } - const result = await issueOrbEnrollment(c.env, installationId, { login: user.login, githubId: user.id ?? null }); + const result = await issueOrbEnrollment(c.env, installationId, { login: user.login, githubId: user.id ?? null }, ORB_SECRET_TYPE_GITHUB_TOKEN, { rotate: action === "rotate" }); /* v8 ignore next -- defensive: the existence + admin + active checks above passed and we just set registered=1, so issueOrbEnrollment (which re-checks existence + registered) cannot return an error here; kept to degrade safely. */ if ("error" in result) return c.html(landingPage(c.env, "Couldn't issue an enrollment", "Please retry, or contact the operator."), 409); - return c.html(secretPage(result.secret)); + const liveCount = await countLiveEnrollmentsForInstallation(c.env, installationId); + c.header("Cache-Control", "no-store"); // #9149: this response body is a plaintext secret shown exactly once — never cacheable + return c.html(secretPage(c.env, result.secret, installationId, liveCount)); } export async function handleOrbOAuthCallback(c: Context<{ Bindings: Env }>): Promise { const code = c.req.query("code"); - const installationId = Number(c.req.query("installation_id")); - // Self-enrollment: a maintainer authorized with an OAuth code + an installation_id, and the broker is enabled. - if (code && Number.isInteger(installationId) && installationId > 0 && isOrbBrokerEnabled(c.env)) { - return handleOrbEnrollment(c, code, installationId); + // #9149: installation_id normally arrives in the query string (GitHub's own install/update Setup-URL + // redirect); a rotate/revoke request instead carries it inside `state` (see parseOAuthState's doc comment + // for why) — the query value, when itself valid, still wins, so a genuine install/update redirect is never + // second-guessed by a stray/malformed `state`. + const state = parseOAuthState(c.req.query("state")); + const rawInstallationId = Number(c.req.query("installation_id")); + const queryInstallationId = Number.isInteger(rawInstallationId) && rawInstallationId > 0 ? rawInstallationId : null; + const installationId = queryInstallationId ?? state.installationId; + // Self-enrollment (or a rotate/revoke of one): a maintainer authorized with an OAuth code + a resolved + // installation_id, and the broker is enabled. + if (code && installationId !== null && isOrbBrokerEnabled(c.env)) { + return handleOrbEnrollment(c, code, installationId, state.action); } const updated = c.req.query("setup_action") === "update"; return c.html( @@ -146,10 +196,30 @@ function landingPage(env: Env, heading: string, message: string): string { } /** Show the freshly-issued enrollment secret ONCE. The secret is a generated opaque token (no user input), safe - * to embed; it is never logged. */ -function secretPage(secret: string): string { + * to embed; it is never logged. #9149: also surfaces how many enrollments are now live for this installation + * (an operator/maintainer previously had no way to see this had accumulated) and, when the client id is + * configured, links to re-authorize with a `state`-carried rotate/revoke intent (see orbOAuthAuthorizeUrl) — + * a self-hoster otherwise had no reachable path to either action at all. `installationId` is never + * user-supplied at render time (bound server-side, same as the enrollment itself), so it is safe to embed. */ +function secretPage(env: Env, secret: string, installationId: number, liveCount: number): string { + const rotateUrl = orbOAuthAuthorizeUrl(env, installationId, "rotate"); + const revokeUrl = orbOAuthAuthorizeUrl(env, installationId, "revoke"); + const manageSection = + rotateUrl && revokeUrl + ? `

This installation now has ${liveCount} live enrollment secret${liveCount === 1 ? "" : "s"}, including this one. Rotate (revokes every other one and issues this as the only valid secret) or revoke all if you no longer need self-host access for this installation.

` + : ""; return shell( "Your enrollment secret", - `

Set this as ORB_ENROLLMENT_SECRET in your self-host .env, then restart the container. It is shown once — store it now.

${secret}
`, + `

Set this as ORB_ENROLLMENT_SECRET in your self-host .env, then restart the container. It is shown once — store it now.

${secret}
${manageSection}`, + ); +} + +/** Confirms a revoke-all action (#9149) — the self-hoster-reachable counterpart to secretPage. `count` is + * never user-supplied (it's the return of revokeAllLiveEnrollmentsForInstallation), so it's safe to embed. */ +function revokedPage(count: number): string { + const plural = count === 1 ? "secret has" : "secrets have"; + return shell( + "Enrollment secrets revoked", + `

${count} live enrollment ${plural} been revoked for this installation. Any self-hosted container still using ${count === 1 ? "it" : "one of them"} loses broker access immediately. Re-run the install flow to issue a new one.

`, ); } diff --git a/test/integration/orb-broker.test.ts b/test/integration/orb-broker.test.ts index f01c4f4333..7e60de2561 100644 --- a/test/integration/orb-broker.test.ts +++ b/test/integration/orb-broker.test.ts @@ -3,12 +3,14 @@ import { createApp } from "../../src/api/routes"; import { hashToken } from "../../src/auth/security"; import { brokerOrbToken, + countLiveEnrollmentsForInstallation, isOrbBrokerEnabled, issueOrbEnrollment, issueOrbStoredSecret, ORB_SECRET_TYPE_AMS_GITHUB_TOKEN, ORB_SECRET_TYPE_GITHUB_TOKEN, ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL, + revokeAllLiveEnrollmentsForInstallation, revokeOrbEnrollment, } from "../../src/orb/broker"; import { MAX_ORB_RELAY_REGISTER_BODY_BYTES } from "../../src/orb/relay"; @@ -88,6 +90,80 @@ describe("issueOrbEnrollment", () => { const row = await db(e).prepare("SELECT secret_type, installation_id FROM orb_enrollments WHERE installation_id=203").first<{ secret_type: string; installation_id: number }>(); expect(row).toMatchObject({ secret_type: ORB_SECRET_TYPE_AMS_GITHUB_TOKEN, installation_id: 203 }); }); + + it("#9149: re-enrolling WITHOUT rotate mints an additional secret, leaving the prior one live (blue/green default)", async () => { + const e = await brokerEnv(); + await seedInstall(e, 210, { registered: 1 }); + const first = (await issueOrbEnrollment(e, 210)) as { enrollId: string; secret: string }; + const second = (await issueOrbEnrollment(e, 210)) as { enrollId: string; secret: string }; + expect(first.enrollId).not.toBe(second.enrollId); + expect(await countLiveEnrollmentsForInstallation(e, 210)).toBe(2); + const firstRow = await db(e).prepare("SELECT state FROM orb_enrollments WHERE enroll_id = ?").bind(first.enrollId).first<{ state: string }>(); + expect(firstRow?.state).toBe("enrolled"); // untouched — the leaked-secret bug this closes is the OPPOSITE default + }); + + it("#9149: re-enrolling WITH rotate:true revokes every prior live enrollment before minting the new one", async () => { + const e = await brokerEnv(); + await seedInstall(e, 211, { registered: 1 }); + const first = (await issueOrbEnrollment(e, 211)) as { enrollId: string; secret: string }; + const second = (await issueOrbEnrollment(e, 211, undefined, ORB_SECRET_TYPE_GITHUB_TOKEN, { rotate: true })) as { enrollId: string; secret: string }; + expect(await countLiveEnrollmentsForInstallation(e, 211)).toBe(1); + const firstRow = await db(e).prepare("SELECT state, revoked_at FROM orb_enrollments WHERE enroll_id = ?").bind(first.enrollId).first<{ state: string; revoked_at: string | null }>(); + expect(firstRow).toMatchObject({ state: "revoked" }); + expect(firstRow?.revoked_at).not.toBeNull(); + expect(await brokerOrbToken(e, first.secret)).toEqual({ error: "invalid_enrollment" }); // the leaked/superseded secret no longer mints + const secondRow = await db(e).prepare("SELECT state FROM orb_enrollments WHERE enroll_id = ?").bind(second.enrollId).first<{ state: string }>(); + expect(secondRow?.state).toBe("enrolled"); + }); + + it("#9149: rotate on a FIRST enrollment (nothing prior to revoke) is a no-op revoke, not an error", async () => { + const e = await brokerEnv(); + await seedInstall(e, 212, { registered: 1 }); + const issued = await issueOrbEnrollment(e, 212, undefined, ORB_SECRET_TYPE_GITHUB_TOKEN, { rotate: true }); + expect(issued).toMatchObject({ enrollId: expect.stringMatching(/^orbenr_/) }); + expect(await countLiveEnrollmentsForInstallation(e, 212)).toBe(1); + }); +}); + +describe("revokeAllLiveEnrollmentsForInstallation + countLiveEnrollmentsForInstallation (#9149)", () => { + it("counts 0 for an installation with no enrollments at all", async () => { + const e = await brokerEnv(); + expect(await countLiveEnrollmentsForInstallation(e, 9999)).toBe(0); + }); + + it("revokes every live enrollment for an installation and returns the count, leaving OTHER installations untouched", async () => { + const e = await brokerEnv(); + await seedInstall(e, 220, { registered: 1 }); + await seedInstall(e, 221, { registered: 1 }); + const a = (await issueOrbEnrollment(e, 220)) as { enrollId: string }; + const b = (await issueOrbEnrollment(e, 220)) as { enrollId: string }; + await issueOrbEnrollment(e, 221); + + expect(await revokeAllLiveEnrollmentsForInstallation(e, 220)).toBe(2); + expect(await countLiveEnrollmentsForInstallation(e, 220)).toBe(0); + const [rowA, rowB] = await Promise.all([ + db(e).prepare("SELECT state FROM orb_enrollments WHERE enroll_id = ?").bind(a.enrollId).first<{ state: string }>(), + db(e).prepare("SELECT state FROM orb_enrollments WHERE enroll_id = ?").bind(b.enrollId).first<{ state: string }>(), + ]); + expect(rowA?.state).toBe("revoked"); + expect(rowB?.state).toBe("revoked"); + // A different installation's enrollment is untouched by another installation's revoke-all. + expect(await countLiveEnrollmentsForInstallation(e, 221)).toBe(1); + }); + + it("revoking an installation with no live enrollments returns 0, not an error", async () => { + const e = await brokerEnv(); + await seedInstall(e, 222, { registered: 1 }); + expect(await revokeAllLiveEnrollmentsForInstallation(e, 222)).toBe(0); + }); + + it("works on a SUSPENDED/disabled installation — revoking access is never blocked by unrelated admin state", async () => { + const e = await brokerEnv(); + await seedInstall(e, 223, { registered: 1, suspended_at: "2026-01-01T00:00:00Z", self_enrollment_disabled: 1 }); + const enrolled = (await issueOrbEnrollment(e, 223)) as { enrollId: string }; + expect(enrolled).toMatchObject({ enrollId: expect.stringMatching(/^orbenr_/) }); + expect(await revokeAllLiveEnrollmentsForInstallation(e, 223)).toBe(1); + }); }); describe("issueOrbStoredSecret", () => { @@ -438,6 +514,22 @@ describe("broker endpoints", () => { expect(await tokRes.json()).toMatchObject({ token: "ghs_flow", installationId: 400 }); }); + it("#9149: POST /v1/internal/orb/enrollments defaults to append (no rotate); { rotate: true } revokes prior live enrollments first", async () => { + const e = await brokerEnv(); + await seedInstall(e, 401, { registered: 1 }); + const first = (await (await app.request("/v1/internal/orb/enrollments", { method: "POST", headers: auth, body: JSON.stringify({ installationId: 401 }) }, e)).json()) as { secret: string }; + // Default (no rotate field at all) — append, matching every existing caller's unaffected behavior. + await app.request("/v1/internal/orb/enrollments", { method: "POST", headers: auth, body: JSON.stringify({ installationId: 401 }) }, e); + expect(await db(e).prepare("SELECT COUNT(*) AS n FROM orb_enrollments WHERE installation_id=401 AND state='enrolled'").first<{ n: number }>()).toMatchObject({ n: 2 }); + tokenFetch("ghs_before_rotate"); + expect((await app.request("/v1/orb/token", { method: "POST", headers: { authorization: `Bearer ${first.secret}` } }, e)).status).toBe(200); // first secret still valid + + // rotate: true — revokes both prior live rows, mints exactly one fresh one. + await app.request("/v1/internal/orb/enrollments", { method: "POST", headers: auth, body: JSON.stringify({ installationId: 401, rotate: true }) }, e); + expect(await db(e).prepare("SELECT COUNT(*) AS n FROM orb_enrollments WHERE installation_id=401 AND state='enrolled'").first<{ n: number }>()).toMatchObject({ n: 1 }); + expect((await app.request("/v1/orb/token", { method: "POST", headers: { authorization: `Bearer ${first.secret}` } }, e)).status).toBe(401); // superseded + }); + it("/v1/orb/token: 401 without a Bearer secret, 401 on a bad secret, 403 when the install became ineligible", async () => { const e = await brokerEnv(); expect((await app.request("/v1/orb/token", { method: "POST" }, e)).status).toBe(401); diff --git a/test/integration/orb-oauth.test.ts b/test/integration/orb-oauth.test.ts index 1c1650c9e3..b4a30a36d9 100644 --- a/test/integration/orb-oauth.test.ts +++ b/test/integration/orb-oauth.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createApp } from "../../src/api/routes"; import * as githubClientModule from "../../src/github/client"; import { exchangeOrbOAuthCode, fetchOrbOAuthUser, verifyInstallationAdmin } from "../../src/orb/oauth"; +import { issueOrbEnrollment } from "../../src/orb/broker"; import { createTestEnv, type TestD1Database } from "../helpers/d1"; const asFetch = (fn: (url: string) => Promise): typeof fetch => ((url: RequestInfo | URL) => fn(String(url))) as typeof fetch; @@ -127,13 +128,103 @@ describe("maintainer self-enrollment via the OAuth callback", () => { stubGitHub(); const res = await app.request("/v1/orb/oauth/callback?code=abc&installation_id=500", {}, e); expect(res.status).toBe(200); + expect(res.headers.get("cache-control")).toBe("no-store"); // #9149: a plaintext secret must never be cacheable const html = await res.text(); expect(html).toContain("Your enrollment secret"); expect(html).toMatch(/orbsec_/); + expect(html).toContain("1 live enrollment secret,"); // #9149: live-enrollment count surfaced, singular form const row = await db(e).prepare("SELECT maintainer_login, maintainer_github_id FROM orb_enrollments WHERE installation_id=500").first<{ maintainer_login: string; maintainer_github_id: number }>(); expect(row).toMatchObject({ maintainer_login: "alice", maintainer_github_id: 7 }); }); + it("#9149: the secret page's live-enrollment count reflects an earlier enrollment, and pluralizes correctly", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 510, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + stubGitHub(); + await app.request("/v1/orb/oauth/callback?code=abc1&installation_id=510", {}, e); + const res = await app.request("/v1/orb/oauth/callback?code=abc2&installation_id=510", {}, e); + const html = await res.text(); + expect(html).toContain("2 live enrollment secrets,"); // plural + expect(html).toMatch(/state=510%3Arotate/); // the Rotate link carries installation 510 + expect(html).toMatch(/state=510%3Arevoke/); // the Revoke-all link carries installation 510 + }); + + it("#9149: state=:rotate revokes every PRIOR live enrollment and mints exactly one fresh secret", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 511, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + stubGitHub(); + await app.request("/v1/orb/oauth/callback?code=abc1&installation_id=511", {}, e); // first, ordinary enrollment + const rotateRes = await app.request("/v1/orb/oauth/callback?code=abc2&state=511:rotate", {}, e); // no installation_id query param — carried in state + expect(rotateRes.status).toBe(200); + const html = await rotateRes.text(); + expect(html).toContain("Your enrollment secret"); + expect(html).toContain("1 live enrollment secret,"); // back down to one after rotating + const rows = await db(e).prepare("SELECT state FROM orb_enrollments WHERE installation_id=511 ORDER BY rowid").all<{ state: string }>(); + expect(rows.results.map((r) => r.state)).toEqual(["revoked", "enrolled"]); + }); + + it("#9149: state=:revoke revokes every live enrollment and mints nothing new", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 512, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + stubGitHub(); + await app.request("/v1/orb/oauth/callback?code=abc1&installation_id=512", {}, e); + await app.request("/v1/orb/oauth/callback?code=abc2&installation_id=512", {}, e); + const revokeRes = await app.request("/v1/orb/oauth/callback?code=abc3&state=512:revoke", {}, e); + expect(revokeRes.status).toBe(200); + const html = await revokeRes.text(); + expect(html).toContain("Enrollment secrets revoked"); + expect(html).toContain("2 live enrollment secrets have been revoked"); + expect(await db(e).prepare("SELECT COUNT(*) AS n FROM orb_enrollments WHERE installation_id=512 AND state='enrolled'").first<{ n: number }>()).toMatchObject({ n: 0 }); + // No new row was minted by a revoke request. + expect(await db(e).prepare("SELECT COUNT(*) AS n FROM orb_enrollments WHERE installation_id=512").first<{ n: number }>()).toMatchObject({ n: 2 }); + }); + + it("#9149: a REVOKE request still requires admin-of-installation — a non-admin revokes nothing", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 513, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + await issueOrbEnrollment(e, 513); + stubGitHub({ membership: { role: "member", state: "active", organization: { id: 20 } } }); + const res = await app.request("/v1/orb/oauth/callback?code=abc&state=513:revoke", {}, e); + expect(res.status).toBe(403); + expect(await db(e).prepare("SELECT COUNT(*) AS n FROM orb_enrollments WHERE installation_id=513 AND state='enrolled'").first<{ n: number }>()).toMatchObject({ n: 1 }); + }); + + it("#9149: revoke works EVEN ON a suspended/self-enrollment-disabled installation — unrelated admin state never blocks it", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 514, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1, suspended_at: "2026-01-01T00:00:00Z", self_enrollment_disabled: 1 }); + await issueOrbEnrollment(e, 514); + stubGitHub(); + const res = await app.request("/v1/orb/oauth/callback?code=abc&state=514:revoke", {}, e); + expect(res.status).toBe(200); + expect(await res.text()).toContain("1 live enrollment secret has been revoked"); + }); + + it("#9149: revoking an installation with no live enrollments still succeeds, reporting 0", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 515, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + stubGitHub(); + const res = await app.request("/v1/orb/oauth/callback?code=abc&state=515:revoke", {}, e); + expect(res.status).toBe(200); + expect(await res.text()).toContain("0 live enrollment secrets have been revoked"); + }); + + it("#9149: an unrecognized state action falls back to a plain enrollment (defensive default)", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 516, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + stubGitHub(); + const res = await app.request("/v1/orb/oauth/callback?code=abc&state=516:bogus", {}, e); + expect(res.status).toBe(200); + expect(await res.text()).toContain("Your enrollment secret"); + }); + + it("#9149: a malformed state (non-numeric id, no installation_id query either) falls through to the generic landing page", async () => { + const e = brokeredEnv(); + stubGitHub(); + const res = await app.request("/v1/orb/oauth/callback?code=abc&state=not-a-number:rotate", {}, e); + expect(await res.text()).toContain("LoopOver Orb connected"); + }); + + it("a NON-admin is refused (403), NO enrollment created, and the install is NOT auto-registered — the escalation gate", async () => { const e = brokeredEnv(); await seedInstall(e, { installation_id: 501, account_login: "acme", account_type: "Organization", account_id: 20, registered: 0 }); diff --git a/test/integration/orb-onboarding.test.ts b/test/integration/orb-onboarding.test.ts index 5703a78274..e43c31845c 100644 --- a/test/integration/orb-onboarding.test.ts +++ b/test/integration/orb-onboarding.test.ts @@ -1,5 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createApp } from "../../src/api/routes"; +import { issueOrbEnrollment, revokeAllLiveEnrollmentsForInstallation } from "../../src/orb/broker"; import { createTestEnv, type TestD1Database } from "../helpers/d1"; async function pkcs8Pem(): Promise { @@ -34,6 +35,30 @@ describe("Central Orb installation registry routes (/v1/internal/orb/installatio expect((await app.request("/v1/internal/orb/installations", {}, createTestEnv())).status).toBe(401); }); + it("#9149: surfaces liveEnrollmentCount per installation, reflecting a revoke and untouched by ANOTHER installation's enrollments", async () => { + const env = createTestEnv({ ORB_BROKER_ENABLED: "true" }); + await seed(env, 102, 1); + await seed(env, 103, 1); + await issueOrbEnrollment(env, 102); + await issueOrbEnrollment(env, 102); + await issueOrbEnrollment(env, 103); + const before = (await (await app.request("/v1/internal/orb/installations", { headers: auth }, env)).json()) as { installations: Array<{ installationId: number; liveEnrollmentCount: number }> }; + expect(before.installations.find((i) => i.installationId === 102)?.liveEnrollmentCount).toBe(2); + expect(before.installations.find((i) => i.installationId === 103)?.liveEnrollmentCount).toBe(1); + + await revokeAllLiveEnrollmentsForInstallation(env, 102); + const after = (await (await app.request("/v1/internal/orb/installations", { headers: auth }, env)).json()) as { installations: Array<{ installationId: number; liveEnrollmentCount: number }> }; + expect(after.installations.find((i) => i.installationId === 102)?.liveEnrollmentCount).toBe(0); + expect(after.installations.find((i) => i.installationId === 103)?.liveEnrollmentCount).toBe(1); // unaffected by the OTHER install's revoke + }); + + it("reports liveEnrollmentCount 0 for an installation with no enrollments at all", async () => { + const env = createTestEnv(); + await seed(env, 104); + const res = (await (await app.request("/v1/internal/orb/installations", { headers: auth }, env)).json()) as { installations: Array<{ installationId: number; liveEnrollmentCount: number }> }; + expect(res.installations.find((i) => i.installationId === 104)?.liveEnrollmentCount).toBe(0); + }); + it("registers an installation, then unregisters it", async () => { const env = createTestEnv(); await seed(env, 101); From c64100cffb868b6fe91b93661a31bf6af1fd26bd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:22:09 -0700 Subject: [PATCH 6/6] chore: regenerate cf-typegen/selfhost env-reference after rebase --- ...pending_enroll_id.sql => 0194_orb_relay_pending_enroll_id.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename migrations/{0191_orb_relay_pending_enroll_id.sql => 0194_orb_relay_pending_enroll_id.sql} (100%) diff --git a/migrations/0191_orb_relay_pending_enroll_id.sql b/migrations/0194_orb_relay_pending_enroll_id.sql similarity index 100% rename from migrations/0191_orb_relay_pending_enroll_id.sql rename to migrations/0194_orb_relay_pending_enroll_id.sql