Skip to content

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

Open
wants to merge 1 commit into
base: graphite-base/18810
Choose a base branch
from

Conversation

ThomasK33
Copy link
Member

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 proper redirect_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:

  • Migrated database schema to remove callback_url column and make redirect_uris the source of truth
  • Updated API endpoints to accept and validate multiple redirect URIs
  • Modified OAuth2 authorization flow to perform exact URI matching against registered URIs
  • Changed device authorization endpoint to accept form-urlencoded data instead of JSON
  • Updated frontend to support adding/removing multiple redirect URIs
  • Added database migration with backward compatibility support

This change improves security by enforcing stricter redirect URI validation and provides more flexibility for OAuth2 clients that need multiple callback endpoints.

Copy link
Member Author

ThomasK33 commented Jul 9, 2025

@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from dd9cb2f to ae754f4 Compare July 9, 2025 12:16
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from f9e6552 to 1444efe Compare July 9, 2025 12:16
@ThomasK33 ThomasK33 changed the base branch from thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation to graphite-base/18810 July 9, 2025 15:19
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from 1444efe to ef4a6c8 Compare July 9, 2025 22:21
@ThomasK33 ThomasK33 force-pushed the graphite-base/18810 branch from ae754f4 to ab73979 Compare July 9, 2025 22:21
@ThomasK33 ThomasK33 changed the base branch from graphite-base/18810 to thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation July 9, 2025 22:21
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from ef4a6c8 to e24c4b5 Compare July 10, 2025 09:15
@ThomasK33 ThomasK33 requested a review from johnstcn July 10, 2025 09:25
@ThomasK33 ThomasK33 marked this pull request as ready for review July 10, 2025 09:25
@ThomasK33 ThomasK33 requested review from Emyrk and mafredri July 10, 2025 11:10
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from e24c4b5 to 98e7f95 Compare July 12, 2025 12:55
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from ab73979 to b89c367 Compare July 12, 2025 12:55
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from b89c367 to b73b71d Compare July 14, 2025 12:43
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from 98e7f95 to 2792051 Compare July 14, 2025 12:43
@ThomasK33 ThomasK33 changed the base branch from thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation to graphite-base/18810 July 14, 2025 15:16
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from 2792051 to 3dd0c37 Compare July 14, 2025 16:22
@ThomasK33 ThomasK33 force-pushed the graphite-base/18810 branch from b73b71d to 386d77d Compare July 14, 2025 16:22
@ThomasK33 ThomasK33 changed the base branch from graphite-base/18810 to thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation July 14, 2025 16:22
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 386d77d to b58eed8 Compare July 14, 2025 17:18
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from 3dd0c37 to 962c22c Compare July 14, 2025 17:18
@ThomasK33 ThomasK33 changed the base branch from thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation to graphite-base/18810 July 15, 2025 16:03
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from 962c22c to bed62ad Compare July 15, 2025 17:24
@ThomasK33 ThomasK33 force-pushed the graphite-base/18810 branch from b58eed8 to 9393060 Compare July 15, 2025 17:24
@ThomasK33 ThomasK33 changed the base branch from graphite-base/18810 to thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation July 15, 2025 17:24
@ThomasK33 ThomasK33 requested a review from dannykopping July 17, 2025 12:53
Comment on lines +7 to +9
-- 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.
Copy link
Contributor

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"?

Copy link
Member Author

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.

Copy link
Member Author

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",
Copy link
Contributor

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>
@ThomasK33 ThomasK33 force-pushed the thomask33/07-08-feat_replace_callback_url_with_redirect_uris_for_oauth2_rfc_6749_compliance branch from bed62ad to 4fb1c39 Compare July 17, 2025 14:38
@ThomasK33 ThomasK33 requested a review from aslilac as a code owner July 17, 2025 14:38
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 9393060 to 2c31819 Compare July 17, 2025 14:38
@ThomasK33 ThomasK33 changed the base branch from thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation to graphite-base/18810 July 21, 2025 12:15
Icon string `json:"icon"`
ID uuid.UUID `json:"id" format:"uuid"`
Name string `json:"name"`
RedirectURIs []string `json:"redirect_uris"`
Copy link
Member

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?

Copy link
Member Author

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

Comment on lines +499 to +502
// RFC 6749: Exact match against registered redirect URIs
if slices.Contains(registeredRedirectURIs, redirectURIParam) {
return redirectURL
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +237 to 238
// @Accept application/x-www-form-urlencoded
// @Produce json
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants