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

[Breaking] Samesite Cookie Fix #8269

Merged
merged 4 commits into from
Oct 10, 2024

Conversation

SchrodingersGat
Copy link
Member

@SchrodingersGat SchrodingersGat commented Oct 10, 2024

A previous PR which was well-intentioned to solve this issue has resulted in problems with session authentication.

PR #8262 changed the default value of CSRF_COOKIE_SAMESITE and SESSION_COOKIE_SAMESITE from None (boolean value) to 'None' (string value). This is in accordance with the django documentation which specifies that these values must be strings and not None:

image

However, note that the boolean value False is allowed, which disables the cookie checks entirely.

Our previous literal value None (disallowed) was resulting a boolean check to evaluate the same as if the value was set to False (allowed):

>>> not 'None'
False
>>> not None
True
>>> not False
True

This means that (prior to this patch), the samesite cookie session was effectively set to False (disabled), and not 'None' - which has different implications in operation. However, this value was causing a downstream error in the allauth package.

Changes

So, this PR makes the following adjustments:

  • Cookie-Samesite options are stringified appropriately (or, set to the False boolean literal)
  • The default value is changed from 'None' to False
  • Value is forced to False in DEBUG mode (to aid in easy development)
  • Update the documentation (to make this whole thing a bit clearer)

Breaking

This is considered a "breaking" change because any production installation which has the samesite set to None will now evaluate to 'None' instead of False - resulting in different behaviour.

Users in this predicament should change the value of INVENTREE_COOKIE_SAMESITE from None to False to disable the checks.

References

- In DEBUG mode, turn off entirely
- Allow False value (note: *not* a string)
- Force insecure cookie in DEBUG mode
@SchrodingersGat SchrodingersGat added bug Identifies a bug which needs to be addressed setup Relates to the InvenTree setup / installation process breaking Indicates a major update or change which breaks compatibility labels Oct 10, 2024
@SchrodingersGat SchrodingersGat added this to the 0.17.0 milestone Oct 10, 2024
Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for inventree-web-pui-preview canceled.

Name Link
🔨 Latest commit 9d9ffee
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/6707979bc0fba70008a62269

Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.22%. Comparing base (8e34fdd) to head (9d9ffee).
Report is 313 commits behind head on master.

Files with missing lines Patch % Lines
src/backend/InvenTree/InvenTree/settings.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8269      +/-   ##
==========================================
- Coverage   84.22%   84.22%   -0.01%     
==========================================
  Files        1162     1162              
  Lines       52517    52519       +2     
  Branches     1894     1894              
==========================================
  Hits        44235    44235              
- Misses       7812     7814       +2     
  Partials      470      470              
Flag Coverage Δ
backend 85.97% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SchrodingersGat SchrodingersGat merged commit 914f59c into inventree:master Oct 10, 2024
22 checks passed
@SchrodingersGat SchrodingersGat added backport Apply this label to a PR to enable auto-backport action backport-to-0.16.x labels Oct 10, 2024
Copy link
Contributor

💔 All backports failed

Status Branch Result
0.16.x Backport failed because of merge conflicts

You might need to backport the following PRs to 0.16.x:
- Samesite cookie fix (#8262)

Manual backport

To create the backport manually run:

backport --pr 8269

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@SchrodingersGat SchrodingersGat removed backport Apply this label to a PR to enable auto-backport action backport-to-0.16.x labels Oct 10, 2024
@SchrodingersGat SchrodingersGat deleted the samesite-fixx branch October 10, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Indicates a major update or change which breaks compatibility bug Identifies a bug which needs to be addressed setup Relates to the InvenTree setup / installation process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant