[WEB-4861] fix: update redirection path in MagicSignInEndpoint to home page#7774
[WEB-4861] fix: update redirection path in MagicSignInEndpoint to home page#7774
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdjusts MagicSignInEndpoint.post redirect: when an existing user has Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant API as MagicSignInEndpoint
participant Redir as Redirect Handler
User->>API: POST /magic-sign-in
API->>API: Validate token, find/create user
alt user exists and is_password_autoset && profile.is_onboarded
API->>Redir: path = "/"
else other cases
API->>Redir: path = previous logic (e.g., accounts/set-password or next_path)
end
Redir-->>User: 302 redirect to base_host + path
note over API,Redir: Only the chosen path for this condition changed
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a redirection issue in the magic sign-in flow where users with auto-set passwords who have completed onboarding were being incorrectly redirected to the password setup page instead of the home page.
- Updates the redirection logic in MagicSignInEndpoint to redirect onboarded users to the home page
- Removes the unnecessary password setup redirection for users who have already completed onboarding
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/authentication/views/app/magic.py (1)
114-118: Open redirect: validate next_path on the success pathHere
next_pathis used raw. If an attacker postsnext_path=https://evil.com,urljoin(base_host, next_path)will send users off-site. In error branches you validate withvalidate_next_path, andMagicSignUpEndpointdoes this correctly—mirror that here.- path = ( - str(next_path) - if next_path - else str(get_redirection_path(user=user)) - ) + path = ( + str(validate_next_path(next_path)) + if next_path + else str(get_redirection_path(user=user)) + )
🧹 Nitpick comments (2)
apps/api/plane/authentication/views/app/magic.py (2)
110-111: Prefer "/" over empty string for home redirectionUsing
path = ""relies onurljoinquirks and the trailing-slash behavior ofbase_host."/"is explicit and avoids edge cases ifbase_host(...)changes format.- path = "" + path = "/"
109-118: Confirm scope: should all magic sign-ins land on home?Current logic redirects to home only when
user.is_password_autoset and profile.is_onboarded. If the product requirement is “all magic sign-ins → home,” drop the guard; if not, keep as-is and add a quick note/test to lock intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/authentication/views/app/magic.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/authentication/views/app/magic.py (1)
115-118: Fix open-redirect: validate next_path in success flownext_path is used without validation in MagicSignIn success path — validate it like other branches to prevent external redirects.
File: apps/api/plane/authentication/views/app/magic.py (around the path assignment in MagicSignInEndpoint.post; originally lines 115-118)
Apply this minimal patch:
- else: - # Get the redirection path - path = ( - str(next_path) - if next_path - else str(get_redirection_path(user=user)) - ) + else: + # Get the redirection path (validate any provided next_path) + path = ( + str(validate_next_path(next_path)) + if next_path + else str(get_redirection_path(user=user)) + )Optional: if next_path should be honored when present even for autoset+onboarded users, use:
- if user.is_password_autoset and profile.is_onboarded: - # Redirect to the home page - path = "/" + if user.is_password_autoset and profile.is_onboarded: + # Redirect to the home page (or a validated next_path if provided) + path = ( + str(validate_next_path(next_path)) + if next_path + else "/" + )Confirm whether next_path should take precedence when present, or whether forcing "/" is the intended behavior for this flow.
♻️ Duplicate comments (1)
apps/api/plane/authentication/views/app/magic.py (1)
110-111: Correct: redirecting to root is clear and matches the PR goal.Explicit "/" is unambiguous and resolves the earlier feedback about empty strings.
🧹 Nitpick comments (1)
apps/api/plane/authentication/views/app/magic.py (1)
110-111: Nit: extract a HOME_PATH constant to avoid magic strings.Keeps intent obvious and avoids scattering "/".
Add near the imports:
HOME_PATH = "/"And update this line:
- path = "/" + path = HOME_PATH
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/authentication/views/app/magic.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…e page (makeplane#7774) * fix: update redirection path in MagicSignInEndpoint to home page * Use / for clarity
Description
This fix will redirect the user to home page when they login via magic sign-in option.
Type of Change
Test Scenarios
Summary by CodeRabbit