Skip to content
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

Redirect to Original URL After Session Expiry and Re-login #6495

Merged
merged 16 commits into from
Nov 6, 2023

Conversation

AshrafMd-1
Copy link
Contributor

@AshrafMd-1 AshrafMd-1 commented Oct 25, 2023

WHAT

🤖 Generated by Copilot at a838ca7

This pull request enhances the login redirection logic to take into account the user's last visited path before session expiry. It uses the local storage to store and retrieve the lastPath value, which is updated by the fireRequest function in src/Redux/fireRequest.tsx and used by the Login component in src/Components/Auth/Login.tsx.

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

HOW

🤖 Generated by Copilot at a838ca7

  • Store the last visited path before session expiry in the local storage (link)
  • Redirect the user to the last visited path after logging in, if it exists, or to the /facility path otherwise (link)

@AshrafMd-1 AshrafMd-1 requested a review from a team October 25, 2023 18:31
@AshrafMd-1 AshrafMd-1 requested a review from a team as a code owner October 25, 2023 18:31
@vercel
Copy link

vercel bot commented Oct 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 5, 2023 7:41am

@netlify
Copy link

netlify bot commented Oct 25, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 13f4233
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/654746f2850d0b0008769005
😎 Deploy Preview https://deploy-preview-6495--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/Components/Auth/Login.tsx Outdated Show resolved Hide resolved
src/Redux/fireRequest.tsx Outdated Show resolved Hide resolved
src/Components/Auth/Login.tsx Fixed Show fixed Hide fixed
src/Components/Auth/Login.tsx Fixed Show fixed Hide fixed
Copy link
Member

@Ashesh3 Ashesh3 left a comment

Choose a reason for hiding this comment

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

Resolve this open redirect security issue:

POC (access while being logged out): https://deploy-preview-6495--care-egov-staging.netlify.app/session-expired?redirect=https://google.com

Ashesh3

This comment was marked as duplicate.

src/Components/Auth/Login.tsx Outdated Show resolved Hide resolved
@AshrafMd-1
Copy link
Contributor Author

AshrafMd-1 commented Oct 26, 2023

now it adds a / in between of the .my.subdomian
do you want me to sanitise the redirect further like remove any . or other symbols in front of the redirect?
@Ashesh3

@Ashesh3
Copy link
Member

Ashesh3 commented Oct 26, 2023

now it adds a / in between of the .my.subdomian do you want me to sanitise the redirect further like remove any . or other symbols in front of the redirect? @Ashesh3

@AshrafMd-1 The POC to redirect to a different domain still works 😅 , It can be exploited to redirect to a different website.

POC: https://deploy-preview-6495--care-egov-staging.netlify.app/session-expired?redirect=https://google.com

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

@AshrafMd-1 you also need to add the redirect param to /session-expired navigate in handleResponse.ts file.

image

@Ashesh3
Copy link
Member

Ashesh3 commented Oct 26, 2023

Try to think of a different way to pass the redirect url across pages/components 🤔 Something like the session expired page can silently pass the redirect url to the login component internally without exposing it through the URL. @rithviknishad any suggestions?

@rithviknishad
Copy link
Member

We could use the custom hook we have useAppHistory (which would inherently be prone from the attack mentioned before)

@rithviknishad
Copy link
Member

The history from useAppHistory contains all previously visited URLs (as long as the page was not refreshed)

@AshrafMd-1
Copy link
Contributor Author

Maybe we can check if the history from useAppHistory matches the site domain and then redirect

src/Components/Auth/Login.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 30, 2023
@github-actions
Copy link

👋 Hi, @AshrafMd-1,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Oct 30, 2023
@nihal467
Copy link
Member

@AshrafMd-1 it is not taking me to the original URL after the session expired

https://www.loom.com/share/00dccbb8d6cb41cf80bf582eb51c6572?sid=4c94d3f8-74d6-474c-9e3e-7b59204be434

CC : @Ashesh3 @rithviknishad

@AshrafMd-1
Copy link
Contributor Author

@nihal467 it works now
The issue was with merging this PR with develop

@nihal467
Copy link
Member

nihal467 commented Nov 6, 2023

LGTM

@khavinshankar khavinshankar merged commit e30d6bd into coronasafe:develop Nov 6, 2023
33 of 35 checks passed
@AshrafMd-1 AshrafMd-1 deleted the Fix#6486 branch November 9, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect to Original URL After Session Expiry and Re-login
5 participants