Item2128: Item2115 (upload CSRF) needs merging to trunk

Also the unit tests are currently banjaxed. The workaround we have in 1.0.7 is OK, but not a long term solution IMHO.

The problem is this:

1. User visits Attach screen, enters name of file and hits Upload
2. Request is sent with the token from the Attach screen
3. Server processes request, expires the token, and uploads the file
4. User hits "back" in browser to get back to the Attach screen.
5. User enters a new filename and hits Upload
6. Request is sent with the original (now expired) token from the Attach screen
7. Server sees the key is invalid, and redirects to the confirmation screen by 302
   • The redirect caches the request, but does not cache the uploaded file
   • when the upload request finishes, the file is removed during DESTROY
8. User gets the OK/Cancel confirmation dialog and hits OK
9. The file has gone from the server (in step 7) so the upload fails

In step 7 we need to be able to cache the uploaded file, but there are a couple of problems with doing that:

1. Uploaded files can be very big, and caching the already cached upload could push the server to its limits
2. Expiring redirect caches becomes more complex than simply deleting the files; now we would have to locate and remove any cached uploads.

Obviously you can get around this by clearing {Validation}{ExpireKeyOnUse}, but that's not a great idea as it weakens the protection considerably.

-- CrawfordCurrie - 21 Sep 2009

Thinking about it more. If you cache the file on the server, then you are opening a potential hole that could be exploited by filling up the disk with cache files to DoS or otherwise kill the server. So it might be better to reject the request on a validation failure. If you do that, then no upload can happen without a minty fresh validation code.

Merged and closed.

-- CrawfordCurrie - 16 Nov 2009

Re-opened to fix a bug in the fix. Testing a hashref using if( $hashref ) will always return true, one has to explicitely bind it to a hash to test its content.

Also cleaned up the useless(?) global $uploads, and the default loading of Data::Dumper (to save performance and memory).

-- OlivierRaginel - 18 Nov 2009