From e56c66c28f8638f320e7c11ce3d20760dff32535 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:22:40 -0700 Subject: [PATCH] fix(control-plane): close tenant-isolation, webhook-secret, and overlap bugs before first deploy (#9143) control-plane/ has never been deployed (no deploy workflow, placeholder wrangler.jsonc values) so every defect here is latent, but a first deploy would ship all eight intact. Deploy blockers: - neon-database-driver.ts: branchNameFor now always appends a hash suffix derived from the RAW pre-sanitization tenant identity (not the already case-folded/punctuation-collapsed string #8026's suffix hashed), so tenant names that sanitize identically ("Acme"/"acme", "acme.corp"/"acme-corp") can no longer share one Neon branch/database/role/password. - tenant-registry.ts: upsert's installation-ID secondary-index cleanup is now a compare-and-delete (only clears the index if it still points at the upserting tenant's own primary key), so a stale tenant's later re-creation/teardown can no longer delete a different, currently-active tenant's live webhook-routing pointer. Added PATCH /v1/tenants/:name/orb-installation as the manual recovery path for a pointer already lost to the pre-fix bug. - secret-driver.ts + src/orb/hosted-webhook-secret.ts (new): a hosted ORB tenant's container now gets its own webhook secret bundled into the same broker exchange as its database credential, and handleOrbWebhook resolves it via the brokered path when no direct env value is set -- wiring fetchBrokeredStoredSecret's previously-unused call site for real, so correctly-signed deliveries no longer 401 forever. Also addressed: - http-app.ts: POST /v1/tenants/rollout now returns an explicit 501 instead of silently no-opping on all four of its promised effects; DELETE refuses a tenant still in the "provisioning" state (409). - container-driver.ts + worker.ts: envVars are now persisted in the container DO's own durable storage and reloaded on construction, so a pinned version / bootstrap secret / central PostHog key survive an implicit restart instead of resetting to an empty environment. - orb-webhook-router.ts: the unauthenticated webhook route now enforces the same content-length precheck + streaming body-size limit its declared twin (src/orb/webhook.ts, #8888) already has. - ams-wake.ts: nextDueAt is claimed before a tenant's poll starts (not after it resolves), the per-tenant poll timeout is capped well under the 5-minute cron interval, and one tick now bounds how many due tenants it wakes -- closing the guaranteed overlapping-tick race. Deferred, documented in the PR: the full KV-eventual-consistency fix (a Durable Object/D1-backed serialized claim) is out of scope for this PR; the deploy workflow and real wrangler.jsonc placeholder values remain outstanding for whichever change first makes this workspace deployable. --- control-plane/src/ams-wake.ts | 53 +++- control-plane/src/container-driver.ts | 15 +- control-plane/src/http-app.ts | 123 +++---- control-plane/src/neon-database-driver.ts | 35 +- control-plane/src/orb-webhook-router.ts | 64 +++- control-plane/src/secret-driver.ts | 65 +++- control-plane/src/tenant-registry.ts | 17 +- control-plane/src/worker.ts | 39 ++- control-plane/test/ams-wake.test.ts | 86 +++++ control-plane/test/container-driver.test.ts | 24 +- control-plane/test/http-app.test.ts | 300 +++++++++++------- .../test/neon-database-driver.test.ts | 47 ++- control-plane/test/orb-webhook-router.test.ts | 119 +++++++ control-plane/test/secret-driver.test.ts | 49 ++- control-plane/test/tenant-registry.test.ts | 34 ++ src/env.d.ts | 12 +- src/orb/hosted-webhook-secret.ts | 91 ++++++ src/orb/webhook.ts | 11 +- test/integration/orb-webhook.test.ts | 38 ++- test/unit/orb-hosted-webhook-secret.test.ts | 153 +++++++++ 20 files changed, 1158 insertions(+), 217 deletions(-) create mode 100644 src/orb/hosted-webhook-secret.ts create mode 100644 test/unit/orb-hosted-webhook-secret.test.ts 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); + }); +});