accounts-js / accounts

Fullstack authentication and accounts-management for Javascript.
https://www.accountsjs.com/
MIT License
1.5k stars 141 forks source link

Support cookie based storage for JWT's to avoid XSS #92

Open ElectricCookie opened 7 years ago

ElectricCookie commented 7 years ago

This library is using the localStorage to store the JWT. This makes it accessible to the JS-Enviroment and therefore vulnerable to XSS-Attacks. A good alternative, that prevents this flaw is to use a cookie that is httpOnly.


Ressources on this problem:

https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage

Cookies, when used with the HttpOnly cookie flag, are not accessible through JavaScript, and are immune to XSS.


https://www.owasp.org/index.php/HTML5_Security_Cheat_Sheet

"Do not store session identifiers in local storage as the data is always accesible by JavaScript. Cookies can mitigate this risk using the httpOnly flag."

To make the claims available to the client, they should be sent seperately, but the whole JWT should not be available to the JS-Enviroment

pradel commented 7 years ago

It's true but modern framework like angular and react auto sanitize the components rendered and prevent the xss attack.

ElectricCookie commented 7 years ago

But an infected npm package or something similar would simply bypass any framework...

Especially since everything requires like 1000 dependencies, how easy would it be to implement some infected code? Not hard if you ask me.

nickredmark commented 7 years ago

Maybe instead you could use the same strategy as ooth client, i.e. use JWTs only to create a cookie-based session. The JWT then is discarded immediately after it's used, making XSS harder.

pradel commented 7 years ago

@nmaro we are currently thinking about adding cookie sessions support and use jwt only to obtain that cookie yeah

davidyaha commented 7 years ago

@pradel @ElectricCookie @nmaro Currently the only problem with cookies is using this package on react-native apps. The RN runtime does not have the concept of cookies natively and while there is a package that implements that, it is quite unstable. I save session tokens on the AsyncStorage which has basically the same API as localStorage but async instead of sync.

