/tags/add_ajax/ needs to add tags from POST, not GET. This is blocking any UTF-8 tags from being added right now. Same with blacklisting and any other code that writes to the db. Our regex filter in bootstrap is going to cause trouble for us when we want to do stuff like preview.addons.mozilla.org/en-US/firefox/tag/下載 but we'll deal with that when we get there. Any create/update/deletes should be done via POST for security reasons (using the anti-CSRF code. This is mostly automatic - just look at the other forms for an example).
Does adding tags work at all? I'm trying to add 'sloppy' to 2848 and the GET comes back with a 404.
(In reply to comment #1) > Does adding tags work at all? I'm trying to add 'sloppy' to 2848 and the GET > comes back with a 404. works for me on preview. My dev copy is blocking on bug 500585#9
Hey Patrick, can you look into this one? Thanks.
Created attachment 386183 [details] [diff] [review] Causes all tag form submissions, AJAX and otherwise, to be done via POST. I hope these patches don't conflict with each other - if you apply them in sequence I think they should work. Please let me know if it doesn't and I'll make one with all my changes against the head.
Hey Patrick, I only see 1 patch, so I'm not sure which patches you're talking about, although this one does fail to apply. Can you make a new one real quick? Also, comments on the patch: - you only changed add_ajax. There is at least removing tags that needs to be changed as well - looking at controllers/tags_controller.php remove_ajax() and remove() specifically check the user is logged in but add_ajax() and add() don't. Should they? - Please only use $_POST and not $_REQUEST. Using $_REQUEST defeats the purpose of the CSRF check.
Created attachment 386206 [details] [diff] [review] Uses post for tag remove; checks for login in ajax add rather than subsequent function, uses $_POST instead of $_REQUEST This does not resolve moving to POST for the non-ajax remove functionality.
Patrick: Why are the ajax function split out from the other functions? They are practically the same thing and just duplicating code. Andrew: We can split non-js functionality into another bug.
Wil: Before we moved to POST it made more sense to me to do it that way; now I'll just pass in a parameter and move it into one function. I see the POST patch I uploaded hasn't been added - did it fail as well, or do you want me to make another that adds the post functionality and combines the ajax add functions? My patch probably failed because I sent a diff, made more changes, and sent another diff. I have just pulled down a fresh checkout (but kept my changes) so I can apply them to my fresh checkout, or wait for you to apply the patch and commit; let me know.
Hey Patrick - go ahead and just make one patch so we can keep things straight. Be sure to update - I made a bunch of changes to that file last night (mostly in other functions, but still). I'll mark the patches so far on this bug as obsolete. Thanks.
Thanks for the patch. I filed bug 501828 for the JS disabled case. That is a low priority at this point. I committed your patch in r29244 with a couple minor changes. I removed the ability to remove tags via GET - I'd rather not have the ability at all than have that security hole. The only thing I didn't change, and getting a patch from you would be great, is removing the links to remove tags in the non-JS case. We'll figure something out in bug 501828.
> The only thing I didn't change, and getting a patch from you would be great, is > removing the links to remove tags in the non-JS case. Why was this closed? Reopening for ^^.
Andrew submitted a patch to remove the X's to 501828. I'll put it in DIFF form later and attach it to this bug.
(In reply to comment #16) > Andrew submitted a patch to remove the X's to 501828. I'll put it in DIFF form > later and attach it to this bug. Sounds good. Be sure to read my comment on that bug.
It's just on add-on pages at the moment. How about we make one big <form> around all the tags, and have the [x] submit buttons just have the tag id in the submit id. Something like <input name="remove-tag-$tagid"... Then we would just have one form instead of a ton of small forms. There is already a function in the addon_html helper that makes an image a submit button so this shouldn't be too bad.
I had to modify the patch so it worked for developer tags. r29370