fix: sync the installation matching GH_ORG during full sync - #1053
Open
DeepDiver1975 wants to merge 1 commit into
Open
fix: sync the installation matching GH_ORG during full sync#1053DeepDiver1975 wants to merge 1 commit into
DeepDiver1975 wants to merge 1 commit into
Conversation
`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 `<other-account>/<ADMIN_REPO>` 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 github-community-projects#782. Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #782.
Problem
syncInstallation()paginatesapps.listInstallationsand then unconditionally takes the first entry, deriving the admin repo owner from it:https://github.com/github-community-projects/safe-settings/blob/main-enterprise/index.js#L234-L246
So when an app is installed on more than one account, which account a full sync reconciles is decided by the order
GET /app/installationshappens to return. That order is not documented as stable, and it is not creation order — in our case the API returned an unrelated account ahead of ours even though its installation id was higher (<other-account>#152779721before<ours>#138202905).This is not only about deployments that intend to manage several orgs.
GET /app/installationslists installations for the app, and anyone can install a public app. Our app was public, an unrelated organisation installed it, and it took over index 0. From then on every scheduled sync read<other-account>/admin/.github/settings.yml, got a 404, and failed — our org silently went unmanaged for about two days. Had that account happened to contain anadminrepo with a settings file, the sync would have applied their configuration to their org using our app's credentials. The operator has no way to express "only ever sync this account".The natural way to say that already exists:
GH_ORG. The GitHub Action recipe indocs/github-action.mdsets it in the full-sync workflow, so operators reasonably assume it scopes the sync — but nothing read it. It was absent fromlib/env.jsand only consumed by the probot manifest flow.Change
GH_ORGtolib/env.js.GH_ORGis set, select the installation whose account login matches it, case-insensitively (GitHub account names are).GH_ORGis set but the app has no installation on it,throwinstead of silently falling back, so a mistargeted sync fails loudly rather than reconciling another account. The message lists the accounts the app is installed on, which is what one needs to debug it.full-sync.jsalready turns a throw into a non-zero exit with the message.GH_ORGis not set, behaviour is unchanged (installations[0]). The change is opt-in and no existing deployment is affected.infowhich installation and account is being synced. Nothing on the full-sync path recorded the account it acted on, which is a large part of why the above took two days to spot.info()is deliberately left alone: it authenticates as an arbitrary installation purely to read the app slug, which is a property of the app, not of any one installation.Relationship to earlier attempts
#783 (@hilmarf) proposed the same idea and was closed by its author after going stale. Two things blocked it, both addressed here:
GH_ORG"is not a required env variable so in most cases it would not be set". filter installations by GH_ORG #783 made thefind()unconditional, so an unsetGH_ORGwould have matched nothing and every existing deployment would have stopped syncing. Here the filter only applies whenGH_ORGis set, andinstallations[0]remains the default — which is also what @Simon-Boyer suggested on that thread.env.GH_ORG, butGH_ORGwas never inlib/env.js, so it wasundefinedregardless of the environment. This PR adds it.#1044 makes full sync iterate all installations. The two are complementary rather than competing: that PR is about not skipping orgs you own, this one is about not syncing accounts you don't. If #1044 lands,
GH_ORGbecomes a filter over its loop rather than a pick-one, and I am happy to rebase into that shape.The failure mode this creates is much easier to read once #1052 stops
configManagerfrom masking config-read errors asTypeError: Cannot read properties of undefined (reading 'data'); that is what the 404 above surfaced as. The two PRs are independent and touch different files.Tests
New
test/unit/sync-installation.test.js(7 tests), driving the exported plugin with a fakerobotand the injectableSettingsargument, asserting on therepohanded toSettings.syncAll:GH_ORGmatching the second -> that one is synced, and the context is authenticated as itGH_ORGdiffering in case from the account name -> still matchedGH_ORGwith no matching installation -> rejects, listing the installed accounts, and nothing is syncedGH_ORGunset -> falls back to the first installationnull, nothing syncednopflag is passed throughReverting
index.jsandlib/env.jstomain-enterprisefails 4 of the 7, so the suite pins the new behaviour rather than passing alongside it.test/unit/lib/env.test.jsgainsGH_ORGcoverage in both the default and override blocks (1 further failure when reverted).Node 22.12.0.
Heads-up so it is not attributed to this PR:
npx eslint index.js lib/env.jsreportsindex.js:5:7 'Glob' is assigned a value but never usedand acomma-dangleerror inlib/env.js. Both are pre-existing onmain-enterprise(verified by linting the pristine files; my added line only shifts theenv.jsone from line 11 to 12). Fixing them felt like it belonged in a separate cleanup PR.Docs
README.md(environment variables),.env.exampleanddocs/github-action.mdnow state whatGH_ORGdoes for a full sync.Possible follow-up
The
CRONtick callssyncInstallation()without a rejection handler, so in the server flow this throw would become an unhandled rejection — as would any error thrown fromsyncAllSettingstoday, so it is not new. Adding a.catch()there seemed out of scope, but I am happy to include it if you would prefer.