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: Don't store JWT token in an SSR environment #9848

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

devcshort
Copy link

@devcshort devcshort commented Oct 28, 2024

What - This change makes it so that JWT doesn't get stored on the Medusa client when in an SSR environment.

Why - Currently when the Medusa client is used in an SSR environment, the last logged in user's JWT gets stored on the Medusa client. This causes customer information to be leaked when it shouldn't be.

How - To keep things DRY, I added a typeof check to the jwt-token-manager that checks if the window is undefined. If it's undefined, I'm simply just returning as I don't believe any further action is needed.

This fixes the issue referenced here #6889

@devcshort devcshort requested a review from a team as a code owner October 28, 2024 21:11
Copy link

changeset-bot bot commented Oct 28, 2024

⚠️ No Changeset found

Latest commit: eb74321

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

vercel bot commented Oct 28, 2024

@devcshort is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

@u11d-bartlomiej-galezowski

@sradevski @olivermrbl @shahednasser can we ask for your review? This fix is really urgent for us.

@thetutlage
Copy link
Contributor

Looks fine to me. I will let @olivermrbl and @sradevski look into it once and especially the failing CI tests

@devcshort
Copy link
Author

@olivermrbl @sradevski any updates on this? This is a fairly urgent request for us and is a prerequisite before we can go to production. Thanks!

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link

vercel bot commented Dec 9, 2024

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

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Dec 9, 2024 8:55am

@u11d-bartlomiej-galezowski

Can we expect the fix in v1.20.11?

Copy link
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jan 11, 2025
@devcshort
Copy link
Author

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Not stale

@olivermrbl olivermrbl merged commit ee5af4d into medusajs:v1.x Jan 17, 2025
18 of 26 checks passed
@devcshort
Copy link
Author

@olivermrbl @kasperkristensen what is your release cycle like? Curious when we can expect this change for the v1 Medusajs client to be released. Thanks!

@SalahAdDin
Copy link

@olivermrbl @kasperkristensen, what is your release cycle like? I'm curious when we can expect this change for the v1 Medusajs client to be released. Thanks!

Being a security issue, they may release it to version 1 too.

@olivermrbl
Copy link
Contributor

@devcshort, there is no release cycle for v1, since PRs are very rarely merged. We tend to do it shortly after this happens, so I expect to cut a new version later today.

@devcshort
Copy link
Author

@olivermrbl sounds great, thank you for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants