fix: block SSRF in SAML metadata URL fetching#250
Merged
Conversation
Adds `validate_saml_metadata_url()` to third_party_auth utils, which enforces HTTPS and blocks loopback, link-local (including cloud metadata endpoints like 169.254.169.254), and reserved IP addresses. RFC 1918 private ranges are blocked by default and can be opted out via `SAML_METADATA_URL_ALLOW_PRIVATE_IPS = True` for deployments where the SAML IdP lives on the same private network. Calls the validator in `fetch_saml_metadata()` before `requests.get()`, also adds a 30s request timeout and removes the previous non-enforcing HTTP warning. Addresses the platform-side fetch path described in: GHSA-328g-7h4g-r2m9 Note: the primary exploit path (`sync_provider_data` endpoint) now lives in edx-enterprise following the migration documented in docs/decisions/0025-saml-admin-views-in-enterprise-plugin.rst and will need a corresponding fix there. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Backports an upstream security patch intended to reduce SSRF risk when fetching SAML IdP metadata, and includes a prerequisite backport migrating UTC handling from pytz to zoneinfo for compatibility with upstream.
Changes:
- Add
validate_saml_metadata_urland enforce it in the SAML metadata refresh task. - Introduce
SAML_METADATA_URL_ALLOW_PRIVATE_IPSto optionally permit RFC1918/ULA metadata URLs in private-network deployments. - Replace many
pytzUTC usages withZoneInfo("UTC")across app code and tests.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openedx/envs/common.py | Adds the new operator setting controlling whether private IP metadata URLs are allowed. |
| common/djangoapps/third_party_auth/utils.py | Introduces URL validation logic for SAML metadata fetching; migrates UTC timestamp parsing to ZoneInfo. |
| common/djangoapps/third_party_auth/tasks.py | Calls URL validation before fetching metadata and adds request timeout. |
| common/djangoapps/third_party_auth/tests/test_utils.py | Adds unit tests covering URL validation behavior and the allow-private setting. |
| common/djangoapps/util/file.py | Migrates timestamp generation to ZoneInfo("UTC"). |
| common/djangoapps/util/tests/test_file.py | Updates tests to use ZoneInfo("UTC") instead of pytz. |
| common/djangoapps/util/tests/test_date_utils.py | Updates date util tests to use ZoneInfo("UTC"). |
| common/djangoapps/track/tests/test_util.py | Updates JSON encoder test datetimes to ZoneInfo("UTC"). |
| common/djangoapps/track/tests/init.py | Updates frozen time constant to ZoneInfo("UTC"). |
| common/djangoapps/third_party_auth/tests/test_pipeline_integration.py | Updates test datetime construction to ZoneInfo("UTC"). |
| common/djangoapps/student/views/management.py | Replaces pytz.UTC now-calls with ZoneInfo("UTC"). |
| common/djangoapps/student/views/dashboard.py | Replaces UTC time calculations with ZoneInfo("UTC"). |
| common/djangoapps/student/tests/tests.py | Updates multiple student tests to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_views.py | Updates datetime construction in view tests to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_refunds.py | Updates refund-related time logic in tests to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_recent_enrollments.py | Updates enrollment created timestamp to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_models.py | Updates model tests to use ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_credit.py | Updates credit deadline test to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_certificates.py | Updates certificate test constants/comparisons to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/test_admin_views.py | Updates admin view tests’ datetime usage to ZoneInfo("UTC"). |
| common/djangoapps/student/tests/factories.py | Updates factory default datetimes to ZoneInfo("UTC"). |
| common/djangoapps/student/models_api.py | Updates audit metadata timestamps to ZoneInfo("UTC"). |
| common/djangoapps/student/models/user.py | Updates several UTC “now” usages to ZoneInfo("UTC"). |
| common/djangoapps/student/models/course_enrollment.py | Updates default tz args and multiple “now” comparisons to ZoneInfo("UTC"). |
| common/djangoapps/student/management/commands/assigngroups.py | Updates management command timestamp to ZoneInfo("UTC"). |
| common/djangoapps/entitlements/tests/test_tasks.py | Updates entitlement test times to ZoneInfo("UTC"). |
| common/djangoapps/entitlements/rest_api/v1/tests/test_views.py | Updates entitlement API tests to ZoneInfo("UTC"). |
| common/djangoapps/course_modes/tests/test_views.py | Updates course mode view tests to ZoneInfo("UTC"). |
| common/djangoapps/course_modes/tests/test_signals.py | Updates course mode signal test time to ZoneInfo("UTC"). |
| common/djangoapps/course_modes/tests/test_admin.py | Updates admin form test deadline to ZoneInfo("UTC"). |
| common/djangoapps/course_modes/admin.py | Replaces pytz timezone usage with ZoneInfo in the course modes admin form. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous two commits are patches cherry-picked from upstream, but here in the 2U fork of platform it generates weird lint errors we do not understand: ``` openedx/envs/common.py:1588:0: E7670: setting annotation (SAML_METADATA_URL_ALLOW_PRIVATE_IPS) cannot have a boolean value (setting-boolean-default-value) ``` I thought maybe it was related to us running an outdated version of edx-lint (5.6.0 vs 6.0.0 in upstream platform) but upgrading did not fix the errors.
Member
Author
|
Added two more commits to fix lint errors: commit a3dba9481227ec20e9d25c68acba3308d9ce70e7
Author: Troy Sankey <tsankey@2u.com>
Date: Fri Apr 24 11:34:18 2026 -0700
style: make long line shorter to fix lint error
Upstream patch "fix: block SSRF in SAML metadata URL fetching"
introduced a long line. How they didn't catch this upstream, I don't
know.
diff --git a/common/djangoapps/third_party_auth/tasks.py b/common/djangoapps/third_party_auth/tasks.py
index a950d35582..7e61878ef7 100644
--- a/common/djangoapps/third_party_auth/tasks.py
+++ b/common/djangoapps/third_party_auth/tasks.py
@@ -97,7 +97,13 @@ def fetch_saml_metadata():
num_updated += 1
else:
log.info(f"→ Updated existing SAMLProviderData. Nothing has changed for entityID {entity_id}")
- except (exceptions.SSLError, exceptions.HTTPError, exceptions.RequestException, MetadataParseError, SAMLMetadataURLError) as error:
+ except (
+ exceptions.SSLError,
+ exceptions.HTTPError,
+ exceptions.RequestException,
+ MetadataParseError,
+ SAMLMetadataURLError,
+ ) as error:
# Catch and process exception in case of errors during fetching and processing saml metadata.
# Here is a description of each exception.
# SSLError is raised in case of errors caused by SSL (e.g. SSL cer verification failure etc.)
commit aa3c6408be1b29beaf5485deef388e3a880a0f8f
Author: Troy Sankey <tsankey@2u.com>
Date: Fri Apr 24 11:28:19 2026 -0700
temp: temporarily disable pylint issue which doesn't happen upstream
The previous two commits are patches cherry-picked from upstream, but
here in the 2U fork of platform it generates weird lint errors we do not
understand:
```
openedx/envs/common.py:1588:0: E7670: setting annotation (SAML_METADATA_URL_ALLOW_PRIVATE_IPS) cannot have a boolean value (setting-boolean-default-value)
```
I thought maybe it was related to us running an outdated version of
edx-lint (5.6.0 vs 6.0.0 in upstream platform) but upgrading did not
fix the errors.
diff --git a/openedx/envs/common.py b/openedx/envs/common.py
index 0a554e33c3..2df6f438d0 100644
--- a/openedx/envs/common.py
+++ b/openedx/envs/common.py
@@ -1585,6 +1585,7 @@ SOCIAL_AUTH_SAML_SP_PUBLIC_CERT = ""
SOCIAL_AUTH_SAML_SP_PRIVATE_KEY_DICT = {}
SOCIAL_AUTH_SAML_SP_PUBLIC_CERT_DICT = {}
+# pylint: disable=setting-boolean-default-value
# .. setting_name: SAML_METADATA_URL_ALLOW_PRIVATE_IPS
# .. setting_default: False
# .. setting_description: When False (the default), fetching SAML metadata from |
Upstream patch "fix: block SSRF in SAML metadata URL fetching" introduced a long line. How they didn't catch this upstream, I don't know.
c1173f6 to
a3dba94
Compare
Member
Author
|
Note for posterity: I only cherry-picked ONE of multiple pytz->ZoneInfo migration commits:
In retrospect I wouldn't have cherry-picked it but tests take so long to run I want to just roll with it. |
iloveagent57
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a backport of upstream security patch:
In order to apply the patch cleanly, I first needed to cherry-pick another unrelated upstream commit to migrate from the deprecated
pytztoZoneInfo: