Add redirection after login #396
Conversation
Store previous page URL before login. Redirect user back to stored URL after successful login. Add default redirect to home page if URL doesn't exists.
WalkthroughThe changes introduced in this pull request enhance the user login flow by implementing location-based redirection. In the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
frontend/src/pages/auth/callback/Index.tsx (1)
24-26: Consider validating the redirect path.The implementation looks good and aligns with the PR objectives. However, consider validating the redirect path from sessionStorage to prevent potential open redirect vulnerabilities.
Here's a suggested improvement:
- const redirect = sessionStorage.getItem("redirect") || "/"; - sessionStorage.removeItem("redirect"); - navigate(redirect); + const redirect = sessionStorage.getItem("redirect"); + sessionStorage.removeItem("redirect"); + // Ensure the redirect path is internal + const isValidPath = redirect && (redirect.startsWith('/') && !redirect.startsWith('//')); + navigate(isValidPath ? redirect : '/');frontend/src/pages/Index.tsx (1)
15-19: Consider handling error cases in the login flow.The implementation meets the core requirement but should consider handling these scenarios:
- Social login failure - how to preserve the redirect path
- Invalid stored paths - how to gracefully fallback
- Timeout scenarios - how long should the redirect path persist
Consider adding error boundaries and appropriate error handling to ensure a smooth user experience even when things go wrong.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/pages/Index.tsx(2 hunks)frontend/src/pages/auth/callback/Index.tsx(1 hunks)
🔇 Additional comments (3)
frontend/src/pages/auth/callback/Index.tsx (1)
24-26: Verify the storage of redirect path.
Let's verify that the Index component correctly stores the redirect path in sessionStorage.
✅ Verification successful
Let me gather more information about how the redirect is being set in Index.tsx.
The redirect path storage and retrieval implementation is correct
The implementation forms a complete flow:
- In
frontend/src/pages/Index.tsx, the redirect path is properly stored usingsessionStorage.setItem("redirect", from)before OAuth login, wherefromcaptures the original location - In the callback component, this path is correctly retrieved and cleared using
sessionStorage.getItem("redirect")followed bysessionStorage.removeItem("redirect")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of redirect path storage in Index component
# Test: Search for sessionStorage.setItem with "redirect"
echo "Searching for redirect path storage implementation..."
rg -l 'sessionStorage\.setItem.*redirect' 'frontend/src'
# Test: Verify the usage of useLocation
echo "Verifying useLocation usage for pathname..."
ast-grep --pattern 'const $_ = useLocation()'
Length of output: 756
Script:
#!/bin/bash
# Get the implementation from Index.tsx where redirect is being set
echo "Checking Index.tsx implementation for redirect storage:"
rg -A 5 -B 5 'sessionStorage\.setItem.*redirect' frontend/src/pages/Index.tsx
# Check the auth callback implementation
echo -e "\nChecking auth callback implementation:"
cat frontend/src/pages/auth/callback/Index.tsx
Length of output: 1575
frontend/src/pages/Index.tsx (2)
4-4: LGTM! Appropriate use of react-router-dom's useLocation hook.
The import is correctly placed and uses the recommended hook for accessing router location information.
14-14: Consider validating the redirect pathname for security.
While the code correctly handles the redirect path, it's important to validate the pathname to prevent open redirect vulnerabilities. Consider implementing checks to ensure the path is internal to your application.
Example validation approach:
const isValidRedirectPath = (path: string) => {
// Ensure path starts with / and doesn't contain protocol/domain
return path.startsWith('/') && !path.includes('://') && !path.includes('//');
};
const from = location.state?.from?.pathname || "/";
const safePath = isValidRedirectPath(from) ? from : "/";Also applies to: 16-16
There was a problem hiding this comment.
It seems that it’s not working as expected.
I tested the following case:
- Access a document without an authorization token and get redirected to the login page.
- Log in with GitHub.
- The page is redirected to the workspace instead of the document page (it should redirect to the document page).
I think it is routed byfrontend/src/components/common/GuestRoute.tsx.
|
@devleejb |
|
@wet6123 I believe it should also work well in strict mode. What do you think? |
|
@devleejb I think it's better to make it work in strict mode. I can finish it until this weekend. |
|
Is there any progress? |
|
@devleejb Currently, this function is implemented to perform using session storage. If we simply remove 'removeItem' for session storage, it will be able to operate in strict mode. This has the disadvantage of leaving url in session storage. However, it seems to be the simplest way. Another way I think is to customize the github authguard on the nestjs server to manage the "url to move after login" as a cookie. This seems quite difficult because oauth is handled by the server and redirection is used. For this, I think I can study more about the nestjs authguard or work with someone I know well. So my suggestion is to use the session storage method and then apply the new method through backend refactoring. If possible, I'd like to continue with refactoring. What do you think about this? |
|
It’s OK! If it can be implemented using |
|
@devleejb But today, I found that after the oauth login process, we are now sending the token as a query parameter. I think we should send it with HTTP header or body. If you want to talk more about this, can we talk on Discord voice chat? Anytime after 9pm is fine today. |
What this PR does / why we need it:
Store previous page URL before login.
Redirect user back to stored URL after successful login.
Add default redirect to home page if URL doesn't exists.
Until now, Codepair always redirects you to home after login failure. This allows you to redirect to the intended location.
Which issue(s) this PR fixes:
Fixes #308
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes