From 91758d76aa69d268768032d95f4681bf64890e42 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 02:46:10 +0000 Subject: [PATCH 1/7] wip(github): Support every App installation Work in progress. Webhook tests in webhook-outcomes.test.ts are not yet updated for the removed installation-ID route options. Co-Authored-By: David Cramer --- apps/example/.env.example | 1 - .../src/content/docs/extend/github-plugin.md | 31 +- .../junior-evals/src/eval-plugin-fixtures.ts | 1 - packages/junior-evals/src/fixture/agent.ts | 3 +- packages/junior-github/SETUP.md | 9 +- .../junior-github/src/credential-support.ts | 317 +++++++++--------- packages/junior-github/src/egress-policy.ts | 63 +++- .../junior-github/src/installation-targets.ts | 74 ++++ packages/junior-github/src/plugin.ts | 32 +- .../junior-github/src/webhooks/check-suite.ts | 6 +- .../junior-github/src/webhooks/handler.ts | 58 ++-- .../src/webhooks/installation-requests.ts | 32 +- .../src/webhooks/issue-outcome.ts | 14 +- .../src/webhooks/pull-request-outcome.ts | 4 +- .../junior-github/tests/github-plugin.test.ts | 177 +++++++--- packages/junior-plugin-api/src/credentials.ts | 1 + packages/junior/.env.example | 1 - .../junior/src/chat/sandbox/egress/session.ts | 5 +- .../tests/component/events/events.test.ts | 7 - .../integration/sandbox-egress-proxy.test.ts | 2 - .../junior/tests/msw/handlers/github-api.ts | 13 + .../plugin-manifest-api-headers.test.ts | 1 - 22 files changed, 543 insertions(+), 309 deletions(-) create mode 100644 packages/junior-github/src/installation-targets.ts diff --git a/apps/example/.env.example b/apps/example/.env.example index 65fca93058..b36f722efe 100644 --- a/apps/example/.env.example +++ b/apps/example/.env.example @@ -14,7 +14,6 @@ GITHUB_APP_ID= GITHUB_APP_CLIENT_ID= GITHUB_APP_CLIENT_SECRET= GITHUB_APP_PRIVATE_KEY= -GITHUB_INSTALLATION_ID= GITHUB_APP_BOT_NAME=sentry-junior[bot] GITHUB_APP_BOT_EMAIL=264270552+sentry-junior[bot]@users.noreply.github.com AI_MODEL= diff --git a/packages/docs/src/content/docs/extend/github-plugin.md b/packages/docs/src/content/docs/extend/github-plugin.md index a46ceae00c..d4c7df0d79 100644 --- a/packages/docs/src/content/docs/extend/github-plugin.md +++ b/packages/docs/src/content/docs/extend/github-plugin.md @@ -67,8 +67,8 @@ narrow write tokens. connected it. 3. Install the App on the organization or repositories Junior should reach. -4. Copy the App ID, OAuth client ID and secret, installation ID, bot name, and - bot noreply email into your deployment environment. +4. Copy the App ID, OAuth client ID and secret, bot name, and bot noreply + email into your deployment environment. If Junior should receive events or report PR/issue outcomes, also: @@ -84,8 +84,13 @@ denies unsupported writes through the egress allowlist. Install the App only on repositories where Junior may push. Protect important branches in GitHub. Across many repositories, users should include `owner/repo` -when the target is not obvious, and those repositories must share the same -installation ID. +when the target is not obvious. + +One deployment works with every installation of the App. Junior finds the +installation for each target repository or organization. A webhook delivery +uses the installation that sent it. Reads that name no repository, such as +search or GraphQL without `owner` and `repo` variables, use the installation on +the account that owns the App. Install the App on its owner account too. ## Config @@ -231,18 +236,6 @@ Names the deployment variable containing the GitHub App private key. -
-installationIdEnv - -Names the deployment variable containing the GitHub App installation ID. - -- **Define:** `githubPlugin({ installationIdEnv: "GITHUB_INSTALLATION_ID" })` in `plugins.ts` -- **Default:** `GITHUB_INSTALLATION_ID` -- **Required:** Yes -- **Environment variable:** The variable named by this option - -
-
botNameEnv @@ -552,8 +545,10 @@ A local `git commit` does not call GitHub. The write happens on push. Grant permission on the installation, and retry. - Webhook delivery returns `401`: the App webhook secret does not match `GITHUB_WEBHOOK_SECRET`, or `X-Hub-Signature-256` is missing. -- Webhook delivery returns `202 Ignored`: wrong installation, or an unsupported - event mapping. Confirm `GITHUB_INSTALLATION_ID` and the event type. +- Webhook delivery returns `202 Ignored`: the payload has no App installation, + or the event mapping is unsupported. Confirm the event type. +- `The GitHub App is not installed on owner/repo`: install the App on that + repository or its organization, and retry. - Delivery succeeds but nothing appears in Slack: create a watch or event automation first. A webhook alone does not create either one. - Missing repository context: include `owner/repo`, or set a thread default diff --git a/packages/junior-evals/src/eval-plugin-fixtures.ts b/packages/junior-evals/src/eval-plugin-fixtures.ts index 1043abb18c..a1e725509e 100644 --- a/packages/junior-evals/src/eval-plugin-fixtures.ts +++ b/packages/junior-evals/src/eval-plugin-fixtures.ts @@ -25,7 +25,6 @@ const githubPrivateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) export function evalGitHubEnv(): Record { return { GITHUB_APP_ID: "12345", - GITHUB_INSTALLATION_ID: "67890", GITHUB_APP_PRIVATE_KEY: githubPrivateKey, GITHUB_APP_BOT_NAME: "junior-eval", GITHUB_APP_BOT_EMAIL: "12345+junior-eval[bot]@users.noreply.github.com", diff --git a/packages/junior-evals/src/fixture/agent.ts b/packages/junior-evals/src/fixture/agent.ts index 3f8a594522..5542e95a51 100644 --- a/packages/junior-evals/src/fixture/agent.ts +++ b/packages/junior-evals/src/fixture/agent.ts @@ -930,7 +930,8 @@ function automationRequest(input: AutomationInput): Request { } const body = JSON.stringify({ ...input.payload, - installation: { id: Number(requiredEnv("GITHUB_INSTALLATION_ID")) }, + // Any id works: Junior uses the installation that sent the delivery. + installation: { id: 67890 }, }); const signature = createHmac("sha256", requiredEnv("GITHUB_WEBHOOK_SECRET")) .update(body) diff --git a/packages/junior-github/SETUP.md b/packages/junior-github/SETUP.md index 778911c551..cd21000625 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -42,13 +42,12 @@ Set on the harness host (never in skill files): - `GITHUB_APP_CLIENT_ID` - `GITHUB_APP_CLIENT_SECRET` - `GITHUB_APP_PRIVATE_KEY` -- `GITHUB_INSTALLATION_ID` - `GITHUB_APP_BOT_NAME` (Git author and committer display name) - `GITHUB_APP_BOT_EMAIL` -Current limitation: one Junior deployment uses one GitHub App installation ID. -That works for repositories covered by the same installation, but not for repositories that live -under different app installations across orgs/accounts. +One Junior deployment works with every installation of the App. Junior finds +the installation for each target repository or organization. Reads that name no +repository use the installation on the account that owns the App. ### Vercel env setup (multiline-safe) @@ -61,7 +60,6 @@ For Vercel, prefer CLI file input so newlines are preserved exactly: vercel env add GITHUB_APP_ID production vercel env add GITHUB_APP_CLIENT_ID production vercel env add GITHUB_APP_CLIENT_SECRET production -vercel env add GITHUB_INSTALLATION_ID production vercel env add GITHUB_APP_BOT_NAME production vercel env add GITHUB_APP_BOT_EMAIL production vercel env add GITHUB_APP_PRIVATE_KEY production --sensitive < ./github-app-private-key.pem @@ -246,7 +244,6 @@ jr-rpc config set github.repo getsentry/junior - `GITHUB_APP_CLIENT_ID` - `GITHUB_APP_CLIENT_SECRET` - `GITHUB_APP_PRIVATE_KEY` - - `GITHUB_INSTALLATION_ID` - `GITHUB_APP_BOT_NAME` - `GITHUB_APP_BOT_EMAIL` 2. Confirm the GitHub App is installed on your test repo with the permissions above. diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 6daa48f09a..1030f5b9aa 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -6,6 +6,7 @@ */ import { createPrivateKey, createSign } from "node:crypto"; import type { + PluginCache, PluginCredentialResult, PluginGrant, PluginProviderAccount, @@ -13,6 +14,7 @@ import type { PluginUserTokenSlot, IssueCredentialHookContext, } from "@sentry/junior-plugin-api"; +import { installationLookupForLeaseScope } from "./installation-targets.js"; import { type GitHubAppPermissions, readGrantPermissions, @@ -78,43 +80,20 @@ interface UserCredentialOptions { userScope?: string; } -interface InstallationCredentialBaseOptions { +/** Environment variable names for the GitHub App credentials. */ +export interface GitHubAppEnv { appIdEnv: string; - installationIdEnv: string; privateKeyEnv: string; } -type InstallationCredentialOptions = InstallationCredentialBaseOptions & - ( - | { - // Omit both to use the full installed App permissions and repos. - loadPermissions?: never; - permissions?: GitHubAppPermissions; - repositories?: string[]; - } - | { - loadPermissions: LoadInstallationReadPermissions; - permissions?: never; - repositories?: never; - } - ); - -type LoadInstallationReadPermissions = (input: { - appJwt: string; - installationId: number; -}) => Promise>; - -interface GitHubRepository { - name: string; - owner: string; -} - export const GITHUB_APP_ID_ENV = "GITHUB_APP_ID"; export const GITHUB_APP_PRIVATE_KEY_ENV = "GITHUB_APP_PRIVATE_KEY"; -export const GITHUB_INSTALLATION_ID_ENV = "GITHUB_INSTALLATION_ID"; export const GITHUB_AUTH_TOKEN_ENV = "GITHUB_TOKEN"; export const GITHUB_AUTH_TOKEN_PLACEHOLDER = "ghp_host_managed_credential"; const MAX_LEASE_MS = 60 * 60 * 1000; +// Installs change rarely. A reinstalled App can keep a stale id this long. +const INSTALLATION_ID_TTL_MS = 10 * 60 * 1000; +const GITHUB_API = "https://api.github.com"; const REFRESH_BUFFER_MS = 5 * 60 * 1000; const USER_REFRESH_TIMEOUT_MS = 20_000; export const GITHUB_GRAPHQL_RESPONSE_BODY_LIMIT_BYTES = 64 * 1024; @@ -211,10 +190,6 @@ function hasRequiredOAuthScope( return required.every((scope) => stored.has(scope)); } -function isGitHubApiUrl(upstreamUrl: URL): boolean { - return upstreamUrl.hostname.toLowerCase() === "api.github.com"; -} - function base64Url(input: string): string { return Buffer.from(input) .toString("base64") @@ -539,36 +514,6 @@ function readInstallationPermissions( return readGrantPermissions(installation.permissions); } -function decodeGitHubPathSegment(value: string): string | undefined { - try { - const decoded = decodeURIComponent(value).trim(); - return decoded && !decoded.includes("/") ? decoded : undefined; - } catch { - return undefined; - } -} - -/** Parse a GitHub repository from an API or Git URL. */ -export function githubRepositoryFromUrl( - upstreamUrl: URL, -): GitHubRepository | undefined { - const segments = upstreamUrl.pathname.split("/").filter(Boolean); - if (isGitHubApiUrl(upstreamUrl) && segments[0]?.toLowerCase() === "repos") { - const owner = segments[1] - ? decodeGitHubPathSegment(segments[1]) - : undefined; - const name = segments[2] ? decodeGitHubPathSegment(segments[2]) : undefined; - return owner && name ? { owner, name } : undefined; - } - if (upstreamUrl.hostname.toLowerCase() !== "github.com") { - return undefined; - } - const owner = segments[0] ? decodeGitHubPathSegment(segments[0]) : undefined; - const rawName = segments[1]?.replace(/\.git$/i, ""); - const name = rawName ? decodeGitHubPathSegment(rawName) : undefined; - return owner && name ? { owner, name } : undefined; -} - /** Resolve the GitHub account associated with stored user tokens. */ export async function resolveUserAccount( tokens: PluginStoredTokens, @@ -823,63 +768,152 @@ async function issueUserCredential( return credentialNeeded("Your GitHub authorization has expired.", scope); } -/** Issue a bounded raw token for plugin-owned GitHub API calls. */ -export async function issueInstallationToken( - options: InstallationCredentialOptions, +async function mintInstallationToken( + appJwt: string, + installationId: number, + body: { permissions?: GitHubAppPermissions; repositories?: string[] }, ): Promise<{ expiresAtMs: number; token: string }> { - const appId = requireEnv(options.appIdEnv); - const installationIdRaw = requireEnv(options.installationIdEnv); - const installationId = Number(installationIdRaw); - if (!Number.isSafeInteger(installationId) || installationId <= 0) { - throw new GitHubPluginSetupError(`Invalid ${options.installationIdEnv}`); - } - - const appJwt = createAppJwt(appId, options.privateKeyEnv); - const permissions = - "permissions" in options - ? options.permissions - : typeof options.loadPermissions === "function" - ? await options.loadPermissions({ appJwt, installationId }) - : undefined; - const repositories = - "repositories" in options ? options.repositories : undefined; - const body = { - ...(permissions ? { permissions } : undefined), - ...(repositories ? { repositories } : undefined), - }; - const accessTokenResponse = await githubRequest( - "https://api.github.com", - `/app/installations/${installationId}/access_tokens`, - { - method: "POST", - token: appJwt, - body, - }, + const parsedToken = parseInstallationTokenResponse( + await githubRequest( + GITHUB_API, + `/app/installations/${installationId}/access_tokens`, + { method: "POST", token: appJwt, body }, + ), ); - const parsedToken = parseInstallationTokenResponse(accessTokenResponse); return { expiresAtMs: Math.min(parsedToken.expiresAtMs, Date.now() + MAX_LEASE_MS), token: parsedToken.token, }; } -/** Issue a bounded GitHub App installation credential. */ -async function issueInstallationCredential( - options: InstallationCredentialOptions, -): Promise { - const token = await issueInstallationToken(options); - return createCredentialLease({ - token: token.token, - expiresAtMs: token.expiresAtMs, +/** + * Issue a bounded raw token for one App installation. Omit `permissions` and + * `repositories` to use the full installed App permissions and repos. + */ +export async function issueInstallationToken( + options: GitHubAppEnv & { + installationId: number; + permissions?: GitHubAppPermissions; + repositories?: string[]; + }, +): Promise<{ expiresAtMs: number; token: string }> { + return await mintInstallationToken( + createAppJwt(requireEnv(options.appIdEnv), options.privateKeyEnv), + options.installationId, + { permissions: options.permissions, repositories: options.repositories }, + ); +} + +/** Read one App installation id, or undefined when the App is not installed. */ +async function findInstallationId( + cache: PluginCache, + appJwt: string, + path: string, +): Promise { + const id = await cache.getOrLoad( + `installation-id:${path}`, + async () => { + let installation: unknown; + try { + installation = await githubRequest(GITHUB_API, path, { token: appJwt }); + } catch (error) { + if (error instanceof GitHubRequestError && error.status === 404) { + // Do not cache: the App can be installed at any time. + return { value: null }; + } + throw error; + } + const value = isRecord(installation) ? installation.id : undefined; + if (typeof value !== "number" || !Number.isSafeInteger(value)) { + throw new Error("GitHub installation response missing id"); + } + return { value, ttlMs: INSTALLATION_ID_TTL_MS }; + }, + ); + return id ?? undefined; +} + +/** Read the installation on the account that owns the App. */ +async function appOwnerInstallationId( + cache: PluginCache, + appJwt: string, +): Promise { + const owner = await cache.getOrLoad("app-owner", async () => { + const app = await githubRequest(GITHUB_API, "/app", { token: appJwt }); + const value = isRecord(app) && isRecord(app.owner) ? app.owner : undefined; + if (typeof value?.login !== "string" || !value.login) { + throw new Error("GitHub App response missing owner"); + } + return { + value: { + login: value.login, + organization: value.type === "Organization", + }, + ttlMs: INSTALLATION_ID_TTL_MS, + }; }); + const id = await findInstallationId( + cache, + appJwt, + `/${owner.organization ? "orgs" : "users"}/${encodeURIComponent(owner.login)}/installation`, + ); + if (!id) { + throw new GitHubPluginSetupError( + `The GitHub App is not installed on ${owner.login}.`, + ); + } + return id; } -interface GitHubCredentialOptions - extends InstallationCredentialBaseOptions, UserCredentialOptions { - /** Declared read permissions, or a loader for the installed ones. */ - readPermissions: - | { permissions: GitHubAppPermissions } - | { loadPermissions: LoadInstallationReadPermissions }; +/** + * Resolve the App installation for an installation grant from its lease + * scope. Writes need the target's own installation. Reads without one, or + * without a target, use the App owner's installation, which can still read + * public repositories. + */ +async function resolveInstallationId( + ctx: IssueCredentialHookContext, + appJwt: string, +): Promise { + const lookup = installationLookupForLeaseScope(ctx.grant.leaseScope); + if (lookup) { + const id = await findInstallationId(ctx.cache, appJwt, lookup.path); + if (id) return id; + if (ctx.grant.access === "write") { + throw new GitHubPluginSetupError( + `The GitHub App is not installed on ${lookup.label}.`, + ); + } + } + return await appOwnerInstallationId(ctx.cache, appJwt); +} + +/** Read an installation's permissions as read grants, cached per lease period. */ +async function installedReadPermissions( + cache: PluginCache, + appJwt: string, + installationId: number, +): Promise> { + return await cache.getOrLoad( + `installation-permissions:${installationId}`, + async () => ({ + value: readInstallationPermissions( + await githubRequest( + GITHUB_API, + `/app/installations/${installationId}`, + { + token: appJwt, + }, + ), + ), + ttlMs: MAX_LEASE_MS, + }), + ); +} + +interface GitHubCredentialOptions extends GitHubAppEnv, UserCredentialOptions { + /** Declared read permissions. Omit to read the installed ones. */ + readPermissions?: Record; } /** @@ -891,22 +925,26 @@ export async function issueGitHubCredential( ctx: IssueCredentialHookContext, options: GitHubCredentialOptions, ): Promise { - const installation = { - appIdEnv: options.appIdEnv, - installationIdEnv: options.installationIdEnv, - privateKeyEnv: options.privateKeyEnv, - }; try { - if (ctx.grant.name === "installation-read") { - return await issueInstallationCredential({ - ...installation, - ...options.readPermissions, - }); - } - if (ctx.grant.name === "installation-write") { - // Use the full installed App permissions. Repo allowlisting stays in - // egress policy, not in per-request token minting. - return await issueInstallationCredential(installation); + if ( + ctx.grant.name === "installation-read" || + ctx.grant.name === "installation-write" + ) { + const appJwt = createAppJwt( + requireEnv(options.appIdEnv), + options.privateKeyEnv, + ); + const installationId = await resolveInstallationId(ctx, appJwt); + // Write tokens use the full installed App permissions and repos. Repo + // allowlisting stays in egress policy, not in per-request token minting. + const permissions = + ctx.grant.name === "installation-read" + ? (options.readPermissions ?? + (await installedReadPermissions(ctx.cache, appJwt, installationId))) + : undefined; + return createCredentialLease( + await mintInstallationToken(appJwt, installationId, { permissions }), + ); } if (USER_TOKEN_GRANTS.has(ctx.grant.name)) { return await issueUserCredential(ctx, options); @@ -921,36 +959,3 @@ export async function issueGitHubCredential( `GitHub plugin cannot issue unknown grant "${ctx.grant.name}".`, ); } - -/** Cache the installation's read permissions for one lease period. */ -export function createPermissionCache(): LoadInstallationReadPermissions { - let cached: - | { - expiresAtMs: number; - permissions: Record; - } - | undefined; - let pending: Promise> | undefined; - return async ({ appJwt, installationId }) => { - if (cached && cached.expiresAtMs > Date.now()) { - return cached.permissions; - } - pending ??= githubRequest( - "https://api.github.com", - `/app/installations/${installationId}`, - { token: appJwt }, - ) - .then((installation) => { - const permissions = readInstallationPermissions(installation); - cached = { - expiresAtMs: Date.now() + MAX_LEASE_MS, - permissions, - }; - return permissions; - }) - .finally(() => { - pending = undefined; - }); - return await pending; - }; -} diff --git a/packages/junior-github/src/egress-policy.ts b/packages/junior-github/src/egress-policy.ts index f87a28e691..4d2f9e88de 100644 --- a/packages/junior-github/src/egress-policy.ts +++ b/packages/junior-github/src/egress-policy.ts @@ -17,12 +17,16 @@ import { GITHUB_GRAPHQL_RESPONSE_BODY_LIMIT_BYTES, HTTP_READ_METHODS, USER_WRITE_REQUIREMENTS, - githubRepositoryFromUrl, isRecord, type GitHubGrant, type GitHubGrantName, type GitHubGrantReason, } from "./credential-support.js"; +import { + githubInstallationLeaseScope, + githubRepositoryFromUrl, + type GitHubInstallationTarget, +} from "./installation-targets.js"; function githubSmartHttpAccess( upstreamUrl: URL, @@ -588,17 +592,65 @@ function grantForAccess( access: PluginGrantAccess, reason: GitHubGrantReason, name: GitHubGrantName, + target?: GitHubInstallationTarget, ): GitHubGrant { return { name, access, reason, + ...(target + ? { leaseScope: githubInstallationLeaseScope(target) } + : undefined), ...(name === "user-write" ? { requirements: USER_WRITE_REQUIREMENTS } : undefined), }; } +const GITHUB_NAME_PATTERN = /^[\w.-]+$/; + +/** Read the repository from GraphQL `owner` and `repo` or `name` variables, as `gh` sends them. */ +function graphqlVariablesRepository( + bodyText: string | undefined, +): GitHubInstallationTarget | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(bodyText ?? ""); + } catch { + return undefined; + } + const variables = isRecord(parsed) ? parsed.variables : undefined; + if (!isRecord(variables)) return undefined; + const owner = variables.owner; + const name = variables.repo ?? variables.name; + return typeof owner === "string" && + typeof name === "string" && + GITHUB_NAME_PATTERN.test(owner) && + GITHUB_NAME_PATTERN.test(name) + ? { owner, name } + : undefined; +} + +/** + * Find the repository or organization whose App installation a request + * needs. Return undefined when the request names neither. + */ +function installationTarget( + upstreamUrl: URL, + bodyText: string | undefined, +): GitHubInstallationTarget | undefined { + const repository = + githubRepositoryFromUrl(upstreamUrl) ?? + (isGitHubGraphqlUrl(upstreamUrl) + ? graphqlVariablesRepository(bodyText) + : undefined); + if (repository) return repository; + const org = isGitHubApiUrl(upstreamUrl) + ? /^\/orgs\/([\w.-]+)(?:\/|$)/i.exec(upstreamUrl.pathname)?.[1] + : undefined; + return org ? { org } : undefined; +} + function requireRepositoryTarget(upstreamUrl: URL): void { if (githubRepositoryFromUrl(upstreamUrl)) { return; @@ -626,6 +678,7 @@ export async function githubGrantForEgress( if (isGitHubAssetUploadRequest(method, upstreamUrl)) { return grantForAccess("write", "github.asset-upload", "user-write"); } + const target = installationTarget(upstreamUrl, ctx.request.bodyText); const smartHttpAccess = githubSmartHttpAccess(upstreamUrl); if (smartHttpAccess) { @@ -635,12 +688,14 @@ export async function githubGrantForEgress( "write", "github.installation-write", "installation-write", + target, ); } return grantForAccess( smartHttpAccess, "github.git-read", "installation-read", + target, ); } @@ -660,6 +715,7 @@ export async function githubGrantForEgress( ? "github.user-write" : "github.installation-write", writeGrantName, + writeGrantName === "installation-write" ? target : undefined, ); } @@ -670,10 +726,12 @@ export async function githubGrantForEgress( ctx.request.bodyText, ); if (hostMutationRepository) { + const [owner = "", name = ""] = hostMutationRepository.split("/"); return grantForAccess( "write", "github.installation-write", "installation-write", + { owner, name }, ); } @@ -692,6 +750,7 @@ export async function githubGrantForEgress( graphqlAccess, "github.graphql-read", "installation-read", + target, ); } @@ -701,5 +760,5 @@ export async function githubGrantForEgress( "This GitHub write request is not supported. Use the matching GitHub tool or a supported REST endpoint.", ); } - return grantForAccess(access, "github.api-read", "installation-read"); + return grantForAccess(access, "github.api-read", "installation-read", target); } diff --git a/packages/junior-github/src/installation-targets.ts b/packages/junior-github/src/installation-targets.ts new file mode 100644 index 0000000000..3ab782a83e --- /dev/null +++ b/packages/junior-github/src/installation-targets.ts @@ -0,0 +1,74 @@ +/** + * GitHub App installation targets. + * + * Owns how a GitHub request names the repository or organization whose App + * installation it needs, and how a grant lease scope carries that target to + * credential issuance. + */ + +/** Repository or organization whose App installation a grant uses. */ +export type GitHubInstallationTarget = + | { name: string; owner: string } + | { org: string }; + +function decodeGitHubPathSegment(value: string): string | undefined { + try { + const decoded = decodeURIComponent(value).trim(); + return decoded && !decoded.includes("/") ? decoded : undefined; + } catch { + return undefined; + } +} + +/** Parse a GitHub repository from an API or Git URL. */ +export function githubRepositoryFromUrl( + upstreamUrl: URL, +): { name: string; owner: string } | undefined { + const segments = upstreamUrl.pathname.split("/").filter(Boolean); + const host = upstreamUrl.hostname.toLowerCase(); + if (host === "api.github.com" && segments[0]?.toLowerCase() === "repos") { + const owner = segments[1] + ? decodeGitHubPathSegment(segments[1]) + : undefined; + const name = segments[2] ? decodeGitHubPathSegment(segments[2]) : undefined; + return owner && name ? { owner, name } : undefined; + } + if (host !== "github.com") { + return undefined; + } + const owner = segments[0] ? decodeGitHubPathSegment(segments[0]) : undefined; + const rawName = segments[1]?.replace(/\.git$/i, ""); + const name = rawName ? decodeGitHubPathSegment(rawName) : undefined; + return owner && name ? { owner, name } : undefined; +} + +/** Build the grant lease scope that keeps each target's installation lease apart. */ +export function githubInstallationLeaseScope( + target: GitHubInstallationTarget, +): string { + return ( + "org" in target + ? `organization:${target.org}` + : `repository:${target.owner}/${target.name}` + ).toLowerCase(); +} + +/** + * Return the GitHub API path that finds the App installation for a lease + * scope, and the target name for errors. Return undefined for no target. + */ +export function installationLookupForLeaseScope( + leaseScope: string | undefined, +): { label: string; path: string } | undefined { + const repository = /^repository:([^/]+)\/([^/]+)$/.exec(leaseScope ?? ""); + if (repository?.[1] && repository[2]) { + return { + label: `${repository[1]}/${repository[2]}`, + path: `/repos/${encodeURIComponent(repository[1])}/${encodeURIComponent(repository[2])}/installation`, + }; + } + const org = /^organization:([^/]+)$/.exec(leaseScope ?? "")?.[1]; + return org + ? { label: org, path: `/orgs/${encodeURIComponent(org)}/installation` } + : undefined; +} diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 7915367a71..8dc07719ce 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -78,8 +78,6 @@ import { GITHUB_APP_PRIVATE_KEY_ENV, GITHUB_AUTH_TOKEN_ENV, GITHUB_AUTH_TOKEN_PLACEHOLDER, - GITHUB_INSTALLATION_ID_ENV, - createPermissionCache, issueGitHubCredential, listVerifiedUserEmails, normalizeScopeList, @@ -125,8 +123,6 @@ export interface GitHubPluginOptions { /** Environment variable containing the GitHub App OAuth client secret. */ clientSecretEnv?: string; - /** Environment variable containing the GitHub App installation id. */ - installationIdEnv?: string; /** Environment variable containing the GitHub App private key. */ privateKeyEnv?: string; /** App-configured pull request event behavior. */ @@ -143,13 +139,10 @@ export function githubPlugin( const clientSecretEnv = options.clientSecretEnv ?? "GITHUB_APP_CLIENT_SECRET"; const appIdEnv = options.appIdEnv ?? GITHUB_APP_ID_ENV; const privateKeyEnv = options.privateKeyEnv ?? GITHUB_APP_PRIVATE_KEY_ENV; - const installationIdEnv = - options.installationIdEnv ?? GITHUB_INSTALLATION_ID_ENV; const declaredAppPermissions = normalizePermissions(options.appPermissions); const declaredReadPermissions = declaredAppPermissions ? readGrantPermissions(declaredAppPermissions) : undefined; - const loadReadPermissions = createPermissionCache(); const userScopes = normalizeScopeList(options.additionalUserScopes); const userScope = userScopes.length ? userScopes.join(" ") : undefined; @@ -216,7 +209,6 @@ export function githubPlugin( envVars: { [appIdEnv]: {}, [privateKeyEnv]: {}, - [installationIdEnv]: {}, [clientIdEnv]: {}, [clientSecretEnv]: {}, [botNameEnv]: { exposeToCommandEnv: true }, @@ -332,11 +324,7 @@ export function githubPlugin( }; }, routes(ctx) { - const installationEnv = { - appIdEnv, - installationIdEnv, - privateKeyEnv, - }; + const app = { appIdEnv, privateKeyEnv }; return [ createGitHubWebhookRoute({ annotations: ctx.annotations, @@ -345,26 +333,21 @@ export function githubPlugin( classifyPullRequestCommits: async (pullRequest) => await classifyGitHubPullRequestCommitComposition({ botEmail: requireEnv(botEmailEnv), - loadPage: await loadPullRequestCommitPages( - installationEnv, - pullRequest, - ), + loadPage: await loadPullRequestCommitPages(app, pullRequest), }), codeChanges: ctx.codeChanges, db: ctx.db as GitHubDb, - installationId: () => readEnv(installationIdEnv), - installationIdEnv, loadRepositoryRole: async (target) => - await loadRepositoryRole(installationEnv, ctx.cache, target), + await loadRepositoryRole(app, ctx.cache, target), loadGitHubLogin: async (identityId) => linkedGitHubLogin( (await ctx.users.readIdentity(identityId))?.user, ), loadPullRequestCommitPages: async (pullRequest) => - await loadPullRequestCommitPages(installationEnv, pullRequest), + await loadPullRequestCommitPages(app, pullRequest), log: ctx.log, markFeedbackReviewing: async (target) => - await markPullRequestFeedbackReviewing(installationEnv, target), + await markPullRequestFeedbackReviewing(app, target), privateKeyEnv, events: ctx.events, webhookSecret: () => readEnv("GITHUB_WEBHOOK_SECRET"), @@ -469,11 +452,8 @@ export function githubPlugin( appIdEnv, clientIdEnv, clientSecretEnv, - installationIdEnv, privateKeyEnv, - readPermissions: declaredReadPermissions - ? { permissions: declaredReadPermissions } - : { loadPermissions: loadReadPermissions }, + readPermissions: declaredReadPermissions, userScope, }); }, diff --git a/packages/junior-github/src/webhooks/check-suite.ts b/packages/junior-github/src/webhooks/check-suite.ts index 46746a074c..bdb2fa9a9a 100644 --- a/packages/junior-github/src/webhooks/check-suite.ts +++ b/packages/junior-github/src/webhooks/check-suite.ts @@ -579,7 +579,7 @@ export function needsCheckSuitePullRequestFacts( export async function loadCheckSuiteFacts(args: { appIdEnv: string; body: unknown; - installationIdEnv: string; + installationId: number; loadPullRequestFacts?: boolean; log?: { error(message: string, metadata?: Record): void }; privateKeyEnv: string; @@ -595,7 +595,7 @@ export async function loadCheckSuiteFacts(args: { try { const token = await issueInstallationToken({ appIdEnv: args.appIdEnv, - installationIdEnv: args.installationIdEnv, + installationId: args.installationId, permissions: { checks: "read" }, privateKeyEnv: args.privateKeyEnv, repositories: [target.repoName], @@ -623,7 +623,7 @@ export async function loadCheckSuiteFacts(args: { try { const token = await issueInstallationToken({ appIdEnv: args.appIdEnv, - installationIdEnv: args.installationIdEnv, + installationId: args.installationId, permissions: { pull_requests: "read" }, privateKeyEnv: args.privateKeyEnv, repositories: [target.repoName], diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index b1ffa60bb0..b933a501a2 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -108,6 +108,9 @@ const feedbackTargetSchema = z.object({ }); type FeedbackTarget = z.output; +/** A webhook lookup target with the installation that sent the delivery. */ +type InstallationScoped = T & { installationId: number }; + /** Return comment-level pull request feedback that the webhook can mark. */ function pullRequestFeedbackTarget( events: EventInput[], @@ -149,27 +152,28 @@ export function createGitHubWebhookRoute(args: { annotations: PluginConversationAnnotations; appIdEnv: string; botEmail(): string | undefined; - classifyPullRequestCommits?(input: { - number: number; - repositoryFullName: string; - }): Promise; + classifyPullRequestCommits?( + input: InstallationScoped, + ): Promise; codeChanges: CodeChangePublisher; db: GitHubDb; - installationId(): string | undefined; - installationIdEnv: string; /** * Read a pull request's commits for the `authors` field and the people * involved. */ loadPullRequestCommitPages?( - target: GitHubPullRequestTarget, + target: InstallationScoped, ): Promise; /** Load a repository role for `openerRole`. */ - loadRepositoryRole?(target: GitHubRoleTarget): Promise; + loadRepositoryRole?( + target: InstallationScoped, + ): Promise; /** Linked GitHub login of the user of one Junior identity, if any. */ loadGitHubLogin?(identityId: string): Promise; - log?: Pick; - markFeedbackReviewing(input: FeedbackTarget): Promise; + log?: Pick; + markFeedbackReviewing( + input: InstallationScoped, + ): Promise; privateKeyEnv: string; events: EventPublisher; webhookSecret(): string | undefined; @@ -190,13 +194,15 @@ export function createGitHubWebhookRoute(args: { } const body = parseJson(rawBody); - const installationId = parseInstallationId(args.installationId()); + // The signature proves the delivery is for this App, from any of its + // installations. Every token for this delivery uses that installation. + const installationId = webhookInstallationId(body); if (!installationId) { - return new Response("GitHub webhook installation is not configured", { - status: 503, + args.log?.info("github.webhook.delivery.skipped", { + "app.github.delivery_id": deliveryId, + "app.github.event_name": eventName, + "app.github.skip_reason": "missing_installation", }); - } - if (webhookInstallationId(body) !== installationId) { return new Response("Ignored", { status: 202 }); } const botEmail = args.botEmail(); @@ -259,6 +265,7 @@ export function createGitHubWebhookRoute(args: { let commitComposition: GitHubPullRequestCommitComposition | undefined; try { commitComposition = await args.classifyPullRequestCommits({ + installationId, number: pullRequestOutcome.number, repositoryFullName: pullRequestOutcome.repositoryFullName, }); @@ -334,7 +341,7 @@ export function createGitHubWebhookRoute(args: { ? await loadCheckSuiteFacts({ appIdEnv: args.appIdEnv, body, - installationIdEnv: args.installationIdEnv, + installationId, loadPullRequestFacts: needsCheckSuitePullRequestFacts(checkSuiteMatchKeys), log: args.log, @@ -342,9 +349,14 @@ export function createGitHubWebhookRoute(args: { }) : undefined; const hasMatch = args.events.hasMatch; - const commitPages = args.loadPullRequestCommitPages - ? shareCommitPages(args.loadPullRequestCommitPages) + const loadCommitPages = args.loadPullRequestCommitPages; + const commitPages = loadCommitPages + ? shareCommitPages( + async (target) => + await loadCommitPages({ ...target, installationId }), + ) : undefined; + const loadRepositoryRole = args.loadRepositoryRole; const events = await withGitHubPullRequestPeople({ body, deliveryId, @@ -370,7 +382,10 @@ export function createGitHubWebhookRoute(args: { } : undefined), loadGitHubLogin: args.loadGitHubLogin, - loadRole: args.loadRepositoryRole, + loadRole: loadRepositoryRole + ? async (target) => + await loadRepositoryRole({ ...target, installationId }) + : undefined, log: args.log, neededMatchKeys: args.events.neededMatchKeys, }), @@ -395,7 +410,10 @@ export function createGitHubWebhookRoute(args: { : false; if (feedbackTarget && feedbackHasMatch) { try { - await args.markFeedbackReviewing(feedbackTarget); + await args.markFeedbackReviewing({ + ...feedbackTarget, + installationId, + }); } catch (error) { args.log?.error("GitHub pull request feedback reaction failed", { commentId: feedbackTarget.commentId, diff --git a/packages/junior-github/src/webhooks/installation-requests.ts b/packages/junior-github/src/webhooks/installation-requests.ts index 9c64f24057..1b1ae79b43 100644 --- a/packages/junior-github/src/webhooks/installation-requests.ts +++ b/packages/junior-github/src/webhooks/installation-requests.ts @@ -9,19 +9,13 @@ import { z } from "zod"; import { githubRequest, issueInstallationToken, + type GitHubAppEnv, } from "../credential-support.js"; import { pullRequestCommentReactionsPath, pullRequestFeedbackReactionContent, } from "../tools/update-pull-request-feedback.js"; -/** Environment variable names for the GitHub App installation. */ -export interface GitHubInstallationEnv { - appIdEnv: string; - installationIdEnv: string; - privateKeyEnv: string; -} - function splitRepository(fullName: string, label: string) { const [owner, name, ...extra] = fullName.split("/"); if (!owner || !name || extra.length > 0) { @@ -32,15 +26,20 @@ function splitRepository(fullName: string, label: string) { /** Return a reader for pages of a pull request's commits, with a read-only token. */ export async function loadPullRequestCommitPages( - env: GitHubInstallationEnv, - pullRequest: { number: number; repositoryFullName: string }, + app: GitHubAppEnv, + pullRequest: { + installationId: number; + number: number; + repositoryFullName: string; + }, ): Promise<(page: number, perPage: number) => Promise> { const { owner, name } = splitRepository( pullRequest.repositoryFullName, "pull request commits", ); const token = await issueInstallationToken({ - ...env, + ...app, + installationId: pullRequest.installationId, permissions: { pull_requests: "read" }, repositories: [name], }); @@ -54,16 +53,18 @@ export async function loadPullRequestCommitPages( /** Add the "reviewing" reaction to one pull request feedback comment. */ export async function markPullRequestFeedbackReviewing( - env: GitHubInstallationEnv, + app: GitHubAppEnv, target: { commentId: number; commentKind: "conversation" | "review"; + installationId: number; repo: string; }, ): Promise { const { owner, name } = splitRepository(target.repo, "pull request feedback"); const token = await issueInstallationToken({ - ...env, + ...app, + installationId: target.installationId, repositories: [name], }); await githubRequest( @@ -96,9 +97,9 @@ const REPOSITORY_ROLE_TTL_MS = 10 * 60 * 1000; * cached for ten minutes. */ export async function loadRepositoryRole( - env: GitHubInstallationEnv, + app: GitHubAppEnv, cache: PluginCache, - target: { login: string; repositoryFullName: string }, + target: { installationId: number; login: string; repositoryFullName: string }, ): Promise { const { owner, name } = splitRepository( target.repositoryFullName, @@ -108,7 +109,8 @@ export async function loadRepositoryRole( `repository-role:${owner}/${name}:${target.login}`.toLowerCase(), async () => { const token = await issueInstallationToken({ - ...env, + ...app, + installationId: target.installationId, permissions: { metadata: "read" }, repositories: [name], }); diff --git a/packages/junior-github/src/webhooks/issue-outcome.ts b/packages/junior-github/src/webhooks/issue-outcome.ts index 5b749c6475..b465b7cda0 100644 --- a/packages/junior-github/src/webhooks/issue-outcome.ts +++ b/packages/junior-github/src/webhooks/issue-outcome.ts @@ -133,7 +133,9 @@ export function normalizeGitHubIssueOutcome(args: { repositoryId: String(parsed.repository.id), state: parsed.action === "closed" ? "closed" : "open", stateReason: - parsed.action === "closed" ? (issue.state_reason ?? undefined) : undefined, + parsed.action === "closed" + ? (issue.state_reason ?? undefined) + : undefined, updatedAt, }; } @@ -147,7 +149,10 @@ const canonicalIssueConversationSchema = z user: z.object({ login: z.string().min(1) }).strict(), }) .strict(), - sender: z.object({ login: z.string().min(1) }).strict().optional(), + sender: z + .object({ login: z.string().min(1) }) + .strict() + .optional(), }) .strict(); @@ -160,7 +165,10 @@ const issueConversationSchema = z user: z.object({ login: z.string().min(1) }).passthrough(), }) .passthrough(), - sender: z.object({ login: z.string().min(1) }).passthrough().optional(), + sender: z + .object({ login: z.string().min(1) }) + .passthrough() + .optional(), }) .passthrough() .transform((provider) => diff --git a/packages/junior-github/src/webhooks/pull-request-outcome.ts b/packages/junior-github/src/webhooks/pull-request-outcome.ts index 2fe0cd8c3d..19f626537f 100644 --- a/packages/junior-github/src/webhooks/pull-request-outcome.ts +++ b/packages/junior-github/src/webhooks/pull-request-outcome.ts @@ -110,9 +110,7 @@ const pullRequestConversationSchema = z user: z.object({ login: z.string().min(1) }).passthrough(), }) .passthrough(), - repository: z - .object({ full_name: z.string().min(1) }) - .passthrough(), + repository: z.object({ full_name: z.string().min(1) }).passthrough(), sender: z.object({ login: z.string().min(1) }).passthrough(), }) .passthrough() diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index de0adcf3cf..84f910462d 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -146,6 +146,22 @@ const pluginLog = { warn() {}, }; +/** In-memory plugin cache with the same load and TTL rules as Junior's. */ +function pluginCache() { + const entries = new Map(); + return { + async getOrLoad( + key: string, + load: () => Promise<{ ttlMs?: number; value: T }>, + ): Promise { + if (entries.has(key)) return entries.get(key) as T; + const entry = await load(); + if (entry.ttlMs) entries.set(key, entry.value); + return entry.value; + }, + }; +} + /** * Recover the exact prepare-commit-msg script sandboxPrepare writes and stage * it with a seeded commit message file, so tests execute the real generated @@ -234,9 +250,37 @@ function cloneStateValue(value: T): T { return value === undefined ? value : structuredClone(value); } +/** + * Mock the App owner `getsentry` (installation 456), `fairsource` repos + * (installation 789), and repositories under `uninstalled` without the App. + */ function mockGitHubInstallationApi(): CapturedRequest[] { const requests: CapturedRequest[] = []; + const installationFor = (owner: unknown) => + owner === "uninstalled" + ? HttpResponse.json({ message: "Not Found" }, { status: 404 }) + : HttpResponse.json({ id: owner === "fairsource" ? 789 : 456 }); mswServer.use( + http.get("https://api.github.com/app", async ({ request }) => { + requests.push(await captureRequest(request)); + return HttpResponse.json({ + owner: { login: "getsentry", type: "Organization" }, + }); + }), + http.get( + "https://api.github.com/repos/:owner/:repo/installation", + async ({ params, request }) => { + requests.push(await captureRequest(request)); + return installationFor(params.owner); + }, + ), + http.get( + "https://api.github.com/orgs/:org/installation", + async ({ params, request }) => { + requests.push(await captureRequest(request)); + return installationFor(params.org); + }, + ), http.get( "https://api.github.com/app/installations/:installationId", async ({ request }) => { @@ -747,8 +791,18 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-read", access: "read", + leaseScope: "repository:getsentry/junior", reason: "github.api-read", }); + expect( + await grantForEgress({ + method: "GET", + url: "https://api.github.com/orgs/FairSource/teams", + }), + ).toMatchObject({ + name: "installation-read", + leaseScope: "organization:fairsource", + }); expect( await grantForEgress({ method: "POST", @@ -2245,6 +2299,21 @@ Conversation: \`local:test:old-conversation\` }); it("selects installation-read for GitHub GraphQL read operations", async () => { + expect( + await grantForEgress({ + method: "POST", + url: "https://api.github.com/graphql", + bodyText: JSON.stringify({ + query: + "query PullRequest($owner: String!, $repo: String!) { repository(owner: $owner, name: $repo) { pullRequest(number: 1) { title } } }", + variables: { owner: "FairSource", repo: "fair.io" }, + }), + }), + ).toMatchObject({ + name: "installation-read", + leaseScope: "repository:fairsource/fair.io", + reason: "github.graphql-read", + }); expect( await grantForEgress({ method: "POST", @@ -2673,12 +2742,11 @@ Conversation: \`local:test:old-conversation\` ); }); - it("issues installation-write credentials without repository filter", async () => { + it("issues installation-write credentials from the target repository's installation", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); process.env.GITHUB_APP_ID = "123"; - process.env.GITHUB_INSTALLATION_ID = "456"; process.env.GITHUB_APP_PRIVATE_KEY = privateKey; const requests = mockGitHubInstallationApi(); const plugin = githubPlugin({ @@ -2686,27 +2754,33 @@ Conversation: \`local:test:old-conversation\` contents: "write", }, }); - - const result = await plugin.hooks?.issueCredential?.({ - actor: { platform: "system", name: "event" }, - grant: { - name: "installation-write", - access: "write", - reason: "github.installation-write", - }, - db, - log: pluginLog, - plugin: { name: "github" }, - tokens: {}, + const grant = await grantForEgress({ + method: "GET", + url: "https://github.com/fairsource/fair.io.git/info/refs?service=git-receive-pack", }); + const issue = async (leaseScope = grant?.leaseScope) => + await plugin.hooks?.issueCredential?.({ + actor: { platform: "system", name: "event" }, + cache: pluginCache(), + grant: { ...grant!, leaseScope }, + db, + log: pluginLog, + plugin: { name: "github" }, + tokens: {}, + }); + + const result = await issue(); expect(result?.type).toBe("lease"); - expect(requests).toHaveLength(1); - expect(requests[0]).toEqual({ - url: "https://api.github.com/app/installations/456/access_tokens", - method: "POST", - body: {}, - headers: expect.any(Object), + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "GET https://api.github.com/repos/fairsource/fair.io/installation", + "POST https://api.github.com/app/installations/789/access_tokens", + ]); + // Write tokens keep the full installed App permissions and repos. + expect(requests[1]?.body).toEqual({}); + await expect(issue("repository:uninstalled/repo")).resolves.toEqual({ + type: "unavailable", + message: "The GitHub App is not installed on uninstalled/repo.", }); }); @@ -2715,7 +2789,6 @@ Conversation: \`local:test:old-conversation\` .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); process.env.GITHUB_APP_ID = "123"; - process.env.GITHUB_INSTALLATION_ID = "456"; process.env.GITHUB_APP_PRIVATE_KEY = privateKey; const requests = mockGitHubInstallationApi(); @@ -2726,18 +2799,23 @@ Conversation: \`local:test:old-conversation\` issues: "write", }, }); - const result = await plugin.hooks?.issueCredential?.({ - actor: { platform: "system", name: "scheduler" }, - grant: { - name: "installation-read", - access: "read", - reason: "github.api-read", - }, - db, - log: pluginLog, - plugin: { name: "github" }, - tokens: {}, - }); + const issue = async (leaseScope?: string) => + await plugin.hooks?.issueCredential?.({ + actor: { platform: "system", name: "scheduler" }, + cache: pluginCache(), + grant: { + name: "installation-read", + access: "read", + reason: "github.api-read", + ...(leaseScope ? { leaseScope } : undefined), + }, + db, + log: pluginLog, + plugin: { name: "github" }, + tokens: {}, + }); + + const result = await issue(); expect(result).toMatchObject({ type: "lease", @@ -2753,10 +2831,13 @@ Conversation: \`local:test:old-conversation\` ], }, }); - expect(requests).toHaveLength(1); - expect(requests[0]).toMatchObject({ - method: "POST", - url: "https://api.github.com/app/installations/456/access_tokens", + // A read without a target uses the App owner's installation. + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "GET https://api.github.com/app", + "GET https://api.github.com/orgs/getsentry/installation", + "POST https://api.github.com/app/installations/456/access_tokens", + ]); + expect(requests[2]).toMatchObject({ body: { permissions: { contents: "read", @@ -2765,6 +2846,18 @@ Conversation: \`local:test:old-conversation\` }, }, }); + + // Public repositories stay readable where the App is not installed. + requests.length = 0; + await expect(issue("repository:uninstalled/repo")).resolves.toMatchObject({ + type: "lease", + }); + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "GET https://api.github.com/repos/uninstalled/repo/installation", + "GET https://api.github.com/app", + "GET https://api.github.com/orgs/getsentry/installation", + "POST https://api.github.com/app/installations/456/access_tokens", + ]); }); it("caches implicit GitHub App installation permissions", async () => { @@ -2772,7 +2865,6 @@ Conversation: \`local:test:old-conversation\` .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); process.env.GITHUB_APP_ID = "123"; - process.env.GITHUB_INSTALLATION_ID = "456"; process.env.GITHUB_APP_PRIVATE_KEY = privateKey; const requests = mockGitHubInstallationApi(); @@ -2780,9 +2872,11 @@ Conversation: \`local:test:old-conversation\` const plugin = githubPlugin(); const ctx = { actor: { platform: "system" as const, name: "scheduler" }, + cache: pluginCache(), grant: { name: "installation-read", access: "read" as const, + leaseScope: "repository:getsentry/junior", reason: "github.api-read", }, db, @@ -2794,15 +2888,16 @@ Conversation: \`local:test:old-conversation\` await plugin.hooks?.issueCredential?.(ctx); await plugin.hooks?.issueCredential?.(ctx); - const permissionReads = requests.filter( + const lookups = requests.filter( (request) => request.method === "GET" && - request.url === "https://api.github.com/app/installations/456", + (request.url === "https://api.github.com/app/installations/456" || + request.url.endsWith("/installation")), ); const tokenRequests = requests.filter((request) => request.url.endsWith("/app/installations/456/access_tokens"), ); - expect(permissionReads).toHaveLength(1); + expect(lookups).toHaveLength(2); expect(tokenRequests).toHaveLength(2); expect(tokenRequests.map((request) => request.body)).toEqual([ { diff --git a/packages/junior-plugin-api/src/credentials.ts b/packages/junior-plugin-api/src/credentials.ts index 4274564957..62b4d0e302 100644 --- a/packages/junior-plugin-api/src/credentials.ts +++ b/packages/junior-plugin-api/src/credentials.ts @@ -51,6 +51,7 @@ export const pluginStoredTokensSchema = z export const pluginGrantSchema = z .object({ access: pluginGrantAccessSchema, + /** Plugin-defined target, such as one repository. Junior remembers shared installation leases per scope. */ leaseScope: nonBlankStringSchema.optional(), name: pluginGrantNameSchema, reason: nonBlankStringSchema.optional(), diff --git a/packages/junior/.env.example b/packages/junior/.env.example index 633bd2e0e9..3908c1056d 100644 --- a/packages/junior/.env.example +++ b/packages/junior/.env.example @@ -18,5 +18,4 @@ REDIS_URL= SKILL_DIRS= # Additional skill directories (colon-separated) GITHUB_APP_ID= GITHUB_APP_PRIVATE_KEY= -GITHUB_INSTALLATION_ID= SENTRY_DSN= diff --git a/packages/junior/src/chat/sandbox/egress/session.ts b/packages/junior/src/chat/sandbox/egress/session.ts index 9ec867a4d9..94bedeb15b 100644 --- a/packages/junior/src/chat/sandbox/egress/session.ts +++ b/packages/junior/src/chat/sandbox/egress/session.ts @@ -52,13 +52,14 @@ function isSharedInstallationGrant( * Build the host key for a shared installation grant. * * Only installation grants are remembered. User and other grants issue live - * headers each time, so they never write a host key. + * headers each time, so they never write a host key. Grants with different + * lease scopes never share remembered headers. */ function leaseKey( provider: string, grant: SandboxEgressCredentialLease["grant"], ): string { - return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:shared`; + return `${SANDBOX_EGRESS_LEASE_PREFIX}:${provider}:${grant.name}:${grant.leaseScope ?? "shared"}`; } /** diff --git a/packages/junior/tests/component/events/events.test.ts b/packages/junior/tests/component/events/events.test.ts index 31ae08737e..c552805c0d 100644 --- a/packages/junior/tests/component/events/events.test.ts +++ b/packages/junior/tests/component/events/events.test.ts @@ -260,7 +260,6 @@ describe("event delivery", () => { }); it("accepts plugin-owned GitHub webhooks through the core delivery bridge", async () => { - const previousInstallationId = process.env.GITHUB_INSTALLATION_ID; const previousSecret = process.env.GITHUB_WEBHOOK_SECRET; const previousSlackBotToken = process.env.SLACK_BOT_TOKEN; const previousPlugins = getPlugins(); @@ -274,7 +273,6 @@ describe("event delivery", () => { let eventAutomationId: string | undefined; try { - process.env.GITHUB_INSTALLATION_ID = "456"; process.env.GITHUB_WEBHOOK_SECRET = "test-secret"; process.env.SLACK_BOT_TOKEN = "xoxb-event-test"; const state = createMemoryState(); @@ -427,11 +425,6 @@ describe("event delivery", () => { } else { process.env.GITHUB_WEBHOOK_SECRET = previousSecret; } - if (previousInstallationId === undefined) { - delete process.env.GITHUB_INSTALLATION_ID; - } else { - process.env.GITHUB_INSTALLATION_ID = previousInstallationId; - } if (previousSlackBotToken === undefined) { delete process.env.SLACK_BOT_TOKEN; } else { diff --git a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts index 0f15fa9f4e..cd193023b0 100644 --- a/packages/junior/tests/integration/sandbox-egress-proxy.test.ts +++ b/packages/junior/tests/integration/sandbox-egress-proxy.test.ts @@ -249,7 +249,6 @@ async function registerGitHubPlugin( function configureGitHubAppEnv() { process.env.GITHUB_APP_ID = "123"; - process.env.GITHUB_INSTALLATION_ID = "456"; process.env.GITHUB_APP_PRIVATE_KEY = generateKeyPairSync("rsa", { modulusLength: 2048, }) @@ -1721,7 +1720,6 @@ describe("sandbox egress proxy integration", () => { it("returns a controlled egress auth response when GitHub App setup is unavailable", async () => { delete process.env.GITHUB_APP_ID; delete process.env.GITHUB_APP_PRIVATE_KEY; - delete process.env.GITHUB_INSTALLATION_ID; await registerGitHubPlugin(); const credentialToken = modules.session.createSandboxEgressCredentialToken({ credentials: { actor: { type: "user", userId: ACTOR_ID } }, diff --git a/packages/junior/tests/msw/handlers/github-api.ts b/packages/junior/tests/msw/handlers/github-api.ts index 4cda3694e3..206eef54b2 100644 --- a/packages/junior/tests/msw/handlers/github-api.ts +++ b/packages/junior/tests/msw/handlers/github-api.ts @@ -9,6 +9,19 @@ export const githubApiHandlers = [ http.get(`${GITHUB_API_ORIGIN}/repos/:owner/:repo/deployments`, () => HttpResponse.json([]), ), + // The GitHub plugin finds the App installation for the target repository, + // organization, or App owner before it mints a token. + http.get(`${GITHUB_API_ORIGIN}/repos/:owner/:repo/installation`, () => + HttpResponse.json({ id: 456 }), + ), + http.get(`${GITHUB_API_ORIGIN}/orgs/:org/installation`, () => + HttpResponse.json({ id: 456 }), + ), + http.get(`${GITHUB_API_ORIGIN}/app`, () => + HttpResponse.json({ + owner: { login: "getsentry", type: "Organization" }, + }), + ), // The GitHub plugin reads the installation's permissions before it mints a // token, unless the host declares `appPermissions`. http.get(`${GITHUB_API_ORIGIN}/app/installations/:installationId`, () => diff --git a/packages/junior/tests/unit/plugins/plugin-manifest-api-headers.test.ts b/packages/junior/tests/unit/plugins/plugin-manifest-api-headers.test.ts index 348ef4a809..bab83ece22 100644 --- a/packages/junior/tests/unit/plugins/plugin-manifest-api-headers.test.ts +++ b/packages/junior/tests/unit/plugins/plugin-manifest-api-headers.test.ts @@ -131,7 +131,6 @@ describe("plugin manifest API headers", () => { GITHUB_APP_CLIENT_SECRET: {}, GITHUB_APP_ID: {}, GITHUB_APP_PRIVATE_KEY: {}, - GITHUB_INSTALLATION_ID: {}, GITHUB_APP_BOT_NAME: { exposeToCommandEnv: true }, GITHUB_APP_BOT_EMAIL: { exposeToCommandEnv: true }, }); From a2943cb05e8b24c315366d99944efa457fa98dd5 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 03:01:05 +0000 Subject: [PATCH 2/7] fix(github): Use each delivery's installation in webhook tests Drop the unused skip log and update webhook tests for the removed installation-ID route options. Co-Authored-By: David Cramer --- packages/junior-github/src/webhooks/handler.ts | 7 +------ .../junior-github/tests/webhook-outcomes.test.ts | 14 +++++--------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index b933a501a2..c5351de87f 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -170,7 +170,7 @@ export function createGitHubWebhookRoute(args: { ): Promise; /** Linked GitHub login of the user of one Junior identity, if any. */ loadGitHubLogin?(identityId: string): Promise; - log?: Pick; + log?: Pick; markFeedbackReviewing( input: InstallationScoped, ): Promise; @@ -198,11 +198,6 @@ export function createGitHubWebhookRoute(args: { // installations. Every token for this delivery uses that installation. const installationId = webhookInstallationId(body); if (!installationId) { - args.log?.info("github.webhook.delivery.skipped", { - "app.github.delivery_id": deliveryId, - "app.github.event_name": eventName, - "app.github.skip_reason": "missing_installation", - }); return new Response("Ignored", { status: 202 }); } const botEmail = args.botEmail(); diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index f25bbe154a..fc5f19bf8a 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -296,8 +296,6 @@ function webhookRoute( async record() {}, }, db: fixture.db(), - installationId: () => "456", - installationIdEnv: "GITHUB_INSTALLATION_ID", log: { error }, markFeedbackReviewing, privateKeyEnv: "GITHUB_APP_PRIVATE_KEY", @@ -1908,6 +1906,8 @@ describe("GitHub-owned pull request outcomes", () => { signedRequest( { action: "created", + // A delivery from any installation of the App uses that installation. + installation: { id: 999 }, repository: { full_name: "getsentry/junior" }, issue: { number: 946, @@ -1927,6 +1927,7 @@ describe("GitHub-owned pull request outcomes", () => { expect(markFeedbackReviewing).toHaveBeenCalledWith({ commentId: 101, commentKind: "conversation", + installationId: 999, repo: "getsentry/junior", }); expect(published).toHaveLength(2); @@ -2055,7 +2056,7 @@ describe("GitHub-owned pull request outcomes", () => { } }); - it("ignores signed deliveries from another installation", async () => { + it("ignores signed deliveries without an installation", async () => { const fixture = await createGitHubFixture(); const published: EventInput[] = []; try { @@ -2063,7 +2064,7 @@ describe("GitHub-owned pull request outcomes", () => { const response = await route.handler( signedRequest({ ...pullRequestPayload(), - installation: { id: 999 }, + installation: undefined, }), ); @@ -2130,7 +2131,6 @@ describe("GitHub-owned pull request outcomes", () => { "CUSTOM_GITHUB_BOT_EMAIL", "264270552+sentry-junior[bot]@users.noreply.github.com", ); - vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); vi.stubEnv("GITHUB_WEBHOOK_SECRET", "test-secret"); try { const plugin = githubPlugin({ @@ -2190,7 +2190,6 @@ describe("GitHub-owned pull request outcomes", () => { .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); vi.stubEnv("GITHUB_APP_ID", "123"); - vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); vi.stubEnv("GITHUB_APP_PRIVATE_KEY", privateKey); vi.stubEnv( "GITHUB_APP_BOT_EMAIL", @@ -2294,7 +2293,6 @@ describe("GitHub-owned pull request outcomes", () => { .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); vi.stubEnv("GITHUB_APP_ID", "123"); - vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); vi.stubEnv("GITHUB_APP_PRIVATE_KEY", privateKey); vi.stubEnv( "GITHUB_APP_BOT_EMAIL", @@ -2419,7 +2417,6 @@ describe("GitHub-owned pull request outcomes", () => { .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); vi.stubEnv("GITHUB_APP_ID", "123"); - vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); vi.stubEnv("GITHUB_APP_PRIVATE_KEY", privateKey); vi.stubEnv( "GITHUB_APP_BOT_EMAIL", @@ -2558,7 +2555,6 @@ describe("GitHub-owned pull request outcomes", () => { .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); vi.stubEnv("GITHUB_APP_ID", "123"); - vi.stubEnv("GITHUB_INSTALLATION_ID", "456"); vi.stubEnv("GITHUB_APP_PRIVATE_KEY", privateKey); vi.stubEnv( "GITHUB_APP_BOT_EMAIL", From 902f79fa94741b164957b64833a33fd55d753bdb Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:39:47 +0000 Subject: [PATCH 3/7] fix(github): Route installation tokens by account owner An App has at most one installation per account, so the owner of the target repository or organization identifies the installation. Load the App's installations once, cache them, and reload them on installation webhooks. Lease scopes now name the owner, so one token serves every repository of that account. Co-Authored-By: David Cramer --- .../src/content/docs/extend/github-plugin.md | 21 ++- packages/junior-github/SETUP.md | 11 +- .../junior-github/src/credential-support.ts | 149 ++++++++++-------- packages/junior-github/src/egress-policy.ts | 81 +++++----- .../junior-github/src/installation-targets.ts | 44 ++---- packages/junior-github/src/plugin.ts | 4 + .../resolve-pull-request-review-thread.ts | 2 +- .../junior-github/src/webhooks/handler.ts | 7 + .../junior-github/tests/github-plugin.test.ts | 93 ++++++----- ...resolve-pull-request-review-thread.test.ts | 2 +- .../tests/webhook-outcomes.test.ts | 36 +++++ .../junior/tests/msw/handlers/github-api.ts | 17 +- 12 files changed, 270 insertions(+), 197 deletions(-) diff --git a/packages/docs/src/content/docs/extend/github-plugin.md b/packages/docs/src/content/docs/extend/github-plugin.md index d4c7df0d79..8ac58c1d93 100644 --- a/packages/docs/src/content/docs/extend/github-plugin.md +++ b/packages/docs/src/content/docs/extend/github-plugin.md @@ -86,11 +86,18 @@ Install the App only on repositories where Junior may push. Protect important branches in GitHub. Across many repositories, users should include `owner/repo` when the target is not obvious. -One deployment works with every installation of the App. Junior finds the -installation for each target repository or organization. A webhook delivery -uses the installation that sent it. Reads that name no repository, such as -search or GraphQL without `owner` and `repo` variables, use the installation on -the account that owns the App. Install the App on its owner account too. +One deployment works with every installation of the App. An App has at most +one installation per account, so Junior uses the installation on the account +that owns the target repository or organization. Junior keeps a list of the +App's installations for 10 minutes. `installation` and `installation_target` +webhooks reload the list. A webhook delivery uses the installation that sent +it. + +Reads that name no owner, such as search or GraphQL without an `owner` +variable, use the installation on the account that owns the App. Reads of +accounts without the App use that installation too, so public repositories stay +readable. Writes never use another account's installation. Install the App on +its owner account too. ## Config @@ -547,8 +554,8 @@ A local `git commit` does not call GitHub. The write happens on push. Grant `GITHUB_WEBHOOK_SECRET`, or `X-Hub-Signature-256` is missing. - Webhook delivery returns `202 Ignored`: the payload has no App installation, or the event mapping is unsupported. Confirm the event type. -- `The GitHub App is not installed on owner/repo`: install the App on that - repository or its organization, and retry. +- `The GitHub App is not installed on owner`: install the App on that account, + and retry. - Delivery succeeds but nothing appears in Slack: create a watch or event automation first. A webhook alone does not create either one. - Missing repository context: include `owner/repo`, or set a thread default diff --git a/packages/junior-github/SETUP.md b/packages/junior-github/SETUP.md index cd21000625..fbbab7e757 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -45,9 +45,12 @@ Set on the harness host (never in skill files): - `GITHUB_APP_BOT_NAME` (Git author and committer display name) - `GITHUB_APP_BOT_EMAIL` -One Junior deployment works with every installation of the App. Junior finds -the installation for each target repository or organization. Reads that name no -repository use the installation on the account that owns the App. +One Junior deployment works with every installation of the App. Junior uses the +installation on the account that owns the target repository or organization. +It keeps a list of the App's installations for 10 minutes, and `installation` +and `installation_target` webhooks reload it. Reads that name no owner, or an +account without the App, use the installation on the account that owns the App. +Writes to an account without the App fail. ### Vercel env setup (multiline-safe) @@ -160,7 +163,7 @@ Use `additionalUserScopes` only when a human-identity integration flow requires - `user-read` and explicitly human `user-write` operations require the actor, or an explicitly delegated user subject, to authorize the GitHub App through the private OAuth flow. Junior-owned issue, pull request, review, inline review comment, and branch operations do not fall back to user OAuth. - Headless event turns use the `event` system actor and may receive the same installation grants. This lets Junior respond to subscribed pull request events by committing and pushing fixes without inheriting a subscriber's OAuth credential. - Git commits use Junior as author and committer. Resolvable human run actors are credited once with `Co-Authored-By` trailers. When the current actor has a linked GitHub identity, Junior prefers a GitHub noreply address so external automation can resolve a login for assignment. -- Installation credential leases are cached on the host by grant name and reused across sandboxes until near expiry. User grants stay actor-scoped. Upstream 403 after injection clears the cached lease, issues a new token, and retries the hop once before recording permission denied. +- Installation credential leases are cached on the host by grant name and target account, and reused across sandboxes until near expiry. User grants stay actor-scoped. Upstream 403 after injection clears the cached lease, issues a new token, and retries the hop once before recording permission denied. - Sandbox does not receive raw tokens via env; host applies Authorization header transforms for GitHub API and upload calls. ## 4) CLI usage diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 1030f5b9aa..710b7e9189 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -14,7 +14,7 @@ import type { PluginUserTokenSlot, IssueCredentialHookContext, } from "@sentry/junior-plugin-api"; -import { installationLookupForLeaseScope } from "./installation-targets.js"; +import { ownerFromLeaseScope } from "./installation-targets.js"; import { type GitHubAppPermissions, readGrantPermissions, @@ -91,8 +91,9 @@ export const GITHUB_APP_PRIVATE_KEY_ENV = "GITHUB_APP_PRIVATE_KEY"; export const GITHUB_AUTH_TOKEN_ENV = "GITHUB_TOKEN"; export const GITHUB_AUTH_TOKEN_PLACEHOLDER = "ghp_host_managed_credential"; const MAX_LEASE_MS = 60 * 60 * 1000; -// Installs change rarely. A reinstalled App can keep a stale id this long. -const INSTALLATION_ID_TTL_MS = 10 * 60 * 1000; +// Installs change rarely, and `installation` webhooks refresh the map sooner. +const INSTALLATIONS_TTL_MS = 10 * 60 * 1000; +const INSTALLATIONS_PAGE_SIZE = 100; const GITHUB_API = "https://api.github.com"; const REFRESH_BUFFER_MS = 5 * 60 * 1000; const USER_REFRESH_TIMEOUT_MS = 20_000; @@ -804,88 +805,102 @@ export async function issueInstallationToken( ); } -/** Read one App installation id, or undefined when the App is not installed. */ -async function findInstallationId( +/** Every installation of the App, by lowercase account login. */ +export interface GitHubInstallations { + /** Lowercase login of the account that owns the App. */ + appOwner: string; + byOwner: Record; +} + +function readLogin(value: unknown): string | undefined { + const login = isRecord(value) ? value.login : undefined; + return typeof login === "string" && login ? login.toLowerCase() : undefined; +} + +/** + * Load every installation of the App. An App can be installed at most once + * per account, so the account login identifies the installation. Set + * `refresh` to reload the cached map now. + */ +export async function loadInstallations( + app: GitHubAppEnv, cache: PluginCache, - appJwt: string, - path: string, -): Promise { - const id = await cache.getOrLoad( - `installation-id:${path}`, + options?: { refresh?: boolean }, +): Promise { + return await cache.getOrLoad( + "installations", async () => { - let installation: unknown; - try { - installation = await githubRequest(GITHUB_API, path, { token: appJwt }); - } catch (error) { - if (error instanceof GitHubRequestError && error.status === 404) { - // Do not cache: the App can be installed at any time. - return { value: null }; - } - throw error; + const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); + const appResponse = await githubRequest(GITHUB_API, "/app", { + token: appJwt, + }); + const appOwner = readLogin( + isRecord(appResponse) ? appResponse.owner : undefined, + ); + if (!appOwner) { + throw new Error("GitHub App response missing owner"); } - const value = isRecord(installation) ? installation.id : undefined; - if (typeof value !== "number" || !Number.isSafeInteger(value)) { - throw new Error("GitHub installation response missing id"); + const byOwner: Record = {}; + for (let page = 1; ; page += 1) { + const installations = await githubRequest( + GITHUB_API, + `/app/installations?per_page=${INSTALLATIONS_PAGE_SIZE}&page=${page}`, + { token: appJwt }, + ); + if (!Array.isArray(installations)) { + throw new Error("GitHub installations response is not a list"); + } + for (const installation of installations) { + const login = readLogin( + isRecord(installation) ? installation.account : undefined, + ); + const id = isRecord(installation) ? installation.id : undefined; + if (login && typeof id === "number" && Number.isSafeInteger(id)) { + byOwner[login] = id; + } + } + if (installations.length < INSTALLATIONS_PAGE_SIZE) break; } - return { value, ttlMs: INSTALLATION_ID_TTL_MS }; + return { value: { appOwner, byOwner }, ttlMs: INSTALLATIONS_TTL_MS }; }, + options, ); - return id ?? undefined; -} - -/** Read the installation on the account that owns the App. */ -async function appOwnerInstallationId( - cache: PluginCache, - appJwt: string, -): Promise { - const owner = await cache.getOrLoad("app-owner", async () => { - const app = await githubRequest(GITHUB_API, "/app", { token: appJwt }); - const value = isRecord(app) && isRecord(app.owner) ? app.owner : undefined; - if (typeof value?.login !== "string" || !value.login) { - throw new Error("GitHub App response missing owner"); - } - return { - value: { - login: value.login, - organization: value.type === "Organization", - }, - ttlMs: INSTALLATION_ID_TTL_MS, - }; - }); - const id = await findInstallationId( - cache, - appJwt, - `/${owner.organization ? "orgs" : "users"}/${encodeURIComponent(owner.login)}/installation`, - ); - if (!id) { - throw new GitHubPluginSetupError( - `The GitHub App is not installed on ${owner.login}.`, - ); - } - return id; } /** - * Resolve the App installation for an installation grant from its lease - * scope. Writes need the target's own installation. Reads without one, or - * without a target, use the App owner's installation, which can still read - * public repositories. + * Resolve the App installation for an installation grant from the owner in + * its lease scope. Writes need the owner's own installation. Reads without + * one, or without an owner, use the App owner's installation, which can still + * read public repositories. */ async function resolveInstallationId( ctx: IssueCredentialHookContext, - appJwt: string, + app: GitHubAppEnv, ): Promise { - const lookup = installationLookupForLeaseScope(ctx.grant.leaseScope); - if (lookup) { - const id = await findInstallationId(ctx.cache, appJwt, lookup.path); + const owner = ownerFromLeaseScope(ctx.grant.leaseScope); + let installations = await loadInstallations(app, ctx.cache); + if (owner) { + const id = installations.byOwner[owner]; if (id) return id; if (ctx.grant.access === "write") { + // The App can be installed before its `installation` webhook arrives. + installations = await loadInstallations(app, ctx.cache, { + refresh: true, + }); + const refreshedId = installations.byOwner[owner]; + if (refreshedId) return refreshedId; throw new GitHubPluginSetupError( - `The GitHub App is not installed on ${lookup.label}.`, + `The GitHub App is not installed on ${owner}.`, ); } } - return await appOwnerInstallationId(ctx.cache, appJwt); + const defaultId = installations.byOwner[installations.appOwner]; + if (!defaultId) { + throw new GitHubPluginSetupError( + `The GitHub App is not installed on ${installations.appOwner}, the account that owns it.`, + ); + } + return defaultId; } /** Read an installation's permissions as read grants, cached per lease period. */ @@ -930,11 +945,11 @@ export async function issueGitHubCredential( ctx.grant.name === "installation-read" || ctx.grant.name === "installation-write" ) { + const installationId = await resolveInstallationId(ctx, options); const appJwt = createAppJwt( requireEnv(options.appIdEnv), options.privateKeyEnv, ); - const installationId = await resolveInstallationId(ctx, appJwt); // Write tokens use the full installed App permissions and repos. Repo // allowlisting stays in egress policy, not in per-request token minting. const permissions = diff --git a/packages/junior-github/src/egress-policy.ts b/packages/junior-github/src/egress-policy.ts index 4d2f9e88de..9e7d6722eb 100644 --- a/packages/junior-github/src/egress-policy.ts +++ b/packages/junior-github/src/egress-policy.ts @@ -23,9 +23,8 @@ import { type GitHubGrantReason, } from "./credential-support.js"; import { - githubInstallationLeaseScope, + githubOwnerLeaseScope, githubRepositoryFromUrl, - type GitHubInstallationTarget, } from "./installation-targets.js"; function githubSmartHttpAccess( @@ -592,63 +591,64 @@ function grantForAccess( access: PluginGrantAccess, reason: GitHubGrantReason, name: GitHubGrantName, - target?: GitHubInstallationTarget, + owner?: string, ): GitHubGrant { return { name, access, reason, - ...(target - ? { leaseScope: githubInstallationLeaseScope(target) } - : undefined), + ...(owner ? { leaseScope: githubOwnerLeaseScope(owner) } : undefined), ...(name === "user-write" ? { requirements: USER_WRITE_REQUIREMENTS } : undefined), }; } -const GITHUB_NAME_PATTERN = /^[\w.-]+$/; +const GITHUB_LOGIN_PATTERN = /^[\w.-]+$/; +// Host tools name their repository at the end of the operation, such as +// `github.pull.review-thread.get:owner/repo`. +const HOST_OPERATION_REPOSITORY_PATTERN = + /^github\.[\w.-]+:([\w.-]+)\/[\w.-]+$/; -/** Read the repository from GraphQL `owner` and `repo` or `name` variables, as `gh` sends them. */ -function graphqlVariablesRepository( +/** Read the GraphQL `owner` variable, as `gh` sends it for repository reads. */ +function graphqlOwnerVariable( bodyText: string | undefined, -): GitHubInstallationTarget | undefined { +): string | undefined { let parsed: unknown; try { parsed = JSON.parse(bodyText ?? ""); } catch { return undefined; } - const variables = isRecord(parsed) ? parsed.variables : undefined; - if (!isRecord(variables)) return undefined; - const owner = variables.owner; - const name = variables.repo ?? variables.name; - return typeof owner === "string" && - typeof name === "string" && - GITHUB_NAME_PATTERN.test(owner) && - GITHUB_NAME_PATTERN.test(name) - ? { owner, name } + const owner = + isRecord(parsed) && isRecord(parsed.variables) + ? parsed.variables.owner + : undefined; + return typeof owner === "string" && GITHUB_LOGIN_PATTERN.test(owner) + ? owner : undefined; } /** - * Find the repository or organization whose App installation a request - * needs. Return undefined when the request names neither. + * Find the account whose App installation a request needs: the repository + * or organization in the URL, the `owner/repo` that a host tool puts at the + * end of its operation, or the GraphQL `owner` variable. Return undefined + * when the request names no owner. */ -function installationTarget( +function installationOwner( upstreamUrl: URL, - bodyText: string | undefined, -): GitHubInstallationTarget | undefined { - const repository = - githubRepositoryFromUrl(upstreamUrl) ?? + request: EgressHookContext["request"], +): string | undefined { + return ( + githubRepositoryFromUrl(upstreamUrl)?.owner ?? + (isGitHubApiUrl(upstreamUrl) + ? /^\/orgs\/([\w.-]+)(?:\/|$)/i.exec(upstreamUrl.pathname)?.[1] + : undefined) ?? + HOST_OPERATION_REPOSITORY_PATTERN.exec(request.operation ?? "")?.[1] ?? (isGitHubGraphqlUrl(upstreamUrl) - ? graphqlVariablesRepository(bodyText) - : undefined); - if (repository) return repository; - const org = isGitHubApiUrl(upstreamUrl) - ? /^\/orgs\/([\w.-]+)(?:\/|$)/i.exec(upstreamUrl.pathname)?.[1] - : undefined; - return org ? { org } : undefined; + ? graphqlOwnerVariable(request.bodyText) + : undefined) + ); } function requireRepositoryTarget(upstreamUrl: URL): void { @@ -678,7 +678,7 @@ export async function githubGrantForEgress( if (isGitHubAssetUploadRequest(method, upstreamUrl)) { return grantForAccess("write", "github.asset-upload", "user-write"); } - const target = installationTarget(upstreamUrl, ctx.request.bodyText); + const owner = installationOwner(upstreamUrl, ctx.request); const smartHttpAccess = githubSmartHttpAccess(upstreamUrl); if (smartHttpAccess) { @@ -688,14 +688,14 @@ export async function githubGrantForEgress( "write", "github.installation-write", "installation-write", - target, + owner, ); } return grantForAccess( smartHttpAccess, "github.git-read", "installation-read", - target, + owner, ); } @@ -715,7 +715,7 @@ export async function githubGrantForEgress( ? "github.user-write" : "github.installation-write", writeGrantName, - writeGrantName === "installation-write" ? target : undefined, + writeGrantName === "installation-write" ? owner : undefined, ); } @@ -726,12 +726,11 @@ export async function githubGrantForEgress( ctx.request.bodyText, ); if (hostMutationRepository) { - const [owner = "", name = ""] = hostMutationRepository.split("/"); return grantForAccess( "write", "github.installation-write", "installation-write", - { owner, name }, + hostMutationRepository.split("/")[0], ); } @@ -750,7 +749,7 @@ export async function githubGrantForEgress( graphqlAccess, "github.graphql-read", "installation-read", - target, + owner, ); } @@ -760,5 +759,5 @@ export async function githubGrantForEgress( "This GitHub write request is not supported. Use the matching GitHub tool or a supported REST endpoint.", ); } - return grantForAccess(access, "github.api-read", "installation-read", target); + return grantForAccess(access, "github.api-read", "installation-read", owner); } diff --git a/packages/junior-github/src/installation-targets.ts b/packages/junior-github/src/installation-targets.ts index 3ab782a83e..546df9b799 100644 --- a/packages/junior-github/src/installation-targets.ts +++ b/packages/junior-github/src/installation-targets.ts @@ -1,15 +1,12 @@ /** * GitHub App installation targets. * - * Owns how a GitHub request names the repository or organization whose App - * installation it needs, and how a grant lease scope carries that target to - * credential issuance. + * An App can be installed at most once per account, so the account that owns + * a repository identifies the installation a request needs. This module owns + * how a grant lease scope carries that owner to credential issuance. */ -/** Repository or organization whose App installation a grant uses. */ -export type GitHubInstallationTarget = - | { name: string; owner: string } - | { org: string }; +const OWNER_LEASE_SCOPE_PREFIX = "owner:"; function decodeGitHubPathSegment(value: string): string | undefined { try { @@ -42,33 +39,16 @@ export function githubRepositoryFromUrl( return owner && name ? { owner, name } : undefined; } -/** Build the grant lease scope that keeps each target's installation lease apart. */ -export function githubInstallationLeaseScope( - target: GitHubInstallationTarget, -): string { - return ( - "org" in target - ? `organization:${target.org}` - : `repository:${target.owner}/${target.name}` - ).toLowerCase(); +/** Build the grant lease scope that keeps each owner's installation lease apart. */ +export function githubOwnerLeaseScope(owner: string): string { + return `${OWNER_LEASE_SCOPE_PREFIX}${owner.toLowerCase()}`; } -/** - * Return the GitHub API path that finds the App installation for a lease - * scope, and the target name for errors. Return undefined for no target. - */ -export function installationLookupForLeaseScope( +/** Read the lowercase owner from a grant lease scope, if it names one. */ +export function ownerFromLeaseScope( leaseScope: string | undefined, -): { label: string; path: string } | undefined { - const repository = /^repository:([^/]+)\/([^/]+)$/.exec(leaseScope ?? ""); - if (repository?.[1] && repository[2]) { - return { - label: `${repository[1]}/${repository[2]}`, - path: `/repos/${encodeURIComponent(repository[1])}/${encodeURIComponent(repository[2])}/installation`, - }; - } - const org = /^organization:([^/]+)$/.exec(leaseScope ?? "")?.[1]; - return org - ? { label: org, path: `/orgs/${encodeURIComponent(org)}/installation` } +): string | undefined { + return leaseScope?.startsWith(OWNER_LEASE_SCOPE_PREFIX) + ? leaseScope.slice(OWNER_LEASE_SCOPE_PREFIX.length) || undefined : undefined; } diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 8dc07719ce..5cee236f18 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -80,6 +80,7 @@ import { GITHUB_AUTH_TOKEN_PLACEHOLDER, issueGitHubCredential, listVerifiedUserEmails, + loadInstallations, normalizeScopeList, readEnv, requireEnv, @@ -350,6 +351,9 @@ export function githubPlugin( await markPullRequestFeedbackReviewing(app, target), privateKeyEnv, events: ctx.events, + refreshInstallations: async () => { + await loadInstallations(app, ctx.cache, { refresh: true }); + }, webhookSecret: () => readEnv("GITHUB_WEBHOOK_SECRET"), }), ]; diff --git a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts index 6ffaf0122f..903d2960a0 100644 --- a/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts +++ b/packages/junior-github/src/tools/resolve-pull-request-review-thread.ts @@ -125,7 +125,7 @@ export function createGitHubResolvePullRequestReviewThreadTool( }`; const lookupResponse = await ctx.egress.fetch({ provider: "github", - operation: "github.pull.review-thread.get", + operation: `github.pull.review-thread.get:${repo.ref.toLowerCase()}`, request: new Request("https://api.github.com/graphql", { method: "POST", headers: { "Content-Type": "application/json" }, diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index c5351de87f..b742b2960f 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -176,6 +176,8 @@ export function createGitHubWebhookRoute(args: { ): Promise; privateKeyEnv: string; events: EventPublisher; + /** Reload the App's installations after one is added, removed, or renamed. */ + refreshInstallations(): Promise; webhookSecret(): string | undefined; }): PluginRoute { return { @@ -200,6 +202,11 @@ export function createGitHubWebhookRoute(args: { if (!installationId) { return new Response("Ignored", { status: 202 }); } + // Installs, uninstalls, and account renames change the installation map. + if (eventName === "installation" || eventName === "installation_target") { + await args.refreshInstallations(); + return new Response("Accepted", { status: 202 }); + } const botEmail = args.botEmail(); const pullRequestOutcome = eventName === "pull_request" diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index 84f910462d..0595546d7c 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -153,8 +153,9 @@ function pluginCache() { async getOrLoad( key: string, load: () => Promise<{ ttlMs?: number; value: T }>, + options?: { refresh?: boolean }, ): Promise { - if (entries.has(key)) return entries.get(key) as T; + if (!options?.refresh && entries.has(key)) return entries.get(key) as T; const entry = await load(); if (entry.ttlMs) entries.set(key, entry.value); return entry.value; @@ -251,15 +252,11 @@ function cloneStateValue(value: T): T { } /** - * Mock the App owner `getsentry` (installation 456), `fairsource` repos - * (installation 789), and repositories under `uninstalled` without the App. + * Mock the App owner `getsentry` (installation 456) and `FairSource` + * (installation 789). The App is not installed on any other account. */ function mockGitHubInstallationApi(): CapturedRequest[] { const requests: CapturedRequest[] = []; - const installationFor = (owner: unknown) => - owner === "uninstalled" - ? HttpResponse.json({ message: "Not Found" }, { status: 404 }) - : HttpResponse.json({ id: owner === "fairsource" ? 789 : 456 }); mswServer.use( http.get("https://api.github.com/app", async ({ request }) => { requests.push(await captureRequest(request)); @@ -268,17 +265,13 @@ function mockGitHubInstallationApi(): CapturedRequest[] { }); }), http.get( - "https://api.github.com/repos/:owner/:repo/installation", - async ({ params, request }) => { - requests.push(await captureRequest(request)); - return installationFor(params.owner); - }, - ), - http.get( - "https://api.github.com/orgs/:org/installation", - async ({ params, request }) => { + "https://api.github.com/app/installations", + async ({ request }) => { requests.push(await captureRequest(request)); - return installationFor(params.org); + return HttpResponse.json([ + { id: 456, account: { login: "getsentry" } }, + { id: 789, account: { login: "FairSource" } }, + ]); }, ), http.get( @@ -791,7 +784,7 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-read", access: "read", - leaseScope: "repository:getsentry/junior", + leaseScope: "owner:getsentry", reason: "github.api-read", }); expect( @@ -801,7 +794,7 @@ describe("github plugin", () => { }), ).toMatchObject({ name: "installation-read", - leaseScope: "organization:fairsource", + leaseScope: "owner:fairsource", }); expect( await grantForEgress({ @@ -2311,7 +2304,7 @@ Conversation: \`local:test:old-conversation\` }), ).toMatchObject({ name: "installation-read", - leaseScope: "repository:fairsource/fair.io", + leaseScope: "owner:fairsource", reason: "github.graphql-read", }); expect( @@ -2385,15 +2378,33 @@ Conversation: \`local:test:old-conversation\` await expect( grantForEgress({ method: "POST", - operation: "github.pull.review-thread.resolve:getsentry/junior", + operation: "github.pull.review-thread.resolve:FairSource/fair.io", url: "https://api.github.com/graphql", bodyText, }), ).resolves.toMatchObject({ name: "installation-write", access: "write", + leaseScope: "owner:fairsource", reason: "github.installation-write", }); + // The lookup names only a node id, so the host tool's operation names + // the owner whose installation can read the thread. + await expect( + grantForEgress({ + method: "POST", + operation: "github.pull.review-thread.get:fairsource/fair.io", + url: "https://api.github.com/graphql", + bodyText: JSON.stringify({ + query: + "query ReviewThread($threadId: ID!) { node(id: $threadId) { id } }", + variables: { threadId: "PRRT_kwDOthread" }, + }), + }), + ).resolves.toMatchObject({ + name: "installation-read", + leaseScope: "owner:fairsource", + }); await expect( grantForEgress({ method: "POST", @@ -2742,7 +2753,7 @@ Conversation: \`local:test:old-conversation\` ); }); - it("issues installation-write credentials from the target repository's installation", async () => { + it("issues installation-write credentials from the target owner's installation", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) .toString(); @@ -2758,10 +2769,11 @@ Conversation: \`local:test:old-conversation\` method: "GET", url: "https://github.com/fairsource/fair.io.git/info/refs?service=git-receive-pack", }); + const cache = pluginCache(); const issue = async (leaseScope = grant?.leaseScope) => await plugin.hooks?.issueCredential?.({ actor: { platform: "system", name: "event" }, - cache: pluginCache(), + cache, grant: { ...grant!, leaseScope }, db, log: pluginLog, @@ -2772,16 +2784,26 @@ Conversation: \`local:test:old-conversation\` const result = await issue(); expect(result?.type).toBe("lease"); + expect(grant?.leaseScope).toBe("owner:fairsource"); expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ - "GET https://api.github.com/repos/fairsource/fair.io/installation", + "GET https://api.github.com/app", + "GET https://api.github.com/app/installations?per_page=100&page=1", "POST https://api.github.com/app/installations/789/access_tokens", ]); // Write tokens keep the full installed App permissions and repos. - expect(requests[1]?.body).toEqual({}); - await expect(issue("repository:uninstalled/repo")).resolves.toEqual({ + expect(requests[2]?.body).toEqual({}); + + // A write never falls back to another installation. It reloads the + // cached installations once, in case the App was just installed. + requests.length = 0; + await expect(issue("owner:uninstalled")).resolves.toEqual({ type: "unavailable", - message: "The GitHub App is not installed on uninstalled/repo.", + message: "The GitHub App is not installed on uninstalled.", }); + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "GET https://api.github.com/app", + "GET https://api.github.com/app/installations?per_page=100&page=1", + ]); }); it("issues read-only GitHub App installation credentials from plugin hooks", async () => { @@ -2831,10 +2853,10 @@ Conversation: \`local:test:old-conversation\` ], }, }); - // A read without a target uses the App owner's installation. + // A read without an owner uses the App owner's installation. expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ "GET https://api.github.com/app", - "GET https://api.github.com/orgs/getsentry/installation", + "GET https://api.github.com/app/installations?per_page=100&page=1", "POST https://api.github.com/app/installations/456/access_tokens", ]); expect(requests[2]).toMatchObject({ @@ -2849,13 +2871,12 @@ Conversation: \`local:test:old-conversation\` // Public repositories stay readable where the App is not installed. requests.length = 0; - await expect(issue("repository:uninstalled/repo")).resolves.toMatchObject({ + await expect(issue("owner:uninstalled")).resolves.toMatchObject({ type: "lease", }); expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ - "GET https://api.github.com/repos/uninstalled/repo/installation", "GET https://api.github.com/app", - "GET https://api.github.com/orgs/getsentry/installation", + "GET https://api.github.com/app/installations?per_page=100&page=1", "POST https://api.github.com/app/installations/456/access_tokens", ]); }); @@ -2876,7 +2897,7 @@ Conversation: \`local:test:old-conversation\` grant: { name: "installation-read", access: "read" as const, - leaseScope: "repository:getsentry/junior", + leaseScope: "owner:getsentry", reason: "github.api-read", }, db, @@ -2891,13 +2912,13 @@ Conversation: \`local:test:old-conversation\` const lookups = requests.filter( (request) => request.method === "GET" && - (request.url === "https://api.github.com/app/installations/456" || - request.url.endsWith("/installation")), + request.url.startsWith("https://api.github.com/app"), ); const tokenRequests = requests.filter((request) => request.url.endsWith("/app/installations/456/access_tokens"), ); - expect(lookups).toHaveLength(2); + // One App, installation list, and permission read serve both tokens. + expect(lookups).toHaveLength(3); expect(tokenRequests).toHaveLength(2); expect(tokenRequests.map((request) => request.body)).toEqual([ { diff --git a/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts b/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts index 59100ac0a7..b63591fef3 100644 --- a/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts +++ b/packages/junior-github/tests/resolve-pull-request-review-thread.test.ts @@ -81,7 +81,7 @@ describe("resolvePullRequestReviewThread", () => { expect(fetch).toHaveBeenCalledTimes(2); expect(fetch.mock.calls[0]?.[0]).toMatchObject({ - operation: "github.pull.review-thread.get", + operation: "github.pull.review-thread.get:getsentry/junior", provider: "github", }); const lookupRequest = fetch.mock.calls[0]?.[0]?.request as Request; diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index fc5f19bf8a..85ca98b94f 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -273,6 +273,7 @@ function webhookRoute( typeof createGitHubWebhookRoute >[0]["markFeedbackReviewing"] = async () => {}, hasMatch: () => Promise = async () => true, + refreshInstallations: () => Promise = async () => {}, ) { return createGitHubWebhookRoute({ annotations: { @@ -299,6 +300,7 @@ function webhookRoute( log: { error }, markFeedbackReviewing, privateKeyEnv: "GITHUB_APP_PRIVATE_KEY", + refreshInstallations, events: { hasMatch, async publish(event) { @@ -2078,6 +2080,40 @@ describe("GitHub-owned pull request outcomes", () => { } }); + it.each(["installation", "installation_target"])( + "reloads the App's installations on %s deliveries", + async (eventName) => { + const fixture = await createGitHubFixture(); + const published: EventInput[] = []; + const refreshInstallations = vi.fn(async () => {}); + try { + const route = webhookRoute( + fixture, + published, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + refreshInstallations, + ); + const response = await route.handler( + signedRequest( + { action: "created", installation: { id: 789 } }, + eventName, + ), + ); + + expect(response.status).toBe(202); + expect(refreshInstallations).toHaveBeenCalledOnce(); + expect(published).toEqual([]); + } finally { + await fixture.close(); + } + }, + ); + it.each([ "sentry-junior@example.com", "264270552+human@users.noreply.github.com", diff --git a/packages/junior/tests/msw/handlers/github-api.ts b/packages/junior/tests/msw/handlers/github-api.ts index 206eef54b2..0baee33251 100644 --- a/packages/junior/tests/msw/handlers/github-api.ts +++ b/packages/junior/tests/msw/handlers/github-api.ts @@ -9,19 +9,20 @@ export const githubApiHandlers = [ http.get(`${GITHUB_API_ORIGIN}/repos/:owner/:repo/deployments`, () => HttpResponse.json([]), ), - // The GitHub plugin finds the App installation for the target repository, - // organization, or App owner before it mints a token. - http.get(`${GITHUB_API_ORIGIN}/repos/:owner/:repo/installation`, () => - HttpResponse.json({ id: 456 }), - ), - http.get(`${GITHUB_API_ORIGIN}/orgs/:org/installation`, () => - HttpResponse.json({ id: 456 }), - ), + // The GitHub plugin lists the App's installations, then uses the one on + // the request's owner, or the App owner's for reads, before it mints a + // token. Tests and evals write to repositories of these owners. http.get(`${GITHUB_API_ORIGIN}/app`, () => HttpResponse.json({ owner: { login: "getsentry", type: "Organization" }, }), ), + http.get(`${GITHUB_API_ORIGIN}/app/installations`, () => + HttpResponse.json([ + { id: 456, account: { login: "getsentry" } }, + { id: 457, account: { login: "acme" } }, + ]), + ), // The GitHub plugin reads the installation's permissions before it mints a // token, unless the host declares `appPermissions`. http.get(`${GITHUB_API_ORIGIN}/app/installations/:installationId`, () => From 5e75cc2002c8d10a72bcf2ca91e44fb8d7261733 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:43:01 +0000 Subject: [PATCH 4/7] docs(plugin-api): Describe lease scope as an account Co-Authored-By: David Cramer --- packages/junior-plugin-api/src/credentials.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/junior-plugin-api/src/credentials.ts b/packages/junior-plugin-api/src/credentials.ts index 62b4d0e302..a751b9b10a 100644 --- a/packages/junior-plugin-api/src/credentials.ts +++ b/packages/junior-plugin-api/src/credentials.ts @@ -51,7 +51,7 @@ export const pluginStoredTokensSchema = z export const pluginGrantSchema = z .object({ access: pluginGrantAccessSchema, - /** Plugin-defined target, such as one repository. Junior remembers shared installation leases per scope. */ + /** Plugin-defined target, such as one GitHub account. Junior remembers shared installation leases per scope. */ leaseScope: nonBlankStringSchema.optional(), name: pluginGrantNameSchema, reason: nonBlankStringSchema.optional(), From 738b4b1216e8c524c1e7445665dfbdeae6a68c85 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:59:50 +0000 Subject: [PATCH 5/7] feat(github): Store App installations in the database Keep a plugin-owned table of active installations by account login. installation and installation_target webhooks save, rename, and remove rows. A lookup that finds no row syncs the table from GET /app/installations at most once a minute, which fills it on first use and repairs missed webhooks. This replaces the cached installation map. Co-Authored-By: David Cramer --- .../src/content/docs/extend/github-plugin.md | 9 +- packages/junior-github/SETUP.md | 9 +- .../migrations/0010_installations.sql | 6 + .../migrations/meta/0010_snapshot.json | 443 ++++++++++++++++++ .../migrations/meta/_journal.json | 7 + .../junior-github/src/credential-support.ts | 135 +++--- packages/junior-github/src/db/schema.ts | 18 + .../junior-github/src/installations/store.ts | 85 ++++ packages/junior-github/src/plugin.ts | 4 - .../junior-github/src/webhooks/handler.ts | 49 +- packages/junior-github/tests/github-db.ts | 1 + .../junior-github/tests/github-plugin.test.ts | 33 +- .../tests/webhook-outcomes.test.ts | 79 ++-- .../tests/fixtures/postgres/global-setup.ts | 16 + 14 files changed, 774 insertions(+), 120 deletions(-) create mode 100644 packages/junior-github/migrations/0010_installations.sql create mode 100644 packages/junior-github/migrations/meta/0010_snapshot.json create mode 100644 packages/junior-github/src/installations/store.ts diff --git a/packages/docs/src/content/docs/extend/github-plugin.md b/packages/docs/src/content/docs/extend/github-plugin.md index 622fff4533..07a78bf80a 100644 --- a/packages/docs/src/content/docs/extend/github-plugin.md +++ b/packages/docs/src/content/docs/extend/github-plugin.md @@ -88,10 +88,11 @@ when the target is not obvious. One deployment works with every installation of the App. An App has at most one installation per account, so Junior uses the installation on the account -that owns the target repository or organization. Junior keeps a list of the -App's installations for 10 minutes. `installation` and `installation_target` -webhooks reload the list. A webhook delivery uses the installation that sent -it. +that owns the target repository or organization. Junior stores the App's +installations in its database. `installation` and `installation_target` +webhooks keep them current. When Junior finds no stored installation for an +account, it syncs the list from GitHub, at most once a minute. A webhook +delivery uses the installation that sent it. Reads that name no owner, such as search or GraphQL without an `owner` variable, use the installation on the account that owns the App. Reads of diff --git a/packages/junior-github/SETUP.md b/packages/junior-github/SETUP.md index fbbab7e757..308feeb6d7 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -47,10 +47,11 @@ Set on the harness host (never in skill files): One Junior deployment works with every installation of the App. Junior uses the installation on the account that owns the target repository or organization. -It keeps a list of the App's installations for 10 minutes, and `installation` -and `installation_target` webhooks reload it. Reads that name no owner, or an -account without the App, use the installation on the account that owns the App. -Writes to an account without the App fail. +It stores the App's installations in the database. `installation` and +`installation_target` webhooks keep them current. A lookup that finds no row +syncs them from `GET /app/installations`, at most once a minute. Reads that +name no owner, or an account without the App, use the installation on the +account that owns the App. Writes to an account without the App fail. ### Vercel env setup (multiline-safe) diff --git a/packages/junior-github/migrations/0010_installations.sql b/packages/junior-github/migrations/0010_installations.sql new file mode 100644 index 0000000000..64b1b4b27e --- /dev/null +++ b/packages/junior-github/migrations/0010_installations.sql @@ -0,0 +1,6 @@ +CREATE TABLE "junior_github_installations" ( + "installation_id" bigint PRIMARY KEY NOT NULL, + "account_login" text NOT NULL, + "updated_at" timestamp with time zone NOT NULL, + CONSTRAINT "junior_github_installations_account_login_unique" UNIQUE("account_login") +); diff --git a/packages/junior-github/migrations/meta/0010_snapshot.json b/packages/junior-github/migrations/meta/0010_snapshot.json new file mode 100644 index 0000000000..7614a8b39d --- /dev/null +++ b/packages/junior-github/migrations/meta/0010_snapshot.json @@ -0,0 +1,443 @@ +{ + "id": "85ace2c4-32a3-4672-9439-282335d44634", + "prevId": "6b12b332-4224-4e69-9ab7-51df6a4dd91c", + "version": "7", + "dialect": "postgresql", + "tables": { + "public.junior_github_installations": { + "name": "junior_github_installations", + "schema": "", + "columns": { + "installation_id": { + "name": "installation_id", + "type": "bigint", + "primaryKey": true, + "notNull": true + }, + "account_login": { + "name": "account_login", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": { + "junior_github_installations_account_login_unique": { + "name": "junior_github_installations_account_login_unique", + "nullsNotDistinct": false, + "columns": [ + "account_login" + ] + } + }, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_issues": { + "name": "junior_github_issues", + "schema": "", + "columns": { + "issue_id": { + "name": "issue_id", + "type": "text", + "primaryKey": true, + "notNull": true + }, + "repository_id": { + "name": "repository_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "state": { + "name": "state", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "state_reason": { + "name": "state_reason", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "conversation_ids": { + "name": "conversation_ids", + "type": "text[]", + "primaryKey": false, + "notNull": true, + "default": "ARRAY[]::text[]" + }, + "opened_at": { + "name": "opened_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "closed_at": { + "name": "closed_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": { + "junior_github_issues_opened_at_idx": { + "name": "junior_github_issues_opened_at_idx", + "columns": [ + { + "expression": "opened_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_issues_closed_at_idx": { + "name": "junior_github_issues_closed_at_idx", + "columns": [ + { + "expression": "closed_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_issues_open_idx": { + "name": "junior_github_issues_open_idx", + "columns": [ + { + "expression": "issue_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_issues\".\"state\" = 'open'", + "concurrently": false, + "method": "btree", + "with": {} + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_request_issues": { + "name": "junior_github_pull_request_issues", + "schema": "", + "columns": { + "pull_request_id": { + "name": "pull_request_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "issue_repository_full_name": { + "name": "issue_repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "issue_number": { + "name": "issue_number", + "type": "integer", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": { + "junior_github_pull_request_issues_pull_request_id_junior_github_pull_requests_pull_request_id_fk": { + "name": "junior_github_pull_request_issues_pull_request_id_junior_github_pull_requests_pull_request_id_fk", + "tableFrom": "junior_github_pull_request_issues", + "tableTo": "junior_github_pull_requests", + "columnsFrom": [ + "pull_request_id" + ], + "columnsTo": [ + "pull_request_id" + ], + "onDelete": "cascade", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": { + "junior_github_pull_request_issues_pull_request_id_issue_repository_full_name_issue_number_pk": { + "name": "junior_github_pull_request_issues_pull_request_id_issue_repository_full_name_issue_number_pk", + "columns": [ + "pull_request_id", + "issue_repository_full_name", + "issue_number" + ] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_request_requesters": { + "name": "junior_github_pull_request_requesters", + "schema": "", + "columns": { + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "identity_id": { + "name": "identity_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "created_at": { + "name": "created_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": {}, + "foreignKeys": {}, + "compositePrimaryKeys": { + "junior_github_pull_request_requesters_repository_full_name_number_identity_id_pk": { + "name": "junior_github_pull_request_requesters_repository_full_name_number_identity_id_pk", + "columns": [ + "repository_full_name", + "number", + "identity_id" + ] + } + }, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + }, + "public.junior_github_pull_requests": { + "name": "junior_github_pull_requests", + "schema": "", + "columns": { + "pull_request_id": { + "name": "pull_request_id", + "type": "text", + "primaryKey": true, + "notNull": true + }, + "repository_id": { + "name": "repository_id", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "repository_full_name": { + "name": "repository_full_name", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "number": { + "name": "number", + "type": "integer", + "primaryKey": false, + "notNull": true + }, + "state": { + "name": "state", + "type": "text", + "primaryKey": false, + "notNull": true + }, + "commit_composition": { + "name": "commit_composition", + "type": "text", + "primaryKey": false, + "notNull": false + }, + "conversation_ids": { + "name": "conversation_ids", + "type": "text[]", + "primaryKey": false, + "notNull": true, + "default": "ARRAY[]::text[]" + }, + "opened_at": { + "name": "opened_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + }, + "merged_at": { + "name": "merged_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "closed_at": { + "name": "closed_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": false + }, + "updated_at": { + "name": "updated_at", + "type": "timestamp with time zone", + "primaryKey": false, + "notNull": true + } + }, + "indexes": { + "junior_github_pull_requests_opened_at_idx": { + "name": "junior_github_pull_requests_opened_at_idx", + "columns": [ + { + "expression": "opened_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_merged_at_idx": { + "name": "junior_github_pull_requests_merged_at_idx", + "columns": [ + { + "expression": "merged_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_closed_at_idx": { + "name": "junior_github_pull_requests_closed_at_idx", + "columns": [ + { + "expression": "closed_at", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_open_idx": { + "name": "junior_github_pull_requests_open_idx", + "columns": [ + { + "expression": "pull_request_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_pull_requests\".\"state\" = 'open'", + "concurrently": false, + "method": "btree", + "with": {} + }, + "junior_github_pull_requests_unmerged_conversations_idx": { + "name": "junior_github_pull_requests_unmerged_conversations_idx", + "columns": [ + { + "expression": "conversation_ids", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "where": "\"junior_github_pull_requests\".\"state\" <> 'merged'", + "concurrently": false, + "method": "gin", + "with": {} + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "policies": {}, + "checkConstraints": {}, + "isRLSEnabled": false + } + }, + "enums": {}, + "schemas": {}, + "sequences": {}, + "roles": {}, + "policies": {}, + "views": {}, + "_meta": { + "columns": {}, + "schemas": {}, + "tables": {} + } +} \ No newline at end of file diff --git a/packages/junior-github/migrations/meta/_journal.json b/packages/junior-github/migrations/meta/_journal.json index 463a340cf9..bb46eb0870 100644 --- a/packages/junior-github/migrations/meta/_journal.json +++ b/packages/junior-github/migrations/meta/_journal.json @@ -71,6 +71,13 @@ "when": 1793225353668, "tag": "0009_nostalgic_thing", "breakpoints": true + }, + { + "idx": 10, + "version": "7", + "when": 1793375469093, + "tag": "0010_installations", + "breakpoints": true } ] } \ No newline at end of file diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 710b7e9189..1a9805ab80 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -14,7 +14,13 @@ import type { PluginUserTokenSlot, IssueCredentialHookContext, } from "@sentry/junior-plugin-api"; +import type { GitHubDb } from "./db/database.js"; import { ownerFromLeaseScope } from "./installation-targets.js"; +import { + findGitHubInstallationId, + replaceGitHubInstallations, + type GitHubInstallation, +} from "./installations/store.js"; import { type GitHubAppPermissions, readGrantPermissions, @@ -91,9 +97,11 @@ export const GITHUB_APP_PRIVATE_KEY_ENV = "GITHUB_APP_PRIVATE_KEY"; export const GITHUB_AUTH_TOKEN_ENV = "GITHUB_TOKEN"; export const GITHUB_AUTH_TOKEN_PLACEHOLDER = "ghp_host_managed_credential"; const MAX_LEASE_MS = 60 * 60 * 1000; -// Installs change rarely, and `installation` webhooks refresh the map sooner. -const INSTALLATIONS_TTL_MS = 10 * 60 * 1000; +// `installation` webhooks keep stored installations current. A sync only +// repairs missed ones, so a short interval bounds GitHub calls on misses. +const INSTALLATIONS_SYNC_INTERVAL_MS = 60 * 1000; const INSTALLATIONS_PAGE_SIZE = 100; +const APP_OWNER_TTL_MS = 60 * 60 * 1000; const GITHUB_API = "https://api.github.com"; const REFRESH_BUFFER_MS = 5 * 60 * 1000; const USER_REFRESH_TIMEOUT_MS = 20_000; @@ -805,66 +813,81 @@ export async function issueInstallationToken( ); } -/** Every installation of the App, by lowercase account login. */ -export interface GitHubInstallations { - /** Lowercase login of the account that owns the App. */ - appOwner: string; - byOwner: Record; -} - function readLogin(value: unknown): string | undefined { const login = isRecord(value) ? value.login : undefined; return typeof login === "string" && login ? login.toLowerCase() : undefined; } +/** Read the lowercase login of the account that owns the App. */ +async function appOwnerLogin( + app: GitHubAppEnv, + cache: PluginCache, +): Promise { + return await cache.getOrLoad("app-owner", async () => { + const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); + const response = await githubRequest(GITHUB_API, "/app", { + token: appJwt, + }); + const login = readLogin(isRecord(response) ? response.owner : undefined); + if (!login) { + throw new Error("GitHub App response missing owner"); + } + return { value: login, ttlMs: APP_OWNER_TTL_MS }; + }); +} + /** - * Load every installation of the App. An App can be installed at most once - * per account, so the account login identifies the installation. Set - * `refresh` to reload the cached map now. + * Replace the stored installations with the active ones that GitHub lists. + * This fills the table on first use and repairs missed `installation` + * webhooks. It runs at most once per `INSTALLATIONS_SYNC_INTERVAL_MS`. */ -export async function loadInstallations( +async function syncInstallations( app: GitHubAppEnv, + db: GitHubDb, cache: PluginCache, - options?: { refresh?: boolean }, -): Promise { - return await cache.getOrLoad( - "installations", - async () => { - const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); - const appResponse = await githubRequest(GITHUB_API, "/app", { - token: appJwt, - }); - const appOwner = readLogin( - isRecord(appResponse) ? appResponse.owner : undefined, +): Promise { + await cache.getOrLoad("installations-synced", async () => { + const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); + const active: GitHubInstallation[] = []; + for (let page = 1; ; page += 1) { + const installations = await githubRequest( + GITHUB_API, + `/app/installations?per_page=${INSTALLATIONS_PAGE_SIZE}&page=${page}`, + { token: appJwt }, ); - if (!appOwner) { - throw new Error("GitHub App response missing owner"); + if (!Array.isArray(installations)) { + throw new Error("GitHub installations response is not a list"); } - const byOwner: Record = {}; - for (let page = 1; ; page += 1) { - const installations = await githubRequest( - GITHUB_API, - `/app/installations?per_page=${INSTALLATIONS_PAGE_SIZE}&page=${page}`, - { token: appJwt }, - ); - if (!Array.isArray(installations)) { - throw new Error("GitHub installations response is not a list"); - } - for (const installation of installations) { - const login = readLogin( - isRecord(installation) ? installation.account : undefined, - ); - const id = isRecord(installation) ? installation.id : undefined; - if (login && typeof id === "number" && Number.isSafeInteger(id)) { - byOwner[login] = id; - } + for (const installation of installations) { + if (!isRecord(installation) || installation.suspended_at) continue; + const accountLogin = readLogin(installation.account); + const installationId = installation.id; + if ( + accountLogin && + typeof installationId === "number" && + Number.isSafeInteger(installationId) + ) { + active.push({ accountLogin, installationId }); } - if (installations.length < INSTALLATIONS_PAGE_SIZE) break; } - return { value: { appOwner, byOwner }, ttlMs: INSTALLATIONS_TTL_MS }; - }, - options, - ); + if (installations.length < INSTALLATIONS_PAGE_SIZE) break; + } + await replaceGitHubInstallations(db, active); + return { value: true, ttlMs: INSTALLATIONS_SYNC_INTERVAL_MS }; + }); +} + +/** Read the installation on one account, and sync once when none is stored. */ +async function installationIdFor( + accountLogin: string, + app: GitHubAppEnv, + ctx: IssueCredentialHookContext, +): Promise { + const db = ctx.db as GitHubDb; + const stored = await findGitHubInstallationId(db, accountLogin); + if (stored) return stored; + await syncInstallations(app, db, ctx.cache); + return await findGitHubInstallationId(db, accountLogin); } /** @@ -878,26 +901,20 @@ async function resolveInstallationId( app: GitHubAppEnv, ): Promise { const owner = ownerFromLeaseScope(ctx.grant.leaseScope); - let installations = await loadInstallations(app, ctx.cache); if (owner) { - const id = installations.byOwner[owner]; + const id = await installationIdFor(owner, app, ctx); if (id) return id; if (ctx.grant.access === "write") { - // The App can be installed before its `installation` webhook arrives. - installations = await loadInstallations(app, ctx.cache, { - refresh: true, - }); - const refreshedId = installations.byOwner[owner]; - if (refreshedId) return refreshedId; throw new GitHubPluginSetupError( `The GitHub App is not installed on ${owner}.`, ); } } - const defaultId = installations.byOwner[installations.appOwner]; + const appOwner = await appOwnerLogin(app, ctx.cache); + const defaultId = await installationIdFor(appOwner, app, ctx); if (!defaultId) { throw new GitHubPluginSetupError( - `The GitHub App is not installed on ${installations.appOwner}, the account that owns it.`, + `The GitHub App is not installed on ${appOwner}, the account that owns it.`, ); } return defaultId; diff --git a/packages/junior-github/src/db/schema.ts b/packages/junior-github/src/db/schema.ts index 93d4943446..7e28014a35 100644 --- a/packages/junior-github/src/db/schema.ts +++ b/packages/junior-github/src/db/schema.ts @@ -1,5 +1,6 @@ import { sql } from "drizzle-orm"; import { + bigint, index, integer, pgTable, @@ -152,7 +153,24 @@ export const juniorGitHubPullRequestRequesters = pgTable( ], ); +/** + * Active installations of the GitHub App. An App has at most one + * installation per account, so the account login finds the installation for + * a request. `installation` webhooks keep the rows current. A sync from + * `GET /app/installations` fills the table and repairs missed webhooks. + */ +export const juniorGitHubInstallations = pgTable( + "junior_github_installations", + { + installationId: bigint("installation_id", { mode: "number" }).primaryKey(), + /** Lowercase login of the account the App is installed on. */ + accountLogin: text("account_login").notNull().unique(), + updatedAt: timestamp("updated_at", { withTimezone: true }).notNull(), + }, +); + export const githubSqlSchema = { + juniorGitHubInstallations, juniorGitHubIssues, juniorGitHubPullRequestIssues, juniorGitHubPullRequestRequesters, diff --git a/packages/junior-github/src/installations/store.ts b/packages/junior-github/src/installations/store.ts new file mode 100644 index 0000000000..0ea4f939c9 --- /dev/null +++ b/packages/junior-github/src/installations/store.ts @@ -0,0 +1,85 @@ +import { and, eq, ne } from "drizzle-orm"; +import type { GitHubDb } from "../db/database.js"; +import { juniorGitHubInstallations } from "../db/schema.js"; + +const table = juniorGitHubInstallations; + +/** One active App installation and the account it is installed on. */ +export interface GitHubInstallation { + installationId: number; + /** Account login. It is stored in lowercase. */ + accountLogin: string; +} + +/** Read the installation on one account, or undefined when there is none. */ +export async function findGitHubInstallationId( + db: GitHubDb, + accountLogin: string, +): Promise { + const [row] = await db + .select({ installationId: table.installationId }) + .from(table) + .where(eq(table.accountLogin, accountLogin.toLowerCase())) + .limit(1); + return row?.installationId; +} + +/** + * Save one installation, such as from an `installation` webhook. A row for + * another installation with the same login is stale after a rename, so it is + * removed. + */ +export async function saveGitHubInstallation( + db: GitHubDb, + installation: GitHubInstallation, +): Promise { + const accountLogin = installation.accountLogin.toLowerCase(); + await db.transaction(async (tx) => { + await tx + .delete(table) + .where( + and( + eq(table.accountLogin, accountLogin), + ne(table.installationId, installation.installationId), + ), + ); + await tx + .insert(table) + .values({ + installationId: installation.installationId, + accountLogin, + updatedAt: new Date(), + }) + .onConflictDoUpdate({ + target: table.installationId, + set: { accountLogin, updatedAt: new Date() }, + }); + }); +} + +/** Remove an installation that was deleted or suspended. */ +export async function deleteGitHubInstallation( + db: GitHubDb, + installationId: number, +): Promise { + await db.delete(table).where(eq(table.installationId, installationId)); +} + +/** Replace every row with the complete list of active installations. */ +export async function replaceGitHubInstallations( + db: GitHubDb, + installations: GitHubInstallation[], +): Promise { + const updatedAt = new Date(); + await db.transaction(async (tx) => { + await tx.delete(table); + if (installations.length === 0) return; + await tx.insert(table).values( + installations.map((installation) => ({ + installationId: installation.installationId, + accountLogin: installation.accountLogin.toLowerCase(), + updatedAt, + })), + ); + }); +} diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index d0bd58be1a..2667853453 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -81,7 +81,6 @@ import { GITHUB_AUTH_TOKEN_PLACEHOLDER, issueGitHubCredential, listVerifiedUserEmails, - loadInstallations, normalizeScopeList, readEnv, requireEnv, @@ -349,9 +348,6 @@ export function githubPlugin( await markPullRequestFeedbackReviewing(app, target), privateKeyEnv, events: ctx.events, - refreshInstallations: async () => { - await loadInstallations(app, ctx.cache, { refresh: true }); - }, webhookSecret: () => readEnv("GITHUB_WEBHOOK_SECRET"), }), ]; diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index b742b2960f..14f95ed58d 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -12,6 +12,10 @@ import type { import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; import type { GitHubPullRequestCommitComposition } from "../db/schema.js"; +import { + deleteGitHubInstallation, + saveGitHubInstallation, +} from "../installations/store.js"; import { recordGitHubIssueConversations, recordGitHubIssueOutcome, @@ -100,6 +104,46 @@ function webhookInstallationId(body: unknown): number | undefined { return parseInstallationId((installation as { id?: unknown }).id); } +const installationEventSchema = z.object({ + action: z.string(), + /** Set on `installation_target` deliveries, with the new login. */ + account: z.object({ login: z.string().min(1) }).optional(), + installation: z.object({ + account: z.object({ login: z.string().min(1) }).optional(), + }), +}); + +/** + * Keep the stored installations current. Installs, permission changes, and + * unsuspends save the account login. Uninstalls and suspends remove the row, + * because GitHub cannot mint tokens for them. Account renames arrive as + * `installation_target` deliveries. + */ +async function recordInstallationEvent( + db: GitHubDb, + eventName: "installation" | "installation_target", + installationId: number, + body: unknown, +): Promise { + const parsed = installationEventSchema.safeParse(body); + if (!parsed.success) return; + const { account, action, installation } = parsed.data; + if ( + eventName === "installation" && + (action === "deleted" || action === "suspend") + ) { + await deleteGitHubInstallation(db, installationId); + return; + } + const accountLogin = + eventName === "installation_target" + ? account?.login + : installation.account?.login; + if (accountLogin) { + await saveGitHubInstallation(db, { accountLogin, installationId }); + } +} + /** The pull request comment data that the feedback reaction needs. */ const feedbackTargetSchema = z.object({ commentId: z.number(), @@ -176,8 +220,6 @@ export function createGitHubWebhookRoute(args: { ): Promise; privateKeyEnv: string; events: EventPublisher; - /** Reload the App's installations after one is added, removed, or renamed. */ - refreshInstallations(): Promise; webhookSecret(): string | undefined; }): PluginRoute { return { @@ -202,9 +244,8 @@ export function createGitHubWebhookRoute(args: { if (!installationId) { return new Response("Ignored", { status: 202 }); } - // Installs, uninstalls, and account renames change the installation map. if (eventName === "installation" || eventName === "installation_target") { - await args.refreshInstallations(); + await recordInstallationEvent(args.db, eventName, installationId, body); return new Response("Accepted", { status: 202 }); } const botEmail = args.botEmail(); diff --git a/packages/junior-github/tests/github-db.ts b/packages/junior-github/tests/github-db.ts index d1492782b8..e3b16693b4 100644 --- a/packages/junior-github/tests/github-db.ts +++ b/packages/junior-github/tests/github-db.ts @@ -33,6 +33,7 @@ export async function createGitHubFixture(): Promise { "0006_fat_korvac.sql", "0007_shallow_millenium_guard.sql", "0009_nostalgic_thing.sql", + "0010_installations.sql", ]) { await fixture.execute(await migrationSql(migrationFile)); } diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index dbf1d2b367..c2ee9c2865 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -11,6 +11,7 @@ import { afterAll, afterEach, beforeAll, + beforeEach, describe, expect, it, @@ -23,6 +24,7 @@ import { gitHubPullRequestResource } from "../src/events/pull-request"; import { gitHubReleaseSourceResource } from "../src/events/release"; import { gitHubRepositoryResource } from "../src/events/repository"; import type { GitHubDb } from "../src/db/database"; +import { juniorGitHubInstallations } from "../src/db/schema"; import { createGitHubFixture, type GitHubFixture } from "./github-db"; import { mswServer } from "./msw"; @@ -36,6 +38,11 @@ beforeAll(async () => { db = dbFixture.db(); }); +beforeEach(async () => { + // Each test starts before Junior has stored any App installation. + await db.delete(juniorGitHubInstallations); +}); + afterAll(async () => { await dbFixture.close(); }); @@ -2793,27 +2800,31 @@ Conversation: \`local:test:old-conversation\` const result = await issue(); + // No installation is stored yet, so Junior syncs them from GitHub. expect(result?.type).toBe("lease"); expect(grant?.leaseScope).toBe("owner:fairsource"); expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ - "GET https://api.github.com/app", "GET https://api.github.com/app/installations?per_page=100&page=1", "POST https://api.github.com/app/installations/789/access_tokens", ]); // Write tokens keep the full installed App permissions and repos. - expect(requests[2]?.body).toEqual({}); + expect(requests[1]?.body).toEqual({}); + + // Later tokens use the stored installation. + requests.length = 0; + await expect(issue()).resolves.toMatchObject({ type: "lease" }); + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "POST https://api.github.com/app/installations/789/access_tokens", + ]); - // A write never falls back to another installation. It reloads the - // cached installations once, in case the App was just installed. + // A write never falls back to another installation. The sync just ran, + // so a miss does not call GitHub again. requests.length = 0; await expect(issue("owner:uninstalled")).resolves.toEqual({ type: "unavailable", message: "The GitHub App is not installed on uninstalled.", }); - expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ - "GET https://api.github.com/app", - "GET https://api.github.com/app/installations?per_page=100&page=1", - ]); + expect(requests).toEqual([]); }); it("issues read-only GitHub App installation credentials from plugin hooks", async () => { @@ -2885,8 +2896,8 @@ Conversation: \`local:test:old-conversation\` type: "lease", }); expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ - "GET https://api.github.com/app", "GET https://api.github.com/app/installations?per_page=100&page=1", + "GET https://api.github.com/app", "POST https://api.github.com/app/installations/456/access_tokens", ]); }); @@ -2927,8 +2938,8 @@ Conversation: \`local:test:old-conversation\` const tokenRequests = requests.filter((request) => request.url.endsWith("/app/installations/456/access_tokens"), ); - // One App, installation list, and permission read serve both tokens. - expect(lookups).toHaveLength(3); + // One installation sync and permission read serve both tokens. + expect(lookups).toHaveLength(2); expect(tokenRequests).toHaveLength(2); expect(tokenRequests.map((request) => request.body)).toEqual([ { diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index 69aba14cf3..a8e297ff74 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -11,6 +11,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { githubSqlSchema, type GitHubPullRequestCommitComposition, + juniorGitHubInstallations, juniorGitHubIssues, juniorGitHubPullRequestRequesters, juniorGitHubPullRequests, @@ -273,7 +274,6 @@ function webhookRoute( typeof createGitHubWebhookRoute >[0]["markFeedbackReviewing"] = async () => {}, hasMatch: () => Promise = async () => true, - refreshInstallations: () => Promise = async () => {}, ) { return createGitHubWebhookRoute({ annotations: { @@ -300,7 +300,6 @@ function webhookRoute( log: { error }, markFeedbackReviewing, privateKeyEnv: "GITHUB_APP_PRIVATE_KEY", - refreshInstallations, events: { hasMatch, async publish(event) { @@ -2082,39 +2081,51 @@ describe("GitHub-owned pull request outcomes", () => { } }); - it.each(["installation", "installation_target"])( - "reloads the App's installations on %s deliveries", - async (eventName) => { - const fixture = await createGitHubFixture(); - const published: EventInput[] = []; - const refreshInstallations = vi.fn(async () => {}); - try { - const route = webhookRoute( - fixture, - published, - undefined, - undefined, - undefined, - undefined, - undefined, - undefined, - refreshInstallations, - ); - const response = await route.handler( - signedRequest( - { action: "created", installation: { id: 789 } }, - eventName, - ), - ); + it("keeps stored installations current from installation deliveries", async () => { + const fixture = await createGitHubFixture(); + const published: EventInput[] = []; + const installations = async () => + await fixture + .db() + .select({ + accountLogin: juniorGitHubInstallations.accountLogin, + installationId: juniorGitHubInstallations.installationId, + }) + .from(juniorGitHubInstallations); + try { + const route = webhookRoute(fixture, published); + const deliver = async (eventName: string, body: unknown) => + expect( + (await route.handler(signedRequest(body, eventName))).status, + ).toBe(202); - expect(response.status).toBe(202); - expect(refreshInstallations).toHaveBeenCalledOnce(); - expect(published).toEqual([]); - } finally { - await fixture.close(); - } - }, - ); + await deliver("installation", { + action: "created", + installation: { id: 789, account: { login: "FairSource" } }, + }); + await expect(installations()).resolves.toEqual([ + { accountLogin: "fairsource", installationId: 789 }, + ]); + + await deliver("installation_target", { + action: "renamed", + account: { login: "fair-source" }, + installation: { id: 789 }, + }); + await expect(installations()).resolves.toEqual([ + { accountLogin: "fair-source", installationId: 789 }, + ]); + + await deliver("installation", { + action: "suspend", + installation: { id: 789, account: { login: "fair-source" } }, + }); + await expect(installations()).resolves.toEqual([]); + expect(published).toEqual([]); + } finally { + await fixture.close(); + } + }); it.each([ "sentry-junior@example.com", diff --git a/packages/junior/tests/fixtures/postgres/global-setup.ts b/packages/junior/tests/fixtures/postgres/global-setup.ts index 93f8b50fe6..85d89d4704 100644 --- a/packages/junior/tests/fixtures/postgres/global-setup.ts +++ b/packages/junior/tests/fixtures/postgres/global-setup.ts @@ -1,3 +1,4 @@ +import path from "node:path"; import type { TestProject } from "vitest/node"; import { cleanupPostgresHarness, @@ -5,6 +6,7 @@ import { type PostgresHarnessConfig, } from "@sentry/junior-testing/postgres"; import { migrateSchema } from "@/chat/conversations/sql/migrations"; +import { migratePluginSchemas } from "@/chat/plugins/migrations"; import type { JuniorSqlMigrationExecutor } from "@/db/db"; import { createPostgresJuniorSqlExecutor } from "@/db/postgres"; @@ -16,6 +18,19 @@ declare module "vitest" { const TEST_DATABASE_URL = process.env.DATABASE_URL; +/** + * Tests run the GitHub plugin through real Junior wiring, and it stores App + * installations in its own table. `junior upgrade` applies the same + * migrations in a deployment. + */ +const GITHUB_PLUGIN_MIGRATIONS = { + dir: path.resolve( + import.meta.dirname, + "../../../../junior-github/migrations", + ), + pluginName: "github", +}; + function assertLocalDatabaseUrl(databaseUrl: string): void { const { hostname } = new URL(databaseUrl); if (hostname !== "localhost" && hostname !== "127.0.0.1") { @@ -46,6 +61,7 @@ export async function setupJuniorPostgresHarness( const executor = createPostgresJuniorSqlExecutor({ connectionString }); try { await migrateSchema(executor); + await migratePluginSchemas(executor, [GITHUB_PLUGIN_MIGRATIONS]); await options.migrateTemplate?.(executor); } finally { await executor.close(); From ab6c225a376220af938b597c5b526106ce52c42d Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:15:58 +0000 Subject: [PATCH 6/7] fix(github): Repair stale installations and keep webhook writes during sync Remove a stored installation when GitHub rejects it as removed or suspended, then sync and retry once. A sync now keeps rows that webhooks wrote while the installation list request ran. Split the issueCredential hook behavior into issue-credential.ts so credential-support.ts stays under the file length limit. --- .../junior-github/src/credential-support.ts | 246 +++++++----------- .../junior-github/src/installations/store.ts | 32 ++- .../junior-github/src/issue-credential.ts | 169 ++++++++++++ packages/junior-github/src/plugin.ts | 2 +- .../junior-github/tests/github-plugin.test.ts | 76 ++++++ 5 files changed, 354 insertions(+), 171 deletions(-) create mode 100644 packages/junior-github/src/issue-credential.ts diff --git a/packages/junior-github/src/credential-support.ts b/packages/junior-github/src/credential-support.ts index 1a9805ab80..95e1ccab99 100644 --- a/packages/junior-github/src/credential-support.ts +++ b/packages/junior-github/src/credential-support.ts @@ -14,13 +14,7 @@ import type { PluginUserTokenSlot, IssueCredentialHookContext, } from "@sentry/junior-plugin-api"; -import type { GitHubDb } from "./db/database.js"; -import { ownerFromLeaseScope } from "./installation-targets.js"; -import { - findGitHubInstallationId, - replaceGitHubInstallations, - type GitHubInstallation, -} from "./installations/store.js"; +import type { GitHubInstallation } from "./installations/store.js"; import { type GitHubAppPermissions, readGrantPermissions, @@ -80,7 +74,8 @@ type TokenResolution = | { ok: true; tokens: PluginStoredTokens } | { ok: false; result: PluginCredentialResult }; -interface UserCredentialOptions { +/** OAuth App settings for GitHub user credentials. */ +export interface UserCredentialOptions { clientIdEnv: string; clientSecretEnv: string; userScope?: string; @@ -97,9 +92,6 @@ export const GITHUB_APP_PRIVATE_KEY_ENV = "GITHUB_APP_PRIVATE_KEY"; export const GITHUB_AUTH_TOKEN_ENV = "GITHUB_TOKEN"; export const GITHUB_AUTH_TOKEN_PLACEHOLDER = "ghp_host_managed_credential"; const MAX_LEASE_MS = 60 * 60 * 1000; -// `installation` webhooks keep stored installations current. A sync only -// repairs missed ones, so a short interval bounds GitHub calls on misses. -const INSTALLATIONS_SYNC_INTERVAL_MS = 60 * 1000; const INSTALLATIONS_PAGE_SIZE = 100; const APP_OWNER_TTL_MS = 60 * 60 * 1000; const GITHUB_API = "https://api.github.com"; @@ -704,7 +696,7 @@ async function refreshUserTokensWithLock( } /** Issue a bounded GitHub user credential for an approved grant. */ -async function issueUserCredential( +export async function issueUserCredential( ctx: IssueCredentialHookContext, options: UserCredentialOptions, ): Promise { @@ -813,13 +805,45 @@ export async function issueInstallationToken( ); } +/** Read an installation's permissions as read grants, cached per lease period. */ +async function installedReadPermissions( + cache: PluginCache, + appJwt: string, + installationId: number, +): Promise> { + return await cache.getOrLoad( + `installation-permissions:${installationId}`, + async () => ({ + value: readInstallationPermissions( + await githubRequest( + GITHUB_API, + `/app/installations/${installationId}`, + { + token: appJwt, + }, + ), + ), + ttlMs: MAX_LEASE_MS, + }), + ); +} + +/** Return whether GitHub rejected an installation that was removed or suspended. */ +export function isInactiveInstallationError(error: unknown): boolean { + return ( + error instanceof GitHubRequestError && + (error.status === 404 || + (error.status === 403 && /suspended/i.test(error.message))) + ); +} + function readLogin(value: unknown): string | undefined { const login = isRecord(value) ? value.login : undefined; return typeof login === "string" && login ? login.toLowerCase() : undefined; } /** Read the lowercase login of the account that owns the App. */ -async function appOwnerLogin( +export async function readAppOwnerLogin( app: GitHubAppEnv, cache: PluginCache, ): Promise { @@ -836,158 +860,64 @@ async function appOwnerLogin( }); } -/** - * Replace the stored installations with the active ones that GitHub lists. - * This fills the table on first use and repairs missed `installation` - * webhooks. It runs at most once per `INSTALLATIONS_SYNC_INTERVAL_MS`. - */ -async function syncInstallations( +/** List every App installation that GitHub reports as active. */ +export async function listActiveInstallations( app: GitHubAppEnv, - db: GitHubDb, - cache: PluginCache, -): Promise { - await cache.getOrLoad("installations-synced", async () => { - const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); - const active: GitHubInstallation[] = []; - for (let page = 1; ; page += 1) { - const installations = await githubRequest( - GITHUB_API, - `/app/installations?per_page=${INSTALLATIONS_PAGE_SIZE}&page=${page}`, - { token: appJwt }, - ); - if (!Array.isArray(installations)) { - throw new Error("GitHub installations response is not a list"); - } - for (const installation of installations) { - if (!isRecord(installation) || installation.suspended_at) continue; - const accountLogin = readLogin(installation.account); - const installationId = installation.id; - if ( - accountLogin && - typeof installationId === "number" && - Number.isSafeInteger(installationId) - ) { - active.push({ accountLogin, installationId }); - } - } - if (installations.length < INSTALLATIONS_PAGE_SIZE) break; +): Promise { + const appJwt = createAppJwt(requireEnv(app.appIdEnv), app.privateKeyEnv); + const active: GitHubInstallation[] = []; + for (let page = 1; ; page += 1) { + const installations = await githubRequest( + GITHUB_API, + `/app/installations?per_page=${INSTALLATIONS_PAGE_SIZE}&page=${page}`, + { token: appJwt }, + ); + if (!Array.isArray(installations)) { + throw new Error("GitHub installations response is not a list"); } - await replaceGitHubInstallations(db, active); - return { value: true, ttlMs: INSTALLATIONS_SYNC_INTERVAL_MS }; - }); -} - -/** Read the installation on one account, and sync once when none is stored. */ -async function installationIdFor( - accountLogin: string, - app: GitHubAppEnv, - ctx: IssueCredentialHookContext, -): Promise { - const db = ctx.db as GitHubDb; - const stored = await findGitHubInstallationId(db, accountLogin); - if (stored) return stored; - await syncInstallations(app, db, ctx.cache); - return await findGitHubInstallationId(db, accountLogin); -} - -/** - * Resolve the App installation for an installation grant from the owner in - * its lease scope. Writes need the owner's own installation. Reads without - * one, or without an owner, use the App owner's installation, which can still - * read public repositories. - */ -async function resolveInstallationId( - ctx: IssueCredentialHookContext, - app: GitHubAppEnv, -): Promise { - const owner = ownerFromLeaseScope(ctx.grant.leaseScope); - if (owner) { - const id = await installationIdFor(owner, app, ctx); - if (id) return id; - if (ctx.grant.access === "write") { - throw new GitHubPluginSetupError( - `The GitHub App is not installed on ${owner}.`, - ); + for (const installation of installations) { + if (!isRecord(installation) || installation.suspended_at) continue; + const accountLogin = readLogin(installation.account); + const installationId = installation.id; + if ( + accountLogin && + typeof installationId === "number" && + Number.isSafeInteger(installationId) + ) { + active.push({ accountLogin, installationId }); + } } + if (installations.length < INSTALLATIONS_PAGE_SIZE) return active; } - const appOwner = await appOwnerLogin(app, ctx.cache); - const defaultId = await installationIdFor(appOwner, app, ctx); - if (!defaultId) { - throw new GitHubPluginSetupError( - `The GitHub App is not installed on ${appOwner}, the account that owns it.`, - ); - } - return defaultId; -} - -/** Read an installation's permissions as read grants, cached per lease period. */ -async function installedReadPermissions( - cache: PluginCache, - appJwt: string, - installationId: number, -): Promise> { - return await cache.getOrLoad( - `installation-permissions:${installationId}`, - async () => ({ - value: readInstallationPermissions( - await githubRequest( - GITHUB_API, - `/app/installations/${installationId}`, - { - token: appJwt, - }, - ), - ), - ttlMs: MAX_LEASE_MS, - }), - ); -} - -interface GitHubCredentialOptions extends GitHubAppEnv, UserCredentialOptions { - /** Declared read permissions. Omit to read the installed ones. */ - readPermissions?: Record; } /** - * Issue the credential for one GitHub grant that host egress selected. Only - * the `issueCredential` hook may call this. Other code reads and writes GitHub - * through host egress. + * Issue a credential lease for one App installation. Read leases get the + * declared read permissions, or the installed ones as read permissions. Write + * leases use the full installed App permissions and repos. Repo allowlisting + * stays in egress policy, not in per-request token minting. */ -export async function issueGitHubCredential( - ctx: IssueCredentialHookContext, - options: GitHubCredentialOptions, -): Promise { - try { - if ( - ctx.grant.name === "installation-read" || - ctx.grant.name === "installation-write" - ) { - const installationId = await resolveInstallationId(ctx, options); - const appJwt = createAppJwt( - requireEnv(options.appIdEnv), - options.privateKeyEnv, - ); - // Write tokens use the full installed App permissions and repos. Repo - // allowlisting stays in egress policy, not in per-request token minting. - const permissions = - ctx.grant.name === "installation-read" - ? (options.readPermissions ?? - (await installedReadPermissions(ctx.cache, appJwt, installationId))) - : undefined; - return createCredentialLease( - await mintInstallationToken(appJwt, installationId, { permissions }), - ); - } - if (USER_TOKEN_GRANTS.has(ctx.grant.name)) { - return await issueUserCredential(ctx, options); - } - } catch (error) { - if (error instanceof GitHubPluginSetupError) { - return credentialUnavailable(error.message); - } - throw error; - } - throw new Error( - `GitHub plugin cannot issue unknown grant "${ctx.grant.name}".`, +export async function issueInstallationLease(input: { + app: GitHubAppEnv; + cache: PluginCache; + installationId: number; + access: "read" | "write"; + readPermissions?: Record; +}): Promise { + const appJwt = createAppJwt( + requireEnv(input.app.appIdEnv), + input.app.privateKeyEnv, + ); + const permissions = + input.access === "read" + ? (input.readPermissions ?? + (await installedReadPermissions( + input.cache, + appJwt, + input.installationId, + ))) + : undefined; + return createCredentialLease( + await mintInstallationToken(appJwt, input.installationId, { permissions }), ); } diff --git a/packages/junior-github/src/installations/store.ts b/packages/junior-github/src/installations/store.ts index 0ea4f939c9..de465ee22c 100644 --- a/packages/junior-github/src/installations/store.ts +++ b/packages/junior-github/src/installations/store.ts @@ -1,4 +1,4 @@ -import { and, eq, ne } from "drizzle-orm"; +import { and, eq, lt, ne } from "drizzle-orm"; import type { GitHubDb } from "../db/database.js"; import { juniorGitHubInstallations } from "../db/schema.js"; @@ -65,21 +65,29 @@ export async function deleteGitHubInstallation( await db.delete(table).where(eq(table.installationId, installationId)); } -/** Replace every row with the complete list of active installations. */ -export async function replaceGitHubInstallations( +/** + * Apply the complete list of active installations that GitHub returned for a + * request sent at `listedAt`. A row written after `listedAt`, such as by an + * `installation` webhook while the request ran, is newer than the list. It + * stays, and a listed installation with the same id or login is skipped. + */ +export async function syncGitHubInstallations( db: GitHubDb, installations: GitHubInstallation[], + listedAt: Date, ): Promise { - const updatedAt = new Date(); await db.transaction(async (tx) => { - await tx.delete(table); + await tx.delete(table).where(lt(table.updatedAt, listedAt)); if (installations.length === 0) return; - await tx.insert(table).values( - installations.map((installation) => ({ - installationId: installation.installationId, - accountLogin: installation.accountLogin.toLowerCase(), - updatedAt, - })), - ); + await tx + .insert(table) + .values( + installations.map((installation) => ({ + installationId: installation.installationId, + accountLogin: installation.accountLogin.toLowerCase(), + updatedAt: listedAt, + })), + ) + .onConflictDoNothing(); }); } diff --git a/packages/junior-github/src/issue-credential.ts b/packages/junior-github/src/issue-credential.ts new file mode 100644 index 0000000000..00b07c6ded --- /dev/null +++ b/packages/junior-github/src/issue-credential.ts @@ -0,0 +1,169 @@ +/** + * GitHub `issueCredential` hook behavior. + * + * This module selects the credential for each GitHub grant. It resolves App + * installations from the stored table and repairs that table from GitHub. + */ +import type { + IssueCredentialHookContext, + PluginCache, + PluginCredentialResult, +} from "@sentry/junior-plugin-api"; +import { + credentialUnavailable, + type GitHubAppEnv, + GitHubPluginSetupError, + isInactiveInstallationError, + issueInstallationLease, + issueUserCredential, + listActiveInstallations, + readAppOwnerLogin, + USER_TOKEN_GRANTS, + type UserCredentialOptions, +} from "./credential-support.js"; +import type { GitHubDb } from "./db/database.js"; +import { ownerFromLeaseScope } from "./installation-targets.js"; +import { + deleteGitHubInstallation, + findGitHubInstallationId, + syncGitHubInstallations, +} from "./installations/store.js"; + +// `installation` webhooks keep stored installations current. A sync only +// repairs missed ones, so a short interval bounds GitHub calls on misses. +const INSTALLATIONS_SYNC_INTERVAL_MS = 60 * 1000; + +/** + * Sync the stored installations with the active ones that GitHub lists. + * This fills the table on first use and repairs missed `installation` + * webhooks. It runs at most once per `INSTALLATIONS_SYNC_INTERVAL_MS`, unless + * `refresh` asks for a new list. + */ +async function syncInstallations( + app: GitHubAppEnv, + db: GitHubDb, + cache: PluginCache, + options?: { refresh?: boolean }, +): Promise { + await cache.getOrLoad( + "installations-synced", + async () => { + const listedAt = new Date(); + const active = await listActiveInstallations(app); + await syncGitHubInstallations(db, active, listedAt); + return { value: true, ttlMs: INSTALLATIONS_SYNC_INTERVAL_MS }; + }, + options, + ); +} + +/** Read the installation on one account, and sync once when none is stored. */ +async function installationIdFor( + accountLogin: string, + app: GitHubAppEnv, + ctx: IssueCredentialHookContext, +): Promise { + const db = ctx.db as GitHubDb; + const stored = await findGitHubInstallationId(db, accountLogin); + if (stored) return stored; + await syncInstallations(app, db, ctx.cache); + return await findGitHubInstallationId(db, accountLogin); +} + +/** + * Resolve the App installation for an installation grant from the owner in + * its lease scope. Writes need the owner's own installation. Reads without + * one, or without an owner, use the App owner's installation, which can still + * read public repositories. + */ +async function resolveInstallationId( + ctx: IssueCredentialHookContext, + app: GitHubAppEnv, +): Promise { + const owner = ownerFromLeaseScope(ctx.grant.leaseScope); + if (owner) { + const id = await installationIdFor(owner, app, ctx); + if (id) return id; + if (ctx.grant.access === "write") { + throw new GitHubPluginSetupError( + `The GitHub App is not installed on ${owner}.`, + ); + } + } + const appOwner = await readAppOwnerLogin(app, ctx.cache); + const defaultId = await installationIdFor(appOwner, app, ctx); + if (!defaultId) { + throw new GitHubPluginSetupError( + `The GitHub App is not installed on ${appOwner}, the account that owns it.`, + ); + } + return defaultId; +} + +interface GitHubCredentialOptions extends GitHubAppEnv, UserCredentialOptions { + /** Declared read permissions. Omit to read the installed ones. */ + readPermissions?: Record; +} + +/** + * Issue the lease for an installation grant. A stored installation can be + * stale when Junior missed its uninstall or suspend webhook. When GitHub + * rejects it, remove the row, sync from GitHub, and try once more. + */ +async function issueInstallationCredential( + ctx: IssueCredentialHookContext, + options: GitHubCredentialOptions, +): Promise { + const db = ctx.db as GitHubDb; + for (let attempt = 1; ; attempt += 1) { + const installationId = await resolveInstallationId(ctx, options); + try { + return await issueInstallationLease({ + app: options, + cache: ctx.cache, + installationId, + access: ctx.grant.name === "installation-read" ? "read" : "write", + readPermissions: options.readPermissions, + }); + } catch (error) { + if (!isInactiveInstallationError(error)) throw error; + await deleteGitHubInstallation(db, installationId); + if (attempt > 1) { + throw new GitHubPluginSetupError( + `The GitHub App installation ${installationId} is no longer active.`, + ); + } + await syncInstallations(options, db, ctx.cache, { refresh: true }); + } + } +} + +/** + * Issue the credential for one GitHub grant that host egress selected. Only + * the `issueCredential` hook may call this. Other code reads and writes GitHub + * through host egress. + */ +export async function issueGitHubCredential( + ctx: IssueCredentialHookContext, + options: GitHubCredentialOptions, +): Promise { + try { + if ( + ctx.grant.name === "installation-read" || + ctx.grant.name === "installation-write" + ) { + return await issueInstallationCredential(ctx, options); + } + if (USER_TOKEN_GRANTS.has(ctx.grant.name)) { + return await issueUserCredential(ctx, options); + } + } catch (error) { + if (error instanceof GitHubPluginSetupError) { + return credentialUnavailable(error.message); + } + throw error; + } + throw new Error( + `GitHub plugin cannot issue unknown grant "${ctx.grant.name}".`, + ); +} diff --git a/packages/junior-github/src/plugin.ts b/packages/junior-github/src/plugin.ts index 2667853453..d9f1ad258c 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -79,13 +79,13 @@ import { GITHUB_APP_PRIVATE_KEY_ENV, GITHUB_AUTH_TOKEN_ENV, GITHUB_AUTH_TOKEN_PLACEHOLDER, - issueGitHubCredential, listVerifiedUserEmails, normalizeScopeList, readEnv, requireEnv, resolveUserAccount, } from "./credential-support.js"; +import { issueGitHubCredential } from "./issue-credential.js"; /** Configure the built-in GitHub plugin manifest and hooks. */ export interface GitHubPluginOptions { diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index c2ee9c2865..1cbab19b75 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -25,6 +25,7 @@ import { gitHubReleaseSourceResource } from "../src/events/release"; import { gitHubRepositoryResource } from "../src/events/repository"; import type { GitHubDb } from "../src/db/database"; import { juniorGitHubInstallations } from "../src/db/schema"; +import { saveGitHubInstallation } from "../src/installations/store"; import { createGitHubFixture, type GitHubFixture } from "./github-db"; import { mswServer } from "./msw"; @@ -2827,6 +2828,81 @@ Conversation: \`local:test:old-conversation\` expect(requests).toEqual([]); }); + it("repairs stale installations without losing concurrent webhook writes", async () => { + const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) + .privateKey.export({ type: "pkcs8", format: "pem" }) + .toString(); + process.env.GITHUB_APP_ID = "123"; + process.env.GITHUB_APP_PRIVATE_KEY = privateKey; + const requests = mockGitHubInstallationApi(); + mswServer.use( + http.get( + "https://api.github.com/app/installations", + async ({ request }) => { + requests.push(await captureRequest(request)); + // FairSource reinstalls the App while Junior lists installations. + await saveGitHubInstallation(db, { + accountLogin: "FairSource", + installationId: 790, + }); + return HttpResponse.json([ + { id: 456, account: { login: "getsentry" } }, + { id: 789, account: { login: "FairSource" } }, + ]); + }, + ), + http.post( + "https://api.github.com/app/installations/999/access_tokens", + async ({ request }) => { + requests.push(await captureRequest(request)); + return HttpResponse.json({ message: "Not Found" }, { status: 404 }); + }, + ), + ); + // Junior missed the webhook that removed this installation. + await db.insert(juniorGitHubInstallations).values({ + accountLogin: "uninstalled", + installationId: 999, + updatedAt: new Date(Date.now() - 60_000), + }); + const result = await githubPlugin().hooks?.issueCredential?.({ + actor: { platform: "system", name: "event" }, + cache: pluginCache(), + grant: { + name: "installation-write", + access: "write", + leaseScope: "owner:uninstalled", + reason: "github.installation-write", + }, + db, + log: pluginLog, + plugin: { name: "github" }, + tokens: {}, + }); + + expect(result).toEqual({ + type: "unavailable", + message: "The GitHub App is not installed on uninstalled.", + }); + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "POST https://api.github.com/app/installations/999/access_tokens", + "GET https://api.github.com/app/installations?per_page=100&page=1", + ]); + // The stale row is gone, and the webhook row is newer than the list. + await expect( + db + .select({ + accountLogin: juniorGitHubInstallations.accountLogin, + installationId: juniorGitHubInstallations.installationId, + }) + .from(juniorGitHubInstallations) + .orderBy(juniorGitHubInstallations.installationId), + ).resolves.toEqual([ + { accountLogin: "getsentry", installationId: 456 }, + { accountLogin: "fairsource", installationId: 790 }, + ]); + }); + it("issues read-only GitHub App installation credentials from plugin hooks", async () => { const privateKey = generateKeyPairSync("rsa", { modulusLength: 2048 }) .privateKey.export({ type: "pkcs8", format: "pem" }) From c80a42a890792c37d21917b9f28e10354d078728 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:49:32 +0000 Subject: [PATCH 7/7] fix(github): Keep removed installations so an older sync cannot add them Uninstall and suspend webhooks, rejected token mints, and syncs now clear the account login instead of deleting the row. The row stays newer than an in-flight sync list, so its insert conflicts and skips the installation. --- .../migrations/0010_installations.sql | 2 +- .../migrations/meta/0010_snapshot.json | 2 +- packages/junior-github/src/db/schema.ts | 14 +++-- .../junior-github/src/installations/store.ts | 63 ++++++++++++++----- .../junior-github/src/issue-credential.ts | 6 +- .../junior-github/src/webhooks/handler.ts | 8 +-- .../junior-github/tests/github-plugin.test.ts | 20 ++++-- .../tests/webhook-outcomes.test.ts | 4 +- 8 files changed, 81 insertions(+), 38 deletions(-) diff --git a/packages/junior-github/migrations/0010_installations.sql b/packages/junior-github/migrations/0010_installations.sql index 64b1b4b27e..e7e5947a26 100644 --- a/packages/junior-github/migrations/0010_installations.sql +++ b/packages/junior-github/migrations/0010_installations.sql @@ -1,6 +1,6 @@ CREATE TABLE "junior_github_installations" ( "installation_id" bigint PRIMARY KEY NOT NULL, - "account_login" text NOT NULL, + "account_login" text, "updated_at" timestamp with time zone NOT NULL, CONSTRAINT "junior_github_installations_account_login_unique" UNIQUE("account_login") ); diff --git a/packages/junior-github/migrations/meta/0010_snapshot.json b/packages/junior-github/migrations/meta/0010_snapshot.json index 7614a8b39d..5857870434 100644 --- a/packages/junior-github/migrations/meta/0010_snapshot.json +++ b/packages/junior-github/migrations/meta/0010_snapshot.json @@ -18,7 +18,7 @@ "name": "account_login", "type": "text", "primaryKey": false, - "notNull": true + "notNull": false }, "updated_at": { "name": "updated_at", diff --git a/packages/junior-github/src/db/schema.ts b/packages/junior-github/src/db/schema.ts index 7e28014a35..0b45edaf7e 100644 --- a/packages/junior-github/src/db/schema.ts +++ b/packages/junior-github/src/db/schema.ts @@ -154,17 +154,21 @@ export const juniorGitHubPullRequestRequesters = pgTable( ); /** - * Active installations of the GitHub App. An App has at most one - * installation per account, so the account login finds the installation for - * a request. `installation` webhooks keep the rows current. A sync from + * Installations of the GitHub App. An App has at most one installation per + * account, so the account login finds the installation for a request. + * `installation` webhooks keep the rows current. A sync from * `GET /app/installations` fills the table and repairs missed webhooks. */ export const juniorGitHubInstallations = pgTable( "junior_github_installations", { installationId: bigint("installation_id", { mode: "number" }).primaryKey(), - /** Lowercase login of the account the App is installed on. */ - accountLogin: text("account_login").notNull().unique(), + /** + * Lowercase login of the account the App is installed on. It is null + * after the installation is removed or suspended. The row stays, so a + * sync with an older list cannot add the installation again. + */ + accountLogin: text("account_login").unique(), updatedAt: timestamp("updated_at", { withTimezone: true }).notNull(), }, ); diff --git a/packages/junior-github/src/installations/store.ts b/packages/junior-github/src/installations/store.ts index de465ee22c..021bea71cf 100644 --- a/packages/junior-github/src/installations/store.ts +++ b/packages/junior-github/src/installations/store.ts @@ -1,4 +1,4 @@ -import { and, eq, lt, ne } from "drizzle-orm"; +import { and, eq, inArray, isNotNull, lt, ne, or } from "drizzle-orm"; import type { GitHubDb } from "../db/database.js"; import { juniorGitHubInstallations } from "../db/schema.js"; @@ -57,37 +57,66 @@ export async function saveGitHubInstallation( }); } -/** Remove an installation that was deleted or suspended. */ -export async function deleteGitHubInstallation( +/** + * Mark an installation as removed, such as after an uninstall or suspend. + * The row stays without a login, so a sync with an older list skips it. + */ +export async function removeGitHubInstallation( db: GitHubDb, installationId: number, ): Promise { - await db.delete(table).where(eq(table.installationId, installationId)); + const updatedAt = new Date(); + await db + .insert(table) + .values({ installationId, accountLogin: null, updatedAt }) + .onConflictDoUpdate({ + target: table.installationId, + set: { accountLogin: null, updatedAt }, + }); } /** * Apply the complete list of active installations that GitHub returned for a * request sent at `listedAt`. A row written after `listedAt`, such as by an - * `installation` webhook while the request ran, is newer than the list. It - * stays, and a listed installation with the same id or login is skipped. + * `installation` webhook or a later sync, is newer than the list. It stays, + * and a listed installation with the same id or login is skipped. Older rows + * that the list omits are marked as removed. */ export async function syncGitHubInstallations( db: GitHubDb, installations: GitHubInstallation[], listedAt: Date, ): Promise { + const rows = installations.map((installation) => ({ + installationId: installation.installationId, + accountLogin: installation.accountLogin.toLowerCase(), + updatedAt: listedAt, + })); + const older = lt(table.updatedAt, listedAt); await db.transaction(async (tx) => { - await tx.delete(table).where(lt(table.updatedAt, listedAt)); - if (installations.length === 0) return; + if (rows.length > 0) { + await tx.delete(table).where( + and( + older, + or( + inArray( + table.installationId, + rows.map((row) => row.installationId), + ), + inArray( + table.accountLogin, + rows.map((row) => row.accountLogin), + ), + ), + ), + ); + } await tx - .insert(table) - .values( - installations.map((installation) => ({ - installationId: installation.installationId, - accountLogin: installation.accountLogin.toLowerCase(), - updatedAt: listedAt, - })), - ) - .onConflictDoNothing(); + .update(table) + .set({ accountLogin: null, updatedAt: listedAt }) + .where(and(older, isNotNull(table.accountLogin))); + if (rows.length > 0) { + await tx.insert(table).values(rows).onConflictDoNothing(); + } }); } diff --git a/packages/junior-github/src/issue-credential.ts b/packages/junior-github/src/issue-credential.ts index 00b07c6ded..3e890f463d 100644 --- a/packages/junior-github/src/issue-credential.ts +++ b/packages/junior-github/src/issue-credential.ts @@ -24,7 +24,7 @@ import { import type { GitHubDb } from "./db/database.js"; import { ownerFromLeaseScope } from "./installation-targets.js"; import { - deleteGitHubInstallation, + removeGitHubInstallation, findGitHubInstallationId, syncGitHubInstallations, } from "./installations/store.js"; @@ -108,7 +108,7 @@ interface GitHubCredentialOptions extends GitHubAppEnv, UserCredentialOptions { /** * Issue the lease for an installation grant. A stored installation can be * stale when Junior missed its uninstall or suspend webhook. When GitHub - * rejects it, remove the row, sync from GitHub, and try once more. + * rejects it, mark the row as removed, sync from GitHub, and try once more. */ async function issueInstallationCredential( ctx: IssueCredentialHookContext, @@ -127,7 +127,7 @@ async function issueInstallationCredential( }); } catch (error) { if (!isInactiveInstallationError(error)) throw error; - await deleteGitHubInstallation(db, installationId); + await removeGitHubInstallation(db, installationId); if (attempt > 1) { throw new GitHubPluginSetupError( `The GitHub App installation ${installationId} is no longer active.`, diff --git a/packages/junior-github/src/webhooks/handler.ts b/packages/junior-github/src/webhooks/handler.ts index 14f95ed58d..d24b91fffb 100644 --- a/packages/junior-github/src/webhooks/handler.ts +++ b/packages/junior-github/src/webhooks/handler.ts @@ -13,7 +13,7 @@ import { z } from "zod"; import type { GitHubDb } from "../db/database.js"; import type { GitHubPullRequestCommitComposition } from "../db/schema.js"; import { - deleteGitHubInstallation, + removeGitHubInstallation, saveGitHubInstallation, } from "../installations/store.js"; import { @@ -115,8 +115,8 @@ const installationEventSchema = z.object({ /** * Keep the stored installations current. Installs, permission changes, and - * unsuspends save the account login. Uninstalls and suspends remove the row, - * because GitHub cannot mint tokens for them. Account renames arrive as + * unsuspends save the account login. Uninstalls and suspends mark the row as + * removed, because GitHub cannot mint tokens for them. Account renames arrive as * `installation_target` deliveries. */ async function recordInstallationEvent( @@ -132,7 +132,7 @@ async function recordInstallationEvent( eventName === "installation" && (action === "deleted" || action === "suspend") ) { - await deleteGitHubInstallation(db, installationId); + await removeGitHubInstallation(db, installationId); return; } const accountLogin = diff --git a/packages/junior-github/tests/github-plugin.test.ts b/packages/junior-github/tests/github-plugin.test.ts index 1cbab19b75..9d7766194d 100644 --- a/packages/junior-github/tests/github-plugin.test.ts +++ b/packages/junior-github/tests/github-plugin.test.ts @@ -25,7 +25,10 @@ import { gitHubReleaseSourceResource } from "../src/events/release"; import { gitHubRepositoryResource } from "../src/events/repository"; import type { GitHubDb } from "../src/db/database"; import { juniorGitHubInstallations } from "../src/db/schema"; -import { saveGitHubInstallation } from "../src/installations/store"; +import { + removeGitHubInstallation, + saveGitHubInstallation, +} from "../src/installations/store"; import { createGitHubFixture, type GitHubFixture } from "./github-db"; import { mswServer } from "./msw"; @@ -2840,10 +2843,12 @@ Conversation: \`local:test:old-conversation\` "https://api.github.com/app/installations", async ({ request }) => { requests.push(await captureRequest(request)); - // FairSource reinstalls the App while Junior lists installations. + // While Junior lists installations, FairSource uninstalls the + // App and Acme installs it. The list does not show either change. + await removeGitHubInstallation(db, 789); await saveGitHubInstallation(db, { - accountLogin: "FairSource", - installationId: 790, + accountLogin: "Acme", + installationId: 800, }); return HttpResponse.json([ { id: 456, account: { login: "getsentry" } }, @@ -2888,7 +2893,8 @@ Conversation: \`local:test:old-conversation\` "POST https://api.github.com/app/installations/999/access_tokens", "GET https://api.github.com/app/installations?per_page=100&page=1", ]); - // The stale row is gone, and the webhook row is newer than the list. + // The stale installation is removed. The webhook rows are newer than + // the list, so the sync keeps them and does not add 789 again. await expect( db .select({ @@ -2899,7 +2905,9 @@ Conversation: \`local:test:old-conversation\` .orderBy(juniorGitHubInstallations.installationId), ).resolves.toEqual([ { accountLogin: "getsentry", installationId: 456 }, - { accountLogin: "fairsource", installationId: 790 }, + { accountLogin: null, installationId: 789 }, + { accountLogin: "acme", installationId: 800 }, + { accountLogin: null, installationId: 999 }, ]); }); diff --git a/packages/junior-github/tests/webhook-outcomes.test.ts b/packages/junior-github/tests/webhook-outcomes.test.ts index a8e297ff74..2118e4df4c 100644 --- a/packages/junior-github/tests/webhook-outcomes.test.ts +++ b/packages/junior-github/tests/webhook-outcomes.test.ts @@ -2120,7 +2120,9 @@ describe("GitHub-owned pull request outcomes", () => { action: "suspend", installation: { id: 789, account: { login: "fair-source" } }, }); - await expect(installations()).resolves.toEqual([]); + await expect(installations()).resolves.toEqual([ + { accountLogin: null, installationId: 789 }, + ]); expect(published).toEqual([]); } finally { await fixture.close();