Pull requests

#14 Declined
Repository
stpierre stpierre
Branch
default
Repository
cherrypy cherrypy
Branch
default

Fixed bug with POST data over 16K with SSL

Comments (13)

  1. Chris St. Pierre author

    Sure, but I'm not sure I quite understand the test structure. Is there an existing test case that this would fit into? test_wsgiapps.py seems like the best fit, but that appears to be mostly interested in testing graft().

    Thanks.

    1. Joseph Tate

      helper.py in cherrypy/tests sets up every test to be run with all of the engines and configurations. So I'd suggest adding the test to test_conn.py:TestConnection maybe? Then it will get tested on all the engines (WSGI, fcgi, etc.). Test it without your patch first to make sure you have a valid test case. Then make sure that applying the patch fixes the test case.

      One note about running the test suite. There are a few tests that give intermittent failures, and some that are just bad tests because they are non-deterministic. So run the test suite on an unmodified tree first to get a handle on those items before you go chasing what you broke.

      1. Joseph Tate

        Another note. You'll need to run nose with the -s option. When a server error happens, the test suite pauses so you can explore the error.

        I really should write a wiki page. It seems like there used to be one, but I can't find it any longer.

        1. Chris St. Pierre author

          Unfortunately, this isn't going very smoothly.

          The CherryPy server brought up by the ConnectionTests class (and every class in test_conn.py) has a max_request_body_size of 1001. It doesn't appear to be possible to redefine that on the fly; at least, my attempts to change it don't seem to work. I copied the code in ConnectionTests.test_readall_or_close(), which claims to redefine it, but I don't think it actually does; test_readall_or_close() expects all of its tests to succeed (i.e., it never checks that a request above the max_request_body_size is refused, only that requests below it succeed, and it only redefines it to be 0, so never _really_ tests it). I also tried using cherrypy.config.update(), but that doesn't seem to work either.

          setup_server seems to be called per class, that is, the server is only set up once per test case. test_readall_or_close() would seem to need two different servers instantiated in order to test, and my new test would need yet another. As I see it, the easiest thing to do would likely be to write a function, get_setup_server(), that accepts arbitrary configuration options and returns a callable that can be used as setup_server() with those options in place. Then test_readall_or_close() would need to be split off into two separate classes (one with max_request_body_size = 1001 and one with max_request_body_size = 0) and my new test would need to be in the class with max_request_body_size = 0. E.g.:

          class UnlimitedBodySizeConnectionTests(helper.CPWebCase): setup_server = staticmethod(get_setup_server({'server.max_request_body_size': 0})

          def test_readall_or_close(self): ...

          def test_large_post_body(self): ...

          I obviously want to avoid duplicating the code for test_readall_or_close(), so there might be some funky inheritance stuff going on to ensure that it a) gets tested twice; and b) gets written once.

          Does this sound reasonable? Metaprogramming is always so fun, especially in unit tests.

          1. Joseph Tate

            I like that idea, I also like the idea of "setup_server" not being a static method so that the test class can specify its own config instead of having a proliferation of static closures that never get garbage collected.

            Anyway, in an ideal world, you could put the test in test_conn where it seems to belong. HOWEVER, it's not an ideal world. What about putting the test in test_http.py or test_mime where the settings are better suited?

            1. Chris St. Pierre author

              This bug only happens with SSL, and as far as I can tell HTTPS never gets tested. All of the tests check for 'self.scheme', but that's set to 'http'; it only gets set to 'https' when a _client_ gets set up to talk to a server that has an ssl_certificate set, but an SSL cert only gets set up on the server if the supervisor scheme is 'https', which it never is. It looks like you can supply a testconfig module, and if I do that and tell it to use https then I can create a test for this bug, but in that case HTTPTests.test_post_multipart() already is a test for this bug -- it's just not getting run under https (which is the only place this bug surfaces), so it's not catching the bug. At any rate, I'm not sure there needs to be an additional test for this bug specifically. It seems like the better solution would be to make the test suite test via https as well as via http by default, or something like that.

  2. Chris St. Pierre author

    This would seem to be a pretty major bug for anyone hoping to use CherryPy with SSL (e.g., me), so I was disappointed to see that it didn't get included in either 3.2.2 or 3.2.3. Is there anything I can do to help get it included into the next release?

  3. Jason R. Coombs

    I think the main reason it didn't get included was because I wasn't following the ticket closely and it wasn't accepted or approved by anyone else.

    So I've decided to take a look.

    The first thing I notice when I look at the patch is it appears to only be implemented in wsgiserver2.py. Is the bug not relevant in Python 3 (implemented as wsgiserver3.py)?

    Also, there are two cases where 'del data' is called with a comment '# explicit free'. The same comment should be included for consistency.

    Next, that assertion was probably put in place for a reason (maybe just to test a programmer's assumptions), but this patch removes it completely without giving a reason why the check was unnecessary. Instead, it only justifies a single case where the check was undesirable. I would want to review a little deeper what is going on here to see if there is perhaps a better fix.

    Another issue is that this patch is against the default branch, so if accepted and not backported, it still wouldn't have been included in CherryPy 3.2.3 but would be awaiting a release of CherryPy 3.3.

    Finally, since you were not able to add tests for this issue, but identified that if tests ran against HTTPS, that would catch the issue, at the very least, we should open a new ticket indicating to "run tests against HTTPS" so that someone with more experience in the test framework can consider adding support for that.

  4. David Andrzejewski

    This does not appear to be an issue in Python 3. I fired up a test server using the FileDemo tutorial and set it to run in SSL mode. I ran it under Python 2.7.3 and was able to reproduce the issue, and I ran it under Python 3.3 and I was able to upload a 40 meg file via HTTPS.

Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.