Fix Microsoft OAuth callback by passing scope in token exchange#1509
Fix Microsoft OAuth callback by passing scope in token exchange#1509N2D4 wants to merge 1 commit into
Conversation
Microsoft's v2.0 token endpoint requires a 'scope' parameter in the authorization code exchange request. The openid-client library does not include it automatically since it is optional per RFC 6749. Pass scope via the exchangeBody extras parameter for all providers' callback and oauthCallback calls. Also forward the provider-specific extra scope from the outer OAuth info to the token exchange. Co-Authored-By: Konstantin Wohlwend <n2d4xc@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends OAuth callback handling to support dynamic scope passing. The route handler now forwards ChangesDynamic scope in OAuth callback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes Microsoft OAuth sign-in failures (AADSTS70011) by including the
Confidence Score: 4/5The change is targeted and low-risk, forwarding an already-validated scope string into the token exchange with no new data sources. Both changed files touch a narrow, well-understood path: base.tsx builds callbackExtras from data already present in the object, and route.tsx adds one field to an existing options struct. The merged scope passed in exchangeBody is computed identically to what getAuthorizationUrl sends, so authorization and token-exchange scopes remain in sync. The small ?? vs || inconsistency is the only finding. No files require special attention; the changes are self-contained within the two modified files. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant CallbackRoute as /callback/[provider_id]
participant BaseProvider as OAuthBaseProvider
participant OpenIDClient as openid-client
participant MicrosoftAAD as Microsoft AAD v2.0
Browser->>CallbackRoute: "GET /callback?code=...&state=..."
CallbackRoute->>CallbackRoute: Read outerInfo (providerScope from DB)
CallbackRoute->>BaseProvider: "getCallback({ extraScope: providerScope, ... })"
BaseProvider->>BaseProvider: "Build callbackExtras { exchangeBody: { scope: merge(this.scope, extraScope) } }"
alt "openid = true"
BaseProvider->>OpenIDClient: oauthClient.callback(...params, callbackExtras)
else "openid = false"
BaseProvider->>OpenIDClient: oauthClient.oauthCallback(...params, callbackExtras)
end
OpenIDClient->>MicrosoftAAD: "POST /token code=...&scope=openid+profile+..."
MicrosoftAAD-->>OpenIDClient: access_token, id_token, refresh_token
OpenIDClient-->>BaseProvider: TokenSet
BaseProvider-->>CallbackRoute: "{ userInfo, tokenSet }"
CallbackRoute->>Browser: Redirect to app
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/backend/src/oauth/providers/base.tsx:414-418
Minor style inconsistency: `getAuthorizationUrl` (line 372) uses `|| ""` for the same fallback, while the new code here uses `?? ""`. Both are functionally equivalent for string scopes, but aligning them keeps the two code sites consistent.
```suggestion
const callbackExtras = {
exchangeBody: {
scope: mergeScopeStrings(this.scope, options.extraScope || ""),
},
};
```
Reviews (1): Last reviewed commit: "Fix Microsoft OAuth callback by passing ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const callbackExtras = { | ||
| exchangeBody: { | ||
| scope: mergeScopeStrings(this.scope, options.extraScope ?? ""), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
getAuthorizationUrl (line 372) uses || "" for the same fallback, while the new code here uses ?? "". Both are functionally equivalent for string scopes, but aligning them keeps the two code sites consistent.
| const callbackExtras = { | |
| exchangeBody: { | |
| scope: mergeScopeStrings(this.scope, options.extraScope ?? ""), | |
| }, | |
| }; | |
| const callbackExtras = { | |
| exchangeBody: { | |
| scope: mergeScopeStrings(this.scope, options.extraScope || ""), | |
| }, | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/oauth/providers/base.tsx
Line: 414-418
Comment:
Minor style inconsistency: `getAuthorizationUrl` (line 372) uses `|| ""` for the same fallback, while the new code here uses `?? ""`. Both are functionally equivalent for string scopes, but aligning them keeps the two code sites consistent.
```suggestion
const callbackExtras = {
exchangeBody: {
scope: mergeScopeStrings(this.scope, options.extraScope || ""),
},
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
1 participant
Microsoft's v2.0 token endpoint requires a
scopeparameter in the authorization code exchange request (AADSTS70011). Theopenid-clientlibrary doesn't include it automatically since it's optional per RFC 6749.Pass scope via
exchangeBodyin the callback extras for all providers, and forward the provider-specific extra scope from the outer OAuth info.Link to Devin session: https://app.devin.ai/sessions/3cdf9182be7c47289d52f66de534c384
Requested by: @N2D4
Summary by cubic
Include scope in the token exchange for OAuth callbacks to satisfy Microsoft v2.0 and prevent AADSTS70011. Applied generically across providers using
openid-client’sexchangeBodyextras.exchangeBody.scopetocallback/oauthCallbackinopenid-client.extraScopefrom the route to the base provider.Written for commit ab1efa2. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit