Repository: AspenWeb/pando.py

Issue:
Handling of non-ASCII URLs is still broken
I tried to fix the `make_franken_uri()` function in #585, but it still doesn't handle all cases properly.

Discussion:

Patch:
diff --git a/pando/http/request.py b/pando/http/request.py
index 40e9e1b2..423ec875 100644
--- a/pando/http/request.py
+++ b/pando/http/request.py
@@ -31,6 +31,7 @@ from __future__ import unicode_literals
 import re
 import string
 import sys
+import traceback
 
 from six import PY2
 
@@ -39,7 +40,7 @@ from aspen.http.request import Path as _Path, Querystring as _Querystring
 from .. import Response
 from ..exceptions import MalformedBody, UnknownBodyType
 from ..urlparse import quote, quote_plus
-from ..utils import try_encode
+from ..utils import maybe_encode
 from .baseheaders import BaseHeaders
 from .mapping import Mapping
 
@@ -51,9 +52,6 @@ from .mapping import Mapping
 # routines for going from WSGI back to HTTP. Since WSGI is lossy, we end up
 # with a Dr. Frankenstein's HTTP message.
 
-quoted_slash_re = re.compile("%2F", re.IGNORECASE)
-
-
 def make_franken_uri(path, qs):
     """Given two bytestrings, return a bytestring.
 
@@ -74,23 +72,17 @@ def make_franken_uri(path, qs):
         try:
             path.decode('ASCII')    # NB: We throw away this unicode!
         except UnicodeDecodeError:
-
-            # XXX How would we get non-ASCII here? The lookout.net post
-            # indicates that all browsers send ASCII for the path.
-
-            # Some servers (gevent) clobber %2F inside of paths, such
-            # that we see /foo%2Fbar/ as /foo/bar/. The %2F is lost to us.
-            parts = [quote(x) for x in quoted_slash_re.split(path)]
-            path = b"%2F".join(parts)
+            # Either the client sent unescaped non-ASCII bytes, or the web server
+            # unescaped the path.
+            path = quote(path, string.punctuation).encode('ASCII')
 
     if qs:
         try:
             qs.decode('ASCII')      # NB: We throw away this unicode!
         except UnicodeDecodeError:
-            # Cross our fingers and hope we have UTF-8 bytes from MSIE. Let's
-            # perform the percent-encoding that we would expect MSIE to have
-            # done for us.
-            qs = quote_plus(qs)
+            # Either the client sent unescaped non-ASCII bytes, or the web server
+            # unescaped the query.
+            qs = quote_plus(qs, string.punctuation).encode('ASCII')
         qs = b'?' + qs
 
     return path + qs
@@ -165,24 +157,23 @@ class Request(object):
         Ref: https://www.python.org/dev/peps/pep-3333/#a-note-on-string-types
 
         """
-        environ = {try_encode(k): try_encode(v) for k, v in environ.items()}
         try:
+            environ = {
+                maybe_encode(k, 'latin1'): maybe_encode(v, 'latin1')
+                for k, v in environ.items()
+            }
             return cls(website, *kick_against_goad(environ))
-        except UnicodeError:
-            # Figure out where the error occurred.
-            # ====================================
-            # This gives us *something* to go on when we have a Request we
-            # can't parse. XXX Make this more nicer. That will require wrapping
-            # every point in Request parsing where we decode bytes.
-
-            tb = sys.exc_info()[2]
-            while tb.tb_next is not None:
-                tb = tb.tb_next
-            frame = tb.tb_frame
-            filename = tb.tb_frame.f_code.co_filename
-
-            raise Response(400, "Request is undecodable. "
-                                "(%s:%d)" % (filename, frame.f_lineno))
+        except UnicodeError as e:
+            if website.show_tracebacks:
+                msg = traceback.format_exc()
+            else:
+                tb = sys.exc_info()[2]
+                while tb.tb_next is not None:
+                    tb = tb.tb_next
+                frame = tb.tb_frame
+                filename = tb.tb_frame.f_code.co_filename
+                msg = "Request is undecodable: %s (%s:%d)" % (e, filename, frame.f_lineno)
+            raise Response(400, msg)
 
     # Aliases
     # =======
