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/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..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 @@ -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,106 @@ 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'] diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index f7ac3dd2b65d..07815837b077 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -420,6 +420,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: 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..9bafea96fcf0 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,54 @@ 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) + + +class TimestampAndNonceValidatorTest(CacheIsolationMixin, TestCase): + """ + 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'): + return self.validator.validate_timestamp_and_nonce( + client_key, str(timestamp), nonce, request=None, + ) + + def test_valid_timestamp_and_new_nonce(self): + assert self._call(FIXED_TIMESTAMP, 'nonce-a') + + def test_stale_timestamp_rejected(self): + assert not self._call(FIXED_TIMESTAMP - 301, 'nonce-b') + + def test_future_timestamp_rejected(self): + assert not self._call(FIXED_TIMESTAMP + 301, 'nonce-c') + + 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): + assert self._call(FIXED_TIMESTAMP, 'nonce-e') + assert not self._call(FIXED_TIMESTAMP, 'nonce-e') + + 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): + # 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') 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