diff --git a/control-plane/src/ams-wake.ts b/control-plane/src/ams-wake.ts index e367ec5522..f46cc036e7 100644 --- a/control-plane/src/ams-wake.ts +++ b/control-plane/src/ams-wake.ts @@ -30,6 +30,9 @@ export type AmsWakeConfig = { /** Overridable for tests only -- production always uses real wall-clock time and real delays. */ pollIntervalMs?: number; pollTimeoutMs?: number; + /** Caps how many due tenants ONE tick will wake (#9143, defect 7) -- production always uses + * {@link DEFAULT_MAX_TENANTS_PER_TICK}; overridable for tests only. */ + maxTenantsPerTick?: number; now?: () => Date; }; @@ -45,10 +48,23 @@ export type AmsWakeResult = { const HOSTED_ENTRY_BIN = "loopover-miner-hosted"; const DEFAULT_POLL_INTERVAL_MS = 1_000; -// A generous ceiling for a real discover/manage-poll/attempt cycle -- long enough that a real, working run -// almost never hits it, short enough that a genuinely hung container doesn't block this tick's remaining -// tenants indefinitely (this loop processes due tenants one at a time, not in parallel; see wakeDueAmsTenants). -const DEFAULT_POLL_TIMEOUT_MS = 10 * 60 * 1000; +// #9143 (defect 7): wrangler.jsonc's own Cron Trigger fires this module's `scheduled()` caller every 5 +// minutes (`*/5 * * * *`). The OLD 10-minute default here GUARANTEED an overlapping tick: nextDueAt used to +// advance only AFTER a woken tenant's poll resolved (see wakeDueAmsTenants's own header comment on the fix), +// so one hung tenant's poll alone could still be running when the NEXT tick fired 5 minutes later -- which +// would then see that SAME tenant's nextDueAt untouched (still overdue) and wake it a SECOND time +// concurrently, with no lock/in-flight marker anywhere to stop it, racing both ticks' upserts against the +// same record. Capped well under the cron interval, AND `wakeDueAmsTenants` now claims each tenant's next +// nextDueAt BEFORE starting its poll (not after) so a genuinely-overlapping tick observes the claim and skips +// it regardless of this timeout's exact value -- but a short timeout is still what keeps a normal, non- +// overlapping tick's wall-clock budget sane across every due tenant it processes sequentially, not just one. +const DEFAULT_POLL_TIMEOUT_MS = 2 * 60 * 1000; +// #9143 (defect 7): bounds how many due tenants ONE tick will wake. Sequential-by-design (see +// wakeDueAmsTenants's own comment on why not Promise.all) means a fleet with more due tenants than this in a +// single 5-minute window would otherwise risk the WHOLE tick running long even when every individual tenant +// behaves within DEFAULT_POLL_TIMEOUT_MS -- deferring the excess to the next tick (their nextDueAt is already +// in the past, so nothing is lost, only delayed) keeps one tick's total wall-clock bounded too. +const DEFAULT_MAX_TENANTS_PER_TICK = 20; /** Same `${product}:${name}` composite container-driver.ts's own `instanceNameFor` derives -- duplicated * (not imported) because container-driver.ts's version takes a `TenantProvisioningRequest`, a shape this @@ -82,21 +98,42 @@ async function pollForExitCode(stub: WakeStubLike, pollIntervalMs: number, timeo /** Wakes every currently-due AMS tenant, one at a time (deliberately sequential, not `Promise.all` -- a * single Cloudflare Cron Trigger invocation has a bounded wall-clock budget shared across every tenant this * tick processes; running them concurrently would trade a slow tick for cross-tenant resource contention on - * shared infra this module has no visibility into). Advances each woken tenant's `nextDueAt` from the tick's - * OWN start time (not the run's completion time) so schedule drift doesn't accumulate when a cycle runs long. - */ + * shared infra this module has no visibility into), up to {@link DEFAULT_MAX_TENANTS_PER_TICK} (#9143, defect + * 7) per invocation. + * + * #9143: `nextDueAt` is now advanced BEFORE the poll starts, not after it resolves -- the pre-#9143 order + * (advance only once `pollForExitCode` returned) meant a hung/slow tenant's `nextDueAt` stayed in the past + * for the ENTIRE poll, so a genuinely-overlapping tick (the 5-minute cron firing again while a >5-minute poll + * from the prior tick was still running, or any other concurrent invocation) would see the same tenant still + * due and wake it a SECOND time, racing both ticks' upserts against the same record with no lock or in-flight + * marker anywhere. Claiming the slot up front means an overlapping tick observes `nextDueAt` already moved + * and skips it -- from the tick's OWN start time (not the run's completion time), so schedule drift doesn't + * accumulate when a cycle runs long, exactly as before. */ export async function wakeDueAmsTenants(config: AmsWakeConfig): Promise { const now = config.now ?? (() => new Date()); const pollIntervalMs = config.pollIntervalMs ?? DEFAULT_POLL_INTERVAL_MS; const pollTimeoutMs = config.pollTimeoutMs ?? DEFAULT_POLL_TIMEOUT_MS; + const maxTenantsPerTick = config.maxTenantsPerTick ?? DEFAULT_MAX_TENANTS_PER_TICK; const tickStartedAt = now(); const records = await config.registry.list(); const results: AmsWakeResult[] = []; + let woken = 0; for (const record of records) { + if (woken >= maxTenantsPerTick) break; if (!isDue(record, tickStartedAt)) continue; const schedule = record.amsSchedule!; + woken += 1; + + // Claim this tenant's next slot BEFORE starting its poll (#9143) -- see this function's own header + // comment on why. + const claimedNextDueAt = new Date(tickStartedAt.getTime() + schedule.intervalMs).toISOString(); + await config.registry.upsert({ + ...record, + amsSchedule: { ...schedule, nextDueAt: claimedNextDueAt }, + updatedAt: now().toISOString(), + }); const stub = config.binding.getByName(instanceNameFor(record.tenant.name, record.product)); await stub.start({ entrypoint: [HOSTED_ENTRY_BIN, schedule.command, ...schedule.args] }); @@ -109,7 +146,7 @@ export async function wakeDueAmsTenants(config: AmsWakeConfig): Promise; entrypoint?: string[]; enableInternet?: boolean }): Promise; stop(): Promise; isProvisioned(): Promise; - markProvisioned(): Promise; + /** `envVars` (#9143, defect 4): the SAME map just passed to `start()` above, so the real DO-backed + * implementation (worker.ts's `ProvisionedContainer`) can persist it in its own durable storage and reload + * it into the base `Container` class's `this.envVars` on every future construction -- the vendored + * `@cloudflare/containers` SDK only ever applies `envVars` from the CALLER's own `start()` options or from + * `this.envVars`, never durably remembering a past `start({envVars})` call itself, so a container's + * auto-wake-on-request path after it sleeps (`containerFetch`'s own restart, which calls `start()` with NO + * options at all) would otherwise silently boot with an empty environment on every restart after the + * first. Optional so a fresh (no envVars at all) create still matches the exact pre-#9143 call shape. */ + markProvisioned(envVars?: Record): Promise; markDeprovisioned(): Promise; }; @@ -104,10 +112,13 @@ export async function createTenantContainer(config: ContainerDriverConfig, reque if (config.centralPosthogKey) envVars[CENTRAL_POSTHOG_KEY_ENV_VAR] = config.centralPosthogKey; if (Object.keys(envVars).length > 0) { await stub.start({ envVars }); + // #9143 (defect 4): hand the SAME envVars to markProvisioned so the real DO-backed implementation can + // persist them durably and survive a later implicit restart -- see ContainerStubLike's own doc comment. + await stub.markProvisioned(envVars); } else { await stub.start(); + await stub.markProvisioned(); } - await stub.markProvisioned(); } /** Idempotent: a tenant that was never provisioned (or already torn down) is a safe no-op, matching every diff --git a/control-plane/src/http-app.ts b/control-plane/src/http-app.ts index 2b9b3439bd..285127a81b 100644 --- a/control-plane/src/http-app.ts +++ b/control-plane/src/http-app.ts @@ -34,7 +34,7 @@ import { provisionTenant, type ProvisioningPagerDutyOptions, } from "./provisioning.js"; -import type { Product, TenantLifecycleState, TenantProvisioningDriver } from "./tenant-provisioning-driver.js"; +import type { TenantLifecycleState, TenantProvisioningDriver } from "./tenant-provisioning-driver.js"; import type { AmsCycleSchedule, TenantRegistry, TenantRegistryRecord } from "./tenant-registry.js"; /** States that do NOT block re-creating a tenant of the same name+product (or re-claiming its installation @@ -111,29 +111,6 @@ function parseOrbInstallationId(value: unknown): number | string | undefined { return value; } -/** Validated body of `POST /v1/tenants/rollout` (#4898): an explicit tenant-name list (no percentage/canary - * selector — no such primitive exists elsewhere in this codebase to build on) plus the version to pin. - * `pinnedVersion: null` is an explicit unpin (revert to the release channel's default). Scoped to a single - * `product` for the whole batch (#8024: a name is no longer globally unique across products, and a version - * rollout is naturally a per-product-fleet operation anyway -- ORB and AMS ship independently versioned - * images, so mixing them in one rollout call was never a meaningful use case even before #8024). */ -type RolloutRequest = { names: string[]; product: Product; pinnedVersion: string | null }; - -function parseRolloutRequest(body: unknown): RolloutRequest | string { - if (body === null || typeof body !== "object" || Array.isArray(body)) return "body must be a JSON object"; - const { names, product, pinnedVersion } = body as Record; - if (!Array.isArray(names) || names.length === 0) return "names must be a non-empty array of tenant names"; - if (!names.every((name): name is string => typeof name === "string" && name.trim() !== "")) { - return "names must be a non-empty array of tenant names"; - } - if (new Set(names).size !== names.length) return "names must not repeat a tenant"; - if (typeof product !== "string" || !product.trim()) return "product is required"; - if (pinnedVersion !== null && (typeof pinnedVersion !== "string" || !pinnedVersion.trim())) { - return "pinnedVersion must be a non-blank string, or null to unpin"; - } - return { names, product: product.trim(), pinnedVersion: pinnedVersion === null ? null : pinnedVersion.trim() }; -} - export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono { const app = new Hono(); @@ -225,40 +202,67 @@ export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono { return c.json({ tenants: records.map((record) => ({ ...safeRecord(record), createdAt: record.createdAt, updatedAt: record.updatedAt })) }); }); - // #4898: rollout/rollback = updating one or more tenants' pinnedVersion via an explicit list. Validates the - // WHOLE list before touching any record (all-or-nothing) so a typo'd name can never leave a fleet half - // rolled out; each updated tenant's container picks its new version up at its next (re)start - // (container-driver.ts's PINNED_VERSION_ENV_VAR). Every unlisted tenant is untouched by construction — - // the per-tenant-independence guarantee this endpoint exists to keep. - app.post("/v1/tenants/rollout", async (c) => { - const body: unknown = await c.req.json().catch(() => null); - if (body === null) return c.json({ error: "invalid_json" }, 400); - const parsed = parseRolloutRequest(body); - if (typeof parsed === "string") return c.json({ error: "invalid_request", message: parsed }, 400); + // #9143 (defect 3): DISABLED, not implemented. #4898's rollout/rollback route used to silently no-op on + // every one of its four promised effects: (1) it only ever wrote the registry's own `pinnedVersion` field + // and restarted nothing; (2) `createTenantContainer` early-returns once `isProvisioned()`, so a "rollout" + // against an already-running tenant never even reaches the one call site that injects + // `PINNED_VERSION_ENV_VAR`; (3) `provisionTenant({name}, ...)` constructs a FRESH `Tenant` carrying only + // `name`, dropping any previously-stored `pinnedVersion` the moment a tenant is next (re)provisioned anyway; + // and (4) nothing anywhere in this repo ever reads `LOOPOVER_PINNED_VERSION` back out of a container. "Rollback + // is the control you least want to discover is fake" — until the image side of a real rollout mechanism + // exists (a restart-and-repin path through createContainer, or a pin baked into the image at build time), + // this returns an explicit 501 rather than accepting a request that changes nothing any caller can observe. + // `Tenant.pinnedVersion` and `PINNED_VERSION_ENV_VAR` (tenant-provisioning-driver.ts/container-driver.ts) are + // left in place, unused by any route now — exactly what a real rollout mechanism would build on later. + app.post("/v1/tenants/rollout", (c) => c.json({ error: "not_implemented", message: "tenant rollout is not implemented yet (#9143)" }, 501)); - const existing = new Map(); - for (const name of parsed.names) { - const record = await deps.registry.get(name, parsed.product); - if (!record) return c.json({ error: "tenant_not_found", message: `unknown tenant "${name}"` }, 404); - // A torn-down tenant has no container to ever read the pin — surfacing the mistake beats silently - // stamping a version onto a terminated record (same conflict posture as the create route's 409). - if (record.state === "torn down") return c.json({ error: "tenant_torn_down", message: `tenant "${name}" is torn down` }, 409); - existing.set(name, record); + // #9143 (defect 2 manual recovery): re-links an EXISTING tenant's `orbInstallationId`. tenant-registry.ts's + // `upsert` used to unconditionally delete the OLD installation-ID index entry whenever a tenant's own + // `orbInstallationId` changed (or was cleared) between upserts — without checking whether that index entry + // still actually pointed at THIS tenant. Since `isRecreatableState` deliberately lets a "failed"/"torn down" + // tenant's installation claim be taken over by a brand-new tenant of a different name, a stale tenant being + // re-created or torn down LONG after its installation ID was reclaimed by someone else could silently delete + // the CURRENT claimant's live routing pointer — leaving that tenant ACTIVE but permanently unreachable by + // webhook (`orbInstallationId` is otherwise settable only at create; a create against an existing tenant + // 409s). `upsert` itself is now fixed to never do this going forward (see its own header comment), but a + // tenant whose pointer was ALREADY wiped by the pre-fix bug has no other way back — this route is that path. + app.patch("/v1/tenants/:name/orb-installation", async (c) => { + const name = c.req.param("name"); + // Product is required so the registry can resolve the same `${product}:${name}` key used at create (#8024). + const product = c.req.query("product"); + if (typeof product !== "string" || !product.trim()) { + return c.json({ error: "invalid_request", message: "product query parameter is required" }, 400); + } + if (product !== "orb") { + return c.json({ error: "invalid_request", message: 'orbInstallationId is only valid for product "orb"' }, 400); } - const now = new Date().toISOString(); - const updated: TenantRegistryRecord[] = []; - for (const name of parsed.names) { - const record = existing.get(name)!; - const next: TenantRegistryRecord = { - ...record, - tenant: { ...record.tenant, pinnedVersion: parsed.pinnedVersion }, - updatedAt: now, - }; - await deps.registry.upsert(next); - updated.push(next); + const existing = await deps.registry.get(name, product); + if (!existing) return c.json({ error: "tenant_not_found" }, 404); + + const body: unknown = await c.req.json().catch(() => null); + if (body === null || typeof body !== "object") return c.json({ error: "invalid_json" }, 400); + const { orbInstallationId: orbInstallationIdInput } = body as Record; + if (orbInstallationIdInput === undefined) { + return c.json({ error: "invalid_request", message: "orbInstallationId is required" }, 400); + } + const orbInstallationId = parseOrbInstallationId(orbInstallationIdInput); + if (typeof orbInstallationId === "string") return c.json({ error: "invalid_request", message: orbInstallationId }, 400); + + // Refuse to steal an installation ID a DIFFERENT, currently-claiming tenant still legitimately holds — + // same conflict posture as the create route's own check, just excluding this tenant's own current record + // (re-linking to the ID it already has, or recovering one it lost, must not 409 against itself). + const conflicting = await deps.registry.getByOrbInstallationId(orbInstallationId!); + if (conflicting && conflicting.tenant.name !== existing.tenant.name && !isRecreatableState(conflicting.state)) { + return c.json( + { error: "installation_already_claimed", message: `installation ${orbInstallationId} is already claimed by tenant "${conflicting.tenant.name}"` }, + 409, + ); } - return c.json({ tenants: updated.map((record) => ({ ...safeRecord(record), createdAt: record.createdAt, updatedAt: record.updatedAt })) }); + + const record: TenantRegistryRecord = { ...existing, orbInstallationId, updatedAt: new Date().toISOString() }; + await deps.registry.upsert(record); + return c.json(safeRecord(record)); }); app.delete("/v1/tenants/:name", async (c) => { @@ -271,6 +275,17 @@ export function createTenantHttpApp(deps: TenantHttpAppDeps): Hono { const existing = await deps.registry.get(name, product); if (!existing) return c.json({ error: "tenant_not_found" }, 404); + // #9143 (defect 8 minimum): a tenant still standing up has no container/database/secrets settled yet -- + // tearing it down mid-provision races the in-flight provisionTenant call (which is still writing its OWN + // "active"/"failed" transition to this same record, #7677) and there is nothing durable yet to actually + // revoke/drop/destroy. KV's eventual consistency means even THIS read-then-act check is advisory, not a + // real compare-and-swap (see tenant-registry.ts's own header comment on why a real serialized claim needs + // a Durable Object or D1) -- but refusing the request while the record is observably "provisioning" is + // strictly better than racing it silently, and costs nothing: the caller can simply retry once the + // in-flight create settles to "active" or "failed". + if (existing.state === "provisioning") { + return c.json({ error: "tenant_provisioning", message: `tenant "${name}" is still provisioning; retry once it settles` }, 409); + } const result = await deprovisionTenant(existing.tenant, existing.product, deps.driver, deps.pagerDuty ?? {}, existing.secretRef); await deps.registry.upsert({ tenant: result.tenant, product: result.product, state: result.state, createdAt: existing.createdAt, updatedAt: new Date().toISOString() }); diff --git a/control-plane/src/neon-database-driver.ts b/control-plane/src/neon-database-driver.ts index 578614d691..18034afcbe 100644 --- a/control-plane/src/neon-database-driver.ts +++ b/control-plane/src/neon-database-driver.ts @@ -50,27 +50,40 @@ type NeonEndpoint = { host: string }; type NeonRole = { name: string; password?: string }; -// #8026: the unconditional .slice(0, 63) below used to have no collision guard -- two distinct tenant names +// #8026: the unconditional .slice(0, 63) used to have no collision guard -- two distinct tenant names // sharing the same first ~54 characters (after the "tenant--" prefix and sanitization) would // truncate to the IDENTICAL Neon branch name. findBranchByName would then find the OTHER tenant's already- // existing branch and hand back its connection/role/password to the new tenant -- a cross-tenant data- -// isolation bug. Only names that actually need truncating get the suffix, so a short tenant name's branch -// name is completely unchanged (this repo has never deployed against a live Neon project yet -- see this -// file's own header comment -- so there is no pre-existing long-name branch a suffix could orphan). +// isolation bug. #8026 only appended a collision suffix past the 63-char truncation threshold. +// +// #9143: that guard missed a SHORTER, much more common collision -- two tenant names that are distinct as +// registry keys (raw strings, validated only as `typeof name === "string" && !!name.trim()` at http-app.ts) +// but sanitize down to the IDENTICAL string well under 63 characters: "Acme" and "acme" both lowercase to +// "acme"; "acme.corp", "acme-corp", and "acme corp" all collapse `[^a-z0-9_-]+` runs to the same single +// hyphen. None of these ever reached the truncation branch, so they shared one branch/database/role/password +// exactly like the pre-#8026 long-name bug. The suffix now (a) is APPENDED UNCONDITIONALLY, not only when +// truncating, and (b) is hashed from the RAW, pre-sanitization `${product}:${tenant.name}` composite -- not +// from `sanitized` (hashing the already-lowercased/collapsed string can't distinguish "Acme" from "acme": by +// the time `sanitized` exists, the information that made them different is already gone). Hashing the raw +// identity instead means the registry key (product + exact tenant name) and the derived branch name are +// effectively 1:1 -- two names that sanitize identically still need to actually BE the same raw name to +// produce the same suffix, and therefore the same branch. const NEON_BRANCH_NAME_MAX_LENGTH = 63; const NEON_BRANCH_NAME_COLLISION_SUFFIX_LENGTH = 8; -/** Neon branch names are case-sensitive but this keeps them predictable and collision-free across products - * sharing a tenant name, and safely truncated well under Neon's own length limit. A name that would - * otherwise be truncated gets a short hash-of-the-untruncated-name suffix instead, so two long, - * prefix-similar tenant names can never collide on the same truncated branch name (#8026). */ +/** Neon branch names are case-sensitive but this keeps them predictable, collision-free across products and + * across sanitize-colliding tenant names (#8026, #9143), and safely truncated well under Neon's own length + * limit. Every name gets a short hash-of-the-raw-identity suffix, so two distinct tenant names -- whether + * they collide by sharing a long common prefix past the truncation point (#8026) or by sanitizing down to + * the same short string (#9143, e.g. differing only in case or punctuation) -- can never resolve to the + * same branch. */ function branchNameFor(request: TenantProvisioningRequest): string { const raw = `tenant-${request.product}-${request.tenant.name}`.toLowerCase(); const sanitized = raw.replaceAll(/[^a-z0-9_-]+/g, "-").replaceAll(/-{2,}/g, "-").replace(/^-+|-+$/g, ""); - if (sanitized.length <= NEON_BRANCH_NAME_MAX_LENGTH) return sanitized; - const suffix = createHash("sha256").update(sanitized).digest("hex").slice(0, NEON_BRANCH_NAME_COLLISION_SUFFIX_LENGTH); + const suffix = createHash("sha256").update(`${request.product}:${request.tenant.name}`).digest("hex").slice(0, NEON_BRANCH_NAME_COLLISION_SUFFIX_LENGTH); const prefixLength = NEON_BRANCH_NAME_MAX_LENGTH - 1 - suffix.length; - return `${sanitized.slice(0, prefixLength)}-${suffix}`; + const prefix = sanitized.length > prefixLength ? sanitized.slice(0, prefixLength) : sanitized; + return `${prefix}-${suffix}`; } /** A tenant-scoped role gets the SAME derived name as its branch -- one branch, one role, one database, no diff --git a/control-plane/src/orb-webhook-router.ts b/control-plane/src/orb-webhook-router.ts index 9ededc4e9b..2f5c95c430 100644 --- a/control-plane/src/orb-webhook-router.ts +++ b/control-plane/src/orb-webhook-router.ts @@ -34,8 +34,22 @@ export type OrbWebhookRouterConfig = { * blank ⇒ every delivery fails closed with 401, matching http-app.ts's own `adminToken` convention and the * main app's own "inert until the secret is injected" comment on this exact check. */ webhookSecret: string | undefined; + /** Body-size ceiling (#9143, defect 6) -- mirrors the main app's OWN `src/orb/webhook.ts#handleOrbWebhook` + * (`GITHUB_WEBHOOK_MAX_BODY_BYTES`-driven, fixed there by #8888), which this route's own header comment + * claims to mirror end-to-end but never actually applied: this route used to buffer an UNBOUNDED body + * TWICE before ever checking the signature (once via `request.clone()`, once via `request.text()`), with no + * content-length precheck and no streaming limit at all -- an unauthenticated caller (this route sits + * OUTSIDE the admin Bearer wall by design) could exhaust memory with an arbitrarily large POST before the + * HMAC check ever ran. Optional so every existing caller/test that doesn't care about the limit gets + * {@link DEFAULT_MAX_ORB_WEBHOOK_BODY_BYTES}. */ + maxBodyBytes?: number; }; +/** Same default as `src/orb/webhook.ts`'s own `DEFAULT_MAX_ORB_WEBHOOK_BODY_BYTES` (#9143) -- duplicated, not + * imported, for the same "separate workspace, no cross-package dependency" reason `verifyGitHubSignature` + * below already is. */ +const DEFAULT_MAX_ORB_WEBHOOK_BODY_BYTES = 1024 * 1024; + /** Same `${product}:${name}` composite container-driver.ts's own `instanceNameFor` derives -- duplicated (not * imported) for the same reason ams-wake.ts's own copy is: this module has no `TenantProvisioningRequest` to * construct, just a name/product pair already in hand from the registry lookup below. */ @@ -43,6 +57,40 @@ function instanceNameFor(name: string, product: Product): string { return `${product}:${name}`; } +/** Same shape as `src/utils/json.ts`'s own `parsePositiveInt` -- duplicated locally for the same + * no-cross-package-import reason as everything else in this file. */ +function parsePositiveInt(value: string | null | undefined): number | null { + if (!value) return null; + if (!/^\d+$/.test(value)) return null; + const parsed = Number.parseInt(value, 10); + if (!Number.isFinite(parsed) || parsed <= 0) return null; + return parsed; +} + +/** Reads the request body incrementally, aborting with `null` the instant the running total exceeds + * `maxBytes` -- rather than buffering the whole thing (however large) and checking its length only + * afterward. Same shape as `src/orb/webhook.ts`'s own `readBodyWithLimit`, duplicated for the same + * no-cross-package-import reason. An absent body (`request.body === null`, e.g. a GET-shaped request) is a + * valid empty string, matching that file's own convention. */ +async function readBodyWithLimit(request: Request, maxBytes: number): Promise { + const stream = request.body; + if (!stream) return ""; + const reader = stream.getReader(); + const decoder = new TextDecoder(); + const chunks: string[] = []; + let total = 0; + while (true) { + const { done, value } = await reader.read(); + if (done) break; + if (!value) continue; + total += value.byteLength; + if (total > maxBytes) return null; + chunks.push(decoder.decode(value, { stream: true })); + } + chunks.push(decoder.decode()); + return chunks.join(""); +} + /** Verifies a GitHub webhook's `x-hub-signature-256` HMAC-SHA256 against the raw request body -- the exact * same algorithm as the main app's `src/utils/crypto.ts#verifyGitHubSignature` (this package can't import * that file; it's a separate workspace with no dependency on the main app), duplicated locally rather than @@ -81,10 +129,22 @@ function hexToBytes(hex: string): Uint8Array | null { * it can't. `request` is consumed for signature verification (its raw body text) but a `.clone()` taken * BEFORE that read is what actually gets forwarded -- the tenant's container re-verifies the same signature * against the same untouched body, so a body-reconstruction bug here can't silently diverge from what GitHub - * actually signed. */ + * actually signed. This route sits OUTSIDE the admin Bearer wall (GitHub authenticates via its own HMAC + * signature, checked below) -- so the body-size limit (#9143, defect 6) is checked BEFORE that clone/read + * ever buffers anything: a cheap content-length precheck first (works whenever the real transport sets the + * header), then a hard ceiling enforced while streaming the body incrementally either way. */ export async function routeOrbWebhook(config: OrbWebhookRouterConfig, request: Request): Promise { + const maxBodyBytes = config.maxBodyBytes ?? DEFAULT_MAX_ORB_WEBHOOK_BODY_BYTES; + const contentLength = parsePositiveInt(request.headers.get("content-length")); + if (contentLength !== null && contentLength > maxBodyBytes) { + return Response.json({ error: "payload_too_large", maxBytes: maxBodyBytes }, { status: 413 }); + } + const forwardRequest = request.clone(); - const rawBody = await request.text(); + const rawBody = await readBodyWithLimit(request, maxBodyBytes); + if (rawBody === null) { + return Response.json({ error: "payload_too_large", maxBytes: maxBodyBytes }, { status: 413 }); + } const signature = request.headers.get("x-hub-signature-256"); const verified = await verifyGitHubSignature(rawBody, signature, config.webhookSecret); diff --git a/control-plane/src/secret-driver.ts b/control-plane/src/secret-driver.ts index 2f095ba63b..4cd932e290 100644 --- a/control-plane/src/secret-driver.ts +++ b/control-plane/src/secret-driver.ts @@ -19,9 +19,19 @@ // Deliberately does NOT implement the full `TenantProvisioningDriver` interface -- only injectSecrets/ // revokeSecrets (see `SecretDriver` below). `withRealSecretDriver` (driver-factory.ts) composes this onto an // otherwise fake/real-mixed driver, same shape as `withRealDatabaseDriver`/`withRealContainerDriver`. -import type { TenantProvisioningRequest } from "./tenant-provisioning-driver.js"; +// +// #9143 (defect 5, deploy blocker): an ORB tenant's own webhook secret rides in the SAME bundled enrollment as +// its database credential -- see injectTenantSecrets' own doc comment for why this bundles rather than mints a +// second enrollment/bootstrap token. `src/orb/hosted-webhook-secret.ts` (the main app, a separate workspace) is +// the consumer that unwraps this payload via `fetchBrokeredStoredSecret` and feeds it to +// `handleOrbWebhook`/`verifyGitHubSignature` -- the two files agree on the wire shape by convention (the +// existing "no cross-package import, duplicate the constant/shape" posture this whole package already uses), +// not by a shared type. +import { randomBytes } from "node:crypto"; +import type { DatabaseConnectionDetails, TenantProvisioningRequest } from "./tenant-provisioning-driver.js"; const DEFAULT_TIMEOUT_MS = 10_000; +const ORB_WEBHOOK_SECRET_BYTE_LENGTH = 32; // #8064's own `ORB_SECRET_TYPE_TENANT_DB_CREDENTIAL` constant, duplicated here rather than imported -- this // package has no dependency on the main app's `src/` (a separate npm workspace, its own package.json/tsconfig), @@ -68,19 +78,52 @@ async function mainAppFetch(config: SecretDriverConfig, method: string, path: return (text ? JSON.parse(text) : undefined) as T; } -/** Stores a tenant's already-provisioned database connection details (`request.database`, #7653) in the main - * app's token broker as a #8064 `tenant_db_credential` enrollment. The WHOLE `DatabaseConnectionDetails` - * object is stored (JSON-encoded), not just the bare `connectionString` -- a later reader gets every field - * back, not just what it can re-parse out of a URI, mirroring that type's own "kept alongside the parts" - * rationale. Returns the enrollment's `enrollId` as this driver's `secretRef` -- the caller (`provisionTenant`, - * via its own result) must persist this to revoke it later -- AND the one-time exchange `secret` as - * `bootstrapSecret` (#8202): the caller threads this into the tenant's container at its next `createContainer` - * call, so the container can itself present it to `/v1/orb/token` and get this exact value back. Previously - * discarded here (see this file's former header comment); #8202 is what actually consumes it now. */ +/** #9143 (defect 5): a fresh, per-tenant, cryptographically random webhook secret for an ORB tenant's own + * container to verify inbound GitHub deliveries with -- see this module's header comment on why it's bundled + * into the SAME enrollment as the database credential rather than minted as a second one. Hex-encoded (not + * base64) purely so it reads identically to every other opaque token this codebase generates (`createHash` + * suffixes in neon-database-driver.ts, etc.) -- the value is never parsed back apart, only compared + * byte-for-byte by `verifyGitHubSignature`, so any sufficiently random encoding would do. */ +function generateOrbWebhookSecret(): string { + return randomBytes(ORB_WEBHOOK_SECRET_BYTE_LENGTH).toString("hex"); +} + +/** The JSON shape bundled into a single `tenant_db_credential` enrollment's `secretValue` (#8064's storage is + * a free-text JSON blob, keyed only by the opaque `secretType` string -- this is a private wire contract + * between this function and `src/orb/hosted-webhook-secret.ts`'s consumer, not a shared type). `orbWebhookSecret` + * is present only for `product: "orb"` tenants -- AMS's one-shot CLI image never verifies an inbound webhook, + * so generating one for it would be dead material with no consumer, ever. */ +type TenantBootstrapPayload = { database: DatabaseConnectionDetails; orbWebhookSecret?: string }; + +/** Stores a tenant's already-provisioned database connection details (`request.database`, #7653) -- and, for + * an ORB tenant, a freshly generated webhook secret (#9143, defect 5) -- in the main app's token broker as a + * single #8064 `tenant_db_credential` enrollment. The WHOLE bundled payload is stored (JSON-encoded), not + * just the bare `connectionString` -- a later reader gets every field back, not just what it can re-parse + * out of a URI, mirroring `DatabaseConnectionDetails`' own "kept alongside the parts" rationale. + * + * #9143 (defect 5, deploy blocker): orb-webhook-router.ts's own header comment claims a hosted tenant's + * container "runs the SAME self-host webhook-handling code unmodified and re-verifies independently" -- but + * that handler (`src/orb/webhook.ts#handleOrbWebhook`) fails closed on a missing `ORB_GITHUB_WEBHOOK_SECRET`, + * and `createTenantContainer` (container-driver.ts) never injected one: every correctly-signed delivery would + * 401 forever. Rather than mint a SECOND enrollment/bootstrap-token pair purely for this one extra value + * (doubling the container's cold-boot exchange traffic and this driver's own interface surface for what is, + * from the container's perspective, a single "give me my secrets" moment), the webhook secret rides in the + * SAME bundle the container already fetches for its database credential -- one enrollment, one + * `bootstrapSecret`, one `POST /v1/orb/token` exchange at boot/first-request (`src/orb/hosted-webhook-secret.ts` + * is the consumer that unwraps it and feeds `handleOrbWebhook`). + * + * Returns the enrollment's `enrollId` as this driver's `secretRef` -- the caller (`provisionTenant`, via its + * own result) must persist this to revoke it later -- AND the one-time exchange `secret` as `bootstrapSecret` + * (#8202): the caller threads this into the tenant's container at its next `createContainer` call, so the + * container can itself present it to `/v1/orb/token` and get this exact bundled payload back. */ export async function injectTenantSecrets(config: SecretDriverConfig, request: TenantProvisioningRequest): Promise<{ secretRef?: string; bootstrapSecret?: string }> { if (!request.database) { throw new Error(`injectTenantSecrets: no database connection details on the request for tenant "${request.tenant.name}"`); } + const payload: TenantBootstrapPayload = { + database: request.database, + ...(request.product === "orb" ? { orbWebhookSecret: generateOrbWebhookSecret() } : {}), + }; // The route's own error responses (#8064: secret_value_required/encryption_unavailable) always pair an // `{error}` body with a non-2xx status -- mainAppFetch already throws on those, so there's no in-band // `{error}`-at-200 shape for this driver to check for separately. @@ -88,7 +131,7 @@ export async function injectTenantSecrets(config: SecretDriverConfig, request: T config, "POST", "/v1/internal/orb/enrollments", - { secretType: SECRET_TYPE_TENANT_DB_CREDENTIAL, secretValue: JSON.stringify(request.database) }, + { secretType: SECRET_TYPE_TENANT_DB_CREDENTIAL, secretValue: JSON.stringify(payload) }, ); return { secretRef: result.enrollId, bootstrapSecret: result.secret }; } diff --git a/control-plane/src/tenant-registry.ts b/control-plane/src/tenant-registry.ts index f3e7ff9d1f..d53a08f118 100644 --- a/control-plane/src/tenant-registry.ts +++ b/control-plane/src/tenant-registry.ts @@ -137,7 +137,22 @@ export function createKvTenantRegistry(kv: KvNamespaceLike): TenantRegistry { const previousRaw = await kv.get(primaryKey); const previous = previousRaw ? (JSON.parse(previousRaw) as TenantRegistryRecord) : undefined; if (previous?.orbInstallationId !== undefined && previous.orbInstallationId !== record.orbInstallationId) { - await kv.delete(installationIndexKeyFor(previous.orbInstallationId)); + // #9143: only clear the index entry if it STILL points at THIS tenant's own primary key. http-app.ts's + // `isRecreatableState` deliberately lets a "failed"/"torn down" tenant's installation claim be taken + // over by a brand-new tenant of a different name -- so by the time a stale record (tenant A, still + // carrying the old installationId because it was only ever written at create time) is finally + // re-created or torn down, the SAME installationId may already belong to a different, currently-ACTIVE + // tenant B (whose own later upsert already repointed this index key at B's primary key). Deleting it + // unconditionally here would rip out B's live webhook-routing pointer even though B never changed its + // own installationId -- every subsequent webhook for B then 404s forever, with no way to re-claim it + // (create 409s since B already exists; orbInstallationId is settable only at create). Reading the + // index before deleting makes the delete a compare-and-delete: it only ever removes an entry this + // exact upsert is the rightful owner of. + const indexKey = installationIndexKeyFor(previous.orbInstallationId); + const currentIndexValue = await kv.get(indexKey); + if (currentIndexValue === primaryKey) { + await kv.delete(indexKey); + } } await kv.put(primaryKey, JSON.stringify(record)); if (record.orbInstallationId !== undefined) { diff --git a/control-plane/src/worker.ts b/control-plane/src/worker.ts index 4ac5313b9f..8cbf5f7fd0 100644 --- a/control-plane/src/worker.ts +++ b/control-plane/src/worker.ts @@ -14,21 +14,56 @@ import { createKvTenantRegistry } from "./tenant-registry.js"; const PROVISIONED_STORAGE_KEY = "provisioned"; +// #9143 (defect 4): the env map createTenantContainer (container-driver.ts) hands to a tenant's ONE-TIME +// cold-boot `start({envVars})` call is otherwise only ever held in the vendored `@cloudflare/containers` +// SDK's own `this.envVars` instance property (a plain class field, default `{}`) -- confirmed against that +// package's source (node_modules/@cloudflare/containers/dist/lib/container.js): `doStartContainer` resolves +// `options?.envVars ?? this.envVars`, and NOTHING in the SDK ever assigns `this.envVars` from a bare +// `start(options)` call -- only this class's OWN constructor `options` (never supplied here) does. A tenant +// container's auto-wake-on-request path after it sleeps (`containerFetch`'s implicit restart, which calls +// `start()` with NO options at all) therefore falls through to `this.envVars`, which resets to `{}` on every +// FRESH instance of this class -- a Durable Object's in-memory fields do not survive eviction, only +// `ctx.storage` does. A tenant's pinned version / bootstrap secret / central PostHog key would silently +// vanish from its container's environment on its very next restart. Fixed the same way +// PROVISIONED_STORAGE_KEY already is: persisted in this DO's own durable storage and reloaded into +// `this.envVars` in the constructor, via `ctx.blockConcurrencyWhile` -- the exact pattern the base `Container` +// class's own constructor already uses for its own async setup -- so no request or implicit restart can ever +// reach `doStartContainer` before the persisted value is back in place. +const ENV_VARS_STORAGE_KEY = "envVars"; + /** Shared base for both product-specific Container classes below: tracks whether THIS tenant's container has * been explicitly provisioned, in the DO's own durable storage -- independent of Cloudflare's own transient * container run-state (`getState()`'s running/stopped/stopped_with_code/etc). That distinction is * load-bearing, concretely for AMS: its one-shot CLI image (see AmsTenantContainer below) is EXPECTED to sit * in a "stopped"-shaped run state almost all the time by design (#7182), indistinguishable from "never * provisioned" using run-state alone -- container-driver.ts's header comment covers this in full. */ -class ProvisionedContainer extends Container { +class ProvisionedContainer extends Container { + // `DurableObjectState>` matches the vendored SDK's own `Container`'s inherited + // `ctx` type exactly (it extends `cloudflare:workers`' `DurableObject`, whose `Props` + // defaults to an empty-object shape) -- `DurableObjectState` alone defaults its own generic to `unknown`, + // which `super(ctx, env)` below then rejects as too loose. + constructor(ctx: DurableObjectState>, env: Env) { + super(ctx, env); + // Not awaited (constructors can't be async) -- `ctx.blockConcurrencyWhile` itself is what defers every + // future request/alarm on this DO until the callback resolves, same fire-and-forget shape the base + // `Container` constructor already uses internally for its own setup. + ctx.blockConcurrencyWhile(async () => { + const persisted = await ctx.storage.get>(ENV_VARS_STORAGE_KEY); + if (persisted) this.envVars = persisted; + }); + } async isProvisioned(): Promise { return (await this.ctx.storage.get(PROVISIONED_STORAGE_KEY)) === true; } - async markProvisioned(): Promise { + async markProvisioned(envVars?: Record): Promise { await this.ctx.storage.put(PROVISIONED_STORAGE_KEY, true); + if (envVars && Object.keys(envVars).length > 0) { + await this.ctx.storage.put(ENV_VARS_STORAGE_KEY, envVars); + } } async markDeprovisioned(): Promise { await this.ctx.storage.delete(PROVISIONED_STORAGE_KEY); + await this.ctx.storage.delete(ENV_VARS_STORAGE_KEY); } } diff --git a/control-plane/test/ams-wake.test.ts b/control-plane/test/ams-wake.test.ts index 05f83c524a..f3eaeb1a67 100644 --- a/control-plane/test/ams-wake.test.ts +++ b/control-plane/test/ams-wake.test.ts @@ -261,6 +261,92 @@ test("wakeDueAmsTenants: wakes multiple due tenants sequentially, not concurrent assert.deepEqual(order, ["acme-start", "beta-start"]); }); +// #9143 (defect 7): the load-bearing overlap-guard regression test. Before this fix, nextDueAt only advanced +// AFTER a woken tenant's poll resolved -- a genuinely-overlapping tick (the 5-minute cron firing again while a +// slow/hung poll from the PRIOR tick was still running) would see the SAME tenant still due and wake it a +// second time concurrently, with no lock or in-flight marker anywhere to stop it. nextDueAt is now claimed +// BEFORE the poll starts, so an overlapping tick observes the claim and skips the tenant entirely. +test("wakeDueAmsTenants: an overlapping tick does not re-wake a tenant whose poll from the PRIOR tick hasn't resolved yet (#9143)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ + tenant: { name: "acme" }, + product: "ams", + // A 1-hour cadence: long enough that tick 2 (5 minutes after tick 1, per wrangler.jsonc's `*/5 * * * *`) + // is NOT yet due again on its own merits -- the only way tick 2 could still see "acme" as due is the + // PRE-#9143 bug (nextDueAt never advanced until the poll resolved), not a legitimately-elapsed interval. + state: "active", + createdAt: "t0", + updatedAt: "t0", + amsSchedule: { command: "discover", args: [], intervalMs: 60 * 60_000, nextDueAt: PAST }, + }); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + let getStateCallCount = 0; + const stub: FakeWakeStub = { + starts: [], + getStateCalls: 0, + async start() {}, + async getState() { + getStateCallCount += 1; + await gate; // simulates a hung/slow container: the PRIOR tick's poll is still in flight + return { status: "stopped_with_code", exitCode: 0 }; + }, + }; + const namespace = fakeNamespace({ "ams:acme": stub }); + + // Tick 1 (the current cron invocation) starts waking "acme" and gets stuck mid-poll. + const tick1 = wakeDueAmsTenants(baseConfig({ binding: namespace, registry, now: () => NOW })); + await new Promise((resolve) => setImmediate(resolve)); + + try { + // Tick 2 (the SAME cron firing again 5 minutes later) must NOT see "acme" as due anymore -- its + // nextDueAt was already claimed forward by tick 1, before tick 1's poll ever started, not after it + // resolves. + const tick2 = await wakeDueAmsTenants(baseConfig({ binding: namespace, registry, now: () => new Date(NOW.getTime() + 5 * 60_000) })); + + assert.deepEqual(tick2, []); + assert.equal(getStateCallCount, 1); // tick 2 never touched this tenant's container at all + } finally { + // Always unblock tick 1 and let it finish, even if an assertion above threw -- otherwise its promise + // dangles past the end of this test and the runner cancels every later test in the file. + release(); + await tick1; + } +}); + +// #9143 (defect 7): bounds a single tick's total wall-clock exposure across MULTIPLE due tenants, not just +// one hung tenant -- a fleet with more due tenants than the cap in one 5-minute window defers the excess to +// the next tick instead. +test("wakeDueAmsTenants: maxTenantsPerTick bounds how many due tenants ONE tick wakes, deferring the rest to the next tick (#9143)", async () => { + const registry = createFakeTenantRegistry(); + for (const name of ["acme", "beta", "gamma"]) { + await registry.upsert({ + tenant: { name }, + product: "ams", + state: "active", + createdAt: "t0", + updatedAt: "t0", + amsSchedule: { command: "discover", args: [], intervalMs: 60_000, nextDueAt: PAST }, + }); + } + const namespace = fakeNamespace({ + "ams:acme": fakeWakeStub([{ status: "stopped_with_code", exitCode: 0 }]), + "ams:beta": fakeWakeStub([{ status: "stopped_with_code", exitCode: 0 }]), + "ams:gamma": fakeWakeStub([{ status: "stopped_with_code", exitCode: 0 }]), + }); + + const results = await wakeDueAmsTenants(baseConfig({ binding: namespace, registry, now: () => NOW, maxTenantsPerTick: 2 })); + + assert.equal(results.length, 2); + assert.deepEqual(results.map((r) => r.tenant.name), ["acme", "beta"]); + // "gamma" was left completely untouched -- still due (nextDueAt unchanged), ready for the next tick. + const gamma = await registry.get("gamma", "ams"); + assert.equal(gamma?.amsSchedule?.nextDueAt, PAST); + assert.deepEqual(namespace.requestedNames, ["ams:acme", "ams:beta"]); +}); + test("wakeDueAmsTenants: defaults now/pollIntervalMs/pollTimeoutMs when not given", async () => { const registry = createFakeTenantRegistry(); await registry.upsert({ diff --git a/control-plane/test/container-driver.test.ts b/control-plane/test/container-driver.test.ts index 8a257ec669..15f837706a 100644 --- a/control-plane/test/container-driver.test.ts +++ b/control-plane/test/container-driver.test.ts @@ -18,13 +18,15 @@ import { type TenantProvisioningRequest, } from "../dist/index.js"; -type FakeContainerStub = ContainerStubLike & { calls: string[] }; +type FakeContainerStub = ContainerStubLike & { calls: string[]; markProvisionedEnvVars: Array | undefined> }; function fakeContainerStub(initial: { provisioned?: boolean } = {}): FakeContainerStub { let provisioned = initial.provisioned ?? false; const calls: string[] = []; + const markProvisionedEnvVars: Array | undefined> = []; return { calls, + markProvisionedEnvVars, async start() { calls.push("start"); }, @@ -34,8 +36,9 @@ function fakeContainerStub(initial: { provisioned?: boolean } = {}): FakeContain async isProvisioned() { return provisioned; }, - async markProvisioned() { + async markProvisioned(envVars) { calls.push("markProvisioned"); + markProvisionedEnvVars.push(envVars); provisioned = true; }, async markDeprovisioned() { @@ -67,6 +70,9 @@ test("createTenantContainer starts a fresh (never-provisioned) container and mar assert.deepEqual(stub.calls, ["start", "markProvisioned"]); assert.equal(await stub.isProvisioned(), true); + // #9143 (defect 4): a tenant with no env vars at all still calls markProvisioned with no envVars argument -- + // byte-identical to the pre-#9143 call shape. + assert.deepEqual(stub.markProvisionedEnvVars, [undefined]); }); test("createTenantContainer is idempotent: an already-provisioned tenant is never restarted", async () => { @@ -139,11 +145,13 @@ test("createContainerDriver bundles all three functions closed over one config", // level. The stub here captures start()'s options, which the package's shared fake deliberately doesn't. type StartOptions = Parameters[0]; -function optionCapturingStub(): ContainerStubLike & { startOptions: StartOptions[] } { +function optionCapturingStub(): ContainerStubLike & { startOptions: StartOptions[]; markProvisionedEnvVars: Array | undefined> } { let provisioned = false; const startOptions: StartOptions[] = []; + const markProvisionedEnvVars: Array | undefined> = []; return { startOptions, + markProvisionedEnvVars, async start(options?: StartOptions) { startOptions.push(options); }, @@ -151,7 +159,8 @@ function optionCapturingStub(): ContainerStubLike & { startOptions: StartOptions async isProvisioned() { return provisioned; }, - async markProvisioned() { + async markProvisioned(envVars) { + markProvisionedEnvVars.push(envVars); provisioned = true; }, async markDeprovisioned() { @@ -170,6 +179,9 @@ test("a pinned tenant's container starts with PINNED_VERSION_ENV_VAR carrying it await createTenantContainer(configFor(stub), { tenant: { name: "acme", pinnedVersion: "v1.4.2" }, product: "orb" }); assert.deepEqual(stub.startOptions, [{ envVars: { [PINNED_VERSION_ENV_VAR]: "v1.4.2" } }]); + // #9143 (defect 4): the SAME envVars markProvisioned receives, so the real DO-backed stub can persist them + // and reload them into `this.envVars` on a future restart -- see ContainerStubLike's own doc comment. + assert.deepEqual(stub.markProvisionedEnvVars, [{ [PINNED_VERSION_ENV_VAR]: "v1.4.2" }]); }); test("an unpinned tenant's container start is byte-identical to the pre-#4898 call (no options at all)", async () => { @@ -179,6 +191,7 @@ test("an unpinned tenant's container start is byte-identical to the pre-#4898 ca await createTenantContainer(configFor(stub), { tenant, product: "orb" }); assert.deepEqual(stub.startOptions, [undefined]); + assert.deepEqual(stub.markProvisionedEnvVars, [undefined]); } }); @@ -248,4 +261,7 @@ test("the central PostHog key merges with per-tenant pinned-version and bootstra assert.deepEqual(stub.startOptions, [ { envVars: { [PINNED_VERSION_ENV_VAR]: "v1.4.2", [TENANT_SECRET_ENV_VAR]: "orbsec_xyz", [CENTRAL_POSTHOG_KEY_ENV_VAR]: "phc_central123" } }, ]); + assert.deepEqual(stub.markProvisionedEnvVars, [ + { [PINNED_VERSION_ENV_VAR]: "v1.4.2", [TENANT_SECRET_ENV_VAR]: "orbsec_xyz", [CENTRAL_POSTHOG_KEY_ENV_VAR]: "phc_central123" }, + ]); }); diff --git a/control-plane/test/http-app.test.ts b/control-plane/test/http-app.test.ts index 0d47b487ea..ac7bb7dae1 100644 --- a/control-plane/test/http-app.test.ts +++ b/control-plane/test/http-app.test.ts @@ -309,6 +309,151 @@ test("POST /v1/tenants allows claiming an orbInstallationId that a torn-down ten assert.equal((await registry.get("newcomer", "orb"))?.orbInstallationId, 555); }); +// #9143 (defect 2 manual recovery): PATCH /v1/tenants/:name/orb-installation re-links an existing tenant's +// orbInstallationId -- the manual recovery path for a tenant whose installation-index pointer was wiped by +// the pre-fix tenant-registry.ts bug (see that file's own header comment, and its own regression test). + +test("PATCH /v1/tenants/:name/orb-installation re-links an existing ORB tenant to a new installation ID", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 999 }) }), + ); + + assert.equal(res.status, 200); + const payload = (await res.json()) as { orbInstallationId?: number }; + assert.equal(payload.orbInstallationId, 999); + assert.equal((await registry.get("acme", "orb"))?.orbInstallationId, 999); +}); + +test("PATCH /v1/tenants/:name/orb-installation re-linking to the SAME id it already has never 409s against itself", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0", orbInstallationId: 555 }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 555 }) }), + ); + + assert.equal(res.status, 200); + assert.equal((await registry.get("acme", "orb"))?.orbInstallationId, 555); +}); + +test("PATCH /v1/tenants/:name/orb-installation rejects a missing product query parameter (400)", async () => { + const app = createTenantHttpApp(baseDeps()); + + const res = await app.request("/v1/tenants/acme/orb-installation", authed({ method: "PATCH", body: JSON.stringify({ orbInstallationId: 1 }) })); + + assert.equal(res.status, 400); + assert.equal((await res.json() as { error: string }).error, "invalid_request"); +}); + +test("PATCH /v1/tenants/:name/orb-installation rejects a non-orb product (400)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "ams", state: "active", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=ams", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 1 }) }), + ); + + assert.equal(res.status, 400); + assert.deepEqual(await res.json(), { error: "invalid_request", message: 'orbInstallationId is only valid for product "orb"' }); +}); + +test("PATCH /v1/tenants/:name/orb-installation on an unknown tenant is a 404", async () => { + const app = createTenantHttpApp(baseDeps()); + + const res = await app.request( + "/v1/tenants/ghost/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 1 }) }), + ); + + assert.equal(res.status, 404); + assert.deepEqual(await res.json(), { error: "tenant_not_found" }); +}); + +test("PATCH /v1/tenants/:name/orb-installation rejects invalid JSON (400)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request("/v1/tenants/acme/orb-installation?product=orb", authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: "not json" })); + + assert.equal(res.status, 400); + assert.deepEqual(await res.json(), { error: "invalid_json" }); +}); + +test("PATCH /v1/tenants/:name/orb-installation requires orbInstallationId in the body (400)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request("/v1/tenants/acme/orb-installation?product=orb", authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({}) })); + + assert.equal(res.status, 400); + assert.deepEqual(await res.json(), { error: "invalid_request", message: "orbInstallationId is required" }); +}); + +test("PATCH /v1/tenants/:name/orb-installation rejects a malformed orbInstallationId (400)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: -1 }) }), + ); + + assert.equal(res.status, 400); + assert.deepEqual(await res.json(), { error: "invalid_request", message: "orbInstallationId must be a positive integer" }); +}); + +test("PATCH /v1/tenants/:name/orb-installation refuses to steal an installation ID a DIFFERENT active tenant still holds (409)", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + await registry.upsert({ tenant: { name: "other" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0", orbInstallationId: 555 }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 555 }) }), + ); + + assert.equal(res.status, 409); + assert.deepEqual(await res.json(), { error: "installation_already_claimed", message: 'installation 555 is already claimed by tenant "other"' }); + assert.equal((await registry.get("acme", "orb"))?.orbInstallationId, undefined); +}); + +test("PATCH /v1/tenants/:name/orb-installation allows reclaiming an ID from a torn-down tenant that used to hold it", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); + await registry.upsert({ tenant: { name: "old" }, product: "orb", state: "torn down", createdAt: "t0", updatedAt: "t0", orbInstallationId: 555 }); + const app = createTenantHttpApp(baseDeps({ registry })); + + const res = await app.request( + "/v1/tenants/acme/orb-installation?product=orb", + authed({ method: "PATCH", headers: { "content-type": "application/json" }, body: JSON.stringify({ orbInstallationId: 555 }) }), + ); + + assert.equal(res.status, 200); + assert.equal((await registry.get("acme", "orb"))?.orbInstallationId, 555); +}); + +test("PATCH /v1/tenants/:name/orb-installation sits behind the same Bearer wall as every other /v1/tenants route", async () => { + const app = createTenantHttpApp(baseDeps()); + + const res = await app.request("/v1/tenants/acme/orb-installation?product=orb", { method: "PATCH", body: JSON.stringify({ orbInstallationId: 1 }) }); + + assert.equal(res.status, 401); + assert.deepEqual(await res.json(), { error: "unauthorized" }); +}); + test("GET /v1/tenants surfaces an ORB tenant's orbInstallationId when set", async () => { const registry = createFakeTenantRegistry(); await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0", orbInstallationId: 555 }); @@ -530,6 +675,37 @@ test("DELETE /v1/tenants/:name on an unknown tenant is a 404, not a silent no-op assert.deepEqual(await res.json(), { error: "tenant_not_found" }); }); +// #9143 (defect 8 minimum): DELETE used to have no state check at all -- a tenant still mid-standup (no +// container/database/secrets settled) could be torn down while provisionTenant was still racing to write its +// own "active"/"failed" transition to the SAME record. Refusing the request while "provisioning" is the +// documented minimum fix (the full DO/D1 serialized-claim migration is deferred, see tenant-registry.ts). +test("DELETE /v1/tenants/:name refuses a tenant that is still provisioning (409, not idempotent)", async () => { + const registry = createFakeTenantRegistry(); + const driver = createFakeTenantProvisioningDriver(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "provisioning", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry, driver })); + + const res = await app.request("/v1/tenants/acme?product=orb", authed({ method: "DELETE" })); + + assert.equal(res.status, 409); + assert.deepEqual(await res.json(), { error: "tenant_provisioning", message: 'tenant "acme" is still provisioning; retry once it settles' }); + // Untouched -- still provisioning, no driver teardown call was ever made. + assert.equal((await registry.get("acme", "orb"))?.state, "provisioning"); + assert.equal(driver.calls.length, 0); +}); + +test("DELETE /v1/tenants/:name still tears down a 'failed' tenant (only 'provisioning' is refused)", async () => { + const registry = createFakeTenantRegistry(); + const driver = createFakeTenantProvisioningDriver(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "failed", createdAt: "t0", updatedAt: "t0" }); + const app = createTenantHttpApp(baseDeps({ registry, driver })); + + const res = await app.request("/v1/tenants/acme?product=orb", authed({ method: "DELETE" })); + + assert.equal(res.status, 200); + assert.equal((await registry.get("acme", "orb"))?.state, "torn down"); +}); + test("DELETE /v1/tenants/:name URL-decodes the name path segment", async () => { const registry = createFakeTenantRegistry(); await registry.upsert({ tenant: { name: "acme corp" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); @@ -711,126 +887,28 @@ test("a rejection writing the 'failed' state never masks the provisioning error assert.equal((await registry.get("acme", "orb"))?.state, "provisioning"); }); -// #4898: POST /v1/tenants/rollout — pin/unpin an explicit list of tenants' pinnedVersion, all-or-nothing. -// The registry-seeding style mirrors the GET /v1/tenants tests above (records seeded directly, no driver run). - -function rollout(app: ReturnType, body: unknown) { - return app.request( - "/v1/tenants/rollout", - authed({ method: "POST", headers: { "content-type": "application/json" }, body: typeof body === "string" ? body : JSON.stringify(body) }), - ); -} - -test("POST /v1/tenants/rollout pins exactly the listed tenants and leaves every other tenant untouched (#4898 acceptance)", async () => { - const registry = createFakeTenantRegistry(); - await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - await registry.upsert({ tenant: { name: "beta" }, product: "ams", state: "active", createdAt: "t0", updatedAt: "t0" }); - await registry.upsert({ tenant: { name: "gamma" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - const app = createTenantHttpApp(baseDeps({ registry })); - - const res = await rollout(app, { names: ["acme", "gamma"], product: "orb", pinnedVersion: "v1.4.2" }); - - assert.equal(res.status, 200); - const payload = (await res.json()) as { tenants: Array<{ tenant: { name: string; pinnedVersion?: string | null } }> }; - assert.deepEqual(payload.tenants.map((t) => t.tenant), [ - { name: "acme", pinnedVersion: "v1.4.2" }, - { name: "gamma", pinnedVersion: "v1.4.2" }, - ]); - // The unlisted tenant (a different product, #8024) is completely unaffected — no pin, no updatedAt churn. - const beta = await registry.get("beta", "ams"); - assert.deepEqual(beta?.tenant, { name: "beta" }); - assert.equal(beta?.updatedAt, "t0"); - // The pinned tenants' records persisted the pin and kept their createdAt. - const acme = await registry.get("acme", "orb"); - assert.deepEqual(acme?.tenant, { name: "acme", pinnedVersion: "v1.4.2" }); - assert.equal(acme?.createdAt, "t0"); - assert.notEqual(acme?.updatedAt, "t0"); -}); - -test("POST /v1/tenants/rollout rolls back independently: re-pinning one tenant leaves another tenant's pin alone", async () => { - const registry = createFakeTenantRegistry(); - await registry.upsert({ tenant: { name: "acme", pinnedVersion: "v2.0.0" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - await registry.upsert({ tenant: { name: "beta", pinnedVersion: "v2.0.0" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - const app = createTenantHttpApp(baseDeps({ registry })); - - const back = await rollout(app, { names: ["acme"], product: "orb", pinnedVersion: "v1.9.0" }); - assert.equal(back.status, 200); - assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme", pinnedVersion: "v1.9.0" }); - assert.deepEqual((await registry.get("beta", "orb"))?.tenant, { name: "beta", pinnedVersion: "v2.0.0" }); - - // Explicit unpin (null) reverts the tenant to its release channel's default. - const unpin = await rollout(app, { names: ["acme"], product: "orb", pinnedVersion: null }); - assert.equal(unpin.status, 200); - assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme", pinnedVersion: null }); - assert.deepEqual((await registry.get("beta", "orb"))?.tenant, { name: "beta", pinnedVersion: "v2.0.0" }); -}); - -test("POST /v1/tenants/rollout trims the pinned version before storing it", async () => { - const registry = createFakeTenantRegistry(); - await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - const app = createTenantHttpApp(baseDeps({ registry })); - - const res = await rollout(app, { names: ["acme"], product: "orb", pinnedVersion: " v1.4.2 " }); +// #9143 (defect 3): POST /v1/tenants/rollout is now a deliberate 501 -- #4898's original implementation +// silently no-op'd on every one of its four promised effects (see http-app.ts's own header comment on the +// route). These tests prove the route is disabled explicitly (not a removed 404, and not a silent 200 that +// changes nothing) and still sits behind the same admin Bearer wall as every other /v1/tenants/* route. - assert.equal(res.status, 200); - assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme", pinnedVersion: "v1.4.2" }); -}); - -test("POST /v1/tenants/rollout 400s malformed bodies without touching any record", async () => { +test("POST /v1/tenants/rollout is disabled: an explicit 501, not a silent no-op (#9143 defect 3)", async () => { const registry = createFakeTenantRegistry(); await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); const app = createTenantHttpApp(baseDeps({ registry })); - const notJson = await rollout(app, "not json at all"); - assert.equal(notJson.status, 400); - assert.deepEqual(await notJson.json(), { error: "invalid_json" }); - - for (const [body, message] of [ - [[], "body must be a JSON object"], - [{ names: [], product: "orb", pinnedVersion: "v1" }, "names must be a non-empty array of tenant names"], - [{ names: "acme", product: "orb", pinnedVersion: "v1" }, "names must be a non-empty array of tenant names"], - [{ names: ["acme", " "], product: "orb", pinnedVersion: "v1" }, "names must be a non-empty array of tenant names"], - [{ names: ["acme", 7], product: "orb", pinnedVersion: "v1" }, "names must be a non-empty array of tenant names"], - [{ names: ["acme", "acme"], product: "orb", pinnedVersion: "v1" }, "names must not repeat a tenant"], - [{ names: ["acme"], pinnedVersion: "v1" }, "product is required"], - [{ names: ["acme"], product: " ", pinnedVersion: "v1" }, "product is required"], - [{ names: ["acme"], product: "orb", pinnedVersion: " " }, "pinnedVersion must be a non-blank string, or null to unpin"], - [{ names: ["acme"], product: "orb", pinnedVersion: 7 }, "pinnedVersion must be a non-blank string, or null to unpin"], - [{ names: ["acme"], product: "orb" }, "pinnedVersion must be a non-blank string, or null to unpin"], - ] as const) { - const res = await rollout(app, body); - assert.equal(res.status, 400, JSON.stringify(body)); - assert.deepEqual(await res.json(), { error: "invalid_request", message }); - } - assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme" }); -}); - -test("POST /v1/tenants/rollout is all-or-nothing: one unknown name 404s and applies nothing", async () => { - const registry = createFakeTenantRegistry(); - await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - const app = createTenantHttpApp(baseDeps({ registry })); - - const res = await rollout(app, { names: ["acme", "ghost"], product: "orb", pinnedVersion: "v1.4.2" }); - - assert.equal(res.status, 404); - assert.deepEqual(await res.json(), { error: "tenant_not_found", message: 'unknown tenant "ghost"' }); - assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme" }); -}); - -test("POST /v1/tenants/rollout 409s a torn-down tenant and applies nothing", async () => { - const registry = createFakeTenantRegistry(); - await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0" }); - await registry.upsert({ tenant: { name: "gone" }, product: "orb", state: "torn down", createdAt: "t0", updatedAt: "t0" }); - const app = createTenantHttpApp(baseDeps({ registry })); - - const res = await rollout(app, { names: ["acme", "gone"], product: "orb", pinnedVersion: "v1.4.2" }); + const res = await app.request( + "/v1/tenants/rollout", + authed({ method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({ names: ["acme"], product: "orb", pinnedVersion: "v1.4.2" }) }), + ); - assert.equal(res.status, 409); - assert.deepEqual(await res.json(), { error: "tenant_torn_down", message: 'tenant "gone" is torn down' }); + assert.equal(res.status, 501); + assert.deepEqual(await res.json(), { error: "not_implemented", message: "tenant rollout is not implemented yet (#9143)" }); + // Nothing was touched -- no silent partial effect the old handler used to have. assert.deepEqual((await registry.get("acme", "orb"))?.tenant, { name: "acme" }); }); -test("POST /v1/tenants/rollout sits behind the same Bearer wall as every other /v1/tenants route", async () => { +test("POST /v1/tenants/rollout still sits behind the same Bearer wall as every other /v1/tenants route", async () => { const app = createTenantHttpApp(baseDeps()); const res = await app.request("/v1/tenants/rollout", { method: "POST", body: JSON.stringify({ names: ["acme"], pinnedVersion: "v1" }) }); diff --git a/control-plane/test/neon-database-driver.test.ts b/control-plane/test/neon-database-driver.test.ts index 08baeb3544..860617ccd1 100644 --- a/control-plane/test/neon-database-driver.test.ts +++ b/control-plane/test/neon-database-driver.test.ts @@ -23,7 +23,9 @@ const CONFIG: NeonDatabaseDriverConfig = { }; const REQUEST: TenantProvisioningRequest = { tenant: { name: "acme" }, product: "orb" }; -const BRANCH_NAME = "tenant-orb-acme"; +// #9143: branchNameFor now ALWAYS appends a hash-of-the-raw-identity suffix (not only past the 63-char +// truncation threshold, #8026) -- this is that suffix for REQUEST's exact `orb:acme` identity. +const BRANCH_NAME = "tenant-orb-acme-a124aa31"; let originalFetch: typeof fetch; @@ -171,7 +173,7 @@ test("provisionNeonDatabase: idempotent re-provision resolves an existing branch }); assert.equal(calls.length, 3); assert.ok(calls.every((call) => call.init.method === "GET" || call.init.method === undefined)); - assert.equal(calls[2]?.url, "https://console.neon.tech/api/v2/projects/proj-1/branches/br-existing/roles/tenant-orb-acme/reveal_password"); + assert.equal(calls[2]?.url, `https://console.neon.tech/api/v2/projects/proj-1/branches/br-existing/roles/${BRANCH_NAME}/reveal_password`); }); test("provisionNeonDatabase: throws when an existing branch's role has no revealable password", async () => { @@ -272,7 +274,7 @@ test("provisionNeonDatabase: two long, prefix-similar tenant names produce DIFFE assert.ok(branchNameB.length <= 63); }); -test("provisionNeonDatabase: a short tenant name's branch name is completely unaffected by the collision-suffix logic", async () => { +test("provisionNeonDatabase: a short tenant name still gets the unconditional collision suffix (#9143)", async () => { const { calls } = mockFetchSequence([ { body: { branches: [] } }, { body: { branch: { id: "br-1", name: BRANCH_NAME }, endpoints: [{ host: "ep-1.neon.tech" }], operations: [] } }, @@ -282,5 +284,42 @@ test("provisionNeonDatabase: a short tenant name's branch name is completely una await provisionNeonDatabase(CONFIG, REQUEST); - assert.equal((bodyOf(calls[1]!.init) as { branch: { name: string } }).branch.name, BRANCH_NAME); + const branchName = (bodyOf(calls[1]!.init) as { branch: { name: string } }).branch.name; + assert.equal(branchName, BRANCH_NAME); + assert.match(branchName, /^tenant-orb-acme-[0-9a-f]{8}$/); +}); + +// #9143 (defect 1, deploy blocker): "Acme", "acme", "acme.corp", "acme-corp", and "acme corp" are all DISTINCT +// registry keys (http-app.ts's create-route validation is only `typeof name === "string" && !!name.trim()`) but +// used to sanitize+lowercase down to the IDENTICAL branch name, since the pre-#9143 suffix only ever applied +// past the 63-char truncation threshold (#8026) and, even then, was hashed from the ALREADY-sanitized string -- +// which had already thrown away the case/punctuation differences. provisionNeonDatabase's idempotent-reuse path +// (findBranchByName) would then resolve the FIRST tenant's already-existing branch for the second tenant, +// handing back its live database/role/password. This is the load-bearing isolation regression test: distinct +// raw tenant names must NEVER produce the same branch name, whether or not they sanitize identically. +test("provisionNeonDatabase: sanitize-colliding tenant names ('Acme' vs 'acme' vs 'acme.corp' vs 'acme-corp' vs 'acme corp') never share a branch (#9143 isolation)", async () => { + const variants: TenantProvisioningRequest[] = [ + { tenant: { name: "Acme" }, product: "orb" }, + { tenant: { name: "acme" }, product: "orb" }, + { tenant: { name: "acme.corp" }, product: "orb" }, + { tenant: { name: "acme-corp" }, product: "orb" }, + { tenant: { name: "acme corp" }, product: "orb" }, + ]; + + const branchNames: string[] = []; + for (const [index, request] of variants.entries()) { + const { calls } = mockFetchSequence([ + { body: { branches: [] } }, + { body: { branch: { id: `br-${index}`, name: "placeholder" }, endpoints: [{ host: `ep-${index}.neon.tech` }], operations: [] } }, + { body: { role: { name: "placeholder", password: `pw-${index}` }, operations: [] } }, + { body: { operations: [] } }, + ]); + await provisionNeonDatabase(CONFIG, request); + branchNames.push((bodyOf(calls[1]!.init) as { branch: { name: string } }).branch.name); + } + + // Every one of the 5 distinct raw tenant names produces a UNIQUE branch name -- including "acme.corp", + // "acme-corp", and "acme corp", which all sanitize down to the identical "acme-corp" prefix. + assert.equal(new Set(branchNames).size, variants.length); + for (const branchName of branchNames) assert.ok(branchName.length <= 63, branchName); }); diff --git a/control-plane/test/orb-webhook-router.test.ts b/control-plane/test/orb-webhook-router.test.ts index 08b808693b..168cc30a96 100644 --- a/control-plane/test/orb-webhook-router.test.ts +++ b/control-plane/test/orb-webhook-router.test.ts @@ -53,6 +53,125 @@ function baseConfig(overrides: Partial & { registry: Ten return { binding: fakeNamespace({}), webhookSecret: WEBHOOK_SECRET, ...overrides }; } +function streamBody(chunk: string): ReadableStream { + return new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(chunk)); + controller.close(); + }, + }); +} + +// #9143 (defect 6): this route used to buffer an UNBOUNDED body TWICE (a `.clone()` plus a `request.text()`) +// before ever checking the signature -- no content-length precheck, no streaming limit at all -- even though +// it sits OUTSIDE the admin Bearer wall (its declared twin, src/orb/webhook.ts#handleOrbWebhook, has both, +// governed by GITHUB_WEBHOOK_MAX_BODY_BYTES, fixed there by #8888). `maxBodyBytes` overrides keep these tests +// fast/deterministic without allocating megabyte-sized strings. +test("routeOrbWebhook: a Content-Length exceeding maxBodyBytes is rejected (413) before ever reading the body", async () => { + const registry = createFakeTenantRegistry(); + let lookups = 0; + const spiedRegistry: TenantRegistry = { ...registry, getByOrbInstallationId: (id) => { lookups += 1; return registry.getByOrbInstallationId(id); } }; + const request = new Request("https://control-plane.example/v1/orb/webhook", { + method: "POST", + headers: { "content-length": "1000", "x-hub-signature-256": githubSignature("ignored") }, + body: "ignored", + }); + + const response = await routeOrbWebhook(baseConfig({ registry: spiedRegistry, maxBodyBytes: 10 }), request); + + assert.equal(response.status, 413); + assert.deepEqual(await response.json(), { error: "payload_too_large", maxBytes: 10 }); + assert.equal(lookups, 0); +}); + +test("routeOrbWebhook: a body exceeding maxBodyBytes with NO Content-Length header is still rejected (413) via the streaming limit", async () => { + const registry = createFakeTenantRegistry(); + const request = new Request("https://control-plane.example/v1/orb/webhook", { + method: "POST", + headers: { "x-hub-signature-256": githubSignature("ignored") }, + body: streamBody("a".repeat(20)), + duplex: "half", + } as RequestInit); + + const response = await routeOrbWebhook(baseConfig({ registry, maxBodyBytes: 10 }), request); + + assert.equal(response.status, 413); + assert.deepEqual(await response.json(), { error: "payload_too_large", maxBytes: 10 }); +}); + +test("routeOrbWebhook: a non-numeric Content-Length header is ignored by the precheck, falling through to the streaming limit", async () => { + const registry = createFakeTenantRegistry(); + const request = new Request("https://control-plane.example/v1/orb/webhook", { + method: "POST", + headers: { "content-length": "not-a-number", "x-hub-signature-256": githubSignature("ignored") }, + body: "ignored", + }); + + const response = await routeOrbWebhook(baseConfig({ registry, maxBodyBytes: 10_000 }), request); + + // Not rejected by the (skipped) precheck -- the correctly-signed body verifies fine and proceeds all the way + // to the (non-JSON) payload check, proving the malformed header didn't crash or silently reject anything. + assert.equal(response.status, 400); + assert.deepEqual(await response.json(), { error: "invalid_json" }); +}); + +test("routeOrbWebhook: a Content-Length of '0' parses to null (not a positive int), falling through to the streaming limit", async () => { + const registry = createFakeTenantRegistry(); + const request = new Request("https://control-plane.example/v1/orb/webhook", { + method: "POST", + headers: { "content-length": "0", "x-hub-signature-256": githubSignature("ignored") }, + body: "ignored", + }); + + const response = await routeOrbWebhook(baseConfig({ registry, maxBodyBytes: 10_000 }), request); + + assert.equal(response.status, 400); + assert.deepEqual(await response.json(), { error: "invalid_json" }); +}); + +test("routeOrbWebhook: an absent request body reads as an empty string, not null (no stream to read)", async () => { + const registry = createFakeTenantRegistry(); + const request = new Request("https://control-plane.example/v1/orb/webhook", { method: "POST" }); + + const response = await routeOrbWebhook(baseConfig({ registry, maxBodyBytes: 10_000 }), request); + + // Empty body verifies against an empty signature only if the secret matches an empty string's HMAC -- here + // it won't, so this proves readBodyWithLimit's `!stream` branch returns "" rather than throwing. + assert.equal(response.status, 401); +}); + +test("routeOrbWebhook: skips undefined stream chunks while reading the body", async () => { + const registry = createFakeTenantRegistry(); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(undefined as unknown as Uint8Array); + controller.close(); + }, + }); + const request = new Request("https://control-plane.example/v1/orb/webhook", { + method: "POST", + headers: { "x-hub-signature-256": "sha256=bad" }, + body: stream, + duplex: "half", + } as RequestInit); + + const response = await routeOrbWebhook(baseConfig({ registry, maxBodyBytes: 10_000 }), request); + + assert.equal(response.status, 401); // empty (undefined-skipped) body + bad sig +}); + +test("routeOrbWebhook: a body within maxBodyBytes is processed normally", async () => { + const registry = createFakeTenantRegistry(); + await registry.upsert({ tenant: { name: "acme" }, product: "orb", state: "active", createdAt: "t0", updatedAt: "t0", orbInstallationId: 42 }); + const containerResponse = Response.json({ ok: true }, { status: 202 }); + const binding = fakeNamespace({ "orb:acme": fakeStub(containerResponse) }); + const request = webhookRequest({ installation: { id: 42 } }); + + const response = await routeOrbWebhook(baseConfig({ registry, binding, maxBodyBytes: 10_000 }), request); + + assert.equal(response.status, 202); +}); + test("routeOrbWebhook: a missing x-hub-signature-256 header is rejected (401), no registry lookup happens", async () => { const registry = createFakeTenantRegistry(); let lookups = 0; diff --git a/control-plane/test/secret-driver.test.ts b/control-plane/test/secret-driver.test.ts index 4a5afbed95..eb03b6ed08 100644 --- a/control-plane/test/secret-driver.test.ts +++ b/control-plane/test/secret-driver.test.ts @@ -31,7 +31,7 @@ function config(fetchImpl: typeof fetch): SecretDriverConfig { return { baseUrl: "https://api.loopover.test", internalJobToken: "internal-test-token", fetchImpl }; } -test("injectTenantSecrets: stores the WHOLE database connection details JSON-encoded, returns enrollId as secretRef and the one-time secret as bootstrapSecret", async () => { +test("injectTenantSecrets: stores the database connection details JSON-encoded, returns enrollId as secretRef and the one-time secret as bootstrapSecret", async () => { const { fetchImpl, calls } = fakeFetch(() => Response.json({ enrollId: "orbenr_abc", secret: "orbsec_xyz" }, { status: 200 })); const result = await injectTenantSecrets(config(fetchImpl), REQUEST); @@ -43,7 +43,52 @@ test("injectTenantSecrets: stores the WHOLE database connection details JSON-enc assert.equal((calls[0]!.init.headers as Record).authorization, "Bearer internal-test-token"); const body = JSON.parse(calls[0]!.init.body as string) as { secretType: string; secretValue: string }; assert.equal(body.secretType, "tenant_db_credential"); - assert.deepEqual(JSON.parse(body.secretValue), DATABASE); + const bundled = JSON.parse(body.secretValue) as { database: DatabaseConnectionDetails; orbWebhookSecret?: string }; + assert.deepEqual(bundled.database, DATABASE); +}); + +// #9143 (defect 5, deploy blocker): an ORB tenant's bundled enrollment ALSO carries a freshly generated +// webhook secret -- container-driver.ts's createTenantContainer never had a way to inject +// ORB_GITHUB_WEBHOOK_SECRET into a hosted tenant's container, so every correctly-signed GitHub delivery would +// 401 forever. Bundled into the SAME enrollment/bootstrapSecret as the database credential (see +// injectTenantSecrets' own doc comment for why), rather than a second one. +test("injectTenantSecrets: an ORB tenant's bundled payload also carries a freshly generated orbWebhookSecret (#9143)", async () => { + const { fetchImpl, calls } = fakeFetch(() => Response.json({ enrollId: "orbenr_abc", secret: "orbsec_xyz" })); + + await injectTenantSecrets(config(fetchImpl), REQUEST); + + const body = JSON.parse(calls[0]!.init.body as string) as { secretValue: string }; + const bundled = JSON.parse(body.secretValue) as { database: DatabaseConnectionDetails; orbWebhookSecret?: string }; + assert.equal(typeof bundled.orbWebhookSecret, "string"); + assert.match(bundled.orbWebhookSecret!, /^[0-9a-f]{64}$/); +}); + +test("injectTenantSecrets: two ORB tenants get two DIFFERENT randomly generated webhook secrets, never a shared/hardcoded value (#9143)", async () => { + const { fetchImpl: fetchA, calls: callsA } = fakeFetch(() => Response.json({ enrollId: "orbenr_a", secret: "orbsec_a" })); + const { fetchImpl: fetchB, calls: callsB } = fakeFetch(() => Response.json({ enrollId: "orbenr_b", secret: "orbsec_b" })); + + await injectTenantSecrets(config(fetchA), { tenant: { name: "acme" }, product: "orb", database: DATABASE }); + await injectTenantSecrets(config(fetchB), { tenant: { name: "beta" }, product: "orb", database: DATABASE }); + + const secretFor = (calls: typeof callsA) => { + const body = JSON.parse(calls[0]!.init.body as string) as { secretValue: string }; + return (JSON.parse(body.secretValue) as { orbWebhookSecret?: string }).orbWebhookSecret; + }; + const secretA = secretFor(callsA); + const secretB = secretFor(callsB); + assert.ok(secretA); + assert.ok(secretB); + assert.notEqual(secretA, secretB); +}); + +test("injectTenantSecrets: an AMS tenant's bundled payload never carries an orbWebhookSecret (its image never verifies a webhook)", async () => { + const { fetchImpl, calls } = fakeFetch(() => Response.json({ enrollId: "orbenr_abc", secret: "orbsec_xyz" })); + + await injectTenantSecrets(config(fetchImpl), { tenant: { name: "acme" }, product: "ams", database: DATABASE }); + + const body = JSON.parse(calls[0]!.init.body as string) as { secretValue: string }; + const bundled = JSON.parse(body.secretValue) as Record; + assert.equal("orbWebhookSecret" in bundled, false); }); test("injectTenantSecrets: throws when the request has no database connection details attached", async () => { diff --git a/control-plane/test/tenant-registry.test.ts b/control-plane/test/tenant-registry.test.ts index 6b9ad018fb..c8b8cb8bfc 100644 --- a/control-plane/test/tenant-registry.test.ts +++ b/control-plane/test/tenant-registry.test.ts @@ -221,3 +221,37 @@ test("createKvTenantRegistry: unlinking a tenant's installation ID (upsert witho assert.equal(kv.store.get("installation:555"), undefined); assert.equal(await registry.getByOrbInstallationId(555), undefined); }); + +// #9143 (defect 2, deploy blocker): load-bearing isolation regression test. Before this fix, `upsert` deleted +// the OLD installation-index entry unconditionally whenever a tenant's own orbInstallationId changed/cleared +// -- WITHOUT checking whether that index entry still actually pointed at THIS tenant. http-app.ts's +// isRecreatableState lets a "failed"/"torn down" tenant's installation claim be taken over by a brand-new +// tenant of a different name; a stale record (still carrying the OLD installationId from its own creation) +// being re-created or torn down LONG after the ID was reclaimed by someone else would then silently delete +// the CURRENT claimant's live routing pointer, even though the current claimant's own installationId never +// changed. That tenant stays "active" but permanently unreachable by webhook forever (create 409s since it +// already exists; orbInstallationId is otherwise settable only at create) -- until this test's own scenario +// no longer reproduces it. +test("createKvTenantRegistry: an unrelated tenant's installation-index entry SURVIVES a stale record's later re-creation/teardown upsert (#9143)", async () => { + const kv = fakeKv(); + const registry = createKvTenantRegistry(kv); + + // Tenant "old" was originally created claiming installation 555, but its provisioning failed -- its own + // record still carries orbInstallationId: 555 from that original creation. + await registry.upsert({ ...recordFor("old", "orb", "failed"), orbInstallationId: 555 }); + + // A brand-new tenant "newcomer" legitimately re-claims installation 555 (http-app.ts's own + // isRecreatableState lets a "failed" tenant's claim be taken over) -- the index now points at newcomer. + await registry.upsert({ ...recordFor("newcomer", "orb", "active"), orbInstallationId: 555 }); + assert.equal(kv.store.get("installation:555"), "tenant:orb:newcomer"); + assert.equal((await registry.getByOrbInstallationId(555))?.tenant.name, "newcomer"); + + // "old" is now (much later) re-created or torn down -- its own upsert no longer carries orbInstallationId + // at all. Before #9143 this unconditionally deleted "installation:555", which by now points at "newcomer", + // NOT "old" -- destroying newcomer's live webhook-routing pointer even though newcomer's own + // installationId never changed. + await registry.upsert(recordFor("old", "orb", "torn down")); + + assert.equal(kv.store.get("installation:555"), "tenant:orb:newcomer"); + assert.equal((await registry.getByOrbInstallationId(555))?.tenant.name, "newcomer"); +}); diff --git a/src/env.d.ts b/src/env.d.ts index 93646d968b..ea6ee7ef44 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -166,8 +166,18 @@ declare global { GITHUB_WEBHOOK_SECRET?: string; GITHUB_WEBHOOK_MAX_BODY_BYTES?: string; /** Webhook secret for the central LoopOver Orb GitHub App (#1255) — distinct from the review app's - * GITHUB_WEBHOOK_SECRET. Verifies inbound POST /v1/orb/webhook deliveries. Inject as a wrangler secret. */ + * GITHUB_WEBHOOK_SECRET. Verifies inbound POST /v1/orb/webhook deliveries. Inject as a wrangler secret. + * Manual self-host/cloud only — a hosted control-plane tenant container (see LOOPOVER_TENANT_SECRET_TOKEN + * below) never has this set directly; it resolves its own value via the broker instead. */ ORB_GITHUB_WEBHOOK_SECRET?: string; + /** #9143 (defect 5, #8202): the one-time bootstrap credential a HOSTED control-plane tenant container + * reads at cold boot (`control-plane/src/container-driver.ts`'s `TENANT_SECRET_ENV_VAR`) to exchange, via + * `fetchBrokeredStoredSecret` (src/orb/broker-client.ts) against `POST /v1/orb/token`, for whatever the + * broker has custodied under it — for an ORB tenant, a bundle containing its own per-tenant webhook + * secret (see `src/orb/hosted-webhook-secret.ts`). Manual self-host/cloud never set this — only a + * container control-plane itself starts does (`createTenantContainer`). Cloud never sets it ⇒ this whole + * path is inert there, same as ORB_ENROLLMENT_SECRET's own "cloud never sets it" convention above. */ + LOOPOVER_TENANT_SECRET_TOKEN?: string; /** Cloudflare Worker error tracking (PostHog, #8288). REPLACES the old @sentry/hono/cloudflare middleware * entirely (2026-07-25 epic #8286 correction). Deliberately NAMED DIFFERENTLY from the dual-purpose * POSTHOG_API_KEY/POSTHOG_HOST (MCP telemetry #6228/#6235 + self-host error tracking #8287): self-host's diff --git a/src/orb/hosted-webhook-secret.ts b/src/orb/hosted-webhook-secret.ts new file mode 100644 index 0000000000..a669cf41a6 --- /dev/null +++ b/src/orb/hosted-webhook-secret.ts @@ -0,0 +1,91 @@ +// Resolves a HOSTED control-plane ORB tenant container's OWN GitHub webhook secret (#9143, defect 5 -- part +// of #8202's originally-scoped-but-never-wired brokered secret bootstrap). +// +// A normal self-host or cloud deployment sets ORB_GITHUB_WEBHOOK_SECRET directly (a wrangler secret, see +// env.d.ts) -- this is a pure passthrough for that case, with ZERO network calls, so behavior there is +// byte-identical to before this file existed. +// +// A tenant container control-plane itself starts (control-plane/src/container-driver.ts's +// createTenantContainer) has no such secret injected directly: instead it gets a one-time +// LOOPOVER_TENANT_SECRET_TOKEN (#8202) it can exchange, via fetchBrokeredStoredSecret (./broker-client.ts), +// for whatever the broker has custodied under that token's enrollment. control-plane's own secret-driver.ts +// bundles this tenant's own webhook secret into that SAME exchange (see its own header comment for why one +// bootstrap token, not two) -- the bundled JSON shape is a private wire contract between that file and this +// one, agreed by convention (this package has no dependency on control-plane/, a separate npm workspace). +// +// Before #9143, ORB_GITHUB_WEBHOOK_SECRET was simply never set on a hosted tenant container at all -- +// orb-webhook-router.ts's own header comment claims the container "runs the SAME self-host webhook-handling +// code unmodified and re-verifies independently", but handleOrbWebhook (./webhook.ts) fails closed on a +// missing secret, so every correctly-signed delivery would 401 forever and GitHub would eventually disable +// the hook. fetchBrokeredStoredSecret (./broker-client.ts) already had full test coverage for exactly this +// exchange but had ZERO production call sites -- this file is that call site. +import { fetchBrokeredStoredSecret } from "./broker-client"; + +type HostedWebhookSecretEnv = { + ORB_GITHUB_WEBHOOK_SECRET?: string | undefined; + LOOPOVER_TENANT_SECRET_TOKEN?: string | undefined; + ORB_BROKER_URL?: string | undefined; +}; + +/** The bundled payload control-plane's secret-driver.ts stores (JSON-encoded) under the tenant's ONE shared + * enrollment -- `database` is present too (the original #8064/#8066 payload), but this consumer only ever + * needs `orbWebhookSecret`. */ +type BundledTenantSecret = { orbWebhookSecret?: string }; + +// Memoized in-module: handleOrbWebhook calls resolveOrbWebhookSecret on EVERY delivery, but a hosted tenant +// container is a single long-lived process for its whole container lifetime (worker.ts's OrbTenantContainer, +// sleepAfter "10m") -- the broker exchange only ever needs to happen once, not on every single webhook. A +// SUCCESSFUL resolution is cached forever (the value never changes once minted); a FAILED one is deliberately +// NOT cached, so a transient broker outage self-heals on the very next delivery instead of wedging this +// container into a permanent 401 state until it restarts. +let cachedSecret: string | undefined; +let inFlight: Promise | undefined; + +/** Test-only: clears both the cached value and any in-flight exchange, so each test starts from a clean + * slate. A real container process never needs this -- the cache is meant to outlive the whole container + * lifetime. */ +export function resetHostedWebhookSecretCacheForTests(): void { + cachedSecret = undefined; + inFlight = undefined; +} + +async function fetchHostedWebhookSecret(env: HostedWebhookSecretEnv, fetchImpl: typeof fetch): Promise { + const stored = await fetchBrokeredStoredSecret(env, fetchImpl); + let bundled: BundledTenantSecret; + try { + bundled = JSON.parse(stored.secretValue) as BundledTenantSecret; + } catch { + throw new Error("hosted webhook secret bundle was not valid JSON"); + } + return typeof bundled.orbWebhookSecret === "string" && bundled.orbWebhookSecret ? bundled.orbWebhookSecret : undefined; +} + +/** The tenant container's own webhook secret: the direct env value if set (cloud/normal self-host, no network + * call at all), else a brokered exchange for a hosted tenant container (#8202/#9143), else `undefined` -- + * matching handleOrbWebhook's own existing fail-closed convention (an undefined/empty secret means every + * signature check fails, answering 401), never a silent bypass. Concurrent callers (several webhook + * deliveries racing in before the first exchange resolves) share the SAME in-flight exchange rather than + * each triggering their own broker call. */ +export async function resolveOrbWebhookSecret(env: HostedWebhookSecretEnv, fetchImpl: typeof fetch = fetch): Promise { + if (env.ORB_GITHUB_WEBHOOK_SECRET) return env.ORB_GITHUB_WEBHOOK_SECRET; + if (cachedSecret !== undefined) return cachedSecret; + if (!env.LOOPOVER_TENANT_SECRET_TOKEN) return undefined; + + if (!inFlight) { + inFlight = fetchHostedWebhookSecret(env, fetchImpl) + .then((secret) => { + if (secret) cachedSecret = secret; + return secret; + }) + .catch((error: unknown) => { + console.error( + JSON.stringify({ level: "error", event: "orb_hosted_webhook_secret_resolve_failed", message: error instanceof Error ? error.message : String(error) }), + ); + return undefined; + }) + .finally(() => { + inFlight = undefined; + }); + } + return inFlight; +} diff --git a/src/orb/webhook.ts b/src/orb/webhook.ts index 2333ed9364..b6ce4dfad7 100644 --- a/src/orb/webhook.ts +++ b/src/orb/webhook.ts @@ -11,6 +11,7 @@ import type { Context } from "hono"; import type { GitHubWebhookPayload } from "../types"; import { sha256Hex, verifyGitHubSignature } from "../utils/crypto"; import { parsePositiveInt } from "../utils/json"; +import { resolveOrbWebhookSecret } from "./hosted-webhook-secret"; import { upsertOrbInstallation } from "./installations"; import { recordOrbPrOutcome } from "./outcomes"; import { forwardOrbEvent, persistRelayForwardOutcome } from "./relay"; @@ -35,9 +36,13 @@ export async function handleOrbWebhook(c: Context<{ Bindings: Env }>): Promise { expect((await post(noSecret, INSTALL)).status).toBe(401); }); + describe("a HOSTED control-plane tenant container (#9143, defect 5)", () => { + afterEach(() => { + resetHostedWebhookSecretCacheForTests(); + vi.unstubAllGlobals(); + }); + + it("resolves its own webhook secret via the broker (LOOPOVER_TENANT_SECRET_TOKEN, no ORB_GITHUB_WEBHOOK_SECRET) and verifies a real delivery end to end", async () => { + resetHostedWebhookSecretCacheForTests(); + const brokeredSecret = "whsec_from_broker"; + vi.stubGlobal( + "fetch", + vi.fn(async () => Response.json({ secretValue: JSON.stringify({ database: { host: "h" }, orbWebhookSecret: brokeredSecret }) })), + ); + const e = createTestEnv({ LOOPOVER_TENANT_SECRET_TOKEN: "orbsec_tenant" }); // ORB_GITHUB_WEBHOOK_SECRET unset + + const res = await post(e, INSTALL, { delivery: "hosted-1", sig: await sign(INSTALL, brokeredSecret) }); + + expect(res.status).toBe(202); + expect(await row(e, "hosted-1")).toMatchObject({ status: "received" }); + }); + + it("still 401s a delivery signed with the WRONG secret, even when a bootstrap token is configured", async () => { + resetHostedWebhookSecretCacheForTests(); + vi.stubGlobal( + "fetch", + vi.fn(async () => Response.json({ secretValue: JSON.stringify({ orbWebhookSecret: "whsec_real" }) })), + ); + const e = createTestEnv({ LOOPOVER_TENANT_SECRET_TOKEN: "orbsec_tenant" }); + + const res = await post(e, INSTALL, { sig: await sign(INSTALL, "whsec_wrong") }); + + expect(res.status).toBe(401); + }); + }); + it("401 when the signature header is absent entirely", async () => { expect((await post(env(), INSTALL, { headers: { "x-hub-signature-256": null } })).status).toBe(401); }); diff --git a/test/unit/orb-hosted-webhook-secret.test.ts b/test/unit/orb-hosted-webhook-secret.test.ts new file mode 100644 index 0000000000..66095da9f7 --- /dev/null +++ b/test/unit/orb-hosted-webhook-secret.test.ts @@ -0,0 +1,153 @@ +// #9143 (defect 5, deploy blocker): resolveOrbWebhookSecret is the fetchBrokeredStoredSecret CALL SITE that +// was missing entirely -- fetchBrokeredStoredSecret (src/orb/broker-client.ts, #8202) had full test coverage +// but zero production callers, so a hosted control-plane tenant container never had any way to obtain its own +// ORB_GITHUB_WEBHOOK_SECRET and would 401 every correctly-signed GitHub delivery forever. +import { beforeEach, describe, expect, it } from "vitest"; +import { resetHostedWebhookSecretCacheForTests, resolveOrbWebhookSecret } from "../../src/orb/hosted-webhook-secret"; + +function captureFetch(handler: (url: string) => Response | Promise): { fetchImpl: typeof fetch; calls: string[] } { + const calls: string[] = []; + const fetchImpl = (async (url: RequestInfo | URL) => { + calls.push(String(url)); + return handler(String(url)); + }) as typeof fetch; + return { fetchImpl, calls }; +} + +beforeEach(() => { + resetHostedWebhookSecretCacheForTests(); +}); + +describe("resolveOrbWebhookSecret", () => { + it("returns a direct ORB_GITHUB_WEBHOOK_SECRET env value with ZERO network calls (cloud/manual self-host)", async () => { + const { fetchImpl, calls } = captureFetch(() => { + throw new Error("fetch should never be called when the direct env value is already set"); + }); + + const secret = await resolveOrbWebhookSecret({ ORB_GITHUB_WEBHOOK_SECRET: "whsec_direct" }, fetchImpl); + + expect(secret).toBe("whsec_direct"); + expect(calls).toEqual([]); + }); + + it("returns undefined with no network call when neither the direct secret nor a bootstrap token is set (unconfigured deployment, fail-closed)", async () => { + const { fetchImpl, calls } = captureFetch(() => { + throw new Error("fetch should never be called with nothing to bootstrap from"); + }); + + const secret = await resolveOrbWebhookSecret({}, fetchImpl); + + expect(secret).toBeUndefined(); + expect(calls).toEqual([]); + }); + + it("exchanges LOOPOVER_TENANT_SECRET_TOKEN for the bundled webhook secret via the broker (#9143)", async () => { + const { fetchImpl, calls } = captureFetch(() => + Response.json({ secretValue: JSON.stringify({ database: { host: "h" }, orbWebhookSecret: "whsec_brokered" }), secretType: "tenant_db_credential" }), + ); + + const secret = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "orbsec_tenant" }, fetchImpl); + + expect(secret).toBe("whsec_brokered"); + expect(calls).toEqual(["https://api.loopover.ai/v1/orb/token"]); + }); + + it("caches a successful resolution -- a second call never hits the broker again", async () => { + const { fetchImpl, calls } = captureFetch(() => + Response.json({ secretValue: JSON.stringify({ database: {}, orbWebhookSecret: "whsec_cached" }) }), + ); + + const first = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + const second = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + + expect(first).toBe("whsec_cached"); + expect(second).toBe("whsec_cached"); + expect(calls.length).toBe(1); + }); + + it("de-duplicates concurrent callers into a SINGLE in-flight broker exchange", async () => { + let resolveResponse!: (value: Response) => void; + const pendingResponse = new Promise((resolve) => { + resolveResponse = resolve; + }); + const { fetchImpl, calls } = captureFetch(() => pendingResponse); + + const first = resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + const second = resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + resolveResponse(Response.json({ secretValue: JSON.stringify({ database: {}, orbWebhookSecret: "whsec_concurrent" }) })); + + expect(await first).toBe("whsec_concurrent"); + expect(await second).toBe("whsec_concurrent"); + expect(calls.length).toBe(1); + }); + + it("does NOT cache a failed exchange -- the very next call retries the broker instead of staying wedged (#9143)", async () => { + let attempt = 0; + const { fetchImpl } = captureFetch(() => { + attempt += 1; + return attempt === 1 + ? new Response("broker down", { status: 503 }) + : Response.json({ secretValue: JSON.stringify({ database: {}, orbWebhookSecret: "whsec_recovered" }) }); + }); + + const first = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + const second = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + + expect(first).toBeUndefined(); // fails closed, matching handleOrbWebhook's own convention -- never throws + expect(second).toBe("whsec_recovered"); + expect(attempt).toBe(2); + }); + + it("resolves to undefined (fails closed) when the bundled payload is not valid JSON", async () => { + const { fetchImpl } = captureFetch(() => Response.json({ secretValue: "not json at all" })); + + const secret = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + + expect(secret).toBeUndefined(); + }); + + it("resolves to undefined (fails closed) when the bundled payload has no orbWebhookSecret (e.g. an AMS tenant's own bundle)", async () => { + const { fetchImpl } = captureFetch(() => Response.json({ secretValue: JSON.stringify({ database: { host: "h" } }) })); + + const secret = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + + expect(secret).toBeUndefined(); + }); + + it("resolves to undefined (fails closed) when the bundled orbWebhookSecret is present but not a string", async () => { + const { fetchImpl } = captureFetch(() => Response.json({ secretValue: JSON.stringify({ orbWebhookSecret: 12345 }) })); + + const secret = await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + + expect(secret).toBeUndefined(); + }); + + it("resolves to undefined (fails closed), never throws, when the broker itself is unreachable", async () => { + const fetchImpl = (async () => { + throw new Error("network down"); + }) as typeof fetch; + + await expect(resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl)).resolves.toBeUndefined(); + }); + + it("resolves to undefined (fails closed), never throws, when a non-Error value is thrown (defensive ?? branch)", async () => { + const fetchImpl = (async () => { + // Deliberately a non-Error throw, to exercise resolveOrbWebhookSecret's `error instanceof Error ? ... : + // String(error)` fallback branch. + throw "not an Error instance"; + }) as typeof fetch; + + await expect(resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl)).resolves.toBeUndefined(); + }); + + it("resetHostedWebhookSecretCacheForTests clears a cached value so the NEXT call re-exchanges (test hygiene)", async () => { + const { fetchImpl, calls } = captureFetch(() => Response.json({ secretValue: JSON.stringify({ orbWebhookSecret: "whsec_a" }) })); + await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + expect(calls.length).toBe(1); + + resetHostedWebhookSecretCacheForTests(); + + await resolveOrbWebhookSecret({ LOOPOVER_TENANT_SECRET_TOKEN: "t" }, fetchImpl); + expect(calls.length).toBe(2); + }); +});