modifying the database needs to be done via POST

RESOLVED FIXED in 5.0.7

Status

addons.mozilla.org Graveyard
Public Pages
--
blocker
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: clouserw, Assigned: Mark Wunsch)

Tracking

unspecified
5.0.7
Dependency tree / graph

Details

(Whiteboard: [webmocha])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
/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.
(Reporter)

Comment 2

9 years ago
(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
(Reporter)

Updated

9 years ago
Whiteboard: [webmocha]

Comment 3

9 years ago
Hey Patrick, can you look into this one? Thanks.
Assignee: mikelee → expatrick
(Reporter)

Updated

9 years ago
Blocks: 501406

Updated

9 years ago
Severity: normal → blocker

Comment 4

9 years ago
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.

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 5

9 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 6

9 years ago
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.

Comment 7

9 years ago
Notes from Patrick:

Andrew,
Wil wants all the ajax and non-ajax functions to use post istead of get. I have them all done and submitted in a patch except the non-javascript tag remove links. Can you take a look and advise the best way to make those post forms instead of links?
Assignee: expatrick → andrew

Comment 8

9 years ago
Without javascript your options are very limited. Some options are

1)Make every tag a form and make the x a submit button. I dont think this is feasible or practical, esp for just non-javascript users. It would really bloat out the page for something only used very rarely.

2)Keep the same url but do a form post on the server side somehow. Perhaps via curl.
http://davidwalsh.name/execute-http-post-php-curl

3)The delete link for non-javascript goes to a page built via php which has the right form parameters. It presents the user with a form which then submits via post. 

4) Accept that for non-javascript users they will use get instead of post...


Is there a precedent for this on some other aspect of the site? Perhaps they already do this somewhere and you could utilize that method.

Updated

9 years ago
Assignee: andrew → expatrick
(Reporter)

Comment 9

9 years ago
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.

Comment 10

9 years ago
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.
(Reporter)

Comment 11

9 years ago
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.
(Reporter)

Updated

9 years ago
Attachment #386183 - Attachment is obsolete: true
(Reporter)

Updated

9 years ago
Attachment #386206 - Attachment is obsolete: true

Comment 12

9 years ago
Will do.

Comment 13

9 years ago
Created attachment 386385 [details] [diff] [review]
Get-> ost, combines ajax/non-JS functions, security issues

- All AJAX calls are now done via post.
- Non-Javascript tag adds are done via post.
- Non-Javascript tag removes are not done via POST but need to be - Mozilla agreed to separate that conversation. In this patch, they still use GET.
- add_ajax() folded into add()
- remove_ajax folded into remove()
- $_REQUEST var replaced qith $_POST var for security
- Login is checked for add() tag function.
(Reporter)

Comment 14

9 years ago
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.

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

9 years ago
> 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 ^^.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 16

9 years ago
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.
(Reporter)

Comment 17

9 years ago
(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.

Comment 18

9 years ago
So if you don't want the links to be there at all, do we want to dynamically walk these links in the DOM and add the links in the javascript case? That's the only way I can think of to do this.
(Reporter)

Comment 19

9 years ago
(In reply to comment #18)
> So if you don't want the links to be there at all, do we want to dynamically
> walk these links in the DOM and add the links in the javascript case? That's
> the only way I can think of to do this.

That sounds good to me

Comment 20

9 years ago
This seems like a lot of extra work for a case that happens so rarely. Is it really the case that people using mozilla/firefox and looking for extensions are going to have their javascript turned off? 

However,I cannot think of any other way you would have this (creating the x's via js). I've used the other technique in the past (hiding via css links when js if off) and its worked out pretty well, but it looks like this case is a special one.

Updated

9 years ago
Assignee: expatrick → yem.huynh

Updated

9 years ago
Assignee: yem.huynh → markwunsch
(Assignee)

Comment 21

9 years ago
Wil -- just coming aboard to this bug and want to clarify things. The remaining blocker is really this, correct?:
- Non-Javascript tag removes are not done via POST but need to be

The "X" link discussed here is currently a link performing a GET that removes the tag?

In my opinion, it would be best to have the X perform a POST by turning it into a form. With JS turned OFF, the X will be displayed for all tags. When JS is on, the X will only appear when the tag is hovered over, and the POST will be done via AJAX.

Where will I be dealing with this case? Is this behavior only on ADD-ON pages?
(Assignee)

Updated

9 years ago
Blocks: 501828
(Reporter)

Comment 22

9 years ago
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.
(Assignee)

Comment 23

9 years ago
Created attachment 386802 [details] [diff] [review]
Remove Tags with POST

This seems to be the desired behavior. The tags are encapsulated in a form, and the remove tag link is now a button, and on submit the form posts to remove the tag.

Instead of walking into the DOM, the AJAX Post will Hijax the form data and perform the post.

The desired behavior will also work for users w/ javascript disabled: Removing the tag occurs through a POST. (thus resolving bug 501828)
(Assignee)

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 24

9 years ago
I had to modify the patch so it worked for developer tags.  r29370
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.