#28638 closed Cleanup/optimization (fixed)

Make the allowed_hosts parameter mandatory for is_safe_url()

Reported by: kemar Owned by: Piotr Domański
Component: Utilities Version: 1.11
Severity: Normal Keywords:
Cc: Florian Apolloner, Keryn Knight Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

>>> 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.

Change History (19)

comment:1 Changed 14 months ago by kemar

Component: UncategorizedUtilities

comment:2 Changed 14 months ago by Tim Graham

Cc: Florian Apolloner added
Type: UncategorizedCleanup/optimization

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?

comment:3 Changed 14 months ago by 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.

comment:4 Changed 14 months ago by Florian Apolloner

Triage Stage: UnreviewedAccepted

comment:5 Changed 12 months ago by Piotr Domański

Owner: changed from nobody to Piotr Domański
Status: newassigned

comment:6 Changed 12 months ago by Piotr Domański

Triage Stage: AcceptedReady for checkin

comment:7 Changed 12 months ago by 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.

comment:8 Changed 12 months ago by kemar

Thank you Piotr

comment:9 Changed 12 months ago by Claude Paroz

Triage Stage: Ready for checkinAccepted

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

comment:10 Changed 12 months ago by Tomer Chachamu

Has patch: set

comment:11 Changed 12 months ago by Piotr Domański

I found that is_safe_url in master branch can end 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'?

Last edited 12 months ago by Piotr Domański (previous) (diff)

comment:12 in reply to:  9 Changed 11 months ago by 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.

comment:13 Changed 10 months ago by Keryn Knight

Cc: Keryn Knight added
Needs documentation: set

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.

comment:14 Changed 10 months ago by 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).

comment:15 Changed 10 months ago by 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.

comment:16 Changed 10 months ago by 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.

comment:17 Changed 10 months ago by Jon Dufresne

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

comment:18 Changed 10 months ago by Tim Graham

Needs documentation: unset
Summary: Allow `is_safe_url` to work without `allowed_hosts` or make the parameter mandatoryMake the allowed_hosts parameter mandatory for is_safe_url()
Triage Stage: AcceptedReady for checkin

Looks good, pending a few small changes.

comment:19 Changed 10 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 1e81a4b8:

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

Note: See TracTickets for help on using tickets.
Back to Top