feat(github): add support for refresh tokens and token management #8667

ysinghc · 2025-12-21T22:39:09Z

I have read through the Contributing Documentation.
I have added relevant tests.
I have added relevant documentation.
I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?
This PR adds OAuth2 refresh token support for github user-to-server tokens.

Does this close any open issues?

Closes #8532

Screenshots

Include any relevant screenshots here.

Other Information

Any other information that is important to this PR.

Copilot

Pull request overview

This PR adds OAuth2 refresh token support for GitHub user-to-server tokens, enabling automatic token refresh before expiration and reactive refresh on authentication failures.

Implements a thread-safe TokenProvider that handles OAuth2 token refresh with configurable expiration buffer
Adds RefreshRoundTripper middleware to automatically inject tokens and retry requests on 401 errors
Extends the GithubConnection model with refresh token fields and expiration timestamps
Includes database migration to add new refresh token columns

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
backend/plugins/github/token/token_provider.go	New TokenProvider implementation that handles OAuth2 token refresh with mutex-based concurrency control
backend/plugins/github/token/round_tripper.go	HTTP RoundTripper middleware that injects tokens and retries requests on 401 authentication failures
backend/plugins/github/tasks/api_client.go	Integration of token refresh logic into the API client creation flow when refresh tokens are present
backend/plugins/github/models/migrationscripts/register.go	Registration of the new refresh token fields migration
backend/plugins/github/models/migrationscripts/20241120_add_refresh_token_fields.go	Database migration to add refresh_token, token_expires_at, and refresh_token_expires_at columns
backend/plugins/github/models/connection_test.go	Test cases for token type classification including the new ghu_ prefix
backend/plugins/github/models/connection.go	Added refresh token fields, UpdateToken method, and ghu_ token prefix support
backend/helpers/pluginhelper/api/api_client.go	New GetClient method to expose the underlying http.Client for transport wrapping

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

backend/plugins/github/token/token_provider.go

backend/plugins/github/token/round_tripper.go

Copilot · 2025-12-21T22:43:16Z

backend/plugins/github/token/token_provider.go

+	data := map[string]string{
+		"refresh_token": tp.conn.RefreshToken,
+		"grant_type":    "refresh_token",
+		"client_id":     tp.conn.AppId,
+		"client_secret": tp.conn.SecretKey,
+	}


Sensitive authentication credentials (client_id and client_secret) are logged in plain text in the request data during token refresh. If debug logging is enabled or if there's an error that logs the request, these credentials could be exposed in log files. Consider sanitizing or redacting sensitive fields before logging to prevent credential leakage.

Copilot · 2025-12-21T22:43:16Z

backend/plugins/github/models/connection.go

+	// Update the internal tokens slice used by SetupAuthentication
+	conn.tokens = []string{newToken}
+	conn.tokenIndex = 0


The UpdateToken method resets the tokens slice and tokenIndex, but this could cause issues with concurrent access. If PrepareApiClient has been called previously and split the Token into multiple tokens for rotation, UpdateToken will replace that slice with a single token. This could lead to unexpected behavior when tokens are rotated, especially since there's no synchronization mechanism to protect against concurrent modifications of these fields.

Copilot · 2025-12-21T22:43:16Z

backend/plugins/github/tasks/api_client.go

+	// Inject TokenProvider if refresh token is present
+	if connection.RefreshToken != "" {
+		logger := taskCtx.GetLogger()
+		db := taskCtx.GetDal()
+
+		// Create TokenProvider
+		tp := token.NewTokenProvider(connection, db, apiClient.GetClient(), logger)
+
+		// Wrap the transport
+		baseTransport := apiClient.GetClient().Transport
+		if baseTransport == nil {
+			baseTransport = http.DefaultTransport
+		}
+
+		rt := token.NewRefreshRoundTripper(baseTransport, tp)
+		apiClient.GetClient().Transport = rt
+	}


The RefreshRoundTripper wraps the existing transport, but this could conflict with the existing authentication setup in SetupAuthentication method. The connection's SetupAuthentication is likely already being used elsewhere to set the Authorization header, and now the RoundTripper is also setting it. This could lead to duplicate or conflicting authorization headers depending on the order of middleware execution. Consider documenting this interaction or ensuring the RoundTripper is the exclusive mechanism for setting authorization when refresh tokens are present.

