Repository: cherrypy/cherrypy

Issue:
Encode URLs in redirect responses
* **I'm submitting a ...**
[x] bug report
[ ] feature request
[ ] question about the decisions made in the repository

* **What is the current behavior?**
The standard response page for 30x errors contains the target URL as `href` as well as clear text.

* **What is the expected behavior?**
Even though browsers *mostly* will not display the result, but redirect instead, you never know:
Any input from unknown sources should be html-escaped.

* **Please tell us about your environment:**
CherryPy 3.8 until 10.1


Discussion:
codacy-bot: 
![Codacy](https://www.codacy.com/assets/images/favicon.png) Here is an overview of what got changed by this pull request:


```diff

Complexity increasing per file
==============================
- cherrypy/_cperror.py  8
         
```


See the complete overview on [Codacy](https://www.codacy.com/app/cherrypy/cherrypy/pullRequest?prid=554915&bid=4292032)
        
mar10: Concerning the tests, it would go something like 
```py
    class RedirectApp(object):
        def index(self, **kwargs):
            raise cherrypy.HTTPRedirect("example.com?redir=<script>alert(1);</script>")
        index.exposed = True
    
    cherrypy.quickstart(RedirectApp())
```

and

```py
    import requests
    res = requests.get("http://127.0.0.1:8080?redir=<abc>", allow_redirects=False)
    assert res.status_code == 303
    assert '<' not in res.content
```

But I don't know where and how to add it.
webknjaz: Regarding tests, please check [`cherrypy/test/test_request_obj.py`](https://github.com/cherrypy/cherrypy/blob/master/cherrypy/test/test_request_obj.py), I don't see tests for 3xx status codes, but it should be relatively easy to add them there :)

P.S. Please note `getPage()`, `assertStatus()` and `assertBody()` helpers.
mar10: Looking at the code I would not consider this change harmful and we already invested quite a bit  of time on this one-liner ;)
But I'll see what I can do (may take a while until I find time though).

Patch:
diff --git a/cherrypy/_cperror.py b/cherrypy/_cperror.py
index 6c952e91..b597c645 100644
--- a/cherrypy/_cperror.py
+++ b/cherrypy/_cperror.py
@@ -271,7 +271,7 @@ class HTTPRedirect(CherryPyException):
                 307: 'This resource has moved temporarily to ',
             }[status]
             msg += '<a href=%s>%s</a>.'
-            msgs = [msg % (saxutils.quoteattr(u), u) for u in self.urls]
+            msgs = [msg % (saxutils.quoteattr(u), escape_html(u)) for u in self.urls]
             response.body = ntob('<br />\n'.join(msgs), 'utf-8')
             # Previous code may have set C-L, so we have to reset it
             # (allow finalize to set it).


Test patch:
diff --git a/cherrypy/test/test_core.py b/cherrypy/test/test_core.py
index 2e590a9d..f16efd58 100644
--- a/cherrypy/test/test_core.py
+++ b/cherrypy/test/test_core.py
@@ -150,6 +150,9 @@ class CoreRequestHandlingTest(helper.CPWebCase):
             def url_with_quote(self):
                 raise cherrypy.HTTPRedirect("/some\"url/that'we/want")
 
+            def url_with_xss(self):
+                raise cherrypy.HTTPRedirect("/some<script>alert(1);</script>url/that'we/want")
+
             def url_with_unicode(self):
                 raise cherrypy.HTTPRedirect(ntou('тест', 'utf-8'))
 
@@ -435,6 +438,13 @@ class CoreRequestHandlingTest(helper.CPWebCase):
         self.assertStatus(303)
         assertValidXHTML()
 
+    def test_redirect_with_xss(self):
+        """A redirect to a URL with HTML injected should result in page contents escaped."""
+        self.getPage('/redirect/url_with_xss')
+        self.assertStatus(303)
+        assert b'<script>' not in self.body
+        assert b'&lt;script&gt;' in self.body
+
     def test_redirect_with_unicode(self):
         """
         A redirect to a URL with Unicode should return a Location