Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  closed
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:  fixed
 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
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"214b36f50aec5ff42776ba847ce33ce08d82ca76" 214b36f]:
 {{{
 #!CommitTicketReference repository=""
 revision="214b36f50aec5ff42776ba847ce33ce08d82ca76"
 Refs #32596 -- Added early return on safe methods in
 CsrfViewMiddleware.process_view().
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.afc02cf90cbd04ae480c805742186126%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  closed
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:  fixed
 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
-+-
Changes (by Mariusz Felisiak ):

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.5bd1cedea28a5d8d6dbafd298bdb8df6%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  closed
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:  fixed
 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
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"cfd8c918390cd5317621124d224a009196f8755c" cfd8c91]:
 {{{
 #!CommitTicketReference repository=""
 revision="cfd8c918390cd5317621124d224a009196f8755c"
 Refs #32596 -- Optimized CsrfViewMiddleware._check_referer() to delay
 computing good_referer.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.aef8bcb760425a21a7af4961e4937cd3%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 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
-+-
Changes (by Mariusz Felisiak):

 * stage:  Accepted => Ready for checkin


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.a595ceff477e1b7e1f1251f1f8d71c20%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
  CsrfViewMiddleware,referer |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"02c59b7a4355fda8c99224b5de9c0a3929bffe22" 02c59b7]:
 {{{
 #!CommitTicketReference repository=""
 revision="02c59b7a4355fda8c99224b5de9c0a3929bffe22"
 Refs #32596 -- Added extra tests for CsrfViewMiddleware's referer logic.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.89b390f052cc5c4adcef0dddeddc9648%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
  CsrfViewMiddleware,referer |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Chris Jerdonek):

 * has_patch:  0 => 1


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.271dfa6cac9c05cf30e35c1c3ffc3a00%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.eb5daab98f8fff18193dde6d59524c4c%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.da329938deb54aed7908f59ee8b2a416%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-

Comment (by Chris Jerdonek):

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.9b17cbb7af775c579b6b21c249f762f8%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  Chris
 Type:   |  Jerdonek
  Cleanup/optimization   |   Status:  assigned
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-
Changes (by Chris Jerdonek):

 * owner:  nobody => Chris Jerdonek
 * status:  new => assigned


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.5eb17a1a9288179bcacd530d3c5c%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-
Changes (by Mariusz Felisiak):

 * stage:  Unreviewed => Accepted


Comment:

 Sounds reasonable.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.667cf99568db20495723d7254e6d728e%40djangoproject.com.

Re: [Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
 Reporter:  Chris Jerdonek   |Owner:  nobody
 Type:   |   Status:  new
  Cleanup/optimization   |
Component:  CSRF |  Version:  dev
 Severity:  Normal   |   Resolution:
 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
-+-
Description changed by Chris Jerdonek:

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.afb0b0cded3892a092284c8e34f7b912%40djangoproject.com.

[Django] #32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic

[Django]

#32596: Add a method to CsrfViewMiddleware to encapsulate its referer logic
-+-
   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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/052.ba0e57e3c984e0932f6a0dae664eee86%40djangoproject.com.