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

Improve cookie SameSite/secure handling #700

Merged
merged 2 commits into from Mar 22, 2021
Merged

Conversation

ix5
Copy link
Collaborator

@ix5 ix5 commented Feb 3, 2021

Add new samesite option in [server] section to configure SameSite header for cookies.

As a fallback, use local.host to detect URL scheme and set SameSite to None (https) or Lax (http) accordingly.

Set Secure attribute in response header so that cookies will only be sent when requesting content from https://
URLs.

Fixes:

Cookie “isso-[id]” will be soon rejected because it has the “SameSite” attribute set to “None”

(werkzeug sets SameSite to None by default anyway)

NOTE: This change might break quite a few local test setups!

Discussion: #682

See: https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite
and https://werkzeug.palletsprojects.com/en/1.0.x/http/#werkzeug.http.dump_cookie


Sidenote:

  • Setting isso_host_script should somehow be unified instead of duplicated in every function. local.host is only available per-request though, so there cannot be a class variable such as self.isso_host_script.

@ix5 ix5 marked this pull request as ready for review February 4, 2021 08:11
@ix5
Copy link
Collaborator Author

ix5 commented Feb 4, 2021

Will need to write tests for this.

@ix5
Copy link
Collaborator Author

ix5 commented Feb 4, 2021

Tagging @Lucas-C (because of #639), @Konzertheld (because of #558) in case any proxy configurations I haven't thought about ruin the day.

I also hadn't thought too much about what happens when people host isso under a subfolder, e.g. https://mysite.com/isso, but this PR only checks for URL scheme and nothing more.

@ix5
Copy link
Collaborator Author

ix5 commented Feb 4, 2021

Added tests

nosetests --with-doctest --with-coverage --cover-package=isso --cover-html isso/
.................................................................................
Name                        Stmts   Miss  Cover
-----------------------------------------------
isso/__init__.py              164     61    63%
isso/config.py                 77     11    86%
isso/core.py                   68     20    71%
isso/db/__init__.py            66      6    91%
isso/db/comments.py           135     20    85%
isso/db/preferences.py         17      0   100%
isso/db/spam.py                36      0   100%
isso/db/threads.py             15      1    93%
isso/dispatch.py               39     23    41%
isso/ext/__init__.py           10      0   100%
isso/ext/notifications.py     150    100    33%
isso/migrate.py               201     27    87%
isso/run.py                     5      0   100%
isso/utils/__init__.py         75     14    81%
isso/utils/hash.py             67      8    88%
isso/utils/html.py             50      1    98%
isso/utils/http.py             39     25    36%
isso/utils/parse.py            43      2    95%
isso/views/__init__.py         36      8    78%
isso/views/comments.py        468     95    80%
isso/wsgi.py                  103     26    75%
-----------------------------------------------
TOTAL                        1864    448    76%
----------------------------------------------------------------------
Ran 81 tests in 5.199s

OK

Note to self: Still need to set SameSite=Strict for http-only requests.

@ix5 ix5 force-pushed the secure-cookie branch 3 times, most recently from 53ee943 to e721c6c Compare February 5, 2021 08:53
@Lucas-C
Copy link
Contributor

Lucas-C commented Feb 5, 2021

The PR code & tests look good to me. Good job!

However I am a bit worried about "hardcoding" SameSite=None for all HTTPS endpoints:
wouldn't some users want to set it to Strict?

While I agree that this should be the default behaviour,
a suggestion in the discussion at #682 was to make it optionnaly configurable,
and that sounds like a good idea to me.

@ix5
Copy link
Collaborator Author

ix5 commented Feb 5, 2021

I've added a config option as suggested by @Lucas-C and @stefangehn and updated tests.

@ix5 ix5 changed the title utils: Improve cookie SameSite/secure handling Improve cookie SameSite/secure handling Feb 5, 2021
ix5 added 2 commits March 22, 2021 23:13
Add new "samesite" option in [server] section to configure
SameSite header for cookies.

As a fallback, use local.host to detect URL scheme and set
SameSite to "None" (https) or "Lax" (http) accordingly.

Set `Secure` attribute in response header so that cookies
will only be sent when requesting content from `https://`
URLs.

Fixes:
```
Cookie “isso-[id]” will be soon rejected because it has the “SameSite” attribute set to “None”
```

See: https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite
and https://werkzeug.palletsprojects.com/en/1.0.x/http/#werkzeug.http.dump_cookie
Ensure that:
> Set `Secure` attribute in response header so that cookies
> will only be sent when requesting content from `https://`
> URLs.

And also:
> Respect samesite conf item in [server] section.
> As a fallback, use local.host to detect URL scheme and set
> SameSite to "None" (https) or "Lax" (http) accordingly.
@ix5
Copy link
Collaborator Author

ix5 commented Mar 22, 2021

@Lucas-C @blatinier rebased, anything else you'd like to see changed or prevents you from merging?

@blatinier blatinier merged commit c736949 into posativ:master Mar 22, 2021
1 check failed
@ix5 ix5 deleted the secure-cookie branch March 22, 2021 22:25
@ix5
Copy link
Collaborator Author

ix5 commented Mar 22, 2021

Sweet, thanks!
Just a heads up, tests are currently failing due to flakes complaining about unused imports

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

Successfully merging this pull request may close these issues.

None yet

3 participants