187753 - Specify a maximum length for quips

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Brant Gurganus]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20021231
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20021231

It seems that quip length is not checked.

Reproducible: Always

Steps to Reproduce:
1. Go to quips.cgi.
2. Add a huge quip.
Actual Results:  
Quip is accepted.

Expected Results:  
Quip is not accepted over fifty characters or so.

This borders on a security bug so I am setting high severity.  By the way, I
probably pissed www.w3.org off, but I apologized.  I also tested this with the
latest tip at landfill.mozilla.org and it was still present.

Comment 1

[Bradley Baetz (:bbaetz)]

The quips table uses TEXT, aka 65535 characters. Quite why it does this I'm not
sure, but 255 may be a bit small.

Comment 2

[Jacob Steenhagen]

A quip is supposed to be a short little bit of nothing... 255 characters should
be more than enough room to say nothing.

Comment 3

[Brant Gurganus]

For these various bugs dealing with lenght of quip, summary, etc. can't you just
use the width attribute on the input element and not mess with any of the CGI
and databases?

Comment 4

[Dave Miller [:justdave]]

The database is the reason we're complaining.  TEXT fields aren't very
cross-database portable, so the more we can stuff in VARCHARs the less special
handling we need to do.

Comment 5

[Vlad Dascalu]

This seems pretty simple to do.

Comment 6

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 7

[Paul]

when the field was text, nobody had to make a judgement call on the size of the field.  That was nice, but now someone needs to agree to a limitation.  Before making this change I would suggest finding the database, that  you want to support that doesn't have an equivalent text field as mysql does.  This would be just to provide a real case for making this.  Otherwise it should be fine as TEXT.

Comment 8

[Paul]

TINYTEXT fields are 255 characters
TEXT fields are 65,535 characters
MEDIUMTEXT fields are 17 Mb

We have the best datatype being used right now.

I recomment closing this bug WONTFIX

Comment 9

[Frédéric Buclin]

there is no patch to approve.

Comment 10

[Crogon]

(In reply to comment #8)
> TINYTEXT fields are 255 characters
> TEXT fields are 65,535 characters
> MEDIUMTEXT fields are 17 Mb
> 
> We have the best datatype being used right now.
> 
> I recomment closing this bug WONTFIX
> 
You can code a sanity check into the web page without screwing around with the database field size.

Comment 11

[Paul]

So as far as you're concerned this would not be allowed as a quip:

Books give you knowledge, and knowledge is power, and power corrupts, and corruption leads to crime, and crime doesn't pay, so if you read books you'll go broke." -Calvin & Hobbes, I think...

because it's a whopping 192 characters.

Please state the exact limitation you want to see with no fuzziness in the number like "or so", "give or take", "more or less", etc.

I don't know why we are even considering a change like this.

Comment 12

[Dave Miller [:justdave]]

TEXT is bad, unless you really need it.  Primary reason is it can't be indexed.  On the other hand, why would you ever want to index a quip? :)

But yes, generally getting rid of generic blob fields except where necessary is still a good idea in database design.  I think 1K is reasonable (varchar(1024)?) if databases support varchars that big.

Comment 13

[Max Kanat-Alexander]

I think that before MySQL 5, the maximum varchar length is 256. Beyond that I think you have to use mediumtext. It could be 512, which would be more acceptable.

I think 256 characters is long enough for a quip. :-)

Comment 14

[Paul]

"Today we tend to go on for years, with tremendous investment to find that the system, which is not well understood to start with, does not work as anticipated.  We build systems like the Wright brothers built airplanes - build the whole thing, push it off a cliff, let it crash, and start over again." -- R.M. Graham, 1969

324 characters.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> "Today we tend to go on for years, [snip]

  Okay, good point. I've seen longer things than that come out of "fortune," too.

  The problem is that in MySQL a varchar can't be longer than 255 characters, and in other databases it's somewhat problematic to have too many non-varchar text fields. In particular, in Oracle it's fairly easy to have a field up to 4000 characters, which would be totally fine for quips. The only trick is how to indicate this in Schema.pm in a way that's cross-database compatible.

Comment 16

[Paul]

As far as I can tell, this is not a bug and certainly not an enhancement...

It's just a bunch of database purist ideas floating around.

Comment 17

[Max Kanat-Alexander]

No, it's a problem because you don't want 65535 characters displayed at the top of a buglist.

Comment 18

[Paul]

That's a nice policy for your site, and it can be managed locally.

Comment 19

[Koosha KM]

Attachment: patch - v1

The number chosen for the length is my choice based upon the discussion in comments. Also, should there be any change in DB schemas?

Comment 20

[Frédéric Buclin]

Since MySQL 5.0.3, varchar() can be used with lengths up to 65535. Since Bugzilla 4.2, we require MySQL 5.0.15 or newer, so that's fine.

PostgreSQL seems to accept lengths for varchar() up to very high values, much higher than MySQL; same for Oracle. SQLite ignores the length passed to varchar() and converts it to text anyway.

So it seems we could now use varchar(512), for instance. And yes, I'm in favor of enforcing this in the DB, i.e. to alter the DB schema. Quips longer than 512 characters should simply be truncated.

Comment 21

[Koosha KM]

Attachment: patch - v1

Comment 22

[Frédéric Buclin]

Comment on attachment 631658 [details] [diff] [review]
patch - v1

>=== modified file 'Bugzilla/Constants.pm'

>+# Maximum length of a quip (in characters) as set by DB schema (varchar(512)).

Simply say "Maximum length of a quip".



>=== modified file 'Bugzilla/DB/Schema.pm'

>-            quip     => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
>+            quip     => {TYPE => 'varchar(512)', NOTNULL => 1},

You also have to fix the type for existing installations, see Bugzilla/Install/DB.pm.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+    [% title = "Too Long Quip" %]

"Quip Too Long" would be better.


Otherwise looks good.

Comment 23

[Koosha KM]

Attachment: patch - v1.1

Comment 24

[Koosha KM]

I think it would be a good idea to change the quip text field to a text area to support multi-line quips.

Comment 25

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #24)
> I think it would be a good idea to change the quip text field to a text area
> to support multi-line quips.

Probably a good idea, but let's do that in a separate bug, please. :)

Comment 26

[Frédéric Buclin]

Comment on attachment 631686 [details] [diff] [review]
patch - v1.1

>=== modified file 'Bugzilla/Install/DB.pm'

>     $dbh->bz_alter_column('quips', 'quip',
>-        { TYPE => 'MEDIUMTEXT', NOTNULL => 1 });
>+        { TYPE => 'varchar(512)', NOTNULL => 1 });

Has this patch been tested on an existing installation? This code is not triggered on Bugzilla 3.2 and newer due to the check done earlier in _change_text_types(). When altering the DB schema, make sure that all scenarios have been tested (new installs + upgrade).

Comment 27

[Koosha KM]

Attachment: patch - v1.2

Check this out.

Comment 28

[Frédéric Buclin]

Comment on attachment 637455 [details] [diff] [review]
patch - v1.2

>=== modified file 'Bugzilla/Install/DB.pm'

>+    # 2012-06-28 [e-mail] - Bug 187753
>+    $dbh->bz_alter_column('quips', 'quip',
>+                         { TYPE => 'varchar(512)', NOTNULL => 1, DEFAULT => "''"});

DEFAULT => "''" should go away. We don't need it here.

Before altering the column, you first have to truncate too long quips and report their ID to the admin, else PostgreSQL crashes (and I suppose Oracle will crash too):

DBD::Pg::db do failed: ERROR:  value too long for type character varying(512) at Bugzilla/DB.pm line 708
        Bugzilla::DB::bz_alter_column_raw('Bugzilla::DB::Pg=HASH(0xa70aae0)', 'quips', 'quip', 'HASH(0xa8f0178)', 'HASH(0xa5ddbd8)', undef) called at Bugzilla/DB.pm line 667


Please make sure to test your patch carefully, especially when it touches DB changes.

Comment 29

[Koosha KM]

Would that be a good idea to save long quips to a file before deleting them?

Comment 30

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #29)
> Would that be a good idea to save long quips to a file before deleting them?

No need. Instead of deleting them, I think truncating them should do it.

Comment 31

[Koosha KM]

Attachment: patch - v1.3

Tested on PostgreSQL and worked fine.

Comment 32

[Frédéric Buclin]

Comment on attachment 647682 [details] [diff] [review]
patch - v1.3

This looks good and PostgreSQL no longer crashes. There are still a few things to fix, though, some of which I didn't notice during my last review.


>=== modified file 'Bugzilla/Install/DB.pm'

>+    # 2012-07-31 [e-mail] - Bug 187753
>+    _shorten_long_quips(); 
>     ################################################################
>     # New --TABLE-- changes should go *** A B O V E *** this point #
>     ################################################################

Add a newline between _shorten_long_quips() and the comment below it.