As react native runtime is quite different than the browser in terms of it cannot just load and run html or js files from the network (except from webview which has it's own sandbox), I think the security flaw is much less apparent over there (at least for non jailbroken devices).

We need to support both strategies IMO.

Also @nmaro, with ooth you set out to implement a good authorization microservice for a microservice inspired system right? Then the JWT granted by ooth should be exchanged with a session cookie on the request to the resource microservice? This means that every resource service needs it's own cookie strategy added? (not a big deal of course but still a concern)

If I am mistaken by my assumptions, why respond with JWT in the first place?

davidyaha commented 7 years ago

For reference adding an old post written by MDG on that subject. Particularly the last part is interesting as it describes a possible enhancment to storing access token in the localStorage.

davidyaha commented 7 years ago

Also to be fair, a counter post

davidyaha commented 7 years ago

Maybe part of our problem is the lack of browser-policy package

nickredmark commented 7 years ago

Hi @davidyaha to quickly answer your question: yes, you are correct. If you use ooth as an independent microservice, every resource service needs its own cookie strategy. The relevant example would be graphql-api-with-auth. I also extended ooth to be usable within an express-based API (i.e. not as an external microservice, see graphql-api-with-ooth). In that case no JWT is needed at all. It's a matter of tradeoffs.

davidyaha commented 7 years ago

Yeah I understand completely. Thanks for your answer. I hope to get on top of this issue in the next couple of weeks. It's really a matter we cannot decide on easily as we want to support as many environments as we can.

Our main focus has always been to have a truly reusable package. By now it seems to be working as @dotansimha and I have used for two projects by now, and we want to use it for another client at least in next month or so.

veeramarni commented 7 years ago

@davidyaha is it fixed?

davidyaha commented 7 years ago

Hey @veeramarni, We currently have no built in support for cookies to avoid XSS attacks.

The solution we would seek IMO would be to be able to turn off storage of JWT's in the localStorage and prevent sending to the client. Then get a resume session cookie sent to the client with httpOnly and secure flags on for any re-login purposes. Server side should have support for both bearer and cookie headers and should have the option to turn off each of them.

At the moment our projects does not require this. We would love to merge a PR if you'd like to tackle this one.

Thanks ahead!

Aetherall commented 7 years ago

In the case in which the tokens are set in the cookies, is there a reason for sending only the access token ? Where would you store the refresh token otherwise ?

https://github.com/js-accounts/rest/issues/27

davidyaha commented 7 years ago

@Aetherall No you are probably right. The only thing I would as I've written here before, is a way to turn on and off cookie storage and localStorage. This way we can support all kinds of use cases. One consideration for example are native mobile apps that usually does not use cookies.

Do you think you have enough information to draft a solution? I'd love to get this ball rolling. If you do, try branching out from our TS PR.

Thanks!

Aetherall commented 7 years ago

Alright, I'll write here if I need to !

darkbasic commented 7 years ago

I'm not going to talk about security right now, I prefer to do so in the community meeting. What I would like to talk about right now is: how do you plan to deal with server side rendering if you don't use cookies at all? How will the server recognize you without further interaction? Unfortunately cookies are the only way.

davidyaha commented 7 years ago

You are right! This is actually quite easy to add on top of the current js-accounts implementation but should probably be part of this package as well. Excited to meet you @darkbasic and hope to see more people that have previously shown interest or opened issues on this project!

darkbasic commented 7 years ago

My pleasure @davidyaha, I'm sure it will be a great talk.

agustif commented 4 years ago

I know we managed to swap LocalStorage with cookies when trying to do the nextJS example, maybe this is possible now (to support cookies) and should be a separate package so one can choose? I love the modularity of the project. and cookies might be out of vogue, but are still pretty much useful in some use-cases.

bmitchinson commented 4 years ago

https://www.accountsjs.com/docs/api/express-session/index/#accountsexpress-session

Does using express-session middlewear end up accommodating this? Wrapping this around GraphQL should enable the use of headers / GQL context instead of the JWT on the client side right?

Any advice appreciated, finding my around this lib. thanks!

agustif commented 4 years ago

https://www.accountsjs.com/docs/api/express-session/index/#accountsexpress-session

Does using express-session middlewear end up accommodating this? Wrapping this around GraphQL should enable the use of headers / GQL context instead of the JWT on the client side right?

Any advice appreciated, finding my around this lib. thanks!

I've accounts working on nextjs, could help you with it.

I use cookies instead of localstorage, same JWT.


import { AccountsClient } from '@accounts/client';
import { AccountsClientPassword } from '@accounts/client-password';
import GraphQLClient from '@accounts/graphql-client';
import { accountsLink } from '@accounts/apollo-link';
import { ApolloClient, InMemoryCache, HttpLink, from } from '@apollo/client';
import gql from 'graphql-tag';
// import cookies from 'next-cookies';
import { CookieStorage } from 'cookie-storage';

import { useMemo } from 'react'
// import { ApolloClient, InMemoryCache, HttpLink, from } from '@apollo/client';
import { concatPagination } from '@apollo/client/utilities'
// import { accountsClient, apolloClient } from '../utils/accounts'
// import { accountsLink } from '@accounts/apollo-link';

const cookieStorage = new CookieStorage();
// This auth link will inject the token in the headers on every request you make using apollo client
const authLink = accountsLink(() => accountsClient);

const httpLink = new HttpLink({
  uri: 'http://localhost:4000/graphql',
  credentials: 'same-origin',

});

let apolloClient = new ApolloClient({
    ssrMode: typeof window === 'undefined',
        link: from([authLink, httpLink]),
    cache: new InMemoryCache({
      typePolicies: {
        Query: {
          fields: {
            allPosts: concatPagination(),
            myDocuments: concatPagination(),
          },
        },
      },
    }),
  })

const accountsGraphQL = new GraphQLClient({
  graphQLClient: apolloClient,
  userFieldsFragment: gql`
    fragment userFields on User {
      id
      emails {
        address
        verified
      }

      documents {
        id
        title
      }
    }
  `,
});
const accountsClient = new AccountsClient(  {
  // We tell the accounts-js client to use cookieStorage to store the tokens
  tokenStorage: cookieStorage,
}, accountsGraphQL);
const accountsPassword = new AccountsClientPassword(accountsClient);

export { accountsClient, accountsGraphQL, accountsPassword, apolloClient };

function createApolloClient() {
  const httpLink = new HttpLink({
        uri: 'http://localhost:4000/graphql',
        credentials: 'same-origin',
    });
    const authLink = accountsLink(() => accountsClient);
  return new ApolloClient({
    ssrMode: typeof window === 'undefined',
        link: from([authLink, httpLink]),
    cache: new InMemoryCache({
      typePolicies: {
        Query: {
          fields: {
            allPosts: concatPagination(),
            myDocuments: concatPagination(),
          },
        },
      },
    }),
  })
}

export function initializeApollo(initialState = null) {
  const _apolloClient = apolloClient ?? createApolloClient()

  // If your page has Next.js data fetching methods that use Apollo Client, the initial state
  // gets hydrated here
  if (initialState) {
    // Get existing cache, loaded during client side data fetching
    const existingCache = _apolloClient.extract()
    // Restore the cache using the data passed from getStaticProps/getServerSideProps
    // combined with the existing cached data
    _apolloClient.cache.restore({ ...existingCache, ...initialState })
  }
  // For SSG and SSR always create a new Apollo Client
  if (typeof window === 'undefined') return _apolloClient
  // Create the Apollo Client once in the client
  if (!apolloClient) apolloClient = _apolloClient

  return _apolloClient
}

export function useApollo(initialState) {
  const store = useMemo(() => initializeApollo(initialState), [initialState])
  return store
}```
PS: You can also use @accounts/apollo-link to tie it up in the frontend!
bmitchinson commented 4 years ago

I've accounts working on nextjs, could help you with it.

I use cookies instead of localstorage, same JWT.

import { AccountsClient } from '@accounts/client';
import { AccountsClientPassword } from '@accounts/client-password';
...

Thank you so much for this! I'll keep you posted. Would love to contribute to official documentation with some of my findings, and that should be possible with your help so, thank you. @agustif

bmitchinson commented 4 years ago

@agustif / @pradel any ideas on how could I deliver a token / in the form of a cookie to the client? Instead of being included in a response body. I'm at the point where although this client side configuration is helpful, I need to know how to implement the server to send a cookie.

bmitchinson commented 4 years ago

I'd make a new issue except I think the discussions above are relevant to what I'm trying to accomplish. It seems like this issue is to make what I'd like to achieve a first class module of accounts-js

stolinski commented 3 years ago

I've been doing quite a bit of reading on this and it seems like JWTs in local storage are a big no-no. I know this convo has been going on a while, but it seems like this should be a pretty urgent problem no?

agustif commented 3 years ago

@agustif / @pradel any ideas on how could I deliver a token / in the form of a cookie to the client? Instead of being included in a response body. I'm at the point where although this client side configuration is helpful, I need to know how to implement the server to send a cookie.

Hey sorry @bmitchinson but I don't know what you mean, can't you read the cookie req.header on getServerSideProps and use accountsClient(validateToken) to decide if render a user auth based view on the server or not?

I would love to help provide more support for accountsjs+nextjs, we maybe could talk over the slack group for Accounts, I've same username there

edw19 commented 3 years ago

@agustif Hi how are things ? I have tested your configuration with cookie-storage and when using getServerSideProps with await accountsClient.getUser () it does not get me the user, I have no error and when it renders on the client using this same method it gets me the user who is logged in. Do you know why this can be?

agustif commented 3 years ago

@agustif Hi how are things ? I have tested your configuration with cookie-storage and when using getServerSideProps with await accountsClient.getUser () it does not get me the user, I have no error and when it renders on the client using this same method it gets me the user who is logged in. Do you know why this can be?

In serverSideProps you should be able to get the cookie from the ctx.req.headers me thinks! accountsClient doesn't work on the server sadly, so you'd have to read the cookie from the req headers and maybe do something with it like validate it etc? idk moved on without using it further can't help much more Im sorry hope you can get it working!

edw19 commented 3 years ago

ohh I understand, of course with ctx.req the cookie is read correctly thanks