Merge lp:~benji/launchpad/bug-719637 into lp:launchpad

[Benji York]

[r=abentley][bug=719637] fix a variant of 719637 by not generating OOPSes on otherwise ignorable exceptions if there is no referer

Bug 719637 was mostly fixed along with bug 730393. However, a new incarnation of the bug was introduced at that time that caused NotFound (and similar errors) to OOPS if no referer is set. This happens in production (e.g., OOPS-1970CF506). This branch fixes this.

The operative change is the "if" statement added to lib/canonical/launchpad/webapp/errorlog.py.

The new tests exercise the different ways the if can be traversed.

The new tests can be run with

    bin/test -c -m canonical.launchpad.webapp.tests -t Test404Oops

The make lint report is clean.

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py	2011-05-20 04:48:41 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py	2011-06-07 21:24:40 +0000
@@ -377,7 +377,12 @@
             return True
         if strtype in self._ignored_exceptions_for_offsite_referer:
             if request is not None:
-                referer = request.get('HTTP_REFERER', '')
+                referer = request.get('HTTP_REFERER')
+                # If there is no referrer then we can't tell if this exception
+                # should be ignored or not, so we'll be conservative and
+                # ignore it.
+                if referer is None:
+                    return True
                 referer_parts = urlparse.urlparse(referer)
                 root_parts = urlparse.urlparse(
                     allvhosts.configs['mainsite'].rooturl)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-05-19 21:01:54 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py	2011-06-07 21:24:40 +0000
@@ -967,5 +967,29 @@
         self.assertIs(None, self.error_utility.getLastOopsReport())
 
 
+class Test404Oops(testtools.TestCase):
+
+    def test_offsite_404_ignored(self):
+        # A request originating from another site that generates a NotFound
+        # (404) is ignored (i.e., no OOPS is logged).
+        utility = ErrorReportingUtility()
+        request = dict(HTTP_REFERER='example.com')
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+    def test_onsite_404_not_ignored(self):
+        # A request originating from a local site that generates a NotFound
+        # (404) produces an OOPS.
+        utility = ErrorReportingUtility()
+        request = dict(HTTP_REFERER='canonical.com')
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+    def test_404_without_referer_is_ignored(self):
+        # If a 404 is generated and there is no HTTP referer, we don't produce
+        # an OOPS.
+        utility = ErrorReportingUtility()
+        request = dict()
+        self.assertTrue(utility._isIgnoredException('NotFound', request))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)