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

fix: remove sessionId from URLs #2485

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

Mike-Heneghan
Copy link
Contributor

What:

  • On saving an application remove the feature which stored the sessionId in the url
  • When a user clicks a link to resume a session, read the sessionId but then remove it from the params

Why:

  • Exposing the sessionId can have security implications

- 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
@Mike-Heneghan Mike-Heneghan self-assigned this Nov 27, 2023
@Mike-Heneghan Mike-Heneghan requested a review from a team November 27, 2023 15:56
Copy link

github-actions bot commented Nov 27, 2023

Removed vultr server and associated DNS entries

Copy link
Member

@jessicamcinchak jessicamcinchak left a 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}`);
Copy link
Member

@jessicamcinchak jessicamcinchak Nov 27, 2023

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?

@jessicamcinchak
Copy link
Member

- 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
@Mike-Heneghan Mike-Heneghan merged commit 255204a into main Nov 27, 2023
12 checks passed
DafyddLlyr added a commit that referenced this pull request Dec 4, 2023
DafyddLlyr added a commit that referenced this pull request Dec 5, 2023
@Mike-Heneghan Mike-Heneghan deleted the mh/remove-session-id-from-urls branch December 13, 2023 11:23
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