-
Notifications
You must be signed in to change notification settings - Fork 948
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 our terms of service and privacy 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
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 stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dd9cb2f
to
ae754f4
Compare
f9e6552
to
1444efe
Compare
1444efe
to
ef4a6c8
Compare
ae754f4
to
ab73979
Compare
ef4a6c8
to
e24c4b5
Compare
e24c4b5
to
98e7f95
Compare
ab73979
to
b89c367
Compare
b89c367
to
b73b71d
Compare
98e7f95
to
2792051
Compare
2792051
to
3dd0c37
Compare
b73b71d
to
386d77d
Compare
386d77d
to
b58eed8
Compare
3dd0c37
to
962c22c
Compare
962c22c
to
bed62ad
Compare
b58eed8
to
9393060
Compare
-- 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 the CHECK
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 for redirect_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.
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.
…pliance Change-Id: I4823e475777ebdf75e3a80e47ff6bef1a556cd55 Signed-off-by: Thomas Kosiewski <tk@coder.com>
bed62ad
to
4fb1c39
Compare
9393060
to
2c31819
Compare
site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/OAuth2AppForm.tsx
Show resolved
Hide resolved
Icon string `json:"icon"` | ||
ID uuid.UUID `json:"id" format:"uuid"` | ||
Name string `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 | ||
if slices.Contains(registeredRedirectURIs, redirectURIParam) { | ||
return redirectURL | ||
} |
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 spec https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2.3
// @Accept application/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 is https://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_url
field with a properredirect_uris
array. This change allows OAuth2 clients to register multiple valid redirect URIs and enforces exact URI matching as required by the specification.Key changes:
callback_url
column and makeredirect_uris
the source of truthThis change improves security by enforcing stricter redirect URI validation and provides more flexibility for OAuth2 clients that need multiple callback endpoints.