-
-
Notifications
You must be signed in to change notification settings - Fork 171
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: authState getting wiped on page reload on static websites #785
Open
Vijayabhaskar96
wants to merge
29
commits into
sidebase:main
Choose a base branch
from
Vijayabhaskar96:update-pull-request
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
6dd86b2
set and reload data, rawToken from cookie
Vijayabhaskar96 8a36118
fix lint
Vijayabhaskar96 9b4b8b1
fix typecheck
Vijayabhaskar96 505c583
Merge branch 'main' into main
zoey-kaiser 534557a
Merge branch 'main' into main
phoenix-ru d371950
set and reload data, rawToken from cookie
Vijayabhaskar96 5780a45
fix undefined sessionCookie case
Vijayabhaskar96 38cb342
Merge remote-tracking branch 'origin/main' into update-pull-request
Vijayabhaskar96 600d69c
Merge branch 'main' into update-pull-request
Vijayabhaskar96 d8f0461
fix lint
Vijayabhaskar96 d814048
fix undefined cases
Vijayabhaskar96 a43147c
Merge branch 'main' into update-pull-request
zoey-kaiser 6d13eb4
Merge branch 'main' into update-pull-request
zoey-kaiser fb7a26a
Merge branch 'main' into update-pull-request
zoey-kaiser e1a122e
Merge branch 'main' into update-pull-request
zoey-kaiser 0060358
Merge branch 'main' into update-pull-request
zoey-kaiser 25ee10e
Merge branch 'main' into update-pull-request
zoey-kaiser e64a195
Merge branch 'main' into update-pull-request
zoey-kaiser b0ad984
fix: linting in useAuth
zoey-kaiser 882061f
fix: lint in plugin
zoey-kaiser 907ff2e
fix: trailing spaces
zoey-kaiser f812357
organize code
Vijayabhaskar96 e7163cc
Merge branch 'main' into update-pull-request
Vijayabhaskar96 86feff3
fix typo
Vijayabhaskar96 75d167e
Merge branch 'update-pull-request' of https://github.com/Vijayabhaskaβ¦
Vijayabhaskar96 0f7e51a
Merge branch 'main' into update-pull-request
Vijayabhaskar96 7d04329
Merge branch 'sidebase:main' into update-pull-request
Vijayabhaskar96 2be6b25
Merge branch 'main' into update-pull-request
phoenix-ru 44d1291
Merge branch 'main' into update-pull-request
phoenix-ru File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -629,3 +629,14 @@ export interface ModuleOptionsNormalized extends ModuleOptions { | |
globalAppMiddleware: NonNullable<ModuleOptions['globalAppMiddleware']> | ||
originEnvKey: string | ||
} | ||
export interface SessionCookie { | ||
lastRefreshedAt?: SessionLastRefreshedAt | ||
data?: SessionDataObject | ||
} | ||
|
||
// Augment types | ||
declare module 'nuxt/schema' { | ||
interface PublicRuntimeConfig { | ||
auth: ModuleOptionsNormalized | ||
} | ||
} | ||
Comment on lines
+637
to
+642
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question as to why was this added? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I don't remember, but I guess it was causing merge conflicts. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Storing user data in the cookie may lead to a GDPR violation if the
getSession
endpoint returns any personal information (which is often the case). It undoubtedly spawns the "protecting personal data" requirement for anyone using the module, even if they weren't affected by the SSG issue in the first place.https://gdpr.eu/cookies/
I don't find it very attractive to introduce such a burden to NuxtAuth :/
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.
Maybe introducing an option to use SessionStorage nor LocalStorage would be great instead of using cookies ?
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.
If an attacker gains access to a token stored in a Cookie, Session, or LocalStorage, the primary concern is not just identifying the user, but also the ability to access personal data and perform actions the user is authorized to do. This presents a clear security risk.
From a GDPR perspective, the critical question is whether the token can be used to track the user or if it contains personal data. Regardless of where the token is stored, if it can uniquely identify a user or track their actions, it falls under GDPR regulations.
For example, JWTs often store a user identifier in the payload. Since JWTs are not opaque and can be decoded, they may be used to directly identify a user, making them subject to GDPR. Even if the token does not explicitly store "personal data" like an email address, it qualifies as personal data if it can uniquely identify a user (e.g., through a user ID).
In this case, you must inform users via a cookie notice or a similar consent mechanism, ensuring transparency about the use of the token and obtaining their consent where required. However, if the cookies are strictly necessary for the operation of the web application (e.g., to keep users logged in or enable certain features), explicit consent may not be required under GDPR. This generally applies to authentication cookies, afaik.
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.
@thorge You are mostly right. However, auth cookies/jwts don't need to store emails or other identifying information. A website can use "shallow" JWTs storing only user id in it (this is a valid
Access Token
in NuxtAuth terms). As to whether an ID is "personal data" is very subjective.However if a website uses such a "shallow" JWT (
Access Token
) and makes a call to agetSession
endpoint to check its validity + get user data, then it's a different thing. For example, internally we optimize such a call to also return user email, name, permissions, etc. on top of a regular JWT validity check. I would prefer to not store this information in a cookie.Please note that I am also not clearly understanding the usecase of static websites fully. If there is authentication, what is the issue with the session being refetched on page reload?
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.
@phoenix-ru Thereβs actually no need to store the session itself; we only need to store the access token. Thatβs what I thought this was about, but I must admit I didnβt examine the changes in detail. Sorry for my tldr answer above :-)
As @Geekimo and @Vijayabhaskar96 suggested, Iβd prefer a solution where storing the access token in a cookie (or alternatively in SessionStorage/LocalStorage) is optional. If the session data isnβt available after a client page reload, the access token could be used to retrieve the session data as needed.