launchpad-reviewers team mailing list archive

[Roman Yepishev <roman.yepishev@xxxxxxxxx>]

Roman Yepishev has proposed merging lp:~rye/python-oops-tools/quote-req-vars 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/~rye/python-oops-tools/quote-req-vars/+merge/81714

req_vars may contain weird content so we will want to sanitize it a bit before putting some values in the database.
-- 
https://code.launchpad.net/~rye/python-oops-tools/quote-req-vars/+merge/81714
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rye/python-oops-tools/quote-req-vars into lp:python-oops-tools.

=== 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-09 11:09:40 +0000
@@ -305,6 +305,7 @@
         is_local_referrer = False)
     # Grab data needed by the Oops database model from the req_vars.
     req_vars = oops.get('req_vars') or ()
+<<<<<<< TREE
     # Some badly written OOPSes have single item tuples. (field.blob was seen).
     def ensure_tuple(iterable):
         for item in iterable:
@@ -315,6 +316,17 @@
                 value = ''
             yield key, value
     for key, value in ensure_tuple(req_vars):
+=======
+    for key, value in req_vars:
+        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)
+
+>>>>>>> MERGE-SOURCE
         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-09 11:09:40 +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