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

💡 [REQUEST] - Allow saving token to memory #197

Open
brumik opened this issue Oct 24, 2024 · 4 comments
Open

💡 [REQUEST] - Allow saving token to memory #197

brumik opened this issue Oct 24, 2024 · 4 comments
Labels
question Further information is requested

Comments

@brumik
Copy link

brumik commented Oct 24, 2024

Summary

I want to be able to save the token to memory instead of local storage or session storage. This allows for higher security (with rolling refresh token).

As a sideeffect prevents issues like:

  • Have old token in storage
  • Open the app again after expiration date
  • Some request are fired earlier than token is changed which fail (and if you do not have them in use effect with token deps it won't repeat)

(this is an issue when the lib is used outside of the given react app or using the token in global config like RTK base auth logic) - Edit: I know you do not want to support this officially, but keeping it here as an additional context for later if somebody has the same issue as me.

Basic Example

const authConfig: TAuthConfig = {
  //...
  storage: 'session',
  storageAccessToken: 'memory',
  storageRefreshToken: 'session',
};
// or
const authConfig: TAuthConfig = {
  //...
  storage: {
    token: 'memory',
    refreshToken: 'session',
    default: 'local',
  },
};

Drawbacks

Added complexity on imlementation and documentation

Unresolved questions

No response

Implementation PR

x

Reference Issues

No response

@brumik brumik added the question Further information is requested label Oct 24, 2024
@sebastianvitterso
Copy link
Collaborator

sebastianvitterso commented Oct 25, 2024

Hi, and thanks for engaging in the library!

Edit: I know you do not want to support this officially, but keeping it here as an additional context for later if somebody has the same issue as me.

I think we should try to understand the issue before deciding that we don't want to support something 😅


From what I understand, you're proposing saving the token state simply in JavaScript memory (using something like a useState for example?). The rationale is that safety-wise it is better. I assume by this you're thinking of how easy it could be for a potential attacker (think XSS etc.) to access the token if it's stored in e.g. SessionStorage, as they can just do sessionStorage.getItem("ROCP_token").

And this is a fair point.

However, since this is all happening on the client, if we assume that we must "withstand" an XSS-attack, then we must also assume that the attacker can inject basically whatever code they want. And so we must also assume that there are ways to get a hold of this in-memory value as well (since it's stored in JS, which is the attack surface of the XSS-attacker).

I think this sounds like security by obscurity, but by all means - security by obscurity is still a form of security, it's just not waterproof. I do think that in most cases, the difference in complexity of retrieving the token from SessionStorage and from a JS state would be dramatic, and it could be the difference that makes the attacker opt to "attack someone else".

Finally, I'm not yet sure if this is something we want to support, mainly because this only solves a problem that arises when the client is already compromised (e.g. by XSS), and at that point, we sort of just consider the battle lost with regards to security.


From a practical standpoint, I think that storing the token outside (partially) persistent memory would require the user to login whenever they refreshed the page.

Unless (!) you store e.g. the refresh token in persistant memory (Session-/LocalStorage). And to my understanding, even if you only store the refresh token in there, whatever issues you've solved by storing the access token in memory will be outshined as anyone with a valid refresh token can just request a valid access token.


If you feel like I've missed something, @brumik, please add more context!

Leaving this open until we've concluded 😄

@brumik
Copy link
Author

brumik commented Oct 25, 2024

Hey,

Thank you for your fast reply.

Yes, you understood it correctly. And yes, the refresh token would still be in session storage.

I am not an expert on security, but working with health data our security team requires us to have the accessToken in memory and refreshToken can be in session storage or local storage but it has to be "rolling" (eg. You get a new one after using it).

I see your point that when a user is compromised it is not really making a difference, but I have to adhere and trust our security team in this regard.

P.S.: Ideal state is to use cookies but PKCE with the above setup should be satisfactory for everyone who cannot use http only cookies.

Implementation wise either a react state or a module level global variable would be good. Neither of them would break any existing API in the context as far as I can see.

@soofstad
Copy link
Owner

soofstad commented Oct 25, 2024

Allowing the refresh_token to be stored in session/local storage, but not the access_token is nonsensical. If anything, it should be opposite.
From my perspective right now, this is not a security enhancement, and adding this feature only to satisfy your specific security department is not something we should do. That kind of development will only lead to bloat.

Please, if there are any other arguments for this feature I would be must interested in hearing them 🙂

@sebastianvitterso
Copy link
Collaborator

Feel free to forward this thread to someone on your security team, in case they want to understand why we are reluctant to implement this.

In short, our conclusion is based on our belief that such functionality would not offer any actual improvement when it comes to security, only a degredation in functionality, since one would have to wait for a new access token upon refreshing the page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants