Opened 4 years ago

Closed 3 years ago

#4317 defect closed fixed (fixed)

twisted.web.http.Request allows you to write to a finished request.

Reported by: dreid Owned by:
Priority: normal Milestone:
Component: web Keywords: easy
Cc: Branch: branches/request-write-after-finish-4317
(diff, github, buildbot, log)
Author: hderms Launchpad Bug:

Description

This can cause corruption to subsequent requests, it is also partially responsible for the behavior described in #3684

Attachments (3)

ticket 4317 patch.patch (380 bytes) - added by hderms 4 years ago.
Simple fix, uses pythonic idiom that was used in the same class
ticket 4317 patch v2.patch (1.9 KB) - added by hderms 3 years ago.
Rewrite of patch and addition of unit test
ticket 4317 patch v3.patch (1.6 KB) - added by hderms 3 years ago.
adjusted changes requested by reviewer

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by exarkun

  • Keywords easy added

We should change Request.write to discard such bytes. We should also introduce a warning for this case (that could be done separately though). And ultimately, this should result in an exception.

Changed 4 years ago by hderms

Simple fix, uses pythonic idiom that was used in the same class

comment:2 Changed 4 years ago by hderms

  • Keywords review added
  • Owner jknight deleted

comment:3 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to hderms

Thanks. We need a unit test which verifies that this behaves as desired, too. Can you add that to the patch?

comment:4 Changed 4 years ago by hderms

I will do so. I was confused and thought unit-tests were only required when adding new features. Will read the twisted documentation on unit tests and then implement.

Changed 3 years ago by hderms

Rewrite of patch and addition of unit test

comment:5 Changed 3 years ago by hderms

  • Keywords review added
  • Owner hderms deleted

Added a new patch which raises a RuntimeError when Request.write is called after Request.finish. Added a unit test which tests this functionality.

comment:6 follow-up: Changed 3 years ago by therve

  • Author set to hderms
  • Keywords review removed
  • Owner set to hderms
  • + if self.finished == 1: You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...
  • twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable fails after your fix is applied. I suspect the test is broken, but it should be fixed.
  • -        self.assertRaises(RuntimeError, req.finish)
    +        self.assertRaises(RuntimeError, req.finish())
    
    This change is broken, it makes the test fails.
  • There should be 2 blank lines before your test
  • The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?

Thanks!

comment:7 in reply to: ↑ 6 Changed 3 years ago by hderms

Replying to therve:

  • + if self.finished == 1: You should simply call `if self.finished:' here. This ought to be a boolean, if though it's not...
  • twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable fails after your fix is applied. I suspect the test is broken, but it should be fixed.
  • -        self.assertRaises(RuntimeError, req.finish)
    +        self.assertRaises(RuntimeError, req.finish())
    
    This change is broken, it makes the test fails.
  • There should be 2 blank lines before your test
  • The test docstring looks a bit weird. Is there an extra "L{Request.finish}" at the beginning?

Thanks!

I examined that test that fails, and I can't seem to find any connection between it and /web/http, it doesn't import anything http related, and the test in particular that fails is just parsing strings. I'm not entirely sure but I think the test is broken as well.

I didn't implement the if self.finished == 1: and I found that to be rather unpythonic as well, but was reluctant to change it because this is my first patch.

The change to the the test_finishAfterConnectionLost(self): was unintentional. I think it resulted from me examining how these unit tests worked, I must have forgotten to eliminate that portion. I will remove it and resubmit.

I will fix the formatting errors.

comment:8 Changed 3 years ago by therve

Hum sorry, the test I mentioned (twisted.web.test.test_xml.MicroDOMTest.testLaterCloserTable) was probably just a output parsing error on my part. It must have been the other one.

Changed 3 years ago by hderms

adjusted changes requested by reviewer

comment:9 Changed 3 years ago by hderms

  • Keywords review added
  • Owner hderms deleted

comment:10 Changed 3 years ago by exarkun

  • Author changed from hderms to exarkun, hderms
  • Branch set to branches/request-write-after-finish-4317

(In [30650]) Branching to 'request-write-after-finish-4317'

comment:11 Changed 3 years ago by exarkun

(In [30651]) Apply patch from the ticket, with a couple trivial changes

refs #4317

comment:12 Changed 3 years ago by exarkun

(In [30652]) news fragment

refs #4317

comment:13 Changed 3 years ago by exarkun

  • Author changed from exarkun, hderms to hderms
  • Keywords review removed
  • Owner set to exarkun
  • Status changed from new to assigned

Did you manually edit the patch to remove the last hunk? However the latest version of the patch was created, neither patch nor emacs is happy with it (Emacs says "Hunk seriously messed up"). It's not a problem because this patch is short enough that I can manually apply it. Whatever happened though, try to avoid it in the future. :)

I checked the changes into a branch, added a news fragment, and made the if self.finished: change therve suggested. Build results look good, so I'll go ahead and merge this. Thanks!

comment:14 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [30653]) Merge request-write-after-finish-4317

Author: hderms
Reviewer: therve, exarkun
Fixes: #4317

In Request.write, check to see if the response has already been finished and raise
an exception if it has, rather than tossing garbage bytes onto the connection.

comment:15 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.