Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32596 closed Cleanup/optimization (fixed)

Add a method to CsrfViewMiddleware to encapsulate its referer logic

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: CSRF Version: dev
Severity: Normal Keywords: CsrfViewMiddleware, referer
Cc: 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 (last modified by Chris Jerdonek)

The CsrfViewMiddleware class's 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):

elif request.is_secure():
    try:
        self._check_referer(request)
    except _RejectRequest as exc:
        return self._reject(request, exc.reason)

Change History (12)

comment:1 by Chris Jerdonek, 4 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 4 years ago

Triage Stage: UnreviewedAccepted

Sounds reasonable.

comment:3 by Chris Jerdonek, 4 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:4 by Chris Jerdonek, 4 years ago

For this, it's probably better if #32612 is resolved first.

comment:5 by Tim Graham, 4 years ago

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).

comment:6 by Chris Jerdonek, 4 years ago

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.

comment:7 by Chris Jerdonek, 4 years ago

Has patch: set

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 02c59b7:

Refs #32596 -- Added extra tests for CsrfViewMiddleware's referer logic.

comment:9 by Mariusz Felisiak, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 71179a61:

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.

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In cfd8c91:

Refs #32596 -- Optimized CsrfViewMiddleware._check_referer() to delay computing good_referer.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 214b36f:

Refs #32596 -- Added early return on safe methods in CsrfViewMiddleware.process_view().

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