bugSamizdat - Bugs: bug #20710, edit conflict for 2 users...

 
 

bug #20710: edit conflict for 2 users uploading files is resolved in a way confusing to both users

Submitter:  Boud Roukema <boud>
Submitted:  Tue 07 Aug 2007 10:36:14 AM UTC
   
 
Category:  None Severity:  4 - Important
Status:  Fixed Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Mon 30 Mar 2009 11:47:07 AM UTC, comment #4: 

This bug is fixed in CVS at 2009-03-30 11:35. The new upload queue mechanism assigns unique id to each upload, making it impossible for one user to confirm another user's upload. Queue size is configurable via limit:pending_upload_queue_size config option and is set to 3 by default, allowing for concurrent uploads to work even for guest users.

Dmitry Borodaenko <angdraug>
Group administrator
Tue 22 Jul 2008 09:25:37 PM UTC, comment #3: 

See the method check_and_delete_pending_uploads in content.rb in the multi_publish-0.6.2 patch:

https://savannah.nongnu.org/patch/index.php?6143

This should work against 080703-2.

The idea is that if there are more than a reasonable number of unconfirmed uploads for a given user (no distinction between guest and other users), then an attempt is made to delete the oldest unconfirmed uploads in excess of the desired reasonable number. However, uploads which are too recent will not be deleted, even if there are too many of them. The algorithm could be varied in many ways to make it better distinguish real users from DoSers, but just varying the two parameters already gives a lot of control.

> We need a way to limit how many uncommitted previews we can keep around,


check_and_delete_pending_uploads uses config['limit']['pending_uploads'] and *config['timeout']['uploads'].

> and a policy on when to clear them up,


A file is normally saved when someone does a preview. Since this requires some disk access anyway, it's probably a good time for checking for an excessive number of pending uploads.

> and when to tell a user that they can't upload a file until other uploaders' files are moved out of the way.


Just a few moments later. :). If check_and_delete_pending_uploads fails to delete enough files, then the user is told this. Depending on the balance between giving too much info to DOSers and too little information to valid users, we could possibly make the message i suggest either less or more informative respectively.

An independent look at this would be good, but at least for me this seems to be at least one way of solving the bug. (Using the Tempfile functionality already in ruby might be another - the problem of creating temporary files which have to be sooner or later cleaned up when space is needed or they're too old is not a problem unique to samizdat!)

Boud Roukema <boud>
Group Member
Sun 24 Feb 2008 11:54:21 PM UTC, comment #2: 

Agree with all three points.

Boud Roukema <boud>
Group Member
Sat 23 Feb 2008 01:24:48 AM UTC, comment #1: 

Downgrading to Important: this behaviour has been exactly the same since Samizdat 0.5.0 back in 2003, if it didn't block all the releases since then, it shouldn't block the next release, either.

Registered users are not affected by this bug as long as they don't upload and preview several images in parallel. It always was a risk for guests, though, and I agree that this needs to be addressed.

The proposed solution to generate different file names for each upload is not good enough as it opens a way to DoS the site by using up disk space with duplicated uploads. We need a way to limit how many uncommitted previews we can keep around, and a policy on when to clear them up, and when to tell a user that they can't upload a file until other uploaders' files are moved out of the way.

Dmitry Borodaenko <angdraug>
Group administrator
Tue 07 Aug 2007 10:36:14 AM UTC, original submission:  

VERSION:

samizdat-0.6.0.070618-1

PROBLEM:

george starts (anonymously) publishing an article with what
on his computer is a file george.jpg with his smiling face.
He hits preview, has a look at the preview, and then goes
away to have coffee.

A few moments later, ossama starts (anonymously) publishing
an article with what on his computer is a file ossama.jpg
showing his smiling face. He hits preview, has a look at the
preview, and then goes away to have coffee.

Meanwhile, back on the ranch, george comes back from coffee.
He looks at his browser screen, he sees his photo and the
button "confirm", he yells "Yeehah!" and he hits "confirm".

george then sees his name as author but... a lovely photo of
ossama instead of his own.  His face goes white in shock and he
concludes that ossama has hacked into the White
House computer and he launches some nuclear warheads
into Eurasia somwhere as revenge.

ossama then gets back to his own browser, hits confirm and
gets a Runtime Error. He concludes that western technology
is crap.


ANALYSIS:

From the server, it's clear that the file upload.jpg initially
posted by george got replaced by ossama before george decided
to confirm. When george confirmed, he published ossama's picture.
When ossama confirmed, no previewed picture was available,
hence the error.


POSSIBLE SOLUTIONS:

Some modification of content.rb e.g. to the  upload_id  method
e.g. with some sort of digest( ) for the file names would
presumably do this.  It would then be good to have something
somewhere in the system to clean up (remove) old files, e.g.
older than 1 week. 

i'm not actually giving a concrete proposal right now (since i
spent quite a bit of time on a multi_publish patch coming up
soon...)

In principle, a standard temporary file creation mechanism
could be used, but see the discussion below for a
counterargument why it would be better to at least keep
previewed files for some reasonable delay due to the typical
use of samizdat. Moreover, we have to give a user some reasonable
delay to decide to confirm or not.




DISCUSSION:

If users publish after logging in, then there is less chance
that they will have a self edit conflict (though it's still
possible logically). However, anonymous users should not be
subjected to this bug.

However, there could be situations were someone posts
a photo in a hurry - e.g. from a cell phone with internet
access, and gets grabbed by the authorities before s/he
manages to hit confirm.  Do we really want to "throw away"
this photo quickly?  In exceptional situations (such as
government repression of a demonstration), surely it would
be better that admins can rescue such photos, even though
this would have to be an exceptional "hack", not a standard
procedure.

i put this at "blocker" level since this sort of edit
conflict could not only discourage users but also lead
to loss of useful uploads.

cheers
boud

Boud Roukema <boud>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Carbon-Copy List
  • -email is unavailable- added by angdraug (Posted a comment)
  • -email is unavailable- added by boud (Submitted the item)
  •  

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-03-30 angdraug StatusWorks For Me Fixed
        Open/ClosedOpen Closed
    2008-07-22 boud Dependencies- Depends on patch #6143
    2008-07-22 boud StatusConfirmed Works For Me
    2008-02-23 angdraug StatusNone Confirmed
    2008-02-23 angdraug Severity5 - Blocker 4 - Important

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code