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

Prevent "invalid token" from being blocking #22459

Merged
merged 9 commits into from
May 14, 2024
Merged

Conversation

br41nslug
Copy link
Member

@br41nslug br41nslug commented May 10, 2024

Scope

What's changed:

  • The verifySessionToken function now throws a "InvalidCredentials" error to be consistent with the rest of the authentication middleware
  • The session cookie will now be deleted if it's found to be invalid allowing a frontend to send you back to the login screen.

Potential Risks / Drawbacks

  • This solution feels like the simplest one, instead of throwing an error and leaving the end user in an error state requiring manual intervention to recover (clearing cookies)

Review Notes / Questions

Copy link

changeset-bot bot commented May 10, 2024

⚠️ No Changeset found

Latest commit: 0ea71f2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@licitdev licitdev left a comment

Choose a reason for hiding this comment

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

Should we add a reason on the login page similar to /admin/login?reason=SESSION_EXPIRED?

image

hanneskuettner

This comment was marked as resolved.

Copy link
Contributor

@hanneskuettner hanneskuettner left a comment

Choose a reason for hiding this comment

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

Nice tests!

LGTM 🎸

api/src/middleware/authenticate.test.ts Outdated Show resolved Hide resolved
api/src/middleware/authenticate.test.ts Outdated Show resolved Hide resolved
api/src/middleware/authenticate.ts Outdated Show resolved Hide resolved
br41nslug and others added 4 commits May 14, 2024 12:57
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
@br41nslug br41nslug merged commit 9a6e236 into main May 14, 2024
4 checks passed
@br41nslug br41nslug deleted the fix-blocking-invalid-cookie branch May 14, 2024 12:01
@github-actions github-actions bot added this to the Next Release milestone May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants