fix: Updated settings.py to dynamically populate ALLOWED_HOSTS using domains from the Django sites. #1384

amirtds · 2023-11-15T22:42:11Z

Change description

We received a security report highlighting a Host Header Injection vulnerability due to the use of a wildcard '*' in our ALLOWED_HOSTS setting. This configuration could lead to open redirects and other security risks.

I have modified settings.py to dynamically construct the ALLOWED_HOSTS list using domain names from our Django sites to ensures that only valid domains are served.

Changes:

Removed the wildcard '*' from ALLOWED_HOSTS.
Populated ALLOWED_HOSTS with domain names fetched from the Site model.
Ensured ENV_TOKENS['LMS_BASE'] and FEATURES['PREVIEW_LMS_BASE'] are included in ALLOWED_HOSTS if they are valid.
Added checks to prevent empty strings in ALLOWED_HOSTS.

Type of change

Bug fix (fixes an issue)
New feature (adds functionality)

Related issues

Fix #1

Checklists

Development

Lint rules pass locally
Application changes have been tested thoroughly
Automated tests covering modified code pass

Security

Security impact of change has been considered
Code follows company security practices and guidelines

Code review

Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
"Ready for review" label attached and reviewers assigned
Changes have been reviewed by at least one other contributor
Pull request linked to task tracker where applicable

bryanlandia · 2023-11-16T00:18:51Z

lms/envs/production.py

-    FEATURES['PREVIEW_LMS_BASE'],
+# Fetching all domain names from the Site model
+site_domains = [site.domain for site in Site.objects.all()]
+


@amirtds We will also need to cover custom domains

@bryanlandia I think the Sites has the custom domain too. I checked cockroach labs domain and could find it there. If not we can load it from somewhere else

Oh, right, the code actually switches Site.domain to be university.cockroachlabs.com and then sets alt domain to the original Tahoe one, but it's good to cover both in case some custom JS or something somewhere still uses the orig .tahoe.appsembler.com domain

bryanlandia · 2023-11-16T00:20:53Z

lms/envs/production.py

@@ -27,7 +27,9 @@
 import yaml
 from corsheaders.defaults import default_headers as corsheaders_default_headers
 from django.core.exceptions import ImproperlyConfigured
+from django.contrib.sites.models import Site


Will apps have loaded by this point? I don't think so. We might need to do this in in an app.ready()

@bryanlandia Moved it to openedx -> core -> djangoapps -> appsembler -> sites -> apps.py. Let me know wdyt

bryanlandia

I think this change is also needed in cms/envs/production.py
and we could include the alternative domains

openedx/core/djangoapps/appsembler/sites/apps.py

bryanlandia · 2023-12-11T22:18:01Z

@amirtds

Looks like the Docker build for checks is failing because py2neo is now End of Life and there are no longer any releases in GitHub for https://github.com/technige/py2neo

We'll need to update in another PR first. Maybe been fixed upstream so will check

bryanlandia · 2023-12-11T22:21:16Z

Yes will cherrypick openedx@1db6867

bryanlandia · 2023-12-12T00:42:57Z

Waiting on merge to main of #1387

amirtds · 2024-01-27T22:09:47Z

Hi @bryanlandia I added same settings for CMS as well, could you please take a look when you have some time

…domains from the Django sites model

need to allow requests from customer custom domains

github-actions · 2024-01-29T18:00:24Z

Checking git merge conflicts against https://github.com/edx/edx-platform.git

Comparing with	`open-release/nutmeg.master`
Benchmark conflicts with `main`	299
Current conflicts	299
Summary	Good work! No added conflicts.

Comparing with	`master`
Benchmark conflicts with `main`	318
Current conflicts	318
Summary	Good work! No added conflicts.

amirtds requested a review from bryanlandia November 15, 2023 22:42