fix(content-lane,federated,apr): close seven pre-flag defects — public-safe scrub bypass, probe OOM/fan-out, self-corroborating grounding, identity farming, peer takeover, and APR tenant binding (#9490) (#9535)

Author: JSONbored

src/orb/apr-repo-binding.ts

@@ -0,0 +1,31 @@
+// Trusted server-side APR repo→customer binding lookup for transfer authorization (#9490).
+//
+// `installationId`, `repoFullName` and `newOwner` are ALL caller-supplied on the transfer route, so without a
+// server-side record binding an APR repo to the customer it belongs to, any authorized caller could transfer
+// any completed APR repo — including another customer's — to themselves the moment #7664 starts persisting
+// completion records. This gate exists NOW, while the route is still inert, precisely so completion landing
+// later cannot silently arm an unauthorized-transfer primitive.
+//
+// Until #7664 persists a binding record, this ALWAYS returns null (fail closed) and
+// `requestAprRepoTransfer` rejects on a null binding. A client-supplied binding must never substitute for
+// this — same contract, same replace-the-body-keep-the-signature instruction, and the same reasoning as
+// ./apr-idea-completion.ts (whose shape this module deliberately mirrors, down to living in its own file so
+// the route's trusted lookup is replaceable at the import seam).
+
+export type AprRepoBinding = {
+  /** The GitHub login of the customer whose OAuth session created (or owns) this APR repo. */
+  customerLogin: string;
+  /** The installation the APR repo actually belongs to. */
+  installationId: number;
+};
+
+export type AprRepoBindingLookup = (env: Env, input: { repoFullName: string }) => Promise<AprRepoBinding | null>;
+
+/**
+ * Resolve the tenant binding for an APR repo (#9490). Fail-closed until a persisted record exists: today's
+ * body always returns null. Declared return is `AprRepoBinding | null` so a future persisted lookup (and test
+ * doubles) can return a real binding; a null always rejects the transfer.
+ */
+export async function loadAprRepoBinding(_env: Env, _input: { repoFullName: string }): Promise<AprRepoBinding | null> {
+  return null;
+}

src/orb/apr-repo-transfer.ts

@@ -13,11 +13,15 @@ import { upsertRepositorySettings } from "../db/repositories";
 import { createInstallationToken } from "../github/app";
 import { githubHeaders, timeoutFetch } from "../github/client";
 import { loadAprIdeaCompletion, type AprIdeaCompletionLookup } from "./apr-idea-completion";
+import { loadAprRepoBinding as loadAprRepoBindingDefault, type AprRepoBinding, type AprRepoBindingLookup } from "./apr-repo-binding";
 // `Env` is the ambient Cloudflare Worker binding interface (worker-configuration.d.ts) — a global, not imported.
 
 export type { AprIdeaCompletionLookup, AprIdeaCompletionLookupInput } from "./apr-idea-completion";
 export { loadAprIdeaCompletion } from "./apr-idea-completion";
 
+export type { AprRepoBinding, AprRepoBindingLookup } from "./apr-repo-binding";
+export { loadAprRepoBinding } from "./apr-repo-binding";
+
 /**
  * Result of initiating an APR repo transfer.
  *
@@ -54,10 +58,36 @@ export type RequestAprRepoTransferInput = {
  *   GitHub accepted a *pending* transfer (see {@link AprRepoTransferResult}), never "transfer done".
  */
 export type RequestAprRepoTransferResult =
-  | { status: "rejected"; reason: "idea_not_complete" }
+  | { status: "rejected"; reason: "idea_not_complete" | AprRepoTransferBindingRejection }
   | { status: "initiated"; transfer: Extract<AprRepoTransferResult, { initiated: true }> }
   | { status: "failed"; transfer: Extract<AprRepoTransferResult, { initiated: false }> };
 
+/** #9490: the three ways the tenant binding can refuse a transfer. Distinct reasons on purpose — an operator
+ *  debugging a refused transfer needs to know WHICH leg failed, and none of them leak anything the caller did
+ *  not already supply. */
+export type AprRepoTransferBindingRejection =
+  /** No server-side binding record exists for this repo at all — either not an APR repo, or #7664 has not
+   *  persisted its record. Fail closed: an unbound repo is never transferable. */
+  | "repo_not_apr_bound"
+  /** The caller-supplied `newOwner` is not the customer this APR repo is bound to. The whole point: a transfer
+   *  may only ever move a repo to its OWN customer, never to whoever asked. */
+  | "new_owner_not_bound_customer"
+  /** The caller-supplied `installationId` is not the installation the binding records for this repo. */
+  | "installation_not_bound";
+
+/** #9490: pure tenant-binding check, separated from the completion gate so each is independently testable. */
+export function evaluateAprRepoTransferBinding(
+  input: Pick<RequestAprRepoTransferInput, "installationId" | "repoFullName" | "newOwner">,
+  binding: AprRepoBinding | null,
+): { allowed: true } | { allowed: false; reason: AprRepoTransferBindingRejection } {
+  if (binding === null) return { allowed: false, reason: "repo_not_apr_bound" };
+  if (binding.customerLogin.toLowerCase() !== input.newOwner.trim().toLowerCase()) {
+    return { allowed: false, reason: "new_owner_not_bound_customer" };
+  }
+  if (binding.installationId !== input.installationId) return { allowed: false, reason: "installation_not_bound" };
+  return { allowed: true };
+}
+
 /** Decide whether a customer may request an APR repo transfer right now (#7742). Pure and deterministic. */
 export function evaluateAprRepoTransferRequestEligibility(input: {
   ideaComplete: boolean;
@@ -116,6 +146,8 @@ export async function requestAprRepoTransfer(
       newOwner: string,
     ) => Promise<AprRepoTransferResult>;
     loadCompletion?: AprIdeaCompletionLookup;
+    /** #9490 seam: the tenant-binding lookup; injectable for tests, fail-closed default. */
+    loadBinding?: AprRepoBindingLookup;
     /** #7741 deliverable 2 seam: how to freeze AMS dispatch once a transfer is pending. Injectable for tests. */
     pauseDispatch?: (env: Env, repoFullName: string) => Promise<void>;
   } = {},
@@ -125,6 +157,15 @@ export async function requestAprRepoTransfer(
   const eligibility = evaluateAprRepoTransferRequestEligibility({ ideaComplete });
   if (!eligibility.allowed) return { status: "rejected", reason: eligibility.reason };
 
+  // #9490: the tenant binding gates AFTER completion (cheapest rejection first is irrelevant here -- both are
+  // local lookups -- but completion-first keeps today's observable behaviour byte-identical: an incomplete
+  // idea still rejects with the same reason it always did) and BEFORE any GitHub call: a transfer request
+  // that fails authorization must never reach GitHub at all, not even to fail there.
+  const loadBinding = options.loadBinding ?? loadAprRepoBindingDefault;
+  const binding = await loadBinding(env, { repoFullName: input.repoFullName });
+  const bindingCheck = evaluateAprRepoTransferBinding(input, binding);
+  if (!bindingCheck.allowed) return { status: "rejected", reason: bindingCheck.reason };
+
   const initiate = options.initiate ?? initiateAprRepoTransfer;
   const transfer = await initiate(env, input.installationId, input.repoFullName, input.newOwner);
   if (transfer.initiated) {
@@ -217,7 +258,21 @@ export async function probeAprRepoTransfer(
     const response = await timeoutFetch(`https://api.github.com/repos/${transfer.repoFullName}`, {
       headers: githubHeaders({ token }),
     });
-    if (response.status === 404) return { state: "access_departed" };
+    // #9490: a plain 404 at the original path is AMBIGUOUS -- ownership moved away, OR the repo was simply
+    // deleted / the App uninstalled. Recording "accepted_departed" (a terminal SUCCESS) for a deleted repo is
+    // a false outcome in the transfer ledger, so departure is only declared once the repo demonstrably
+    // resolves under the TARGET owner. When that corroboration cannot be obtained (a private repo the App can
+    // no longer see), the transfer stays "pending" and the existing expiry clock resolves it -- a bounded,
+    // late, truthful answer over a fast wrong one.
+    if (response.status === 404) {
+      const repoName = transfer.repoFullName.split("/")[1] ?? "";
+      const relocated = repoName ? await timeoutFetch(`https://api.github.com/repos/${transfer.newOwner}/${repoName}`, { headers: githubHeaders({ token }) }).catch(() => null) : null;
+      if (relocated?.ok) {
+        const relocatedBody = (await relocated.json().catch(() => null)) as { owner?: { login?: string } } | null;
+        if (relocatedBody?.owner?.login?.toLowerCase() === transfer.newOwner.toLowerCase()) return { state: "access_departed" };
+      }
+      return { state: "pending" };
+    }
     if (!response.ok) return { state: "pending" };
     const body = (await response.json().catch(() => null)) as { owner?: { login?: string } } | null;
     const owner = body?.owner?.login;

src/orb/federated-import.ts

@@ -47,6 +47,10 @@ const MAX_BUNDLE_AGE_MS = 7 * 86_400_000;
  *  self-hosted instances without opening a meaningful backdating/replay window. */
 const MAX_CLOCK_SKEW_MS = 5 * 60_000;
 
+/** #9490: hard cap on `instanceId` length -- see isBundleBodyShaped's own comment for why this is a
+ *  persistence-integrity bound, not a cosmetic one. */
+export const MAX_INSTANCE_ID_CHARS = 128;
+
 /** 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
@@ -90,6 +94,11 @@ export type FederatedRejectionReason =
    *  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"
+  /** #9490: this `instanceId` is already bound to a DIFFERENT verifying key. First-writer-wins: the id was
+   *  admitted under one key's fingerprint, and a bundle for the same id verifying under any other allowlisted
+   *  key is rejected outright — the takeover primitive (shadow an honest peer's id, ratchet its watermark,
+   *  suppress its genuine bundles) must not exist even between two currently-trusted keys. */
+  | "instance_key_conflict"
   /** 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";
@@ -136,6 +145,13 @@ function isBundleBodyShaped(bundle: FederatedSignalBundle): boolean {
   const nullableNumeric = (value: unknown): boolean => value === null || numeric(value);
   return (
     typeof bundle.instanceId === "string" &&
+    // #9490: bounded, because instanceIds are PERSISTED into the single system_flags peer-state blob. Nothing
+    // else bounded them, and the pull body cap is 1 MB total -- so roughly 3-4 bundles carrying ~600 KB ids
+    // pushed that blob past D1's ~2 MB value limit, writeFederatedPeerState swallowed the failure, and replay/
+    // rollback watermarks silently stopped persisting for EVERY peer. 128 chars fits every reasonable id
+    // scheme (a UUID is 36, a hex fingerprint 64) with headroom.
+    bundle.instanceId.length > 0 &&
+    bundle.instanceId.length <= MAX_INSTANCE_ID_CHARS &&
     typeof bundle.generatedAt === "string" &&
     typeof bundle.signature === "string" &&
     numeric(bundle.windowDays) &&
@@ -345,7 +361,22 @@ async function writeFederatedPeerState(db: D1Database, state: FederatedPeerState
     .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);
+    .catch((error: unknown) => {
+      // #9490: still fail-open (a write failure must never fail the sync tick), but LOUDLY. This blob carries
+      // the replay/rollback watermarks and the Sybil cap's instance ledger for every peer -- a persistent
+      // write failure silently regresses all of #9148's protections, which an operator needs to SEE, not
+      // infer. error level so the structured-log forwarder picks it up, same convention as
+      // regate_repair_exhausted.
+      console.error(
+        JSON.stringify({
+          level: "error",
+          event: "federated_peer_state_write_failed",
+          instances: Object.keys(state).length,
+          approxBytes: JSON.stringify(state).length,
+          message: error instanceof Error ? error.message.slice(0, 200) : String(error).slice(0, 200),
+        }),
+      );
+    });
 }
 
 /**
@@ -394,6 +425,19 @@ export async function applyFederatedPeerWatermarks(
     const generatedAtMs = Date.parse(bundle.generatedAt); // already validated finite by importPeerBundles' rejectionFor
     const existing = state[bundle.instanceId];
     if (existing) {
+      // #9490: an instanceId is BOUND to the first key that verified it. Without this, any hostile-but-
+      // allowlisted peer B could claim honest peer A's id: shadow A's bundle in-batch (last-wins dedup keys
+      // on id alone), overwrite the watermark entry's keyFingerprint, then ratchet lastGeneratedAtMs forward
+      // so A's genuine bundles reject as replayed_or_rollback forever -- targeted suppression plus stat
+      // replacement, and a bypass of B's own MAX_INSTANCES_PER_KEY cap (only NEW ids are counted against it).
+      // That sits squarely inside #9148's declared threat model: bound the damage one still-trusted key can
+      // do. The binding is first-writer-wins and permanent for the life of the state entry; a peer that
+      // legitimately rotates keys re-enters under a new id (or after PEER_STATE_PRUNE_AFTER_MS frees the old
+      // one), which is the cheap, honest path -- as opposed to any takeover path existing at all.
+      if (existing.keyFingerprint !== fingerprint) {
+        rejected.push(reject(bundle.instanceId, "instance_key_conflict"));
+        continue;
+      }
       if (generatedAtMs <= existing.lastGeneratedAtMs) {
         rejected.push(reject(bundle.instanceId, "replayed_or_rollback"));
         continue;

src/review/content-lane/orchestrator.ts

@@ -72,8 +72,34 @@ export function diffAppendedSurfaceEntries(headRaw: string | null, baseRaw: stri
   const headEntries = surfacesOf(safeParseJson(headRaw), field);
   if (headEntries === null) return null;
   const baseEntries = surfacesOf(safeParseJson(baseRaw), field) ?? [];
-  const baseKeys = new Set(baseEntries.map((entry) => JSON.stringify(entry)));
-  return headEntries.filter((entry) => !baseKeys.has(JSON.stringify(entry)));
+  const baseKeys = new Set(baseEntries.map((entry) => canonicalEntryKey(entry)));
+  return headEntries.filter((entry) => !baseKeys.has(canonicalEntryKey(entry)));
+}
+
+/**
+ * #9490: entry identity, KEY-ORDER INVARIANT. Both structural diffs below used raw `JSON.stringify(entry)` as
+ * the identity key, and JSON.stringify preserves insertion order -- so reordering an existing entry's keys
+ * (`{url, kind}` -> `{kind, url}`) read as a FRESH append while simultaneously removing the entry's prior self
+ * from survivingExistingEntries' scope, taking it out of the duplicate check too. That is a recipe for
+ * unbounded content-free auto-merged registry PRs: each one "contributes" a reorder, farms a merge, and leaves
+ * the registry byte-different but semantically identical. Sorting keys recursively makes a reorder read as
+ * exactly what it is -- zero appended entries. Arrays keep their order (order IS meaning there, same rule as
+ * decision-record.ts's canonicalJson).
+ */
+export function canonicalEntryKey(entry: unknown): string {
+  return JSON.stringify(sortKeysDeep(entry));
+}
+
+function sortKeysDeep(value: unknown): unknown {
+  if (Array.isArray(value)) return value.map(sortKeysDeep);
+  if (value !== null && typeof value === "object") {
+    return Object.fromEntries(
+      Object.keys(value as Record<string, unknown>)
+        .sort()
+        .map((key) => [key, sortKeysDeep((value as Record<string, unknown>)[key])]),
+    );
+  }
+  return value;
 }
 
 /**
@@ -100,8 +126,10 @@ export function survivingExistingEntries(headRaw: string | null, baseRaw: string
   const headEntries = surfacesOf(safeParseJson(headRaw), field);
   if (headEntries === null) return [];
   const baseEntries = surfacesOf(safeParseJson(baseRaw), field) ?? [];
-  const headKeys = new Set(headEntries.map((entry) => JSON.stringify(entry)));
-  return baseEntries.filter((entry) => headKeys.has(JSON.stringify(entry)));
+  // #9490: same canonical key as diffAppendedSurfaceEntries, and for the same reason -- a key-reordered entry
+  // must still count as "surviving" so its identity stays inside the duplicate check's scope.
+  const headKeys = new Set(headEntries.map((entry) => canonicalEntryKey(entry)));
+  return baseEntries.filter((entry) => headKeys.has(canonicalEntryKey(entry)));
 }
 
 function fromProvider(assessment: ProviderAssessment): SurfaceReviewResult {
@@ -212,15 +240,38 @@ function pickAggregateAssessment(assessments: Assessment[]): Assessment {
  * for review instead. `makeSurfaceEntryVerifier` is itself written not to throw, so this is a belt-and-braces
  * guard against a future/third-party verifier that is less careful.
  */
+/**
+ * #9490: hard ceiling on live verifications per review run. Each verification fetches up to 2 URLs, each
+ * following up to 5 hops with a 10s per-hop timeout -- so an uncapped run over a spec with
+ * `maxAppendedEntries: Infinity` (metagraphed's documented policy) let a 500-entry PR command thousands of
+ * outbound subrequests from the bot: a request-amplification primitive, and on Workers a subrequest-exhaustion
+ * path that converts into bulk `probe_fetch_failed` holds for everyone else in the isolate. Ten matches
+ * source-evidence.ts's own fan-out cap. Entries past the cap are HELD for a human, never merged unverified --
+ * the cap bounds spend, it must not become a way to sneak entry #11 through unprobed.
+ */
+export const MAX_VERIFIED_ENTRIES_PER_RUN = 10;
+
 async function verifyMergedEntries(
   staticAssessments: Assessment[],
   appendedEntries: readonly unknown[],
   verifyEntry: SurfaceReviewInput["verifyEntry"],
 ): Promise<Assessment[]> {
   if (!verifyEntry) return staticAssessments;
+  let verificationsStarted = 0;
   return await Promise.all(
     staticAssessments.map(async (assessment, idx) => {
       if (assessment.verdict !== "merged") return assessment;
+      // Counted SYNCHRONOUSLY, before any await, so the concurrent map cannot race the budget check --
+      // .map's callbacks run their synchronous prefix in order, so exactly the first N merged entries verify.
+      if (verificationsStarted >= MAX_VERIFIED_ENTRIES_PER_RUN) {
+        return {
+          verdict: "manual-review" as const,
+          summary: `This PR appends more than ${MAX_VERIFIED_ENTRIES_PER_RUN} entries needing live verification; this entry is past that per-run probe budget, so it is routed to review rather than accepted unverified.`,
+          candidate: assessment.candidate,
+          reason: "verification-capacity",
+        };
+      }
+      verificationsStarted += 1;
       try {
         return (await verifyEntry(appendedEntries[idx])) ?? assessment;
       } catch {