If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Incorrect regexp used to filter bug IDs in Bugzilla::WebService::BugUserLastVisit

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
WebService
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Frédéric Buclin, Assigned: dylan)

Tracking

({perf})

5.0.1
Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +
approval5.0 +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
In Bugzilla/WebService/BugUserLastVisit.pm, in get() and update(), the following code is used to cache bugs:

  $user->visible_bugs([grep /^[0-9]$/, @$ids])

/^[0-9]$/ will only catch bugs having only one digit. This regexp should contain [0-9]+ or \d+.
(Reporter)

Comment 1

2 years ago
Created attachment 8697857 [details] [diff] [review]
patch, v1
Assignee: webservice → LpSolit
Status: NEW → ASSIGNED
Attachment #8697857 - Flags: review?(dkl)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8697857 [details] [diff] [review]
patch, v1

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

Nice catch, but please avoid \d in new code unless you really mean all digits of all alphabets (or specify the behavior with the relevant pragma)
Attachment #8697857 - Flags: review?(dkl) → review-
(Reporter)

Comment 3

2 years ago
(In reply to Dylan William Hardison [:dylan] from comment #2)
> Nice catch, but please avoid \d in new code unless you really mean all
> digits of all alphabets (or specify the behavior with the relevant pragma)

\d+ is what we use everywhere in the Bugzilla code, including detaint_natural() which we use everywhere for our security checks. \d+ is safe here because if non-ASCII digits are passed and do not match any bug ID, they will simply be ignored. So IMO, this is not a valid reason to deny review.

And Bugzilla 6.0 will require Perl 5.14 which will allow us to use /a with regexps to force an ASCII comparison, so this distinction won't be relevant either.
(Assignee)

Comment 4

2 years ago
Created attachment 8698043 [details] [diff] [review]
1232180_1.patch

Updated patch that also fixes the REST version of BugUserLastVisited.
Attachment #8697857 - Attachment is obsolete: true
Attachment #8698043 - Flags: review?(dkl)
(Assignee)

Updated

2 years ago
Blocks: 1232324
Comment on attachment 8698043 [details] [diff] [review]
1232180_1.patch

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

r=dkl
Attachment #8698043 - Flags: review?(dkl) → review+
Need a patch for 5.0 as well that does not include Bugzilla/API/* 

dkl
(Reporter)

Updated

2 years ago
Assignee: LpSolit → dylan
(Assignee)

Comment 7

2 years ago
Created attachment 8698528 [details] [diff] [review]
5.0.patch
Attachment #8698528 - Flags: review?(dkl)
Comment on attachment 8698528 [details] [diff] [review]
5.0.patch

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

r=dkl
Attachment #8698528 - Flags: review?(dkl) → review+

Updated

2 years ago
Flags: approval5.0+
Flags: approval+
(Assignee)

Comment 9

2 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   e247772..c7affd0  5.0 -> 5.0
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   dee37d0..76ecb18  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.