From f7aa64e9799495fa5833c52778f5f878bf58e5fe Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Sun, 26 Jul 2026 13:20:38 +0000 Subject: [PATCH] fix(orb): degrade a thrown OAuth self-enrollment network error to the clean identity page handleOrbEnrollment's three GitHub network calls -- exchangeOrbOAuthCode, fetchOrbOAuthUser, and verifyInstallationAdmin -- handled every non-OK or malformed response cleanly but none was wrapped in try/catch. A thrown network error (DNS failure, or a timeout past timeoutFetch's own retry budget) escaped handleOrbOAuthCallback as an uncaught framework 500 instead of the module's own "Couldn't verify your GitHub identity" landing page. Wrap each of the three calls so a rejected fetch degrades to that clean 400 page, mirroring the failure-doesn't-escape convention already used in webhook.ts/relay.ts/ingest.ts. Cover each call's new catch path with an integration test that rejects the corresponding fetch. Closes #8881 --- src/orb/oauth.ts | 27 ++++++++++++++-- test/integration/orb-oauth.test.ts | 52 ++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/orb/oauth.ts b/src/orb/oauth.ts index d5d3216ed2..88d23bdfae 100644 --- a/src/orb/oauth.ts +++ b/src/orb/oauth.ts @@ -68,9 +68,25 @@ export async function verifyInstallationAdmin( } async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, installationId: number): Promise { - const token = await exchangeOrbOAuthCode(c.env, code); + // A thrown network error (DNS failure, or a timeout past timeoutFetch's own retry budget) from any of the three + // GitHub calls below degrades to the same clean landing page a bad HTTP *response* already produces, instead of + // escaping handleOrbOAuthCallback as an uncaught framework 500. Mirrors the failure-doesn't-escape convention in + // webhook.ts/relay.ts/ingest.ts. The calls stay separate (not one body-wide wrapper) so a throw is never confused + // with a DB/broker fault after identity is established. + const identityError = () => c.html(landingPage(c.env, "Couldn't verify your GitHub identity", "We couldn't reach GitHub to verify your identity — try the install again."), 400); + let token: string | null; + try { + token = await exchangeOrbOAuthCode(c.env, code); + } catch { + return identityError(); + } if (!token) return c.html(landingPage(c.env, "Couldn't verify your GitHub identity", "The authorization didn't complete — re-run the install from GitHub and try again."), 400); - const user = await fetchOrbOAuthUser(token); + let user: GitHubUser | null; + try { + user = await fetchOrbOAuthUser(token); + } catch { + return identityError(); + } if (!user) return c.html(landingPage(c.env, "Couldn't verify your GitHub identity", "We couldn't read your GitHub account — try the install again."), 400); const install = await c.env.DB.prepare("SELECT account_login, account_type, account_id, registered, self_enrollment_disabled, suspended_at, removed_at FROM orb_github_installations WHERE installation_id = ?") .bind(installationId) @@ -79,7 +95,12 @@ async function handleOrbEnrollment(c: Context<{ Bindings: Env }>, code: string, // The admin-of-installation check is the authorization gate — it runs BEFORE we reveal or change any state, so a // non-admin learns nothing about the install and can never enroll someone else's. It binds to the immutable // GitHub account id (logins can be renamed/reused), so a stale account_login can never grant access. - const isAdmin = await verifyInstallationAdmin(token, user.login, user.id, install.account_login, install.account_type, install.account_id); + let isAdmin: boolean; + try { + isAdmin = await verifyInstallationAdmin(token, user.login, user.id, install.account_login, install.account_type, install.account_id); + } catch { + return identityError(); + } if (!isAdmin) return c.html(landingPage(c.env, "Admin access required", "You must be an admin of this installation's account to enroll it for self-host."), 403); if (install.removed_at !== null || install.suspended_at !== null) return c.html(landingPage(c.env, "Installation not active", "This installation is suspended or uninstalled — re-install the Orb App, then retry."), 403); if (install.self_enrollment_disabled === 1) return c.html(landingPage(c.env, "Installation disabled", "This installation was disabled by the operator — contact the operator to re-enable self-host enrollment."), 403); diff --git a/test/integration/orb-oauth.test.ts b/test/integration/orb-oauth.test.ts index 305f57face..b620687110 100644 --- a/test/integration/orb-oauth.test.ts +++ b/test/integration/orb-oauth.test.ts @@ -222,3 +222,55 @@ describe("maintainer self-enrollment via the OAuth callback", () => { expect(await (await app.request("/v1/orb/oauth/callback?code=abc&installation_id=0", {}, brokeredEnv())).text()).toContain("LoopOver Orb connected"); // installationId > 0 false }); }); + +describe("self-enrollment degrades a thrown GitHub network error to the clean identity page (not an uncaught 500)", () => { + const app = createApp(); + const db = (e: Env) => e.DB as unknown as TestD1Database; + const brokeredEnv = () => createTestEnv({ ORB_BROKER_ENABLED: "true", ORB_GITHUB_CLIENT_ID: "id", ORB_GITHUB_CLIENT_SECRET: "sec" }); + const seedInstall = (e: Env, cols: Record) => { + const keys = Object.keys(cols); + return db(e).prepare(`INSERT INTO orb_github_installations (${keys.join(", ")}) VALUES (${keys.map(() => "?").join(", ")})`).bind(...keys.map((k) => cols[k] as string | number)).run(); + }; + // Each of the three network calls resolves through the module's default timeoutFetch, so spying it lets us reject + // exactly one URL while the earlier calls succeed — reaching the specific call under test before it throws. + const rejectAt = (throwUrlFragment: string) => + vi.spyOn(githubClientModule, "timeoutFetch").mockImplementation((async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes(throwUrlFragment)) throw new Error("network down"); + if (url.includes("/login/oauth/access_token")) return Response.json({ access_token: "ghu_x" }); + if (url.includes("api.github.com/user/memberships/orgs/")) return Response.json({ role: "admin", state: "active", organization: { id: 20 } }); + if (url.endsWith("api.github.com/user")) return Response.json({ login: "alice", id: 7 }); + return new Response("nf", { status: 404 }); + }) as typeof githubClientModule.timeoutFetch); + afterEach(() => vi.restoreAllMocks()); + + it("a thrown code-exchange fetch degrades to the identity page (400), not an uncaught exception", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 600, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + rejectAt("/login/oauth/access_token"); + const res = await app.request("/v1/orb/oauth/callback?code=abc&installation_id=600", {}, e); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Couldn't verify your GitHub identity"); + }); + + it("a thrown /user read degrades to the identity page (400)", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 601, account_login: "acme", account_type: "Organization", account_id: 20, registered: 1 }); + rejectAt("api.github.com/user"); // exchange succeeds first, then the /user read throws + const res = await app.request("/v1/orb/oauth/callback?code=abc&installation_id=601", {}, e); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Couldn't verify your GitHub identity"); + expect(await db(e).prepare("SELECT 1 AS x FROM orb_enrollments WHERE installation_id=601").first()).toBeUndefined(); // never reached enrollment + }); + + it("a thrown org-membership check degrades to the identity page (400), leaving the install unregistered", async () => { + const e = brokeredEnv(); + await seedInstall(e, { installation_id: 602, account_login: "acme", account_type: "Organization", account_id: 20, registered: 0 }); + rejectAt("/user/memberships/orgs/"); // exchange + /user succeed, then the admin check throws + const res = await app.request("/v1/orb/oauth/callback?code=abc&installation_id=602", {}, e); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Couldn't verify your GitHub identity"); + const row = await db(e).prepare("SELECT registered FROM orb_github_installations WHERE installation_id=602").first<{ registered: number }>(); + expect(row?.registered).toBe(0); // a throw at the gate never auto-registers + }); +});