Repository: falconry/falcon

Issue:
Falcon crashes on decoding query string
When Falcon receives a request with query string that cannot be percent decoded (e.g. `x=%`), it crashes resulting in a rather unexpected 502 response.

	Traceback (most recent call last):
	  File "/opt/ads/venv/local/lib/python2.7/site-packages/falcon/api.py", line 154, in __call__
		req = self._request_type(env, options=self.req_options)
	  File "/opt/ads/venv/local/lib/python2.7/site-packages/falcon/request.py", line 237, in __init__
		keep_blank_qs_values=self.options.keep_blank_qs_values,
	  File "/opt/ads/venv/local/lib/python2.7/site-packages/falcon/util/uri.py", line 327, in parse_query_string
		params[k] = decode(v)
	  File "/opt/ads/venv/local/lib/python2.7/site-packages/falcon/util/uri.py", line 181, in decode
		char, byte = _HEX_TO_BYTE[token[:2]]
	KeyError: ''

Would it make more sense to pass-through the original value in this case without trying to decode it? It's a rather small change so I could prepare a pull request for it, but I would like to confirm first that this was not intentional behavior.

Discussion:
philiptzou: Rebased onto the latest version.
philiptzou: Rebased onto the latest version. Again.
kgriffs: @philiptzou Nice work! Just a couple of nits to address and this should be ready to merge.
philiptzou: @kgriffs Sorry for the delay. Just fixed the issues you addressed.

Patch:
diff --git a/falcon/util/uri.py b/falcon/util/uri.py
index f014e5a..2359672 100644
--- a/falcon/util/uri.py
+++ b/falcon/util/uri.py
@@ -178,9 +178,12 @@ if six.PY2:
         tokens = decoded_uri.split('%')
         decoded_uri = tokens[0]
         for token in tokens[1:]:
-            char, byte = _HEX_TO_BYTE[token[:2]]
-            decoded_uri += char + token[2:]
-
+            token_partial = token[:2]
+            if token_partial in _HEX_TO_BYTE:
+                char, byte = _HEX_TO_BYTE[token_partial]
+            else:
+                char, byte = '%', 0
+            decoded_uri += char + (token[2:] if byte else token)
             only_ascii = only_ascii and (byte <= 127)
 
         # PERF(kgriffs): Only spend the time to do this if there
@@ -235,7 +238,12 @@ else:
         tokens = decoded_uri.split(b'%')
         decoded_uri = tokens[0]
         for token in tokens[1:]:
-            decoded_uri += _HEX_TO_BYTE[token[:2]] + token[2:]
+            token_partial = token[:2]
+            if token_partial in _HEX_TO_BYTE:
+                decoded_uri += _HEX_TO_BYTE[token_partial] + token[2:]
+            else:
+                # malformed percentage like "x=%" or "y=%+"
+                decoded_uri += b'%' + token
 
         # Convert back to str
         return decoded_uri.decode('utf-8', 'replace')


Test patch:
diff --git a/tests/test_query_params.py b/tests/test_query_params.py
index 5604e9a..50ed010 100644
--- a/tests/test_query_params.py
+++ b/tests/test_query_params.py
@@ -64,6 +64,16 @@ class _TestQueryParams(testing.TestBase):
         self.assertEqual(req.get_param_as_list('id', int), [23, 42])
         self.assertEqual(req.get_param('q'), u'\u8c46 \u74e3')
 
+    def test_bad_percentage(self):
+        query_string = 'x=%%20%+%&y=peregrine&z=%a%z%zz%1%20e'
+        self.simulate_request('/', query_string=query_string)
+        self.assertEqual(self.srmock.status, falcon.HTTP_200)
+
+        req = self.resource.req
+        self.assertEqual(req.get_param('x'), '% % %')
+        self.assertEqual(req.get_param('y'), 'peregrine')
+        self.assertEqual(req.get_param('z'), '%a%z%zz%1 e')
+
     def test_allowed_names(self):
         query_string = ('p=0&p1=23&2p=foo&some-thing=that&blank=&'
                         'some_thing=x&-bogus=foo&more.things=blah&'