validate proxied paths starting with /

- restrict regex to `(/.*|)` to require starting with `/` or being empty string
- double-safe ensure proxied path starts with / in get_client_uri

Author: minrk

jupyter_server_proxy/handlers.py

@@ -209,12 +209,16 @@ def _get_context_path(self, host, port):
             return url_path_join(self.base_url, 'proxy', host_and_port)
 
     def get_client_uri(self, protocol, host, port, proxied_path):
-        context_path = self._get_context_path(host, port)
         if self.absolute_url:
+            context_path = self._get_context_path(host, port)
             client_path = url_path_join(context_path, proxied_path)
         else:
             client_path = proxied_path
 
+        # ensure client_path always starts with '/'
+        if not client_path.startswith("/"):
+            client_path = "/" + client_path
+
         # Quote spaces, åäö and such, but only enough to send a valid web
         # request onwards. To do this, we mark the RFC 3986 specs' "reserved"
         # and "un-reserved" characters as safe that won't need quoting. The
@@ -228,7 +232,7 @@ def get_client_uri(self, protocol, host, port, proxied_path):
             protocol=protocol,
             host=host,
             port=port,
-            path=client_path
+            path=client_path,
         )
         if self.request.query:
             client_uri += '?' + self.request.query
@@ -297,13 +301,14 @@ async def proxy(self, host, port, proxied_path):
         client = httpclient.AsyncHTTPClient()
 
         req = self._build_proxy_request(host, port, proxied_path, body)
+        self.log.debug(f"Proxying request to {req.url}")
 
         try:
             # Here, "response" is a tornado.httpclient.HTTPResponse object.
             response = await client.fetch(req, raise_error=False)
         except httpclient.HTTPError as err:
             # We need to capture the timeout error even with raise_error=False,
-            # because it only affects the HTTPError raised when a non-200 response 
+            # because it only affects the HTTPError raised when a non-200 response
             # code is used, instead of suppressing all errors.
             # Ref: https://www.tornadoweb.org/en/stable/httpclient.html#tornado.httpclient.AsyncHTTPClient.fetch
             if err.code == 599:
@@ -324,7 +329,7 @@ async def proxy(self, host, port, proxied_path):
         else:
             # Represent the original response as a RewritableResponse object.
             original_response = RewritableResponse(orig_response=response)
-            
+
             # The function (or list of functions) which should be applied to modify the
             # response.
             rewrite_response = self.rewrite_response
@@ -688,53 +693,57 @@ def options(self, path):
 def setup_handlers(web_app, serverproxy_config):
     host_allowlist = serverproxy_config.host_allowlist
     rewrite_response = serverproxy_config.non_service_rewrite_response
-    web_app.add_handlers('.*', [
-        (
-            url_path_join(
-                web_app.settings['base_url'],
-                r'/proxy/([^/]*):(\d+)(.*)',
+    web_app.add_handlers(
+        ".*",
+        [
+            (
+                url_path_join(
+                    web_app.settings["base_url"],
+                    r"/proxy/([^/:@]+):(\d+)(/.*|)",
+                ),
+                RemoteProxyHandler,
+                {
+                    "absolute_url": False,
+                    "host_allowlist": host_allowlist,
+                    "rewrite_response": rewrite_response,
+                },
             ),
-            RemoteProxyHandler,
-            {
-                'absolute_url': False,
-                'host_allowlist': host_allowlist,
-                'rewrite_response': rewrite_response,
-            }
-        ),
-        (
-            url_path_join(
-                web_app.settings['base_url'],
-                r'/proxy/absolute/([^/]*):(\d+)(.*)',
+            (
+                url_path_join(
+                    web_app.settings["base_url"],
+                    r"/proxy/absolute/([^/:@]+):(\d+)(/.*|)",
+                ),
+                RemoteProxyHandler,
+                {
+                    "absolute_url": True,
+                    "host_allowlist": host_allowlist,
+                    "rewrite_response": rewrite_response,
+                },
             ),
-            RemoteProxyHandler,
-            {
-                'absolute_url': True,
-                'host_allowlist': host_allowlist,
-                'rewrite_response': rewrite_response,
-            }
-        ),
-        (
-            url_path_join(
-                web_app.settings['base_url'],
-                r'/proxy/(\d+)(.*)',
+            (
+                url_path_join(
+                    web_app.settings["base_url"],
+                    r"/proxy/(\d+)(/.*|)",
+                ),
+                LocalProxyHandler,
+                {
+                    "absolute_url": False,
+                    "rewrite_response": rewrite_response,
+                },
             ),
-            LocalProxyHandler,
-            {
-                'absolute_url': False,
-                'rewrite_response': rewrite_response,
-            },
-        ),
-        (
-            url_path_join(
-                web_app.settings['base_url'],
-                r'/proxy/absolute/(\d+)(.*)',
+            (
+                url_path_join(
+                    web_app.settings["base_url"],
+                    r"/proxy/absolute/(\d+)(/.*|)",
+                ),
+                LocalProxyHandler,
+                {
+                    "absolute_url": True,
+                    "rewrite_response": rewrite_response,
+                },
             ),
-            LocalProxyHandler,
-            {
-                'absolute_url': True,
-                'rewrite_response': rewrite_response,
-            },
-        ),
-    ])
+        ],
+    )
+
 
 # vim: set et ts=4 sw=4:

tests/test_proxies.py

@@ -291,3 +291,21 @@ async def _websocket_subprotocols():
 def test_server_proxy_websocket_subprotocols(event_loop):
     event_loop.run_until_complete(_websocket_subprotocols())
 
+@pytest.mark.parametrize(
+    "proxy_path, status",
+    [
+        ("127.0.0.1", 404),
+        ("127.0.0.1/path", 404),
+        ("127.0.0.1@192.168.1.1", 404),
+        ("127.0.0.1@192.168.1.1/path", 404),
+        ("user:pass@host:123/foo", 403),
+        ("user:pass@host/foo", 404),
+        ("absolute/127.0.0.1:123@192.168.1.1/path", 404),
+    ]
+)
+def test_bad_server_proxy_url(proxy_path, status):
+    r = request_get(PORT, f"/proxy/{proxy_path}", TOKEN)
+    assert r.code == status
+    if status >= 400:
+        # request should not have been proxied
+        assert 'X-ProxyContextPath' not in r.headers