Opened 7 years ago
Closed 7 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 , 7 years ago
Component: | Uncategorized → Utilities |
---|
comment:2 by , 7 years ago
Cc: | added |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 7 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 , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:7 by , 7 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.
follow-up: 12 comment:9 by , 7 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Ready for checkin is not supposed to be set by the patch author, but by a reviewer.
comment:10 by , 7 years ago
Has patch: | set |
---|
comment:11 by , 7 years ago
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'?
comment:12 by , 7 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 , 7 years ago
Cc: | 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 , 7 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 , 7 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 , 7 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 , 7 years ago
Depending on what gets decided, here is a PR to make allowed_hosts
required.
comment:18 by , 7 years ago
Needs documentation: | unset |
---|---|
Summary: | Allow `is_safe_url` to work without `allowed_hosts` or make the parameter mandatory → Make the allowed_hosts parameter mandatory for is_safe_url() |
Triage Stage: | Accepted → Ready for checkin |
Looks good, pending a few small changes.
The
host
argument has been optional since the original commit that addedis_safe_url()
(a2f2a399566dd68ce7e312fff5a5ba857066797d) -- now the parameter is namedallowed_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?