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

Set session cookie SameSite attribute to Lax for main site #11721

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 29, 2024

We are now injecting ads via addons,
which doesn't make use of the sustainability endpoint. That endpoint was used to check if the user was a Gold member, and not show ads to them.

The usage of the sustainability API was he only reason to set the SameSite attribute to None.

Using Lax is more secure, as browser will never send the cookie in a cross-site request.


📚 Documentation previews 📚

We are now injecting ads via addons,
which doesn't make use of the sustainability endpoint.
That endpoint was used to check if the user was a Gold member,
and not show ads to them.

The usage of the sustainability API was he only reason to set the
`SameSite` attribute to `None`.

Using Lax is more secure, as browser will never send the cookie
in a cross-site request.
@stsewd stsewd marked this pull request as ready for review October 29, 2024 19:11
@stsewd stsewd requested review from a team as code owners October 29, 2024 19:11
@stsewd stsewd requested a review from ericholscher October 29, 2024 19:11
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

I wonder if we should have any kind of other update about this...

I thought we were using the sustainability API to configure project groups on the RTD docs, but seems like we're getting that data properly:

"publisher": get_publisher(project),

@stsewd
Copy link
Member Author

stsewd commented Oct 29, 2024

I wonder if we should have any kind of other update about this...

Do you mean like other place that depended on this?

@ericholscher
Copy link
Member

I wonder if we should have any kind of other update about this...

Do you mean like other place that depended on this?

No, more like telling users that it's changed. But I think it's probably fine given it's a small number of users.

@ericholscher
Copy link
Member

That said, we can probably remove a bunch of the sustainability API code, since we aren't hitting it anymore, but not necessary in this PR.

@stsewd
Copy link
Member Author

stsewd commented Oct 29, 2024

No, more like telling users that it's changed. But I think it's probably fine given it's a small number of users.

We can probably mention it in the newsletter.

@stsewd stsewd merged commit 4a8f937 into main Oct 29, 2024
9 checks passed
@stsewd stsewd deleted the default-samesite-to-lax branch October 29, 2024 22:40
@humitos
Copy link
Member

humitos commented Oct 30, 2024

Good points were raised here. I think we need to:

  • remove any mention in our docs that Gold membership gives people ad-free docs
  • remove sustainability API code
  • communicate this change in the newsletter

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.

3 participants