Last modified: 2011-09-12 18:00:20 UTC
Upload campaigns are deleted with a simple GET request, without confirmation. Make a sysop visit http://commons.prototype.wikimedia.org/wiki/Special:UploadCampaigns/del/campaign and it's gone.
Right. I forgot to do a token check. Will fix tomorrow.
Fixed by r95880. Not sure this is the best way to deal with it though, if there is a better one, please enlighten me :)
Two problems with that revision: - First, you are using the plain, unsalted edit token in the url. This discloses the secret in eg. proxy logs. We always salt the tokens with the modified data in such cases so that once consumed they can't be reused (but see below). - Second, it is still deleting on visiting the page. The main risk is fixed, but what would happen if a sysop presses delete when he wanted to press edit? I think that pressing delete should lead you to an intermediate page, where you should press a button to actually delete the campaign (just as anonymous purge or normal deletion). Linking with bug 30645, it could be a copy of the usual deletion interface, logging a deletion comment and storing the last data in the log.
> what would happen if a sysop presses delete when he wanted to press edit? There is a confirmation dialogue. That ofc won't work w/ JS disabled, but then you are sort of shooting yourself in the foot IMO. > I think that pressing delete should lead you to an intermediate page An intermediate page would definitely make sense when there is something more happening then a simple delete (ie for providing a deletion reason as you suggest). > We always salt the tokens with the modified data in such cases so that once consumed they can't be reused. Is there documentation on this? I'm not sure how to proceed. What data should I use as salt?
> > what would happen if a sysop presses delete when he wanted to press edit? > > There is a confirmation dialogue. That ofc won't work w/ JS disabled, but then > you are sort of shooting yourself in the foot IMO. Good. It wasn't there yesterday :) > > I think that pressing delete should lead you to an intermediate page > > An intermediate page would definitely make sense when there is something more > happening then a simple delete (ie for providing a deletion reason as you > suggest). It would be the "right" solution (semantic reasons, no javascript dependency...). > > We always salt the tokens with the modified data in such cases so that once consumed they can't be reused. > > Is there documentation on this? I'm not sure how to proceed. What data should I > use as salt? Maybe not. I remember I had this same talk with someone in CR. There may be more info there. You can have a look at how rollback or patrolling links are made. Just pass the dependent data as a parameter to $wgUser->editToken(). In this case I would pass $campaign->campaign_name. There's not much to put there in this case, although it has the weakness that if someone recreated the campaign, the old token would still be able to delete it.
> Good. It wasn't there yesterday :) The dialogue has been there since I created the deletion link. > It would be the "right" solution (semantic reasons, no javascript dependency...). This makes me wonder what do do for an interface similar to what we have now, but where deletions are made via HTTP request to the API, after getting a dialogue (which also allows entering data such as deletion reason). I think such an approach is more user friendly then having a separate page. > In this case I would pass $campaign->campaign_name. There's not much to put there in this case, although it has the weakness that if someone recreated the campaign, the old token would still be able to delete it. Right, since this is just deleting, we're getting of rather easily. Just adding a campaign constant, the campaign name, and the campaign id should yield something unique. But what to do for edit actions, ie for a API module that allows modifying one or more settings of an upload campaign (if there was such a module)?
> > Good. It wasn't there yesterday :) > > The dialogue has been there since I created the deletion link. No. It didn't prevent me from removing a campaign yesterday when I followed it (naively expecting to see a confirmation form). > But what to do for edit actions, ie for a API module that > allows modifying one or more settings of an upload campaign (if there was such > a module)? Not using GET? If you are using AJAX you pass the parameters in the body of the request.
> If you are using AJAX you pass the parameters in the body of the request. These can still be obtained by third party when not using SSL or other encryption right?
I put in the id and name as salt in r95976
> I put in the id and name as salt in r95976 Looks good. > These can still be obtained by third party when not using SSL or other > encryption right? Yes, but we are not trying to protect from a complete sniffing.
Well, in that case I'd also not consider having the param in GET a problem. The difference between the two is just some effort, not real security. I just tried to have such a dynamic hash for the token of an API module, and apparently this is not supported. If it's good enough for the API, why would it not be for some UI? Or am I wrong and can you salt tokens used in the API with some variables?
Salted tokens are supported in the API, see ApiRollback.php in core. There is a general paradigm that GET requests should not be able to change things; deletions and creations and such should always use POST.
> Salted tokens are supported in the API, see ApiRollback.php in core. I guess I made some wrong assumptions there. Awesome! :) > There is a general paradigm that GET requests should not be able to change things; deletions and creations and such should always use POST. Sure, will do that in the future. Worth changing here though?
Ok, will implement the POST stuff soonish.
Fixed by r96575. Now doing post call to API.
Yes, anything that modifies server state should be a POST. To be more precise; any GET request ought to be repeated any number of times and get the same result. http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html You can make exceptions but only if there's absolutely no other way to achieve your needed functionality.
