Closed benholzman closed 8 years ago
mmerickel
commented
8 years ago This looks like a good fix. It should be easy to test this as most of the tests are integration-level, which means they are infact using webob and should behave identical to your application.
Basically add a test of some batch request that fails, and then works with your fix.
benholzman
commented
8 years ago Interesting. I tried doing that in test_jsonprc.py using the wsgi TestApp but I could not reproduce it.
On Nov 12, 2015, at 5:04 PM, Michael Merickel notifications@github.com wrote:
This looks like a good fix. It should be easy to test this as most of the tests are integration-level, which means they are infact using webob and should behave identical to your application.
Basically add a test of some batch request that fails, and then works with your fix.
— Reply to this email directly or view it on GitHub.
mmerickel
commented
8 years ago https://github.com/Pylons/pyramid_rpc/blob/32b1cd9fd6c4e9a74aabfec15c646f8ff95fb8fc/pyramid_rpc/tests/test_jsonrpc.py#L114-L134 is a pretty straight-up integration test. It seems to be you ought to be able to reproduce it somehow using that as a starting point. Even if you wanted to try passing in a raw body string from one of your apps or formulate the entire request yourself and then do request.get_response(app).
benholzman
commented
8 years ago Thanks, that's the one I had been playing with. I'll give it another shot
On Nov 12, 2015, at 5:37 PM, Michael Merickel notifications@github.com wrote:
https://github.com/Pylons/pyramid_rpc/blob/32b1cd9fd6c4e9a74aabfec15c646f8ff95fb8fc/pyramid_rpc/tests/test_jsonrpc.py#L114-L134 is a pretty straight-up integration test. It seems to be you ought to be able to reproduce it somehow using that as a starting point. Even if you wanted to try passing in a raw string from one of your apps.
— Reply to this email directly or view it on GitHub.
benholzman
commented
8 years ago Ok, I got the unit test working -- fails before my change, passes aftewards.
On Thu, Nov 12, 2015 at 5:48 PM, Ben Holzman ben.holzman@axial.net wrote:
Thanks, that's the one I had been playing with. I'll give it another shot
On Nov 12, 2015, at 5:37 PM, Michael Merickel notifications@github.com wrote:
https://github.com/Pylons/pyramid_rpc/blob/32b1cd9fd6c4e9a74aabfec15c646f8ff95fb8fc/pyramid_rpc/tests/test_jsonrpc.py#L114-L134 is a pretty straight-up integration test. It seems to be you ought to be able to reproduce it somehow using that as a starting point. Even if you wanted to try passing in a raw string from one of your apps.
— Reply to this email directly or view it on GitHub https://github.com/Pylons/pyramid_rpc/pull/41#issuecomment-156259454.
benholzman
commented
8 years ago Hi, wondering if there's anything else you need for this change?
Thanks,
Ben
On Fri, Nov 13, 2015 at 11:39 AM, Ben Holzman ben.holzman@axial.net wrote:
Ok, I got the unit test working -- fails before my change, passes aftewards.
On Thu, Nov 12, 2015 at 5:48 PM, Ben Holzman ben.holzman@axial.net wrote:
Thanks, that's the one I had been playing with. I'll give it another shot
On Nov 12, 2015, at 5:37 PM, Michael Merickel notifications@github.com wrote:
https://github.com/Pylons/pyramid_rpc/blob/32b1cd9fd6c4e9a74aabfec15c646f8ff95fb8fc/pyramid_rpc/tests/test_jsonrpc.py#L114-L134 is a pretty straight-up integration test. It seems to be you ought to be able to reproduce it somehow using that as a starting point. Even if you wanted to try passing in a raw string from one of your apps.
— Reply to this email directly or view it on GitHub https://github.com/Pylons/pyramid_rpc/pull/41#issuecomment-156259454.
mmerickel
commented
8 years ago Sorry, hadn't taken the time to verify the changes yet. Looks good!
mmerickel
commented
8 years ago Can you please submit another pr with your name in the CONTRIBUTORS.txt? I forgot to ask, but I'll need this prior to cutting a release!
benholzman
commented
8 years ago Sure, will do. Thanks!
On Tue, Nov 17, 2015 at 9:48 AM, Michael Merickel notifications@github.com wrote:
Can you please submit another pr with your name in the CONTRIBUTORS.txt? I forgot to ask, but I'll need this prior to cutting a release!
— Reply to this email directly or view it on GitHub https://github.com/Pylons/pyramid_rpc/pull/41#issuecomment-157390476.
We noticed this issue when processing a batched JSON-RPC request originating from chrome with two requests in the batch. The first was quite short, and the second had a lot of parameters and so was very long. We were getting a JsonRpcParseError when trying to process the second request. The issue is that pyramid_rpc is blindly passing all of the original request headers to the Request.blank for the subrequest, including the Content-Length as supplied by chrome. Because chrome serializes JSON with no spaces after separators, but python's json module by default adds spaces after , and :, the actual size of the 2nd subrequest body is larger than the Content-Length provided by the browser, so webob does not read all of the body of the 2nd request, resulting in the parse error.
The fix here copies the headers for each subrequest and removes the Content-Length header if it exists. I'm not quite sure how to unit test this, as it seems the test suite here doesn't actually use webob.