fix(github): Refresh access token after app (re)installation (backport #6731) - #7185
fix(github): Refresh access token after app (re)installation (backport #6731)#7185mergify[bot] wants to merge 2 commits into
Conversation
Token refresh in the GitHub callback was gated entirely on an OAuth
`code` being present:
if code and state:
obtain_access_token(code, team)
When a user (re)installs the Frappe Cloud GitHub App, GitHub redirects
back to /github/authorize with installation_id + setup_action + the
echoed state but no `code` (a code only appears when user-authorization
runs). So the branch was skipped, the stale token survived in the Team
doc, and the next "Add from GitHub" used a dead token.
Now when the callback arrives with a valid state but no code (and no
error), start GitHub user-authorization so the user bounces back with a
code that refreshes the token. If the app is already authorized this is
a silent round-trip; a denied authorization falls through to a plain
dashboard redirect, so there is no loop. Code-bearing callbacks are
unchanged.
The guest-login redirect now preserves state without a code too, so a
guest landing on the install callback completes the same refresh after
logging in.
(cherry picked from commit b3a71e5)
The install/setup callback reuses the same HMAC-signed state for the extra user-authorization hop, but the state carries its original issued_at. The install wizard plus the OAuth consent screen can together exceed GITHUB_OAUTH_STATE_MAX_AGE, so the final code-bearing callback fails decode_github_oauth_state, the broad except in get_context swallows it, obtain_access_token is skipped, and the stale token survives — the exact failure this change set out to fix. Re-issue the state (fresh issued_at, same team/redirect) before redirecting to authorization so each leg gets its own validity window and neither bears the cumulative wizard + consent time. (cherry picked from commit b7eb288)
|
Tick the box to add this pull request to the merge queue (same as
|
|
@greptileai review |
Confidence Score: 4/5The guest denial path should be fixed before merging because login currently converts a denied callback into another authorization attempt. Callback reconstruction drops Files Needing Attention: press/api/github.py, press/www/github/authorize.py Reviews (1): Last reviewed commit: "fix(github): Re-issue OAuth state on the..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if code: | ||
| params["code"] = code | ||
| params["state"] = state | ||
| callback_url = f"/github/authorize?{urlencode(params)}" |
There was a problem hiding this comment.
When a guest callback contains state and error=access_denied, this reconstruction drops the error; after login, the state-only callback starts GitHub authorization again, causing a user who denied access to be sent through another authorization attempt.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7185 +/- ##
==========================================
+ Coverage 58.90% 58.93% +0.02%
==========================================
Files 1011 1011
Lines 91706 91767 +61
Branches 1226 1226
==========================================
+ Hits 54021 54079 +58
- Misses 37662 37665 +3
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Token refresh in the GitHub callback was gated entirely on an OAuth
codebeing present:When a user (re)installs the Frappe Cloud GitHub App, GitHub redirects back to /github/authorize with installation_id + setup_action + the echoed state but no
code(a code only appears when user-authorization runs). So the branch was skipped, the stale token survived in the Team doc, and the next "Add from GitHub" used a dead token.Now when the callback arrives with a valid state but no code (and no error), start GitHub user-authorization so the user bounces back with a code that refreshes the token. If the app is already authorized this is a silent round-trip; a denied authorization falls through to a plain dashboard redirect, so there is no loop. Code-bearing callbacks are unchanged.
The guest-login redirect now preserves state without a code too, so a guest landing on the install callback completes the same refresh after logging in.
This is an automatic backport of pull request #6731 done by Mergify.