Repository: cherrypy/cherrypy

Issue:
`cherrypy.url` fails to normalize path 
This call to `cherrypy.url` fails with `IndexError`:
```
>>> cherrypy.url(qs='../../../../../../etc/passwd')
...
IndexError: pop from empty list
```
The culprit seems in this logic, which strips `newurl` of as many `atoms` as there are `..`: 
https://github.com/cherrypy/cherrypy/blob/master/cherrypy/_helper.py#L261,L271

There are various problems.

- That logic should only applied to the "path" part of `newurl`, not to the full url.

- As a consequence of the point above, `..` in the query string `qs` should not be considered

- To consider: redundant `..` should be ignored, to mimic `os.path.normpath`:

```
>>> os.path.normpath('/etc/../../../usr')
'/usr'
```

Discussion:

Patch:
diff --git a/cherrypy/_helper.py b/cherrypy/_helper.py
index 5875ec0f..9b727eac 100644
--- a/cherrypy/_helper.py
+++ b/cherrypy/_helper.py
@@ -223,6 +223,30 @@ def url(path='', qs='', script_name=None, base=None, relative=None):
     if qs:
         qs = '?' + qs
 
+    def normalize_path(path):
+        if './' not in path:
+            return path
+
+        # Normalize the URL by removing ./ and ../
+        atoms = []
+        for atom in path.split('/'):
+            if atom == '.':
+                pass
+            elif atom == '..':
+                # Don't pop from empty list
+                # (i.e. ignore redundant '..')
+                if atoms:
+                    atoms.pop()
+            elif atom:
+                atoms.append(atom)
+
+        newpath = '/'.join(atoms)
+        # Preserve leading '/'
+        if path.startswith('/'):
+            newpath = '/' + newpath
+
+        return newpath
+
     if cherrypy.request.app:
         if not path.startswith('/'):
             # Append/remove trailing slash from path_info as needed
@@ -246,7 +270,7 @@ def url(path='', qs='', script_name=None, base=None, relative=None):
         if base is None:
             base = cherrypy.request.base
 
-        newurl = base + script_name + path + qs
+        newurl = base + script_name + normalize_path(path) + qs
     else:
         # No request.app (we're being called outside a request).
         # We'll have to guess the base from server.* attributes.
@@ -256,19 +280,7 @@ def url(path='', qs='', script_name=None, base=None, relative=None):
             base = cherrypy.server.base()
 
         path = (script_name or '') + path
-        newurl = base + path + qs
-
-    if './' in newurl:
-        # Normalize the URL by removing ./ and ../
-        atoms = []
-        for atom in newurl.split('/'):
-            if atom == '.':
-                pass
-            elif atom == '..':
-                atoms.pop()
-            else:
-                atoms.append(atom)
-        newurl = '/'.join(atoms)
+        newurl = base + normalize_path(path) + qs
 
     # At this point, we should have a fully-qualified absolute URL.
 


Test patch:
diff --git a/cherrypy/test/test_core.py b/cherrypy/test/test_core.py
index f16efd58..252c1ac5 100644
--- a/cherrypy/test/test_core.py
+++ b/cherrypy/test/test_core.py
@@ -74,6 +74,9 @@ class CoreRequestHandlingTest(helper.CPWebCase):
                     relative = bool(relative)
                 return cherrypy.url(path_info, relative=relative)
 
+            def qs(self, qs):
+                return cherrypy.url(qs=qs)
+
         def log_status():
             Status.statuses.append(cherrypy.response.status)
         cherrypy.tools.log_status = cherrypy.Tool(
@@ -647,6 +650,8 @@ class CoreRequestHandlingTest(helper.CPWebCase):
         self.assertBody('%s/url/other/page1' % self.base())
         self.getPage('/url/?path_info=/other/./page1')
         self.assertBody('%s/other/page1' % self.base())
+        self.getPage('/url/?path_info=/other/././././page1')
+        self.assertBody('%s/other/page1' % self.base())
 
         # Double dots
         self.getPage('/url/leaf?path_info=../page1')
@@ -655,6 +660,20 @@ class CoreRequestHandlingTest(helper.CPWebCase):
         self.assertBody('%s/url/page1' % self.base())
         self.getPage('/url/leaf?path_info=/other/../page1')
         self.assertBody('%s/page1' % self.base())
+        self.getPage('/url/leaf?path_info=/other/../../../page1')
+        self.assertBody('%s/page1' % self.base())
+        self.getPage('/url/leaf?path_info=/other/../../../../../page1')
+        self.assertBody('%s/page1' % self.base())
+
+        # qs param is not normalized as a path
+        self.getPage('/url/qs?qs=/other')
+        self.assertBody('%s/url/qs?/other' % self.base())
+        self.getPage('/url/qs?qs=/other/../page1')
+        self.assertBody('%s/url/qs?/other/../page1' % self.base())
+        self.getPage('/url/qs?qs=../page1')
+        self.assertBody('%s/url/qs?../page1' % self.base())
+        self.getPage('/url/qs?qs=../../page1')
+        self.assertBody('%s/url/qs?../../page1' % self.base())
 
         # Output relative to current path or script_name
         self.getPage('/url/?path_info=page1&relative=True')