430307 - unsafe regexp used in global/userselect.html.tmpl

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Frédéric Buclin]

global/userselect.html.tmpl uses an unsafe regexp when displaying user accounts:

value.match("\\b$tmpuser.login\\b")

When usemenuforusers is enabled, [e-mail] is never displayed when a page is loaded despite being the currently "selected" value due to the "+" character. Maybe some other characters also trigger this problem, I don't know. This makes me think that TT is confused, also because we pass @ unescaped (i.e. it should be \@ I think to not be seen as an array).

I suppose we should enclose the login name in \Q \E, assuming TT supports this. Also, as discussed with justdave on IRC, we suspect we may abuse this regexp as [e-mail] could be caught in place of [e-mail], which may be a security problem as the wrong user would be selected (think e.g. about bookmarkable templates with a predefined list of CC users).

Marking as blocking 3.2, but it would be nice to have it for 3.0.5 too (too late for 3.0.4 I suppose).

Comment 1

[Jesse Clark]

Attachment: v1

This regex will match both user.login and user.identity

Comment 2

[Frédéric Buclin]

Comment on attachment 317110 [details] [diff] [review]
v1

>-    [% IF tmpuser.visible OR value.match("\\b$tmpuser.login\\b") %]
>+    [% IF tmpuser.visible OR value.match("(?:<|^)\Q${tmpuser.login}\E(?:>|$)") %]
>       <option value="[% tmpuser.login FILTER html %]"
>         [% " selected=\"selected\"" IF value.match("\\b$tmpuser.login\\b") %]

This last line must be fixed too. Did you test your patch to make sure it fixes the problem?

Comment 3

[Jesse Clark]

Attachment: v2

Oops, I was getting false positives because my test user with a "+" address was also my first user alphabetically.

\Q and \E do not work in TT, so we need a regex filter for when matching is necessary. An alternative would be to only do direct comparisons instead of matches, which would also improve performance.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> An alternative would be to only do direct comparisons instead of
> matches, which would also improve performance.

I would prefer that. Maybe could we use the "contains" method of arrays, as defined in Bugzilla::Template, line 376? So the first step would be to split the value string on commas and spaces, then use value_array.contains($tmpuser.login). This would be safer.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 317139 [details] [diff] [review]
v2

  You know, I think you might instead be able to just make the filter look like:

  $var = qr/\Q$var\E/;

  Though I'm not totally certain that will continue to work in TT. But it might!

Comment 6

[Jesse Clark]

Using qr// appears to work correctly, but apparently userselects are being treated differently in process_bug than in show_bug, so even when the match works correctly, the same bug can have two different behaviors.

I think this will have to be redone by ensuring that the select value is eligible for direct comparison everywhere userselect gets called.

Comment 7

[Jesse Clark]

Attachment: v3

This is the probably most elegant way to make the regex match work correctly, but I will still explore the direct comparison option.

Comment 8

[Jesse Clark]

Attachment: v4

Ok, I think this is the most efficient way to check all visible users against the passed value list. Any comments about my template code style?

Comment 9

[Frédéric Buclin]

Comment on attachment 318101 [details] [diff] [review]
v4

Works correctly and I like this fix. I wonder if we shouldn't pass an arrayref to value instead of a concatenated string. But this could be done in a separate bug as an enhancement. This one is to fix a real bug. r=LpSolit

Comment 10

[Frédéric Buclin]

Oops, the patch doesn't apply cleanly on 3.0.4. Backport needed.

Comment 11

[Jesse Clark]

Attachment: v5 - for 3.0.4

Here is v4 backported to 3.0.4

Comment 12

[Frédéric Buclin]

Comment on attachment 318205 [details] [diff] [review]
v5 - for 3.0.4

Thanks! r=LpSolit

Comment 13

[Frédéric Buclin]

I think it's fine to take it for 3.0.4. None of our Selenium scripts tests usemenuforusers, though, so if this regresses anything, we won't catch it.

Comment 14

[Frédéric Buclin]

tip:

Checking in template/en/default/global/userselect.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/userselect.html.tmpl,v  <--  userselect.html.tmpl
new revision: 1.9; previous revision: 1.8
done

3.0.3:

Checking in template/en/default/global/userselect.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/userselect.html.tmpl,v  <--  userselect.html.tmpl
new revision: 1.5.8.1; previous revision: 1.5
done