Skip to content

Cookies: set samesite: Lax by default #8304

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

Merged
merged 7 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions docs/server-side-search.rst
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,14 @@ and then click on :guilabel:`Search Analytics`.
API
---

Search is exposed through our API that's proxied from the domain where your docs are being served.
This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project, for example.
If you are using :doc:`/commercial/index` you will need to replace
https://readthedocs.org/ with https://readthedocs.com/ in all the URLs used in the following examples.
Copy link
Member

Choose a reason for hiding this comment

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

This should be under it's own section, probably Read the Docs for Business Users or similar

Copy link
Member Author

Choose a reason for hiding this comment

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

This is describing the API and examples, not sure if I follow about having another section

Copy link
Member

@ericholscher ericholscher Aug 9, 2021

Choose a reason for hiding this comment

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

Like, we need a section of the docs explaining the 2 different sites and endpoints (and auth, etc.), and then probably link to it from here for more detail.


.. warning::

This API isn't stable yet, some small things may change in the future.

.. http:get:: /_/api/v2/search/
.. http:get:: /api/v2/search/

Return a list of search results for a project,
including results from its :doc:`/subprojects`.
Expand Down Expand Up @@ -120,12 +120,12 @@ This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project

.. code-tab:: bash

$ curl "https://docs.readthedocs.io/_/api/v2/search/?project=docs&version=latest&q=server%20side%20search"
$ curl "https://readthedocs.org/api/v2/search/?project=docs&version=latest&q=server%20side%20search"

.. code-tab:: python

