From eb7cad02a8a13b5dd2651badbe451e8d598d3a64 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 12 Jun 2024 12:27:44 -0400 Subject: [PATCH 1/7] feat: add custom middleware for x-frame-options to allow overrides --- .../security/clickjacking/README.rst | 28 +++++ .../security/clickjacking/__init__.py | 0 .../security/clickjacking/middleware.py | 56 +++++++++ .../clickjacking/tests/test_middleware.py | 119 ++++++++++++++++++ 4 files changed, 203 insertions(+) create mode 100644 edx_django_utils/security/clickjacking/README.rst create mode 100644 edx_django_utils/security/clickjacking/__init__.py create mode 100644 edx_django_utils/security/clickjacking/middleware.py create mode 100644 edx_django_utils/security/clickjacking/tests/test_middleware.py diff --git a/edx_django_utils/security/clickjacking/README.rst b/edx_django_utils/security/clickjacking/README.rst new file mode 100644 index 00000000..81f5f54d --- /dev/null +++ b/edx_django_utils/security/clickjacking/README.rst @@ -0,0 +1,28 @@ +Security Utils +############## + +Common security utilities. + +Deprecated - Clickjacking middleware +************************************ + +Consider this deprecated as new projects should be using Content Security Policy headers instead. +However, this won't be removed until no openedx project depends on it anymore, specifically, edx-platform. +Please do not add this middleware to a project unless you have to use the ``X-Frame-Options`` header, +and do not add it outside the openedx github organization, as it may be removed in the future. + +Clickjacking middleware +*********************** + +The preferred way to prevent clickjacking is to use the Content Security Policy headers. +In some legacy code where those headers are not used, it is required to instead use the older +header ``X-Frame-Options`` set either to ``DENY`` or to ``SAMEORIGIN``. +In any case, this middleware allows you both to set the ``X-Frame-Options`` header to any recognized value - +``DENY``, ``SAMEORIGIN``, ``ALLOW`` per django setting - but defaults to ``DENY``. +It also allows you to override the header for specific urls defined via regex. + +- Add the middleware ``'edx_django_utils.security.clickjacking.middleware.EdxXFrameOptionsMiddleware'`` near the end of your ``MIDDLEWARE`` list. +- This will add an `X-Frame-Options` header to all responses. +- Add ``X_FRAME_OPTIONS = value`` to your django settings file with "value" being ``DENY``, ``SAMEORIGIN``, or ``ALLOW``. +- Optionally, add ``X_FRAME_OPTIONS_OVERRIDES = [[regex, value]]`` where ``[[regex, value]]`` is a list of + pairs consisting of a regex that matches urls to override and a value that's one of ``DENY``, ``SAMEORIGIN``, and ``ALLOW``. diff --git a/edx_django_utils/security/clickjacking/__init__.py b/edx_django_utils/security/clickjacking/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/edx_django_utils/security/clickjacking/middleware.py b/edx_django_utils/security/clickjacking/middleware.py new file mode 100644 index 00000000..9c29945b --- /dev/null +++ b/edx_django_utils/security/clickjacking/middleware.py @@ -0,0 +1,56 @@ +""" +Middleware to add correct x-frame-options headers. + +The headers get set to the platform default which we assume is `DENY`. +However, there's a number of paths that are set to `SAMEORIGIN` which +we identify via regexes stored in a django setting in the application calling this. +""" +import re + +from django.conf import settings +from django.middleware.clickjacking import XFrameOptionsMiddleware + +from django.http.request import HttpRequest +from django.http.response import HttpResponse + +PERMISSIBLE_VALUES = ['DENY', 'SAMEORIGIN', 'ALLOW'] + +class InvalidHeaderValueError(ValueError): + """ A custom error that is thrown when we try to set an invalid value for a header """ + pass + +def _validate_header_value(value): + if value not in PERMISSIBLE_VALUES: + raise InvalidHeaderValueError( + f'Invalid value "{value}" for header "X-Frame-Options"' + ) + +class EdxXFrameOptionsMiddleware(XFrameOptionsMiddleware): + """ + A class extending the django XFrameOptionsMiddleware with the ability to override + the header for URLs specified in a `X_FRAME_OPTIONS_OVERRIDES` django setting. + You can set this via `X_FRAME_OPTIONS_OVERRIDES = [[regex, value]]` in your django application + where you specify a list of pairs of regex and value, where regex matches urls and value is + one of `DENY`, `SAMEORIGIN`, `ALLOW`. The latter is not advisable unless you have a content security + policy in place. + """ + def process_response(self, request: HttpRequest, response: HttpResponse) -> HttpResponse: + """ + Process the response and set the x-frame-options header to the value specified + """ + response = super().process_response(request, response) + headers = response.headers + request_path = request.path + frame_options = getattr(settings, 'X_FRAME_OPTIONS', 'DENY') + _validate_header_value(frame_options) + + headers['X-Frame-Options'] = frame_options + overrides = getattr(settings, 'X_FRAME_OPTIONS_OVERRIDES', []) + for override in overrides: + print('override', override) + regex, value = override + _validate_header_value(value) + if re.search(regex, request_path): + headers['X-Frame-Options'] = value + response.headers = headers + return response diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py new file mode 100644 index 00000000..a9d605d2 --- /dev/null +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -0,0 +1,119 @@ +""" +Tests for Content-Security-Policy middleware. +""" + +from unittest import TestCase +from unittest.mock import MagicMock, patch + +import ddt +from edx_django_utils.security.clickjacking.middleware import EdxXFrameOptionsMiddleware + + +@ddt.ddt +class TestEdxXFrameOptionsMiddleware(TestCase): + """Test the actual middleware.""" + + def setUp(self): + super().setUp() + + @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') + @patch('edx_django_utils.security.clickjacking.middleware.settings') + def test_x_frame_must_be_deny_on_no_override(self, settings, validate_header): + settings.X_FRAME_OPTIONS = 'DENY' + validate_header.return_value = True + + request = MagicMock() + response = MagicMock() + response.headers = {} + middleware = EdxXFrameOptionsMiddleware(get_response=lambda _: response) + + middleware.process_response(request, response) + + assert response.headers['X-Frame-Options'] == 'DENY' + validate_header.assert_called_once_with('DENY') + + @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') + @patch('edx_django_utils.security.clickjacking.middleware.settings') + def test_on_override_with_valid_regex_is_sameorigin(self, settings, validate_header): + """ + If the URL matches one of the overrides, the header should be set to + the correct override setting as specified in the `X_FRAME_OPTIONS_OVERRIDES` list. + """ + settings.X_FRAME_OPTIONS = 'DENY' + settings.X_FRAME_OPTIONS_OVERRIDES = [['.*/media/scorm/.*', 'SAMEORIGIN']] + validate_header.return_value = True + + request = MagicMock() + response = MagicMock() + response.headers = {} + request.path = 'http://localhost:18010/media/scorm/hello/world' + middleware = EdxXFrameOptionsMiddleware(get_response=lambda _: response) + + middleware.process_response(request, response) + + assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' + + @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') + @patch('edx_django_utils.security.clickjacking.middleware.settings') + def test_on_override_for_non_matching_urls_is_deny(self, settings, validate_header): + """ + If the URL does not match any of the overrides, the header should be set to + the `X_FRAME_OPTIONS` setting. + """ + settings.X_FRAME_OPTIONS = 'DENY' + settings.X_FRAME_OPTIONS_OVERRIDES = [['.*/media/scorm/.*', 'SAMEORIGIN']] + validate_header.return_value = True + + request = MagicMock() + response = MagicMock() + response.headers = {} + request.path = 'http://localhost:18010/notmedia/scorm/hello/world' + middleware = EdxXFrameOptionsMiddleware(get_response=lambda _: response) + + middleware.process_response(request, response) + + assert response.headers['X-Frame-Options'] == 'DENY' + + def test_x_frame_defaults_to_deny(self): + request = MagicMock() + response = MagicMock() + response.headers = {} + middleware = EdxXFrameOptionsMiddleware(get_response=lambda _: response) + + middleware.process_response(request, response) + + assert response.headers['X-Frame-Options'] == 'DENY' + + + + + + + + + + + + + + + + + + + + + + + + + # settings.X_FRAME_OPTIONS_OVERRIDES = [['regex', 'SAMEORIGIN']] + # search.return_value = False + # middleware.process_response(request, response) + # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' + # validate_header.assert_called_with('SAMEORIGIN') + # search.assert_called_with('regex') + # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' + # validate_header.assert_called_with('SAMEORIGIN') + # search.assert_called_with('regex') + # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' From 389a7e9d6cb0970de897865f427bfcb30368869e Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 12 Jun 2024 15:15:32 -0400 Subject: [PATCH 2/7] fix: lint --- .../clickjacking/tests/test_middleware.py | 54 +++++-------------- 1 file changed, 14 insertions(+), 40 deletions(-) diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py index a9d605d2..eb2e1b73 100644 --- a/edx_django_utils/security/clickjacking/tests/test_middleware.py +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -13,13 +13,15 @@ class TestEdxXFrameOptionsMiddleware(TestCase): """Test the actual middleware.""" - def setUp(self): - super().setUp() @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') - def test_x_frame_must_be_deny_on_no_override(self, settings, validate_header): - settings.X_FRAME_OPTIONS = 'DENY' + def test_x_frame_setting_must_apply_on_no_override(self, settings, validate_header): + """ + If the setting `X_FRAME_OPTIONS` is set but no overrides are specified, + the `X-Frame-Options` header should be set to that setting. + """ + settings.X_FRAME_OPTIONS = 'SAMEORIGIN' validate_header.return_value = True request = MagicMock() @@ -29,8 +31,9 @@ def test_x_frame_must_be_deny_on_no_override(self, settings, validate_header): middleware.process_response(request, response) - assert response.headers['X-Frame-Options'] == 'DENY' - validate_header.assert_called_once_with('DENY') + assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' + validate_header.assert_called_once_with('SAMEORIGIN') + @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') @@ -53,6 +56,7 @@ def test_on_override_with_valid_regex_is_sameorigin(self, settings, validate_hea assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' + @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') def test_on_override_for_non_matching_urls_is_deny(self, settings, validate_header): @@ -74,7 +78,11 @@ def test_on_override_for_non_matching_urls_is_deny(self, settings, validate_head assert response.headers['X-Frame-Options'] == 'DENY' + def test_x_frame_defaults_to_deny(self): + """ + The default value of the `X-Frame-Options` header should be `DENY`. + """ request = MagicMock() response = MagicMock() response.headers = {} @@ -83,37 +91,3 @@ def test_x_frame_defaults_to_deny(self): middleware.process_response(request, response) assert response.headers['X-Frame-Options'] == 'DENY' - - - - - - - - - - - - - - - - - - - - - - - - - # settings.X_FRAME_OPTIONS_OVERRIDES = [['regex', 'SAMEORIGIN']] - # search.return_value = False - # middleware.process_response(request, response) - # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' - # validate_header.assert_called_with('SAMEORIGIN') - # search.assert_called_with('regex') - # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' - # validate_header.assert_called_with('SAMEORIGIN') - # search.assert_called_with('regex') - # assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' From 0f5bc7dfa2fb96b4f7e2413b5a4aa1b87e42a6be Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 12 Jun 2024 15:16:50 -0400 Subject: [PATCH 3/7] fix: lint --- edx_django_utils/security/clickjacking/middleware.py | 3 +++ .../security/clickjacking/tests/test_middleware.py | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/edx_django_utils/security/clickjacking/middleware.py b/edx_django_utils/security/clickjacking/middleware.py index 9c29945b..8fc22caa 100644 --- a/edx_django_utils/security/clickjacking/middleware.py +++ b/edx_django_utils/security/clickjacking/middleware.py @@ -15,16 +15,19 @@ PERMISSIBLE_VALUES = ['DENY', 'SAMEORIGIN', 'ALLOW'] + class InvalidHeaderValueError(ValueError): """ A custom error that is thrown when we try to set an invalid value for a header """ pass + def _validate_header_value(value): if value not in PERMISSIBLE_VALUES: raise InvalidHeaderValueError( f'Invalid value "{value}" for header "X-Frame-Options"' ) + class EdxXFrameOptionsMiddleware(XFrameOptionsMiddleware): """ A class extending the django XFrameOptionsMiddleware with the ability to override diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py index eb2e1b73..c83522cb 100644 --- a/edx_django_utils/security/clickjacking/tests/test_middleware.py +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -13,7 +13,6 @@ class TestEdxXFrameOptionsMiddleware(TestCase): """Test the actual middleware.""" - @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') def test_x_frame_setting_must_apply_on_no_override(self, settings, validate_header): @@ -34,7 +33,6 @@ def test_x_frame_setting_must_apply_on_no_override(self, settings, validate_head assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' validate_header.assert_called_once_with('SAMEORIGIN') - @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') def test_on_override_with_valid_regex_is_sameorigin(self, settings, validate_header): @@ -56,7 +54,6 @@ def test_on_override_with_valid_regex_is_sameorigin(self, settings, validate_hea assert response.headers['X-Frame-Options'] == 'SAMEORIGIN' - @patch('edx_django_utils.security.clickjacking.middleware._validate_header_value') @patch('edx_django_utils.security.clickjacking.middleware.settings') def test_on_override_for_non_matching_urls_is_deny(self, settings, validate_header): @@ -78,7 +75,6 @@ def test_on_override_for_non_matching_urls_is_deny(self, settings, validate_head assert response.headers['X-Frame-Options'] == 'DENY' - def test_x_frame_defaults_to_deny(self): """ The default value of the `X-Frame-Options` header should be `DENY`. From 77f496681899f856e601babd44426caa74cf7ed4 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Wed, 12 Jun 2024 16:12:27 -0400 Subject: [PATCH 4/7] fix: lint --- edx_django_utils/security/clickjacking/middleware.py | 6 +++--- .../security/clickjacking/tests/test_middleware.py | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/edx_django_utils/security/clickjacking/middleware.py b/edx_django_utils/security/clickjacking/middleware.py index 8fc22caa..0f0bd176 100644 --- a/edx_django_utils/security/clickjacking/middleware.py +++ b/edx_django_utils/security/clickjacking/middleware.py @@ -5,13 +5,13 @@ However, there's a number of paths that are set to `SAMEORIGIN` which we identify via regexes stored in a django setting in the application calling this. """ -import re from django.conf import settings -from django.middleware.clickjacking import XFrameOptionsMiddleware - from django.http.request import HttpRequest from django.http.response import HttpResponse +from django.middleware.clickjacking import XFrameOptionsMiddleware + +import re PERMISSIBLE_VALUES = ['DENY', 'SAMEORIGIN', 'ALLOW'] diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py index c83522cb..fec5294a 100644 --- a/edx_django_utils/security/clickjacking/tests/test_middleware.py +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -2,11 +2,12 @@ Tests for Content-Security-Policy middleware. """ +import ddt +from edx_django_utils.security.clickjacking.middleware import EdxXFrameOptionsMiddleware + from unittest import TestCase from unittest.mock import MagicMock, patch -import ddt -from edx_django_utils.security.clickjacking.middleware import EdxXFrameOptionsMiddleware @ddt.ddt From 5e3094f26d2cc929683e4819f7d2a4dbe9e186c3 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 13 Jun 2024 10:57:23 -0400 Subject: [PATCH 5/7] fix: lint --- edx_django_utils/security/clickjacking/tests/test_middleware.py | 1 - 1 file changed, 1 deletion(-) diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py index fec5294a..5b65d858 100644 --- a/edx_django_utils/security/clickjacking/tests/test_middleware.py +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -9,7 +9,6 @@ from unittest.mock import MagicMock, patch - @ddt.ddt class TestEdxXFrameOptionsMiddleware(TestCase): """Test the actual middleware.""" From db67f12ba62a1e44e276b9958d0fd2b116b52339 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 13 Jun 2024 11:14:20 -0400 Subject: [PATCH 6/7] fix: lint --- edx_django_utils/security/clickjacking/middleware.py | 4 ++-- .../security/clickjacking/tests/test_middleware.py | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/edx_django_utils/security/clickjacking/middleware.py b/edx_django_utils/security/clickjacking/middleware.py index 0f0bd176..b79c5a2e 100644 --- a/edx_django_utils/security/clickjacking/middleware.py +++ b/edx_django_utils/security/clickjacking/middleware.py @@ -6,13 +6,13 @@ we identify via regexes stored in a django setting in the application calling this. """ +import re + from django.conf import settings from django.http.request import HttpRequest from django.http.response import HttpResponse from django.middleware.clickjacking import XFrameOptionsMiddleware -import re - PERMISSIBLE_VALUES = ['DENY', 'SAMEORIGIN', 'ALLOW'] diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_middleware.py index 5b65d858..4e33d0d7 100644 --- a/edx_django_utils/security/clickjacking/tests/test_middleware.py +++ b/edx_django_utils/security/clickjacking/tests/test_middleware.py @@ -2,12 +2,13 @@ Tests for Content-Security-Policy middleware. """ -import ddt -from edx_django_utils.security.clickjacking.middleware import EdxXFrameOptionsMiddleware - from unittest import TestCase from unittest.mock import MagicMock, patch +import ddt + +from edx_django_utils.security.clickjacking.middleware import EdxXFrameOptionsMiddleware + @ddt.ddt class TestEdxXFrameOptionsMiddleware(TestCase): From 0ce0e0fc468b50d856dce24f5d7ab5a264be8718 Mon Sep 17 00:00:00 2001 From: Jesper Hodge Date: Thu, 13 Jun 2024 11:20:41 -0400 Subject: [PATCH 7/7] fix: tests --- .../tests/{test_middleware.py => test_clickjacking_middleware.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename edx_django_utils/security/clickjacking/tests/{test_middleware.py => test_clickjacking_middleware.py} (100%) diff --git a/edx_django_utils/security/clickjacking/tests/test_middleware.py b/edx_django_utils/security/clickjacking/tests/test_clickjacking_middleware.py similarity index 100% rename from edx_django_utils/security/clickjacking/tests/test_middleware.py rename to edx_django_utils/security/clickjacking/tests/test_clickjacking_middleware.py