diff --git a/pando/http/response.py b/pando/http/response.py
index e8143f34..8a48a2f1 100644
--- a/pando/http/response.py
+++ b/pando/http/response.py
@@ -102,6 +102,10 @@ class Response(Exception):
         return "<Response: %s>" % self._status_text()
 
     def __str__(self):
+        body = self.body
+        if len(body) < 500:
+            body = body.decode('ascii', 'repr') if isinstance(body, bytes) else body
+            return ': '.join((self._status_text(), body))
         return self._status_text()
 
     def _status_text(self):
diff --git a/pando/state_chain.py b/pando/state_chain.py
index f7e60513..5d064476 100644
--- a/pando/state_chain.py
+++ b/pando/state_chain.py
@@ -59,7 +59,7 @@ def request_available():
 
 def raise_200_for_OPTIONS(request):
     """A hook to return 200 to an 'OPTIONS \*' request"""
-    if request.line.method == b"OPTIONS" and request.line.uri == "*":
+    if request.line.method == b"OPTIONS" and request.line.uri == b"*":
         raise Response(200)
 
 
diff --git a/pando/utils.py b/pando/utils.py
index 46813466..2a1618b8 100644
--- a/pando/utils.py
+++ b/pando/utils.py
@@ -31,13 +31,6 @@ def maybe_encode(s, codec='ascii'):
     return s.encode(codec) if isinstance(s, text_type) else s
 
 
-def try_encode(s, codec='ascii'):
-    try:
-        return maybe_encode(s, codec)
-    except UnicodeError:
-        return s
-
-
 # datetime helpers
 # ================
 


Test patch:
diff --git a/pando/testing/client.py b/pando/testing/client.py
index d0e51df6..8e64f76c 100644
--- a/pando/testing/client.py
+++ b/pando/testing/client.py
@@ -164,7 +164,7 @@ class Client(object):
         return out
 
 
-    def build_wsgi_environ(self, method, path, body, content_type, cookies=None, **kw):
+    def build_wsgi_environ(self, method, url, body, content_type, cookies=None, **kw):
 
         # NOTE that in Pando (request.py make_franken_headers) only headers
         # beginning with ``HTTP`` are included in the request - and those are
@@ -177,13 +177,22 @@ class Client(object):
             for k, v in d.items():
                 cookies[str(k)] = str(v)
 
-        typecheck(path, (bytes, text_type), method, text_type, content_type, bytes, body, bytes)
+        typecheck(url, (bytes, text_type), method, text_type, content_type, bytes, body, bytes)
+        url = url.encode('ascii') if type(url) != bytes else url
+        if b'?' in url:
+            path, qs = url.split(b'?', 1)
+        else:
+            path, qs = url, None
+
         environ = {}
         environ[b'CONTENT_TYPE'] = content_type
         if cookies is not None:
             environ[b'HTTP_COOKIE'] = cookies.output(header='', sep='; ')
         environ[b'HTTP_HOST'] = b'localhost'
-        environ[b'PATH_INFO'] = path.encode('ascii') if type(path) != bytes else path
+        if path:
+            environ[b'PATH_INFO'] = path
+        if qs:
+            environ[b'QUERY_STRING'] = qs
         environ[b'REMOTE_ADDR'] = b'0.0.0.0'
         environ[b'REQUEST_METHOD'] = method.encode('ascii')
         environ[b'SERVER_PROTOCOL'] = b'HTTP/1.1'
diff --git a/tests/test_request.py b/tests/test_request.py
index aa2fffcf..c59c773c 100644
--- a/tests/test_request.py
+++ b/tests/test_request.py
@@ -8,9 +8,9 @@ from __future__ import unicode_literals
 from pytest import raises
 
 from pando import Response
-from pando.http.request import kick_against_goad, Request
-from pando.http.baseheaders import BaseHeaders
 from pando.exceptions import MalformedHeader
