-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: remove sessionId from URLs #2485
Conversation
- Temporarily removing the feature which allows users to resume on browser refresh - The implementation exposed the sessionId which has security implications - Removed the code for the feature and accompanying test
…rom the url - Exposing the sessionId has security implications - The sessionId and the user email are required to successfully resume their session - Read the sessionId but then immediately remove it from the url. - This means it's barely visible and not dispalyed for the rest of the session
Removed vultr server and associated DNS entries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice & simple!
All works as expected for me 🙌
- I can start a session, but my id is not in the URL
- I can save and return successfully via emailed magic link
- I can go away to pay and not be prompted to enter my email address again on redirect back to planx
expect(screen.getByText("Testing 123")).toBeInTheDocument(); | ||
}); | ||
|
||
expect(window.location.href).toContain(`sessionId=${sessionId}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would it perhaps be more meaningful to flip this test to check not.toContain
rather than removing it all together?
- As per: https://github.com/theopensystemslab/planx-new/pull/2485/files/fcc4df227e74eb399d496b3bb14e69a1617f4512#r1406392300 - Reinstate the test as it can add value checking that the sessionId is *not* in the url after being input by user
This reverts commit 255204a.
What:
Why: