320082 - Comment field length needs a sanity check

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Crogon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

I discovered this fiddling around with some of the form filling extensions. The form field length is reported as unknown by the extension. I typed in some nonsence, then copy / pasted for about a minute. No end of field. I'm guessing it's 65,000 in the database, but the webpage itself needs a sanity check. Personally I can't see more than 1,000 ever being necessary, perhaps 500. Anyway, currently, a user could hose the page loading by submitting an insanely long comment. Note I didn't actually submit a comment like that, so if there is a sanity check post form submit, I apologize for the bug report in the first place.

Reproducible: Always

Steps to Reproduce:
1. Click any extension 'add your own comment' link
2. Fill in the form with trash over and over
3. Click Post if you want to REALLY annoy the moderators. ;)

Actual Results:  
Not tested, but hung page load or rediculously long page

Expected Results:  
Decently long extension page

Most likely just needs a form field length on the actual page coding. I'm marking this security, as I've seen other bugs marked severe for such form length issues.

Comment 1

[alanjstr]

We need a sanity check, if just to prevent a user from submitting something huge.  The DB probably defines it as TEXT, but that doesn't mean we should allow it.

http://lxr.mozilla.org/update1.0/source/core/postfeedback.php#144

Comment 2

[Crogon]

(In reply to comment #1)
> We need a sanity check, if just to prevent a user from submitting something
> huge.  The DB probably defines it as TEXT, but that doesn't mean we should
> allow it.
> 
> http://lxr.mozilla.org/update1.0/source/core/postfeedback.php#144
> 

Changing out a live database field type / length is dangerous at best, at the least changing it could chop off someones post or simply delete all oversized posts from the database table, at the worst, it could confuse some embedded code (see - god knows where) and crash out the page generation.

I don't know exactly what you guys are using for page generation (quite frankly I'd like to get my hands on some of it to adapt into some phpbb stuff!), but the 'safer' solution is to throw some sort of field length limiting code / widget into the form itself.

Of course, depending on your database backup software, it may be WELL worth taking the database offline temporarily to bit of tweaking, in order to save the space on storage and backup. The storage bit is easy enough to figure out. 'Are we going to run out of database space on the hard drive partition or not?' Some backup software is smart enough to chop off the 64k worth of blank space in each field, some backup software doesn't consider TOUCHING the fields inside the database.

Personally, I'd test out any solution on a 'closet' system first. If I were calling the shots, I would:

1) Patch (at least temporarily) the field length in the form to limit it to a sane length.
then..
2) Research wheather or not there would be a storage or backup benefit to reducing the database field size.

Just a shot in the dark, but I'm gussing that ten years from now, there wouldn't be but a few thousand (at most) Firefox extensions, so it is probably not going to be worth the time and storage to mess around with the database field size / type. ..unless there are a great number of these extremely long fields inside other tables that no one has run across yet.

At any rate, these are just my opinions. I'm ranked 'nobody' on this totem pole, so I would defer to anyone who actually knew what they were talking about (database type, etc., etc.). ;)

Comment 3

[alanjstr]

I would make an assumtion about reasonable comment length, say 255 chars.  Not only would the form have a maxlength, the php would enforce it.  After all, someone could remove the property from the form field in an attempt to crash the DB or cause web pages to not draw.

Comment 4

[Crogon]

Oh no! Not to disagree, but my last comment was exactly 984 characters long. https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=267 Given, I am currently researching some of the 'top-of-the-line' extensions in Firefox and providing as much info as possible about why they aren't working in FF 1.5. That way every single Firefox user doesn't need to waste all those hours doing the same thing. So, it IS a special situation, but a needed one, and one that is likely to pop up again for FF 2.0, 2.5 and etc. I think 1000 characters would be sufficiently long for any purpose. Even if I had more info to provide, I could have gone back and chopped the post down a bit if it was oversized.

Now I do agree with the form field method being the incorrect way to do it. What you want to do is set it up so that when the user clicks 'Post', instead of submitting directly to the database, it sends to some server side script that checks the length. If it's within boundaries, the script submits it, then generates the new page (or in this case, the comment page). If it's not within boundaries, it generates a static page saying 'Your comment is too long, the max field length is xxxx characters. Please click the back button on your browser, repair the problem, and try again.' If you wanted to be cute, you could add in 'Your comment was xxxx characters long.'

Now then, the next thing you are going to run into is some jerk submitting a 500 character long 'word'. That would be best dealt with, with the exact same sort of server side script. You could probably combine the two scripts actually. Actually the funner thing to do is to use a script that runs a sanity check on word / line length, then auto crops if necessary, then posts the repaired field. This will break very long URLs, but will cause any trouble makers trying to intentionally throw off the page width to start cussing and beating on their keyboard. :D

Once again, just my opinions, I have no clue what you guys actually have 'under the hood'.

Comment 5

[Michael Morgan [:morgamic]]

Thanks for your comments -- I will take a look at this issue this afternoon.

Comment 6

[Michael Morgan [:morgamic]]

This is an optimization / validation issue, but it's not a serious security issue.  

The textarea data (and all other data) is escaped.  Users are still limited to the size of the MySQL text column type (which I agree is too large as a reasonable limit).

Anyway, will add some app logic to limit comment length this afternoon.

Comment 7

[Michael Morgan [:morgamic]]

Attachment: Set an arbitrary length (and error #) for comments > 2000 chars.

Comment 8

[Michael Morgan [:morgamic]]

Tested and verified -- will be added in next update.