Skip to content

core: fix oauth login redirect url#1172

Merged
undefined-moe merged 1 commit into
hydro-dev:masterfrom
renbaoshuo:oauth-redirect-url-fix
Jun 6, 2026
Merged

core: fix oauth login redirect url#1172
undefined-moe merged 1 commit into
hydro-dev:masterfrom
renbaoshuo:oauth-redirect-url-fix

Conversation

@renbaoshuo
Copy link
Copy Markdown
Contributor

@renbaoshuo renbaoshuo commented Jun 6, 2026

Summary by CodeRabbit

  • New Features
    • Enhanced OAuth authentication flow to preserve user navigation context, redirecting users back to their original page after successfully logging in via OAuth providers.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 6, 2026

Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes.

Review Change Stack

Walkthrough

This PR implements redirect parameter threading throughout the OAuth login flow. UserLoginHandler now accepts an optional redirect query parameter and passes it to the rendered login template. Login templates (login_dialog.html and user_login.html) are updated to include this redirect parameter when generating OAuth method links. OauthHandler captures the redirect parameter and stores it in the session before initiating provider login. OauthCallbackHandler retrieves the session-stored redirect value and uses it to determine the post-authentication destination, clearing the session value after use. This allows users to be returned to their original location after OAuth authentication completes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'core: fix oauth login redirect url' directly addresses the main change in the PR, which updates OAuth login redirect behavior across multiple handlers and templates to properly preserve and use redirect URLs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/hydrooj/src/handler/user.ts

ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/hydrooj/src/handler/user.ts (1)

480-504: ⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Critical: Registration flow doesn't preserve the redirect parameter.

The three early-return paths (bind, existing user, email match) correctly use this.session.oauthRedirect and clean it up. However, when the flow continues past line 504 to handle new registrations, the session.oauthRedirect is never transferred to the registration token created at lines 523-537.

The registration token uses this.domain.registerRedirect (line 529) instead of this.session.oauthRedirect, so after the user completes registration via UserRegisterWithCodeHandler, they'll be redirected to the domain's default register redirect instead of their original page.

🔧 Proposed fix

At line 529, use the session redirect with fallback:

             {
                 mail: r.email,
                 username,
-                redirect: this.domain.registerRedirect,
+                redirect: this.session.oauthRedirect || this.domain.registerRedirect,
                 set,
                 setInDomain: r.setInDomain,
                 identity: {

Then clean up the session after creating the token (around line 538):

             },
         );
+        delete this.session.oauthRedirect;
         this.response.redirect = this.url('user_register_with_code', { code: t });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/hydrooj/src/handler/user.ts` around lines 480 - 504, The
registration path currently sets the registration token's redirect to
this.domain.registerRedirect instead of the per-request
this.session.oauthRedirect; update the token creation to use
(this.session.oauthRedirect || this.domain.registerRedirect) when constructing
the registration token (the same place where the register token is created after
the user.getByEmail check), then immediately delete this.session.oauthRedirect
after the token is created so the session is cleaned up; keep all other behavior
(ids mapping, token creation call) the same and locate changes around the
registration token creation code that follows the existing successfulAuth /
user.getByEmail logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@packages/hydrooj/src/handler/user.ts`:
- Around line 480-504: The registration path currently sets the registration
token's redirect to this.domain.registerRedirect instead of the per-request
this.session.oauthRedirect; update the token creation to use
(this.session.oauthRedirect || this.domain.registerRedirect) when constructing
the registration token (the same place where the register token is created after
the user.getByEmail check), then immediately delete this.session.oauthRedirect
after the token is created so the session is cleaned up; keep all other behavior
(ids mapping, token creation call) the same and locate changes around the
registration token creation code that follows the existing successfulAuth /
user.getByEmail logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9141559e-88d6-4b5d-af53-d3c1f84acf3f

📥 Commits

Reviewing files that changed from the base of the PR and between 3cc1325 and 5242efb.

📒 Files selected for processing (3)
  • packages/hydrooj/src/handler/user.ts
  • packages/ui-default/templates/partials/login_dialog.html
  • packages/ui-default/templates/user_login.html

@undefined-moe undefined-moe merged commit 01771a4 into hydro-dev:master Jun 6, 2026
6 checks passed
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.

2 participants