fix: scope GitHub App tokens by owner#597
Conversation
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 <codex@openai.com>
roborev: Combined Review (
|
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 <codex@openai.com>
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 <codex@openai.com>
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 <codex@openai.com>
roborev: Combined Review (
|
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
roborev: Combined Review (
|
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 <noreply@anthropic.com>
roborev: Combined Review (
|
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 <noreply@anthropic.com>
roborev: Combined Review (
|
1 participant
Uh oh!
There was an error while loading. Please reload this page.