becem-gharbi / nuxt-directus

Unofficial Directus client for Nuxt
MIT License
44 stars 9 forks source link

SECURITY / DISCUSSION: use HttpOnly cookie for accessToken #49

Closed Dominic-Marcelino closed 1 year ago

Dominic-Marcelino commented 1 year ago

Currently the module stores the accessToken in a httpOnly: false,. In order to secure the token I'd suggest to store it in a more secure httpOnly cookie, so that it's secured against XSS attacks.

https://github.com/becem-gharbi/nuxt-directus/blob/5bb18b82262d785968de0aa8e6b629b7245cb611/src/runtime/composables/useDirectusSession.ts#L37-L49

As the cookie than can't be read anymore from the JS, the auth-plugin as well as the useDirectusSession composable needs to be refactored:

  1. The access token needs to be passed via cookie (instead of a bearer-header right now)
  2. The expiracy-time needs to be stored separately (e.g via useState())
  3. Some other things I might have missed in this first checkup

Looking forwards to your thoughts on this :)

becem-gharbi commented 1 year ago

Hi, thanks for noting this concern,

The security of access token will always be in question. I believe that with limited age and proper code check it can be protected against XSS attacks.

  1. The access token is a JWT so it's common to be sent via authorization header. That's how directus accepts it https://docs.directus.io/reference/authentication.html#access-tokens.
  2. Stroring the access token and its expiration on two different contexts (cookie and memory) will lead to inconsistencies. For example, when multiple windows opened (same cookie but different expiration).
Dominic-Marcelino commented 1 year ago

Thanks for your quick response! I totally do get why you've decided for the current implementation.

Nevertheless I think access tokens should never be in unsecured cookies as this is a huge security risk (even the directus-app itself had an vulnerability that allowed XSS attacks in the past)

Using a scoped memory storage like the SDK does could be a solution:

Not 100% sure though how this could be implemented in nuxt for SSR

becem-gharbi commented 1 year ago

Hi, access token should be stored on a shared context (localStorage, cookie...) given that the refresh token is (cookie). By opening new window (document), you're effectively revoking all the others. This is because an access token is related to a single refresh token.

Edit I'm wrong about this. Yes it's feasible to store access token in memory. On ssr useState can be used to pass access token. The only drawback is that it needs to be refreshed on every page load.

Edit I have created token-inmemory branch. It works as you needed to be. I'm not sure if this is better. What do you think? also @vanling how you see this change.

vanling commented 1 year ago

I believe the in-memory approach aligns well with how Directus handles authentication in their app. I'm fine with storing the access token in memory and refreshing it on each page load, as long as the refreshToken flow is robust and auto-refreshes reliably. I've successfully implemented this approach using useState with sidebase/nuxt-auth before.

In my setup, I have:

Each tab can request a new access token while updating the shared refresh_token in the cookie.

Note: @becem-gharbi, I've experienced random logouts on the current production version when using multiple tabs. I'm still investigating the cause. Also auto refreshing when doing an api call when token expired does not always work, again not sure if this is caused by module or my code

Will checkout the in memory branch.

Dominic-Marcelino commented 1 year ago

Edit I'm wrong about this. Yes it's feasible to store access token in memory. On ssr useState can be used to pass access token. The only drawback is that it needs to be refreshed on every page load.

Edit I have created token-inmemory branch. It works as you needed to be. I'm not sure if this is better. What do you think? also @vanling how you see this change.

This would be an SSR friendly solution but tbh I don't think that this would make any difference as useState stores it's data in the window object --> same situation as before.

I'm currently thinking if it wouldn't be best of both by using the current cookie approach on ssr and trying to use the SDK default access token management on csr. 🤔

becem-gharbi commented 1 year ago

This would be an SSR friendly solution but tbh I don't think that this would make any difference as useState stores it's data in the window object --> same situation as before.

I wonder where to find this info, because I know that useState is a Vue composable so it can only be called on its lifecycle. Therefore it cannot be part of the global scope.

Dominic-Marcelino commented 1 year ago

UseState is provided by Nuxt and is globally available as mentioned in the Nuxt Docs:

useState is an SSR-friendly ref replacement. Its value will be preserved after server-side rendering (during client-side hydration) and shared across all components using a unique key.

You can easily check this on your token-inmemory branch by checking the window object. Afaik this behaviour is the same for ssr and csr: Bildschirmfoto 2023-09-01 um 09 41 04

becem-gharbi commented 1 year ago

Thanks very much for mentioning this, I didn't know that.

becem-gharbi commented 1 year ago

I'm closing this issue for now, If you have any working alternative solution please let me know.