From b6b0bf50a300e89c178865fa3acf2ae77727fed0 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Sun, 9 Aug 2026 14:45:26 -0400 Subject: [PATCH 1/5] adding checks for ovs webhook links --- learning_resources/etl/ovs.py | 90 +++++++++++++++++++++++++++++------ main/settings_course_etl.py | 11 ++++- openapi/specs/v1.yaml | 2 + webhooks/serializers.py | 72 +++++++++++++++++++++++++++- webhooks/views.py | 8 ++-- 5 files changed, 163 insertions(+), 20 deletions(-) diff --git a/learning_resources/etl/ovs.py b/learning_resources/etl/ovs.py index 35309014c9..1b4d4e918c 100644 --- a/learning_resources/etl/ovs.py +++ b/learning_resources/etl/ovs.py @@ -3,7 +3,7 @@ import logging from collections import defaultdict from collections.abc import Generator -from urllib.parse import urlencode, urlparse, urlunparse +from urllib.parse import quote, urlencode, urlparse, urlunparse import requests from django.conf import settings @@ -25,10 +25,56 @@ OVS_API_PATH = "/api/v0/public/videos/" +def allowed_media_hosts() -> set[str]: + """ + Return the set of hosts that OVS media urls are allowed to point at. + + Returns: + set of lowercased host patterns, including the OVS API host + """ + hosts = {host.lower() for host in settings.OVS_ALLOWED_MEDIA_HOSTS if host} + api_host = urlparse(settings.OVS_API_BASE_URL or "").hostname + if api_host: + hosts.add(api_host.lower()) + return hosts + + +def is_allowed_media_url(url: str | None) -> bool: + """ + Check that a url from an OVS payload points at an allowlisted host over https. + + OVS payloads (including webhook payloads, which are untrusted input) carry + absolute urls for thumbnails, streaming sources and subtitles. We fetch some + of those urls server-side and hand the rest to browsers, so a forged payload + could otherwise direct requests at an arbitrary, possibly internal, host. + + Args: + url: the url to check + + Returns: + True if the url is an https url on an allowlisted host + """ + if not url: + return False + parsed = urlparse(url) + if parsed.scheme != "https": + return False + hostname = (parsed.hostname or "").lower() + if not hostname: + return False + return any( + hostname == allowed or (allowed.startswith(".") and hostname.endswith(allowed)) + for allowed in allowed_media_hosts() + ) + + def _get_cloudfront_domain(video_data: dict) -> str | None: """ Extract the CloudFront domain from thumbnail or source URLs. + Only urls on an allowlisted host are considered, so the returned domain can + safely be used to build further urls. + Args: video_data: OVS video API response dict @@ -38,15 +84,13 @@ def _get_cloudfront_domain(video_data: dict) -> str | None: # Try thumbnails first for thumbnail in video_data.get("videothumbnail_set", []): cf_url = thumbnail.get("cloudfront_url", "") - if cf_url: - parsed = urlparse(cf_url) - return parsed.netloc + if is_allowed_media_url(cf_url): + return urlparse(cf_url).hostname # Fall back to sources for source in video_data.get("sources", []): src = source.get("src", "") - if src: - parsed = urlparse(src) - return parsed.netloc + if is_allowed_media_url(src): + return urlparse(src).hostname return None @@ -68,9 +112,12 @@ def _build_caption_urls(video_data: dict) -> list[dict]: captions = [] for subtitle in video_data.get("videosubtitle_set", []): - s3_key = subtitle.get("s3_object_key", "") + s3_key = quote(subtitle.get("s3_object_key", "").lstrip("/")) if s3_key: url = f"https://{cf_domain}/{s3_key}" + if not is_allowed_media_url(url): + log.warning("Skipping OVS caption url on disallowed host: %s", url) + continue captions.append( { "language": subtitle.get("language", "en"), @@ -93,7 +140,10 @@ def _get_cover_image_url(video_data: dict) -> str | None: """ thumbnails = video_data.get("videothumbnail_set", []) if thumbnails: - return thumbnails[0].get("cloudfront_url") + cover_url = thumbnails[0].get("cloudfront_url") + if is_allowed_media_url(cover_url): + return cover_url + log.warning("Ignoring OVS cover image url on disallowed host: %s", cover_url) return None @@ -111,8 +161,11 @@ def _get_source_url(video_data: dict) -> str | None: src = source.get("src", "") if not src: continue - if urlparse(src).path.endswith(".m3u8"): + if not urlparse(src).path.endswith(".m3u8"): + continue + if is_allowed_media_url(src): return src + log.warning("Ignoring OVS streaming source on disallowed host: %s", src) return None @@ -146,7 +199,8 @@ def _get_resource_url(video_data: dict) -> str: """ Get the user-facing URL for a video resource. - Uses cta_link if available, otherwise builds a URL from OVS_API_BASE_URL. + Uses cta_link if it points at an allowlisted host, otherwise builds a URL + from OVS_API_BASE_URL. Args: video_data: OVS video API response dict @@ -156,9 +210,11 @@ def _get_resource_url(video_data: dict) -> str: """ cta_link = video_data.get("cta_link") if cta_link: - return cta_link + if is_allowed_media_url(cta_link): + return cta_link + log.warning("Ignoring OVS cta_link on disallowed host: %s", cta_link) base_url = settings.OVS_API_BASE_URL.rstrip("/") - return f"{base_url}/videos/{video_data['key']}" + return f"{base_url}/videos/{quote(str(video_data['key']))}" def extract(*, url=None) -> Generator[dict, None, None]: @@ -263,7 +319,7 @@ def transform_collection(collection_data: dict) -> dict: "platform": PlatformType.ovs.name, "title": collection_data.get("title", ""), "description": collection_data.get("description", ""), - "url": f"{base_url}/collections/{collection_data['key']}", + "url": f"{base_url}/collections/{quote(str(collection_data['key']))}", "published": True, } @@ -313,6 +369,12 @@ def _fetch_transcript(caption_urls: list[dict]) -> str: """ for caption in caption_urls: if caption.get("language") == "en": + if not is_allowed_media_url(caption.get("url")): + log.warning( + "Refusing to fetch transcript from disallowed host: %s", + caption.get("url"), + ) + break try: result = extract_text_from_url(caption["url"], mime_type="text/vtt") if result and result.get("content"): diff --git a/main/settings_course_etl.py b/main/settings_course_etl.py index fe27d826ce..d262deea7c 100644 --- a/main/settings_course_etl.py +++ b/main/settings_course_etl.py @@ -2,7 +2,7 @@ Django settings specific to learning_resources ingestion """ -from main.envs import get_bool, get_int, get_string +from main.envs import get_bool, get_int, get_list_of_str, get_string # EDX API Credentials EDX_API_URL = get_string("EDX_API_URL", None) @@ -121,6 +121,15 @@ "OVS_API_BASE_URL", None, ) +# Hosts that OVS media urls (thumbnails, streaming sources, subtitles) may point +# at. OVS payloads arrive over a webhook, so any url in them is untrusted input: +# without an allowlist a forged payload could point our requests at an arbitrary +# (possibly internal) host. The OVS_API_BASE_URL host is always allowed. An entry +# beginning with a "." matches any subdomain of that domain. +OVS_ALLOWED_MEDIA_HOSTS = get_list_of_str( + "OVS_ALLOWED_MEDIA_HOSTS", + [".cloudfront.net"], +) # course catalog podcast etl settings OPEN_PODCAST_DATA_BRANCH = get_string("OPEN_PODCAST_DATA_BRANCH", "master") diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 72888d26bb..76d3b6b769 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -13798,9 +13798,11 @@ components: key: type: string minLength: 1 + pattern: ^[A-Za-z0-9._-]{1,255}$ video_id: type: string minLength: 1 + pattern: ^[A-Za-z0-9._-]{1,255}$ delete: type: boolean default: false diff --git a/webhooks/serializers.py b/webhooks/serializers.py index 8a5e735531..9438d77c7e 100644 --- a/webhooks/serializers.py +++ b/webhooks/serializers.py @@ -1,6 +1,13 @@ +import re + from rest_framework import serializers from learning_resources.etl.constants import ETLSource +from learning_resources.etl.ovs import is_allowed_media_url + +# OVS keys are opaque identifiers. Restrict them to characters that cannot alter +# the structure of any url or index path they get interpolated into. +OVS_KEY_REGEX = r"^[A-Za-z0-9._-]{1,255}$" class ContentFileWebHookRequestSerializer(serializers.Serializer): @@ -24,10 +31,68 @@ class OVSVideoWebhookRequestSerializer(serializers.Serializer): `delete: true`). """ - key = serializers.CharField(required=False) - video_id = serializers.CharField(required=False) + key = serializers.RegexField(OVS_KEY_REGEX, required=False) + video_id = serializers.RegexField(OVS_KEY_REGEX, required=False) delete = serializers.BooleanField(required=False, default=False) + def _nested_objects(self, field): + """ + Return the nested list of objects at `field`, requiring the right shape. + + The loader iterates these lists, so a payload of the wrong shape has to + be refused here rather than blowing up downstream. + """ + items = self.initial_data.get(field) + if items is None: + return [] + if not isinstance(items, list) or any( + not isinstance(item, dict) for item in items + ): + raise serializers.ValidationError( + {field: f"{field} must be a list of objects"} + ) + return items + + def _validate_media_urls(self): + """ + Reject upsert payloads carrying urls outside the OVS media allowlist. + + The payload is attacker-controlled input, and its urls are fetched + server-side (subtitles) or served to browsers (thumbnails, streams), so + an unrecognized host is refused rather than silently dropped. + """ + candidates = [ + ("cta_link", self.initial_data.get("cta_link")), + *[("sources", src.get("src")) for src in self._nested_objects("sources")], + *[ + ("videothumbnail_set", thumbnail.get("cloudfront_url")) + for thumbnail in self._nested_objects("videothumbnail_set") + ], + ] + # subtitle urls are built from an allowlisted domain rather than taken + # from the payload, but the list still has to have the expected shape + self._nested_objects("videosubtitle_set") + errors = { + field: f"url is not on an allowed OVS media host: {url}" + for field, url in candidates + if url and not is_allowed_media_url(url) + } + if errors: + raise serializers.ValidationError(errors) + + def _validate_collection(self): + """Validate the nested collection key, which becomes a playlist id""" + collection = self.initial_data.get("collection") or {} + if not isinstance(collection, dict): + raise serializers.ValidationError( + {"collection": "collection must be an object"} + ) + key = collection.get("key") + if key and not re.match(OVS_KEY_REGEX, str(key)): + raise serializers.ValidationError( + {"collection": "collection key contains invalid characters"} + ) + def validate(self, attrs): if attrs.get("delete"): if not attrs.get("video_id"): @@ -36,6 +101,9 @@ def validate(self, attrs): ) elif not self.initial_data.get("key"): raise serializers.ValidationError({"key": "key is required for upsert"}) + else: + self._validate_media_urls() + self._validate_collection() return attrs diff --git a/webhooks/views.py b/webhooks/views.py index 1b0efcd791..dfee99fb33 100644 --- a/webhooks/views.py +++ b/webhooks/views.py @@ -123,10 +123,12 @@ def post(self, request): payload = json.loads(request.body) except json.JSONDecodeError: return HttpResponseBadRequest("Invalid JSON format") - OVSVideoWebhookRequestSerializer(data=payload).is_valid(raise_exception=True) + serializer = OVSVideoWebhookRequestSerializer(data=payload) + serializer.is_valid(raise_exception=True) + data = serializer.validated_data - if payload.get("delete"): - video_id = payload["video_id"] + if data.get("delete"): + video_id = data["video_id"] resource = LearningResource.objects.filter( readable_id=video_id, etl_source=ETLSource.ovs.name, From 3cfb898f76a18c8271905ed71baf8998b782e57f Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Sun, 9 Aug 2026 14:45:55 -0400 Subject: [PATCH 2/5] adding tests --- learning_resources/etl/ovs_test.py | 116 ++++++++++++++++++++++++++++- webhooks/views_test.py | 116 +++++++++++++++++++++++++++++ 2 files changed, 231 insertions(+), 1 deletion(-) diff --git a/learning_resources/etl/ovs_test.py b/learning_resources/etl/ovs_test.py index 7d5d582fe3..6cbbaf8f45 100644 --- a/learning_resources/etl/ovs_test.py +++ b/learning_resources/etl/ovs_test.py @@ -2,6 +2,7 @@ import json from unittest.mock import Mock +from urllib.parse import urlparse import pytest import requests @@ -23,6 +24,7 @@ extract, get_ovs_transcripts, get_ovs_videos_for_transcripts_job, + is_allowed_media_url, transform, transform_collection, transform_video, @@ -40,8 +42,9 @@ @pytest.fixture(autouse=True) def ovs_settings(settings): - """Ensure OVS_API_BASE_URL is set for all tests in this module""" + """Ensure OVS url settings are set for all tests in this module""" settings.OVS_API_BASE_URL = OVS_TEST_BASE_URL + settings.OVS_ALLOWED_MEDIA_HOSTS = [".cloudfront.net", "example.com"] return settings @@ -686,6 +689,117 @@ def test_fetch_transcript_tika_returns_none(mocker): assert result == "" +def test_fetch_transcript_disallowed_host(mocker): + """Should not request a transcript from a host outside the allowlist""" + mock_extract = mocker.patch("learning_resources.etl.ovs.extract_text_from_url") + caption_urls = [ + {"language": "en", "url": "http://169.254.169.254/latest/meta-data/"}, + ] + assert _fetch_transcript(caption_urls) == "" + mock_extract.assert_not_called() + + +class TestMediaUrlAllowlist: + """Tests for is_allowed_media_url and the url filtering that depends on it""" + + @pytest.mark.parametrize( + ("url", "expected"), + [ + pytest.param( + "https://d1rlgptj9v7p9j.cloudfront.net/a.vtt", True, id="subdomain" + ), + pytest.param("https://example.com/a.vtt", True, id="exact_host"), + pytest.param(f"{OVS_TEST_BASE_URL}/videos/abc", True, id="ovs_api_host"), + pytest.param("http://example.com/a.vtt", False, id="not_https"), + pytest.param( + "https://evilcloudfront.net/a.vtt", False, id="suffix_lookalike" + ), + pytest.param("https://notexample.com/a.vtt", False, id="host_lookalike"), + pytest.param( + "https://example.com.evil.io/a.vtt", False, id="host_prefix_lookalike" + ), + pytest.param( + "https://example.com@evil.io/a.vtt", False, id="userinfo_confusion" + ), + pytest.param( + "http://169.254.169.254/latest/meta-data/", False, id="link_local" + ), + pytest.param("file:///etc/passwd", False, id="file_scheme"), + pytest.param("", False, id="empty"), + pytest.param(None, False, id="none"), + ], + ) + def test_is_allowed_media_url(self, url, expected): + """Only https urls on allowlisted hosts are permitted""" + assert is_allowed_media_url(url) is expected + + def test_source_url_on_disallowed_host_ignored(self): + """A streaming source on an unknown host is not used""" + assert ( + _get_source_url({"sources": [{"src": "https://evil.io/video__index.m3u8"}]}) + is None + ) + + def test_cover_image_on_disallowed_host_ignored(self): + """A thumbnail on an unknown host is not used""" + assert ( + _get_cover_image_url( + {"videothumbnail_set": [{"cloudfront_url": "https://evil.io/t.jpg"}]} + ) + is None + ) + + def test_caption_urls_require_allowed_domain(self): + """Captions are dropped when no allowlisted domain can be determined""" + assert ( + _build_caption_urls( + { + "videothumbnail_set": [ + {"cloudfront_url": "https://evil.io/t.jpg"}, + ], + "sources": [{"src": "https://evil.io/video__index.m3u8"}], + "videosubtitle_set": [ + {"s3_object_key": "subtitles/a.vtt", "language": "en"} + ], + } + ) + == [] + ) + + def test_caption_urls_escape_object_key(self): + """A subtitle key cannot break out of the allowlisted host""" + captions = _build_caption_urls( + { + "videothumbnail_set": [ + {"cloudfront_url": "https://abc.cloudfront.net/t.jpg"}, + ], + "videosubtitle_set": [ + {"s3_object_key": "/../@evil.io/a.vtt", "language": "en"} + ], + } + ) + assert len(captions) == 1 + assert urlparse(captions[0]["url"]).hostname == "abc.cloudfront.net" + + def test_resource_url_ignores_disallowed_cta_link(self, settings): + """cta_link pointing off-allowlist falls back to the OVS url""" + settings.OVS_API_BASE_URL = OVS_TEST_BASE_URL + video = {"key": "abc123", "cta_link": "https://evil.io/watch/abc123"} + assert _get_resource_url(video) == f"{OVS_TEST_BASE_URL}/videos/abc123" + + def test_transform_video_skips_disallowed_source(self): + """A video whose only source is off-allowlist is not transformed""" + assert ( + transform_video( + { + "key": "abc123", + "sources": [{"src": "https://evil.io/video__index.m3u8"}], + } + ) + is None + ) + + def test_filters_ovs_videos_without_transcripts(ovs_platform): """Should return OVS videos with empty transcripts""" video = VideoFactory.create( diff --git a/webhooks/views_test.py b/webhooks/views_test.py index bf777d7152..28ffcf93e5 100644 --- a/webhooks/views_test.py +++ b/webhooks/views_test.py @@ -515,3 +515,119 @@ def test_ovs_video_webhook_missing_key(settings, client): headers={"X-MITLearn-Signature": get_secret(payload, settings)}, ) assert response.status_code == 400 + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("field", "mutation"), + [ + pytest.param( + "sources", + lambda payload: payload["sources"][0].update( + {"src": "http://169.254.169.254/latest/meta-data/video__index.m3u8"} + ), + id="streaming_source", + ), + pytest.param( + "videothumbnail_set", + lambda payload: payload["videothumbnail_set"][0].update( + {"cloudfront_url": "https://evil.io/thumb.jpg"} + ), + id="thumbnail", + ), + pytest.param( + "cta_link", + lambda payload: payload.update({"cta_link": "https://evil.io/watch"}), + id="cta_link", + ), + ], +) +def test_ovs_video_webhook_rejects_disallowed_urls( + settings, client, mocker, field, mutation +): + """A payload url outside the OVS media allowlist is rejected, not fetched.""" + mocker.patch("webhooks.views.clear_views_cache") + mock_load = mocker.patch("webhooks.views.load_ovs_video_from_webhook") + payload = _ovs_payload(key="ssrf") + mutation(payload) + + url = reverse("webhooks:v1:ovs_video_webhook") + response = client.post( + url, + data=json.dumps(payload), + content_type="application/json", + headers={"X-MITLearn-Signature": get_secret(payload, settings)}, + ) + assert response.status_code == 400 + assert field in response.json() + mock_load.assert_not_called() + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "payload_update", + [ + pytest.param({"key": "abc/../../evil"}, id="key"), + pytest.param({"collection": {"key": "col/../evil"}}, id="collection_key"), + ], +) +def test_ovs_video_webhook_rejects_structural_keys( + settings, client, mocker, payload_update +): + """Keys that could alter a url or index path are rejected.""" + mocker.patch("webhooks.views.clear_views_cache") + mock_load = mocker.patch("webhooks.views.load_ovs_video_from_webhook") + payload = {**_ovs_payload(), **payload_update} + + url = reverse("webhooks:v1:ovs_video_webhook") + response = client.post( + url, + data=json.dumps(payload), + content_type="application/json", + headers={"X-MITLearn-Signature": get_secret(payload, settings)}, + ) + assert response.status_code == 400 + mock_load.assert_not_called() + + +@pytest.mark.django_db +def test_ovs_video_webhook_delete_rejects_structural_video_id(settings, client, mocker): + """A delete payload with a structural video_id is rejected.""" + mocker.patch("webhooks.views.clear_views_cache") + mock_delete = mocker.patch("webhooks.views.resource_delete_actions") + + payload = {"video_id": "../../evil", "delete": True} + url = reverse("webhooks:v1:ovs_video_webhook") + response = client.post( + url, + data=json.dumps(payload), + content_type="application/json", + headers={"X-MITLearn-Signature": get_secret(payload, settings)}, + ) + assert response.status_code == 400 + mock_delete.assert_not_called() + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "payload", + [ + pytest.param([1, 2, 3], id="list_body"), + pytest.param({"key": "abc", "sources": 5}, id="sources_not_a_list"), + pytest.param({"key": "abc", "videothumbnail_set": "x"}, id="thumbnails_string"), + pytest.param({"key": "abc", "collection": "x"}, id="collection_not_object"), + ], +) +def test_ovs_video_webhook_malformed_payload(settings, client, mocker, payload): + """Structurally invalid payloads are rejected with a 400, not a 500.""" + mocker.patch("webhooks.views.clear_views_cache") + mock_load = mocker.patch("webhooks.views.load_ovs_video_from_webhook") + url = reverse("webhooks:v1:ovs_video_webhook") + response = client.post( + url, + data=json.dumps(payload), + content_type="application/json", + headers={"X-MITLearn-Signature": get_secret(payload, settings)}, + ) + assert response.status_code == 400 + mock_load.assert_not_called() From d64e91d797d3793889e62ac644b554f61fa9a902 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Mon, 10 Aug 2026 10:59:11 -0400 Subject: [PATCH 3/5] default OVS_ALLOWED_MEDIA_HOSTS to empty list --- main/settings_course_etl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/settings_course_etl.py b/main/settings_course_etl.py index d262deea7c..ddd31cd9f2 100644 --- a/main/settings_course_etl.py +++ b/main/settings_course_etl.py @@ -128,7 +128,7 @@ # beginning with a "." matches any subdomain of that domain. OVS_ALLOWED_MEDIA_HOSTS = get_list_of_str( "OVS_ALLOWED_MEDIA_HOSTS", - [".cloudfront.net"], + [], ) # course catalog podcast etl settings From 15952ac85332927641a9615f9a9cdf989c42886c Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Mon, 10 Aug 2026 11:17:18 -0400 Subject: [PATCH 4/5] fix test and fix edge case --- learning_resources/etl/ovs.py | 97 ++++++++++++++++++++++-------- learning_resources/etl/ovs_test.py | 60 ++++++++++++++++++ webhooks/views_test.py | 37 ++++++++++++ 3 files changed, 169 insertions(+), 25 deletions(-) diff --git a/learning_resources/etl/ovs.py b/learning_resources/etl/ovs.py index 1b4d4e918c..315db12a8b 100644 --- a/learning_resources/etl/ovs.py +++ b/learning_resources/etl/ovs.py @@ -3,7 +3,7 @@ import logging from collections import defaultdict from collections.abc import Generator -from urllib.parse import quote, urlencode, urlparse, urlunparse +from urllib.parse import ParseResult, quote, urlencode, urlparse, urlunparse import requests from django.conf import settings @@ -39,7 +39,59 @@ def allowed_media_hosts() -> set[str]: return hosts -def is_allowed_media_url(url: str | None) -> bool: +def parse_media_url(url: object) -> ParseResult | None: + """ + Parse a url from an OVS payload, returning None if it cannot be trusted. + + Payload values are untrusted, so they are not necessarily strings and not + necessarily parseable (urlparse raises on, for example, an unterminated ipv6 + host). A netloc containing a backslash or userinfo is refused outright: + urllib reads `https://evil.io\\@allowed.example` as a url on + allowed.example, while browsers read it as a url on evil.io, so comparing + urllib's hostname against the allowlist would not mean what it appears to. + + Args: + url: the value to parse + + Returns: + the parsed url, or None if it is unusable + """ + if not isinstance(url, str) or not url: + return None + try: + parsed = urlparse(url) + except ValueError: + return None + if "\\" in parsed.netloc or "@" in parsed.netloc: + return None + return parsed + + +def allowed_media_hostname(url: object) -> str | None: + """ + Return the hostname of a url that points at an allowlisted OVS media host. + + Args: + url: the url to check + + Returns: + the lowercased hostname, or None if the url is not allowed + """ + parsed = parse_media_url(url) + if parsed is None or parsed.scheme != "https": + return None + hostname = (parsed.hostname or "").lower() + if not hostname: + return None + if any( + hostname == allowed or (allowed.startswith(".") and hostname.endswith(allowed)) + for allowed in allowed_media_hosts() + ): + return hostname + return None + + +def is_allowed_media_url(url: object) -> bool: """ Check that a url from an OVS payload points at an allowlisted host over https. @@ -54,18 +106,7 @@ def is_allowed_media_url(url: str | None) -> bool: Returns: True if the url is an https url on an allowlisted host """ - if not url: - return False - parsed = urlparse(url) - if parsed.scheme != "https": - return False - hostname = (parsed.hostname or "").lower() - if not hostname: - return False - return any( - hostname == allowed or (allowed.startswith(".") and hostname.endswith(allowed)) - for allowed in allowed_media_hosts() - ) + return allowed_media_hostname(url) is not None def _get_cloudfront_domain(video_data: dict) -> str | None: @@ -83,14 +124,14 @@ def _get_cloudfront_domain(video_data: dict) -> str | None: """ # Try thumbnails first for thumbnail in video_data.get("videothumbnail_set", []): - cf_url = thumbnail.get("cloudfront_url", "") - if is_allowed_media_url(cf_url): - return urlparse(cf_url).hostname + hostname = allowed_media_hostname(thumbnail.get("cloudfront_url")) + if hostname: + return hostname # Fall back to sources for source in video_data.get("sources", []): - src = source.get("src", "") - if is_allowed_media_url(src): - return urlparse(src).hostname + hostname = allowed_media_hostname(source.get("src")) + if hostname: + return hostname return None @@ -112,7 +153,10 @@ def _build_caption_urls(video_data: dict) -> list[dict]: captions = [] for subtitle in video_data.get("videosubtitle_set", []): - s3_key = quote(subtitle.get("s3_object_key", "").lstrip("/")) + s3_object_key = subtitle.get("s3_object_key") + if not isinstance(s3_object_key, str): + continue + s3_key = quote(s3_object_key.lstrip("/")) if s3_key: url = f"https://{cf_domain}/{s3_key}" if not is_allowed_media_url(url): @@ -158,14 +202,17 @@ def _get_source_url(video_data: dict) -> str | None: HLS streaming URL or None """ for source in video_data.get("sources", []): - src = source.get("src", "") + src = source.get("src") if not src: continue - if not urlparse(src).path.endswith(".m3u8"): + # parse before looking at the path: an unparseable or non-string src is + # not a url we can reason about at all + parsed = parse_media_url(src) + if parsed is None or not is_allowed_media_url(src): + log.warning("Ignoring OVS streaming source on disallowed host: %s", src) continue - if is_allowed_media_url(src): + if parsed.path.endswith(".m3u8"): return src - log.warning("Ignoring OVS streaming source on disallowed host: %s", src) return None diff --git a/learning_resources/etl/ovs_test.py b/learning_resources/etl/ovs_test.py index 6cbbaf8f45..1b86455e5d 100644 --- a/learning_resources/etl/ovs_test.py +++ b/learning_resources/etl/ovs_test.py @@ -727,12 +727,72 @@ class TestMediaUrlAllowlist: pytest.param("file:///etc/passwd", False, id="file_scheme"), pytest.param("", False, id="empty"), pytest.param(None, False, id="none"), + # urllib reads the host of these as example.com, browsers read it + # as evil.io, so they must not be treated as allowlisted + pytest.param( + "https://evil.io\\@example.com/a.vtt", False, id="backslash_userinfo" + ), + pytest.param("https://evil.io\\.example.com/a.vtt", False, id="backslash"), + pytest.param("https:/\\evil.io/a.vtt", False, id="leading_backslash"), + # malformed urls that urlparse refuses to parse + pytest.param("https://[", False, id="unterminated_ipv6"), + pytest.param("https://[::1]bad]/a.vtt", False, id="bad_ipv6"), + # payload values are not guaranteed to be strings + pytest.param(123, False, id="int"), + pytest.param({"url": "https://example.com"}, False, id="dict"), + pytest.param(["https://example.com"], False, id="list"), + pytest.param(True, False, id="bool"), ], ) def test_is_allowed_media_url(self, url, expected): """Only https urls on allowlisted hosts are permitted""" assert is_allowed_media_url(url) is expected + @pytest.mark.parametrize( + "url", + [ + "https://[", + 123, + {"url": "https://example.com"}, + "https://evil.io\\@example.com/video__index.m3u8", + ], + ) + def test_hostile_urls_do_not_crash_transform(self, url): + """A malformed or non-string url is rejected rather than raising""" + video_data = { + "key": "abc123", + "cta_link": url, + "sources": [{"src": url}, {"src": "https://example.com/v__index.m3u8"}], + "videothumbnail_set": [{"cloudfront_url": url}], + "videosubtitle_set": [{"s3_object_key": "subtitles/a.vtt"}], + } + transformed = transform_video(video_data) + assert transformed["url"] == f"{OVS_TEST_BASE_URL}/videos/abc123" + assert transformed["video"]["streaming_url"] == ( + "https://example.com/v__index.m3u8" + ) + assert transformed["image"] is None + assert urlparse(transformed["video"]["caption_urls"][0]["url"]).hostname == ( + "example.com" + ) + + def test_caption_urls_skip_non_string_object_key(self): + """A subtitle whose s3_object_key is not a string is skipped""" + assert ( + _build_caption_urls( + { + "videothumbnail_set": [ + {"cloudfront_url": "https://abc.cloudfront.net/t.jpg"}, + ], + "videosubtitle_set": [ + {"s3_object_key": 5, "language": "en"}, + {"s3_object_key": None, "language": "fr"}, + ], + } + ) + == [] + ) + def test_source_url_on_disallowed_host_ignored(self): """A streaming source on an unknown host is not used""" assert ( diff --git a/webhooks/views_test.py b/webhooks/views_test.py index 28ffcf93e5..cb24298e40 100644 --- a/webhooks/views_test.py +++ b/webhooks/views_test.py @@ -259,6 +259,7 @@ def test_content_file_webhook_view_invalid_json(settings, client): @pytest.fixture def ovs_platform(settings): settings.OVS_API_BASE_URL = "https://video.odl.mit.edu" + settings.OVS_ALLOWED_MEDIA_HOSTS = [".cloudfront.net"] return LearningResourcePlatformFactory.create(code=PlatformType.ovs.name) @@ -563,6 +564,42 @@ def test_ovs_video_webhook_rejects_disallowed_urls( mock_load.assert_not_called() +@pytest.mark.django_db +@pytest.mark.parametrize( + "hostile_url", + [ + # urllib reads the host as du3yhovcx8dht.cloudfront.net, browsers read + # it as evil.io + pytest.param( + "https://evil.io\\@du3yhovcx8dht.cloudfront.net/x.jpg", id="backslash" + ), + pytest.param("https://[", id="unparseable"), + pytest.param(5, id="not_a_string"), + pytest.param( + {"src": "https://du3yhovcx8dht.cloudfront.net/x.jpg"}, id="object" + ), + ], +) +def test_ovs_video_webhook_rejects_hostile_urls(settings, client, mocker, hostile_url): + """Urls urllib and browsers disagree about are rejected with a 400, not a 500.""" + mocker.patch("webhooks.views.clear_views_cache") + mock_load = mocker.patch("webhooks.views.load_ovs_video_from_webhook") + settings.OVS_ALLOWED_MEDIA_HOSTS = [".cloudfront.net"] + payload = _ovs_payload(key="hostile") + payload["cta_link"] = hostile_url + + url = reverse("webhooks:v1:ovs_video_webhook") + response = client.post( + url, + data=json.dumps(payload), + content_type="application/json", + headers={"X-MITLearn-Signature": get_secret(payload, settings)}, + ) + assert response.status_code == 400 + assert "cta_link" in response.json() + mock_load.assert_not_called() + + @pytest.mark.django_db @pytest.mark.parametrize( "payload_update", From 0fe0058776108e199b41d9b76f48202c846441c5 Mon Sep 17 00:00:00 2001 From: shankar ambady Date: Mon, 10 Aug 2026 16:12:31 -0400 Subject: [PATCH 5/5] adding example to backend env --- env/backend.local.example.env | 3 +++ 1 file changed, 3 insertions(+) diff --git a/env/backend.local.example.env b/env/backend.local.example.env index bdc13bc00d..01f723f11f 100644 --- a/env/backend.local.example.env +++ b/env/backend.local.example.env @@ -58,3 +58,6 @@ CELERY_BEAT_DISABLED=False # Assign the bucket here if you want to enable edx contentfile ingestion locally COURSE_ARCHIVE_BUCKET_NAME= + +# Hosts that OVS media urls may point at +OVS_ALLOWED_MEDIA_HOSTS=['.cloudfront.net']