Item2612: Issues with beforeSaveAttachment plugin dispatch (trunk)

Because I needed a closed bug item for 1.0.9 I split the problem up in two.

This is the same bug as Item2529 but for trunk.

See 2529 for the full story.

-- KennethLavrsen - 09 Jan 2010

Can't see what I can contribute here as Item2529 clearly goes over my head.

-- MichaelDaum - 25 Jun 2010

I cannot remember why it was waiting for Sven and Michael.

Item2529 was fixed by someone not working on trunk and the code has since been changed.

But the reporter Tim claims that from looking at the code we have the same potential problem.

We need someone with strong perl knowledge to review the change in 2529 and merge over "the principle" - if applicable.

Setting back to New.

-- KennethLavrsen - 12 Jul 2010

OK, there are some important design issues here.

First, as Timothe states, "There is no good solution without changing the plugin API - we really should be passing the stream, not the temp filename" - absolutely. However one plugin in SVN does write the attachment in the beforeAttachmentSaveHandler (MaxImageSizePlugin). A number of others read the attachment data (BatchUploadPlugin, AttachmentFilterPlugin, BlackListPlugin, AntiWikiSpamPlugin). Besides the CGI 'upload' script, a number of other extensions invoke Foswiki::Func::saveAttachment, any of which may result in an unseekable stream being sent to the handler. However who cares? The core code doesn't do a seek, the API doesn't pass a stream to the handler.

We need to weigh the alternatives. There are many more extensions that use the handler to analyse the filename (or protections), and just don't care about the data. Copying it is expensive, which is why I switched back to the original (tm)wiki method of using the CGI copy of the file, rather than copying it to a temporary file. The risk of the stream not being seekable is limited to attach calls coming from within extensions, as the stream for an upload comes from CGI (or whatever Engine is in use, which is under our control). Also, given that this is all happening within the context of a single request, exposure to race conditions is limited. Against these risks you have to balance the risk of DoS resulting from copying massive attachments into limited disk space.

Having said that, there is a clear need to fix this problem, once and for all, and deprecate the broken handler as soon as possible.

Timothe describes some actions that need to be acted on for 1.1.

• For performance, I'd suggest only doing the copy if the input stream isn't seekable. Rather than use -f or !-p guesswork, I think I'd try a seek before dispatching & copy only if it failed.
• But we need to document that plugins should NOT use the filename in an open or close.

Other remarks relate to the design of an improved API, which is out of scope for 1.1

• We should pass the stream to the plugin. The /dev/fd hack doesn't work everywhere, and for debugging, the tempfile name is more useful in error messages.
• To support writes/replacement, we should allow the plugin to replace the stream with a new one.
• We need a release note to the effect that any plugin that uses the callout needs to be updated. For a low-medium security risk if it just reads the attachment. But if it writes/replaces it, for correctness.
• Distributed plugins need to be checked for use of this callout & updated. For phase-in, they probably should check for the stream (needs a new name as we currently pass a useless parameter of that name) and fallback to the filename otherwise. This would be easier/safer if we update the patch for V1.0 to pass the stream (in both directions). We can't force a synchronous update across the customer base -- particularly with plugins that we don't know about.
• Since we support TWiki plugins, we need to coordinate with that community. I've raised this issue with them (and recommended that they monitor this topic & coordinate with us). Their tracking item is rather content-free as the conversation has been in e-mail.
• We need regression tests for this.

Later: of course, the new API is already there; Michael had to implement it for the other various bugs that were floating around. So I polished and finished the doc and wrote Michael's unit tests for him (how unusual!)

-- CrawfordCurrie - 12 Jul 2010

File::Temp->seek only available in File::Temp 0.17 and later

-- CrawfordCurrie - 18 Jul 2010