baton-github: refersh appjwttoken once it expires #117
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
WalkthroughReworks GitHub App authentication in pkg/connector: introduces JWT generation and a jwtExpiryTime, adds an app JWT refresher TokenSource, changes installation lookup to return installation+error, and refactors token refreshers to use oauth2.TokenSource for JWT and installation tokens. Changes
Sequence Diagram(s)sequenceDiagram
participant Connector as "Connector\n(pkg/connector)"
participant JWTRefresher as "appJWTTokenRefresher\n(oauth2.TokenSource)"
participant GitHub as "GitHub API\n(Installations & App Auth)"
participant AppTokenRef as "appTokenRefresher\n(Token provider)"
Connector->>JWTRefresher: Request Token()
JWTRefresher->>GitHub: generate signed JWT (getJWTToken)
GitHub-->>JWTRefresher: signed JWT + expiry (jwtExpiryTime)
JWTRefresher-->>Connector: oauth2.Token (app JWT)
Connector->>GitHub: findInstallation(appID, owner/repo)
GitHub-->>Connector: Installation ID
Connector->>AppTokenRef: Request Token() using jwt TokenSource
AppTokenRef->>GitHub: Exchange app JWT for installation token
GitHub-->>AppTokenRef: installation token + expiry
AppTokenRef-->>Connector: oauth2.Token (installation token)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Bencheng21
force-pushed
the
ben/refersh_jwtToken
branch
from
9e6bfb1 to
a7c5106
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/connector.go (1)
219-227:⚠️ Potential issue | 🟠 MajorGuard against empty org list to avoid panic.
orgLogins[0]will panic whengh.orgsis empty. Align this withNew()by requiring exactly one org in app mode.Suggested fix
func (gh *GitHub) validateAppCredentials(ctx context.Context) (annotations.Annotations, error) { orgLogins := gh.orgs - if len(orgLogins) > 1 { - return nil, fmt.Errorf("github-connector: only one org is allowed when using github app") - } + if len(orgLogins) != 1 { + return nil, fmt.Errorf("github-connector: exactly one org is required when using github app") + } _, err := findInstallation(ctx, gh.appClient, orgLogins[0])
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 399-414: The getJWTToken function should validate that appID is
non-empty before creating the JWT; add an explicit check at the start of
getJWTToken (before calling loadPrivateKeyFromString) that returns a clear error
if appID is empty, so the generated token never contains an empty "iss" claim;
update the function to return that error (e.g., from getJWTToken) and ensure
callers handle it.
| func getJWTToken(appID string, privateKey string) (string, error) { | ||
| key, err := loadPrivateKeyFromString(privateKey) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| now := time.Now() | ||
| token, err := jwtv5.NewWithClaims(jwtv5.SigningMethodRS256, jwtv5.MapClaims{ | ||
| "iat": now.Unix() - 60, // issued at | ||
| "exp": now.Add(time.Minute * 10).Unix(), // expires | ||
| "iss": ghc.AppId, // GitHub App ID | ||
| "iss": appID, // GitHub App ID | ||
| }).SignedString(key) | ||
| if err != nil { | ||
| return "", "", err | ||
| return "", err | ||
| } | ||
| return token, "", nil | ||
| return token, nil | ||
| } |
There was a problem hiding this comment.
Validate appID before signing.
An empty appID yields an invalid iss claim and opaque auth failures. Add an explicit check.
Suggested fix
func getJWTToken(appID string, privateKey string) (string, error) {
+ if strings.TrimSpace(appID) == "" {
+ return "", errors.New("github-connector: app ID is required")
+ }
key, err := loadPrivateKeyFromString(privateKey)
if err != nil {
return "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func getJWTToken(appID string, privateKey string) (string, error) { | |
| key, err := loadPrivateKeyFromString(privateKey) | |
| if err != nil { | |
| return "", err | |
| } | |
| now := time.Now() | |
| token, err := jwtv5.NewWithClaims(jwtv5.SigningMethodRS256, jwtv5.MapClaims{ | |
| "iat": now.Unix() - 60, // issued at | |
| "exp": now.Add(time.Minute * 10).Unix(), // expires | |
| "iss": ghc.AppId, // GitHub App ID | |
| "iss": appID, // GitHub App ID | |
| }).SignedString(key) | |
| if err != nil { | |
| return "", "", err | |
| return "", err | |
| } | |
| return token, "", nil | |
| return token, nil | |
| } | |
| func getJWTToken(appID string, privateKey string) (string, error) { | |
| if strings.TrimSpace(appID) == "" { | |
| return "", errors.New("github-connector: app ID is required") | |
| } | |
| key, err := loadPrivateKeyFromString(privateKey) | |
| if err != nil { | |
| return "", err | |
| } | |
| now := time.Now() | |
| token, err := jwtv5.NewWithClaims(jwtv5.SigningMethodRS256, jwtv5.MapClaims{ | |
| "iat": now.Unix() - 60, // issued at | |
| "exp": now.Add(time.Minute * 10).Unix(), // expires | |
| "iss": appID, // GitHub App ID | |
| }).SignedString(key) | |
| if err != nil { | |
| return "", err | |
| } | |
| return token, nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 399 - 414, The getJWTToken function
should validate that appID is non-empty before creating the JWT; add an explicit
check at the start of getJWTToken (before calling loadPrivateKeyFromString) that
returns a clear error if appID is empty, so the generated token never contains
an empty "iss" claim; update the function to return that error (e.g., from
getJWTToken) and ensure callers handle it.
| installation, resp, err := c.Apps.FindOrganizationInstallation(ctx, orgName) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return nil, wrapGitHubError(err, resp, "github-connector: failed to find installation") |
There was a problem hiding this comment.
this is an unrelated change just to wrap the GitHub error in one place yes?
| jwtTokenSource: oauth2.ReuseTokenSource( | ||
| &oauth2.Token{ | ||
| AccessToken: jwttoken, | ||
| Expiry: time.Now().Add(jwtExpiryTime), |
There was a problem hiding this comment.
so we set an expiration time on the token that is different than on the jwt, because it will expire sooner and lead to a refresh before the token expires. That aligns with the description of recommended library in #115
There was a problem hiding this comment.
The JWT is valid for 10 minutes. It’s better to refresh it a bit earlier to avoid a 401 error interrupting the workflow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/connector.go`:
- Around line 225-228: The code indexes orgLogins[0] before ensuring the slice
is non-empty which can panic; update the caller in connector.go to check
len(orgLogins) > 0 before calling findInstallation and return a descriptive
error (e.g., "no org logins provided") when empty, then only call
findInstallation(ctx, gh.appClient, orgLogins[0]) when the guard passes;
reference orgLogins and findInstallation to locate the change and ensure any
returned error is propagated unchanged.
🧹 Nitpick comments (1)
pkg/connector/connector.go (1)
279-307: Reuse the refreshable JWT source forappClient.Currently,
appClientis created with a static JWT token (lines 270-275) but is used in long-running contexts: it's passed toorgBuilderand included inResourceSyncers(line 110) where it will be used during resource syncing, and it's also used invalidateAppCredentials(line 225). Since JWT tokens expire after ~10 minutes,appClientwill become invalid if syncing or validation operations extend beyond initialization. The refreshable JWT source introduced at lines 298-307 should be reused forappClientas well, similar to how it's used for the main clients (lines 312-316).
| _, err := findInstallation(ctx, gh.appClient, orgLogins[0]) | ||
| if err != nil { | ||
| return nil, wrapGitHubError(err, resp, "github-connector: failed to retrieve org installation") | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Guard against empty org list before indexing.
Line 225 indexes orgLogins[0]; if the org list is empty (misconfig or future call path), this will panic. Return a clear error instead.
✅ Proposed fix
func (gh *GitHub) validateAppCredentials(ctx context.Context) (annotations.Annotations, error) {
orgLogins := gh.orgs
if len(orgLogins) > 1 {
return nil, fmt.Errorf("github-connector: only one org is allowed when using github app")
}
+ if len(orgLogins) == 0 {
+ return nil, fmt.Errorf("github-connector: org is required when using github app")
+ }
_, err := findInstallation(ctx, gh.appClient, orgLogins[0])
if err != nil {
return nil, err
}
return nil, nil
}🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 225 - 228, The code indexes
orgLogins[0] before ensuring the slice is non-empty which can panic; update the
caller in connector.go to check len(orgLogins) > 0 before calling
findInstallation and return a descriptive error (e.g., "no org logins provided")
when empty, then only call findInstallation(ctx, gh.appClient, orgLogins[0])
when the guard passes; reference orgLogins and findInstallation to locate the
change and ensure any returned error is propagated unchanged.
Description.
see here for the issue.
Fixes
Refresh the app JWT token once it expired.
Test
Summary by CodeRabbit