fix: refresh GitHub App installation token on legacy client#3489
fix: refresh GitHub App installation token on legacy client#3489yashrajdighe wants to merge 3 commits into
Conversation
|
👋 Hi, and thank you for this contribution! This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can. You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions. 🤖 This is an automated message. |
|
@nickfloyd @deiga Would be great if this PR can be reviewed! |
|
Please don't ping us. We are fully aware of the open PRs. And in this case, you're pinging a person who is no longer working on the project. |
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used. No blocking (HIGH/MEDIUM) findings were identified.
This PR fixes #977: when using app_auth with the legacy client, the GitHub App installation token was minted once at provider startup via GenerateOAuthTokenFromApp and stored in a static, non-refreshing oauth2.StaticTokenSource. Since installation tokens expire after one hour, long-running plans/applies failed with 401/403 once the hour elapsed. The fix routes the legacy client through jferrl/go-githubauth (already used by the non-legacy client), which transparently refreshes the installation token before expiry. Because the REST (v3) and GraphQL (v4) clients share the same authenticated *http.Client, both are covered.
I verified that:
AuthenticatedHTTPClient's new error-returning signature is updated at its only non-test caller (provider.go:518).GenerateOAuthTokenFromAppis still used bydata_source_github_app_token.goandacc_test.go, so it is not orphaned.appInstallationTokenSourcefollows the existinginternal/ghclientpattern, andWithEnterpriseURLexists in go-githubauth v1.6.0.validateAppAuthguaranteesAppInstallationIDis non-nil wheneverAppIDis set, so the dereferences in the new code are safe in the production flow.- The GH CLI fallback condition (
config.AppID == nil && config.Token == "") is behaviorally equivalent to the prior logic.
Changes:
- Replace the static installation-token logic in the legacy client with a refreshing
go-githubauthinstallation token source (REST + GraphQL share it). - Change
AuthenticatedHTTPClient()to return(*http.Client, error)and update the caller inprovider.go. - Add
TestAuthenticatedHTTPClientAppAuthRefreshasserting a valid token is reused (1 fetch) and an expired token is rotated (2 fetches).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
github/config.go |
Adds appInstallationTokenSource and switches AuthenticatedHTTPClient to a refreshing App token source, returning an error. |
github/provider.go |
Removes eager GenerateOAuthTokenFromApp token minting for the legacy client and handles the new AuthenticatedHTTPClient error return. |
github/config_test.go |
Adds a unit test verifying installation-token reuse and refresh-on-expiry behavior. |
I apologize for the misdirected ping and completely respect your team's capacity constraints. I only flagged this because the PR is currently a blocker for us, and a review when your schedule permits would be greatly appreciated. |
|
@yashrajdighe why can't you use the new client? It'd be much more useful to provide feedback on the new client than to attempt to backport functionality into the old one. FYI I'm not expecting us to make any changes to the legacy client as the plan is to remove it in the next major version. |
Resolve conflict in internal/ghclient/app_test.go and adapt app auth changes to upstream ghclient refactor (string URLs, updated test helpers). Co-authored-by: Cursor <cursoragent@cursor.com>
@stevehipwell It was an inspired fix from #2695 (comment). But it makes sense to go with the new client rather than fixing with the legacy client. |
|
@yashrajdighe I'm not sure I follow what the purpose of this PR is now? |
Resolves #977
Before the change?
GitHub App installation tokens expire after an hour. With
app_auth, we were minting the token once during provider setup and stuffing it into a static OAuth source that never refreshed. Longterraform applyruns would start failing with401 Bad credentialsonce that hour was up.The new client in
internal/ghclientalready handles refresh viago-githubauth, but App auth still went through the legacy path unless you setlegacy_client = false.After the change?
App auth now always uses the new client, even when
legacy_clientis left at the default. No changes to the legacy client itself.GenerateOAuthTokenFromAppcall from provider setup — tokens are minted on demand and refreshed automatically.WithEnterpriseURLthrough ininternal/ghclientso GHES/custombase_urltoken minting hits the right host.PAT/token and anonymous auth on the legacy client are untouched.
One thing to be aware of: the new client looks up the installation from
owner(API call), notapp_auth.installation_id. That field is still required in the schema but isn't used for provider auth on this path.owneris already required for App auth, so this should be fine for the usual case.Also,
read_delay_ms,write_delay_ms,parallel_requests, andretryable_errorsdon't apply to REST on the new client — same as documented today.Pull request checklist
Local test validation
make fmt,make lint,make test,make buildall passTest_appSource_installationTokenRefresh— checks token reuse and refresh after expiryTest_configureProviderMeta_appAuthUsesNewClient— confirmsapp_authhits the new client path even withlegacy_client = trueDoes this introduce a breaking change?
App-auth users get a different HTTP stack under the hood, but there's no schema change and the old behavior was broken past the 1-hour mark anyway.