unsafe regexp used in global/userselect.html.tmpl

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
User Interface
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Frédéric Buclin, Assigned: Jesse Clark)

Tracking

2.20.5
Bugzilla 3.0
Bug Flags:
approval +
blocking3.2 +
approval3.0 +

Details

Attachments

(2 attachments, 3 obsolete attachments)

v4
1.18 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
1.10 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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).
Flags: blocking3.2+
(Assignee)

Comment 1

10 years ago
Created attachment 317110 [details] [diff] [review]
v1

This regex will match both user.login and user.identity
Assignee: ui → jjclark1982
Status: NEW → ASSIGNED
Attachment #317110 - Flags: review?(LpSolit)
(Reporter)

Comment 2

10 years ago
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?
Attachment #317110 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 3

10 years ago
Created attachment 317139 [details] [diff] [review]
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.
Attachment #317110 - Attachment is obsolete: true
Attachment #317139 - Flags: review?(LpSolit)
(Reporter)

Comment 4

10 years ago
(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

10 years ago
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!
(Assignee)

Comment 6

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

Comment 7

10 years ago
Created attachment 318099 [details] [diff] [review]
v3

This is the probably most elegant way to make the regex match work correctly, but I will still explore the direct comparison option.
Attachment #317139 - Attachment is obsolete: true
Attachment #318099 - Flags: review?(LpSolit)
Attachment #317139 - Flags: review?(LpSolit)
(Assignee)

Comment 8

10 years ago
Created attachment 318101 [details] [diff] [review]
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?
Attachment #318101 - Flags: review?(LpSolit)
(Reporter)

Comment 9

10 years ago
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
Attachment #318101 - Flags: review?(LpSolit) → review+
(Reporter)

Updated

10 years ago
Attachment #318099 - Attachment is obsolete: true
Attachment #318099 - Flags: review?(LpSolit)
(Reporter)

Updated

10 years ago
Flags: approval3.0+
Flags: approval+
(Reporter)

Comment 10

10 years ago
Oops, the patch doesn't apply cleanly on 3.0.4. Backport needed.
Flags: approval3.0+
(Assignee)

Comment 11

10 years ago
Created attachment 318205 [details] [diff] [review]
v5 - for 3.0.4

Here is v4 backported to 3.0.4
Attachment #318205 - Flags: review?(LpSolit)
(Reporter)

Comment 12

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

Thanks! r=LpSolit
Attachment #318205 - Flags: review?(LpSolit) → review+
(Reporter)

Comment 13

10 years ago
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.
Flags: approval3.0+
(Reporter)

Comment 14

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Updated

9 years ago
Duplicate of this bug: 451120
You need to log in before you can comment on or make changes to this bug.