fix(oauth): allow absolute OAuth authorization consent URLs#2417
fix(oauth): allow absolute OAuth authorization consent URLs#2417SergioChan wants to merge 2 commits intosupabase:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a new helper that builds the authorization redirect URL by appending Assessment against linked issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/oauthserver/authorize.go`:
- Around line 625-629: The issue is that absolute URLs returned by
buildAuthorizationURL are being concatenated with "?authorization_id=..." as raw
strings so existing query strings or fragments are lost; to fix, update
buildAuthorizationURL and the redirect construction to treat absolute URLs as
proper url.URL objects: in buildAuthorizationURL, when parsed.IsAbs() return
parsed.String() (preserving parsed.RawQuery and parsed.Fragment), and change the
code that appends the authorization_id (the place that currently does raw string
concatenation like "+\"?authorization_id=\"+id") to parse the returned URL with
url.Parse, use u.Query().Add("authorization_id", id), set u.RawQuery =
q.Encode(), and then use u.String() so query params and fragments are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a9c080d4-2a60-4f59-9bc9-f3bc37edbddd
📒 Files selected for processing (2)
internal/api/oauthserver/authorize.gointernal/api/oauthserver/authorize_test.go
| // buildAuthorizationURL safely joins a base URL with a path, handling slashes correctly. | ||
| // If pathToJoin is an absolute URL, it is returned as-is. | ||
| func (s *Server) buildAuthorizationURL(baseURL, pathToJoin string) string { | ||
| if parsed, err := url.Parse(pathToJoin); err == nil && parsed.IsAbs() { | ||
| return pathToJoin |
There was a problem hiding this comment.
Absolute consent UIs still fail the follow-up API calls in the browser flow.
This redirect can now send the user to an external consent page, but GET /oauth/authorizations/{authorization_id} and POST /oauth/authorizations/{authorization_id}/consent still reject origins that are not allowed by validateRequestOrigin. So a browser-hosted consent UI on the configured absolute URL still 400s with unauthorized request origin unless that origin is separately duplicated in the allow-list, which means the new config alone does not complete the hosted flow this PR is trying to unblock.
Preserve query/fragment handling when passing absolute URLs through.
With this new absolute-URL mode, the redirect built at Line 170 is now wrong for valid values like https://consent.example.com/ui?tenant=acme or URLs with a fragment, because ?authorization_id=... is appended by raw string concatenation. In those cases the consent UI will not receive a usable authorization_id.
Proposed fix
baseURL := s.buildAuthorizationURL(config.SiteURL, config.OAuthServer.AuthorizationPath)
- redirectURL := fmt.Sprintf("%s?authorization_id=%s", baseURL, authorization.AuthorizationID)
+ u, err := url.Parse(baseURL)
+ if err != nil {
+ errorRedirectURL := s.buildErrorRedirectURL(params.RedirectURI, oAuth2ErrorServerError, "invalid oauth authorization path", params.State)
+ http.Redirect(w, r, errorRedirectURL, http.StatusFound)
+ return nil
+ }
+ q := u.Query()
+ q.Set("authorization_id", authorization.AuthorizationID)
+ u.RawQuery = q.Encode()
+ redirectURL := u.String()
http.Redirect(w, r, redirectURL, http.StatusFound)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // buildAuthorizationURL safely joins a base URL with a path, handling slashes correctly. | |
| // If pathToJoin is an absolute URL, it is returned as-is. | |
| func (s *Server) buildAuthorizationURL(baseURL, pathToJoin string) string { | |
| if parsed, err := url.Parse(pathToJoin); err == nil && parsed.IsAbs() { | |
| return pathToJoin | |
| baseURL := s.buildAuthorizationURL(config.SiteURL, config.OAuthServer.AuthorizationPath) | |
| u, err := url.Parse(baseURL) | |
| if err != nil { | |
| errorRedirectURL := s.buildErrorRedirectURL(params.RedirectURI, oAuth2ErrorServerError, "invalid oauth authorization path", params.State) | |
| http.Redirect(w, r, errorRedirectURL, http.StatusFound) | |
| return nil | |
| } | |
| q := u.Query() | |
| q.Set("authorization_id", authorization.AuthorizationID) | |
| u.RawQuery = q.Encode() | |
| redirectURL := u.String() | |
| http.Redirect(w, r, redirectURL, http.StatusFound) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/oauthserver/authorize.go` around lines 625 - 629, The issue is
that absolute URLs returned by buildAuthorizationURL are being concatenated with
"?authorization_id=..." as raw strings so existing query strings or fragments
are lost; to fix, update buildAuthorizationURL and the redirect construction to
treat absolute URLs as proper url.URL objects: in buildAuthorizationURL, when
parsed.IsAbs() return parsed.String() (preserving parsed.RawQuery and
parsed.Fragment), and change the code that appends the authorization_id (the
place that currently does raw string concatenation like
"+\"?authorization_id=\"+id") to parse the returned URL with url.Parse, use
u.Query().Add("authorization_id", id), set u.RawQuery = q.Encode(), and then use
u.String() so query params and fragments are preserved.
|
Thanks for the review — I pushed an update that addresses the URL-concatenation concern. What changed
Validation
|
1 participant
Summary
GOTRUE_OAUTH_SERVER_AUTHORIZATION_PATHas either a relative path or an absolute URL when building the consent redirect target/oauth/consentjoins againstSITE_URL) while allowing hosted users to point consent flows at a dedicated external UIbuildAuthorizationURLTesting
go test ./internal/api/oauthserver -run 'TestBuildAuthorizationURL|TestValidateRequestOrigin'Related