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

Getting SameSite Cookie warnings in Firefox #627

Closed
Jaifroid opened this issue Jun 27, 2020 · 8 comments · Fixed by #635
Closed

Getting SameSite Cookie warnings in Firefox #627

Jaifroid opened this issue Jun 27, 2020 · 8 comments · Fixed by #635
Assignees
Milestone

Comments

@Jaifroid
Copy link
Member

I am getting warnings in Kiwix JS with Firefox that our Cookies have an insecure SameSite attribute that will soon be rejected. There is a link to https://developer.mozilla.org/es/docs/Web/HTTP/Headers/Set-Cookie/SameSite . In fact we don't set the SameSite attribute, but this assumes SameSite=None (for now) and I believe this is what is causing the warning.

For almost all use cases, we have moved from Cookies to localStorage as a result of #613 . This has been merged into master, but not yet released. However, in some rare contexts (documented in #613) the app will still use Cookies.

It seems the fix would be as simple as setting the Cookie with SameSite=Lax or (ideally) SameSite=Strict.

@Jaifroid Jaifroid added this to the v2.8 milestone Jun 27, 2020
@Jaifroid Jaifroid self-assigned this Jun 27, 2020
@Abhirup-99
Copy link
Contributor

Is this issue still up?

@Jaifroid
Copy link
Member Author

Jaifroid commented Jul 7, 2020

@Abhirup-99 Yes it is, but you'd need to read #613 carefully before tackling it, because Cookies are now only used in a few rare cases (mostly IE11 and Edge Legacy from file:// protocol). I guess Firefox was showing that warning due to our migration code from Cookies to LocalStorage.

@mossroy
Copy link
Contributor

mossroy commented Jul 8, 2020

I suppose we can use SameSite=Strict here

@Abhirup-99
Copy link
Contributor

Yes I guess that would help.

@Abhirup-99
Copy link
Contributor

@mossroy @Jaifroid I pushed a pr to fix this isue. But the build failed unexpectedly, but I didn't push any breaking changes. Can you guys take a look?Thanks.

@Jaifroid
Copy link
Member Author

Jaifroid commented Jul 9, 2020

@Abhirup-99 Don't worry, the problem is because Travis can't connect to Sauce Labs due to your not having write access to this Repo. I'll push the same commit to a temporary branch to enable the tests to start running on this one. It's just a quirk of how the tests are set up.

@Abhirup-99
Copy link
Contributor

Thanks for the response. Okay, I was confused how this minor change failed.

@Jaifroid
Copy link
Member Author

Jaifroid commented Jul 9, 2020

It's passed now. Will test on a browser that still uses cookies (most use localStorage).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants