launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/sanity-check-bugwatch-bug-id into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #634907 in Launchpad itself: "Launchpad allows invalid Bugzilla remotebug IDs"
  https://bugs.launchpad.net/launchpad/+bug/634907

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/sanity-check-bugwatch-bug-id/+merge/128625

Change the parsing methods that check if a remote URL for a bugwatch is valid. From what I can see, before I started this branch, only debbugs actually gave an error if the bug id supplied is not all digits, and the Trac and RoundUp parsers did something *awesome* and truncated the URL at the first non-digit character.

I think I've sorted it out so all parsers (except the e-mail address parser for obvious reasons) will croak when the remote bug id is not all digits. The LoC count can come off my something like -1000 count, but I have removed the pylint garbage and a comment that didn't add value in the slightest.
-- 
https://code.launchpad.net/~stevenk/launchpad/sanity-check-bugwatch-bug-id/+merge/128625
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/sanity-check-bugwatch-bug-id into lp:launchpad.

=== modified file 'lib/lp/bugs/interfaces/bugtracker.py'
--- lib/lp/bugs/interfaces/bugtracker.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/interfaces/bugtracker.py	2012-10-09 04:56:19 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0211,E0213
-
 """Bug tracker interfaces."""
 
 __metaclass__ = type

=== modified file 'lib/lp/bugs/interfaces/bugwatch.py'
--- lib/lp/bugs/interfaces/bugwatch.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/interfaces/bugwatch.py	2012-10-09 04:56:19 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0211,E0213
-
 """Bug watch interfaces."""
 
 __metaclass__ = type

=== modified file 'lib/lp/bugs/model/bugwatch.py'
--- lib/lp/bugs/model/bugwatch.py	2012-06-29 08:40:05 +0000
+++ lib/lp/bugs/model/bugwatch.py	2012-10-09 04:56:19 +0000
@@ -1,8 +1,6 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=E0611,W0212
-
 __metaclass__ = type
 __all__ = [
     'BugWatch',
@@ -20,7 +18,6 @@
 from lazr.lifecycle.snapshot import Snapshot
 from lazr.uri import find_uris_in_text
 from pytz import utc
-# SQL imports
 from sqlobject import (
     ForeignKey,
     SQLObjectNotFound,
@@ -495,6 +492,8 @@
             remote_bug = query['issue']
         else:
             return None
+        if remote_bug is None or not remote_bug.isdigit():
+            return None
         base_path = path[:-len(bug_page)]
         base_url = urlunsplit((scheme, host, base_path, '', ''))
         return base_url, remote_bug
@@ -504,9 +503,8 @@
         bug_page = 'view.php'
         if not path.endswith(bug_page):
             return None
-        if query.get('id'):
-            remote_bug = query['id']
-        else:
+        remote_bug = query.get('id')
+        if remote_bug is None or not remote_bug.isdigit():
             return None
         base_path = path[:-len(bug_page)]
         base_url = urlunsplit((scheme, host, base_path, '', ''))
@@ -539,7 +537,7 @@
 
     def parseRoundupURL(self, scheme, host, path, query):
         """Extract the RoundUp base URL and bug ID."""
-        match = re.match(r'(.*/)issue(\d+)', path)
+        match = re.match(r'(.*/)issue(\d+)$', path)
         if not match:
             return None
         base_path = match.group(1)
@@ -569,13 +567,15 @@
 
         base_path = match.group(1)
         remote_bug = query['id']
+        if remote_bug is None or not remote_bug.isdigit():
+            return None
 
         base_url = urlunsplit((scheme, host, base_path, '', ''))
         return base_url, remote_bug
 
     def parseTracURL(self, scheme, host, path, query):
         """Extract the Trac base URL and bug ID."""
-        match = re.match(r'(.*/)ticket/(\d+)', path)
+        match = re.match(r'(.*/)ticket/(\d+)$', path)
         if not match:
             return None
         base_path = match.group(1)
@@ -605,6 +605,8 @@
             return None
 
         remote_bug = query['aid']
+        if remote_bug is None or not remote_bug.isdigit():
+            return None
 
         # There's only one global SF instance registered in Launchpad,
         # so we return that if the hostnames match.
@@ -638,6 +640,8 @@
         # a value, so we simply use the first and only key we come
         # across as a best-effort guess.
         remote_bug = query.popitem()[0]
+        if remote_bug is None or not remote_bug.isdigit():
+            return None
 
         if host in savannah_hosts:
             return savannah_tracker.baseurl, remote_bug
@@ -669,7 +673,7 @@
         if path != '/bug.php' or len(query) != 1:
             return None
         remote_bug = query.get('id')
-        if remote_bug is None:
+        if remote_bug is None or not remote_bug.isdigit():
             return None
         base_url = urlunsplit((scheme, host, '/', '', ''))
         return base_url, remote_bug
@@ -687,7 +691,7 @@
             return None
 
         remote_bug = query.get('id')
-        if remote_bug is None:
+        if remote_bug is None or not remote_bug.isdigit():
             return None
 
         tracker_path = path_match.groupdict()['base_path']
@@ -708,9 +712,8 @@
             if not bugtracker_data:
                 continue
             base_url, remote_bug = bugtracker_data
-            bugtrackerset = getUtility(IBugTrackerSet)
             # Check whether we have a registered bug tracker already.
-            bugtracker = bugtrackerset.queryByBaseURL(base_url)
+            bugtracker = getUtility(IBugTrackerSet).queryByBaseURL(base_url)
 
             if bugtracker is not None:
                 return bugtracker, remote_bug

=== modified file 'lib/lp/bugs/tests/test_bugwatch.py'
--- lib/lp/bugs/tests/test_bugwatch.py	2012-08-08 07:23:58 +0000
+++ lib/lp/bugs/tests/test_bugwatch.py	2012-10-09 04:56:19 +0000
@@ -9,6 +9,7 @@
     datetime,
     timedelta,
     )
+import re
 import unittest
 from urlparse import urlunsplit
 
@@ -122,6 +123,12 @@
             self.fail(
                 "NoBugTrackerFound wasn't raised by extractBugTrackerAndBug")
 
+    def test_invalid_bug_number(self):
+        invalid_url = re.sub(r'(\d)\d', r'\1E', self.bug_url, count=1)
+        self.assertRaises(
+            UnrecognizedBugTrackerURL,
+            self.bugwatch_set.extractBugTrackerAndBug, invalid_url)
+
 
 class MantisExtractBugTrackerAndBugTest(
     ExtractBugTrackerAndBugTestBase, unittest.TestCase):
@@ -339,6 +346,10 @@
             self.bugwatch_set.extractBugTrackerAndBug,
             url='this\.is@@a.bad.email.address')
 
+    def test_invalid_bug_number(self):
+        # Test does not make sense for email addresses.
+        pass
+
 
 class PHPProjectBugTrackerExtractBugTrackerAndBugTest(
     ExtractBugTrackerAndBugTestBase, unittest.TestCase):