Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1953 (closed defect: fixed)

Opened 11 years ago

Last modified 11 years ago

[PATCH] Add request filter to handle file uploads from buggy Flash clients

Reported by: Chris Arndt Owned by: Chris Arndt
Priority: normal Milestone: 1.1b2
Component: TurboGears Version: 1.1 HEAD
Severity: normal Keywords: needs tests, needs review
Cc:

Description (last modified by Chris Arndt) (diff)

As described in this  post on the TurboGears mailing list, Flash does not terminate the last part in a multipart request correctly with CRLF and CherryPy will timeout while parsing the request, waiting for it multipart part to finish.

The attached patch implements the workaround from  this CherryPy ticket. The patch was included in CP 3 but hasn't made it into CP 2.3 so it's probably best to handle this in TurboGears.

The patch provides no test yet, since I have no idea yet, how to set up a broken request to test the workaround, but field testing shows that TG with the patch is able to handle file uploads from buggy Flash clients correctly, whereas without, it throws a timeout exception.

Attachments

flash-multipart-fix.diff Download (7.4 KB) - added by Chris Arndt 11 years ago.
safemultipart-filter.diff Download (7.7 KB) - added by Chris Arndt 11 years ago.
New version of patch. Renamed FlashMultipartFilter to SafeMultipartFilter
FlashUploadTest-1.0-py2.5.egg Download (117.1 KB) - added by Chris Arndt 11 years ago.
test_mp_upload.py Download (1.6 KB) - added by Chris Arndt 11 years ago.
dev.cfg Download (1.8 KB) - added by Chris Arndt 11 years ago.
test_safemultipart.py Download (2.6 KB) - added by Chris Arndt 11 years ago.

Change History

Changed 11 years ago by Chris Arndt

comment:1 Changed 11 years ago by faide

  • Milestone changed from 1.5 to 1.1

Changed 11 years ago by Chris Arndt

New version of patch. Renamed FlashMultipartFilter to SafeMultipartFilter

comment:2 Changed 11 years ago by Chris Arndt

  • Status changed from new to assigned
  • Owner changed from anonymous to Chris Arndt
  • Description modified (diff)

I'm having trouble writing a working unit test case for this patch, so I have written a small example project that servers as a demonstration of the problem and as a functional test for the solution (if you apply the safemultipart-filter.diff patch).

Download & install the example project, apply the patch to TG 1.1 and run the app with TG 1.1. Try out the two different upload pages. One posts to the URL /okupload1 that has the SafeMultpartFilter enabled, the other posts to /failupload1 where the filter is not active. Please note that the bug manifests itself only when using a Mac OS Flash client''

You can also download the attached script test_mp_upload.py that sends a crafted, malformed multipart request to simulate a Mac OS Flash client to the same two URLs.

I have also attached my attempt at writing a unit test for this, but there are two problem with testutil.

  1. There is a bug in wsgiref.validate.InputWrapper: it's readline method does not accept arguments and thus breaks the MultipartWrapper from my patch which uses the optional size argument that the standard readline method of file objects accepts.
  1. Even when fixing wsgiref I can not reproduce the bug with the testutil request setup. Apparentyl webtest.TestApp.do_request() somehow mangles the request so that it is correctly parsed by CherryPy, even if the original multipart request body was malformed.

If anybody has an idea how to write a proper unit test for this I'd be grateful to hear about it!

Changed 11 years ago by Chris Arndt

Changed 11 years ago by Chris Arndt

Changed 11 years ago by Chris Arndt

Changed 11 years ago by Chris Arndt

comment:3 Changed 11 years ago by Chris Arndt

  • Milestone changed from 1.1 to 1.1 beta 2

comment:4 Changed 11 years ago by Chris Arndt

The bug in wsgiref is here:  http://bugs.python.org/issue3834

comment:5 Changed 11 years ago by Chris Arndt

  • Keywords needs tests, needs review added

comment:6 Changed 11 years ago by Chris Arndt

One caveat: the example project will NOT work with Flash Player 10 which was released recently. I'll add an updated example soon.

comment:7 Changed 11 years ago by faide

PJE filed the wsgiref ticket as invalid. This means we won't get support for the tests... Shall we go ahead and commit as-is ? Do you intend to add support for flash 10 ?

comment:8 Changed 11 years ago by Chris Arndt

We can submit this without a proper test, the example project shows that it works. We just won't have regression protection.

Adding Flash 10 support is not a TurboGears matter, it is the FancyUploader? Flash/JS that breaks, so there should be nothing to change one this is fixed.

comment:9 Changed 11 years ago by Chris Arndt

Applied (without tests) in r5666.

comment:10 Changed 11 years ago by Chris Arndt

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

Added documentation to 1.1. configuration reference at  http://docs.turbogears.org/1.1/Configuration.

Created new ticket #2032 for the test issue.

Note: See TracTickets for help on using tickets.