Repository navigation
feat(github): support browser App installation onboarding - #1
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| }, | ||
| account.login, | ||
| ); | ||
| const callback = new URL(ghApp.setup_url ?? `${baseUrl}/settings/installations/${installationId}`); |
There was a problem hiding this comment.
🟡 Install POST 500s on invalid setup_url after install.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:169-173):
Problem: Install POST 500s on invalid setup_url after install
Detail: The redirect target is built with `new URL(ghApp.setup_url ?? ...)` without validation, and setup_url flows straight from seed config (`seedFromConfig` stores `a.setup_url ?? null`, so an empty string or relative path like "/setup" is kept verbatim). Any non-absolute value makes `new URL` throw a TypeError — but the throw happens AFTER the installation row is inserted and the `installation` webhook is dispatched, so the browser gets a 500 for a request that actually succeeded. Retrying the form then fails with 409 "Already installed", leaving onboarding unrecoverable until the config is fixed and the store reset. Repro: Given a seeded App with setup_url "" (or "/setup") and a signed-in session, When the user submits the install form with a valid nonce, Then `new URL("")` throws → 500, and the installation + webhook already happened; When the user retries, Then 409 Already installed.
Suggested fix: Validate setup_url at seed time (reject or coerce non-absolute values to null in seedFromConfig), or guard at the use site: `const fallback = `${baseUrl}/settings/installations/${installationId}`; const callback = new URL(URL.canParse(ghApp.setup_url ?? "") ? ghApp.setup_url! : fallback);` so the redirect always resolves.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The redirect target is built with new URL(ghApp.setup_url ?? ...) without validation, and setup_url flows straight from seed config (seedFromConfig stores a.setup_url ?? null, so an empty string or relative path like "/setup" is kept verbatim). Any non-absolute value makes new URL throw a TypeError — but the throw happens AFTER the installation row is inserted and the installation webhook is dispatched, so the browser gets a 500 for a request that actually succeeded. Retrying the form then fails with 409 "Already installed", leaving onboarding unrecoverable until the config is fixed and the store reset. Repro: Given a seeded App with setup_url "" (or "/setup") and a signed-in session, When the user submits the install form with a valid nonce, Then new URL("") throws → 500, and the installation + webhook already happened; When the user retries, Then 409 Already installed.
sender: formatUser(gh.users.get(user.id)!, baseUrl),
},
account.login,
);
const callback = new URL(ghApp.setup_url ?? `${baseUrl}/settings/installations/${installationId}`);
callback.searchParams.set("installation_id", String(installationId));
callback.searchParams.set("setup_action", "install");
callback.searchParams.set("state", pending.state);
return c.redirect(callback.toString(), 302);| ? body.repository_ids | ||
| : [body.repository_ids]; | ||
| const repoIds = [...new Set(rawIds.map((id) => (typeof id === "string" && /^\d+$/.test(id) ? Number(id) : NaN)))]; | ||
| const repositories = gh.repos |
There was a problem hiding this comment.
🟡 Account-repositories filter duplicated four times.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:127-129):
Problem: Account-repositories filter duplicated four times
Detail: The predicate `gh.repos.all().filter((repo) => repo.owner_id === X && repo.owner_type === Y)` is now hand-written four times: twice in this file (GET form at lines 66-68 and POST validation at 127-129), once in installation-repositories.ts:24-25, plus the pre-existing copy in routes/apps.ts:137-139. The account's repository set is one fact derived four times by hand; a future change (e.g. excluding archived repos) must be replicated in every copy or the picker, the submission validation, and the listing endpoints silently drift apart.
Suggested fix: Extract a `reposForAccount(gh, accountId, accountType): GitHubRepo[]` helper (e.g. in src/helpers.ts) and use it in both handlers of app-installation-flow.ts and in installation-repositories.ts, deriving the set once.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The predicate gh.repos.all().filter((repo) => repo.owner_id === X && repo.owner_type === Y) is now hand-written four times: twice in this file (GET form at lines 66-68 and POST validation at 127-129), once in installation-repositories.ts:24-25, plus the pre-existing copy in routes/apps.ts:137-139. The account's repository set is one fact derived four times by hand; a future change (e.g. excluding archived repos) must be replicated in every copy or the picker, the submission validation, and the listing endpoints silently drift apart.
const repoIds = [...new Set(rawIds.map((id) => (typeof id === "string" && /^\d+$/.test(id) ? Number(id) : NaN)))];
const repositories = gh.repos
.all()
.filter((repo) => repo.owner_id === account.id && repo.owner_type === account.type);
if (
(selection !== "all" && selection !== "selected") ||| } | ||
| const account = allowedAccounts.find((candidate) => candidate.login === selectedLogin); | ||
| if (!account) return c.html(error("Forbidden", "You cannot install an application on this account."), 403); | ||
| const nonce = randomBytes(24).toString("base64url"); |
There was a problem hiding this comment.
🟡 Unbounded pending-install nonces enable memory DoS.
| const nonce = randomBytes(24).toString("base64url"); | |
| Store one pending form per user/app/account (overwrite on re-issue instead of minting a new nonce per GET), or enforce a per-user cap on live pending forms before `forms().set(...)`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:64-65):
Problem: Unbounded pending-install nonces enable memory DoS
Detail: Every authenticated GET of the repository-selection form mints a fresh 24-byte nonce and inserts it into the shared in-memory `github.installation.pendingForms` map. The `_emu_session` cookie is set with `SameSite=Lax` (oauth.ts:183), so it is sent on cross-site top-level GET navigations — an attacker page (window.open/link loop) or a simple scripted loop can allocate unlimited live entries. Entries are purged only lazily and only after FORM_TTL_MS (10 min), so millions of entries can accumulate and exhaust the emulator's memory. The nonce binding, single-use consumption and HTML escaping in this flow are otherwise correct. Repro: Given a signed-in browser, When GET /apps/:slug/installations/new?account=<own-login>&state=x is looped ~1e6 times, Then the pendingForms Map holds ~1e6 entries for 10 minutes and the heap grows until the process is killed.
Suggested fix: Store one pending form per user/app/account (overwrite on re-issue instead of minting a new nonce per GET), or enforce a per-user cap on live pending forms before `forms().set(...)`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every authenticated GET of the repository-selection form mints a fresh 24-byte nonce and inserts it into the shared in-memory github.installation.pendingForms map. The _emu_session cookie is set with SameSite=Lax (oauth.ts:183), so it is sent on cross-site top-level GET navigations — an attacker page (window.open/link loop) or a simple scripted loop can allocate unlimited live entries. Entries are purged only lazily and only after FORM_TTL_MS (10 min), so millions of entries can accumulate and exhaust the emulator's memory. The nonce binding, single-use consumption and HTML escaping in this flow are otherwise correct. Repro: Given a signed-in browser, When GET /apps/:slug/installations/new?account=&state=x is looped ~1e6 times, Then the pendingForms Map holds ~1e6 entries for 10 minutes and the heap grows until the process is killed.
const account = allowedAccounts.find((candidate) => candidate.login === selectedLogin);
if (!account) return c.html(error("Forbidden", "You cannot install an application on this account."), 403);
const nonce = randomBytes(24).toString("base64url");
forms().set(nonce, { userId: user.id, appId: ghApp.app_id, account, state, createdAt: Date.now() });
const repositories = gh.repos
.all()| import { escapeAttr, escapeHtml, renderCardPage, renderErrorPage, type RouteContext } from "@emulators/core"; | ||
| import { getGitHubStore } from "../store.js"; | ||
| import { formatUser } from "../helpers.js"; | ||
| import { formatInstallation } from "./apps.js"; |
There was a problem hiding this comment.
🟡 Route modules now import helpers from each other.
| import { formatInstallation } from "./apps.js"; | |
| Move formatInstallation into src/helpers.ts (alongside formatUser/formatRepo) and resolveBrowserSessionUser (optionally with orgRoleForUser) into src/route-helpers.ts, so route modules consume shared helpers instead of each other's internals. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:5-7):
Problem: Route modules now import helpers from each other
Detail: The package convention keeps cross-file shared functions in src/helpers.ts (every format* helper: formatApp, formatUser, formatRepo, formatOrgBrief) and auth/session helpers in src/route-helpers.ts. This PR instead exports formatInstallation from routes/apps.ts, orgRoleForUser from routes/orgs.ts and resolveBrowserSessionUser from routes/oauth.ts, so the new route files import internals from three sibling route files (app-installation-flow.ts imports ./apps.js, ./orgs.js, ./oauth.js; installation-repositories.ts imports ./apps.js). This route-to-route coupling did not exist before and grows with each new consumer.
Suggested fix: Move formatInstallation into src/helpers.ts (alongside formatUser/formatRepo) and resolveBrowserSessionUser (optionally with orgRoleForUser) into src/route-helpers.ts, so route modules consume shared helpers instead of each other's internals.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The package convention keeps cross-file shared functions in src/helpers.ts (every format* helper: formatApp, formatUser, formatRepo, formatOrgBrief) and auth/session helpers in src/route-helpers.ts. This PR instead exports formatInstallation from routes/apps.ts, orgRoleForUser from routes/orgs.ts and resolveBrowserSessionUser from routes/oauth.ts, so the new route files import internals from three sibling route files (app-installation-flow.ts imports ./apps.js, ./orgs.js, ./oauth.js; installation-repositories.ts imports ./apps.js). This route-to-route coupling did not exist before and grows with each new consumer.
import { getGitHubStore } from "../store.js";
import { formatUser } from "../helpers.js";
import { formatInstallation } from "./apps.js";
import { orgRoleForUser } from "./orgs.js";
import { resolveBrowserSessionUser } from "./oauth.js";| .map((org) => ({ id: org.id, login: org.login, type: "Organization" as const })), | ||
| ]; | ||
| } | ||
| const error = (title: string, message: string) => renderErrorPage(title, message, "GitHub"); |
There was a problem hiding this comment.
⚪ "GitHub" service label hardcoded instead of const.
| const error = (title: string, message: string) => renderErrorPage(title, message, "GitHub"); | |
| Define the same SERVICE_LABEL constant in this file (or export it from @emulators/core alongside the render helpers) and pass it to the renderErrorPage/renderCardPage calls. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:42):
Problem: "GitHub" service label hardcoded instead of const
Detail: The sibling UI route file oauth.ts defines `const SERVICE_LABEL = "GitHub"` (line 77) and passes it to every renderErrorPage/renderCardPage/renderSettingsPage call. The new file repeats the string literal "GitHub" three times (lines 42, 60, 83) instead of using a shared constant, duplicating the service label by hand across the two files that render GitHub UI pages.
Suggested fix: Define the same SERVICE_LABEL constant in this file (or export it from @emulators/core alongside the render helpers) and pass it to the renderErrorPage/renderCardPage calls.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The sibling UI route file oauth.ts defines const SERVICE_LABEL = "GitHub" (line 77) and passes it to every renderErrorPage/renderCardPage/renderSettingsPage call. The new file repeats the string literal "GitHub" three times (lines 42, 60, 83) instead of using a shared constant, duplicating the service label by hand across the two files that render GitHub UI pages.
| @@ -0,0 +1,175 @@ | |||
| import { randomBytes } from "node:crypto"; | |||
There was a problem hiding this comment.
⚪ Import style deviates from neighbouring route files.
| import { randomBytes } from "node:crypto"; | |
| Split into `import { randomBytes } from "crypto";`, `import type { RouteContext } from "@emulators/core";` and a value import for the render/escape helpers, matching routes/oauth.ts. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:1-2):
Problem: Import style deviates from neighbouring route files
Detail: All neighbouring route modules use a separate `import type { ... } from "@emulators/core"` statement plus a value import, and import crypto as "crypto" (routes/oauth.ts:1, routes/apps.ts:1). The new file mixes the type into the value import with an inline `type RouteContext` modifier and uses the `node:crypto` specifier, so the module header style diverges from every sibling route file (lint/format pass, so nothing enforces this).
Suggested fix: Split into `import { randomBytes } from "crypto";`, `import type { RouteContext } from "@emulators/core";` and a value import for the render/escape helpers, matching routes/oauth.ts.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
All neighbouring route modules use a separate import type { ... } from "@emulators/core" statement plus a value import, and import crypto as "crypto" (routes/oauth.ts:1, routes/apps.ts:1). The new file mixes the type into the value import with an inline type RouteContext modifier and uses the node:crypto specifier, so the module header style diverges from every sibling route file (lint/format pass, so nothing enforces this).
| ) | ||
| .join("\n"); | ||
| const body = `<form method="post" action="${escapeAttr(path)}"> | ||
| <input type="hidden" name="nonce" value="${nonce}"> |
There was a problem hiding this comment.
⚪ Nonce interpolated into HTML without escapeAttr.
--- a/packages/@emulators/github/src/routes/app-installation-flow.ts
+++ b/packages/@emulators/github/src/routes/app-installation-flow.ts
@@ -73,7 +73,7 @@
const body = `<form method="post" action="${escapeAttr(path)}">
- <input type="hidden" name="nonce" value="${nonce}">
+ <input type="hidden" name="nonce" value="${escapeAttr(nonce)}">
<input type="hidden" name="account" value="${escapeAttr(account.login)}">
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:76):
Problem: Nonce interpolated into HTML without escapeAttr
Detail: Every other dynamic value interpolated into HTML in this file goes through escapeHtml/escapeAttr (the picker links at lines 56-57, the form action, the account hidden field at line 77, the checkbox labels), but the hidden nonce field is inlined raw. The nonce is server-generated base64url ([A-Za-z0-9_-]) so it is currently safe, yet the omission breaks the file's own all-interpolations-escaped convention and would silently become an XSS vector if nonce generation ever changed.
Suggested fix: Use `value="${escapeAttr(nonce)}"` like the adjacent hidden account field on the next line.
Suggested patch:
--- a/packages/@emulators/github/src/routes/app-installation-flow.ts
+++ b/packages/@emulators/github/src/routes/app-installation-flow.ts
@@ -73,7 +73,7 @@
const body = `<form method="post" action="${escapeAttr(path)}">
- <input type="hidden" name="nonce" value="${nonce}">
+ <input type="hidden" name="nonce" value="${escapeAttr(nonce)}">
<input type="hidden" name="account" value="${escapeAttr(account.login)}">
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every other dynamic value interpolated into HTML in this file goes through escapeHtml/escapeAttr (the picker links at lines 56-57, the form action, the account hidden field at line 77, the checkbox labels), but the hidden nonce field is inlined raw. The nonce is server-generated base64url ([A-Za-z0-9_-]) so it is currently safe, yet the omission breaks the file's own all-interpolations-escaped convention and would silently become an XSS vector if nonce generation ever changed.
| { | ||
| action: "created", | ||
| installation: formatInstallation(installation, ghApp, baseUrl, gh), | ||
| repositories: selectedRepos.map((repo) => ({ |
There was a problem hiding this comment.
⚪ Repo-brief mapping duplicated with apps.ts handler.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/github-emulator PR #1, packages/@emulators/github/src/routes/app-installation-flow.ts:158-164):
Problem: Repo-brief mapping duplicated with apps.ts handler
Detail: The five-field repository brief {id, node_id, name, full_name, private} is hand-duplicated from routes/apps.ts:220-229 (the access_tokens response's `repositories` mapping). Both are meant to be the same webhook/API repository shape, so the two copies must be kept in sync manually; a formatRepoBrief helper would derive it once, the same way helpers.ts centralizes the full formatRepo.
Suggested fix: Add a small formatRepoBrief(repo) helper (e.g. in src/helpers.ts next to formatRepo) and reuse it here and in the access_tokens response in routes/apps.ts.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The five-field repository brief {id, node_id, name, full_name, private} is hand-duplicated from routes/apps.ts:220-229 (the access_tokens response's repositories mapping). Both are meant to be the same webhook/API repository shape, so the two copies must be kept in sync manually; a formatRepoBrief helper would derive it once, the same way helpers.ts centralizes the full formatRepo.
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 4 · ⚪ 4 · 0/8 resolved
🤖 Fix all 8 open findings with your agent📋 Out-of-diff findings (8)
Reviewed 20 files · 0 inline · view all 8 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
byapparov
added
enhancement
Review response: PR 1Verified all eight findings against Issues addressed (pushed to this PR)
Review claims verified false (no change needed)None. Not addressed hereNone. Validation: 13 failing regressions reproduced before the fixes; all 23 installation-flow tests and all 266 workspace tests now pass. Build, type checking, formatting, HTTP OAuth/App-token/mutation/signed-webhook smoke, packed external-consumer smoke, and built CLI help passed. Lint reports zero errors and 17 existing warnings. Independent review found no remaining issues. Normal commits and push used no bypass; this repository has no active commit/push hooks. The verify, JavaScript/TypeScript analysis and CodeQL checks all passed on final revision f5e77d0. The application-side cutover is aictrl PR6836. Its canonical local command passed all 14 real-stack cases against this pinned public revision. The application GitHub integration Actions workflow is manual-only (workflow_dispatch), with no PR, push or scheduled test triggers. |
Problem
Seeded installations support GitHub App API tests, but onboarding also needs a browser installation, repository selection, a setup callback and a signed installation webhook.
Behavior
Add
/apps/:slug/installations/newusing the existing OAuth browser session. Users can install on their own account or an organization they administer, choose all or selected repositories, and return to the configuredsetup_urlwithinstallation_id,setup_actionand opaquestate. The emulator dispatches a signedinstallation.createdwebhook.Add scoped installation repository listing, numeric repository installation lookup, organization memberships and granted OAuth scope headers. These let a consuming application validate identity, role and repository access through the emulator.
Boundaries
Verification
The consuming application cutover is aictrl PR6836. Its canonical local command passed all 14 real-stack cases against public revision
f5e77d0b4ccfd2cdb00e877ef0c7c384aad547c9, including installation, scoped tokens/APIs, signed/forged webhooks, callback replay, identity mismatch, returning connected organizations and durable picker/consent/pagination cases. Its GitHub integration workflow is manual-only. Firebase Auth's provider link uses an explicit test helper; GitHub OAuth, installation, API and webhook traffic reach this emulator.Rollout and upstream
Targets the fork's
github-onlybranch. No npm publication or upstream PR is included. Behavior and tests remain in a separate commit from fork docs and CLI help so they can later be proposed fromupstream/main, followingdocs/upstream-workflow.md.Complete GitHub protocol compatibility is not established. Issues are disabled in this fork, so there is no linked issue.