Merge lp:~rye/python-oops-tools/quote-req-vars into lp:python-oops-tools

[Roman Yepishev]

req_vars may contain weird content so we will want to sanitize it a bit before putting some values in the database.

[Aaron Bentley (abentley)]

I don't understand why you're changing the input you get before storing it. Isn't it better to store the true values you received, and then sanitize your output if necessary?

[Robert Collins (lifeless)]

The problem is data that doesn't fit the schema. The schema is varchar rather than bytea, so some form of coercion is needed. The quoting is at least lossless.... We might want to change the schema as well, but I think that can wait for us to at least get the U1 oops-tools stack live with the new code, which should be soon.

Wgrant also had a fix in this area for LP itself, I'll merge them all together and either land or propose for review depending how much fix up is needed.

review: Approve

[Robert Collins (lifeless)]

Follow on MP is at https://code.launchpad.net/~lifeless/python-oops-tools/bug-884265, if you have any further questions (or suggestions [short of schema changes :)] on how this might be done now).

If we think a schema change should be done - and perhaps it should be - I think we should do that in 2-3 releases - once all the deploys in Canonical are successfully running unmodified releases.

=== 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