From 020145722fb1251f53d7331ccba6bdbc56217a60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:40:06 +0200 Subject: [PATCH] fix: sync the installation matching GH_ORG during full sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `syncInstallation()` authenticated as `installations[0]` and derived the admin repo owner from it, so the account a full sync reconciled depended on the order `GET /app/installations` happened to return. The order is not documented as stable, and it is not creation order, so an app installed on more than one account can silently switch to a different account: the sync then reads its configuration from `/` and, if that repo exists, applies it there. `GH_ORG` is the natural way to express which account to sync. The GitHub Action recipe in docs/github-action.md already tells operators to set it, but nothing read it: it was absent from lib/env.js and only consumed by the manifest flow. So add `GH_ORG` to lib/env.js and, when it is set, select the installation whose account login matches it (case-insensitively, as GitHub account names are). If it is set and the app has no installation on it, throw rather than fall back, so a misconfigured sync fails loudly instead of reconciling somebody else's account. When `GH_ORG` is unset the behavior is unchanged, so this is opt-in and no existing deployment changes. Also log at info level which installation and account is being synced. Previously nothing on the full-sync path recorded the account it acted on, which made a mistargeted sync hard to spot. `info()` is deliberately left alone: it authenticates as an arbitrary installation only to read the app slug, which is a property of the app rather than of any installation. Fixes #782. Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- .env.example | 2 + README.md | 13 +++ docs/github-action.md | 2 + index.js | 20 +++- lib/env.js | 1 + test/unit/lib/env.test.js | 8 ++ test/unit/sync-installation.test.js | 171 ++++++++++++++++++++++++++++ 7 files changed, 215 insertions(+), 2 deletions(-) create mode 100644 test/unit/sync-installation.test.js diff --git a/.env.example b/.env.example index d98f7d4b9..568b1ac46 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,8 @@ # The organization where you want to register the app in the app creation manifest flow. # If set, the app is registered for an organization (https://github.com/organizations/ORGANIZATION/settings/apps/new), # if not set, the GitHub app would be registered for the user account (https://github.com/settings/apps/new). +# It also scopes the full sync: the installation on this account is the one that +# gets synced, instead of whichever installation the API lists first. # GH_ORG= # The ID of your GitHub App diff --git a/README.md b/README.md index 07a626748..2020bc12d 100644 --- a/README.md +++ b/README.md @@ -549,6 +549,19 @@ You can pass environment variables; the easiest way to do it is via a `.env` fil ``` BLOCK_REPO_RENAME_BY_HUMAN=true ``` +1. Scope the full sync to one account using `GH_ORG`. For e.g. + ``` + GH_ORG=my-org + ``` + If the app is installed on more than one account, a full sync (`CRON` or + `npm run full-sync`) picks the installation on the `GH_ORG` account and reads + its configuration from `GH_ORG/`. If `GH_ORG` is set but the app + is not installed on it, the full sync fails instead of syncing a different + account. When `GH_ORG` is not set, the first installation returned by the API + is used, so setting it is recommended whenever the app is installed on more + than one account. Note that `GH_ORG` is also used by the + [manifest flow](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest) + to decide which account the app is registered for. ### Runtime Settings diff --git a/docs/github-action.md b/docs/github-action.md index 5424b7cc8..b274a5db1 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -14,6 +14,8 @@ Running a full-sync with `safe-settings` can be done via `npm run full-sync`. Th ### Example GHA Workflow The below example uses the GHA "cron" feature to run a full-sync every 4 hours. While not required, this example uses the `.github` repo as the `admin` repo (set via `ADMIN_REPO` env var) and the safe-settings configurations are stored in the `safe-settings/` directory (set via `CONFIG_PATH` and `DEPLOYMENT_CONFIG_FILE`). +`GH_ORG` names the account to sync. If the App is installed on more than one account, set it: it selects the installation to sync, and the full-sync fails rather than syncing a different account if the App is not installed on it. + ```yaml name: Safe Settings Sync on: diff --git a/index.js b/index.js index 12aae7c79..beaf686c5 100644 --- a/index.js +++ b/index.js @@ -231,8 +231,24 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => github.rest.apps.listInstallations.endpoint.merge({ per_page: 100 }) ) - if (installations.length > 0) { - const installation = installations[0] + // When GH_ORG is set, sync the installation on that account instead of + // whichever one the API happens to list first. The order of + // `GET /app/installations` is not guaranteed to keep the account you care + // about at index 0, so an app installed on more than one account can + // otherwise start reading its config from, and applying settings to, a + // different account than the operator intended. Without GH_ORG the + // behavior is unchanged. + const installation = env.GH_ORG + ? installations.find(i => i.account?.login?.toLowerCase() === env.GH_ORG.toLowerCase()) + : installations[0] + + if (env.GH_ORG && !installation) { + const accounts = installations.map(i => i.account?.login).join(', ') + throw new Error(`No app installation found for GH_ORG '${env.GH_ORG}'. Installed on: [${accounts}]`) + } + + if (installation) { + robot.log.info(`Syncing installation ${installation.id} on account ${installation.account?.login}`) const github = await robot.auth(installation.id) const context = { payload: { diff --git a/lib/env.js b/lib/env.js index 8ed5d927e..f21d86e17 100644 --- a/lib/env.js +++ b/lib/env.js @@ -7,6 +7,7 @@ module.exports = { CREATE_ERROR_ISSUE: process.env.CREATE_ERROR_ISSUE || 'true', BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false', FULL_SYNC_NOP: process.env.FULL_SYNC_NOP === 'true', + GH_ORG: process.env.GH_ORG, GHE_HOST: process.env.GHE_HOST, GHE_PROTOCOL: process.env.GHE_PROTOCOL, } diff --git a/test/unit/lib/env.test.js b/test/unit/lib/env.test.js index d345fe543..6dd464b57 100644 --- a/test/unit/lib/env.test.js +++ b/test/unit/lib/env.test.js @@ -32,6 +32,11 @@ describe('env', () => { const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP expect(FULL_SYNC_NOP).toEqual(false) }) + + it('leaves GH_ORG undefined if not passed', () => { + const GH_ORG = envTest.GH_ORG + expect(GH_ORG).toBeUndefined() + }) }) describe('load override values', () => { @@ -43,6 +48,7 @@ describe('env', () => { process.env.DEPLOYMENT_CONFIG_FILE = 'safe-settings-deployment.yml' process.env.CREATE_PR_COMMENT = 'false' process.env.FULL_SYNC_NOP = false + process.env.GH_ORG = 'my-org' }) it('loads override values if passed', () => { @@ -59,6 +65,8 @@ describe('env', () => { expect(CREATE_PR_COMMENT).toEqual('false') const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP expect(FULL_SYNC_NOP).toEqual(false) + const GH_ORG = envTest.GH_ORG + expect(GH_ORG).toEqual('my-org') }) }) }) diff --git a/test/unit/sync-installation.test.js b/test/unit/sync-installation.test.js new file mode 100644 index 000000000..59742b124 --- /dev/null +++ b/test/unit/sync-installation.test.js @@ -0,0 +1,171 @@ +/* eslint-disable no-undef */ +const path = require('path') + +// The plugin reads GH_ORG through lib/env, which snapshots process.env at +// require time, so each scenario needs a freshly required copy of both. +function loadPlugin (ghOrg) { + jest.resetModules() + if (ghOrg === undefined) { + delete process.env.GH_ORG + } else { + process.env.GH_ORG = ghOrg + } + return require('../../index') +} + +function installation (id, login) { + return { id, account: { login } } +} + +describe('syncInstallation', () => { + const originalGhOrg = process.env.GH_ORG + const originalDeploymentConfigFile = process.env.DEPLOYMENT_CONFIG_FILE + let robot, octokit, syncAll + + beforeAll(() => { + // Point the deployment config at a file that does not exist so + // loadYamlFileSystem() falls back to its built-in defaults instead of + // picking up whatever happens to sit in the working directory. + process.env.DEPLOYMENT_CONFIG_FILE = path.join(__dirname, 'no-such-deployment-settings.yml') + }) + + afterAll(() => { + if (originalGhOrg === undefined) { + delete process.env.GH_ORG + } else { + process.env.GH_ORG = originalGhOrg + } + if (originalDeploymentConfigFile === undefined) { + delete process.env.DEPLOYMENT_CONFIG_FILE + } else { + process.env.DEPLOYMENT_CONFIG_FILE = originalDeploymentConfigFile + } + }) + + beforeEach(() => { + octokit = { + paginate: jest.fn(), + rest: { + apps: { + listInstallations: { endpoint: { merge: jest.fn(options => options) } }, + getAuthenticated: jest.fn().mockResolvedValue({ data: { slug: 'safe-settings' } }) + }, + repos: { + // The global settings file is irrelevant here: these tests assert + // which installation is selected, not what gets synced. + getContent: jest.fn().mockResolvedValue({ data: { content: '' } }) + } + } + } + robot = { + auth: jest.fn().mockResolvedValue(octokit), + log: Object.assign(jest.fn(), { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + trace: jest.fn() + }), + on: jest.fn() + } + syncAll = jest.fn().mockResolvedValue({ errors: [] }) + }) + + // Returns the `repo` argument Settings.syncAll was called with, i.e. the + // admin repo of the account safe-settings decided to sync. + function syncedRepo () { + expect(syncAll).toHaveBeenCalledTimes(1) + return syncAll.mock.calls[0][2] + } + + it('syncs the installation matching GH_ORG, not the first one listed', async () => { + const plugin = loadPlugin('my-org') + octokit.paginate.mockResolvedValue([ + installation(1, 'another-account'), + installation(2, 'my-org') + ]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + await app.syncInstallation() + + expect(syncedRepo()).toEqual({ owner: 'my-org', repo: 'admin' }) + // The context handed to syncAll must be authenticated as the GH_ORG + // installation. (info() separately authenticates as installations[0] to + // read the app slug; that is left as-is, see the PR description.) + expect(robot.auth).toHaveBeenLastCalledWith(2) + }) + + it('matches GH_ORG case-insensitively, as GitHub account names are', async () => { + const plugin = loadPlugin('My-Org') + octokit.paginate.mockResolvedValue([ + installation(1, 'another-account'), + installation(7, 'my-org') + ]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + await app.syncInstallation() + + expect(syncedRepo()).toEqual({ owner: 'my-org', repo: 'admin' }) + }) + + it('throws when GH_ORG has no installation instead of syncing another account', async () => { + const plugin = loadPlugin('my-org') + octokit.paginate.mockResolvedValue([ + installation(1, 'another-account'), + installation(2, 'yet-another-account') + ]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + + await expect(app.syncInstallation()).rejects.toThrow( + "No app installation found for GH_ORG 'my-org'. Installed on: [another-account, yet-another-account]" + ) + expect(syncAll).not.toHaveBeenCalled() + }) + + it('falls back to the first installation when GH_ORG is not set', async () => { + const plugin = loadPlugin(undefined) + octokit.paginate.mockResolvedValue([ + installation(1, 'first-account'), + installation(2, 'second-account') + ]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + await app.syncInstallation() + + expect(syncedRepo()).toEqual({ owner: 'first-account', repo: 'admin' }) + }) + + it('returns null without syncing when the app has no installations', async () => { + const plugin = loadPlugin(undefined) + octokit.paginate.mockResolvedValue([]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + + await expect(app.syncInstallation()).resolves.toBeNull() + expect(syncAll).not.toHaveBeenCalled() + }) + + it('passes the nop flag through to the sync', async () => { + const plugin = loadPlugin('my-org') + octokit.paginate.mockResolvedValue([installation(2, 'my-org')]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + await app.syncInstallation(true) + + expect(syncAll).toHaveBeenCalledWith(true, expect.anything(), expect.anything(), expect.anything()) + }) + + it('logs which account is being synced', async () => { + const plugin = loadPlugin('my-org') + octokit.paginate.mockResolvedValue([ + installation(1, 'another-account'), + installation(2, 'my-org') + ]) + + const app = plugin(robot, {}, { syncAll, handleError: jest.fn() }) + await app.syncInstallation() + + expect(robot.log.info).toHaveBeenCalledWith('Syncing installation 2 on account my-org') + }) +})