Opened 7 years ago

Closed 6 years ago

#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 by kemar, 7 years ago

Component: UncategorizedUtilities

comment:2 by Tim Graham, 7 years ago

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 by Florian Apolloner, 6 years ago

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 by Florian Apolloner, 6 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Piotr Domański, 6 years ago

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

comment:6 by Piotr Domański, 6 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Piotr Domański, 6 years ago

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 by kemar, 6 years ago

Thank you Piotr

comment:9 by Claude Paroz, 6 years ago

Triage Stage: Ready for checkinAccepted

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

comment:10 by Tomer Chachamu, 6 years ago

Has patch: set

comment:11 by Piotr Domański, 6 years ago

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'?

Version 1, edited 6 years ago by Piotr Domański (previous) (next) (diff)

in reply to:  9 comment:12 by Piotr Domański, 6 years ago

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 by Keryn Knight, 6 years ago

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 by Jon Dufresne, 6 years ago

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 by Jon Dufresne, 6 years ago

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 by Tim Graham, 6 years ago

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 by Jon Dufresne, 6 years ago

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

comment:18 by Tim Graham, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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