Validate the type of ALLOWED_HOSTS

[rafis]

Python has soft behavior for conducting iteration process over lists and over strings making them look the same:

    for char_or_item in str_or_list:
        -- `char_or_item` can be character or list item

It would be better if it would have more strict behavior, for example,

    for char in some_str.chars():
        -- now `char` can be only of string type and `list` class would not have `chars` method

and for list

    for item in some_list.list_items():
        -- `string` class would not have `list_items` method

This soft behavior usually leads to many nasty bugs to appear. Our two software engineers from our team wasted about 1 hour debugging the issue with ALLOWED_HOSTS being initialized with string in local_settings.py which is included at the end of settings.py. Django was matching each separate character of ALLOWED_HOSTS string against the "Host:" header from an incoming HTTP request.

An obvious self-suggesting solution is to add a new system check that will check the type of ALLOWED_HOSTS if it is string or not and notify the developer about possible improper configuration. I think blacklist checking (string or not) is more appropiate here, but I can be wrong.

[Tim Graham]

​PR

I'm not sure if a system check is the way to go. The deployment checks must be run manually manage.py check --deploy. The developer might not do that as part of a debugging workflow.

[Adam Johnson]

Although it may or may not have helped, I believe in general we should check as much configuration as possible up front. Another error I myself have made with ALLOWED_HOSTS is trying "*.example.com" instead of ".example.com" (confusion with "*" being an allowed value) - this (or in general valid/invalid characters?) could also be checked for.

[Tim Graham]

I'm not opposed to some validation, my suggestion was merely that maybe we could be more helpful than a system check.

[Nick Mavrakis]

Sorry for the interfere but I would like to add my opinion on this. If this New Feature is added, then proportionate validations should apply on other settings.py variables (such as INTERNAL_IPS to be a list of valid IPs, STATIC_ROOT to be a string of valid pathname etc). See where it's going? I am not opposed on this improvement but if it's going to happen, IMO it should happen if not on the whole ​settings list, at least on most of them.
Best regards, Nick

[Octavio Peri]

Hi, I've assigned this ticket to myself and would like to check my idea with you before writing any code.

I'm not sure about what Nick Mavrakis stated, maybe that'd be a good idea.

But sticking to this ticket, I believe the best way to proceed is checking the setting is both iterable and not a str.
I would do so in two places:
1 - In runserver, where there's already one check related to ALLOWED_HOSTS
2 - As a deployment check, that would raise an error if the constraints are not satisfied.

If you could confirm this would be the way to go, I'd be more than happy to add them in a PR.

[Adam Johnson]

It needs only be in one place, as a system check. runserver runs system checks at startup already. IMO it should not be a deployment check, as ALLOWED_HOSTS affects use of runserver locally too if DEBUG = False.

[Akshat Dixit]

Hello, I am Akshat, new to this organisation and I would like to contribute on this bug. I do understand what rafis is trying to say here, but I don't understand how to proceed with fixing with this issue. Can someone tell me what files need to be modified or how to figure that out.

[Marius Räsener]

Wouldn't it be best to check if the entries in ALLOWED_HOSTS are actually valid hosts?
So either IPv4/IPv6 Addresses or some dot separated string with valid characters.

Maybe I didn't understand how the bug appeared. Or was it something like:

# wrong
ALLOWED_HOSTS="127.0.0.1"

vs.

ALLOWED_HOSTS=["127.0.0.1",]

also, doesn't work a tuple too?

thx for any hints giving me a better understanding...

[Adam Johnson]

The validation basically only needs to check it's an iterable of strings.

Checking that everything looks like a hostname is very complicated - we also support wildcards. And it would probably break for someone out there using a proxy server or middleware that adds something not-quite-host-looking in the Host header.

[Marius Räsener]

ok, so here's what I came up with for the check:

def check_allowedhosts(ALLOWED_HOSTS):
    if isinstance(ALLOWED_HOSTS,(list,tuple)):
        return all(isinstance(element,str) for element in ALLOWED_HOSTS)
    else:
        return False

for this use-case all([]) returning True isn't really helpful, so the if else clause is needed I think ... but of course I'm happy to get hints for a better implementation.

Besides, where would I put the check and the test?

[Adam Johnson]

See the PR linked in comment #1 for an intial implementation.

[Marius Räsener]

I didn't see the PR earlier, thank you for pointing to it.

Alright, so I could recreate or copy the PR, but there are things still open to discuss. Can someone point me in the right direction what Tim Graham is referring to?
Like how could this be more helpful than a system check - maybe something that's hooked into a test run?

(for a better understanding, I'm trying since yesterday my luck with "easy picking" issues to contribute something to Django - so please keep in mind I'm a beginner.)

[Adam Johnson]

Personally I think a system check would be fine. We point users to run them during deployment in the deplyoment checklist: ​https://docs.djangoproject.com/en/3.1/howto/deployment/checklist/

[Marius Räsener]

well, having an app running in production for several years now, I never knew this exists :P
but, whatever - that's my responsibility.
Anyways, I don't have a strong opinion on this, I'd just help closing this issue since I want to be some kind of contributor and head to the next issue.

So, what needs to be done to merge this and close the issue?

[Mariusz Felisiak]

Replying to Marius Räsener:

So, what needs to be done to merge this and close the issue?

The second step is to send PR via GitHub.

[Marius Räsener]

Alright, here is my PR :) ​https://github.com/django/django/pull/13927

[Marius Räsener]

Should I ping this every ~24 hrs or what could be a good strategy?

[Simon Charette]

Please refer to ​the contributing documentation to make sure your patch appears in the review queue.

In this case you'll want to uncheck Patch needs improvement

[Marius Räsener]

I hope that’s the right thing to do...

[AdamDonna]

I've recreated the PR and incorporated the feedback from the reviewers. It's ready for another review at ​https://github.com/django/django/pull/14149.
Functionally i think it's good; the docs will need some review (especially if they needed to be included in other places)

[Mariusz Felisiak]

#26709

[Mariusz Felisiak <felisiak.mariusz@…>]

In cdd0b21:

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