Opened 6 years ago

Closed 5 years ago

#29667 closed Bug (fixed)

path converters don't handle spaces well.

Reported by: Keryn Knight Owned by: Hasan Ramezani
Component: Core (URLs) Version: dev
Severity: Normal Keywords: converters path _route_to_regex
Cc: Tom Forbes, Vishvajit Pathak 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

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.

Change History (12)

comment:1 by Carlton Gibson, 6 years ago

Triage Stage: UnreviewedAccepted

Thanks for the report Keryn. This looks reasonable.

comment:2 by Jeff, 6 years ago

Owner: changed from nobody to Jeff
Status: newassigned

happy to look into this.

comment:3 by Tom Forbes, 6 years ago

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?

comment:4 by Tom Forbes, 6 years ago

Cc: Tom Forbes added

comment:5 by Tim Graham, 6 years ago

I think it would be be best to prohibit whitespace.

comment:6 by Adam Johnson, 6 years ago

I agree, prohibiting whitespace would be best, it would avoid 'dialects' of urlconfs appearing and the potential for future problems.

comment:7 by Vishvajit Pathak, 6 years ago

Tim, Adam,
So do we want to raise ImproperlyConfigured when whitespace is used ?

comment:8 by Vishvajit Pathak, 6 years ago

Cc: Vishvajit Pathak added

comment:9 by Hasan Ramezani, 5 years ago

Has patch: set
Owner: changed from Jeff to Hasan Ramezani

comment:10 by Carlton Gibson, 5 years ago

Patch needs improvement: set

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!

comment:11 by Carlton Gibson, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:12 by Carlton Gibson <carlton.gibson@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 22394bd3:

Fixed #29667 -- Prohibited whitespaces in path() URLs.

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