Make the allowed_hosts parameter mandatory for is_safe_url()

[kemar]

>>> from django.utils.http import is_safe_url

>>> is_safe_url("https://www.djangoproject.com")
False

>>> is_safe_url("https://www.djangoproject.com", allowed_hosts={"www.djangoproject.com"})
True

If this may have an impact on security, then make it clear that allowed_hosts is mandatory by removing its None default value ​https://github.com/django/django/blob/98706bb35e7de0e445cc336f669919047bf46b75/django/utils/http.py#L265.

[Tim Graham]

The host argument has been optional since the original commit that added is_safe_url() (a2f2a399566dd68ce7e312fff5a5ba857066797d) -- now the parameter is named allowed_hosts. Django always provides that argument -- I'm not sure if there's a use case for not providing it. Did you have something in mind, Florian?

[Florian Apolloner]

Not that I can think of, we could default allowed_hosts to settings.ALLOWED_HOSTS but I am not sure if that is a good idea given that we can have star in there.

[Piotr Domański]

In this PR: ​https://github.com/django/django/pull/9386 if allowed hosts are not provided then they are retrieved from settings.ALLOWED_HOSTS as you wish.
CI passed so this PR is ready for QA.

[kemar]

Thank you Piotr

[Claude Paroz]

Ready for checkin is not supposed to be set by the patch author, but by a reviewer.

[Piotr Domański]

I found that is_safe_url can ends with exception if user passes both host and allowed_hosts value and allowed hosts is for example list. I fixed this in updated patch. Will this patch be reviewed by somebody? Do i need to find someone to review? Is this ticket marked correctly as 'Waiting for QA'?

[Piotr Domański]

Replying to Claude Paroz:

Ready for checkin is not supposed to be set by the patch author, but by a reviewer.

Can you perform review of this task? If not, can you find reviewer for this task?
I am waiting very long for review of my first ticket in Django project :)

Please help.

[Keryn Knight]

Assuming there's no objections to default allowed_hosts to settings.ALLOWED_HOSTS but I am not sure if that is a good idea given that we can have star in there the PR now looks OK to me, barring the need (I think) to add the change in behaviour to the release notes.

[Jon Dufresne]

I'm not sure if there's a use case for not providing it.

I'm also unsure what the use case if for using is_safe_url without an allowed_hosts argument. To anyone that is asking for this feature, can you provide details on your use case that would make it clear why this is a good idea?

When I added allowed_hosts it was originally optional because, at the time, the user could pass either allowed_hosts or hosts. Now that there is only allowed_hosts, I think it should be a required argument (unless a use case surfaces).

[Jon Dufresne]

A test was added for is_safe_url() without a host argument in commit c5544d289233f501917e25970c03ed444abbd4f0. If the test case represents a use case, it looks like it could exist to verify URLs without a host component are safe.

[Tim Graham]

I don't know that I would put much weight on that test case. is_safe_url() is undocumented, so backwards compatibility isn't a higher priority than making the change that makes the most sense. One of my thoughts is that things in django.utils shouldn't rely on Django settings, if possible. Like Jon, I lean toward making allowed_hosts a required argument.

[Jon Dufresne]

Depending on what gets decided, here is a ​PR to make allowed_hosts required.

[Tim Graham]

Looks good, pending a few small changes.

[Tim Graham <timograham@…>]

In 1e81a4b8:

Fixed #28638 -- Made allowed_hosts a required argument of is_safe_url().