[Django] #28936: simplify_regex should remove redundant escape sequences outside groups
Django
-------------------------------------+-------------------------------------
Reporter: Cristi | Owner: nobody
Vîjdea |
Type: Bug | Status: new
Component: | Version: 2.0
contrib.admindocs |
Severity: Normal | Keywords: simplify_regex path
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 1 |
-------------------------------------+-------------------------------------
**django.contrib.admindocs.views.simplify_urls** should clean up escapes
found outside path parameters. Otherwise, broken URLs with backslashes can
be generated and displayed.
This is readily apparent with Django 2's **path()**, which aggresively
escapes everything outside a **<parameter>** specifier, resulting in a
urlpattern with backslash-escaped forward slashes:
{{{
>>>
simplify_regex(r"^(?P<sport_slug>\w+)/athletes/(?P<athlete_slug>\w+)/$")
'/<sport_slug>/athletes/<athlete_slug>/'
>>>
simplify_regex(r"^(?P<sport_slug>\w+)\/athletes\/(?P<athlete_slug>\w+)\/$")
'/<sport_slug>\\/athletes\\/<athlete_slug>\\/'
}}}
The second example is what **path()** would generate in urlpatterns.
--
Ticket URL: <https://code.djangoproject.com/ticket/28936>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Keywords: simplify_regex path | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
Old description:
> **django.contrib.admindocs.views.simplify_urls** should clean up escapes
> found outside path parameters. Otherwise, broken URLs with backslashes
> can be generated and displayed.
>
> This is readily apparent with Django 2's **path()**, which aggresively
> escapes everything outside a **<parameter>** specifier, resulting in a
> urlpattern with backslash-escaped forward slashes:
>
> {{{
> >>>
> simplify_regex(r"^(?P<sport_slug>\w+)/athletes/(?P<athlete_slug>\w+)/$")
> '/<sport_slug>/athletes/<athlete_slug>/'
> >>>
> simplify_regex(r"^(?P<sport_slug>\w+)\/athletes\/(?P<athlete_slug>\w+)\/$")
> '/<sport_slug>\\/athletes\\/<athlete_slug>\\/'
> }}}
>
> The second example is what **path()** would generate in urlpatterns.
New description:
**django.contrib.admindocs.views.simplify_urls** should clean up escapes
found outside path parameters. Otherwise, broken URLs with backslashes can
be generated and displayed.
This is readily apparent with Django 2's **path()**, which aggresively
escapes everything outside a **<parameter>** specifier, resulting in a
urlpattern with backslash-escaped forward slashes:
{{{
>>>
simplify_regex(r"^(?P<sport_slug>\w+)/athletes/(?P<athlete_slug>\w+)/$")
'/<sport_slug>/athletes/<athlete_slug>/'
>>>
simplify_regex(r"^(?P<sport_slug>\w+)\/athletes\/(?P<athlete_slug>\w+)\/$")
'/<sport_slug>\\/athletes\\/<athlete_slug>\\/'
}}}
The second example is what **path()** would generate in urlpatterns.
You can, for example, see this issue affecting django-rest-framework
[https://github.com/encode/django-rest-framework/issues/5675 here].
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:1>
Django
-------------------------------------+------------------------------------
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+------------------------------------
* cc: ChillarAnand (added)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:2>
Django
-------------------------------------+-------------------------------------
| Raj
Type: Bug | Status: assigned
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Avinash Raj
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:3>
Django
Reporter: Cristi Vîjdea | Owner: (none)
Type: Bug | Status: new
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+------------------------------------
* owner: Avinash Raj => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:4>
Django
-------------------------------------+-------------------------------------
| Cleary
Type: Bug | Status: assigned
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Oliver Cleary
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:3>
Django
-------------------------------------+-------------------------------------
| Cleary
Type: Bug | Status: assigned
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
Comment (by Oliver Cleary):
I have a PR for this ticket, however I am not sure it really needs to be
fixed.
The referenced DRF ticket was resolved by changing the usage of the
simplify_regex function to match the usage by Django, which is to pass in
the paths route string directly, rather than using it's generated regex
pattern.
Additionally, the given example does not seem to be correct as path does
not escape forward slashes.
{{{
>>> path('<slug:sports_slug>/athletes/<slug:athletes_slug>/', lambda:
None).pattern.regex.pattern
'^(?P<sports_slug>[-a-zA-Z0-9_]+)/athletes/(?P<athletes_slug>[-a-zA-Z0-9_]+)/$'
}}}
Testing the example in the DRF ticket however does exhibit the issue:
{{{
>>> path('^api/token-auth/', lambda: None).pattern.regex.pattern
'^\\^api/token\\-auth/$'
>>> simplify_regex(r'^\^api/token\-auth/$')
'/\\api/token\\-auth/'
}}}
With the fix in the PR the special characters are unescaped, and the `^?$`
are only stripped if not escaped.
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:4>
Django
Reporter: Cristi Vîjdea | Owner: (none)
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+------------------------------------
* status: assigned => new
* owner: Oliver Cleary => (none)
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:5>
Django
Reporter: Cristi Vîjdea | Owner: (none)
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+------------------------------------
* cc: Carlton Gibson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:7>
Django
-------------------------------------+-------------------------------------
Component: contrib.admindocs | Version: 2.0
Severity: Normal | Resolution: invalid
Keywords: simplify_regex path | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Thanks for the patch Oliver.
> ... I am not sure it really needs to be fixed.
I think this is right.
As with the DRF issue, calling `str()` on `path.pattern` before passing to
`simplify_regex()` is the correct usage, and
[https://github.com/django/django/blob/5ea1621c724e765c9a642a1b3b6e83419fda920b/django/contrib/admindocs/views.py#L389
it is what `admindocs` does].
Here using [https://github.com/encode/django-rest-
framework/issues/5675#issue-282663045 the original example from the DRF
issue]:
{{{
>>> from django.urls import path
>>> from django.contrib.admindocs.views import simplify_regex
>>> p = path('^api/token-auth/', lambda: None) # Not sure about the `^`,
but either way…
>>> p.pattern
<django.urls.resolvers.RoutePattern object at 0x10acd49d0>
>>> p.pattern.regex.pattern
'^\\^api/token\\-auth/$'
>>> simplify_regex(p.pattern.regex.pattern) # Wrong usage.
'/\\api/token\\-auth/'
>>> str(p.pattern)
'^api/token-auth/'
>>> simplify_regex(str(p.pattern)) # Correct usage.
'/api/token-auth/'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28936#comment:8>