From e0649fdcb07e81df32915a6e2bbb758d1785091d Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 16:48:44 -0400 Subject: [PATCH 1/8] fix: scope GitHub App tokens by owner GitHub App installation tokens are account-scoped, but middleman previously treated an app entry as if it covered an entire host. That made the install CLI warn about unrelated owner repos and risked using or validating an installation outside the repository owner it can actually read. This keeps app candidates in the host credential chain while resolving them only when the request or startup validation carries a matching GitHub owner. Multiple installations for the same host can now coexist, and install no longer reports other-owner repos as unreachable. Validation: go test ./internal/tokenauth ./internal/config ./internal/github/... ./internal/server/e2etest ./cmd/middleman ./cmd/middleman-github-app -shuffle=on; make test-short; ./middleman-github-app list; scripts/context-sync --check; git diff --check --cached Generated with Codex Co-authored-by: Codex --- cmd/middleman-github-app/create.go | 94 +++------------ cmd/middleman-github-app/e2e_test.go | 96 +++++++++------ cmd/middleman-github-app/manage.go | 3 +- cmd/middleman-github-app/shared.go | 39 +++++- cmd/middleman/provider_startup.go | 25 ++-- cmd/middleman/startup_github_app_e2e_test.go | 36 +++++- context/github-sync-invariants.md | 33 +++++ internal/config/config.go | 114 +++++++++++------- internal/config/github_app_config_test.go | 54 ++++++++- internal/github/client.go | 40 ++++++ internal/github/graphql.go | 3 + internal/githubapp/githubapptest/fake.go | 12 ++ internal/githubapp/jwt_test.go | 21 +++- internal/githubapp/manifest.go | 2 +- .../e2etest/github_app_split_auth_test.go | 18 ++- internal/tokenauth/descriptor.go | 18 ++- internal/tokenauth/github_app_test.go | 73 +++++++++++ internal/tokenauth/source.go | 61 +++++++--- internal/tokenauth/transport.go | 7 +- 19 files changed, 547 insertions(+), 202 deletions(-) diff --git a/cmd/middleman-github-app/create.go b/cmd/middleman-github-app/create.go index ddeb5b460..db53d31d4 100644 --- a/cmd/middleman-github-app/create.go +++ b/cmd/middleman-github-app/create.go @@ -396,21 +396,6 @@ func (env *appEnv) runInstallFlow( break } } - if refreshed { - // Refreshing cannot help when the installation sits on an - // account that does not own the configured repos — the - // repair for that is installing on the right account, so - // fall through to waiting for a new installation. - candidate := app - candidate.InstallationAccount = picked.Account.Login - if uncovered := reposNotCoveredByInstallation(cfg, candidate); len(uncovered) > 0 { - fmt.Fprintf(env.stdout, - "Recorded installation %d on %q cannot reach %s; waiting for an installation on the right account.\n", - picked.ID, picked.Account.Login, strings.Join(uncovered, ", "), - ) - refreshed = false - } - } if refreshed { fmt.Fprintf(env.stdout, "Refreshing recorded installation %d on %s.\n", @@ -428,17 +413,29 @@ func (env *appEnv) runInstallFlow( fmt.Fprintf(env.stdout, "Install the app on the account that owns your synced repos:\n %s\n", url, ) + known := make(map[int64]struct{}) + if app.InstallationID != 0 { + known[app.InstallationID] = struct{}{} + } + if open { + jwt, err := appJWT(app, env.now()) + if err != nil { + return err + } + installs, err := client.ListInstallations(ctx, jwt) + if err != nil { + return err + } + for _, install := range installs { + known[install.ID] = struct{}{} + } + } if open { if err := env.openBrowser(url); err != nil { fmt.Fprintf(env.stdout, "could not open browser: %v\n", err) } } fmt.Fprintln(env.stdout, "Waiting for the installation to appear...") - - known := make(map[int64]struct{}) - if app.InstallationID != 0 { - known[app.InstallationID] = struct{}{} - } err := env.pollUntil(ctx, timeout, func(ctx context.Context) (bool, error) { jwt, err := appJWT(app, env.now()) if err != nil { @@ -452,20 +449,6 @@ func (env *appEnv) runInstallFlow( if _, ok := known[install.ID]; ok { continue } - // A pre-existing installation on an account that does - // not own the configured repos is not the one we are - // waiting for; report it once and keep polling so the - // user can install on the right account. - candidate := app - candidate.InstallationAccount = install.Account.Login - if uncovered := reposNotCoveredByInstallation(cfg, candidate); len(uncovered) > 0 { - fmt.Fprintf(env.stdout, - "Ignoring installation %d on %q: it cannot reach %s. Still waiting for an installation on the owning account.\n", - install.ID, install.Account.Login, strings.Join(uncovered, ", "), - ) - known[install.ID] = struct{}{} - continue - } picked = install return true, nil } @@ -478,21 +461,6 @@ func (env *appEnv) runInstallFlow( app.InstallationID = picked.ID app.InstallationAccount = picked.Account.Login - // Installation tokens only reach repos owned by the installed - // account. Surface uncovered repos before saving: config - // validation would reject the entry anyway, and the user needs to - // know the GitHub-side installation exists but was not recorded. - if uncovered := reposNotCoveredByInstallation(cfg, app); len(uncovered) > 0 { - return fmt.Errorf( - "the app was installed on %q on GitHub, but that installation cannot reach "+ - "configured repos %s; not recording it in config. Uninstall it in the "+ - "browser and install on the account that owns those repos, or remove "+ - "them from middleman's config before using an app on this host "+ - "(middleman resolves one credential chain per host, so per-repo token "+ - "overrides cannot mix with an app)", - picked.Account.Login, strings.Join(uncovered, ", "), - ) - } // Account ownership is not enough for an "Only select repositories" // install: the token reaches only the chosen repos, and anything // else 404s during sync while the config looks healthy. The @@ -512,8 +480,8 @@ func (env *appEnv) runInstallFlow( return fmt.Errorf("saving installation to config: %w", err) } fmt.Fprintf(env.stdout, - "Installed on %s (installation %d). middleman will now sync %s with app tokens.\n", - picked.Account.Login, picked.ID, app.Host, + "Installed on %s (installation %d). middleman will now sync %s repos on %s with app tokens.\n", + picked.Account.Login, picked.ID, picked.Account.Login, app.Host, ) return nil } @@ -537,11 +505,6 @@ func (env *appEnv) refreshAppMetadata( return app, nil } -// reposNotCoveredByInstallation lists configured github repos on the -// app's host that would resolve to the app token but are owned by a -// different account than the installation. Mirrors the config-level -// coverage validation so the CLI can explain the problem instead of -// failing a save. // verifySelectedInstallationCoverage checks an "Only select // repositories" installation against the configured repos it is // supposed to serve, by listing what an installation token can @@ -580,25 +543,6 @@ func (env *appEnv) verifySelectedInstallationCoverage( return names, nil } -func reposNotCoveredByInstallation( - cfg *config.Config, app config.GitHubAppConfig, -) []string { - var uncovered []string - for _, r := range cfg.Repos { - if r.PlatformOrDefault() != "github" || r.PlatformHostOrDefault() != app.Host { - continue - } - if r.TokenEnv != "" || r.TokenFile != "" { - continue - } - if strings.EqualFold(r.Owner, app.InstallationAccount) { - continue - } - uncovered = append(uncovered, r.Owner+"/"+r.Name) - } - return uncovered -} - // validAppSlug matches GitHub's app slug shape (letters, digits, // hyphens). The slug arrives from the manifest conversion response // and is used as a filename, so anything else — path separators, diff --git a/cmd/middleman-github-app/e2e_test.go b/cmd/middleman-github-app/e2e_test.go index 3d0665042..351a81767 100644 --- a/cmd/middleman-github-app/e2e_test.go +++ b/cmd/middleman-github-app/e2e_test.go @@ -23,6 +23,7 @@ import ( "github.com/stretchr/testify/require" "go.kenn.io/middleman/internal/config" + "go.kenn.io/middleman/internal/githubapp" "go.kenn.io/middleman/internal/githubapp/githubapptest" "go.kenn.io/middleman/internal/tokenauth" ) @@ -225,13 +226,17 @@ func TestCreateFlowEndToEnd(t *testing.T) { manifests := fake.Manifests() require.Len(manifests, 1) var sent struct { - Public bool `json:"public"` + URL string `json:"url"` + Public bool `json:"public"` HookAttributes struct { - Active bool `json:"active"` + URL string `json:"url"` + Active bool `json:"active"` } `json:"hook_attributes"` DefaultPermissions map[string]string `json:"default_permissions"` } require.NoError(json.Unmarshal([]byte(manifests[0]), &sent)) + assert.Equal(githubapp.DefaultHomepageURL, sent.URL) + assert.Equal(githubapp.DefaultHomepageURL, sent.HookAttributes.URL) assert.False(sent.Public) // The app stays read-only; mutations use the user's PAT chain. for scope, level := range sent.DefaultPermissions { @@ -403,7 +408,7 @@ func TestInstallHydratesMinimalAppMetadataBeforeOpeningInstallURL(t *testing.T) assert.NotZero(cfg.GitHubApps[0].InstallationID) } -func TestInstallRefusesInstallationThatMissesConfiguredRepos(t *testing.T) { +func TestInstallRecordsInstallationForOtherAccountWithoutJudgingOtherRepos(t *testing.T) { t.Parallel() require := require.New(t) fake := githubapptest.NewFake() @@ -414,25 +419,20 @@ func TestInstallRefusesInstallationThatMissesConfiguredRepos(t *testing.T) { env, _ := newTestEnv(t, fake, configPath) require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) - // Installing on an account that does not own kenn-io/middleman - // must not be recorded: the installation token cannot reach the - // repo, so sync would 404 while the config looks healthy. The - // flow reports the uncovering installation and keeps waiting for - // one on the owning account instead of dead-ending. + // Installing on another account is valid: repo owner is part of the sync + // identity, so this installation simply will not be used for kenn-io repos. env, out := newTestEnv(t, fake, configPath) env.openBrowser = scriptBrowser(t, fake, "someone-else") - err := runCLI([]string{"install", "--timeout", "1s"}, env) - require.Error(err) - require.ErrorContains(err, "timed out") - require.Contains(out.String(), "someone-else") - require.Contains(out.String(), "kenn-io/middleman") - require.Contains(out.String(), "Still waiting for an installation on the owning account") + require.NoError(runCLI([]string{"install", "--timeout", "10s"}, env)) + require.NotContains(out.String(), "cannot reach") + require.NotContains(out.String(), "kenn-io/middleman") cfg, err := config.Load(configPath) require.NoError(err) require.Len(cfg.GitHubApps, 1) - assert.Zero(t, cfg.GitHubApps[0].InstallationID, - "uncovered installation must not be recorded") + assert := assert.New(t) + assert.NotZero(cfg.GitHubApps[0].InstallationID) + assert.Equal("someone-else", cfg.GitHubApps[0].InstallationAccount) } func TestDeleteRefusesWhenCredentialsCannotBeVerified(t *testing.T) { @@ -520,8 +520,6 @@ func TestDeleteOpensSettingsForRepairInvalidConfig(t *testing.T) { broken := strings.Replace(string(raw), `installation_account = "kenn-io"`, `installation_account = "other-org"`, 1) require.NotEqual(string(raw), broken) require.NoError(os.WriteFile(configPath, []byte(broken), 0o600)) - _, err = config.Load(configPath) - require.ErrorContains(err, "not covered by the github app") var opened string env, _ := newTestEnv(t, fake, configPath) @@ -687,27 +685,26 @@ name = "thing" }, env)) // The user re-points middleman at repos the recorded installation's - // account does not own; the strict loader now rejects the config. + // account does not own; install must not refresh that no-longer-relevant + // installation. raw, err := os.ReadFile(configPath) require.NoError(err) repointed := strings.Replace(string(raw), `owner = "wrongorg"`, `owner = "kenn-io"`, 1) repointed = strings.Replace(repointed, `name = "thing"`, `name = "middleman"`, 1) require.NoError(os.WriteFile(configPath, []byte(repointed), 0o600)) - _, err = config.Load(configPath) - require.ErrorContains(err, "not covered by the github app") - // install must not dead-end on refreshing the wrong-account - // installation: it falls through to waiting for an installation on - // the owning account and records that one. + // The existing installation remains valid for wrongorg. It is refreshed + // rather than judged against kenn-io repos. env, out := newTestEnv(t, fake, configPath) env.openBrowser = scriptBrowser(t, fake, "kenn-io") require.NoError(runCLI([]string{"install", "--timeout", "10s"}, env)) - require.Contains(out.String(), "waiting for an installation on the right account") + require.Contains(out.String(), "Refreshing recorded installation") + require.NotContains(out.String(), "cannot reach") cfg, err := config.Load(configPath) require.NoError(err) require.Len(cfg.GitHubApps, 1) - require.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) + require.Equal("wrongorg", cfg.GitHubApps[0].InstallationAccount) } func TestInstallSkipsPreexistingUncoveringInstallation(t *testing.T) { @@ -721,11 +718,9 @@ func TestInstallSkipsPreexistingUncoveringInstallation(t *testing.T) { env, _ := newTestEnv(t, fake, configPath) require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) - // An unrecorded installation on an account that does not own the - // configured repos already exists before install runs. The poll - // must not grab it as "the first new installation" and dead-end at - // the coverage check; it ignores it and keeps waiting for the - // installation the browser flow creates on the owning account. + // An unrecorded installation already exists before install runs. When the + // browser path is active, the poll waits for a newly-created installation + // instead of grabbing an old one. app, ok := fake.AppBySlug("middleman-preexist") require.True(ok) _, err := fake.Install(app.ID, "someone-else") @@ -734,8 +729,8 @@ func TestInstallSkipsPreexistingUncoveringInstallation(t *testing.T) { env, out := newTestEnv(t, fake, configPath) env.openBrowser = scriptBrowser(t, fake, "kenn-io") require.NoError(runCLI([]string{"install", "--timeout", "10s"}, env)) - require.Contains(out.String(), "Ignoring installation") - require.Contains(out.String(), "someone-else") + require.NotContains(out.String(), "Ignoring installation") + require.NotContains(out.String(), "cannot reach") cfg, err := config.Load(configPath) require.NoError(err) @@ -743,6 +738,39 @@ func TestInstallSkipsPreexistingUncoveringInstallation(t *testing.T) { require.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) } +func TestInstallRecordsOrgInstallationWithoutCoveringOtherOwners(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + + createTestApp(t, fake, configPath, "middleman-scoped") + cfg, err := config.Load(configPath) + require.NoError(err) + appID := cfg.GitHubApps[0].AppID + raw, err := os.ReadFile(configPath) + require.NoError(err) + require.NoError(os.WriteFile(configPath, append(raw, []byte(` + +[[repos]] +owner = "mariusvniekerk" +name = "skills" +`)...), 0o600)) + _, err = fake.Install(appID, "kenn-io") + require.NoError(err) + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{ + "install", "--no-browser", "--timeout", "10s", + }, env)) + + cfg, err = config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + require.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) +} + func TestCreateWithNestedRelativeConfigPath(t *testing.T) { // A relative --config with a directory component previously saved // a key path that later loads re-resolved against the config dir, @@ -942,8 +970,6 @@ func TestOpenOpensSettingsForRepairInvalidConfig(t *testing.T) { broken := strings.Replace(string(raw), `installation_account = "kenn-io"`, `installation_account = "other-org"`, 1) require.NotEqual(string(raw), broken) require.NoError(os.WriteFile(configPath, []byte(broken), 0o600)) - _, err = config.Load(configPath) - require.ErrorContains(err, "not covered by the github app") var opened string env, _ := newTestEnv(t, fake, configPath) diff --git a/cmd/middleman-github-app/manage.go b/cmd/middleman-github-app/manage.go index c38b649f5..a86c036d5 100644 --- a/cmd/middleman-github-app/manage.go +++ b/cmd/middleman-github-app/manage.go @@ -83,9 +83,10 @@ func runUninstall(args []string, env *appEnv) error { "Uninstalled app %q from %s. middleman falls back to PAT tokens for %s.\n", app.Slug, app.InstallationAccount, app.Host, ) + oldApp := app app.InstallationID = 0 app.InstallationAccount = "" - return updateAppInConfig(cfg, env.configPath, app) + return updateAppSlotInConfig(cfg, env.configPath, oldApp, app) } func runDelete(args []string, env *appEnv) error { diff --git a/cmd/middleman-github-app/shared.go b/cmd/middleman-github-app/shared.go index ff2dca41f..a918b2649 100644 --- a/cmd/middleman-github-app/shared.go +++ b/cmd/middleman-github-app/shared.go @@ -38,8 +38,9 @@ func (env *appEnv) webBaseFor(host string) string { return githubapp.WebBaseForHost(host) } -// selectApp picks the configured app for host. With one configured -// app and no --host flag, that app is selected. +// selectApp picks a configured app credential for host. Multiple installation +// rows can share one GitHub App credential; management commands only need one +// of those rows to sign app JWTs. func selectApp(cfg *config.Config, host string) (config.GitHubAppConfig, error) { if host == "" { switch len(cfg.GitHubApps) { @@ -85,12 +86,24 @@ func installURL(webBase string, app config.GitHubAppConfig) string { return fmt.Sprintf("%s/apps/%s/installations/new", webBase, app.Slug) } -// updateAppInConfig replaces the entry for app.Host and saves. +// updateAppInConfig replaces the entry for app.Host and app.InstallationAccount +// and saves. If a create flow saved a pending, uninstalled row for the same +// app credential, the first successful install replaces that pending row. func updateAppInConfig( cfg *config.Config, configPath string, app config.GitHubAppConfig, ) error { for i := range cfg.GitHubApps { - if cfg.GitHubApps[i].Host == app.Host { + existing := cfg.GitHubApps[i] + if existing.Host != app.Host { + continue + } + sameInstallation := strings.EqualFold( + existing.InstallationAccount, app.InstallationAccount, + ) + pendingSameApp := existing.InstallationAccount == "" && + existing.AppID == app.AppID && + existing.PrivateKeyPath == app.PrivateKeyPath + if sameInstallation || pendingSameApp { cfg.GitHubApps[i] = app return cfg.Save(configPath) } @@ -99,6 +112,24 @@ func updateAppInConfig( return cfg.Save(configPath) } +func updateAppSlotInConfig( + cfg *config.Config, + configPath string, + oldApp config.GitHubAppConfig, + newApp config.GitHubAppConfig, +) error { + for i := range cfg.GitHubApps { + existing := cfg.GitHubApps[i] + if existing.Host == oldApp.Host && + existing.InstallationID == oldApp.InstallationID && + strings.EqualFold(existing.InstallationAccount, oldApp.InstallationAccount) { + cfg.GitHubApps[i] = newApp + return cfg.Save(configPath) + } + } + return updateAppInConfig(cfg, configPath, newApp) +} + func removeAppFromConfig( cfg *config.Config, configPath, host string, ) error { diff --git a/cmd/middleman/provider_startup.go b/cmd/middleman/provider_startup.go index 593474473..492d105d4 100644 --- a/cmd/middleman/provider_startup.go +++ b/cmd/middleman/provider_startup.go @@ -108,15 +108,22 @@ func collectProviderTokenSources( add := func(plan config.ProviderTokenSource) error { desc := plan.Descriptor key := providerHostKey(desc.Key.Platform, desc.Key.Host) - if _, seen := providerSources[key]; seen { - return nil + src, seen := providerSources[key] + if !seen { + src = set.Upsert(desc) } - src := set.Upsert(desc) - if _, err := src.Token(ctx); err != nil { + tokenCtx := ctx + if plan.GitHubOwner != "" { + tokenCtx = tokenauth.WithGitHubOwner(tokenCtx, plan.GitHubOwner) + } + if _, err := src.Token(tokenCtx); err != nil { if !plan.Required && errors.Is(err, tokenauth.ErrMissingToken) { return nil } label := fmt.Sprintf("%s host %s", desc.Key.Platform, desc.Key.Host) + if plan.GitHubOwner != "" { + label = fmt.Sprintf("%s owner %s", label, plan.GitHubOwner) + } if plan.Required { return fmt.Errorf("no token for %s via %s: %w", label, desc.SafeString(), err) } @@ -125,7 +132,9 @@ func collectProviderTokenSources( label, desc.SafeString(), err, ) } - providerSources[key] = src + if !seen { + providerSources[key] = src + } return nil } for _, plan := range cfg.ProviderTokenSources() { @@ -164,12 +173,6 @@ func buildProviderStartup( githubHosts := make(map[string]struct{}, len(providerSources)) for key, tokenSource := range providerSources { platformName, host := splitProviderHostKey(key) - if _, err := tokenSource.Token(context.Background()); err != nil { - return providerStartup{}, fmt.Errorf( - "read token for %s host %s via %s: %w", - platformName, host, tokenSource.Descriptor().SafeString(), err, - ) - } rateKey := github.RateBucketKey(platformName, host) if _, ok := startup.rateTrackers[rateKey]; !ok { startup.rateTrackers[rateKey] = github.NewPlatformRateTracker( diff --git a/cmd/middleman/startup_github_app_e2e_test.go b/cmd/middleman/startup_github_app_e2e_test.go index 6f5e8e188..842b4584c 100644 --- a/cmd/middleman/startup_github_app_e2e_test.go +++ b/cmd/middleman/startup_github_app_e2e_test.go @@ -115,7 +115,7 @@ repository_selection = "all" key := providerHostKey("github", "github.com") require.Contains(sources, key) - got, err := sources[key].Token(t.Context()) + got, err := sources[key].Token(tokenauth.WithGitHubOwner(t.Context(), "kenn-io")) require.NoError(err) assert.True(strings.HasPrefix(got, "ghs_"), "expected a minted installation token, got %q", got) @@ -126,3 +126,37 @@ repository_selection = "all" require.NoError(err) assert.Equal(5000, rate.Limit) } + +func TestCollectProviderTokensValidatesGitHubAppPerRepoOwner(t *testing.T) { + require := require.New(t) + dir := t.TempDir() + configPath := filepath.Join(dir, "config.toml") + require.NoError(os.WriteFile(configPath, []byte(` +[[repos]] +owner = "kenn-io" +name = "middleman" + +[[repos]] +owner = "mariusvniekerk" +name = "skills" + +[[github_apps]] +host = "github.com" +app_id = 4321 +private_key_path = "app.pem" +installation_id = 99 +installation_account = "kenn-io" +repository_selection = "all" +`), 0o600)) + cfg, err := config.Load(configPath) + require.NoError(err) + + set := tokenauth.NewSourceSet(tokenauth.Options{ + GitHubApp: func(_ context.Context, c tokenauth.Candidate) (string, time.Time, error) { + return "ghs_kenn", time.Now().Add(time.Hour), nil + }, + }) + _, err = collectProviderTokenSources(t.Context(), cfg, set) + require.Error(err) + Assert.ErrorContains(t, err, "owner mariusvniekerk") +} diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index ea11ab836..f4732c83f 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -105,6 +105,39 @@ change what a field means. Provider-neutral persistence should receive the same semantic shape regardless of whether data came from GraphQL, REST, tags, or fallback repository listing. +## GitHub App Manifest Flow + +`middleman-github-app create` uses GitHub's App Manifest flow so sync can read +with installation tokens. Even though middleman disables webhooks and polls, +the manifest must still include a syntactically valid `hook_attributes.url`; +GitHub's live manifest validator can report the missing hook URL as a generic +`"url" wasn't supplied` error. Do not remove that hook URL from +`internal/githubapp/manifest.go::NewManifest`; keep +`cmd/middleman-github-app/e2e_test.go::TestCreateFlowEndToEnd` asserting the +serialized manifest shape so the fake cannot accept a payload GitHub rejects. + +GitHub App installation tokens are account-scoped, not host-scoped. A +`github.com` app installation for `kenn-io` must not authenticate +`mariusvniekerk/*` reads just because both repos share the same host. Keep +`internal/tokenauth/source.go::WithGitHubOwner`, +`internal/tokenauth/source.go::ManagedSource.githubAppToken`, +`internal/github/client.go::githubOwnerFromRequest`, and +`internal/github/graphql.go::GraphQLFetcher.fetchRepoPRsWithPageSize` aligned: +repo-scoped REST and GraphQL reads must put the GitHub owner into token +resolution, token caches must be keyed per installation candidate, and +ownerless contexts such as clone auth must fall through to PAT/`gh` credentials. +Ownerless GitHub REST endpoints that are still logically scoped by an owner +argument, such as installation repository discovery in +`internal/github/client.go::liveClient.ListRepositoriesByOwner`, must set that +owner explicitly before resolving the token. +Config may carry multiple `[[github_apps]]` rows for one host when the same app +is installed on multiple accounts; validation in +`internal/config/config.go::validateGitHubAppCoverage` applies selected-repo +coverage only to repos owned by that row's `installation_account`. The install +CLI must not warn that an installation on one account "cannot reach" repos owned +by another account; those repos are outside that installation's owner scope and +fall through to the next credential source. + ## Testing Expectations Changes in this area should usually add or update tests at the boundary where diff --git a/internal/config/config.go b/internal/config/config.go index 579b28229..eb8b7242e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1704,7 +1704,7 @@ func (c *Config) validatePlatforms() error { } func (c *Config) validateGitHubApps() error { - seenHosts := make(map[string]struct{}, len(c.GitHubApps)) + seenInstallations := make(map[string]struct{}, len(c.GitHubApps)) for i := range c.GitHubApps { app := &c.GitHubApps[i] app.Host = strings.TrimSpace(app.Host) @@ -1751,14 +1751,22 @@ func (c *Config) validateGitHubApps() error { "installation_id is set", i, ) } - // One app per host: the token source chain is host-scoped, so a - // second app for the same host could never be selected. - if _, ok := seenHosts[host]; ok { + // GitHub App credentials are host-shaped, but installation tokens are + // account-scoped. Multiple rows may share a host when the same app is + // installed on multiple owners; only one row per installation account + // can be selected for a given owner. + installKey := host + "\x00" + strings.ToLower(app.InstallationAccount) + if _, ok := seenInstallations[installKey]; ok { + account := app.InstallationAccount + if account == "" { + account = "" + } return fmt.Errorf( - "config: github_apps[%d]: duplicate github app for host %q", i, host, + "config: github_apps[%d]: duplicate github app for host %q and installation account %q", + i, host, account, ) } - seenHosts[host] = struct{}{} + seenInstallations[installKey] = struct{}{} } return nil } @@ -1766,10 +1774,9 @@ func (c *Config) validateGitHubApps() error { // validateGitHubAppCoverage rejects github repos that would resolve // to an app installation token without being reachable by it. // Installation tokens are scoped to the installed account, not the -// host, so a repo owned by anyone else would 404 during sync while -// the config looks healthy. Repos with their own token_env/token_file -// override never reach the app candidate and are exempt. Runs after -// repo normalization so owners are canonical. +// host. Only repos owned by that account use the app candidate; other +// owners on the same host fall through to the PAT/gh credential chain. +// Runs after repo normalization so owners are canonical. func (c *Config) validateGitHubAppCoverage() error { for _, app := range c.GitHubApps { if app.InstallationID == 0 || app.InstallationAccount == "" { @@ -1783,24 +1790,12 @@ func (c *Config) validateGitHubAppCoverage() error { if r.TokenEnv != "" || r.TokenFile != "" { continue } - if strings.EqualFold(r.Owner, app.InstallationAccount) { - if err := validateSelectedRepoCoverage(i, r, app); err != nil { - return err - } + if !strings.EqualFold(r.Owner, app.InstallationAccount) { continue } - // Do not suggest per-repo token overrides here: middleman - // resolves one credential chain per host, so an override on - // only this repo would be rejected by the same-host conflict - // check. The real options are changing where the app is - // installed or not using an app for this host at all. - return fmt.Errorf( - "config: repos[%d]: %s/%s is not covered by the github app for %s "+ - "(installed on %q): installation tokens only reach repos owned by the "+ - "installed account. Install the app on %q instead, or remove the "+ - "[[github_apps]] entry for %s so the host uses PAT auth for every repo", - i, r.Owner, r.Name, app.Host, app.InstallationAccount, r.Owner, app.Host, - ) + if err := validateSelectedRepoCoverage(i, r, app); err != nil { + return err + } } } return nil @@ -1843,21 +1838,33 @@ func validateSelectedRepoCoverage(i int, r Repo, app GitHubAppConfig) error { ) } -// GitHubAppForHost returns the configured GitHub App for host, if any. -func (c *Config) GitHubAppForHost(host string) (GitHubAppConfig, bool) { +// GitHubAppsForHost returns the configured GitHub App installations for host. +func (c *Config) GitHubAppsForHost(host string) []GitHubAppConfig { if c == nil { - return GitHubAppConfig{}, false + return nil } h, err := normalizePlatformHost(defaultPlatform, host) if err != nil { - return GitHubAppConfig{}, false + return nil } + var apps []GitHubAppConfig for _, app := range c.GitHubApps { if app.Host == h { - return app, true + apps = append(apps, app) } } - return GitHubAppConfig{}, false + return apps +} + +// GitHubAppForHost returns a configured GitHub App credential for host, if any. +// When multiple installation rows share one app credential, the first row is +// enough for management operations that only need the app's private key. +func (c *Config) GitHubAppForHost(host string) (GitHubAppConfig, bool) { + apps := c.GitHubAppsForHost(host) + if len(apps) == 0 { + return GitHubAppConfig{}, false + } + return apps[0], true } func (c *Config) validateAgents() error { @@ -2065,8 +2072,9 @@ func (c *Config) ResolveRepoTokenSource(r Repo) tokenauth.Descriptor { } type ProviderTokenSource struct { - Descriptor tokenauth.Descriptor - Required bool + Descriptor tokenauth.Descriptor + Required bool + GitHubOwner string } func (c *Config) ProviderTokenSources() []ProviderTokenSource { @@ -2094,7 +2102,27 @@ func (c *Config) ProviderTokenSources() []ProviderTokenSource { }) } for _, repo := range c.Repos { - add(c.ResolveRepoTokenSource(repo), true) + plan := ProviderTokenSource{ + Descriptor: c.ResolveRepoTokenSource(repo), + Required: true, + } + if repo.PlatformOrDefault() == defaultPlatform { + plan.GitHubOwner = repo.Owner + } + key := plan.Descriptor.Key + if key.Host == "" { + continue + } + if _, ok := seen[key]; !ok { + seen[key] = struct{}{} + out = append(out, plan) + continue + } + // Keep same-host GitHub repo plans after the first so startup validates + // each owner against the owner-scoped app/PAT chain. + if plan.GitHubOwner != "" { + out = append(out, plan) + } } for _, p := range c.Platforms { add(c.TokenSourceForPlatformHost(p.Type, p.Host, "", ""), false) @@ -2170,13 +2198,17 @@ func (c *Config) TokenSourceForPlatformHost( // validation exempts overridden repos and a fall-through would // reopen the cross-account 404 hole when the override is unset. if p == defaultPlatform && repoTokenEnv == "" && repoTokenFile == "" { - if app, ok := c.GitHubAppForHost(h); ok && app.AppID > 0 && app.PrivateKeyPath != "" { + for _, app := range c.GitHubAppsForHost(h) { + if app.AppID <= 0 || app.PrivateKeyPath == "" { + continue + } desc.Candidates = append(desc.Candidates, tokenauth.Candidate{ - Kind: tokenauth.SourceKindGitHubApp, - Host: h, - FilePath: app.PrivateKeyPath, - AppID: app.AppID, - InstallationID: app.InstallationID, + Kind: tokenauth.SourceKindGitHubApp, + Host: h, + FilePath: app.PrivateKeyPath, + AppID: app.AppID, + InstallationID: app.InstallationID, + InstallationAccount: app.InstallationAccount, }) } } diff --git a/internal/config/github_app_config_test.go b/internal/config/github_app_config_test.go index 1b916ef29..ef86c53e2 100644 --- a/internal/config/github_app_config_test.go +++ b/internal/config/github_app_config_test.go @@ -89,18 +89,24 @@ installation_id = 5 wantErr: "installation_account is required when installation_id is set", }, { - name: "duplicate host", + name: "duplicate installation account", toml: ` [[github_apps]] app_id = 1 private_key_path = "a.pem" +installation_id = 10 +installation_account = "kenn-io" +repository_selection = "all" [[github_apps]] host = "github.com" app_id = 2 private_key_path = "b.pem" +installation_id = 11 +installation_account = "KENN-IO" +repository_selection = "all" `, - wantErr: `duplicate github app for host "github.com"`, + wantErr: `duplicate github app for host "github.com" and installation account "KENN-IO"`, }, } for _, tt := range tests { @@ -149,7 +155,7 @@ repository_selection = "all" `, }, { - name: "uncovered repo without override fails", + name: "repo owned by another account does not use the app", toml: ` [[repos]] owner = "kenn-io" @@ -166,7 +172,6 @@ installation_id = 9 installation_account = "kenn-io" repository_selection = "all" `, - wantErr: "otherorg/thing is not covered by the github app", }, { name: "uncovered repo with its own token override passes", @@ -458,10 +463,51 @@ repository_selection = "all" assert.Equal(int64(4321), app.AppID) assert.Equal(int64(99), app.InstallationID) assert.Equal("github.com", app.Host) + assert.Equal("kenn-io", app.InstallationAccount) assert.True(filepath.IsAbs(app.FilePath), "key path %q", app.FilePath) assert.Equal("MY_PAT", desc.Candidates[1].EnvName) } +func TestTokenSourceChainIncludesGitHubAppsForEachInstalledAccount(t *testing.T) { + cfg, err := Load(writeConfig(t, ` +github_token_env = "MY_PAT" + +[[repos]] +owner = "kenn-io" +name = "middleman" + +[[repos]] +owner = "other-org" +name = "tool" + +[[github_apps]] +host = "github.com" +app_id = 4321 +private_key_path = "app.pem" +installation_id = 99 +installation_account = "kenn-io" +repository_selection = "all" + +[[github_apps]] +host = "github.com" +app_id = 4321 +private_key_path = "app.pem" +installation_id = 100 +installation_account = "other-org" +repository_selection = "all" +`)) + require.NoError(t, err) + + desc := cfg.TokenSourceForPlatformHost("github", "github.com", "", "") + var appAccounts []string + for _, cand := range desc.Candidates { + if cand.Kind == tokenauth.SourceKindGitHubApp { + appAccounts = append(appAccounts, cand.InstallationAccount) + } + } + Assert.Equal(t, []string{"kenn-io", "other-org"}, appAccounts) +} + func TestTokenSourceChainRepoOverrideExcludesGitHubApp(t *testing.T) { cfg, err := Load(writeConfig(t, ` github_token_env = "MY_PAT" diff --git a/internal/github/client.go b/internal/github/client.go index 49eb6f821..e44446883 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -241,6 +241,7 @@ func NewClient( SetHeader: tokenauth.BearerAuthHeader, RetryOnUnauthorized: true, AllowedOrigin: allowedOrigin, + GitHubOwner: githubOwnerFromRequest, } et := &etagTransport{base: authRT} var transport http.RoundTripper = et @@ -319,6 +320,44 @@ func NewClient( }, nil } +func githubOwnerFromRequest(req *http.Request) string { + if req == nil || req.URL == nil { + return "" + } + if owner := githubOwnerFromPath(req.URL.Path); owner != "" { + return owner + } + if req.GetBody == nil { + return "" + } + body, err := req.GetBody() + if err != nil { + return "" + } + defer body.Close() + var payload struct { + Variables map[string]any `json:"variables"` + } + if err := json.NewDecoder(body).Decode(&payload); err != nil { + return "" + } + owner, _ := payload.Variables["owner"].(string) + return owner +} + +func githubOwnerFromPath(path string) string { + parts := strings.Split(strings.Trim(path, "/"), "/") + for i, part := range parts { + if part == "repos" && i+2 < len(parts) { + return parts[i+1] + } + if (part == "orgs" || part == "users") && i+2 < len(parts) && parts[i+2] == "repos" { + return parts[i+1] + } + } + return "" +} + // mutationAuthTransport marks every request's context with // tokenauth.WithMutationAuth before auth resolution, steering token // selection away from github_app installation tokens. @@ -1334,6 +1373,7 @@ func (c *liveClient) ListRepositoriesByOwner( ctx context.Context, owner string, ) ([]*gh.Repository, error) { if c.splitAuthActive() { + ctx = tokenauth.WithGitHubOwner(ctx, owner) repos, err := collectPages( ctx, func(opts *gh.ListOptions) ([]*gh.Repository, *gh.Response, error) { diff --git a/internal/github/graphql.go b/internal/github/graphql.go index b977c548a..d79c10a9f 100644 --- a/internal/github/graphql.go +++ b/internal/github/graphql.go @@ -721,6 +721,7 @@ func NewGraphQLFetcher( SetHeader: tokenauth.BearerAuthHeader, RetryOnUnauthorized: true, AllowedOrigin: graphQLEndpointForHost(platformHost), + GitHubOwner: githubOwnerFromRequest, } var base http.RoundTripper = authRT if rateTracker != nil { @@ -792,6 +793,7 @@ func (g *GraphQLFetcher) FetchRepoPRs( func (g *GraphQLFetcher) fetchRepoPRsWithPageSize( ctx context.Context, owner, name string, pageSize int, ) (*RepoBulkResult, error) { + ctx = tokenauth.WithGitHubOwner(ctx, owner) progress := newMergeRequestListFetchProgressLogger(RepoRef{ Owner: owner, Name: name, @@ -850,6 +852,7 @@ func (g *GraphQLFetcher) FetchRepoIssues( func (g *GraphQLFetcher) fetchRepoIssuesWithPageSize( ctx context.Context, owner, name string, pageSize int, ) (*RepoBulkResult, error) { + ctx = tokenauth.WithGitHubOwner(ctx, owner) progress := newIssueListFetchProgressLogger(RepoRef{ Owner: owner, Name: name, diff --git a/internal/githubapp/githubapptest/fake.go b/internal/githubapp/githubapptest/fake.go index f8c19e37a..8d4f6e079 100644 --- a/internal/githubapp/githubapptest/fake.go +++ b/internal/githubapp/githubapptest/fake.go @@ -216,12 +216,24 @@ func (f *Fake) handleManifestSubmit(w http.ResponseWriter, r *http.Request) { return } var parsed struct { + URL string `json:"url"` + HookAttributes struct { + URL string `json:"url"` + } `json:"hook_attributes"` RedirectURL string `json:"redirect_url"` } if err := json.Unmarshal([]byte(manifest), &parsed); err != nil || parsed.RedirectURL == "" { http.Error(w, "manifest missing redirect_url", http.StatusBadRequest) return } + if parsed.URL == "" { + http.Error(w, "manifest missing url", http.StatusBadRequest) + return + } + if parsed.HookAttributes.URL == "" { + http.Error(w, "manifest missing hook_attributes.url", http.StatusBadRequest) + return + } code := randomHex(16) f.mu.Lock() f.pendingCodes[code] = manifest diff --git a/internal/githubapp/jwt_test.go b/internal/githubapp/jwt_test.go index 7cada6736..1826ebeaf 100644 --- a/internal/githubapp/jwt_test.go +++ b/internal/githubapp/jwt_test.go @@ -114,20 +114,35 @@ func TestParsePrivateKeyFormats(t *testing.T) { func TestNewManifestValidation(t *testing.T) { t.Parallel() + require := require.New(t) _, err := NewManifest("", "", "http://127.0.0.1:1/callback") - require.ErrorContains(t, err, "name is required") + require.ErrorContains(err, "name is required") _, err = NewManifest(strings.Repeat("x", 35), "", "http://127.0.0.1:1/callback") - require.ErrorContains(t, err, "34 character limit") + require.ErrorContains(err, "34 character limit") m, err := NewManifest("middleman-test", "", "http://127.0.0.1:9/callback") - require.NoError(t, err) + require.NoError(err) assert := assert.New(t) assert.False(m.Public) assert.False(m.HookAttributes.Active) + assert.Equal(DefaultHomepageURL, m.HookAttributes.URL) assert.Empty(m.DefaultEvents) assert.Equal("http://127.0.0.1:9/callback", m.RedirectURL) assert.Equal(DefaultHomepageURL, m.URL) + manifestJSON, err := m.JSON() + require.NoError(err) + var encoded struct { + URL string `json:"url"` + HookAttributes struct { + URL string `json:"url"` + Active bool `json:"active"` + } `json:"hook_attributes"` + } + require.NoError(json.Unmarshal([]byte(manifestJSON), &encoded)) + assert.Equal(DefaultHomepageURL, encoded.URL) + assert.Equal(DefaultHomepageURL, encoded.HookAttributes.URL) + assert.False(encoded.HookAttributes.Active) // Sync needs to read repo contents and PRs. assert.Equal("read", m.DefaultPermissions["contents"]) assert.Equal("read", m.DefaultPermissions["pull_requests"]) diff --git a/internal/githubapp/manifest.go b/internal/githubapp/manifest.go index 5db568489..cdfc9a786 100644 --- a/internal/githubapp/manifest.go +++ b/internal/githubapp/manifest.go @@ -83,7 +83,7 @@ func NewManifest(name, homepageURL, redirectURL string) (Manifest, error) { return Manifest{ Name: name, URL: homepageURL, - HookAttributes: HookAttributes{Active: false}, + HookAttributes: HookAttributes{URL: homepageURL, Active: false}, RedirectURL: redirectURL, Public: false, DefaultPermissions: DefaultPermissions(), diff --git a/internal/server/e2etest/github_app_split_auth_test.go b/internal/server/e2etest/github_app_split_auth_test.go index c061ce2b1..581e5681d 100644 --- a/internal/server/e2etest/github_app_split_auth_test.go +++ b/internal/server/e2etest/github_app_split_auth_test.go @@ -192,7 +192,11 @@ repository_selection = "all" var source tokenauth.Source for _, plan := range cfg.ProviderTokenSources() { src := sourceSet.Upsert(plan.Descriptor) - if _, err := src.Token(t.Context()); err != nil { + tokenCtx := t.Context() + if plan.GitHubOwner != "" { + tokenCtx = tokenauth.WithGitHubOwner(tokenCtx, plan.GitHubOwner) + } + if _, err := src.Token(tokenCtx); err != nil { if !plan.Required && errors.Is(err, tokenauth.ErrMissingToken) { continue } @@ -502,7 +506,11 @@ repository_selection = "all" var source tokenauth.Source for _, plan := range cfg.ProviderTokenSources() { src := sourceSet.Upsert(plan.Descriptor) - if _, err := src.Token(t.Context()); err != nil { + tokenCtx := t.Context() + if plan.GitHubOwner != "" { + tokenCtx = tokenauth.WithGitHubOwner(tokenCtx, plan.GitHubOwner) + } + if _, err := src.Token(tokenCtx); err != nil { if !plan.Required && errors.Is(err, tokenauth.ErrMissingToken) { continue } @@ -665,7 +673,11 @@ repository_selection = "all" var source tokenauth.Source for _, plan := range cfg.ProviderTokenSources() { src := sourceSet.Upsert(plan.Descriptor) - if _, err := src.Token(t.Context()); err != nil { + tokenCtx := t.Context() + if plan.GitHubOwner != "" { + tokenCtx = tokenauth.WithGitHubOwner(tokenCtx, plan.GitHubOwner) + } + if _, err := src.Token(tokenCtx); err != nil { if !plan.Required && errors.Is(err, tokenauth.ErrMissingToken) { continue } diff --git a/internal/tokenauth/descriptor.go b/internal/tokenauth/descriptor.go index 088478b9e..2d1a6073f 100644 --- a/internal/tokenauth/descriptor.go +++ b/internal/tokenauth/descriptor.go @@ -49,6 +49,10 @@ type Candidate struct { // Host carries the platform host; FilePath the private key path. AppID int64 InstallationID int64 + // InstallationAccount is the GitHub owner/org account the + // installation belongs to. App candidates are only valid for + // requests scoped to this owner. + InstallationAccount string } func (c Candidate) SafeString() string { @@ -60,6 +64,9 @@ func (c Candidate) SafeString() string { case SourceKindGitHubCLI: return fmt.Sprintf("github_cli:%s", c.Host) case SourceKindGitHubApp: + if c.InstallationAccount != "" { + return fmt.Sprintf("github_app:%d@%s/%s", c.AppID, c.Host, c.InstallationAccount) + } return fmt.Sprintf("github_app:%d@%s", c.AppID, c.Host) default: return string(c.Kind) @@ -147,11 +154,12 @@ func canonicalCandidate(candidate Candidate) Candidate { return Candidate{Kind: candidate.Kind, Host: candidate.Host} case SourceKindGitHubApp: return Candidate{ - Kind: candidate.Kind, - Host: candidate.Host, - FilePath: candidate.FilePath, - AppID: candidate.AppID, - InstallationID: candidate.InstallationID, + Kind: candidate.Kind, + Host: candidate.Host, + FilePath: candidate.FilePath, + AppID: candidate.AppID, + InstallationID: candidate.InstallationID, + InstallationAccount: candidate.InstallationAccount, } default: return candidate diff --git a/internal/tokenauth/github_app_test.go b/internal/tokenauth/github_app_test.go index 2120cccc4..0bea39029 100644 --- a/internal/tokenauth/github_app_test.go +++ b/internal/tokenauth/github_app_test.go @@ -3,6 +3,7 @@ package tokenauth import ( "context" "errors" + "fmt" "sync/atomic" "testing" "time" @@ -94,6 +95,78 @@ func TestGitHubAppNilMinterFallsThrough(t *testing.T) { assert.Equal(t, "pat-token", token) } +func TestGitHubAppRequiresMatchingOwnerScope(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + t.Setenv("TEST_GITHUB_APP_FALLBACK", "pat-token") + var mints atomic.Int64 + desc := githubAppDescriptor(42) + desc.Candidates[0].InstallationAccount = "kenn-io" + src := NewManagedSource(desc, Options{ + GitHubApp: func(context.Context, Candidate) (string, time.Time, error) { + mints.Add(1) + return "ghs_minted", time.Now().Add(time.Hour), nil + }, + }) + + token, err := src.Token(context.Background()) + require.NoError(err) + assert.Equal("pat-token", token) + + token, err = src.Token(WithGitHubOwner(context.Background(), "mariusvniekerk")) + require.NoError(err) + assert.Equal("pat-token", token) + + token, err = src.Token(WithGitHubOwner(context.Background(), "Kenn-IO")) + require.NoError(err) + assert.Equal("ghs_minted", token) + assert.Equal(int64(1), mints.Load()) +} + +func TestGitHubAppCacheIsScopedToInstallationAccount(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + desc := githubAppDescriptor(42) + desc.Candidates = []Candidate{ + { + Kind: SourceKindGitHubApp, + Host: "github.com", + FilePath: "/tmp/app.pem", + AppID: 77, + InstallationID: 42, + InstallationAccount: "kenn-io", + }, + { + Kind: SourceKindGitHubApp, + Host: "github.com", + FilePath: "/tmp/app.pem", + AppID: 77, + InstallationID: 43, + InstallationAccount: "other-org", + }, + } + minted := make(map[int64]int) + src := NewManagedSource(desc, Options{ + GitHubApp: func(_ context.Context, c Candidate) (string, time.Time, error) { + minted[c.InstallationID]++ + return fmt.Sprintf("ghs_%d", c.InstallationID), time.Now().Add(time.Hour), nil + }, + }) + + token, err := src.Token(WithGitHubOwner(context.Background(), "kenn-io")) + require.NoError(err) + assert.Equal("ghs_42", token) + + token, err = src.Token(WithGitHubOwner(context.Background(), "other-org")) + require.NoError(err) + assert.Equal("ghs_43", token) + + token, err = src.Token(WithGitHubOwner(context.Background(), "kenn-io")) + require.NoError(err) + assert.Equal("ghs_42", token) + assert.Equal(map[int64]int{42: 1, 43: 1}, minted) +} + func TestGitHubAppMintFailureSurfacesError(t *testing.T) { t.Setenv("TEST_GITHUB_APP_FALLBACK", "pat-token") src := NewManagedSource(githubAppDescriptor(42), Options{ diff --git a/internal/tokenauth/source.go b/internal/tokenauth/source.go index 4d660daa4..f80208972 100644 --- a/internal/tokenauth/source.go +++ b/internal/tokenauth/source.go @@ -33,13 +33,17 @@ type Source interface { } type ManagedSource struct { - mu sync.Mutex - desc Descriptor - options Options - ghToken string - ghCached bool - appToken string - appTokenExp time.Time + mu sync.Mutex + desc Descriptor + options Options + ghToken string + ghCached bool + appTokens map[Candidate]githubAppTokenCache +} + +type githubAppTokenCache struct { + token string + exp time.Time } func NewManagedSource(desc Descriptor, options Options) *ManagedSource { @@ -58,8 +62,7 @@ func (s *ManagedSource) Update(desc Descriptor) { if !s.desc.EqualSource(desc) { s.ghToken = "" s.ghCached = false - s.appToken = "" - s.appTokenExp = time.Time{} + s.appTokens = nil } s.desc = cloneDescriptor(desc) } @@ -68,8 +71,7 @@ func (s *ManagedSource) Invalidate() { s.mu.Lock() s.ghToken = "" s.ghCached = false - s.appToken = "" - s.appTokenExp = time.Time{} + s.appTokens = nil s.mu.Unlock() } @@ -119,6 +121,7 @@ func (s *ManagedSource) tokenFromCandidate( const githubAppTokenRefreshSkew = 5 * time.Minute type mutationAuthCtxKey struct{} +type githubOwnerCtxKey struct{} // WithMutationAuth marks ctx so token resolution skips github_app // installation tokens and resolves the user's own credential chain @@ -138,6 +141,22 @@ func IsMutationAuth(ctx context.Context) bool { return ok && marked } +// WithGitHubOwner scopes token resolution to a GitHub repository or account +// owner. GitHub App installation tokens are account-scoped, so a candidate for +// one installation account must not be used for another owner on the same host. +func WithGitHubOwner(ctx context.Context, owner string) context.Context { + owner = strings.TrimSpace(owner) + if owner == "" { + return ctx + } + return context.WithValue(ctx, githubOwnerCtxKey{}, owner) +} + +func githubOwnerFromContext(ctx context.Context) (string, bool) { + owner, ok := ctx.Value(githubOwnerCtxKey{}).(string) + return owner, ok && owner != "" +} + func (s *ManagedSource) githubAppToken( ctx context.Context, candidate Candidate, @@ -153,16 +172,22 @@ func (s *ManagedSource) githubAppToken( if IsMutationAuth(ctx) { return "", false, nil } + if candidate.InstallationAccount != "" { + owner, ok := githubOwnerFromContext(ctx) + if !ok || !strings.EqualFold(owner, candidate.InstallationAccount) { + return "", false, nil + } + } + cacheKey := canonicalCandidate(candidate) s.mu.Lock() minter := s.options.GitHubApp - token := s.appToken - exp := s.appTokenExp + cached := s.appTokens[cacheKey] s.mu.Unlock() if minter == nil { return "", false, nil } - if token != "" && time.Until(exp) > githubAppTokenRefreshSkew { - return token, true, nil + if cached.token != "" && time.Until(cached.exp) > githubAppTokenRefreshSkew { + return cached.token, true, nil } token, exp, err := minter(ctx, candidate) if err != nil { @@ -177,8 +202,10 @@ func (s *ManagedSource) githubAppToken( return "", true, nil } s.mu.Lock() - s.appToken = token - s.appTokenExp = exp + if s.appTokens == nil { + s.appTokens = make(map[Candidate]githubAppTokenCache) + } + s.appTokens[cacheKey] = githubAppTokenCache{token: token, exp: exp} s.mu.Unlock() return token, true, nil } diff --git a/internal/tokenauth/transport.go b/internal/tokenauth/transport.go index 27555cf86..c86b8b2c8 100644 --- a/internal/tokenauth/transport.go +++ b/internal/tokenauth/transport.go @@ -34,6 +34,7 @@ type AuthTransport struct { SetHeader HeaderSetter RetryOnUnauthorized bool AllowedOrigin string + GitHubOwner func(*http.Request) string } func (t AuthTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -73,7 +74,11 @@ func (t AuthTransport) authorizedRequest(req *http.Request) (*http.Request, erro if t.Source == nil { return nil, fmt.Errorf("%w: nil token source", ErrMissingToken) } - token, err := t.Source.Token(req.Context()) + ctx := req.Context() + if t.GitHubOwner != nil { + ctx = WithGitHubOwner(ctx, t.GitHubOwner(req)) + } + token, err := t.Source.Token(ctx) if err != nil { return nil, err } From 866089ac8f48d03c8978f0889ca0adf9020bb1cc Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 16:59:03 -0400 Subject: [PATCH 2/8] docs: keep GitHub App context generic The GitHub App owner-scope context should preserve the invariant without baking a maintainer account name or low-level call path into the docs. Keep the operational rule generic and leave implementation-specific cache detail next to the tokenauth state it explains. Validation: go test ./internal/tokenauth -shuffle=on; scripts/context-sync --check; git diff --check Generated with Codex Co-authored-by: Codex --- context/github-sync-invariants.md | 29 +++++++++-------------------- internal/tokenauth/source.go | 12 +++++++----- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index f4732c83f..24539bc49 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -116,27 +116,16 @@ GitHub's live manifest validator can report the missing hook URL as a generic `cmd/middleman-github-app/e2e_test.go::TestCreateFlowEndToEnd` asserting the serialized manifest shape so the fake cannot accept a payload GitHub rejects. -GitHub App installation tokens are account-scoped, not host-scoped. A -`github.com` app installation for `kenn-io` must not authenticate -`mariusvniekerk/*` reads just because both repos share the same host. Keep -`internal/tokenauth/source.go::WithGitHubOwner`, -`internal/tokenauth/source.go::ManagedSource.githubAppToken`, -`internal/github/client.go::githubOwnerFromRequest`, and -`internal/github/graphql.go::GraphQLFetcher.fetchRepoPRsWithPageSize` aligned: -repo-scoped REST and GraphQL reads must put the GitHub owner into token -resolution, token caches must be keyed per installation candidate, and -ownerless contexts such as clone auth must fall through to PAT/`gh` credentials. -Ownerless GitHub REST endpoints that are still logically scoped by an owner -argument, such as installation repository discovery in -`internal/github/client.go::liveClient.ListRepositoriesByOwner`, must set that -owner explicitly before resolving the token. +GitHub App installation tokens are account-scoped, not host-scoped. An app +installation for one owner must not authenticate reads for another owner just +because both repos share the same host. Repo-scoped GitHub reads must resolve app +tokens with the repository owner in context, and ownerless contexts such as +clone auth must fall through to PAT/`gh` credentials. Config may carry multiple `[[github_apps]]` rows for one host when the same app -is installed on multiple accounts; validation in -`internal/config/config.go::validateGitHubAppCoverage` applies selected-repo -coverage only to repos owned by that row's `installation_account`. The install -CLI must not warn that an installation on one account "cannot reach" repos owned -by another account; those repos are outside that installation's owner scope and -fall through to the next credential source. +is installed on multiple accounts. Selected-repository coverage applies only to +repos owned by that row's `installation_account`, and the install CLI must not +warn that an installation on one account "cannot reach" repos owned by another +account. ## Testing Expectations diff --git a/internal/tokenauth/source.go b/internal/tokenauth/source.go index f80208972..853b5f1a5 100644 --- a/internal/tokenauth/source.go +++ b/internal/tokenauth/source.go @@ -33,11 +33,13 @@ type Source interface { } type ManagedSource struct { - mu sync.Mutex - desc Descriptor - options Options - ghToken string - ghCached bool + mu sync.Mutex + desc Descriptor + options Options + ghToken string + ghCached bool + // App tokens are scoped to one installation candidate; a host source may + // carry several app candidates for different owners on the same GitHub host. appTokens map[Candidate]githubAppTokenCache } From 111535a7f7d9c3e2b79b3b5e13235c3fa1fe9a43 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 17:05:16 -0400 Subject: [PATCH 3/8] fix: support distinct GitHub Apps per host Private GitHub Apps are owned by one account in the setup middleman creates, so same-host multi-account support must come from multiple app credentials rather than treating one app as installable everywhere. Management commands now disambiguate same-host apps by owner, deletion removes the selected app entry, and validation rejects duplicate app owners or installation accounts while allowing distinct same-host app credentials. Validation: go test ./internal/config ./cmd/middleman-github-app -run 'TestGitHubAppsValidation|TestTokenSourceChainIncludesGitHubAppsForEachInstalledAccount|TestCreate|TestInstall|TestDelete|TestOpen|TestUninstall' -shuffle=on; go test ./internal/tokenauth ./internal/config ./internal/github/... ./internal/server/e2etest ./cmd/middleman ./cmd/middleman-github-app -shuffle=on; scripts/context-sync --check; git diff --check Generated with Codex Co-authored-by: Codex --- cmd/middleman-github-app/create.go | 22 ++++-- cmd/middleman-github-app/manage.go | 14 ++-- cmd/middleman-github-app/shared.go | 84 +++++++++++++---------- context/github-sync-invariants.md | 4 +- internal/config/config.go | 42 ++++++++---- internal/config/github_app_config_test.go | 26 ++++++- 6 files changed, 127 insertions(+), 65 deletions(-) diff --git a/cmd/middleman-github-app/create.go b/cmd/middleman-github-app/create.go index db53d31d4..a7a646101 100644 --- a/cmd/middleman-github-app/create.go +++ b/cmd/middleman-github-app/create.go @@ -49,12 +49,22 @@ func runCreate(args []string, env *appEnv) error { if err != nil { return err } - if existing, ok := cfg.GitHubAppForHost(h); ok { - return fmt.Errorf( - "a github app for host %q already exists (app id %d, slug %q); "+ - "use \"install\" to add an installation or \"delete\" to replace it", - h, existing.AppID, existing.Slug, - ) + for _, existing := range cfg.GitHubAppsForHost(h) { + if *org != "" && strings.EqualFold(existing.Owner, *org) { + return fmt.Errorf( + "a github app for host %q and owner %q already exists (app id %d, slug %q); "+ + "use \"install --owner %s\" to add an installation or \"delete --owner %s\" to replace it", + h, existing.Owner, existing.AppID, existing.Slug, existing.Owner, existing.Owner, + ) + } + if *org == "" && strings.EqualFold(existing.OwnerType, "User") { + return fmt.Errorf( + "a user-owned github app for host %q already exists (app id %d, slug %q); "+ + "use \"install --owner %s\" to add an installation, \"create --org\" for an org-owned app, "+ + "or \"delete --owner %s\" to replace it", + h, existing.AppID, existing.Slug, existing.Owner, existing.Owner, + ) + } } appName := strings.TrimSpace(*name) diff --git a/cmd/middleman-github-app/manage.go b/cmd/middleman-github-app/manage.go index a86c036d5..1d6f35555 100644 --- a/cmd/middleman-github-app/manage.go +++ b/cmd/middleman-github-app/manage.go @@ -20,6 +20,7 @@ func runInstall(args []string, env *appEnv) error { fs.SetOutput(env.stdout) configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to install") + owner := fs.String("owner", "", "GitHub account that owns the app or installation") noBrowser := fs.Bool("no-browser", false, "print URLs instead of opening a browser") timeout := fs.Duration("timeout", 10*time.Minute, "how long to wait for the installation") registerTestFlags(fs, env) @@ -31,7 +32,7 @@ func runInstall(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host) + app, err := selectApp(cfg, *host, *owner) if err != nil { return err } @@ -45,6 +46,7 @@ func runUninstall(args []string, env *appEnv) error { fs.SetOutput(env.stdout) configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to uninstall") + owner := fs.String("owner", "", "GitHub account that owns the app or installation") yes := fs.Bool("yes", false, "confirm uninstalling without prompting") registerTestFlags(fs, env) if err := fs.Parse(args); err != nil { @@ -55,7 +57,7 @@ func runUninstall(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host) + app, err := selectApp(cfg, *host, *owner) if err != nil { return err } @@ -94,6 +96,7 @@ func runDelete(args []string, env *appEnv) error { fs.SetOutput(env.stdout) configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to delete") + owner := fs.String("owner", "", "GitHub account that owns the app or installation") yes := fs.Bool("yes", false, "confirm deletion without prompting") localOnly := fs.Bool("local-only", false, "only remove the local config entry and key (app already deleted on GitHub)") @@ -108,7 +111,7 @@ func runDelete(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host) + app, err := selectApp(cfg, *host, *owner) if err != nil { return err } @@ -179,7 +182,7 @@ func runDelete(args []string, env *appEnv) error { } } - if err := removeAppFromConfig(cfg, env.configPath, app.Host); err != nil { + if err := removeAppFromConfig(cfg, env.configPath, app); err != nil { return err } if appPrivateKeyOwnedByCLI(env.configPath, configuredApp, app.Slug) { @@ -228,6 +231,7 @@ func runOpen(args []string, env *appEnv) error { fs.SetOutput(env.stdout) configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to open") + owner := fs.String("owner", "", "GitHub account that owns the app or installation") registerTestFlags(fs, env) if err := fs.Parse(args); err != nil { return err @@ -237,7 +241,7 @@ func runOpen(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host) + app, err := selectApp(cfg, *host, *owner) if err != nil { return err } diff --git a/cmd/middleman-github-app/shared.go b/cmd/middleman-github-app/shared.go index a918b2649..483444fcd 100644 --- a/cmd/middleman-github-app/shared.go +++ b/cmd/middleman-github-app/shared.go @@ -38,29 +38,45 @@ func (env *appEnv) webBaseFor(host string) string { return githubapp.WebBaseForHost(host) } -// selectApp picks a configured app credential for host. Multiple installation -// rows can share one GitHub App credential; management commands only need one -// of those rows to sign app JWTs. -func selectApp(cfg *config.Config, host string) (config.GitHubAppConfig, error) { - if host == "" { - switch len(cfg.GitHubApps) { - case 0: - return config.GitHubAppConfig{}, fmt.Errorf( - "no github apps configured; run \"middleman-github-app create\" first", - ) - case 1: - return cfg.GitHubApps[0], nil - default: +// selectApp picks a configured app credential. Same-host configs can carry +// multiple private apps, so management commands must disambiguate by owner when +// a host alone is not unique. +func selectApp(cfg *config.Config, host, owner string) (config.GitHubAppConfig, error) { + if cfg == nil || len(cfg.GitHubApps) == 0 { + return config.GitHubAppConfig{}, fmt.Errorf( + "no github apps configured; run \"middleman-github-app create\" first", + ) + } + owner = strings.TrimSpace(owner) + var matches []config.GitHubAppConfig + for _, app := range cfg.GitHubApps { + if host != "" && app.Host != normalizeHostFlag(host) { + continue + } + if owner != "" && + !strings.EqualFold(app.Owner, owner) && + !strings.EqualFold(app.InstallationAccount, owner) { + continue + } + matches = append(matches, app) + } + if len(matches) == 0 { + if host != "" && owner != "" { return config.GitHubAppConfig{}, fmt.Errorf( - "multiple github apps configured; pass --host to pick one", + "no github app configured for host %q and owner %q", normalizeHostFlag(host), owner, ) } + if host != "" { + return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for host %q", normalizeHostFlag(host)) + } + return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for owner %q", owner) } - app, ok := cfg.GitHubAppForHost(host) - if !ok { - return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for host %q", host) + if len(matches) > 1 { + return config.GitHubAppConfig{}, fmt.Errorf( + "multiple github apps match; pass --owner to select one", + ) } - return app, nil + return matches[0], nil } func appJWT(app config.GitHubAppConfig, now time.Time) (string, error) { @@ -86,9 +102,8 @@ func installURL(webBase string, app config.GitHubAppConfig) string { return fmt.Sprintf("%s/apps/%s/installations/new", webBase, app.Slug) } -// updateAppInConfig replaces the entry for app.Host and app.InstallationAccount -// and saves. If a create flow saved a pending, uninstalled row for the same -// app credential, the first successful install replaces that pending row. +// updateAppInConfig replaces the matching app credential and saves. Same-host +// entries are distinct apps, usually one private app per GitHub account. func updateAppInConfig( cfg *config.Config, configPath string, app config.GitHubAppConfig, ) error { @@ -97,13 +112,13 @@ func updateAppInConfig( if existing.Host != app.Host { continue } - sameInstallation := strings.EqualFold( - existing.InstallationAccount, app.InstallationAccount, - ) - pendingSameApp := existing.InstallationAccount == "" && - existing.AppID == app.AppID && - existing.PrivateKeyPath == app.PrivateKeyPath - if sameInstallation || pendingSameApp { + sameAppID := existing.AppID == app.AppID + sameOwner := existing.Owner != "" && app.Owner != "" && + strings.EqualFold(existing.Owner, app.Owner) + sameInstallation := existing.InstallationAccount != "" && + app.InstallationAccount != "" && + strings.EqualFold(existing.InstallationAccount, app.InstallationAccount) + if sameAppID || sameOwner || sameInstallation { cfg.GitHubApps[i] = app return cfg.Save(configPath) } @@ -121,7 +136,7 @@ func updateAppSlotInConfig( for i := range cfg.GitHubApps { existing := cfg.GitHubApps[i] if existing.Host == oldApp.Host && - existing.InstallationID == oldApp.InstallationID && + existing.AppID == oldApp.AppID && strings.EqualFold(existing.InstallationAccount, oldApp.InstallationAccount) { cfg.GitHubApps[i] = newApp return cfg.Save(configPath) @@ -130,14 +145,13 @@ func updateAppSlotInConfig( return updateAppInConfig(cfg, configPath, newApp) } -func removeAppFromConfig( - cfg *config.Config, configPath, host string, -) error { +func removeAppFromConfig(cfg *config.Config, configPath string, app config.GitHubAppConfig) error { kept := cfg.GitHubApps[:0] - for _, app := range cfg.GitHubApps { - if app.Host != host { - kept = append(kept, app) + for _, existing := range cfg.GitHubApps { + if existing.Host == app.Host && existing.AppID == app.AppID { + continue } + kept = append(kept, existing) } cfg.GitHubApps = kept return cfg.Save(configPath) diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index 24539bc49..859277be6 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -121,8 +121,8 @@ installation for one owner must not authenticate reads for another owner just because both repos share the same host. Repo-scoped GitHub reads must resolve app tokens with the repository owner in context, and ownerless contexts such as clone auth must fall through to PAT/`gh` credentials. -Config may carry multiple `[[github_apps]]` rows for one host when the same app -is installed on multiple accounts. Selected-repository coverage applies only to +Config may carry multiple `[[github_apps]]` rows for one host, but those rows +represent distinct app credentials. Selected-repository coverage applies only to repos owned by that row's `installation_account`, and the install CLI must not warn that an installation on one account "cannot reach" repos owned by another account. diff --git a/internal/config/config.go b/internal/config/config.go index eb8b7242e..b228b2fcf 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1704,6 +1704,7 @@ func (c *Config) validatePlatforms() error { } func (c *Config) validateGitHubApps() error { + seenOwners := make(map[string]struct{}, len(c.GitHubApps)) seenInstallations := make(map[string]struct{}, len(c.GitHubApps)) for i := range c.GitHubApps { app := &c.GitHubApps[i] @@ -1751,22 +1752,35 @@ func (c *Config) validateGitHubApps() error { "installation_id is set", i, ) } - // GitHub App credentials are host-shaped, but installation tokens are - // account-scoped. Multiple rows may share a host when the same app is - // installed on multiple owners; only one row per installation account - // can be selected for a given owner. - installKey := host + "\x00" + strings.ToLower(app.InstallationAccount) - if _, ok := seenInstallations[installKey]; ok { - account := app.InstallationAccount - if account == "" { - account = "" + // GitHub App credentials are host-shaped, but private apps are owned by + // one account. Multiple rows may share a host only when they describe + // distinct apps, selected by app owner or installation account. + owner := strings.ToLower(app.Owner) + if owner == "" { + owner = fmt.Sprintf("app:%d", app.AppID) + } + ownerKey := host + "\x00" + owner + if _, ok := seenOwners[ownerKey]; ok { + label := app.Owner + if label == "" { + label = fmt.Sprintf("app:%d", app.AppID) } return fmt.Errorf( - "config: github_apps[%d]: duplicate github app for host %q and installation account %q", - i, host, account, + "config: github_apps[%d]: duplicate github app for host %q and owner %q", + i, host, label, ) } - seenInstallations[installKey] = struct{}{} + seenOwners[ownerKey] = struct{}{} + if app.InstallationAccount != "" { + installKey := host + "\x00" + strings.ToLower(app.InstallationAccount) + if _, ok := seenInstallations[installKey]; ok { + return fmt.Errorf( + "config: github_apps[%d]: duplicate github app installation for host %q and account %q", + i, host, app.InstallationAccount, + ) + } + seenInstallations[installKey] = struct{}{} + } } return nil } @@ -1857,8 +1871,8 @@ func (c *Config) GitHubAppsForHost(host string) []GitHubAppConfig { } // GitHubAppForHost returns a configured GitHub App credential for host, if any. -// When multiple installation rows share one app credential, the first row is -// enough for management operations that only need the app's private key. +// Callers that manage app state should use GitHubAppsForHost and disambiguate +// when multiple app credentials share one host. func (c *Config) GitHubAppForHost(host string) (GitHubAppConfig, bool) { apps := c.GitHubAppsForHost(host) if len(apps) == 0 { diff --git a/internal/config/github_app_config_test.go b/internal/config/github_app_config_test.go index ef86c53e2..d9b8581aa 100644 --- a/internal/config/github_app_config_test.go +++ b/internal/config/github_app_config_test.go @@ -93,6 +93,7 @@ installation_id = 5 toml: ` [[github_apps]] app_id = 1 +owner = "app-owner-a" private_key_path = "a.pem" installation_id = 10 installation_account = "kenn-io" @@ -101,12 +102,29 @@ repository_selection = "all" [[github_apps]] host = "github.com" app_id = 2 +owner = "app-owner-b" private_key_path = "b.pem" installation_id = 11 installation_account = "KENN-IO" repository_selection = "all" `, - wantErr: `duplicate github app for host "github.com" and installation account "KENN-IO"`, + wantErr: `duplicate github app installation for host "github.com" and account "KENN-IO"`, + }, + { + name: "duplicate app owner", + toml: ` +[[github_apps]] +app_id = 1 +owner = "kenn-io" +private_key_path = "a.pem" + +[[github_apps]] +host = "github.com" +app_id = 2 +owner = "KENN-IO" +private_key_path = "b.pem" +`, + wantErr: `duplicate github app for host "github.com" and owner "KENN-IO"`, }, } for _, tt := range tests { @@ -408,6 +426,7 @@ token_env = "OTHER_ORG_PAT" [[github_apps]] host = "github.com" app_id = 4321 +owner = "kenn-io" private_key_path = "app.pem" installation_id = 99 installation_account = "kenn-io" @@ -490,8 +509,9 @@ repository_selection = "all" [[github_apps]] host = "github.com" -app_id = 4321 -private_key_path = "app.pem" +app_id = 4322 +owner = "other-org" +private_key_path = "other-app.pem" installation_id = 100 installation_account = "other-org" repository_selection = "all" From 6aa38c260eb3b1434878b6e30138529d40e24a90 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 17:25:25 -0400 Subject: [PATCH 4/8] fix: make GitHub App ownership selectable Same-host GitHub App configs now model separate private app credentials, so management has to target the credential itself instead of treating installation account as a replacement key. Without that, a duplicate install could overwrite the wrong row and ownerless legacy rows had no stable selector. Reload and GraphQL reads also need the repository owner carried into token resolution; otherwise app-only installs can fail during hot reload or nested review-thread pagination even though startup and top-level reads work. Validation: go test ./internal/server ./internal/github ./cmd/middleman-github-app -run 'TestValidateReloadProviderTokenSourcesScopesGitHubAppByOwner|TestListPullRequestReviewThreadsScopesPaginatedCommentAuthByOwner|TestCreateAllowsOrgOwnedAppForSameHost|TestManageSameHostAppsByOwnerOrAppID|TestInstallRejectsDuplicateInstallationAccountAcrossApps|TestListReportsInstallationAndRateBudget' -shuffle=on; go test ./internal/server -run 'TestValidateReloadProviderTokenSourcesScopesGitHubAppByOwner' -shuffle=on; go test ./cmd/middleman-github-app -shuffle=on; go test ./internal/github -shuffle=on; GOFLAGS="${GOFLAGS:+$GOFLAGS }-buildvcs=false" go run ./cmd/testify-helper-check ./cmd/middleman-github-app; scripts/context-sync --check; git diff --check. Broader go test ./internal/tokenauth ./internal/config ./internal/github/... ./internal/server ./internal/server/e2etest ./cmd/middleman ./cmd/middleman-github-app -shuffle=on failed only in existing internal/server fleet tmux monitor tests under shuffle. Generated with Codex Co-authored-by: Codex --- cmd/middleman-github-app/create.go | 5 +- cmd/middleman-github-app/e2e_test.go | 125 +++++++++++++++++++++++ cmd/middleman-github-app/list.go | 15 ++- cmd/middleman-github-app/manage.go | 12 ++- cmd/middleman-github-app/shared.go | 57 +++++++---- context/github-sync-invariants.md | 8 +- internal/github/client.go | 1 + internal/github/client_test.go | 56 ++++++++++ internal/githubapp/githubapptest/fake.go | 49 ++++++--- internal/server/config_reload.go | 9 +- internal/server/config_reload_test.go | 42 ++++++++ 11 files changed, 330 insertions(+), 49 deletions(-) diff --git a/cmd/middleman-github-app/create.go b/cmd/middleman-github-app/create.go index a7a646101..9d338d697 100644 --- a/cmd/middleman-github-app/create.go +++ b/cmd/middleman-github-app/create.go @@ -40,7 +40,10 @@ func runCreate(args []string, env *appEnv) error { return err } env.configPath = *configPath - h := normalizeHostFlag(*host) + h, err := normalizeHostFlag(*host) + if err != nil { + return err + } if err := config.EnsureDefault(env.configPath); err != nil { return fmt.Errorf("ensuring middleman config exists: %w", err) diff --git a/cmd/middleman-github-app/e2e_test.go b/cmd/middleman-github-app/e2e_test.go index 351a81767..be6b22de7 100644 --- a/cmd/middleman-github-app/e2e_test.go +++ b/cmd/middleman-github-app/e2e_test.go @@ -180,6 +180,17 @@ func createTestApp(t *testing.T, fake *githubapptest.Fake, configPath, name stri }, env)) } +func createOrgTestApp( + t *testing.T, fake *githubapptest.Fake, configPath, name, org string, +) { + t.Helper() + env, _ := newTestEnv(t, fake, configPath) + env.openBrowser = scriptBrowser(t, fake, org) + require.NoError(t, runCLI([]string{ + "create", "--name", name, "--org", org, "--timeout", "10s", + }, env)) +} + func TestCreateFlowEndToEnd(t *testing.T) { t.Parallel() fake := githubapptest.NewFake() @@ -259,6 +270,26 @@ func TestCreateRefusesSecondAppForSameHost(t *testing.T) { assert.ErrorContains(t, err, "already exists") } +func TestCreateAllowsOrgOwnedAppForSameHost(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-user") + createOrgTestApp(t, fake, configPath, "middleman-org", "acme") + + cfg, err := config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + assert := assert.New(t) + assert.Equal("fake-owner", cfg.GitHubApps[0].Owner) + assert.Equal("User", cfg.GitHubApps[0].OwnerType) + assert.Equal("acme", cfg.GitHubApps[1].Owner) + assert.Equal("Organization", cfg.GitHubApps[1].OwnerType) + assert.Equal("acme", cfg.GitHubApps[1].InstallationAccount) +} + func TestListReportsInstallationAndRateBudget(t *testing.T) { t.Parallel() fake := githubapptest.NewFake() @@ -275,6 +306,8 @@ func TestListReportsInstallationAndRateBudget(t *testing.T) { require.Len(t, statuses, 1) assert := assert.New(t) assert.Equal("middleman-list", statuses[0].Slug) + assert.Equal("fake-owner", statuses[0].Owner) + assert.Equal("User", statuses[0].OwnerType) assert.Equal("kenn-io", statuses[0].InstallationAccount) assert.Equal(5000, statuses[0].RateLimit) assert.Empty(statuses[0].Error) @@ -283,6 +316,98 @@ func TestListReportsInstallationAndRateBudget(t *testing.T) { assert.Equal(5000, statuses[0].RateRemaining) } +func TestManageSameHostAppsByOwnerOrAppID(t *testing.T) { + t.Parallel() + require := require.New(t) + assert := assert.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-user") + createOrgTestApp(t, fake, configPath, "middleman-org", "acme") + + cfg, err := config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + userApp := cfg.GitHubApps[0] + orgApp := cfg.GitHubApps[1] + + env, _ := newTestEnv(t, fake, configPath) + err = runCLI([]string{"open"}, env) + require.Error(err) + require.ErrorContains(err, "--owner or --app-id") + + var opened string + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = func(target string) error { + opened = target + return nil + } + require.NoError(runCLI([]string{"open", "--host", "https://github.com/", "--owner", "acme"}, env)) + assert.Contains(opened, "/organizations/acme/settings/apps/middleman-org") + + env, _ = newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--owner", "acme", "--yes"}, env)) + cfg, err = config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + assert.NotZero(cfg.GitHubApps[0].InstallationID) + assert.Equal(userApp.AppID, cfg.GitHubApps[0].AppID) + assert.Zero(cfg.GitHubApps[1].InstallationID) + assert.Equal(orgApp.AppID, cfg.GitHubApps[1].AppID) + + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = scriptBrowser(t, fake, "acme") + require.NoError(runCLI([]string{ + "install", "--app-id", fmt.Sprint(orgApp.AppID), "--timeout", "10s", + }, env)) + cfg, err = config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + assert.NotZero(cfg.GitHubApps[1].InstallationID) + assert.Equal("acme", cfg.GitHubApps[1].InstallationAccount) + + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = scriptBrowser(t, fake, "acme") + require.NoError(runCLI([]string{"delete", "--owner", "acme", "--yes", "--timeout", "10s"}, env)) + cfg, err = config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + assert.Equal(userApp.AppID, cfg.GitHubApps[0].AppID) +} + +func TestInstallRejectsDuplicateInstallationAccountAcrossApps(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-user") + createOrgTestApp(t, fake, configPath, "middleman-org", "acme") + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--owner", "acme", "--yes"}, env)) + + cfg, err := config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + orgAppID := cfg.GitHubApps[1].AppID + + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = scriptBrowser(t, fake, "kenn-io") + err = runCLI([]string{ + "install", "--app-id", fmt.Sprint(orgAppID), "--timeout", "10s", + }, env) + require.Error(err) + require.ErrorContains(err, "duplicate github app installation") + + cfg, err = config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 2) + assert.Zero(t, cfg.GitHubApps[1].InstallationID) + assert.Empty(t, cfg.GitHubApps[1].InstallationAccount) +} + func TestUninstallClearsInstallationButKeepsApp(t *testing.T) { t.Parallel() fake := githubapptest.NewFake() diff --git a/cmd/middleman-github-app/list.go b/cmd/middleman-github-app/list.go index df02124e5..b422000ef 100644 --- a/cmd/middleman-github-app/list.go +++ b/cmd/middleman-github-app/list.go @@ -16,6 +16,8 @@ type appStatus struct { Host string `json:"host"` AppID int64 `json:"app_id"` Slug string `json:"slug"` + Owner string `json:"owner,omitempty"` + OwnerType string `json:"owner_type,omitempty"` InstallationID int64 `json:"installation_id,omitempty"` InstallationAccount string `json:"installation_account,omitempty"` RateLimit int `json:"rate_limit,omitempty"` @@ -51,6 +53,8 @@ func runList(args []string, env *appEnv) error { Host: app.Host, AppID: app.AppID, Slug: app.Slug, + Owner: app.Owner, + OwnerType: app.OwnerType, InstallationID: app.InstallationID, InstallationAccount: app.InstallationAccount, } @@ -66,9 +70,12 @@ func runList(args []string, env *appEnv) error { return enc.Encode(statuses) } w := tabwriter.NewWriter(env.stdout, 2, 4, 2, ' ', 0) - fmt.Fprintln(w, "HOST\tAPP ID\tSLUG\tINSTALLATION\tACCOUNT\tRATE (CORE)\tSTATUS") + fmt.Fprintln(w, "HOST\tAPP ID\tSLUG\tOWNER\tINSTALLATION\tACCOUNT\tRATE (CORE)\tSTATUS") for _, s := range statuses { - rate, install, account := "-", "-", "-" + rate, owner, install, account := "-", "-", "-", "-" + if s.Owner != "" { + owner = s.Owner + } if s.InstallationID != 0 { install = fmt.Sprintf("%d", s.InstallationID) } @@ -84,8 +91,8 @@ func runList(args []string, env *appEnv) error { } else if s.InstallationID == 0 { status = "not installed" } - fmt.Fprintf(w, "%s\t%d\t%s\t%s\t%s\t%s\t%s\n", - s.Host, s.AppID, s.Slug, install, account, rate, status) + fmt.Fprintf(w, "%s\t%d\t%s\t%s\t%s\t%s\t%s\t%s\n", + s.Host, s.AppID, s.Slug, owner, install, account, rate, status) } return w.Flush() } diff --git a/cmd/middleman-github-app/manage.go b/cmd/middleman-github-app/manage.go index 1d6f35555..c1353c63b 100644 --- a/cmd/middleman-github-app/manage.go +++ b/cmd/middleman-github-app/manage.go @@ -21,6 +21,7 @@ func runInstall(args []string, env *appEnv) error { configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to install") owner := fs.String("owner", "", "GitHub account that owns the app or installation") + appID := fs.Int64("app-id", 0, "GitHub App ID to select when host or owner is ambiguous") noBrowser := fs.Bool("no-browser", false, "print URLs instead of opening a browser") timeout := fs.Duration("timeout", 10*time.Minute, "how long to wait for the installation") registerTestFlags(fs, env) @@ -32,7 +33,7 @@ func runInstall(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host, *owner) + app, err := selectApp(cfg, *host, *owner, *appID) if err != nil { return err } @@ -47,6 +48,7 @@ func runUninstall(args []string, env *appEnv) error { configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to uninstall") owner := fs.String("owner", "", "GitHub account that owns the app or installation") + appID := fs.Int64("app-id", 0, "GitHub App ID to select when host or owner is ambiguous") yes := fs.Bool("yes", false, "confirm uninstalling without prompting") registerTestFlags(fs, env) if err := fs.Parse(args); err != nil { @@ -57,7 +59,7 @@ func runUninstall(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host, *owner) + app, err := selectApp(cfg, *host, *owner, *appID) if err != nil { return err } @@ -97,6 +99,7 @@ func runDelete(args []string, env *appEnv) error { configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to delete") owner := fs.String("owner", "", "GitHub account that owns the app or installation") + appID := fs.Int64("app-id", 0, "GitHub App ID to select when host or owner is ambiguous") yes := fs.Bool("yes", false, "confirm deletion without prompting") localOnly := fs.Bool("local-only", false, "only remove the local config entry and key (app already deleted on GitHub)") @@ -111,7 +114,7 @@ func runDelete(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host, *owner) + app, err := selectApp(cfg, *host, *owner, *appID) if err != nil { return err } @@ -232,6 +235,7 @@ func runOpen(args []string, env *appEnv) error { configPath := fs.String("config", env.configPath, "middleman config path") host := fs.String("host", "", "GitHub host of the app to open") owner := fs.String("owner", "", "GitHub account that owns the app or installation") + appID := fs.Int64("app-id", 0, "GitHub App ID to select when host or owner is ambiguous") registerTestFlags(fs, env) if err := fs.Parse(args); err != nil { return err @@ -241,7 +245,7 @@ func runOpen(args []string, env *appEnv) error { if err != nil { return err } - app, err := selectApp(cfg, *host, *owner) + app, err := selectApp(cfg, *host, *owner, *appID) if err != nil { return err } diff --git a/cmd/middleman-github-app/shared.go b/cmd/middleman-github-app/shared.go index 483444fcd..be1373120 100644 --- a/cmd/middleman-github-app/shared.go +++ b/cmd/middleman-github-app/shared.go @@ -8,7 +8,6 @@ import ( "go.kenn.io/middleman/internal/config" "go.kenn.io/middleman/internal/githubapp" - "go.kenn.io/middleman/internal/platform" ) // loadConfig loads with GitHub App coverage validation relaxed: every @@ -39,18 +38,31 @@ func (env *appEnv) webBaseFor(host string) string { } // selectApp picks a configured app credential. Same-host configs can carry -// multiple private apps, so management commands must disambiguate by owner when -// a host alone is not unique. -func selectApp(cfg *config.Config, host, owner string) (config.GitHubAppConfig, error) { +// multiple private apps, so management commands must disambiguate by owner or +// app id when a host alone is not unique. +func selectApp( + cfg *config.Config, host, owner string, appID int64, +) (config.GitHubAppConfig, error) { if cfg == nil || len(cfg.GitHubApps) == 0 { return config.GitHubAppConfig{}, fmt.Errorf( "no github apps configured; run \"middleman-github-app create\" first", ) } + normalizedHost := "" + if strings.TrimSpace(host) != "" { + var err error + normalizedHost, err = normalizeHostFlag(host) + if err != nil { + return config.GitHubAppConfig{}, err + } + } owner = strings.TrimSpace(owner) var matches []config.GitHubAppConfig for _, app := range cfg.GitHubApps { - if host != "" && app.Host != normalizeHostFlag(host) { + if normalizedHost != "" && app.Host != normalizedHost { + continue + } + if appID != 0 && app.AppID != appID { continue } if owner != "" && @@ -61,19 +73,27 @@ func selectApp(cfg *config.Config, host, owner string) (config.GitHubAppConfig, matches = append(matches, app) } if len(matches) == 0 { - if host != "" && owner != "" { + if normalizedHost != "" && owner != "" { return config.GitHubAppConfig{}, fmt.Errorf( - "no github app configured for host %q and owner %q", normalizeHostFlag(host), owner, + "no github app configured for host %q and owner %q", normalizedHost, owner, ) } - if host != "" { - return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for host %q", normalizeHostFlag(host)) + if normalizedHost != "" && appID != 0 { + return config.GitHubAppConfig{}, fmt.Errorf( + "no github app configured for host %q and app id %d", normalizedHost, appID, + ) + } + if normalizedHost != "" { + return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for host %q", normalizedHost) + } + if appID != 0 { + return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for app id %d", appID) } return config.GitHubAppConfig{}, fmt.Errorf("no github app configured for owner %q", owner) } if len(matches) > 1 { return config.GitHubAppConfig{}, fmt.Errorf( - "multiple github apps match; pass --owner to select one", + "multiple github apps match; pass --owner or --app-id to select one", ) } return matches[0], nil @@ -112,13 +132,7 @@ func updateAppInConfig( if existing.Host != app.Host { continue } - sameAppID := existing.AppID == app.AppID - sameOwner := existing.Owner != "" && app.Owner != "" && - strings.EqualFold(existing.Owner, app.Owner) - sameInstallation := existing.InstallationAccount != "" && - app.InstallationAccount != "" && - strings.EqualFold(existing.InstallationAccount, app.InstallationAccount) - if sameAppID || sameOwner || sameInstallation { + if existing.AppID == app.AppID { cfg.GitHubApps[i] = app return cfg.Save(configPath) } @@ -222,10 +236,11 @@ func (env *appEnv) pollUntil( } } -func normalizeHostFlag(host string) string { +func normalizeHostFlag(host string) (string, error) { host = strings.TrimSpace(host) - if host == "" { - return platform.DefaultGitHubHost + normalized, err := config.NormalizePlatformHost("github", host) + if err != nil { + return "", err } - return host + return normalized, nil } diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index 859277be6..0356e7b02 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -122,9 +122,11 @@ because both repos share the same host. Repo-scoped GitHub reads must resolve ap tokens with the repository owner in context, and ownerless contexts such as clone auth must fall through to PAT/`gh` credentials. Config may carry multiple `[[github_apps]]` rows for one host, but those rows -represent distinct app credentials. Selected-repository coverage applies only to -repos owned by that row's `installation_account`, and the install CLI must not -warn that an installation on one account "cannot reach" repos owned by another +represent distinct app credentials. Management commands must target one row by +app owner/installation account or app id, and duplicate installation accounts on +the same host are invalid. Selected-repository coverage applies only to repos +owned by that row's `installation_account`, and the install CLI must not warn +that an installation on one account "cannot reach" repos owned by another account. ## Testing Expectations diff --git a/internal/github/client.go b/internal/github/client.go index e44446883..488ea1274 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -1637,6 +1637,7 @@ func (c *liveClient) ListPullRequestReviewThreads( repo string, number int, ) ([]PullRequestReviewThread, error) { + ctx = tokenauth.WithGitHubOwner(ctx, owner) type graphQLResponse struct { Errors []graphQLError `json:"errors"` Data struct { diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 331c19689..95cc2f722 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -679,6 +679,62 @@ func TestListPullRequestReviewThreads(t *testing.T) { assert.Equal([]string{"application/json", "application/json", "application/json"}, contentTypes) } +func TestListPullRequestReviewThreadsScopesPaginatedCommentAuthByOwner(t *testing.T) { + assert := Assert.New(t) + require := require.New(t) + var calls int + var minted int + mux := http.NewServeMux() + mux.HandleFunc("/graphql", func(w http.ResponseWriter, r *http.Request) { + calls++ + assert.Equal("Bearer app-token", r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + if calls == 1 { + _, _ = w.Write([]byte(`{"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[{"id":"PRRT_1","isResolved":false,"isOutdated":false,"path":"src/main.go","line":12,"originalLine":12,"diffSide":"RIGHT","comments":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"comment-cursor-1"}}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}`)) + return + } + _, _ = w.Write([]byte(`{"data":{"node":{"comments":{"nodes":[{"id":"PRRC_1","databaseId":101,"fullDatabaseId":101,"body":"reply","path":"src/main.go","line":12,"originalLine":12,"subjectType":"LINE","diffHunk":"@@","url":"https://github.example/pr#discussion_r101","author":{"login":"reviewer"},"createdAt":"2026-05-27T16:01:31Z","updatedAt":"2026-05-27T16:02:31Z"}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}`)) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + src := tokenauth.NewManagedSource(tokenauth.Descriptor{ + Key: tokenauth.Key{Platform: "github", Host: "github.com"}, + Candidates: []tokenauth.Candidate{{ + Kind: tokenauth.SourceKindGitHubApp, + Host: "github.com", + AppID: 42, + InstallationID: 7, + InstallationAccount: "owner", + }}, + }, tokenauth.Options{ + GitHubApp: func(_ context.Context, c tokenauth.Candidate) (string, time.Time, error) { + minted++ + assert.Equal("owner", c.InstallationAccount) + return "app-token", time.Now().Add(time.Hour), nil + }, + }) + auth := tokenauth.AuthTransport{ + Source: src, + Base: http.DefaultTransport, + SetHeader: tokenauth.BearerAuthHeader, + AllowedOrigin: srv.URL, + GitHubOwner: githubOwnerFromRequest, + } + c := &liveClient{ + httpClient: &http.Client{Transport: auth}, + graphQLEndpoint: srv.URL + "/graphql", + } + + threads, err := c.ListPullRequestReviewThreads(t.Context(), "owner", "repo", 42) + require.NoError(err) + require.Len(threads, 1) + require.Len(threads[0].Comments, 1) + assert.Equal("reply", threads[0].Comments[0].Body) + assert.Equal(2, calls) + assert.Equal(1, minted) +} + func TestListPullRequestTimelineEventsReturnsGraphQLErrors(t *testing.T) { require := require.New(t) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/githubapp/githubapptest/fake.go b/internal/githubapp/githubapptest/fake.go index 8d4f6e079..1ff84f1c8 100644 --- a/internal/githubapp/githubapptest/fake.go +++ b/internal/githubapp/githubapptest/fake.go @@ -36,6 +36,11 @@ type App struct { Deleted bool } +type appOwner struct { + Login string + Type string +} + type Installation struct { ID int64 Account string @@ -58,23 +63,25 @@ type mintedToken struct { // Fake is an in-process GitHub stand-in. URL() serves both the web // surface (manifest form POST target) and the REST API. type Fake struct { - mu sync.Mutex - srv *httptest.Server - apps map[int64]*App - pendingCodes map[string]string // conversion code -> manifest JSON - tokens map[string]*mintedToken - nextID int64 - rateLimit int - manifests []string // every manifest JSON received, in order + mu sync.Mutex + srv *httptest.Server + apps map[int64]*App + pendingCodes map[string]string // conversion code -> manifest JSON + pendingOwners map[string]appOwner + tokens map[string]*mintedToken + nextID int64 + rateLimit int + manifests []string // every manifest JSON received, in order } func NewFake() *Fake { f := &Fake{ - apps: make(map[int64]*App), - pendingCodes: make(map[string]string), - tokens: make(map[string]*mintedToken), - nextID: 100, - rateLimit: 5000, + apps: make(map[int64]*App), + pendingCodes: make(map[string]string), + pendingOwners: make(map[string]appOwner), + tokens: make(map[string]*mintedToken), + nextID: 100, + rateLimit: 5000, } mux := http.NewServeMux() mux.HandleFunc("POST /settings/apps/new", f.handleManifestSubmit) @@ -234,9 +241,16 @@ func (f *Fake) handleManifestSubmit(w http.ResponseWriter, r *http.Request) { http.Error(w, "manifest missing hook_attributes.url", http.StatusBadRequest) return } + owner := "fake-owner" + ownerType := "User" + if org := r.PathValue("org"); org != "" { + owner = org + ownerType = "Organization" + } code := randomHex(16) f.mu.Lock() f.pendingCodes[code] = manifest + f.pendingOwners[code] = appOwner{Login: owner, Type: ownerType} f.manifests = append(f.manifests, manifest) f.mu.Unlock() redirect, err := url.Parse(parsed.RedirectURL) @@ -257,14 +271,19 @@ func (f *Fake) handleConversion(w http.ResponseWriter, r *http.Request) { code := r.PathValue("code") f.mu.Lock() manifest, ok := f.pendingCodes[code] + owner := f.pendingOwners[code] if ok { delete(f.pendingCodes, code) + delete(f.pendingOwners, code) } f.mu.Unlock() if !ok { writeJSONError(w, http.StatusNotFound, "unknown or used conversion code") return } + if owner.Login == "" { + owner = appOwner{Login: "fake-owner", Type: "User"} + } var parsed struct { Name string `json:"name"` } @@ -288,8 +307,8 @@ func (f *Fake) handleConversion(w http.ResponseWriter, r *http.Request) { Name: parsed.Name, Key: key, PEM: string(pemBytes), - Owner: "fake-owner", - OwnerType: "User", + Owner: owner.Login, + OwnerType: owner.Type, } f.apps[app.ID] = app f.mu.Unlock() diff --git a/internal/server/config_reload.go b/internal/server/config_reload.go index c295db735..a5df5cd1f 100644 --- a/internal/server/config_reload.go +++ b/internal/server/config_reload.go @@ -533,8 +533,15 @@ func (s *Server) validateReloadProviderTokenSources( if _, ok := s.tokenSources.Get(desc.Key); !ok { continue } - if _, err := s.tokenSources.ProbeToken(ctx, desc); err != nil { + tokenCtx := ctx + if plan.GitHubOwner != "" { + tokenCtx = tokenauth.WithGitHubOwner(tokenCtx, plan.GitHubOwner) + } + if _, err := s.tokenSources.ProbeToken(tokenCtx, desc); err != nil { label := fmt.Sprintf("%s host %s", desc.Key.Platform, desc.Key.Host) + if plan.GitHubOwner != "" { + label = fmt.Sprintf("%s owner %s", label, plan.GitHubOwner) + } return fmt.Errorf( "no token for %s via %s: %w", label, desc.SafeString(), err, ) diff --git a/internal/server/config_reload_test.go b/internal/server/config_reload_test.go index 049f28c09..afd1642d1 100644 --- a/internal/server/config_reload_test.go +++ b/internal/server/config_reload_test.go @@ -1072,6 +1072,48 @@ repository_selection = "all" assert.Equal(int64(4242), apps[0].AppID) } +func TestValidateReloadProviderTokenSourcesScopesGitHubAppByOwner(t *testing.T) { + require := require.New(t) + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.toml") + require.NoError(os.WriteFile(filepath.Join(dir, "app.pem"), []byte("pem"), 0o600)) + writeConfigToml(t, cfgPath, ` +sync_interval = "5m" +github_token_env = "UNUSED_RELOAD_GITHUB_TOKEN" + +[[repos]] +owner = "acme" +name = "widget" + +[[github_apps]] +host = "github.com" +app_id = 4242 +private_key_path = "app.pem" +installation_id = 7 +installation_account = "acme" +repository_selection = "all" +`) + cfg, err := config.Load(cfgPath) + require.NoError(err) + + var minted []tokenauth.Candidate + set := tokenauth.NewSourceSet(tokenauth.Options{ + GitHubApp: func(_ context.Context, c tokenauth.Candidate) (string, time.Time, error) { + minted = append(minted, c) + return "app-token", time.Now().Add(time.Hour), nil + }, + }) + for _, plan := range cfg.ProviderTokenSources() { + set.Upsert(plan.Descriptor) + } + + srv := &Server{tokenSources: set} + require.NoError(srv.validateReloadProviderTokenSources(t.Context(), cfg)) + require.Len(minted, 1) + assert.Equal(t, int64(4242), minted[0].AppID) + assert.Equal(t, "acme", minted[0].InstallationAccount) +} + // newReloadServerWithTokenSources mirrors startup: one source per // provider token plan, registered in a SourceSet the server reloads // against. From cd1f0141fc1cee289a5ca44a07aecfd3d263fd49 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 19:52:14 -0400 Subject: [PATCH 5/8] fix: close GitHub App owner-scoping gaps in repo listing and install recovery When a host carries a GitHub App installation for one owner, other owners on that host stay on the PAT/gh chain. ListRepositoriesByOwner gated the installation-repositories endpoint on any app being active on the host, so listing repos for a PAT-backed owner that shares the host with another owner's app hit an installation-token-only endpoint with a PAT and 403'd. Gate that endpoint on whether reads for the requested owner actually resolve to an app token, mirroring the per-owner scoping token resolution already applies. The install flow snapshots existing installations as known and waits only for a brand-new id. But editing an installation's repository access or re-running after a selected-repo coverage failure reconfigures the existing installation without minting a new id, so the poll never completes -- the dead-end the coverage error's own "re-run install" guidance would otherwise lead users into. On poll timeout with exactly one installation present, adopt it; multiple installations stay ambiguous and keep the timeout, and a user interrupt is never treated as an adoptable timeout. Validation: go test ./internal/tokenauth ./internal/github ./cmd/middleman-github-app ./internal/config -shuffle=on; go vet; golangci-lint run (0 issues) Generated with Claude Code (Opus 4.8) Co-Authored-By: Claude Opus 4.8 --- cmd/middleman-github-app/create.go | 53 ++++++++++++++++++++++++- cmd/middleman-github-app/e2e_test.go | 35 +++++++++++++++++ internal/github/client.go | 16 +++++++- internal/github/client_test.go | 56 +++++++++++++++++++++++++++ internal/tokenauth/descriptor.go | 24 ++++++++++++ internal/tokenauth/descriptor_test.go | 39 +++++++++++++++++++ 6 files changed, 221 insertions(+), 2 deletions(-) diff --git a/cmd/middleman-github-app/create.go b/cmd/middleman-github-app/create.go index 9d338d697..702b683d2 100644 --- a/cmd/middleman-github-app/create.go +++ b/cmd/middleman-github-app/create.go @@ -468,7 +468,26 @@ func (env *appEnv) runInstallFlow( return false, nil }) if err != nil { - return err + // Editing an installation's repository access or re-running + // after a coverage failure reconfigures the existing + // installation instead of minting a new ID, so no new + // installation ever appears and the poll times out -- the + // dead-end the coverage error's own "re-run install" guidance + // would otherwise hit. When exactly one installation exists + // for this app the intent is unambiguous, so adopt it. + // ctx.Err() separates the poll timeout from a user interrupt, + // which must not be swallowed. + adopted, adoptErr := env.adoptSoleInstallation(ctx, app, &picked) + if adoptErr != nil { + return adoptErr + } + if !adopted { + return err + } + fmt.Fprintf(env.stdout, + "No new installation appeared; recording the existing installation %d on %s.\n", + picked.ID, picked.Account.Login, + ) } } @@ -499,6 +518,38 @@ func (env *appEnv) runInstallFlow( return nil } +// adoptSoleInstallation recovers the install flow when the poll timed +// out without a new installation appearing. Re-running "install" after a +// coverage failure or a restored config reconfigures the existing +// installation rather than minting a new ID, so the wait never +// completes; when the app has exactly one GitHub-side installation the +// target is unambiguous, so adopt it into picked and report true. +// Multiple installations stay ambiguous and keep the timeout. A user +// interrupt (ctx canceled) is never treated as an adoptable timeout, and +// a non-zero picked means the poll already found a new installation. +func (env *appEnv) adoptSoleInstallation( + ctx context.Context, + app config.GitHubAppConfig, + picked *githubapp.Installation, +) (bool, error) { + if picked.ID != 0 || ctx.Err() != nil { + return false, nil + } + jwt, err := appJWT(app, env.now()) + if err != nil { + return false, err + } + installs, err := env.apiClient(app.Host).ListInstallations(ctx, jwt) + if err != nil { + return false, err + } + if len(installs) != 1 { + return false, nil + } + *picked = installs[0] + return true, nil +} + func (env *appEnv) refreshAppMetadata( ctx context.Context, client *githubapp.Client, diff --git a/cmd/middleman-github-app/e2e_test.go b/cmd/middleman-github-app/e2e_test.go index be6b22de7..16470a66d 100644 --- a/cmd/middleman-github-app/e2e_test.go +++ b/cmd/middleman-github-app/e2e_test.go @@ -863,6 +863,41 @@ func TestInstallSkipsPreexistingUncoveringInstallation(t *testing.T) { require.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) } +func TestInstallAdoptsSoleExistingInstallationWhenNoNewAppears(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-readopt") + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) + + // The installation exists on GitHub but was never recorded -- the + // shape left behind by a coverage failure or a restored config. The + // coverage error tells the user to edit repo access and re-run + // "install"; that reconfigures the existing installation without + // minting a new id, so the browser poll never sees a new one. + app, ok := fake.AppBySlug("middleman-readopt") + require.True(ok) + _, err := fake.Install(app.ID, "kenn-io") + require.NoError(err) + + env, out := newTestEnv(t, fake, configPath) + // The browser opens but no new installation is created. + env.openBrowser = func(string) error { return nil } + require.NoError(runCLI([]string{"install", "--timeout", "200ms"}, env)) + require.Contains(out.String(), "No new installation appeared") + + cfg, err := config.Load(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + assert := assert.New(t) + assert.NotZero(cfg.GitHubApps[0].InstallationID) + assert.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) +} + func TestInstallRecordsOrgInstallationWithoutCoveringOtherOwners(t *testing.T) { t.Parallel() require := require.New(t) diff --git a/internal/github/client.go b/internal/github/client.go index 488ea1274..98facac8d 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -468,6 +468,20 @@ func (c *liveClient) splitAuthActive() bool { return c.source.Descriptor().HasActiveGitHubApp() } +// splitAuthActiveForOwner is splitAuthActive scoped to a single owner: +// it reports whether reads for owner actually resolve through an app +// installation token. A host descriptor may carry app candidates for +// several owners, but a candidate scoped to one installation account is +// skipped for any other owner during token resolution. Gate +// installation-token-only endpoints on this so a PAT-backed owner on a +// host that also hosts another owner's app is not routed there. +func (c *liveClient) splitAuthActiveForOwner(owner string) bool { + if c.source == nil { + return false + } + return c.source.Descriptor().HasActiveGitHubAppForOwner(owner) +} + func (c *liveClient) bypassNotificationReadRateReserve() bool { return c.splitAuthActive() } @@ -1372,7 +1386,7 @@ func (c *liveClient) ListOpenIssues( func (c *liveClient) ListRepositoriesByOwner( ctx context.Context, owner string, ) ([]*gh.Repository, error) { - if c.splitAuthActive() { + if c.splitAuthActiveForOwner(owner) { ctx = tokenauth.WithGitHubOwner(ctx, owner) repos, err := collectPages( ctx, diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 95cc2f722..cc79f8bc3 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -471,6 +471,62 @@ func TestListRepositoriesByOwnerUsesInstallationReposWithAppToken(t *testing.T) }, paths) } +// A host can carry an app installation for one owner while other owners +// on the same host stay on the PAT/gh chain. Listing repos for such an +// owner must not route to the installation-token-only endpoint: an app +// candidate scoped to a different account is skipped during token +// resolution, so /installation/repositories would 403 on the PAT. +func TestListRepositoriesByOwnerSkipsInstallationReposForUnmatchedOwner(t *testing.T) { + require := require.New(t) + var installationEndpointUsed bool + var orgEndpointUsed bool + + mux := http.NewServeMux() + mux.HandleFunc("/api/v3/installation/repositories", func(w http.ResponseWriter, _ *http.Request) { + installationEndpointUsed = true + http.Error(w, "installation token cannot serve another owner", http.StatusForbidden) + }) + mux.HandleFunc("/api/v3/user", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"login":"mariusvniekerk"}`)) + }) + mux.HandleFunc("/api/v3/orgs/acme/repos", func(w http.ResponseWriter, _ *http.Request) { + orgEndpointUsed = true + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode([]map[string]any{{ + "name": "infra", + "owner": map[string]string{"login": "acme"}, + }}) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + ghClient, err := gh.NewClient(srv.Client()).WithEnterpriseURLs( + srv.URL+"/api/v3/", srv.URL+"/api/uploads/", + ) + require.NoError(err) + c := &liveClient{ + gh: ghClient, + source: tokenauth.NewManagedSource(tokenauth.Descriptor{ + Candidates: []tokenauth.Candidate{{ + Kind: tokenauth.SourceKindGitHubApp, + Host: "github.com", + AppID: 123, + InstallationID: 456, + InstallationAccount: "kenn-io", + }}, + }, tokenauth.Options{}), + } + + repos, err := c.ListRepositoriesByOwner(t.Context(), "acme") + require.NoError(err) + require.Len(repos, 1) + require.Equal("infra", repos[0].GetName()) + require.True(orgEndpointUsed) + require.False(installationEndpointUsed, + "a PAT-backed owner must not be routed to the installation-token-only endpoint") +} + func TestListRepositoriesByOwnerUsesPublicUserEndpointForOtherUsers(t *testing.T) { require := require.New(t) var paths []string diff --git a/internal/tokenauth/descriptor.go b/internal/tokenauth/descriptor.go index 2d1a6073f..1baa89a00 100644 --- a/internal/tokenauth/descriptor.go +++ b/internal/tokenauth/descriptor.go @@ -105,6 +105,30 @@ func (d Descriptor) HasActiveGitHubApp() bool { return false } +// HasActiveGitHubAppForOwner reports whether reads for owner resolve +// through a GitHub App installation token. It mirrors the owner scoping +// in app-token resolution (see githubAppToken): an installed app +// candidate applies when it is unscoped (no installation account, so it +// serves every owner) or its installation account matches owner. Gate +// installation-token-only endpoints on this rather than +// HasActiveGitHubApp so a PAT-backed owner sharing a host with another +// owner's app is not routed to an endpoint its credential cannot use. +func (d Descriptor) HasActiveGitHubAppForOwner(owner string) bool { + owner = strings.TrimSpace(owner) + for _, candidate := range d.Candidates { + if candidate.Kind != SourceKindGitHubApp || candidate.InstallationID == 0 { + continue + } + if candidate.InstallationAccount == "" { + return true + } + if owner != "" && strings.EqualFold(owner, candidate.InstallationAccount) { + return true + } + } + return false +} + // CanonicalSourceString returns a stable identifier for the descriptor's // resolution chain with duplicate and field-redundant candidates removed. // Two descriptors that resolve from the same ordered sources produce the same diff --git a/internal/tokenauth/descriptor_test.go b/internal/tokenauth/descriptor_test.go index 09078c20b..346284460 100644 --- a/internal/tokenauth/descriptor_test.go +++ b/internal/tokenauth/descriptor_test.go @@ -77,3 +77,42 @@ func TestCanonicalSourceStringEqualityMirrorsResolution(t *testing.T) { assert.NotEqual(t, single.CanonicalSourceString(), different.CanonicalSourceString()) assert.Equal(t, "env:SHARED -> env:SHARED", repeated.SafeString()) } + +// TestHasActiveGitHubAppForOwner pins the gate to the same owner scoping +// that githubAppToken applies when resolving a token: an account-scoped +// app serves only its installation account, an unscoped app serves every +// owner, and an app with no installation id mints nothing. +func TestHasActiveGitHubAppForOwner(t *testing.T) { + scoped := Candidate{ + Kind: SourceKindGitHubApp, Host: "github.com", + AppID: 1, InstallationID: 10, InstallationAccount: "kenn-io", + } + unscoped := Candidate{ + Kind: SourceKindGitHubApp, Host: "github.com", + AppID: 2, InstallationID: 20, + } + uninstalled := Candidate{ + Kind: SourceKindGitHubApp, Host: "github.com", AppID: 3, + } + pat := Candidate{Kind: SourceKindEnv, EnvName: "TOKEN"} + + for _, tc := range []struct { + name string + candidates []Candidate + owner string + want bool + }{ + {name: "scoped app matches its account", candidates: []Candidate{scoped, pat}, owner: "kenn-io", want: true}, + {name: "scoped app match is case-insensitive", candidates: []Candidate{scoped}, owner: "Kenn-IO", want: true}, + {name: "scoped app skipped for other owner", candidates: []Candidate{scoped, pat}, owner: "acme", want: false}, + {name: "scoped app skipped without an owner", candidates: []Candidate{scoped}, owner: "", want: false}, + {name: "unscoped app serves any owner", candidates: []Candidate{unscoped}, owner: "acme", want: true}, + {name: "uninstalled app mints nothing", candidates: []Candidate{uninstalled}, owner: "kenn-io", want: false}, + {name: "pat-only chain has no app", candidates: []Candidate{pat}, owner: "kenn-io", want: false}, + } { + t.Run(tc.name, func(t *testing.T) { + desc := Descriptor{Candidates: tc.candidates} + assert.Equal(t, tc.want, desc.HasActiveGitHubAppForOwner(tc.owner)) + }) + } +} From 16d4a48925f67350785208195ce706c3e4a611c9 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 19:57:11 -0400 Subject: [PATCH 6/8] docs: record owner-scoped endpoint gating for GitHub App reads The GitHub App owner-scoping invariant already said reads must resolve app tokens with the repository owner in context, but a host-wide gate on an installation-token-only endpoint still slipped a PAT-backed owner onto an endpoint its credential cannot use. Make the doc say owner scoping governs endpoint selection, not just token resolution, so a future installation-token endpoint is not gated on host-wide app presence and re-introduce the same 403. Also note that re-running install after a coverage failure reconfigures the existing installation without a new id, so the install flow must adopt an already-present installation rather than waiting only for a new one -- the recovery the coverage error's "re-run install" guidance depends on. Generated with Claude Code (Opus 4.8) Co-Authored-By: Claude Opus 4.8 --- context/github-sync-invariants.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index 0356e7b02..8c8253264 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -120,14 +120,23 @@ GitHub App installation tokens are account-scoped, not host-scoped. An app installation for one owner must not authenticate reads for another owner just because both repos share the same host. Repo-scoped GitHub reads must resolve app tokens with the repository owner in context, and ownerless contexts such as -clone auth must fall through to PAT/`gh` credentials. +clone auth must fall through to PAT/`gh` credentials. This owner scoping governs +endpoint selection, not just token resolution: choose an installation-token-only +read endpoint (such as installation-repositories listing) only when the requested +owner actually resolves to an app installation. Gating it on whether the host has +any active app sends a PAT-backed owner that shares the host with another owner's +app to an endpoint its credential cannot use, which fails even though the token +chain "correctly" falls back to the PAT. Config may carry multiple `[[github_apps]]` rows for one host, but those rows represent distinct app credentials. Management commands must target one row by app owner/installation account or app id, and duplicate installation accounts on the same host are invalid. Selected-repository coverage applies only to repos owned by that row's `installation_account`, and the install CLI must not warn that an installation on one account "cannot reach" repos owned by another -account. +account. Re-running `install` after a coverage failure (or against a restored +config) reconfigures the existing installation instead of minting a new +installation id, so the install flow must be able to adopt an already-present +installation rather than waiting only for a newly created one. ## Testing Expectations From bc7cbfe12ccdd6584f608b73917c97efa27429b1 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 20:13:38 -0400 Subject: [PATCH 7/8] fix: bound GitHub App install adoption by intent and poll timeout Review of the install-recovery path surfaced two ways it could record the wrong installation. First, recovery ran for any pollUntil error, so a transient list-installations failure could be mistaken for "nothing appeared" and immediately adopt a stale installation, hiding the API error. pollUntil now wraps a sentinel on its deadline and recovery only runs for that clean timeout; probe errors and user interrupts surface unchanged. Second, adoption accepted any sole installation regardless of account. A lone installation on an account that owns none of the configured repos would be recorded while the CLI reported success, leaving the intended owner on the PAT path. Adoption is now bounded by intent: the sole installation's account must be the recorded installation account or own a configured repo that resolves to the app. Multiple installations, an unrelated lone account, a probe error, or an interrupt keep the timeout. The owner-scoped repo-listing fix stays covered at the github client level against a real HTTP server, where the installation-vs-org endpoint decision lives; the server e2e harness injects a mock GitHub client that bypasses that decision, so a full-stack case there would assert the mock rather than the gating. Validation: go test ./cmd/middleman-github-app ./internal/github ./internal/tokenauth ./internal/githubapp/... ./internal/config -shuffle=on; go vet; golangci-lint run (0 issues) Generated with Claude Code (Opus 4.8) Co-Authored-By: Claude Opus 4.8 --- cmd/middleman-github-app/create.go | 68 ++++++++++++---- cmd/middleman-github-app/e2e_test.go | 99 ++++++++++++++++++++++++ cmd/middleman-github-app/shared.go | 15 +++- context/github-sync-invariants.md | 9 ++- internal/githubapp/githubapptest/fake.go | 21 +++++ 5 files changed, 192 insertions(+), 20 deletions(-) diff --git a/cmd/middleman-github-app/create.go b/cmd/middleman-github-app/create.go index 702b683d2..ff09f8909 100644 --- a/cmd/middleman-github-app/create.go +++ b/cmd/middleman-github-app/create.go @@ -474,10 +474,14 @@ func (env *appEnv) runInstallFlow( // installation ever appears and the poll times out -- the // dead-end the coverage error's own "re-run install" guidance // would otherwise hit. When exactly one installation exists - // for this app the intent is unambiguous, so adopt it. - // ctx.Err() separates the poll timeout from a user interrupt, - // which must not be swallowed. - adopted, adoptErr := env.adoptSoleInstallation(ctx, app, &picked) + // for this app the intent is unambiguous, so adopt it. Only a + // clean deadline qualifies: a probe error (transient API + // failure) or a user interrupt must surface, not silently + // adopt a stale installation. + if !errors.Is(err, errPollDeadline) { + return err + } + adopted, adoptErr := env.adoptSoleInstallation(ctx, cfg, app, &picked) if adoptErr != nil { return adoptErr } @@ -518,23 +522,28 @@ func (env *appEnv) runInstallFlow( return nil } -// adoptSoleInstallation recovers the install flow when the poll timed -// out without a new installation appearing. Re-running "install" after a +// adoptSoleInstallation recovers the install flow after the poll reached +// its deadline without a new installation appearing. The caller gates +// this on errPollDeadline, so probe errors and user interrupts never +// reach it and are surfaced instead. Re-running "install" after a // coverage failure or a restored config reconfigures the existing -// installation rather than minting a new ID, so the wait never -// completes; when the app has exactly one GitHub-side installation the -// target is unambiguous, so adopt it into picked and report true. -// Multiple installations stay ambiguous and keep the timeout. A user -// interrupt (ctx canceled) is never treated as an adoptable timeout, and -// a non-zero picked means the poll already found a new installation. +// installation rather than minting a new id, so the wait never +// completes; when the app has exactly one GitHub-side installation that +// belongs to an account this config actually intends the app to serve, +// the target is unambiguous, so adopt it into picked and report true. +// +// Adoption is bounded by intent: the sole installation's account must be +// the recorded installation account or own a configured repo that +// resolves to the app. A lone installation on an unrelated account is +// not what the user is waiting for, so it keeps the timeout rather than +// recording the wrong account while reporting success. Multiple +// installations stay ambiguous and also keep the timeout. func (env *appEnv) adoptSoleInstallation( ctx context.Context, + cfg *config.Config, app config.GitHubAppConfig, picked *githubapp.Installation, ) (bool, error) { - if picked.ID != 0 || ctx.Err() != nil { - return false, nil - } jwt, err := appJWT(app, env.now()) if err != nil { return false, err @@ -546,10 +555,37 @@ func (env *appEnv) adoptSoleInstallation( if len(installs) != 1 { return false, nil } - *picked = installs[0] + inst := installs[0] + account := inst.Account.Login + if !strings.EqualFold(account, app.InstallationAccount) && + !accountServesConfiguredRepos(cfg, app.Host, account) { + return false, nil + } + *picked = inst return true, nil } +// accountServesConfiguredRepos reports whether account owns at least one +// configured github repo on host that resolves to the app token (no +// per-repo credential override). It marks an account the app is actually +// meant to serve, so install recovery adopts a sole existing +// installation only for an intended account instead of any account that +// happens to be the app's only installation. +func accountServesConfiguredRepos(cfg *config.Config, host, account string) bool { + for _, r := range cfg.Repos { + if r.PlatformOrDefault() != "github" || r.PlatformHostOrDefault() != host { + continue + } + if r.TokenEnv != "" || r.TokenFile != "" { + continue + } + if strings.EqualFold(r.Owner, account) { + return true + } + } + return false +} + func (env *appEnv) refreshAppMetadata( ctx context.Context, client *githubapp.Client, diff --git a/cmd/middleman-github-app/e2e_test.go b/cmd/middleman-github-app/e2e_test.go index 16470a66d..ad07a08d9 100644 --- a/cmd/middleman-github-app/e2e_test.go +++ b/cmd/middleman-github-app/e2e_test.go @@ -898,6 +898,105 @@ func TestInstallAdoptsSoleExistingInstallationWhenNoNewAppears(t *testing.T) { assert.Equal("kenn-io", cfg.GitHubApps[0].InstallationAccount) } +func TestInstallSurfacesProbeErrorWithoutAdoptingStaleInstallation(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-probeerr") + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) + + // A sole unrecorded installation exists, so adoption recovery would + // grab it. But a transient list failure during the poll is a probe + // error, not a clean "nothing appeared in time"; it must surface + // rather than silently recording the stale installation. + app, ok := fake.AppBySlug("middleman-probeerr") + require.True(ok) + _, err := fake.Install(app.ID, "kenn-io") + require.NoError(err) + fake.FailNextListInstallations(1) + + env, _ = newTestEnv(t, fake, configPath) + err = runCLI([]string{"install", "--no-browser", "--timeout", "10s"}, env) + require.Error(err) + + cfg, err := config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + require.Zero(cfg.GitHubApps[0].InstallationID, + "a transient probe error must not adopt a stale installation") +} + +func TestInstallKeepsTimeoutWhenSoleInstallationIsUnrelated(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) // configures kenn-io/middleman + createTestApp(t, fake, configPath, "middleman-unrelated") + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) + + // The app's only installation is on an account that owns none of the + // configured repos. A browser timeout must not adopt it as if it were + // the intended install -- the user still needs to install on the + // owning account -- so the timeout surfaces unchanged. + app, ok := fake.AppBySlug("middleman-unrelated") + require.True(ok) + _, err := fake.Install(app.ID, "unrelated-org") + require.NoError(err) + + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = func(string) error { return nil } // no new installation + err = runCLI([]string{"install", "--timeout", "200ms"}, env) + require.Error(err) + require.ErrorContains(err, "timed out") + + cfg, err := config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + require.Zero(cfg.GitHubApps[0].InstallationID, + "an unrelated sole installation must not be adopted on timeout") +} + +func TestInstallKeepsTimeoutWhenMultipleInstallationsExist(t *testing.T) { + t.Parallel() + require := require.New(t) + fake := githubapptest.NewFake() + t.Cleanup(fake.Close) + configPath := writeTestConfig(t) + createTestApp(t, fake, configPath, "middleman-multi") + + env, _ := newTestEnv(t, fake, configPath) + require.NoError(runCLI([]string{"uninstall", "--yes"}, env)) + + // Two installations exist, one of them on a configured-repo owner. + // Which one to record is ambiguous, so a browser timeout keeps + // waiting instead of guessing. + app, ok := fake.AppBySlug("middleman-multi") + require.True(ok) + _, err := fake.Install(app.ID, "kenn-io") + require.NoError(err) + _, err = fake.Install(app.ID, "acme") + require.NoError(err) + + env, _ = newTestEnv(t, fake, configPath) + env.openBrowser = func(string) error { return nil } // no new installation + err = runCLI([]string{"install", "--timeout", "200ms"}, env) + require.Error(err) + require.ErrorContains(err, "timed out") + + cfg, err := config.LoadForGitHubAppRepair(configPath) + require.NoError(err) + require.Len(cfg.GitHubApps, 1) + require.Zero(cfg.GitHubApps[0].InstallationID, + "ambiguous multiple installations must not be adopted on timeout") +} + func TestInstallRecordsOrgInstallationWithoutCoveringOtherOwners(t *testing.T) { t.Parallel() require := require.New(t) diff --git a/cmd/middleman-github-app/shared.go b/cmd/middleman-github-app/shared.go index be1373120..9251ac7b9 100644 --- a/cmd/middleman-github-app/shared.go +++ b/cmd/middleman-github-app/shared.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "strings" "time" @@ -207,8 +208,18 @@ func missingSelectedRepos( return missing } +// errPollDeadline marks a pollUntil return that ended on its own timeout +// deadline, as opposed to a probe error or context cancellation. Callers +// that recover from a clean "nothing appeared in time" (for example +// adopting an existing installation when no new one shows up) must match +// this so a transient probe failure surfaces instead of being silently +// treated as a timeout. +var errPollDeadline = errors.New("poll deadline reached") + // pollUntil runs probe at the env's poll interval until it reports -// done, the context ends, or timeout elapses. +// done, the context ends, or timeout elapses. A timeout wraps +// errPollDeadline; probe errors and context cancellation are returned +// as-is so callers can tell them apart. func (env *appEnv) pollUntil( ctx context.Context, timeout time.Duration, @@ -230,7 +241,7 @@ func (env *appEnv) pollUntil( case <-ctx.Done(): return ctx.Err() case <-deadline.C: - return fmt.Errorf("timed out after %s", timeout) + return fmt.Errorf("timed out after %s: %w", timeout, errPollDeadline) case <-ticker.C: } } diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index 8c8253264..dcacf225e 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -135,8 +135,13 @@ owned by that row's `installation_account`, and the install CLI must not warn that an installation on one account "cannot reach" repos owned by another account. Re-running `install` after a coverage failure (or against a restored config) reconfigures the existing installation instead of minting a new -installation id, so the install flow must be able to adopt an already-present -installation rather than waiting only for a newly created one. +installation id, so on a clean install-poll timeout the flow adopts an +already-present installation rather than only ever waiting for a newly created +one. Adoption is bounded by intent: adopt only the app's sole installation when +its account is the recorded installation account or owns a configured repo that +resolves to the app. Multiple installations, a lone installation on an unrelated +account, a transient probe error, or a user interrupt keep the timeout instead +of recording the wrong account. ## Testing Expectations diff --git a/internal/githubapp/githubapptest/fake.go b/internal/githubapp/githubapptest/fake.go index 1ff84f1c8..58c6a54ff 100644 --- a/internal/githubapp/githubapptest/fake.go +++ b/internal/githubapp/githubapptest/fake.go @@ -72,6 +72,10 @@ type Fake struct { nextID int64 rateLimit int manifests []string // every manifest JSON received, in order + // failListInstallations, when > 0, makes that many upcoming + // /app/installations requests respond 500 before serving normally, + // so tests can exercise transient install-poll probe failures. + failListInstallations int } func NewFake() *Fake { @@ -112,6 +116,15 @@ func (f *Fake) Manifests() []string { return append([]string(nil), f.manifests...) } +// FailNextListInstallations makes the next n list-installations requests +// respond 500 before subsequent ones serve normally, letting tests model +// a transient probe failure during the install poll. +func (f *Fake) FailNextListInstallations(n int) { + f.mu.Lock() + defer f.mu.Unlock() + f.failListInstallations = n +} + // Install simulates the user completing the browser install flow for // the app, returning the new installation id. func (f *Fake) Install(appID int64, account string) (int64, error) { @@ -344,6 +357,14 @@ func (f *Fake) handleGetApp(w http.ResponseWriter, r *http.Request) { } func (f *Fake) handleListInstallations(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + if f.failListInstallations > 0 { + f.failListInstallations-- + f.mu.Unlock() + writeJSONError(w, http.StatusInternalServerError, "transient listing failure") + return + } + f.mu.Unlock() app, ok := f.authenticateAppJWT(r) if !ok { writeJSONError(w, http.StatusUnauthorized, "bad app JWT") From 4b068ef27b0bcd01246cec06bd075a0638f899d7 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 25 Jun 2026 20:45:49 -0400 Subject: [PATCH 8/8] fix: keep install poll timeout message clean and align recovery docs Marking a clean poll deadline by wrapping a sentinel in the returned error leaked the marker text into the user-facing "timed out after ..." message for both the install and delete CLIs. pollUntil now returns a pollTimeoutError whose Error() keeps the plain message while Is() still matches errPollDeadline, so recovery can branch on a clean timeout without changing what the user sees. The install-recovery invariant doc said probe errors and interrupts "keep the timeout," which contradicts the behavior: those are not clean deadlines, so they surface the original error or cancellation and never adopt. Only an unrelated or ambiguous installation after a clean deadline leaves a timeout. Reword the invariant to match, and strengthen tests: the probe-error case now asserts the listing error surfaces (not a timeout), and a pollUntil unit test pins that only a deadline matches errPollDeadline while probe errors and cancellation pass through. Validation: go test ./cmd/middleman-github-app ./internal/github ./internal/tokenauth ./internal/githubapp/... -shuffle=on; go vet; golangci-lint run (0 issues) Generated with Claude Code (Opus 4.8) Co-Authored-By: Claude Opus 4.8 --- cmd/middleman-github-app/e2e_test.go | 4 ++ cmd/middleman-github-app/shared.go | 34 +++++++++++++---- cmd/middleman-github-app/shared_test.go | 50 +++++++++++++++++++++++++ context/github-sync-invariants.md | 12 +++--- 4 files changed, 87 insertions(+), 13 deletions(-) create mode 100644 cmd/middleman-github-app/shared_test.go diff --git a/cmd/middleman-github-app/e2e_test.go b/cmd/middleman-github-app/e2e_test.go index ad07a08d9..0745a8a41 100644 --- a/cmd/middleman-github-app/e2e_test.go +++ b/cmd/middleman-github-app/e2e_test.go @@ -922,6 +922,10 @@ func TestInstallSurfacesProbeErrorWithoutAdoptingStaleInstallation(t *testing.T) env, _ = newTestEnv(t, fake, configPath) err = runCLI([]string{"install", "--no-browser", "--timeout", "10s"}, env) require.Error(err) + // The probe failure must surface unchanged, not be rewritten as a + // timeout that triggers stale adoption. + require.ErrorContains(err, "listing app installations") + require.NotErrorIs(err, errPollDeadline) cfg, err := config.LoadForGitHubAppRepair(configPath) require.NoError(err) diff --git a/cmd/middleman-github-app/shared.go b/cmd/middleman-github-app/shared.go index 9251ac7b9..3ffbdac88 100644 --- a/cmd/middleman-github-app/shared.go +++ b/cmd/middleman-github-app/shared.go @@ -208,18 +208,36 @@ func missingSelectedRepos( return missing } -// errPollDeadline marks a pollUntil return that ended on its own timeout -// deadline, as opposed to a probe error or context cancellation. Callers +// errPollDeadline is the sentinel a pollTimeoutError matches via Is, so +// callers can recognize a clean deadline with errors.Is(err, +// errPollDeadline) without the marker text leaking into the user-facing +// message. It marks a pollUntil return that ended on its own timeout +// deadline, as opposed to a probe error or context cancellation: callers // that recover from a clean "nothing appeared in time" (for example // adopting an existing installation when no new one shows up) must match -// this so a transient probe failure surfaces instead of being silently -// treated as a timeout. +// this so a transient probe failure or interrupt surfaces instead of +// being treated as a timeout. var errPollDeadline = errors.New("poll deadline reached") +// pollTimeoutError is pollUntil's deadline result. Its message stays the +// plain "timed out after " the CLIs have always shown, while Is +// reports a match against errPollDeadline so recovery paths can branch on +// a clean timeout without changing what the user sees. +type pollTimeoutError struct{ timeout time.Duration } + +func (e pollTimeoutError) Error() string { + return fmt.Sprintf("timed out after %s", e.timeout) +} + +func (e pollTimeoutError) Is(target error) bool { + return target == errPollDeadline +} + // pollUntil runs probe at the env's poll interval until it reports -// done, the context ends, or timeout elapses. A timeout wraps -// errPollDeadline; probe errors and context cancellation are returned -// as-is so callers can tell them apart. +// done, the context ends, or timeout elapses. A timeout returns a +// pollTimeoutError (errors.Is(err, errPollDeadline) is true); probe +// errors and context cancellation are returned as-is so callers can tell +// them apart. func (env *appEnv) pollUntil( ctx context.Context, timeout time.Duration, @@ -241,7 +259,7 @@ func (env *appEnv) pollUntil( case <-ctx.Done(): return ctx.Err() case <-deadline.C: - return fmt.Errorf("timed out after %s: %w", timeout, errPollDeadline) + return pollTimeoutError{timeout: timeout} case <-ticker.C: } } diff --git a/cmd/middleman-github-app/shared_test.go b/cmd/middleman-github-app/shared_test.go new file mode 100644 index 000000000..ab6529378 --- /dev/null +++ b/cmd/middleman-github-app/shared_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestPollUntilDistinguishesDeadlineFromErrorAndCancel pins the contract +// install recovery relies on: only a clean deadline matches +// errPollDeadline (with the plain user-facing message), while probe +// errors and cancellation surface unchanged so adoption never runs for +// them. +func TestPollUntilDistinguishesDeadlineFromErrorAndCancel(t *testing.T) { + t.Parallel() + env := &appEnv{pollInterval: time.Millisecond} + + t.Run("deadline matches errPollDeadline with a clean message", func(t *testing.T) { + t.Parallel() + err := env.pollUntil(context.Background(), 20*time.Millisecond, + func(context.Context) (bool, error) { return false, nil }) + require.ErrorIs(t, err, errPollDeadline) + assert.Contains(t, err.Error(), "timed out after") + assert.NotContains(t, err.Error(), "poll deadline reached") + }) + + t.Run("probe error surfaces and is not a deadline", func(t *testing.T) { + t.Parallel() + boom := errors.New("probe boom") + err := env.pollUntil(context.Background(), time.Second, + func(context.Context) (bool, error) { return false, boom }) + require.ErrorIs(t, err, boom) + assert.NotErrorIs(t, err, errPollDeadline) + }) + + t.Run("cancellation surfaces and is not a deadline", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + err := env.pollUntil(ctx, time.Second, func(context.Context) (bool, error) { + cancel() + return false, nil + }) + require.ErrorIs(t, err, context.Canceled) + assert.NotErrorIs(t, err, errPollDeadline) + }) +} diff --git a/context/github-sync-invariants.md b/context/github-sync-invariants.md index dcacf225e..8347884e4 100644 --- a/context/github-sync-invariants.md +++ b/context/github-sync-invariants.md @@ -137,11 +137,13 @@ account. Re-running `install` after a coverage failure (or against a restored config) reconfigures the existing installation instead of minting a new installation id, so on a clean install-poll timeout the flow adopts an already-present installation rather than only ever waiting for a newly created -one. Adoption is bounded by intent: adopt only the app's sole installation when -its account is the recorded installation account or owns a configured repo that -resolves to the app. Multiple installations, a lone installation on an unrelated -account, a transient probe error, or a user interrupt keep the timeout instead -of recording the wrong account. +one. Adoption runs only after a clean poll deadline and is bounded by intent: +adopt only the app's sole installation when its account is the recorded +installation account or owns a configured repo that resolves to the app. +Multiple installations or a lone installation on an unrelated account leave the +deadline as a timeout instead of recording the wrong account. A transient probe +error or a user interrupt is not a clean deadline: it surfaces the original +error or cancellation unchanged and never adopts. ## Testing Expectations