Skip to content

fix: refresh GitHub App installation token on legacy client#3489

Closed
yashrajdighe wants to merge 3 commits into
integrations:mainfrom
yashrajdighe:fix/github-app-token-refresh
Closed

fix: refresh GitHub App installation token on legacy client#3489
yashrajdighe wants to merge 3 commits into
integrations:mainfrom
yashrajdighe:fix/github-app-token-refresh

Conversation

@yashrajdighe

@yashrajdighe yashrajdighe commented Jun 15, 2026

Copy link
Copy Markdown

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. Long terraform apply runs would start failing with 401 Bad credentials once that hour was up.

The new client in internal/ghclient already handles refresh via go-githubauth, but App auth still went through the legacy path unless you set legacy_client = false.

After the change?

App auth now always uses the new client, even when legacy_client is left at the default. No changes to the legacy client itself.

  • Dropped the GenerateOAuthTokenFromApp call from provider setup — tokens are minted on demand and refreshed automatically.
  • Passed WithEnterpriseURL through in internal/ghclient so GHES/custom base_url token 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), not app_auth.installation_id. That field is still required in the schema but isn't used for provider auth on this path. owner is already required for App auth, so this should be fine for the usual case.

Also, read_delay_ms, write_delay_ms, parallel_requests, and retryable_errors don't apply to REST on the new client — same as documented today.

Pull request checklist

  • Schema migrations have been created if needed — N/A
  • Tests for the changes have been added
  • Docs reviewed — no schema changes, nothing to regenerate

Local test validation

  • make fmt, make lint, make test, make build all pass
  • Test_appSource_installationTokenRefresh — checks token reuse and refresh after expiry
  • Test_configureProviderMeta_appAuthUsesNewClient — confirms app_auth hits the new client path even with legacy_client = true

Does this introduce a breaking change?

  • Yes
  • No

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.

@github-actions

Copy link
Copy Markdown

👋 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.

@github-actions github-actions Bot added the Type: Bug Something isn't working as documented label Jun 15, 2026
@yashrajdighe

Copy link
Copy Markdown
Author

@nickfloyd @deiga Would be great if this PR can be reviewed!

@deiga

deiga commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

Please don't ping us. We are fully aware of the open PRs.
What we don't have is unlimited time, which means that we need to choose where to put our capacity.

And in this case, you're pinging a person who is no longer working on the project.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
  • GenerateOAuthTokenFromApp is still used by data_source_github_app_token.go and acc_test.go, so it is not orphaned.
  • appInstallationTokenSource follows the existing internal/ghclient pattern, and WithEnterpriseURL exists in go-githubauth v1.6.0.
  • validateAppAuth guarantees AppInstallationID is non-nil whenever AppID is 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-githubauth installation token source (REST + GraphQL share it).
  • Change AuthenticatedHTTPClient() to return (*http.Client, error) and update the caller in provider.go.
  • Add TestAuthenticatedHTTPClientAppAuthRefresh asserting 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.

@yashrajdighe

Copy link
Copy Markdown
Author

Please don't ping us. We are fully aware of the open PRs. What we don't have is unlimited time, which means that we need to choose where to put our capacity.

And in this case, you're pinging a person who is no longer working on the project.

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.

@stevehipwell

Copy link
Copy Markdown
Collaborator

@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.

yashrajdighe and others added 2 commits June 24, 2026 23:05
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>
@yashrajdighe

Copy link
Copy Markdown
Author

@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.

@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.
Please review again.

@stevehipwell

Copy link
Copy Markdown
Collaborator

@yashrajdighe I'm not sure I follow what the purpose of this PR is now?

@yashrajdighe yashrajdighe deleted the fix/github-app-token-refresh branch June 24, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

app_auth credentials expire after an hour

4 participants