>@@ -3159,7 +3161,7 @@
>     $dbh->bz_alter_column('quips', 'quip',
>-        { TYPE => 'MEDIUMTEXT', NOTNULL => 1 });
>+        { TYPE => 'varchar(512)', NOTNULL => 1 });

I just realized that if someone is upgrading from Bugzilla 3.0 or older directly to 4.4 and there are very long quips, this change is going to make checksetup.pl to crash with the same error message as in comment 28. So we have two ways to fix this problem:
a) call _check_content_length() as we do for other fields in _change_text_types(). The disadvantage is that this will make checksetup to stop the upgrade process and the admin will have to fix each too long quip manually before he can run checksetup.pl again. The advantage is that no quips are lost.
b) remove this line entirely as we will convert the quips.quip column later anyway. The disadvantage is that we give the admin no chance to backup very long quips, but quips aren't important enough to stop the upgrade process, IMO. This is my preferred solution.
c) a third solution would be to leave this line as is, but in this case, someone upgrading from 3.0 or older directly to 4.4 would see two consecutive changes for the same column: TEXT -> MEDIUMTEXT -> varchar(512), which is a waste of time. This is why I don't consider it as a viable solution.


>+sub _shorten_long_quips {
>+    my $quips = $dbh->selectall_arrayref(q{
>+                SELECT quipid, quip
>+                  FROM quips
>+                 WHERE LENGTH(quip) > 512});

We want to limit the length to 512 *characters*, not bytes, so you have to use CHAR_LENGTH() instead of LENGTH(). Also, this SQL query is short enough to be written on a single line:

>+    my $quips = $dbh->selectall_arrayref(
>+                  'SELECT quipid, quip FROM quips WHERE CHAR_LENGTH(quip) > 512');


>+    my $query = $dbh->prepare(q(UPDATE quips
>+                                   SET quip = ?
>+                                 WHERE quipid = ?));

Be consistent and write q{} or q(), not both. Or simply use quotes as there is no quotes in the query, which is simpler. Also, the SQL statement is short enough to be written on a single line:

>+    my $query = $dbh->prepare('UPDATE quips SET quip = ? WHERE quipid = ?');


>+        $quip_str = substr($quip_str, 0, 512);

I suggest that we cut the string at 509 characters and append "..." to it so that a too long quip would now look like "foo bar...". This is more aesthetic. :)



>=== modified file 'quips.cgi'

>+    ThrowUserError("quip_too_long") if length($comment) > MAX_QUIP_LENGTH;

You should pass the length of the quip to the error message, so that the user knows if his quip is way too long or just a bit too long. Else it's hard to know.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+    The length of a quip cannot exceed [% constants.MAX_QUIP_LENGTH FILTER none %]
>+    characters.

I just realized that we give no information to the user about the length of the quip he just typed. The error message should start with:
  "Your quip is [% quip_length FILTER none %] characters long, but ..."

One way to prevent this problem is to limit the length of the text that the user can enter in the text field. Please add maxlength="512" to the text field in list/quips.html.tmpl.

Comment 33

[Koosha KM]

Attachment: patch - v1.4

This simple useless enhancement is turning into a headache.

Comment 34

[Frédéric Buclin]

Comment on attachment 648084 [details] [diff] [review]
patch - v1.4

You reverted the change in _change_text_types(), which means you followed suggestion c) from comment 32. But as I said, this is not a viable solution, because this is a waste of time for the DB being upgraded. Please follow suggestion b). Otherwise looks good.

Comment 35

[Koosha KM]

Attachment: patch - v1.4.1

Comment 36

[Frédéric Buclin]

Comment on attachment 648101 [details] [diff] [review]
patch - v1.4.1

>=== modified file 'Bugzilla/Install/DB.pm'

>+sub _shorten_long_quips {

>+    my $query = $dbh->prepare("UPDATE quips SET quip = ? WHERE quipid = ?");

Nit: we should only prepare the SQL query if there are quips to convert.


>+        print "Shortening quip $quipid...\n";

Nit: for consistency with other conversions, all IDs can be displayed on the same line.


This can easily be fixed on checkin and your patch is working fine on PostgreSQL. Thanks a lot for the patch! :) r=LpSolit

Comment 37

[Frédéric Buclin]

I tested it with Oracle too. Works like a charm! :)

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified quips.cgi
modified Bugzilla/Constants.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/global/user-error.html.tmpl
modified template/en/default/list/quips.html.tmpl
Committed revision 8348.

Comment 38

[Frédéric Buclin]

Added to relnotes for 4.4.