From 693551f3b4ec31d4994361b96842afd6a0a634ab Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 30 Jun 2021 13:18:57 -0500 Subject: [PATCH 1/5] Cookies: set samesite: `Lax` by default The django's default is `Lax`, so I'm removing the setting. I'm overriding only for .org in our ops repos. We also are only allowing cross site requests from public versions only. Even if the domain is known, if the version is private we don't set the CORS header. --- docs/server-side-search.rst | 16 ++++++++----- readthedocs/core/middleware.py | 18 +++++--------- readthedocs/core/signals.py | 24 +++---------------- .../rtd_tests/tests/test_middleware.py | 16 +++++++++++-- readthedocs/settings/base.py | 2 -- 5 files changed, 33 insertions(+), 43 deletions(-) diff --git a/docs/server-side-search.rst b/docs/server-side-search.rst index 4be97103024..5087f19e491 100644 --- a/docs/server-side-search.rst +++ b/docs/server-side-search.rst @@ -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/ by https://readthedocs.com/ in all the URLs used in the following examples. .. 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`. @@ -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', @@ -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": [ { @@ -194,3 +194,7 @@ If you are using :ref:`private versions `, 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 `. + +Cookie sessions can be used only from the same domain as the API, +we proxy the API from the domain where your docs are being served so you can use it. +This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project, for example. diff --git a/readthedocs/core/middleware.py b/readthedocs/core/middleware.py index 5b48fb2f8de..a768756ce52 100644 --- a/readthedocs/core/middleware.py +++ b/readthedocs/core/middleware.py @@ -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__) diff --git a/readthedocs/core/signals.py b/readthedocs/core/signals.py index 703b73c1a8c..f468758d6b6 100644 --- a/readthedocs/core/signals.py +++ b/readthedocs/core/signals.py @@ -1,18 +1,17 @@ """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 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__) @@ -49,8 +48,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:: @@ -62,8 +60,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'): @@ -103,20 +99,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 diff --git a/readthedocs/rtd_tests/tests/test_middleware.py b/readthedocs/rtd_tests/tests/test_middleware.py index c0ea03ccbd6..3c143e1b900 100644 --- a/readthedocs/rtd_tests/tests/test_middleware.py +++ b/readthedocs/rtd_tests/tests/test_middleware.py @@ -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( @@ -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( @@ -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() @@ -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() @@ -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) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 887d81ef670..b0aea53dae0 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -73,8 +73,6 @@ 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 # CSRF CSRF_COOKIE_HTTPONLY = True From 6eb5a8aeff015da56a72e9a4b05cca4b5e05553f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 1 Jul 2021 10:44:37 -0500 Subject: [PATCH 2/5] Updates from suggestions --- docs/server-side-search.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/server-side-search.rst b/docs/server-side-search.rst index 5087f19e491..e575cf97025 100644 --- a/docs/server-side-search.rst +++ b/docs/server-side-search.rst @@ -73,7 +73,7 @@ API --- If you are using :doc:`/commercial/index` you will need to replace -https://readthedocs.org/ by https://readthedocs.com/ in all the URLs used in the following examples. +https://readthedocs.org/ with https://readthedocs.com/ in all the URLs used in the following examples. .. warning:: @@ -196,5 +196,5 @@ Authentication and authorization is done using the current session, or any of the valid :doc:`sharing methods `. Cookie sessions can be used only from the same domain as the API, -we proxy the API from the domain where your docs are being served so you can use it. +we proxy the API on the domain where your docs are being served (``/_/api/v2/search``) so you can use it. This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project, for example. From b77ca8c29fc85705b00a20839607f3d144416e77 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 2 Aug 2021 14:46:53 -0500 Subject: [PATCH 3/5] Make the value dynamic --- readthedocs/settings/base.py | 10 ++++++++++ readthedocs/settings/proxito/base.py | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index b0aea53dae0..60fb8887722 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -74,6 +74,16 @@ class CommunityBaseSettings(Settings): SESSION_COOKIE_AGE = 30 * 24 * 60 * 60 # 30 days SESSION_SAVE_EVERY_REQUEST = True + @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 CSRF_COOKIE_AGE = 30 * 24 * 60 * 60 diff --git a/readthedocs/settings/proxito/base.py b/readthedocs/settings/proxito/base.py index ad09677c089..e02ecfc0590 100644 --- a/readthedocs/settings/proxito/base.py +++ b/readthedocs/settings/proxito/base.py @@ -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, From 1f3cc26a634ba0c3a0890628f171074d15248ed8 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 5 Aug 2021 17:45:38 -0500 Subject: [PATCH 4/5] Don't mention cookies --- docs/server-side-search.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/server-side-search.rst b/docs/server-side-search.rst index e575cf97025..74299337c7d 100644 --- a/docs/server-side-search.rst +++ b/docs/server-side-search.rst @@ -74,6 +74,7 @@ API 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. +Check :ref:`server-side-search:authentication and authorization` if you are using private versions. .. warning:: @@ -195,6 +196,7 @@ 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 `. -Cookie sessions can be used only from the same domain as the API, -we proxy the API on the domain where your docs are being served (``/_/api/v2/search``) so you can use it. -This is ``https://docs.readthedocs.io/_/api/v2/search`` for the ``docs`` project, for example. +To be able to use the user's current session you need to use the API from the domain where your docs are being served +(``/_/api/v2/search/``). +This is ``https://docs.readthedocs-hosted.com/_/api/v2/search/`` +for the ``https://docs.readthedocs-hosted.com/`` project, for example. From 0de9386be5be5227f816d7e5ce099957f905b7bd Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 5 Aug 2021 17:47:20 -0500 Subject: [PATCH 5/5] Expand comment --- readthedocs/settings/proxito/base.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/settings/proxito/base.py b/readthedocs/settings/proxito/base.py index 21f06b846e8..774560f0b70 100644 --- a/readthedocs/settings/proxito/base.py +++ b/readthedocs/settings/proxito/base.py @@ -15,6 +15,8 @@ class CommunityProxitoSettingsMixin: # Always set to Lax for proxito cookies. # Even if the donate app is present. + # Since we don't want to allow cookies from cross origin requests. + # This is django's default. SESSION_COOKIE_SAMESITE = 'Lax' @property