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

RESOLVED FIXED

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: f1sh, Assigned: mail)

Tracking

4.4.12

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

2 years ago
Assignee: administration → mail
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8797116 [details] [diff] [review]
bug1290588-v1.patch
Attachment #8797116 - Flags: review?(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.
(Assignee)

Comment 3

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

Comment 5

2 years ago
Created attachment 8798268 [details] [diff] [review]
bug1290588-v2.patch

Now gets column length from the database rather than a constant :)
Attachment #8797116 - Attachment is obsolete: true
Attachment #8797116 - Flags: review?(gerv)
Attachment #8798268 - Flags: review?(gerv)
Comment on attachment 8798268 [details] [diff] [review]
bug1290588-v2.patch

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

r=gerv.

Gerv
Attachment #8798268 - Flags: review?(gerv) → review+
(Assignee)

Comment 7

2 years ago
To github.com:bugzilla/bugzilla.git
   63f79c6..8171193  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.