[Django] #33365: Functionality change in 3.2.10 for re_path
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Type: Bug | Status: new
Component: Core | Version: 3.2
(URLs) | Keywords: 3.2.10 resolvers
Severity: Normal | re_path
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
3.2.10 does the following change in urls/resolvers.conf:
out:
{{{
match = self.regex.search(path)
}}}
in:
{{{
match = (
self.regex.fullmatch(path)
if self._is_endpoint and self.regex.pattern.endswith('$')
else self.regex.search(path)
)
}}}
Now, that's a big change, because I have an endpoint that is now giving
404s:
{{{
re_path(r"validate$", ValidateView.as_view(), name="validate"),
}}}
The error is assuming that just because it ends in ''$'', it has to use
fullmatch. I'm using (was using) that to catch ''/*whatever*/validate''
urls.
It is a big enough change that it should at least have been noted
explicitly in the release notes. And, IMHO, it's a mistake (I'll admit to
not having read the CVE that prompted this change).
--
Ticket URL: <https://code.djangoproject.com/ticket/33365>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Florian Apolloner (added)
* status: new => closed
* resolution: => invalid
Comment:
Thanks for this report, however all security patches are backward
incompatible. You can add `.*` to your pattern if you want to match
everything, e.g.
{{{
re_path( r"^.*validate$", ValidateView.as_view(), name="validate"),
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
> It is a big enough change that it should at least have been noted
explicitly in the release notes.
Agreed, if we would have known at that point that those URLs are broken we
would have noted or fixed it. Sadly it is always easier to note this after
the fact and with security releases we generally have less review (or at
least reviews from a rather homogenous group).
> And, IMHO, it's a mistake (I'll admit to not having read the CVE that
prompted this change).
Well the most common (documented as also what our tests cover) is having
urls like `re_path(r'^…$')` -- while it is possible to drop `^` and `$` I
think it is rather uncommon which is why we didn't realize it. That said,
because a simple work-around does exist, I think we maybe should keep it
like it is currently. After all one usually wants to match the whole URL.
I'd even go as far as to issue a warning if `^` is not present.
Out of curiosity. What did you validate with that view? Ie wouldn't have
`r"^(?P<prefix>.*)/validate$"` made more sense? I am not saying you are
doing anything wrong but merely trying to understand which other issues
people could run into -- so I need to know the usecases.
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Pkt):
Hey, Mariusz & Florian!
Thanks for the quick answer.
That's old code that we use so we can match any of these:
/validate
/auth/validate
/v2/validate
/v3/validate
In this case all do the same, but some frontends will call different
endpoints (other endpoints do have different results depending on URL),
which is why we don't care to capture version (if present) in this case.
Frontends will have a base url (with version included) and then call
endpoints under that. It's not something I really like, but we do have a
lot of legacy code to update, and that one is in the queue.
Once we noticed the 404, we did indeed change it to
{{{
r"^.*validate$"
}}}
I like the idea of the warning, and I'd update the docs just so it doesn't
happen to anyone else. I know it is a very rare case, but as it is now, it
says "''a regular expression compatible with Python’s re module. [...]
When a match is made''". It should mention that any regular expression
ending in ''$'' will expect a full match.
Oh, BTW, we validate email providers, but that's in the payload and sent
as a POST, so it really makes no difference.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
Okay that explains it a better :) Do you think you are up to writing a
docs patch and adding that warning?
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
I proposed a small clarification in docs, see
[https://github.com/django/django/pull/15200 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Pkt):
Looks good to me. Cheers!
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"5de12a369a7b2231e668e0460c551c504718dbf6" 5de12a36]:
{{{
#!CommitTicketReference repository=""
revision="5de12a369a7b2231e668e0460c551c504718dbf6"
Refs #33365, Refs #30530 -- Doc'd re_path() behavior change in Django
2.2.25, 3.1.14, and 3.2.10.
Follow up to d4dcd5b9dd9e462fec8220e33e3e6c822b7e88a6.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"267a743bf253a4e0703c0257a5df7774116c3194" 267a743b]:
{{{
#!CommitTicketReference repository=""
revision="267a743bf253a4e0703c0257a5df7774116c3194"
[4.0.x] Refs #33365, Refs #30530 -- Doc'd re_path() behavior change in
Django 2.2.25, 3.1.14, and 3.2.10.
Follow up to d4dcd5b9dd9e462fec8220e33e3e6c822b7e88a6.
Backport of 5de12a369a7b2231e668e0460c551c504718dbf6 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"ae242235db910a94ea476b7a3efacf8dbab31c4f" ae242235]:
{{{
#!CommitTicketReference repository=""
revision="ae242235db910a94ea476b7a3efacf8dbab31c4f"
[3.2.x] Refs #33365, Refs #30530 -- Doc'd re_path() behavior change in
Django 2.2.25, 3.1.14, and 3.2.10.
Follow up to d4dcd5b9dd9e462fec8220e33e3e6c822b7e88a6.
Backport of 5de12a369a7b2231e668e0460c551c504718dbf6 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0b8a0296bfd30748f08021834e95cdae241686e8" 0b8a0296]:
{{{
#!CommitTicketReference repository=""
revision="0b8a0296bfd30748f08021834e95cdae241686e8"
[3.1.x] Refs #33365, Refs #30530 -- Doc'd re_path() behavior change in
Django 2.2.25, 3.1.14, and 3.2.10.
Follow up to d4dcd5b9dd9e462fec8220e33e3e6c822b7e88a6.
Backport of 5de12a369a7b2231e668e0460c551c504718dbf6 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Pkt | Owner: nobody
Component: Core (URLs) | Version: 3.2
Severity: Normal | Resolution: invalid
Keywords: 3.2.10 resolvers | Triage Stage:
re_path | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"b87820668e7bd519dbc05f6ee46f551858fb1d6d" b8782066]:
{{{
#!CommitTicketReference repository=""
revision="b87820668e7bd519dbc05f6ee46f551858fb1d6d"
[2.2.x] Refs #33365, Refs #30530 -- Doc'd re_path() behavior change in
Django 2.2.25, 3.1.14, and 3.2.10.
Follow up to d4dcd5b9dd9e462fec8220e33e3e6c822b7e88a6.
Backport of 5de12a369a7b2231e668e0460c551c504718dbf6 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33365#comment:11>