Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #29606 -- Added type check for ALLOWED_HOSTS setting. #14149

Merged
merged 1 commit into from Mar 24, 2021

Conversation

AdamDonna
Copy link
Contributor

Literally just the same as other people have pushed up but having made changes for feedback that wasn't addressed. #13927

I attempted to contribute to the existing fix but there were permission issues

docs/ref/checks.txt Outdated Show resolved Hide resolved
@AdamDonna
Copy link
Contributor Author

@felixxm This is ready for another review - the test failure was just a flaky test

django/core/checks/security/base.py Outdated Show resolved Hide resolved
docs/ref/checks.txt Outdated Show resolved Hide resolved
tests/check_framework/test_security.py Outdated Show resolved Hide resolved
@AdamDonna
Copy link
Contributor Author

@tim-mccurrach This is ready for another review - I think it's addressing all the feedback across all the PRs now

Copy link
Contributor

@tim-mccurrach tim-mccurrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdamDonna
Looks good, just a few small changes requested.

Also, this PR should be a single commit. If you're able to, squash these commits to a single commit and then force push it.

django/core/checks/security/base.py Outdated Show resolved Hide resolved
docs/ref/checks.txt Outdated Show resolved Hide resolved
@AdamDonna AdamDonna force-pushed the ticket-29606-validate-allowed-hosts branch 2 times, most recently from df5cec9 to dee8a07 Compare March 23, 2021 02:41
@AdamDonna
Copy link
Contributor Author

AdamDonna commented Mar 23, 2021

Also, this PR should be a single commit. If you're able to, squash these commits to a single commit and then force push it.

Great. I've fixed up the last comments, then squashed. 👍

@felixxm felixxm changed the title Fixed #29606 -- Added system check for the type of ALLOWED_HOSTS Fixed #29606 -- Added system check for ALLOWED_HOSTS setting. Mar 23, 2021
@felixxm
Copy link
Member

felixxm commented Mar 23, 2021

@AdamDonna Thanks for updates 👍 However I don't think we need a separate system check, it should be enough to add 'ALLOWED_HOSTS' to the tuple_settings (see Tim's comment):

diff --git a/django/conf/__init__.py b/django/conf/__init__.py
index 03bf923bb2..3f387a5ffc 100644
--- a/django/conf/__init__.py
+++ b/django/conf/__init__.py
@@ -141,6 +141,7 @@ class Settings:
         mod = importlib.import_module(self.SETTINGS_MODULE)
 
         tuple_settings = (
+            "ALLOWED_HOSTS",
             "INSTALLED_APPS",
             "TEMPLATE_DIRS",
             "LOCALE_PATHS",

Checking types of elements it's not crucial, IMO.

@AdamDonna AdamDonna force-pushed the ticket-29606-validate-allowed-hosts branch from dee8a07 to fb7c876 Compare March 24, 2021 00:26
@AdamDonna
Copy link
Contributor Author

@felixxm @tim-mccurrach Thanks for your work on this so far. I've revised the implementation to be more in keeping with Tim's comment. Does any documentation need to change as a result of this?

@felixxm
Copy link
Member

felixxm commented Mar 24, 2021

@AdamDonna Thanks 👍 Docs changes are not needed.

@felixxm felixxm changed the title Fixed #29606 -- Added system check for ALLOWED_HOSTS setting. Fixed #29606 -- Added type check for ALLOWED_HOSTS setting. Mar 24, 2021
@felixxm felixxm force-pushed the ticket-29606-validate-allowed-hosts branch from fb7c876 to cdd0b21 Compare March 24, 2021 08:24
@felixxm
Copy link
Member

felixxm commented Mar 24, 2021

I added msg to tests.

@felixxm felixxm merged commit cdd0b21 into django:main Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants