launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/python-oops-tools/bug-884265 into lp:python-oops-tools.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #884265 in python-oops-tools: "req_vars header values may cause crash in dboopsloader"
  https://bugs.launchpad.net/python-oops-tools/+bug/884265

For more details, see:
https://code.launchpad.net/~lifeless/python-oops-tools/bug-884265/+merge/81932

Fixup rye's req_vars branch and adjust per wgrants suggestion.
-- 
https://code.launchpad.net/~lifeless/python-oops-tools/bug-884265/+merge/81932
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/python-oops-tools/bug-884265 into lp:python-oops-tools.

=== modified file 'src/oopstools/NEWS.txt'
--- src/oopstools/NEWS.txt	2011-11-02 23:19:28 +0000
+++ src/oopstools/NEWS.txt	2011-11-11 03:32:27 +0000
@@ -12,7 +12,8 @@
   OOPS-prefix (if they had one). (Robert Collins, #884571)
 
 * OOPS reports that don't meet the normal rules for req_vars are handled
-  a bit better (Robert Collins, William Grant, #885416)
+  a bit better.
+  (Robert Collins, William Grant, Roman Yepishev, #885416, #884265)
 
 0.6.1
 =====

=== modified file 'src/oopstools/oops/models.py'
--- src/oopstools/oops/models.py	2011-11-02 20:25:00 +0000
+++ src/oopstools/oops/models.py	2011-11-11 03:32:27 +0000
@@ -315,6 +315,14 @@
                 value = ''
             yield key, value
     for key, value in ensure_tuple(req_vars):
+        if isinstance(value, str):
+            try:
+                # We can get anything in HTTP headers
+                # Trying to squeeze the bytes into ASCII plane
+                value.decode('ascii')
+            except UnicodeDecodeError:
+                # If that fails, percent-quote them
+                value = urllib.quote(value)
         if key == 'REQUEST_METHOD':
             # Truncate the REQUEST_METHOD if it's longer than 10 characters
             # since a str longer than that is not allowed in the DB.

=== modified file 'src/oopstools/oops/test/test_dboopsloader.py'
--- src/oopstools/oops/test/test_dboopsloader.py	2011-11-02 20:25:00 +0000
+++ src/oopstools/oops/test/test_dboopsloader.py	2011-11-11 03:32:27 +0000
@@ -155,15 +155,22 @@
         oops = parsed_oops_to_model_oops(report, 'bug_880641')
         self.assertEqual('', oops.pageid)
 
-    def test_broken_url_handling(self):
-        # This URL is not a valid URL - URL's are a subset of ASCII.
+    def test_broken_header_handling(self):
         broken_url = '/somep\xe1th'
-        report = { 'url': broken_url, 'id': 'testbrokenurl'}
+        user_agent = '\xf8\x07\x9e'
+        report = { 'url': broken_url,
+                   'id': 'testbrokenheader',
+                   'req_vars': [
+                       [ 'HTTP_USER_AGENT', user_agent ]
+                   ]
+        }
         # We handle such URL's by url quoting them, failing to do so being a
         # (not uncommon) producer mistake.
         expected_url = urllib.quote(broken_url)
+        expected_user_agent = urllib.quote(user_agent)
         oops = parsed_oops_to_model_oops(report, 'test_broken_url_handling')
         self.assertEqual(expected_url, oops.url)
+        self.assertEqual(expected_user_agent, oops.user_agent)
 
     def test_broken_reqvars_bug_885416(self):
         # If the tuples in req_vars have only one element the oops still needs