+from pando.http.request import kick_against_goad, make_franken_uri, Request
+from pando.http.baseheaders import BaseHeaders
 
 
 def test_raw_is_raw():
@@ -154,6 +154,14 @@ def test_cookie_alias_is_read_only(harness):
         request.cookie = 'foo'
 
 
+# make_franken_uri
+
+def test_make_franken_uri_works_with_properly_escaped_uri():
+    expected = b'/%C2%B5?%C2%B5=%C2%B5&foo=bar'
+    actual = make_franken_uri(b'/%C2%B5', b'%C2%B5=%C2%B5&foo=bar')
+    assert actual == expected
+
+
 # kick_against_goad
 
 def test_goad_passes_method_through():
@@ -212,17 +220,36 @@ def test_goad_passes_body_through():
 
 # from_wsgi
 
-def test_from_wsgi_tolerates_non_ascii_environ():
+def test_from_wsgi_tolerates_non_ascii_environ(harness):
     environ = {}
     environ[b'REQUEST_METHOD'] = b'GET'
-    environ[b'HTTP_HOST'] = b'localhost'
+    environ[b'HTTP_HOST'] = 'µ.example.com'.encode('utf8')
     environ[b'SERVER_PROTOCOL'] = b'HTTP/1.0'
     environ[b'wsgi.input'] = None
     environ[b'HTTP_\xff'] = b'\xdead\xbeef'
-    environ['HTTP_À'] = 'µ'
-    headers = Request.from_wsgi(None, environ).headers
+    environ['HTTP_À'.encode('utf8')] = 'µ'.encode('utf8')
+    environ[b'PATH_INFO'] = '/µ'.encode('utf8')
+    environ[b'QUERY_STRING'] = 'µ=µ'.encode('utf8')
+    request = Request.from_wsgi(harness.client.website, environ)
+    assert request.line.uri == b'/%C2%B5?%C2%B5=%C2%B5'
+    headers = request.headers
+    assert headers[b'Host'] == b'\xc2\xb5.example.com'
+    assert headers[b'\xff'] is environ[b'HTTP_\xff']
+    assert headers['À'.encode('utf8')] == 'µ'.encode('utf8')
+
+def test_from_wsgi_tolerates_unicode_environ(harness):
+    environ = {}
+    environ['REQUEST_METHOD'] = 'GET'
+    environ['HTTP_HOST'] = 'µ.example.com'.encode('utf8').decode('latin1')
+    environ['SERVER_PROTOCOL'] = 'HTTP/1.0'
+    environ['wsgi.input'] = None
+    environ[b'HTTP_\xff'] = b'\xdead\xbeef'
+    environ['HTTP_À'] = 'µ'.encode('utf8').decode('latin1')
+    environ['PATH_INFO'] = '/µ'.encode('utf8').decode('latin1')
+    environ['QUERY_STRING'] = 'µ=µ'.encode('utf8').decode('latin1')
+    request = Request.from_wsgi(harness.client.website, environ)
+    assert request.line.uri == b'/%C2%B5?%C2%B5=%C2%B5'
+    headers = request.headers
+    assert headers[b'Host'] == b'\xc2\xb5.example.com'
     assert headers[b'\xff'] is environ[b'HTTP_\xff']
-    if str is bytes:
-        assert headers['À'] is environ['HTTP_À']
-    else:
-        assert 'À' not in headers
+    assert headers['À'.encode('latin1')] == 'µ'.encode('utf8')
diff --git a/tests/test_website_flow.py b/tests/test_website_flow.py
index 1d5d9966..aff4f0f9 100644
--- a/tests/test_website_flow.py
+++ b/tests/test_website_flow.py
@@ -73,3 +73,8 @@ def test_static_resource_unknown_method(harness):
     harness.fs.www.mk(('file.js', "Hello world!"))
     r = harness.client.hxt('UNKNOWN', '/file.js')
     assert r.code == 405
+
+
+def test_raise_200_for_OPTIONS(harness):
+    r = harness.client.hxt('OPTIONS', '*')
+    assert r.code == 200