Most Bugzilla::BugUrl::* modules incorrectly validate the domain name

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Frédéric Buclin, Assigned: Simon Green)

Tracking

Bugzilla 4.4
Bug Flags:
approval +
approval4.4 +

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Description

5 years ago
Most see also modules have checks of the form $uri->authority =~ /^github.com$/i, but it's a regexp, and in regexp, a dot . means any character. So dots must be escaped as \.
(Assignee)

Comment 1

5 years ago
Created attachment 696915 [details] [diff] [review]
v1 patch

There are also other values in this extension that have the same issue.
Attachment #696915 - Flags: review?
(Reporter)

Comment 2

5 years ago
Comment on attachment 696915 [details] [diff] [review]
v1 patch

Your patch must also include files in Bugzilla/BugUrl/, not only those being in extensions/.
Attachment #696915 - Flags: review? → review-

Comment 3

5 years ago
(In reply to Frédéric Buclin from comment #0)
> Most see also modules have checks of the form $uri->authority =~
> /^github.com$/i, but it's a regexp, and in regexp, a dot . means any
> character. So dots must be escaped as \.

In cases that the regexp is surrounded between ^ and $ and the regexp is constant (can have only one value), it would be nicer to use 'eq' instead of regexp matching. For example:

$uri->authority =~ /^github\.com$/i

should be changed to 

lc($uri->authority) eq "github.com"

This could omit some overhead.
(Assignee)

Comment 4

5 years ago
Created attachment 697577 [details] [diff] [review]
v2 patch
Attachment #696915 - Attachment is obsolete: true
Attachment #697577 - Flags: review?(LpSolit)

Comment 5

5 years ago
(In reply to Hugo Seabrook from comment #4)
> Created attachment 697577 [details] [diff] [review]
> v2 patch

To enhance performance, please address my comment.
(Reporter)

Comment 6

5 years ago
Comment on attachment 697577 [details] [diff] [review]
v2 patch

I agree with koosha's comment 3. It's crazy to write /^foo$/ when eq 'foo' can do the same job in a more readable and more efficient way.
Attachment #697577 - Flags: review?(LpSolit) → review-
(Assignee)

Updated

5 years ago
Assignee: create-and-change → hugo.seabrook
(Assignee)

Comment 7

5 years ago
(In reply to Frédéric Buclin from comment #6)
> I agree with koosha's comment 3. It's crazy to write /^foo$/ when eq 'foo'
> can do the same job in a more readable and more efficient way.

I agree too :) Unfortunately I don't receive comments on bugs that aren't assigned to me, so I never read the comment. I've since gained the editbugs privilege so I can assign bugs to myself.

I'm about to submit a new patch, but there are cases like

 26     return ($uri->authority =~ /launchpad\.net$/

which I haven't changed to

 26     return (substr($uri->authority, -13) eq 'launchpad.net'

If you would like me to change these, feel free to decline the review and ask me to change them.

Regards,
Hugo
(Assignee)

Comment 8

5 years ago
Created attachment 708519 [details] [diff] [review]
v3 patch
Attachment #697577 - Attachment is obsolete: true
Attachment #708519 - Flags: review?(LpSolit)
(Reporter)

Comment 9

5 years ago
Comment on attachment 708519 [details] [diff] [review]
v3 patch

>-    return ($uri->authority =~ /^bugs.debian.org$/i
>+    return ($uri->authority eq 'bugs.debian.org'

Your patch looks good, but you regress URL which are (partially) uppercase. So you should write:

  lc($uri->authority) eq 'foo.com'

to reproduce the //i behavior of the regexp. This applies to all parts of your patch.
Attachment #708519 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 714771 [details] [diff] [review]
v4 patch

The use of mixed case domains names seemed inconsistent (e.g. launchpad.net doesn't have it), so the thought it wasn't deliberate that soome allowed it. However, as requested I have modified my patch.

Regards,
Hug
Attachment #708519 - Attachment is obsolete: true
Attachment #714771 - Flags: review?(LpSolit)

Comment 11

5 years ago
Comment on attachment 714771 [details] [diff] [review]
v4 patch

>=== modified file 'Bugzilla/BugUrl/Launchpad.pm'
>--- Bugzilla/BugUrl/Launchpad.pm	2012-12-01 01:18:21 +0000
>+++ Bugzilla/BugUrl/Launchpad.pm	2013-01-03 19:39:51 +0000
>@@ -23,7 +23,7 @@
>     #   https://bugs.launchpad.net/ubuntu/+bug/1234
>     #   https://launchpad.net/bugs/1234
>     # All variations end with either "/bugs/1234" or "/+bug/1234"
>-    return ($uri->authority =~ /launchpad.net$/
>+    return ($uri->authority =~ /launchpad\.net$/
>             and $uri->path =~ m|bugs?/\d+$|) ? 1 : 0;
> }
> 
>
>=== modified file 'extensions/MoreBugUrl/lib/PHP.pm'
>--- extensions/MoreBugUrl/lib/PHP.pm	2013-01-01 22:59:13 +0000
>+++ extensions/MoreBugUrl/lib/PHP.pm	2013-02-16 10:30:39 +0000
>@@ -20,8 +20,8 @@
> 
>     # PHP Bug URLs have only one form:
>     #   https://bugs.php.net/bug.php?id=1234
>-    return ($uri->authority =~ /^bugs.php.net$/i
>-            and $uri->path =~ m|/bug.php$|
>+    return (lc $uri->authority eq 'bugs.php.net'
>+            and $uri->path =~ m|/bug\.php$|
>             and $uri->query_param('id') =~ /^\d+$/) ? 1 : 0;
> }
> 
>
>=== modified file 'extensions/MoreBugUrl/lib/RT.pm'
>--- extensions/MoreBugUrl/lib/RT.pm	2013-01-01 22:59:13 +0000
>+++ extensions/MoreBugUrl/lib/RT.pm	2013-01-31 10:58:53 +0000
>@@ -21,7 +21,7 @@
>     # RT URLs can look like various things:
>     #   http://example.com/rt/Ticket/Display.html?id=1234
>     #   https://example.com/Public/Bug/Display.html?id=1234
>-    return ($uri->path =~ m|/Display.html$|
>+    return ($uri->path =~ m|/Display\.html$|
>             and $uri->query_param('id') =~ /^\d+$/) ? 1 : 0;
> }
> 
>

Add the 'i' modifier when matching regular expressions to match case-insensitively.
(Reporter)

Comment 12

5 years ago
(In reply to Koosha Khajeh Moogahi [:koosha] from comment #11)
> Add the 'i' modifier when matching regular expressions to match
> case-insensitively.

No. Those are case-sensitive. Only the domain name is case-insensitive.
(Reporter)

Comment 13

5 years ago
Comment on attachment 714771 [details] [diff] [review]
v4 patch

Looks good. Thanks! r=LpSolit
Attachment #714771 - Flags: review?(LpSolit) → review+
(Reporter)

Updated

5 years ago
Flags: approval4.4+
Flags: approval+
(Reporter)

Comment 14

5 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/BugUrl/Debian.pm
modified Bugzilla/BugUrl/GitHub.pm
modified Bugzilla/BugUrl/Google.pm
modified Bugzilla/BugUrl/Launchpad.pm
modified Bugzilla/BugUrl/SourceForge.pm
modified extensions/MoreBugUrl/lib/GetSatisfaction.pm
modified extensions/MoreBugUrl/lib/PHP.pm
modified extensions/MoreBugUrl/lib/RT.pm
Committed revision 8578.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/BugUrl/Debian.pm
modified Bugzilla/BugUrl/GitHub.pm
modified Bugzilla/BugUrl/Google.pm
modified Bugzilla/BugUrl/Launchpad.pm
modified Bugzilla/BugUrl/SourceForge.pm
modified extensions/MoreBugUrl/lib/GetSatisfaction.pm
modified extensions/MoreBugUrl/lib/PHP.pm
modified extensions/MoreBugUrl/lib/RT.pm
Committed revision 8519.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.