search

mozilla / http-observatory

Mozilla HTTP Observatory
https://observatory.mozilla.org/
Mozilla Public License 2.0
1.85k stars 168 forks source link

Support SameSite=None Correctly #437

Closed thecontrarycat closed 3 years ago

thecontrarycat commented 3 years ago

Update the way we check the SameSite attribute of session cookies.

SameSite=None is treated as a valid entry if the cookie is also marked as Secure. SameSite; and SameSite=true; should both be invalid.

Also only grant the bonus score if all session cookies have a valid SameSite attribute (previously this would be awarded if any of the cookies had a valid SameSite attribute).

As discussed in #433

thecontrarycat commented 3 years ago

@april Sorry to bug you on this one, but my company is actually waiting for this as we have a customer that requires us to score a B or higher on Observatory and we can't due this issue. Are you happy with this PR?

april commented 3 years ago

I'm not sure what the count is necessary for? It's still only checking True/False, correct?

thecontrarycat commented 3 years ago

The count is used to ensure that the bonus points are only awarded if all session cookies have a SameSite attribute. Currently, you are awarded bonus points if any of the cookies have the attribute.

april commented 3 years ago

Hmm, why not simply set SameSite to True by default and then if you encounter ones that don't have it set then mark it as False?

Also, this change will need accompanying tests. :)

(thanks for pinging me, this is my last week at Mozilla and I have been swamped)

thecontrarycat commented 3 years ago

Hmm, why not simply set SameSite to True by default and then if you encounter ones that don't have it set then mark it as False?

That's smart. I can change that.

Also, this change will need accompanying tests. :)

If I can work out how to run the tests I'll have a shot at this. I'm not a Python girl though, so any tips you can give would be gratefully received.

(thanks for pinging me, this is my last week at Mozilla and I have been swamped)

Sorry! <3

april commented 3 years ago

No problem, you can run:

nosetests -v httpobs/tests --cover-inclusive -e insert_test_result -e existent -e test_valid_hostname --with-coverage --cover-package=httpobs; flake8 --config .flake8 .

To run all the tests.

The test you'll need to update is:

httpobs/tests/unittests/test_headers.py

Thanks!

thecontrarycat commented 3 years ago

@april Thanks for your help. I've cleaned up the code (fixing a bug in the process) and also added new tests to cover the new behaviour.