[Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
Django
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Jerdonek |
Type: | Status: new
Cleanup/optimization |
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: | CsrfViewMiddleware,referer
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The `CsrfViewMiddleware` class's
[https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L259-L379
process_view()`] method currently clocks in at 120 lines. If a method were
added that encapsulates its HTTP referer logic (similar to how
`CsrfViewMiddleware._origin_verified()` encapsulates its origin logic),
its length could be cut in half. I think this would make the method a lot
easier to maintain and understand. For example, in addition to being
shorter, one optimization I'm thinking of would be easier to implement.
This is because there's no way to "jump out" of an if-block, whereas with
a method, you can return early.
Given that the reason string for rejecting a request can vary in the
referer case, I would suggest a method that raises an exception internal
to the module, rather than returning a value. The call could then look
something like this (it would go here:
https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L283):
{{{#!python
elif request.is_secure():
try:
self._check_referer(request)
except _RejectRequest as exc:
return self._reject(request, exc.reason)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32596>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: CSRF | Version: dev
Keywords: | Triage Stage:
CsrfViewMiddleware,referer | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> The `CsrfViewMiddleware` class's
> [https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L259-L379
> process_view()`] method currently clocks in at 120 lines. If a method
> were added that encapsulates its HTTP referer logic (similar to how
> `CsrfViewMiddleware._origin_verified()` encapsulates its origin logic),
> its length could be cut in half. I think this would make the method a lot
> easier to maintain and understand. For example, in addition to being
> shorter, one optimization I'm thinking of would be easier to implement.
> This is because there's no way to "jump out" of an if-block, whereas with
> a method, you can return early.
>
> Given that the reason string for rejecting a request can vary in the
> referer case, I would suggest a method that raises an exception internal
> to the module, rather than returning a value. The call could then look
> something like this (it would go here:
> https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L283):
>
> {{{#!python
> elif request.is_secure():
> try:
> self._check_referer(request)
> except _RejectRequest as exc:
> return self._reject(request, exc.reason)
> }}}
New description:
The `CsrfViewMiddleware` class's
[https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L259-L379
process_view()] method currently clocks in at 120 lines. If a method were
added that encapsulates its HTTP referer logic (similar to how
`CsrfViewMiddleware._origin_verified()` encapsulates its origin logic),
its length could be cut in half. I think this would make the method a lot
easier to maintain and understand. For example, in addition to being
shorter, one optimization I'm thinking of would be easier to implement.
This is because there's no way to "jump out" of an if-block, whereas with
a method, you can return early.
Given that the reason string for rejecting a request can vary in the
referer case, I would suggest a method that raises an exception internal
to the module, rather than returning a value. The call could then look
something like this (it would go here:
https://github.com/django/django/blob/cac9ec73db35a6d38d33f271f4724da486c60e9f/django/middleware/csrf.py#L283):
{{{#!python
elif request.is_secure():
try:
self._check_referer(request)
except _RejectRequest as exc:
return self._reject(request, exc.reason)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Sounds reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
For this, it's probably better if #32612 is resolved first.
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
It might be easier to leave this alone as I think referer checking will
simply be removed at some point. Until then, Florian suggested adding a
setting to disable it. After the introduction of Origin header checking
(#16010), referer header checking isn't used when Origin header is
provided (all modern browsers).
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
I posted a PR here: https://github.com/django/django/pull/14211
To go along with the refactor, the PR includes two related refactorings,
one of which that can also be viewed as an optimization (this is the one
alluded to in my original comment above).
I also improved the tests to go along with the refactor. These are
included in the same commit as the refactor and check that an exception
was raised by the method. This is similar to how the `HTTP_ORIGIN` tests
also test the `_origin_verified()` method in addition to testing
`process_view()`.
While working on enhancing those tests, I also discovered two code paths
that previously weren't covered, so I added tests for those in a separate
commit, before the refactor.
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
CsrfViewMiddleware,referer |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"02c59b7a4355fda8c99224b5de9c0a3929bffe22" 02c59b7]:
{{{
#!CommitTicketReference repository=""
revision="02c59b7a4355fda8c99224b5de9c0a3929bffe22"
Refs #32596 -- Added extra tests for CsrfViewMiddleware's referer logic.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
CsrfViewMiddleware,referer | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
CsrfViewMiddleware,referer | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"cfd8c918390cd5317621124d224a009196f8755c" cfd8c91]:
{{{
#!CommitTicketReference repository=""
revision="cfd8c918390cd5317621124d224a009196f8755c"
Refs #32596 -- Optimized CsrfViewMiddleware._check_referer() to delay
computing good_referer.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
CsrfViewMiddleware,referer | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"214b36f50aec5ff42776ba847ce33ce08d82ca76" 214b36f]:
{{{
#!CommitTicketReference repository=""
revision="214b36f50aec5ff42776ba847ce33ce08d82ca76"
Refs #32596 -- Added early return on safe methods in
CsrfViewMiddleware.process_view().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
CsrfViewMiddleware,referer | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"71179a6124142e43fd3c0eea2bfabf600a9b2d91" 71179a61]:
{{{
#!CommitTicketReference repository=""
revision="71179a6124142e43fd3c0eea2bfabf600a9b2d91"
Fixed #32596 -- Added CsrfViewMiddleware._check_referer().
This encapsulates CsrfViewMiddleware's referer logic into a method and
updates existing tests to check the "seam" introduced by the refactor,
when doing so would improve the test.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32596#comment:10>