1290588 - Group Administration via interface should error if regular expression size is greater than column length

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[u430950]

While it's not the best practice to have a regular expression larger than the limits of TINYTEXT, really nasty things can happen if the regular expression ends up cut off in the end. I propose a error handling check to prevent such a change from going through.

Comment 1

[Simon Green]

Attachment: bug1290588-v1.patch

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 8797116 [details] [diff] [review]
bug1290588-v1.patch

Review of attachment 8797116 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Group.pm
@@ +482,5 @@
>  sub _check_user_regexp {
>      my ($invocant, $regex) = @_;
>      $regex = trim($regex) || '';
> +    ThrowUserError( "group_regexp_too_long", { text => $regex } )
> +      if length($regex) > MAX_GROUP_USER_REGEXP_LENGTH;

Given that what you are really comparing with here is length(TINYTEXT), is there a way to express that directly rather than creating a new constant which might get out of sync? Then you just make a generic "field too long" error and use that.

Comment 3

[Simon Green]

(In reply to Gervase Markham [:gerv] from comment #2)
> Given that what you are really comparing with here is length(TINYTEXT), is
> there a way to express that directly rather than creating a new constant
> which might get out of sync? Then you just make a generic "field too long"
> error and use that.

There is a generic 'freetext_too_long' error, but since the group's user regexp is not a field (as defined in the fielddefs table, we cannot use this value.

Comment 4

[Gervase Markham [:gerv]]

(In reply to mail from comment #3)
> (In reply to Gervase Markham [:gerv] from comment #2)
> > Given that what you are really comparing with here is length(TINYTEXT), is
> > there a way to express that directly rather than creating a new constant
> > which might get out of sync? Then you just make a generic "field too long"
> > error and use that.
> 
> There is a generic 'freetext_too_long' error, but since the group's user
> regexp is not a field (as defined in the fielddefs table, we cannot use this
> value.

Fair enough. But my other points stand, including comparing directly with len(TINYTEXT).

Gerv

Comment 5

[Simon Green]

Attachment: bug1290588-v2.patch

Now gets column length from the database rather than a constant :)

Comment 6

[Gervase Markham [:gerv]]

Comment on attachment 8798268 [details] [diff] [review]
bug1290588-v2.patch

Review of attachment 8798268 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv.

Gerv

Comment 7

[Simon Green]

To github.com:bugzilla/bugzilla.git
   63f79c6..8171193  master -> master