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/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/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, 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()