666594 - emailregexp should not be allowed to be an empty string

Status: NEW

(Bugzilla :: Bugzilla-General, defect)

Description

[Kent Rogers [:mrbball]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.6.17-1.fc14 Firefox/3.6.17
Build Identifier: 4.1.2+

If the emailregexp admin parameter is set to an empty string, the
code in sendbugmail.pl that attempts to validate the changer address
($changer) does not recognize that fact and attempts to match an empty
pattern.  This will cause the match to use the last successfully matched
regular expression (i.e. "^(\d+)$" - from the  code that validates the
bug number) instead, which is obviously not what is intended.

sendbugmail.pl will never work if emailregexp is set to an empty
string, and this can go undetected for an extended period of time.

If the emailregexp is empty, sendbugmail.pl should simply skip the
attempted validation (at least against emailregexp) of $changer.

Reproducible: Always

Comment 1

[Max Kanat-Alexander]

Is it even valid for emailregexp to be an empty string??

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> Is it even valid for emailregexp to be an empty string??

The code and the documentation say nothing about this parameter being empty. IMO, it doesn't really make sense to leave it empty, but the checker doesn't prevent this case.

Comment 3

[Frédéric Buclin]

And this is in no way a major issue as it doesn't make sense to leave this parameter empty. We could enforce the checker to forbird the empty string.

Comment 4

[Kent Rogers [:mrbball]]

It made sense to me, as I have no restrictions on legal email addresses,
and until the empty string is forbidden, this is a big problem for
installations that set it to an empty string and use sendbugmail.pl.

I don't have a problem with forbidding the empty string, but I think
sendbugmail.pl should also be updated to make sure an empty string isn't
matched.

Comment 5

[Frédéric Buclin]

This is not a big problem. Just set the parameter to .* (no check) and that's it. That's not the job of sendbugmail.pl to do this check.

Comment 6

[Kent Rogers [:mrbball]]

Yeah, that's a great workaround ... if I'd known that was the problem!
Unfortunately, it took me almost 3 months to figure that out.

Comment 7

[Kent Rogers [:mrbball]]

Attachment: Patch for sendbugmail.pl to detect empty "emailregexp"

This allows sendbugmail.pl to function properly if someone inadvertently
sets emailregexp to an empty string (which is allowed by Bugzilla).

Comment 8

[Max Kanat-Alexander]

Yeah, we should update the params checkers to make sure that that param isn't an empty string.

We wouldn't handle this separately, but we might throw a warning about it in checksetup.pl during an upgrade for people who emptied the string.

Comment 9

[Marc Schumann [:Wurblzap]]

Comment on attachment 542448 [details] [diff] [review]
Patch for sendbugmail.pl to detect empty "emailregexp"

r- by comment 3 and comment 8 (matching my own opinion). Let's define a check_nonempty checker in Bugzilla/Config/Common.pm, make Bugzilla/Config.pm's sub SetParam to be able to do checker chaining, and have the param in Bugzilla/Config/Auth.pm use both check_regexp and check_nonempty checkers.

Agreed, only checksetup.pl would catch it if somebody has already set it to an empty string. But in conjunction with comment 3, I don't think that's too much of a problem.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

dylan, could you comment and let us know if this bug is still valid in today's codebase?  If so, could you find a mentor for this?

Comment 11

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.