- Notifications
You must be signed in to change notification settings - Fork948
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:graphite-base/18810
Are you sure you want to change the base?
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 to962c22cCompare962c22c tobed62adCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| -- being an array with a single NULL element. This is preferable to an empty | ||
| -- array as it will pass a CHECK for array length > 0, enforcing that all | ||
| -- apps have at least one URI entry, even if it's null. |
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.
Can't we make theCHECK check for the cardinality OR the "nullness"?
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 think I'm doing that in the client_credentials PR up in the stack, but I would need to double-check.
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'm sorry. I just reread the comment. I'd rather not introduce a possible null here and perform a clean migration from callback_url to redirect_uris, then make sure that all of them get carried over.
The constraint forredirect_uris will be loosened in the PR introducing client_credentials. We'll then allow setting an empty redirect_uris array but not a null one.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ | ||
| Status: http.StatusInternalServerError, | ||
| HideStatus: false, | ||
| Title: "Internal Server Error", | ||
| Description:err.Error(), | ||
| Description:"OAuth2 app has no registered 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.
Ditto; bad request rather than internal server error.
Uh oh!
There was an error while loading.Please reload this page.
…plianceChange-Id: I4823e475777ebdf75e3a80e47ff6bef1a556cd55Signed-off-by: Thomas Kosiewski <tk@coder.com>
bed62ad to4fb1c39Compare9393060 to2c31819Comparesite/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsxShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| Iconstring`json:"icon"` | ||
| ID uuid.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
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.