import requests
URL = 'https://docs.readthedocs.io/_/api/v2/search/'
URL = 'https://readthedocs.org/api/v2/search/'
params = {
'q': 'server side search',
'project': 'docs',
Expand All @@ -140,7 +140,7 @@ This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project

{
"count": 41,
"next": "https://docs.readthedocs.io/api/v2/search/?page=2&project=read-the-docs&q=server+side+search&version=latest",
"next": "https://readthedocs.org/api/v2/search/?page=2&project=read-the-docs&q=server+side+search&version=latest",
"previous": null,
"results": [
{
Expand Down Expand Up @@ -194,3 +194,7 @@ If you are using :ref:`private versions <versions:privacy levels>`,
users will only be allowed to search projects they have permissions over.
Authentication and authorization is done using the current session,
or any of the valid :doc:`sharing methods </commercial/sharing>`.

Cookie sessions can be used only from the same domain as the API,
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be explained more. What is the takeaway for a user? When should they use this URL instead?

Presumably this should use a readthedocs.com example, where it's most necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be explained more. What is the takeaway for a user? When should they use this URL instead?

This is under the "Authentication and authorization" section of this doc, the paragraph above explains this is for private versions. I have changed this paragraph to avoid mentioning cookies.

we proxy the API on the domain where your docs are being served (``<you-docs-domain>/_/api/v2/search``) so you can use it.
This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project, for example.
18 changes: 6 additions & 12 deletions readthedocs/core/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,16 @@
import time

from django.conf import settings
from django.contrib.sessions.backends.base import SessionBase
from django.contrib.sessions.backends.base import UpdateError
from django.contrib.sessions.backends.base import SessionBase, UpdateError
from django.contrib.sessions.middleware import SessionMiddleware
from django.core.exceptions import SuspiciousOperation
from django.core.exceptions import ImproperlyConfigured, MiddlewareNotUsed
from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
from django.http import Http404, HttpResponseBadRequest
from django.urls.base import set_urlconf
from django.core.exceptions import (
ImproperlyConfigured,
MiddlewareNotUsed,
SuspiciousOperation,
)
from django.utils.cache import patch_vary_headers
from django.utils.deprecation import MiddlewareMixin
from django.utils.http import http_date
from django.utils.translation import ugettext_lazy as _
from django.shortcuts import render

from readthedocs.projects.models import Domain, Project


log = logging.getLogger(__name__)

Expand Down
24 changes: 3 additions & 21 deletions readthedocs/core/signals.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
"""Signal handling for core app."""

import logging
from urllib.parse import urlparse

from corsheaders import signals
from django.conf import settings
from django.db.models import Count, Q
from django.db.models import Count
from django.db.models.signals import pre_delete
from django.dispatch import Signal, receiver
from rest_framework.permissions import SAFE_METHODS
from simple_history.signals import pre_create_historical_record

from readthedocs.builds.models import Version
from readthedocs.core.unresolver import unresolve
from readthedocs.projects.models import Domain, Project
from readthedocs.projects.models import Project

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -50,8 +49,7 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
* It's a safe HTTP method
* The origin is in ALLOWED_URLS
* The URL is owned by the project that they are requesting data from
* The version is public or the domain is linked to the project
(except for the embed API).
* The version is public

.. note::

Expand All @@ -63,8 +61,6 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
if 'HTTP_ORIGIN' not in request.META or request.method not in SAFE_METHODS:
return False

host = urlparse(request.META['HTTP_ORIGIN']).netloc.split(':')[0]

# Always allow the sustainability API,
# it's used only on .org to check for ad-free users.
if _has_donate_app() and request.path_info.startswith('/api/v2/sustainability'):
Expand Down Expand Up @@ -104,20 +100,6 @@ def decide_if_cors(sender, request, **kwargs): # pylint: disable=unused-argumen
if is_public:
return True

# Don't check for known domains for the embed api.
# It gives a lot of information,
# we should use a list of trusted domains from the user.
if valid_url == '/api/v2/embed':
return False

# Or allow if they have a registered domain
# linked to that project.
domain = Domain.objects.filter(
Q(domain__iexact=host),
Q(project=project) | Q(project__subprojects__child=project),
)
if domain.exists():
return True
return False


Expand Down
16 changes: 14 additions & 2 deletions readthedocs/rtd_tests/tests/test_middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def test_allow_linked_domain_from_public_version(self):
resp = self.middleware.process_response(request, {})
self.assertIn('Access-Control-Allow-Origin', resp)

def test_allow_linked_domain_from_private_version(self):
def test_dont_allow_linked_domain_from_private_version(self):
self.version.privacy_level = PRIVATE
self.version.save()
request = self.factory.get(
Expand All @@ -81,7 +81,7 @@ def test_allow_linked_domain_from_private_version(self):
HTTP_ORIGIN='http://my.valid.domain',
)
resp = self.middleware.process_response(request, {})
self.assertIn('Access-Control-Allow-Origin', resp)
self.assertNotIn('Access-Control-Allow-Origin', resp)

def test_allowed_api_public_version_from_another_domain(self):
request = self.factory.get(
Expand Down Expand Up @@ -228,6 +228,7 @@ def setUp(self):

self.user = create_user(username='owner', password='test')

@override_settings(SESSION_COOKIE_SAMESITE=None)
def test_fallback_cookie(self):
request = self.factory.get('/')
response = HttpResponse()
Expand All @@ -238,6 +239,7 @@ def test_fallback_cookie(self):
self.assertTrue(settings.SESSION_COOKIE_NAME in response.cookies)
self.assertTrue(self.middleware.cookie_name_fallback in response.cookies)

@override_settings(SESSION_COOKIE_SAMESITE=None)
def test_main_cookie_samesite_none(self):
request = self.factory.get('/')
response = HttpResponse()
Expand All @@ -247,3 +249,13 @@ def test_main_cookie_samesite_none(self):

self.assertEqual(response.cookies[settings.SESSION_COOKIE_NAME]['samesite'], 'None')
self.assertEqual(response.cookies[self.middleware.cookie_name_fallback]['samesite'], '')

def test_main_cookie_samesite_lax(self):
request = self.factory.get('/')
response = HttpResponse()
self.middleware.process_request(request)
request.session['test'] = 'value'
response = self.middleware.process_response(request, response)

self.assertEqual(response.cookies[settings.SESSION_COOKIE_NAME]['samesite'], 'Lax')
self.assertTrue(self.test_main_cookie_samesite_none not in response.cookies)
12 changes: 10 additions & 2 deletions readthedocs/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,16 @@ class CommunityBaseSettings(Settings):
SESSION_COOKIE_HTTPONLY = True
SESSION_COOKIE_AGE = 30 * 24 * 60 * 60 # 30 days
SESSION_SAVE_EVERY_REQUEST = True
# This cookie is used in cross-origin API requests from *.readthedocs.io to readthedocs.org
SESSION_COOKIE_SAMESITE = None

@property
def SESSION_COOKIE_SAMESITE(self):
"""
Cookie used in cross-origin API requests from *.rtd.io to rtd.org/api/v2/sustainability/.
"""
if self.USE_PROMOS:
return None
# This is django's default.
return 'Lax'

# CSRF
CSRF_COOKIE_HTTPONLY = True
Expand Down
4 changes: 4 additions & 0 deletions readthedocs/settings/proxito/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ class CommunityProxitoSettingsMixin:
USE_SUBDOMAIN = True
SECURE_REFERRER_POLICY = "no-referrer-when-downgrade"

# Always set to Lax for proxito cookies.
# Even if the donate app is present.
SESSION_COOKIE_SAMESITE = 'Lax'

@property
def DATABASES(self):
# This keeps connections to the DB alive,
Expand Down