Repository: auth0/auth0-spa-js
Pull request #472 (2020-05-19 14:47:08)

Add secure attribute to cookies if served over HTTPS
By submitting a PR to this repository, you agree to the terms within the [Auth0 Code of Conduct](https://github.com/auth0/open-source-template/blob/master/CODE-OF-CONDUCT.md). Please see the [contributing guidelines](https://github.com/auth0/.github/blob/master/CONTRIBUTING.md) for how to create and submit a high-quality PR for this repo.

### Description

This PR adds the `secure` flag to the cookies used by auth0-spa-js, when the web app is served over https. Otherwise, it doesn't change anything.

----
**Old description**
This PR adds `samesite=strict` to the cookies used by auth0-spa-js. This because browsers will reject cookies with `samesite=none` (which is the default) when served over http in the future.

### References

See #459 

### Testing

- [x] This change adds test coverage for new/changed/fixed functionality

### Checklist

- [ ] I have added documentation for new/changed functionality in this PR or in auth0.com/docs
- [x] All active GitHub checks for tests, formatting, and security are passing
- [x] The correct base branch is being used, if not `master`


stevehobbsdev: Thanks for raising @ties-v - let me review and get back to you.
natbusa: please if this solution is correct, approve it, Currently, I have to manually delete the `auth0.is.authenticated` cookie every time I reload my page on vue.js

stevehobbsdev: @natbusa that's strange - why are you having to delete the cookie when your app starts?
natbusa: Hi @stevehobbsdev, I am using the vue.js spa demo provided by oauth0 as a primer for my spa design. After refreshing the page the auth0.is.authenticated cookie is left back. After reloading however oauth0 cookies fail same site check on chrome. I was hoping this PR could help.
stevehobbsdev: @natbusa Thanks. To be clear, nothing is failing here, it's a warning about the cookie possibly being removed in the future. As it stands at the moment your app will still work just fine.

Removing the cookie manually could cause unintended side effects. For example, you might find that your users will suddenly appear logged out when they refresh the page.
stevehobbsdev: @ties-v We've had a look at this and setting `SameSite=strict` will not work for us here. The cookie will not be able to be read on page load - this change affects not only the `auth0.is.authenticated` cookie but the transaction cookies as well. You should have found in your testing that when you try to complete a login flow, you should have got an error complaining about invalid state (because it can't read the cookie).

So we're proposing the following, if you'd like to update the PR:

- On HTTPS origins use `SameSite=None` and `secure=true`
- On non-HTTPS origins, don't set either the `SameSite` or the `Secure` attributes at all

For HTTP origins, this means that new browsers will default to `SameSite=Lax` and older browsers will default to `SameSite=None`, and this should all just work everywhere.

Let me know your thoughts. Unit tests will need to be updated as well when you can find an environment to run them in.
ties-v: @stevehobbsdev Thanks for the clear feedback. It seems that once again something is not as simple as it looks 😉. 
Today I was able to get the test suites up and running on my ubuntu environment, so I am probably good to go. 

The suggested strategy is a good one I think, so I'll implement it somewhere next week. Just to be sure: the proposed cookie flags are applicable to all cookies that the lib uses, right?
ties-v: I've updated the code as discussed. But I was thinking if we could default (explicitly) to `samesite=lax` in all cases, as this is / becomes the default in browsers? Or is this too much a breaking change?
ties-v: Added `sameSite=None`. Had to change the logic a bit because typescript only allows `sameSite='none'` when `secure=true` in object constructor.
stevehobbsdev: Thanks @ties-v - let me review this week!

Patch:
diff --git a/src/storage.ts b/src/storage.ts
index dafb1c3..08b0a4d 100644
--- a/src/storage.ts
+++ b/src/storage.ts
@@ -18,9 +18,15 @@ export const save = (
   value: any,
   options: ClientStorageOptions
 ) => {
-  Cookies.set(key, JSON.stringify(value), {
-    expires: options.daysUntilExpire
-  });
+  let cookieAttributes: Cookies.CookieAttributes = {};
+  if ('https:' === window.location.protocol) {
+    cookieAttributes = {
+      secure: true,
+      sameSite: 'none'
+    };
+  }
+  cookieAttributes.expires = options.daysUntilExpire;
+  Cookies.set(key, JSON.stringify(value), cookieAttributes);
 };
 export const remove = (key: string) => {
   Cookies.remove(key);


Test patch:
diff --git a/__tests__/storage.test.ts b/__tests__/storage.test.ts
index 238a465..384afd0 100644
--- a/__tests__/storage.test.ts
+++ b/__tests__/storage.test.ts
@@ -18,6 +18,25 @@ describe('storage', () => {
       }
     );
   });
+  it('saves object with secure flag and samesite=none when on https', () => {
+    const key = 'key';
+    const value = { some: 'value' };
+    const options = { daysUntilExpire: 1 };
+    const originalLocation = window.location;
+    delete window.location;
+    window.location = { ...originalLocation, protocol: 'https:' };
+    storage.save(key, value, options);
+    expect(require('es-cookie').set).toHaveBeenCalledWith(
+      key,
+      JSON.stringify(value),
+      {
+        expires: options.daysUntilExpire,
+        secure: true,
+        sameSite: 'none'
+      }
+    );
+    window.location = originalLocation;
+  });
   it('returns undefined when there is no object', () => {
     const Cookie = require('es-cookie');
     const key = 'key';