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 b0527f2f0d..07a78bf80a 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,21 @@ 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. 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 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 +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 @@ -231,18 +244,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 @@ -562,8 +563,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`: 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-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 9b6e7dcf60..2afccfdfd7 100644 --- a/packages/junior-evals/src/fixture/agent.ts +++ b/packages/junior-evals/src/fixture/agent.ts @@ -941,7 +941,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..308feeb6d7 100644 --- a/packages/junior-github/SETUP.md +++ b/packages/junior-github/SETUP.md @@ -42,13 +42,16 @@ 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 uses the +installation on the account that owns the target repository or organization. +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) @@ -61,7 +64,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 @@ -162,7 +164,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 @@ -246,7 +248,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/migrations/0010_installations.sql b/packages/junior-github/migrations/0010_installations.sql new file mode 100644 index 0000000000..e7e5947a26 --- /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, + "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..5857870434 --- /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": false + }, + "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 6daa48f09a..95e1ccab99 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 type { GitHubInstallation } from "./installations/store.js"; import { type GitHubAppPermissions, readGrantPermissions, @@ -72,49 +74,27 @@ 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; } -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; +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; export const GITHUB_GRAPHQL_RESPONSE_BODY_LIMIT_BYTES = 64 * 1024; @@ -211,10 +191,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 +515,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, @@ -750,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 { @@ -823,134 +769,155 @@ 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 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 InstallationCredentialBaseOptions, UserCredentialOptions { - /** Declared read permissions, or a loader for the installed ones. */ - readPermissions: - | { permissions: GitHubAppPermissions } - | { loadPermissions: LoadInstallationReadPermissions }; +/** 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))) + ); } -/** - * 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 { - 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); +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. */ +export async function readAppOwnerLogin( + 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"); } - if (USER_TOKEN_GRANTS.has(ctx.grant.name)) { - return await issueUserCredential(ctx, options); + return { value: login, ttlMs: APP_OWNER_TTL_MS }; + }); +} + +/** List every App installation that GitHub reports as active. */ +export async function listActiveInstallations( + app: GitHubAppEnv, +): 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"); } - } catch (error) { - if (error instanceof GitHubPluginSetupError) { - return credentialUnavailable(error.message); + 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 }); + } } - throw error; + if (installations.length < INSTALLATIONS_PAGE_SIZE) return active; } - throw new Error( - `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; - }; +/** + * 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 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/db/schema.ts b/packages/junior-github/src/db/schema.ts index 93d4943446..0b45edaf7e 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,28 @@ export const juniorGitHubPullRequestRequesters = pgTable( ], ); +/** + * 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. 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(), + }, +); + export const githubSqlSchema = { + juniorGitHubInstallations, juniorGitHubIssues, juniorGitHubPullRequestIssues, juniorGitHubPullRequestRequesters, diff --git a/packages/junior-github/src/egress-policy.ts b/packages/junior-github/src/egress-policy.ts index f87a28e691..9e7d6722eb 100644 --- a/packages/junior-github/src/egress-policy.ts +++ b/packages/junior-github/src/egress-policy.ts @@ -17,12 +17,15 @@ 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 { + githubOwnerLeaseScope, + githubRepositoryFromUrl, +} from "./installation-targets.js"; function githubSmartHttpAccess( upstreamUrl: URL, @@ -588,17 +591,66 @@ function grantForAccess( access: PluginGrantAccess, reason: GitHubGrantReason, name: GitHubGrantName, + owner?: string, ): GitHubGrant { return { name, access, reason, + ...(owner ? { leaseScope: githubOwnerLeaseScope(owner) } : undefined), ...(name === "user-write" ? { requirements: USER_WRITE_REQUIREMENTS } : undefined), }; } +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 GraphQL `owner` variable, as `gh` sends it for repository reads. */ +function graphqlOwnerVariable( + bodyText: string | undefined, +): string | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(bodyText ?? ""); + } catch { + return undefined; + } + const owner = + isRecord(parsed) && isRecord(parsed.variables) + ? parsed.variables.owner + : undefined; + return typeof owner === "string" && GITHUB_LOGIN_PATTERN.test(owner) + ? owner + : undefined; +} + +/** + * 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 installationOwner( + upstreamUrl: URL, + 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) + ? graphqlOwnerVariable(request.bodyText) + : 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 owner = installationOwner(upstreamUrl, ctx.request); const smartHttpAccess = githubSmartHttpAccess(upstreamUrl); if (smartHttpAccess) { @@ -635,12 +688,14 @@ export async function githubGrantForEgress( "write", "github.installation-write", "installation-write", + owner, ); } return grantForAccess( smartHttpAccess, "github.git-read", "installation-read", + owner, ); } @@ -660,6 +715,7 @@ export async function githubGrantForEgress( ? "github.user-write" : "github.installation-write", writeGrantName, + writeGrantName === "installation-write" ? owner : undefined, ); } @@ -674,6 +730,7 @@ export async function githubGrantForEgress( "write", "github.installation-write", "installation-write", + hostMutationRepository.split("/")[0], ); } @@ -692,6 +749,7 @@ export async function githubGrantForEgress( graphqlAccess, "github.graphql-read", "installation-read", + owner, ); } @@ -701,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"); + 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 new file mode 100644 index 0000000000..546df9b799 --- /dev/null +++ b/packages/junior-github/src/installation-targets.ts @@ -0,0 +1,54 @@ +/** + * GitHub App installation targets. + * + * 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. + */ + +const OWNER_LEASE_SCOPE_PREFIX = "owner:"; + +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 owner's installation lease apart. */ +export function githubOwnerLeaseScope(owner: string): string { + return `${OWNER_LEASE_SCOPE_PREFIX}${owner.toLowerCase()}`; +} + +/** Read the lowercase owner from a grant lease scope, if it names one. */ +export function ownerFromLeaseScope( + leaseScope: string | undefined, +): 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/installations/store.ts b/packages/junior-github/src/installations/store.ts new file mode 100644 index 0000000000..021bea71cf --- /dev/null +++ b/packages/junior-github/src/installations/store.ts @@ -0,0 +1,122 @@ +import { and, eq, inArray, isNotNull, lt, ne, or } 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() }, + }); + }); +} + +/** + * 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 { + 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 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) => { + 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 + .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 new file mode 100644 index 0000000000..3e890f463d --- /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 { + removeGitHubInstallation, + 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, mark the row as removed, 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 removeGitHubInstallation(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 6f486dc36c..d9f1ad258c 100644 --- a/packages/junior-github/src/plugin.ts +++ b/packages/junior-github/src/plugin.ts @@ -79,15 +79,13 @@ import { GITHUB_APP_PRIVATE_KEY_ENV, GITHUB_AUTH_TOKEN_ENV, GITHUB_AUTH_TOKEN_PLACEHOLDER, - GITHUB_INSTALLATION_ID_ENV, - createPermissionCache, - 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 { @@ -126,8 +124,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. */ @@ -144,13 +140,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; @@ -214,7 +207,6 @@ export function githubPlugin( envVars: { [appIdEnv]: {}, [privateKeyEnv]: {}, - [installationIdEnv]: {}, [clientIdEnv]: {}, [clientSecretEnv]: {}, [botNameEnv]: { exposeToCommandEnv: true }, @@ -330,11 +322,7 @@ export function githubPlugin( }; }, routes(ctx) { - const installationEnv = { - appIdEnv, - installationIdEnv, - privateKeyEnv, - }; + const app = { appIdEnv, privateKeyEnv }; return [ createGitHubWebhookRoute({ annotations: ctx.annotations, @@ -343,26 +331,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"), @@ -467,11 +450,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/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/check-suite.ts b/packages/junior-github/src/webhooks/check-suite.ts index f026d1a925..bb2c9b5a28 100644 --- a/packages/junior-github/src/webhooks/check-suite.ts +++ b/packages/junior-github/src/webhooks/check-suite.ts @@ -583,7 +583,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; @@ -599,7 +599,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], @@ -627,7 +627,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..d24b91fffb 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 { + removeGitHubInstallation, + 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 mark the row as + * removed, 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 removeGitHubInstallation(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(), @@ -108,6 +152,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 +196,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; + markFeedbackReviewing( + input: InstallationScoped, + ): Promise; privateKeyEnv: string; events: EventPublisher; webhookSecret(): string | undefined; @@ -190,15 +238,16 @@ 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, - }); - } - if (webhookInstallationId(body) !== installationId) { return new Response("Ignored", { status: 202 }); } + if (eventName === "installation" || eventName === "installation_target") { + await recordInstallationEvent(args.db, eventName, installationId, body); + return new Response("Accepted", { status: 202 }); + } const botEmail = args.botEmail(); const pullRequestOutcome = eventName === "pull_request" @@ -259,6 +308,7 @@ export function createGitHubWebhookRoute(args: { let commitComposition: GitHubPullRequestCommitComposition | undefined; try { commitComposition = await args.classifyPullRequestCommits({ + installationId, number: pullRequestOutcome.number, repositoryFullName: pullRequestOutcome.repositoryFullName, }); @@ -334,7 +384,7 @@ export function createGitHubWebhookRoute(args: { ? await loadCheckSuiteFacts({ appIdEnv: args.appIdEnv, body, - installationIdEnv: args.installationIdEnv, + installationId, loadPullRequestFacts: needsCheckSuitePullRequestFacts(checkSuiteMatchKeys), log: args.log, @@ -342,9 +392,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 +425,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 +453,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-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 0ad740d34f..9d7766194d 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,11 @@ 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 { + removeGitHubInstallation, + saveGitHubInstallation, +} from "../src/installations/store"; import { createGitHubFixture, type GitHubFixture } from "./github-db"; import { mswServer } from "./msw"; @@ -36,6 +42,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(); }); @@ -146,6 +157,23 @@ 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 }>, + options?: { refresh?: boolean }, + ): Promise { + 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; + }, + }; +} + /** * 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 +262,29 @@ function cloneStateValue(value: T): T { return value === undefined ? value : structuredClone(value); } +/** + * 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[] = []; 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/app/installations", + async ({ request }) => { + requests.push(await captureRequest(request)); + return HttpResponse.json([ + { id: 456, account: { login: "getsentry" } }, + { id: 789, account: { login: "FairSource" } }, + ]); + }, + ), http.get( "https://api.github.com/app/installations/:installationId", async ({ request }) => { @@ -757,8 +805,18 @@ describe("github plugin", () => { ).toMatchObject({ name: "installation-read", access: "read", + leaseScope: "owner:getsentry", reason: "github.api-read", }); + expect( + await grantForEgress({ + method: "GET", + url: "https://api.github.com/orgs/FairSource/teams", + }), + ).toMatchObject({ + name: "installation-read", + leaseScope: "owner:fairsource", + }); expect( await grantForEgress({ method: "POST", @@ -2255,6 +2313,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: "owner:fairsource", + reason: "github.graphql-read", + }); expect( await grantForEgress({ method: "POST", @@ -2326,15 +2399,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", @@ -2683,12 +2774,11 @@ Conversation: \`local:test:old-conversation\` ); }); - it("issues installation-write credentials without repository filter", 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(); 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({ @@ -2696,12 +2786,97 @@ Conversation: \`local:test:old-conversation\` contents: "write", }, }); + const grant = await grantForEgress({ + 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, + grant: { ...grant!, leaseScope }, + db, + log: pluginLog, + plugin: { name: "github" }, + tokens: {}, + }); - const result = await plugin.hooks?.issueCredential?.({ + 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/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({}); + + // 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. 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).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)); + // 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: "Acme", + installationId: 800, + }); + 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, @@ -2710,14 +2885,30 @@ Conversation: \`local:test:old-conversation\` tokens: {}, }); - 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(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 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({ + accountLogin: juniorGitHubInstallations.accountLogin, + installationId: juniorGitHubInstallations.installationId, + }) + .from(juniorGitHubInstallations) + .orderBy(juniorGitHubInstallations.installationId), + ).resolves.toEqual([ + { accountLogin: "getsentry", installationId: 456 }, + { accountLogin: null, installationId: 789 }, + { accountLogin: "acme", installationId: 800 }, + { accountLogin: null, installationId: 999 }, + ]); }); it("issues read-only GitHub App installation credentials from plugin hooks", async () => { @@ -2725,7 +2916,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(); @@ -2736,18 +2926,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", @@ -2763,10 +2958,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 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/app/installations?per_page=100&page=1", + "POST https://api.github.com/app/installations/456/access_tokens", + ]); + expect(requests[2]).toMatchObject({ body: { permissions: { contents: "read", @@ -2775,6 +2973,17 @@ Conversation: \`local:test:old-conversation\` }, }, }); + + // Public repositories stay readable where the App is not installed. + requests.length = 0; + await expect(issue("owner:uninstalled")).resolves.toMatchObject({ + type: "lease", + }); + expect(requests.map(({ method, url }) => `${method} ${url}`)).toEqual([ + "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", + ]); }); it("caches implicit GitHub App installation permissions", async () => { @@ -2782,7 +2991,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(); @@ -2790,9 +2998,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: "owner:getsentry", reason: "github.api-read", }, db, @@ -2804,15 +3014,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.startsWith("https://api.github.com/app"), ); const tokenRequests = requests.filter((request) => request.url.endsWith("/app/installations/456/access_tokens"), ); - expect(permissionReads).toHaveLength(1); + // 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/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 50420d0014..2118e4df4c 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, @@ -296,8 +297,6 @@ function webhookRoute( async record() {}, }, db: fixture.db(), - installationId: () => "456", - installationIdEnv: "GITHUB_INSTALLATION_ID", log: { error }, markFeedbackReviewing, privateKeyEnv: "GITHUB_APP_PRIVATE_KEY", @@ -1910,6 +1909,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, @@ -1929,6 +1930,7 @@ describe("GitHub-owned pull request outcomes", () => { expect(markFeedbackReviewing).toHaveBeenCalledWith({ commentId: 101, commentKind: "conversation", + installationId: 999, repo: "getsentry/junior", }); expect(published).toHaveLength(2); @@ -2057,7 +2059,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 { @@ -2065,7 +2067,7 @@ describe("GitHub-owned pull request outcomes", () => { const response = await route.handler( signedRequest({ ...pullRequestPayload(), - installation: { id: 999 }, + installation: undefined, }), ); @@ -2079,6 +2081,54 @@ describe("GitHub-owned pull request outcomes", () => { } }); + 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); + + 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([ + { accountLogin: null, installationId: 789 }, + ]); + expect(published).toEqual([]); + } finally { + await fixture.close(); + } + }); + it.each([ "sentry-junior@example.com", "264270552+human@users.noreply.github.com", @@ -2132,7 +2182,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({ @@ -2192,7 +2241,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", @@ -2296,7 +2344,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", @@ -2421,7 +2468,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", @@ -2560,7 +2606,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", diff --git a/packages/junior-plugin-api/src/credentials.ts b/packages/junior-plugin-api/src/credentials.ts index 4274564957..a751b9b10a 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 GitHub account. 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/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(); 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..0baee33251 100644 --- a/packages/junior/tests/msw/handlers/github-api.ts +++ b/packages/junior/tests/msw/handlers/github-api.ts @@ -9,6 +9,20 @@ export const githubApiHandlers = [ http.get(`${GITHUB_API_ORIGIN}/repos/:owner/:repo/deployments`, () => HttpResponse.json([]), ), + // 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`, () => 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 }, });