💡 [REQUEST] - Allow saving `token` to memory

brumik commented 2 days ago

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

sebastianvitterso commented 2 days ago

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 commented 2 days ago

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 commented 1 day ago

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 :slightly_smiling_face:

sebastianvitterso commented 1 day ago

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.

soofstad / react-oauth2-pkce