global/userselect.html.tmpl uses an unsafe regexp when displaying user accounts: value.match("\\b$tmpuser.login\\b") When usemenuforusers is enabled, foo+bmo@bar.com 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 evil-foo@bar.com could be caught in place of foo@bar.com, 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).

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

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?

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.

(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 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!

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.

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

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 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

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

Here is v4 backported to 3.0.4

Comment on attachment 318205 [details] [diff] [review] v5 - for 3.0.4 Thanks! r=LpSolit

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.

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