Closed Bug 1232180 Opened 4 years ago Closed 4 years ago

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

Categories

(Bugzilla :: WebService, defect)

5.0.1
defect
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: LpSolit, Assigned: dylan)

References

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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+.
Attached patch patch, v1 (obsolete) — Splinter Review
Assignee: webservice → LpSolit
Status: NEW → ASSIGNED
Attachment #8697857 - Flags: review?(dkl)
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-
(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.
Attached patch 1232180_1.patchSplinter Review
Updated patch that also fixes the REST version of BugUserLastVisited.
Attachment #8697857 - Attachment is obsolete: true
Attachment #8698043 - Flags: review?(dkl)
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
Assignee: LpSolit → dylan
Attached patch 5.0.patchSplinter Review
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+
Flags: approval5.0+
Flags: approval+
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
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.