From fd93ef5f9940f4ad6f50cf7faecad8d9cf2d3336 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 27 May 2026 15:03:35 -0400 Subject: [PATCH 1/7] fix: require Django staff to call set_course_mode_price endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The set_course_mode_price view had no authorization check beyond @login_required, meaning any authenticated user could POST to it and rewrite the honor-mode price for any course — a privilege escalation vulnerability. Ideally this endpoint would be removed: it has no known callers in the UI (no templates or JS reference it), no tests, and targets the legacy 'honor' mode. However, it is publicly routed and external consumers may depend on it, so removal requires going through the DEPR process before we can act. In the meantime, this commit closes the security hole regardless of how active the endpoint is. Co-Authored-By: Claude Sonnet 4.6 --- lms/djangoapps/instructor/views/instructor_dashboard.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 3c47a67b632a..170c3a2280b0 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -412,6 +412,11 @@ def set_course_mode_price(request, course_id): """ set the new course price and add new entry in the CourseModesArchive Table """ + if not request.user.is_staff: + return JsonResponse( + {'message': _("You do not have permission to perform this action.")}, + status=403 + ) try: course_price = int(request.POST['course_price']) except ValueError: From 0a92cf25de8844bf840ffd5d18dcfd940031a2ba Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 1 Jun 2026 11:56:23 -0400 Subject: [PATCH 2/7] fix: implement OAuth nonce replay protection in LTI provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_timestamp_and_nonce previously returned True unconditionally, allowing any captured LTI launch request to be replayed indefinitely. Now rejects requests whose oauth_timestamp falls outside a ±5-minute window, then atomically records the nonce in the Django cache via cache.add() (OEP-0022 key generation via get_cache_key). A replay returns False immediately because cache.add() only writes when the key is absent. TieredCache is intentionally not used here: it has no atomic add primitive, so a separate get-then-set would leave a race window that defeats the protection. See the updated docstring for details. Documents the requirement for a shared cache backend (Redis or Memcached) in multi-node deployments in both the app and repo READMEs. Fixes GHSA-6gm5-c49g-p3h9 Co-Authored-By: Claude Sonnet 4.6 --- README.rst | 17 +++++++ lms/djangoapps/lti_provider/README.rst | 31 ++++++++++++ .../lti_provider/signature_validator.py | 32 +++++++++++- .../tests/test_signature_validator.py | 49 +++++++++++++++++++ 4 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 lms/djangoapps/lti_provider/README.rst diff --git a/README.rst b/README.rst index dcd6e32c4998..fcadf7070e96 100644 --- a/README.rst +++ b/README.rst @@ -228,6 +228,23 @@ running at the given ports. - localhost:1997 - ACCOUNT_MICROFRONTEND_URL +Security Deployment Requirements +******************************** + +Some platform features require a **shared** Django cache backend (Redis or +Memcached) to function correctly across multiple LMS nodes: + +* **LTI Provider** — OAuth nonce replay protection stores seen nonces in the + Django ``default`` cache. A per-process backend (e.g. ``LocMemCache``) will + not detect replays that arrive on a different node. See + `lms/djangoapps/lti_provider/README.rst`_ for details. + +Tutor-based deployments satisfy this requirement automatically. For bare-metal +or custom deployments, verify that ``CACHES['default']`` points at a shared +Redis or Memcached instance before enabling these features. + +.. _lms/djangoapps/lti_provider/README.rst: lms/djangoapps/lti_provider/README.rst + License ******* diff --git a/lms/djangoapps/lti_provider/README.rst b/lms/djangoapps/lti_provider/README.rst new file mode 100644 index 000000000000..58ab7671521a --- /dev/null +++ b/lms/djangoapps/lti_provider/README.rst @@ -0,0 +1,31 @@ +LTI Provider +############ + +This Django app implements the server (provider) side of the `1EdTech LTI +1.1`_ specification, allowing external tools and platforms to launch content +inside the Open edX LMS using OAuth 1.0-signed requests. LTI 1.2 and 1.3 +are not supported by this app. + +.. _1EdTech LTI 1.1: https://www.imsglobal.org/lti/ltiv1p2/ltiIMGv1p2.html + +Security Requirements +********************* + +Shared Cache Backend (Required for Multi-Node Deployments) +=========================================================== + +The LTI provider protects against OAuth replay attacks by storing each +``oauth_nonce`` in Django's cache after it is first seen and rejecting any +subsequent request that presents the same nonce within the validity window +(±5 minutes around the ``oauth_timestamp``). + +**This protection only works correctly when all LMS nodes share the same cache +backend.** If you run more than one LMS process or server, you must configure +Django's ``default`` cache to use a shared backend such as Redis or Memcached. +A per-process backend (e.g. Django's built-in ``LocMemCache``) keeps a +separate in-memory store per process, so a replayed request arriving on a +different node will not be detected. + +Tutor-based deployments use Redis by default and satisfy this requirement +automatically. For bare-metal or custom deployments, verify that +``CACHES['default']`` is pointed at a shared Redis or Memcached instance. diff --git a/lms/djangoapps/lti_provider/signature_validator.py b/lms/djangoapps/lti_provider/signature_validator.py index a17bd41ffda2..e7daab5169de 100644 --- a/lms/djangoapps/lti_provider/signature_validator.py +++ b/lms/djangoapps/lti_provider/signature_validator.py @@ -2,7 +2,10 @@ Subclass of oauthlib's RequestValidator that checks an OAuth signature. """ +import time +from django.core.cache import cache +from edx_django_utils.cache import get_cache_key from oauthlib.oauth1 import RequestValidator, SignatureOnlyEndpoint @@ -63,10 +66,37 @@ def validate_timestamp_and_nonce(self, client_key, timestamp, nonce, in which the timestamp marks a request as valid. This method signature is required by the oauthlib library. + Rejects requests whose timestamp falls outside a ±5-minute window, then + performs an atomic check-and-store of the nonce so that replayed requests + are rejected. + + We use Django's cache.add() directly rather than TieredCache because + nonce replay prevention requires an atomic check-and-store: cache.add() + only writes the key when it does not already exist and returns False + immediately if it does. TieredCache has no equivalent atomic primitive, + so a separate get-then-set would leave a race window that an attacker + could exploit. + + Security requirement: this protection only holds across all LMS nodes if + the Django cache backend is shared (e.g. Redis or Memcached). A + per-process backend such as LocMemCache will not prevent replays that + arrive on a different node. See lms/djangoapps/lti_provider/README.rst. + :return: True if the OAuth nonce and timestamp are valid, False if they are not. """ - return True + try: + if abs(time.time() - int(timestamp)) > 300: + return False + except (ValueError, TypeError): + return False + + # Nonces are scoped per client_key so that two different LTI consumers + # generating the same nonce string do not interfere with each other. + cache_key = get_cache_key(namespace="lti_nonce", client_key=client_key, nonce=nonce) + # cache.add() atomically sets the key only if absent. + # Returns True (new nonce, allow) or False (already seen, reject). + return cache.add(cache_key, 1, 600) def validate_client_key(self, client_key, request): """ diff --git a/lms/djangoapps/lti_provider/tests/test_signature_validator.py b/lms/djangoapps/lti_provider/tests/test_signature_validator.py index 2c92df8984a8..74ba5ec18f9f 100644 --- a/lms/djangoapps/lti_provider/tests/test_signature_validator.py +++ b/lms/djangoapps/lti_provider/tests/test_signature_validator.py @@ -11,6 +11,9 @@ from lms.djangoapps.lti_provider.models import LtiConsumer from lms.djangoapps.lti_provider.signature_validator import SignatureValidator +from openedx.core.djangolib.testing.utils import CacheIsolationMixin + +FIXED_TIMESTAMP = 1_000_000_000 def get_lti_consumer(): @@ -118,3 +121,49 @@ def test_verification_parameters(self, verify_mock): SignatureValidator(self.lti_consumer).verify(request) verify_mock.assert_called_once_with( request.build_absolute_uri(), 'POST', body.encode('utf-8'), headers) + + +@patch('lms.djangoapps.lti_provider.signature_validator.time.time', return_value=FIXED_TIMESTAMP) +class TimestampAndNonceValidatorTest(CacheIsolationMixin, TestCase): + ENABLED_CACHES = ['default'] + """ + Tests for validate_timestamp_and_nonce. The clock is frozen via a class-level + patch so every test method receives a consistent `time.time` return value. + """ + + def setUp(self): + super().setUp() + self.validator = SignatureValidator(get_lti_consumer()) + + def _call(self, timestamp, nonce, client_key='Consumer Key'): + return self.validator.validate_timestamp_and_nonce( + client_key, str(timestamp), nonce, request=None, + ) + + def test_valid_timestamp_and_new_nonce(self, _mock_time): + assert self._call(FIXED_TIMESTAMP, 'nonce-a') + + def test_stale_timestamp_rejected(self, _mock_time): + assert not self._call(FIXED_TIMESTAMP - 301, 'nonce-b') + + def test_future_timestamp_rejected(self, _mock_time): + assert not self._call(FIXED_TIMESTAMP + 301, 'nonce-c') + + def test_malformed_timestamp_rejected(self, _mock_time): + assert not self.validator.validate_timestamp_and_nonce( + 'Consumer Key', 'not-a-number', 'nonce-d', request=None, + ) + + def test_replay_rejected(self, _mock_time): + assert self._call(FIXED_TIMESTAMP, 'nonce-e') + assert not self._call(FIXED_TIMESTAMP, 'nonce-e') + + def test_different_nonce_same_consumer_accepted(self, _mock_time): + assert self._call(FIXED_TIMESTAMP, 'nonce-f1') + assert self._call(FIXED_TIMESTAMP, 'nonce-f2') + + def test_same_nonce_different_consumer_accepted(self, _mock_time): + # Nonces are scoped per client_key; the same nonce string from two + # different consumers must not block each other. + assert self._call(FIXED_TIMESTAMP, 'nonce-g', client_key='Consumer Key') + assert self._call(FIXED_TIMESTAMP, 'nonce-g', client_key='Other Key') From 67ef02d526a7dcd59aecbd1ffb1c3b5dee95398c Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 4 Jun 2026 11:24:19 -0400 Subject: [PATCH 3/7] style: fix pylint and ruff issues in LTI nonce replay test - Add missing class docstring to TimestampAndNonceValidatorTest (C0115) - Move time.time patch from class decorator into setUp/addCleanup to eliminate unused mock parameters in every test method (PT019) Co-Authored-By: Claude Sonnet 4.6 --- .../tests/test_signature_validator.py | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/lti_provider/tests/test_signature_validator.py b/lms/djangoapps/lti_provider/tests/test_signature_validator.py index 74ba5ec18f9f..9bafea96fcf0 100644 --- a/lms/djangoapps/lti_provider/tests/test_signature_validator.py +++ b/lms/djangoapps/lti_provider/tests/test_signature_validator.py @@ -123,16 +123,21 @@ def test_verification_parameters(self, verify_mock): request.build_absolute_uri(), 'POST', body.encode('utf-8'), headers) -@patch('lms.djangoapps.lti_provider.signature_validator.time.time', return_value=FIXED_TIMESTAMP) class TimestampAndNonceValidatorTest(CacheIsolationMixin, TestCase): - ENABLED_CACHES = ['default'] """ - Tests for validate_timestamp_and_nonce. The clock is frozen via a class-level - patch so every test method receives a consistent `time.time` return value. + Tests for the validate_timestamp_and_nonce method in SignatureValidator. """ + ENABLED_CACHES = ['default'] + def setUp(self): super().setUp() + patcher = patch( + 'lms.djangoapps.lti_provider.signature_validator.time.time', + return_value=FIXED_TIMESTAMP, + ) + patcher.start() + self.addCleanup(patcher.stop) self.validator = SignatureValidator(get_lti_consumer()) def _call(self, timestamp, nonce, client_key='Consumer Key'): @@ -140,29 +145,29 @@ def _call(self, timestamp, nonce, client_key='Consumer Key'): client_key, str(timestamp), nonce, request=None, ) - def test_valid_timestamp_and_new_nonce(self, _mock_time): + def test_valid_timestamp_and_new_nonce(self): assert self._call(FIXED_TIMESTAMP, 'nonce-a') - def test_stale_timestamp_rejected(self, _mock_time): + def test_stale_timestamp_rejected(self): assert not self._call(FIXED_TIMESTAMP - 301, 'nonce-b') - def test_future_timestamp_rejected(self, _mock_time): + def test_future_timestamp_rejected(self): assert not self._call(FIXED_TIMESTAMP + 301, 'nonce-c') - def test_malformed_timestamp_rejected(self, _mock_time): + def test_malformed_timestamp_rejected(self): assert not self.validator.validate_timestamp_and_nonce( 'Consumer Key', 'not-a-number', 'nonce-d', request=None, ) - def test_replay_rejected(self, _mock_time): + def test_replay_rejected(self): assert self._call(FIXED_TIMESTAMP, 'nonce-e') assert not self._call(FIXED_TIMESTAMP, 'nonce-e') - def test_different_nonce_same_consumer_accepted(self, _mock_time): + def test_different_nonce_same_consumer_accepted(self): assert self._call(FIXED_TIMESTAMP, 'nonce-f1') assert self._call(FIXED_TIMESTAMP, 'nonce-f2') - def test_same_nonce_different_consumer_accepted(self, _mock_time): + def test_same_nonce_different_consumer_accepted(self): # Nonces are scoped per client_key; the same nonce string from two # different consumers must not block each other. assert self._call(FIXED_TIMESTAMP, 'nonce-g', client_key='Consumer Key') From 4f15b6d4e011f59a590f6f949d2ad62a10056d44 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 4 Jun 2026 12:02:28 -0400 Subject: [PATCH 4/7] style: fix E7670 pylint error for SAML_METADATA_URL_ALLOW_PRIVATE_IPS The setting annotation used setting_name/setting_default which triggers E7670 (setting-boolean-default-value) for boolean defaults. This was fixed on master using toggle_name/toggle_default annotations (which allow boolean values) but was not backported to ulmo. Fixing here alongside the other style issues. Co-Authored-By: Claude Sonnet 4.6 --- openedx/envs/common.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/openedx/envs/common.py b/openedx/envs/common.py index 33275894910d..1034346c8e21 100644 --- a/openedx/envs/common.py +++ b/openedx/envs/common.py @@ -1576,16 +1576,19 @@ def _make_locale_paths(settings): SOCIAL_AUTH_SAML_SP_PRIVATE_KEY_DICT = {} SOCIAL_AUTH_SAML_SP_PUBLIC_CERT_DICT = {} -# .. setting_name: SAML_METADATA_URL_ALLOW_PRIVATE_IPS -# .. setting_default: False -# .. setting_description: When False (the default), fetching SAML metadata from +# .. toggle_name: SAML_METADATA_URL_ALLOW_PRIVATE_IPS +# .. toggle_default: False +# .. toggle_description: When False (the default), fetching SAML metadata from # private IP address ranges (RFC 1918: 10.x, 172.16.x, 192.168.x) is blocked # as a defense against SSRF attacks. Set to True only in deployments where the # SAML Identity Provider is hosted on the same private network as the Open edX # server. Note: loopback (127.x) and link-local (169.254.x) addresses remain -# blocked regardless of this setting. Operators are also encouraged to enforce +# blocked regardless of this toggle. Operators are also encouraged to enforce # network-level egress filtering as a complementary control, particularly to # cover hostname-based URLs that are not subject to IP validation. +# .. toggle_implementation: DjangoSetting +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2026-04-24 SAML_METADATA_URL_ALLOW_PRIVATE_IPS = False ########################### django-fernet-fields ########################### From 723be60058670b47b339b993d0e83e9da694de5f Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed Date: Thu, 4 Jun 2026 14:12:36 +0500 Subject: [PATCH 5/7] fix: backport openedx-forum 0.4.1 to ulmo for thread sort bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps the `openedx-forum` pin from 0.3.8 to 0.4.1 on `release/ulmo` to pull in the fix for the "pinned"-NULL sort bug (openedx/forum#270, commit 78b36e4) where discussion threads were not ordering correctly when users selected "recent first" — old threads with NULL `pinned` values floated above newer threads. Reported in: https://discuss.openedx.org/t/discuss-forum-messages-order-not-organized-as-expected-in-teak/18665 The 0.3.8 -> 0.4.1 delta brings in: * 0.3.9 — five small bug fixes (Mongo content_type, timestamps, MySQL read_states lookup, Forum V2 author field, build). * 0.4.0 — MySQL backend query optimizations (N+1 fixes, prefetch/select_related); backwards compatible. * 0.4.1 — the `pinned` NULL sort fix (commit 78b36e4) plus the optional Typesense search backend (additive, off by default). Caps openedx-forum at <=0.4.1 in constraints.txt to avoid: * 0.4.2 — drops Python 3.11 support. * 0.4.3 — removes the MongoDB backend (Ulmo deployments may rely on it). The constraint should be removed on master / post-Ulmo release lines. Co-Authored-By: Claude Opus 4.7 --- requirements/constraints.txt | 11 +++++++++++ requirements/edx/base.txt | 13 ++++++++++--- requirements/edx/development.txt | 10 +++++++++- requirements/edx/doc.txt | 12 ++++++++++-- requirements/edx/testing.txt | 12 ++++++++++-- 5 files changed, 50 insertions(+), 8 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index dd80bc9c72c1..d25f0099da96 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -68,6 +68,17 @@ openedx-learning==0.30.2 # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35268 openai<=0.28.1 +# Date: 2026-06-04 +# Cap openedx-forum to 0.4.1 on the Ulmo release line: +# * 0.4.1 contains the fix for the "pinned" NULL sort bug +# (https://github.com/openedx/forum/pull/270 — discussion threads were not +# ordering correctly when sorted by "recent first"). See user report: +# https://discuss.openedx.org/t/discuss-forum-messages-order-not-organized-as-expected-in-teak/18665 +# * 0.4.2 drops Python 3.11 support. +# * 0.4.3 removes the MongoDB backend, which Ulmo deployments may still rely on. +# Remove this cap on master / post-Ulmo release lines. +openedx-forum<=0.4.1 + # Date: 2024-04-26 # path==16.12.0 breaks the unit test collections check # needs to be investigated and fixed separately diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6b1596a06543..2112b43dab11 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -661,7 +661,9 @@ html5lib==1.1 httpcore==1.0.9 # via httpx httpx[http2]==0.28.1 - # via firebase-admin + # via + # firebase-admin + # typesense hyperframe==6.1.0 # via h2 icalendar==6.3.1 @@ -852,8 +854,10 @@ openedx-filters==2.1.0 # -r requirements/edx/kernel.in # lti-consumer-xblock # ora2 -openedx-forum==0.3.8 - # via -r requirements/edx/kernel.in +openedx-forum==0.4.1 + # via + # -c requirements/constraints.txt + # -r requirements/edx/kernel.in openedx-learning==0.30.2 # via # -c requirements/constraints.txt @@ -1189,6 +1193,8 @@ tqdm==4.67.1 # via # nltk # openai +typesense==2.0.0 + # via openedx-forum typing-extensions==4.15.0 # via # aiosignal @@ -1204,6 +1210,7 @@ typing-extensions==4.15.0 # pyopenssl # referencing # snowflake-connector-python + # typesense # typing-inspection typing-inspection==0.4.2 # via pydantic diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index c16e2117053f..b1953f6c6849 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1077,6 +1077,7 @@ httpx[http2]==0.28.1 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # firebase-admin + # typesense hyperframe==6.1.0 # via # -r requirements/edx/doc.txt @@ -1411,8 +1412,9 @@ openedx-filters==2.1.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # ora2 -openedx-forum==0.3.8 +openedx-forum==0.4.1 # via + # -c requirements/constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt openedx-learning==0.30.2 @@ -2129,6 +2131,11 @@ types-pyyaml==6.0.12.20250915 # djangorestframework-stubs types-requests==2.32.4.20250913 # via djangorestframework-stubs +typesense==2.0.0 + # via + # -r requirements/edx/doc.txt + # -r requirements/edx/testing.txt + # openedx-forum typing-extensions==4.15.0 # via # -r requirements/edx/doc.txt @@ -2155,6 +2162,7 @@ typing-extensions==4.15.0 # referencing # snowflake-connector-python # starlette + # typesense # typing-inspection typing-inspection==0.4.2 # via diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 0cb0bf334d13..90fcb2081903 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -790,6 +790,7 @@ httpx[http2]==0.28.1 # via # -r requirements/edx/base.txt # firebase-admin + # typesense hyperframe==6.1.0 # via # -r requirements/edx/base.txt @@ -1028,8 +1029,10 @@ openedx-filters==2.1.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-forum==0.3.8 - # via -r requirements/edx/base.txt +openedx-forum==0.4.1 + # via + # -c requirements/constraints.txt + # -r requirements/edx/base.txt openedx-learning==0.30.2 # via # -c requirements/constraints.txt @@ -1503,6 +1506,10 @@ tqdm==4.67.1 # -r requirements/edx/base.txt # nltk # openai +typesense==2.0.0 + # via + # -r requirements/edx/base.txt + # openedx-forum typing-extensions==4.15.0 # via # -r requirements/edx/base.txt @@ -1520,6 +1527,7 @@ typing-extensions==4.15.0 # pyopenssl # referencing # snowflake-connector-python + # typesense # typing-inspection typing-inspection==0.4.2 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 0d87636f6689..c60f91688343 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -827,6 +827,7 @@ httpx[http2]==0.28.1 # via # -r requirements/edx/base.txt # firebase-admin + # typesense hyperframe==6.1.0 # via # -r requirements/edx/base.txt @@ -1074,8 +1075,10 @@ openedx-filters==2.1.0 # -r requirements/edx/base.txt # lti-consumer-xblock # ora2 -openedx-forum==0.3.8 - # via -r requirements/edx/base.txt +openedx-forum==0.4.1 + # via + # -c requirements/constraints.txt + # -r requirements/edx/base.txt openedx-learning==0.30.2 # via # -c requirements/constraints.txt @@ -1578,6 +1581,10 @@ tqdm==4.67.1 # -r requirements/edx/base.txt # nltk # openai +typesense==2.0.0 + # via + # -r requirements/edx/base.txt + # openedx-forum typing-extensions==4.15.0 # via # -r requirements/edx/base.txt @@ -1598,6 +1605,7 @@ typing-extensions==4.15.0 # referencing # snowflake-connector-python # starlette + # typesense # typing-inspection typing-inspection==0.4.2 # via From 241b914a191ec24658b9cde75a7114db40a5d53a Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 9 Jun 2026 00:03:09 +0000 Subject: [PATCH 6/7] fix: prevent SSRF in the Studio video download endpoint The PUT /api/contentstore/v1/videos/{course_id}/download endpoint fetched every client-supplied files[].url server-side with requests.get(url, allow_redirects=True) and returned the bytes inside the ZIP response. Because the URLs were never validated, an authenticated user with studio read access could point them at internal services or cloud metadata endpoints and exfiltrate the responses (GHSA-fpf9-9rpr-jvrx). By design these URLs are always a subset of the course's own VAL encoded_videos[].url values (the same data the video listing hands the frontend). Restrict fetches to that allowlist: build the set of legitimate URLs for the course and reject any request containing a URL outside it before any HTTP request is made. This eliminates the SSRF rather than merely narrowing it. Adds VideoDownloadViewTest (the endpoint previously had no test coverage) covering the allowed-URL success path, rejection of disallowed URLs without any outbound request, mixed allowed/disallowed batches, and the non-staff permission gate. Co-Authored-By: Claude Opus 4.8 --- .../rest_api/v1/views/tests/test_videos.py | 111 +++++++++++++++++- .../contentstore/video_storage_handlers.py | 31 +++++ 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py index 1d086cb9fda0..24655a9bd1e5 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py @@ -1,20 +1,25 @@ """ Unit tests for course settings views. """ -from unittest.mock import patch +from datetime import datetime +from unittest.mock import MagicMock, patch import ddt +import pytz from django.conf import settings from django.contrib.staticfiles.storage import staticfiles_storage from django.urls import reverse from edx_toggles.toggles import WaffleSwitch from edx_toggles.toggles.testutils import override_waffle_switch from edxval.api import ( + create_profile, + create_video, get_3rd_party_transcription_plans, get_transcript_credentials_state_for_org, get_transcript_preferences, ) from rest_framework import status +from rest_framework.test import APIClient from cms.djangoapps.contentstore.video_storage_handlers import get_all_transcript_languages from cms.djangoapps.contentstore.tests.utils import CourseTestCase @@ -135,3 +140,107 @@ def test_VideoTranscriptEnabledFlag_enabled(self): response = self.client.get(self.url) self.assertIn("is_ai_translations_enabled", response.data) self.assertTrue(response.data["is_ai_translations_enabled"]) + + + +class VideoDownloadViewTest(CourseTestCase): + """ + Tests for VideoDownloadView. + + The download endpoint fetches each requested ``files[].url`` server-side and + returns the bytes inside a zip. Those URLs must therefore be restricted to + the course's own video URLs, otherwise the endpoint is an SSRF primitive + (see GHSA-fpf9-9rpr-jvrx). + """ + + ALLOWED_URL = "http://example.com/profile1/test.mp4" + # An internal address an attacker might try to reach via SSRF. + SSRF_URL = "http://169.254.169.254/latest/meta-data/" + + def setUp(self): + super().setUp() + # reverse() with only course_id resolves to the download route (the + # usage route with the same name additionally requires edx_video_id). + self.url = reverse( + "cms.djangoapps.contentstore:v1:video_usage", + kwargs={"course_id": self.course.id}, + ) + self.api_client = APIClient() + self.api_client.force_authenticate(user=self.user) + create_profile("profile1") + create_video({ + "edx_video_id": "test-video", + "client_video_id": "test.mp4", + "duration": 42.0, + "status": "file_complete", + "courses": [str(self.course.id)], + "created": datetime.now(pytz.utc), + "encoded_videos": [ + { + "profile": "profile1", + "url": self.ALLOWED_URL, + "file_size": 1600, + "bitrate": 100, + }, + ], + }) + + @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get") + def test_download_allowed_url(self, mock_get): + """A URL that belongs to the course's videos is fetched and zipped.""" + mock_get.return_value = MagicMock( + content=b"video-bytes", + headers={"Content-Type": "video/mp4"}, + ) + response = self.api_client.put( + self.url, + data={"files": [{"url": self.ALLOWED_URL, "name": "test.mp4"}]}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) # noqa: PT009 + mock_get.assert_called_once_with(self.ALLOWED_URL, allow_redirects=True) + + @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get") + def test_rejects_url_not_belonging_to_course(self, mock_get): + """ + A URL that is not one of the course's video URLs is rejected before any + server-side request is made (SSRF protection). + """ + response = self.api_client.put( + self.url, + data={"files": [{"url": self.SSRF_URL, "name": "evil.txt"}]}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) # noqa: PT009 + mock_get.assert_not_called() + + @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get") + def test_rejects_when_any_url_is_disallowed(self, mock_get): + """ + A request mixing an allowed URL with a disallowed one is rejected + outright, without fetching the allowed URL either. + """ + response = self.api_client.put( + self.url, + data={"files": [ + {"url": self.ALLOWED_URL, "name": "test.mp4"}, + {"url": self.SSRF_URL, "name": "evil.txt"}, + ]}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) # noqa: PT009 + mock_get.assert_not_called() + + @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get") + def test_non_staff_user_denied(self, mock_get): + """A user without studio read access cannot reach the fetch path.""" + __, nonstaff_user = self.create_non_staff_authed_user_client() + client = APIClient() + client.force_authenticate(user=nonstaff_user) + response = client.put( + self.url, + data={"files": [{"url": self.ALLOWED_URL, "name": "test.mp4"}]}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) # noqa: PT009 + mock_get.assert_not_called() diff --git a/cms/djangoapps/contentstore/video_storage_handlers.py b/cms/djangoapps/contentstore/video_storage_handlers.py index 87086c9951ac..e51d54a723ea 100644 --- a/cms/djangoapps/contentstore/video_storage_handlers.py +++ b/cms/djangoapps/contentstore/video_storage_handlers.py @@ -47,6 +47,7 @@ from path import Path as path from pytz import UTC from rest_framework import status as rest_status +from rest_framework.exceptions import ValidationError from rest_framework.response import Response from tempfile import NamedTemporaryFile, mkdtemp from wsgiref.util import FileWrapper @@ -242,6 +243,29 @@ def send_zip(zip_file, size=None): return response +def get_course_video_download_urls(course_key_string): + """ + Return the set of encoded-video URLs that legitimately belong to the given + course, as recorded in VAL. + + The video download endpoint only ever needs to fetch URLs that were already + surfaced to the client by the video listing. Restricting fetches to this set + prevents server-side request forgery (SSRF) via attacker-supplied URLs. + """ + videos, __ = get_videos_for_course( + course_key_string, + VideoSortField.created, + SortDirection.desc, + None, + ) + return { + encoding['url'] + for video in videos + for encoding in video['encoded_videos'] + if encoding.get('url') + } + + def create_video_zip(course_key_string, files): """ Generates the video zip, or returns None if there was an error. @@ -254,6 +278,13 @@ def create_video_zip(course_key_string, files): root_dir = path(mkdtemp()) video_dir = root_dir + '/' + name zip_folder = None + # Only allow fetching URLs that belong to this course's videos. Anything + # else (internal services, cloud metadata endpoints, arbitrary hosts) is a + # potential SSRF target and is rejected before any request is made. + allowed_urls = get_course_video_download_urls(course_key_string) + for file in files: + if file['url'] not in allowed_urls: + raise ValidationError(f"Invalid video download url: {file['url']}") try: for file in files: url = file['url'] From 336bb682afca7b4905223da2d00c7a3758c4fbc4 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Mon, 15 Jun 2026 23:09:45 +1000 Subject: [PATCH 7/7] fix(lint): extra empty line added during merge --- .../contentstore/rest_api/v1/views/tests/test_videos.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py index 24655a9bd1e5..987294c81990 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py @@ -142,7 +142,6 @@ def test_VideoTranscriptEnabledFlag_enabled(self): self.assertTrue(response.data["is_ai_translations_enabled"]) - class VideoDownloadViewTest(CourseTestCase): """ Tests for VideoDownloadView.