[Django] #29667: path converters don't handle spaces well.
Django
-------------------------------------+-------------------------------------
Reporter: Keryn | Owner: nobody
Knight |
Type: Bug | Status: new
Component: Core | Version: master
(URLs) | Keywords: converters path
Severity: Normal | _route_to_regex
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This came up for someone on IRC last week, but I can't see that they
raised a ticket about it.
Correct:
{{{
>>> from django.urls.resolvers import _route_to_regex
>>> _route_to_regex("<uuid:test>")
('^(?P<test>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})',
{'test': <django.urls.converters.UUIDConverter at 0x1055e8c88>})
}}}
Also working correctly:
{{{
>>> from django.urls.resolvers import _route_to_regex
>>> _route_to_regex("<uuid:2>")
ImproperlyConfigured: URL route '<uuid:2>' uses parameter name '2' which
isn't a valid Python identifier.
}}}
however, constructing a valid **looking** converter reference apparently
hits neither the happy nor the unhappy path, and also I presume passes any
system checks in place that might otherwise warn about the sensitivity:
{{{
>>> from django.urls.resolvers import _route_to_regex
>>> _route_to_regex("<uuid: test>") # note the preceeding space
('^\\<uuid\\:\\ test\\>', {})
}}}
- the regex is invalid
- the kwargs dictionary is (sort of rightly) empty.
- the same is true with "test " and "te st"
Unless I'm misunderstanding the code therein, "test ", " test" and "te st"
should all be hitting the invalid identifier part, and personally I feel
like leading/trailing spaces at least could just be sanitised (stripped)
as they're almost certainly accidental or for display purposes.
Tested in a shell against master @
7eb556a6c2b2ac9313158f8b812eebea02a43f20.
--
Ticket URL: <https://code.djangoproject.com/ticket/29667>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Thanks for the report Keryn. This looks reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Jeff
* status: new => assigned
Comment:
happy to look into this.
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tom Forbes):
I ran into this recently and ended up diagnosing the failure so I really
hope you do not mind me jumping in and offering a PR Jeff. The path
parameter regex was just a bit too strict about parsing spaces, and so
ended up ignoring the pattern entirely
PR: https://github.com/django/django/pull/10336
I believe this fixes the issue but I'm not sure how much we want to
document this. Should we raise a check failure?
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Tom Forbes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
I think it would be be best to prohibit whitespace.
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
I agree, prohibiting whitespace would be best, it would avoid 'dialects'
of urlconfs appearing and the potential for future problems.
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vishvajit Pathak):
Tim, Adam,
So do we want to raise `ImproperlyConfigured` when whitespace is used ?
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Vishvajit Pathak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:8>
Django
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Jeff => Hasan Ramezani
* has_patch: 0 => 1
Comment:
PR [https://github.com/django/django/pull/11688]
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:9>
Django
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: converters path | Triage Stage: Accepted
_route_to_regex |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Patch looks good: just a few adjustments and ready to go. (I'll mark RFC
to keep it on my rader: I assume Hasan will turn them around quickly, as
ever 🙂) Thanks Hasan!
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:10>
Django
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
_route_to_regex | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:11>
Django
-------------------------------------+-------------------------------------
| Ramezani
Component: Core (URLs) | Version: master
Keywords: converters path | Triage Stage: Ready for
_route_to_regex | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"22394bd3a18a7d9a8957a0b431f8ae4e5ca03a8c" 22394bd3]:
{{{
#!CommitTicketReference repository=""
revision="22394bd3a18a7d9a8957a0b431f8ae4e5ca03a8c"
Fixed #29667 -- Prohibited whitespaces in path() URLs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29667#comment:12>