backend/plugins/github/token/token_provider.go

Copilot · 2025-12-21T22:43:17Z

backend/plugins/github/token/round_tripper.go

+type RefreshRoundTripper struct {
+	base          http.RoundTripper
+	tokenProvider *TokenProvider
+}
+
+func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider) *RefreshRoundTripper {
+	return &RefreshRoundTripper{
+		base:          base,
+		tokenProvider: tp,
+	}
+}


The RefreshRoundTripper struct and RoundTrip method lack documentation. These should include comments explaining that this is an HTTP transport middleware that automatically refreshes OAuth tokens and retries requests on authentication failures. The RoundTrip method should document its behavior of cloning requests, handling 401 responses, and potential retry attempts.

Copilot · 2025-12-21T22:43:18Z

backend/plugins/github/token/token_provider.go

+func (tp *TokenProvider) needsRefresh() bool {
+	if tp.conn.RefreshToken == "" {
+		return false
+	}
+
+	buffer := DefaultRefreshBuffer
+	if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" {
+		if val, err := strconv.Atoi(envBuffer); err == nil {
+			buffer = time.Duration(val) * time.Minute
+		}
+	}
+
+	return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt)
+}


The needsRefresh method doesn't check if TokenExpiresAt is a zero value before comparing it with the current time. If TokenExpiresAt is not set (zero value), the comparison 'time.Now().Add(buffer).After(tp.conn.TokenExpiresAt)' will always return true, causing unnecessary refresh attempts. Consider adding a check to return false if TokenExpiresAt.IsZero() to handle the case where expiry time is not set.

backend/plugins/github/token/token_provider.go

Co-authored-by: Copilot <[email protected]>

ysinghc · 2025-12-21T22:50:03Z

I will work on adding the tests and documentation and fixing some logical issues i didnt think of

klesh · 2025-12-22T13:27:54Z

I will work on adding the tests and documentation and fixing some logical issues i didnt think of

Thanks for the update! Please let me know when the PR is ready—I'll be happy to review it.

…e and added tests

…er tests

ysinghc · 2026-01-01T11:42:06Z

@klesh I think the PR is ready for a review, also a very Happy New Year

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

backend/plugins/github/models/connection.go

Copilot · 2026-01-01T11:46:02Z

backend/plugins/github/token/round_tripper.go

+	}
+
+	// Clone request before modifying
+	reqClone := req.Clone(req.Context())


The req.Clone() method does not clone the request body, which means if the original request has a body (e.g., POST/PUT requests), attempting to retry the request after a 401 error will fail because the body has already been read. The body needs to be preserved or made replayable for retry scenarios. Consider using req.GetBody if available, or document that this implementation only supports requests without bodies.

Copilot · 2026-01-01T11:46:02Z

backend/plugins/github/token/round_tripper.go

+		}
+
+		// Retry request with new token
+		reqRetry := req.Clone(req.Context())


The req.Clone() method is called again for the retry request, but request bodies cannot be replayed after being read. If this is a POST/PUT/PATCH request with a body, the retry will fail or send an empty body. This is the same issue as line 58 - the request body needs to be preserved or made replayable.

backend/plugins/github/token/token_provider.go

Copilot · 2026-01-01T11:46:03Z

backend/plugins/github/token/token_provider.go

+	buffer := DefaultRefreshBuffer
+	if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" {
+		if val, err := strconv.Atoi(envBuffer); err == nil {
+			buffer = time.Duration(val) * time.Minute
+		}
+	}


The environment variable check is performed on every call to needsRefresh(), which could be inefficient. Consider caching the buffer value once during initialization or using sync.Once for lazy initialization to avoid repeated environment variable lookups and parsing.

Copilot · 2026-01-01T11:46:03Z

backend/plugins/github/models/connection.go

+func (conn *GithubConn) UpdateToken(newToken, newRefreshToken string, expiry, refreshExpiry time.Time) {
+	conn.Token = newToken
+	conn.RefreshToken = newRefreshToken
+	conn.TokenExpiresAt = expiry
+	conn.RefreshTokenExpiresAt = refreshExpiry
+
+	// Update the internal tokens slice used by SetupAuthentication
+	conn.tokens = []string{newToken}
+	conn.tokenIndex = 0
 }


The new UpdateToken method lacks test coverage. Since this method modifies critical security-related state (tokens and expiry times), it should have unit tests to verify that all fields are updated correctly, including the internal tokens slice and tokenIndex.

Copilot · 2026-01-01T11:46:04Z

backend/plugins/github/token/token_provider.go

+		"client_id":     tp.conn.AppId,
+		"client_secret": tp.conn.SecretKey,
+	}
+	jsonData, _ := json.Marshal(data)


The error from json.Marshal is silently ignored. While marshaling a simple map[string]string is unlikely to fail, errors should still be handled properly for robustness and to follow best practices. Consider returning or logging the error if it occurs.

Suggested change

jsonData, _ := json.Marshal(data)

jsonData, err := json.Marshal(data)

if err != nil {

return errors.Convert(err)

}

backend/plugins/github/token/token_provider.go

backend/plugins/github/token/round_tripper.go

…arity

Co-authored-by: Copilot <[email protected]>

ysinghc · 2026-01-05T16:04:15Z

the failing checks seems to be not exactly related to code so i will fix them asap, although i am aactively looking at the co-pilot's suggestions and trying to fix some of the genuine suggestions that it has pointed out

klesh

Great work! I really appreciate the concurrency unit test—thanks for putting in the effort.

feat(github): add support for refresh tokens and token management

dff9b6a

Copilot AI review requested due to automatic review settings December 21, 2025 22:39

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. component/plugins This issue or PR relates to plugins pr-type/feature-development This PR is to develop a new feature labels Dec 21, 2025

Copilot started reviewing on behalf of ysinghc December 21, 2025 22:39 View session

Copilot AI reviewed Dec 21, 2025

View reviewed changes

ysinghc and others added 3 commits December 22, 2025 04:14

Update backend/plugins/github/token/token_provider.go

8f68983

Co-authored-by: Copilot <[email protected]>

Update backend/plugins/github/token/token_provider.go

9deb463

Co-authored-by: Copilot <[email protected]>

Update backend/plugins/github/token/token_provider.go

cb79908

Co-authored-by: Copilot <[email protected]>

ysinghc and others added 4 commits December 26, 2025 02:33

Merge branch 'apache:main' into feat/github-referesh-tokens

fa99f08

added documentation for roundtripper and fixed the infinite loop issu…

ef6b168

…e and added tests

fixed the illegal plugins/github/models import

b304afa

Remove error conversion on token refresh failure and add token provid…

7552742

…er tests

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 1, 2026

Merge branch 'main' into feat/github-referesh-tokens

e1d9d1e

ysinghc requested a review from Copilot January 1, 2026 11:42

Copilot started reviewing on behalf of ysinghc January 1, 2026 11:42 View session

Copilot AI reviewed Jan 1, 2026

View reviewed changes

ysinghc and others added 3 commits January 1, 2026 17:19

renamed TestIsUserToServerToken to TestTokenTypeClassification for cl…

ee306d7

…arity

Update backend/plugins/github/token/token_provider.go

8a8b678

Co-authored-by: Copilot <[email protected]>

Update backend/plugins/github/token/token_provider.go

eca3279

Co-authored-by: Copilot <[email protected]>

klesh approved these changes Jan 6, 2026

View reviewed changes

klesh merged commit 1a66e79 into apache:main Jan 6, 2026
8 of 10 checks passed

-	jsonData, _ := json.Marshal(data)
+	jsonData, err := json.Marshal(data)
+	if err != nil {
+		return errors.Convert(err)
+	}

feat(github): add support for refresh tokens and token management #8667

feat(github): add support for refresh tokens and token management #8667

Uh oh!

Conversation

ysinghc commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Does this close any open issues?

Screenshots

Other Information

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ysinghc commented Dec 21, 2025

Uh oh!

klesh commented Dec 22, 2025

Uh oh!

ysinghc commented Jan 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ysinghc commented Jan 5, 2026

Uh oh!

klesh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ysinghc commented Dec 21, 2025 •

edited

Loading