Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions backend/helpers/pluginhelper/api/api_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ func (apiClient *ApiClient) SetLogger(logger log.Logger) {
apiClient.logger = logger
}

// GetClient returns the underlying http.Client
func (apiClient *ApiClient) GetClient() *http.Client {
return apiClient.client
}

func (apiClient *ApiClient) logDebug(format string, a ...interface{}) {
if apiClient.logger != nil {
apiClient.logger.Debug(format, a...)
Expand Down
17 changes: 16 additions & 1 deletion backend/plugins/github/models/connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,21 @@ type GithubConn struct {
helper.MultiAuth `mapstructure:",squash"`
GithubAccessToken `mapstructure:",squash" authMethod:"AccessToken"`
GithubAppKey `mapstructure:",squash" authMethod:"AppKey"`
RefreshToken string `mapstructure:"refreshToken" json:"refreshToken" gorm:"type:text;serializer:encdec"`
TokenExpiresAt time.Time `mapstructure:"tokenExpiresAt" json:"tokenExpiresAt"`
RefreshTokenExpiresAt time.Time `mapstructure:"refreshTokenExpiresAt" json:"refreshTokenExpiresAt"`
}

// UpdateToken updates the token and refresh token information
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
Comment on lines +71 to +73
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
Comment on lines +65 to 74
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// PrepareApiClient splits Token to tokens for SetupAuthentication to utilize
Expand Down Expand Up @@ -249,7 +264,7 @@ func (conn *GithubConn) typeIs(token string) string {
// total len is 40, {prefix}{showPrefix}{secret}{showSuffix}
// fine-grained tokens
// github_pat_{82_characters}
classicalTokenClassicalPrefixes := []string{"ghp_", "gho_", "ghs_", "ghr_"}
classicalTokenClassicalPrefixes := []string{"ghp_", "gho_", "ghs_", "ghr_", "ghu_"}
classicalTokenFindGrainedPrefixes := []string{"github_pat_"}
for _, prefix := range classicalTokenClassicalPrefixes {
if strings.HasPrefix(token, prefix) {
Expand Down
11 changes: 11 additions & 0 deletions backend/plugins/github/models/connection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,3 +227,14 @@ func TestGithubConnection_Sanitize(t *testing.T) {
})
}
}

func TestTokenTypeClassification(t *testing.T) {
conn := &GithubConn{}
assert.Equal(t, GithubTokenTypeClassical, conn.typeIs("ghp_123"))
assert.Equal(t, GithubTokenTypeClassical, conn.typeIs("gho_123"))
assert.Equal(t, GithubTokenTypeClassical, conn.typeIs("ghu_123"))
assert.Equal(t, GithubTokenTypeClassical, conn.typeIs("ghs_123"))
assert.Equal(t, GithubTokenTypeClassical, conn.typeIs("ghr_123"))
assert.Equal(t, GithubTokenTypeFineGrained, conn.typeIs("github_pat_123"))
assert.Equal(t, GithubTokenTypeUnknown, conn.typeIs("some_other_token"))
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package migrationscripts

import (
"time"

"github.com/apache/incubator-devlake/core/context"
"github.com/apache/incubator-devlake/core/errors"
"github.com/apache/incubator-devlake/helpers/migrationhelper"
)

type githubConnection20241120 struct {
RefreshToken string `gorm:"type:text;serializer:encdec"`
TokenExpiresAt time.Time
RefreshTokenExpiresAt time.Time
}

func (githubConnection20241120) TableName() string {
return "_tool_github_connections"
}

type addRefreshTokenFields struct{}

func (*addRefreshTokenFields) Up(basicRes context.BasicRes) errors.Error {
return migrationhelper.AutoMigrateTables(
basicRes,
&githubConnection20241120{},
)
}

func (*addRefreshTokenFields) Version() uint64 {
return 20241120000001
}

func (*addRefreshTokenFields) Name() string {
return "add refresh token fields to github_connections"
}
1 change: 1 addition & 0 deletions backend/plugins/github/models/migrationscripts/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,6 @@ func All() []plugin.MigrationScript {
new(addIsDraftToPr),
new(changeIssueComponentType),
new(addIndexToGithubJobs),
new(addRefreshTokenFields),
}
}
19 changes: 19 additions & 0 deletions backend/plugins/github/tasks/api_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/apache/incubator-devlake/core/plugin"
"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
"github.com/apache/incubator-devlake/plugins/github/models"
"github.com/apache/incubator-devlake/plugins/github/token"
)

func CreateApiClient(taskCtx plugin.TaskContext, connection *models.GithubConnection) (*api.ApiAsyncClient, errors.Error) {
Expand All @@ -34,6 +35,24 @@ func CreateApiClient(taskCtx plugin.TaskContext, connection *models.GithubConnec
return nil, err
}

// 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
}
Comment on lines +38 to +54
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

// create rate limit calculator
rateLimiter := &api.ApiRateLimitCalculator{
UserRateLimitPerHour: connection.RateLimitPerHour,
Expand Down
90 changes: 90 additions & 0 deletions backend/plugins/github/token/round_tripper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/*
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package token

import (
"net/http"
)

// RefreshRoundTripper is an HTTP transport middleware that automatically manages OAuth token refreshes.
// It wraps an underlying http.RoundTripper and provides token refresh on auth failures.
// On 401's the round tripper will:
// - Force a refresh of the OAuth token via the TokenProvider
// - Retry the original request with the new token
type RefreshRoundTripper struct {
base http.RoundTripper
tokenProvider *TokenProvider
}

func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider) *RefreshRoundTripper {
return &RefreshRoundTripper{
base: base,
tokenProvider: tp,
}
}
Comment on lines +29 to +39
Copy link

Copilot AI Dec 21, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.

// RoundTrip implements the http.RoundTripper interface and handles automatic token refresh on 401 responses.
// It clones the request, adds the Authorization header, and retries once with a refreshed token if needed.
func (rt *RefreshRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
return rt.roundTripWithRetry(req, false)
}

// roundTripWithRetry performs the actual request with retry on 401.
// The refreshAttempted parameter tracks whether a refresh has already been tried for this request
// to prevent infinite retry loops if token refresh itself fails.
func (rt *RefreshRoundTripper) roundTripWithRetry(req *http.Request, refreshAttempted bool) (*http.Response, error) {
// Get token
token, err := rt.tokenProvider.GetToken()
if err != nil {
return nil, err
}

// Clone request before modifying
reqClone := req.Clone(req.Context())
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
reqClone.Header.Set("Authorization", "Bearer "+token)

// Execute request
resp, reqErr := rt.base.RoundTrip(reqClone)
if reqErr != nil {
return nil, reqErr
}

// Reactive refresh on 401
if resp.StatusCode == http.StatusUnauthorized && !refreshAttempted {
// Close previous response body
resp.Body.Close()

// Force refresh
if err := rt.tokenProvider.ForceRefresh(token); err != nil {
return nil, err
}

// Get new token
newToken, err := rt.tokenProvider.GetToken()
if err != nil {
return nil, err
}

// Retry request with new token
reqRetry := req.Clone(req.Context())
Copy link

Copilot AI Jan 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
reqRetry.Header.Set("Authorization", "Bearer "+newToken)
return rt.roundTripWithRetry(reqRetry, true)
}

return resp, nil
}
101 changes: 101 additions & 0 deletions backend/plugins/github/token/round_tripper_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package token

import (
"bytes"
"io"
"net/http"
"testing"
"time"

"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
"github.com/apache/incubator-devlake/impls/logruslog"
"github.com/apache/incubator-devlake/plugins/github/models"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

func TestRoundTripper401Refresh(t *testing.T) {
mockRT := new(MockRoundTripper)
client := &http.Client{Transport: mockRT}

conn := &models.GithubConnection{
GithubConn: models.GithubConn{
RefreshToken: "refresh_token",
GithubAccessToken: models.GithubAccessToken{
AccessToken: api.AccessToken{
Token: "old_token",
},
},
TokenExpiresAt: time.Now().Add(10 * time.Minute), // Not expired
GithubAppKey: models.GithubAppKey{
AppKey: api.AppKey{
AppId: "123",
SecretKey: "secret",
},
},
},
}

logger, _ := logruslog.NewDefaultLogger(logrus.New())
tp := NewTokenProvider(conn, nil, client, logger)
rt := NewRefreshRoundTripper(mockRT, tp)

// Request
req, _ := http.NewRequest("GET", "https://api.github.com/user", nil)

// 1. First call returns 401
resp401 := &http.Response{
StatusCode: 401,
Body: io.NopCloser(bytes.NewBufferString("Unauthorized")),
}
mockRT.On("RoundTrip", mock.MatchedBy(func(r *http.Request) bool {
return r.Header.Get("Authorization") == "Bearer old_token" && r.URL.String() == "https://api.github.com/user"
})).Return(resp401, nil).Once()

// 2. Refresh call (triggered by 401)
respRefresh := &http.Response{
StatusCode: 200,
Body: io.NopCloser(bytes.NewBufferString(`{"access_token":"new_token","refresh_token":"new_refresh_token","expires_in":3600,"refresh_token_expires_in":3600}`)),
}
// The refresh call uses the same client, so it goes through mockRT too!
mockRT.On("RoundTrip", mock.MatchedBy(func(r *http.Request) bool {
return r.URL.String() == "https://github.com/login/oauth/access_token"
})).Return(respRefresh, nil).Once()

// 3. Retry call with new token
resp200 := &http.Response{
StatusCode: 200,
Body: io.NopCloser(bytes.NewBufferString("Success")),
}
mockRT.On("RoundTrip", mock.MatchedBy(func(r *http.Request) bool {
return r.Header.Get("Authorization") == "Bearer new_token" && r.URL.String() == "https://api.github.com/user"
})).Return(resp200, nil).Once()

// Execute
resp, err := rt.RoundTrip(req)
assert.NoError(t, err)
assert.Equal(t, 200, resp.StatusCode)

body, _ := io.ReadAll(resp.Body)
assert.Equal(t, "Success", string(body))

mockRT.AssertExpectations(t)
}
Loading
Loading