- Notifications
You must be signed in to change notification settings - Fork1k
refactor: replace OAuth2 callback_url with RFC 6749 compliant redirect_uris#18810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ThomasK33 commentedJul 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
dd9cb2f toae754f4Comparef9e6552 to1444efeCompare1444efe toef4a6c8Compareef4a6c8 toe24c4b5Comparee24c4b5 to98e7f95Compareab73979 tob89c367Compareb89c367 tob73b71dCompare98e7f95 to2792051Compare2792051 to3dd0c37Compare386d77d tob58eed8Compare3dd0c37 to962c22cComparesite/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| Iconstring`json:"icon"` | ||
| IDuuid.UUID`json:"id" format:"uuid"` | ||
| Namestring`json:"name"` | ||
| RedirectURIs []string`json:"redirect_uris"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As I understand, you could only get this response if the OAuth2 provider was enabled, which was only available on development builds and not release builds. So not a breaking change, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, either a dev build or if one actively enables the oauth2 experiment
| // RFC 6749: Exact match against registered redirect URIs | ||
| ifslices.Contains(registeredRedirectURIs,redirectURIParam) { | ||
| returnredirectURL | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
suggestion: add a link to the spechttps://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.3
| // @Acceptapplication/x-www-form-urlencoded | ||
| // @Produce json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I found it hard to believe, but here it ishttps://datatracker.ietf.org/doc/html/rfc8628#section-3.2
| <divcss={{display:"flex",alignItems:"center",gap:8}}> | ||
| <spancss={{fontWeight:500,fontSize:14}}>Redirect URIs</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could you please use tailwind classes? we're currently trying to move away from emotion
| <IconButton | ||
| size="small" | ||
| onClick={addRedirectUri} | ||
| css={{padding:4}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| css={{padding:4}} | |
| className="p-1" |
same here, and for reference one tailwind "spacing" is basically 4px
| {redirectUris.map((uri,index)=>( | ||
| <div | ||
| key={index} | ||
| css={{display:"flex",gap:8,alignItems:"flex-start"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| css={{display:"flex",gap:8,alignItems:"flex-start"}} | |
| className="flex gap-2 items-start" |
| <IconButton | ||
| size="small" | ||
| onClick={()=>removeRedirectUri(index)} | ||
| css={{padding:4,marginTop:index===0 ?8 :0}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is themarginTop here to "center" the button? if so you could useflex items-center.
also we probably want to add a story for this component, I went to look at it locally and it doesn't seem accessible from the existing OAuth2 stories.
4fb1c39 to055d631Compare055d631 tobdc94d5Compare9dfcc8a toaca2f6aCompare…plianceChange-Id: I4823e475777ebdf75e3a80e47ff6bef1a556cd55Signed-off-by: Thomas Kosiewski <tk@coder.com>
bdc94d5 to67bf443Compareaca2f6a to35d7f5aCompare
RFC 6749 Compliance: Replace callback_url with redirect_uris Array
This PR improves OAuth2 provider compliance with RFC 6749 by replacing the single
callback_urlfield with a properredirect_urisarray. This change allows OAuth2 clients to register multiple valid redirect URIs and enforces exact URI matching as required by the specification.Key changes:
callback_urlcolumn and makeredirect_uristhe source of truthThis change improves security by enforcing stricter redirect URI validation and provides more flexibility for OAuth2 clients that need multiple callback endpoints.