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

Support samesite option in AuthtktAuthenticationPolicy and CookieCSRFStoragePolicy #3319

Merged
merged 14 commits into from
Aug 10, 2018

Conversation

mcdonc
Copy link
Member

@mcdonc mcdonc commented Aug 1, 2018

See #2733

@stevepiercy
Copy link
Member

It would be good to add a mention of what are same site cookies, why a developer would want to use them, and what the setting does in the narrative documentation. Perhaps in Security?

Are we going to support Lax only, or Strict, too?
#2733 (comment)

Finally perhaps linking to the change in WebOb would be useful?
#2733 (comment)

Default: ``'Lax'``. The 'samesite' option of the session cookie. Set
the value to ``None`` to turn off the samesite option.

This option is available as of :app:`Pyramid` 1.10.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the version info down to a `.. versionchanged:: 1.10directive at the bottom of the docstring and document it as "Added thesamesite`` option and made the default ``'Lax'``." similar to below in the csrf storage policy.

CHANGES.rst Outdated
- Modify ``pyramid.authentication.AuthTktAuthenticationPolicy`` and
``pyramid.csrf.CookieCSRFStoragePolicy`` to support the SameSite option on
cookies. See https://github.com/Pylons/pyramid/pull/3319

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document that the default value was changed to Lax.

@mmerickel
Copy link
Member

@stevepiercy

It would be good to add a mention of what are same site cookies, why a developer would want to use them, and what the setting does in the narrative documentation. Perhaps in Security?

This is fine but I don't think it's required for this PR. That has more to do with speculative best practices and hardening like #1845 is requesting.

Are we going to support Lax only, or Strict, too?

lax/strict is not something to "support", it's just something that gets set on the cookie and the browser handles.

Finally perhaps linking to the change in WebOb would be useful?

I don't think that's useful.

@mmerickel mmerickel added this to the 1.10 milestone Aug 2, 2018
@digitalresistor
Copy link
Member

@mcdonc please run the following commands:

git checkout master
git fetch --all
git stash -u
git reset --hard origin/master

So next time you branch from master it doesn't contain all of the merges you've made in the mean time.

@mmerickel mmerickel merged commit 3a89ed3 into master Aug 10, 2018
@AvnerCohen
Copy link

Is this expected to be released any time soon ?

@stevepiercy
Copy link
Member

@AvnerCohen no hard date yet. Progress on the release of Pyramid 1.10 is tracked in its milestone. The four "docs" tagged issues are nearly complete or will be dropped from the milestone. Other issues are in progress. Feedback is sought.

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.

5 participants