Use commonpath rather than common prefix for more secure access (#7901)

* Use commonpath rather than common prefix for more secure access

* Update Static File Handler

lib/streamlit/web/server/app_static_file_handler.py

@@ -45,7 +45,7 @@ def validate_absolute_path(self, root: str, absolute_path: str) -> Optional[str]
             # we don't want to serve directories, and serve only files
             raise tornado.web.HTTPError(404)
 
-        if os.path.commonprefix([full_path, root]) != root:
+        if os.path.commonpath([full_path, root]) != root:
             # Don't allow misbehaving clients to break out of the static files directory
             _LOGGER.warning(
                 "Serving files outside of the static directory is not supported"

lib/streamlit/web/server/component_request_handler.py

@@ -43,18 +43,14 @@ def get(self, path: str) -> None:
         abspath = os.path.realpath(os.path.join(component_root, filename))
 
         # Do NOT expose anything outside of the component root.
-        if os.path.commonprefix([component_root, abspath]) != component_root or (
-            not os.path.normpath(abspath).startswith(
-                component_root
-            )  # this is a recommendation from CodeQL, probably a bit redundant
-        ):
+        if os.path.commonpath([component_root, abspath]) != component_root:
             self.write("forbidden")
             self.set_status(403)
             return
         try:
             with open(abspath, "rb") as file:
                 contents = file.read()
-        except (OSError) as e:
+        except OSError as e:
             _LOGGER.error(
                 "ComponentRequestHandler: GET %s read error", abspath, exc_info=e
             )

lib/tests/streamlit/web/server/app_static_file_handler_test.py

@@ -69,7 +69,7 @@ def get_app(self):
                 (
                     r"/app/static/(.*)",
                     AppStaticFileHandler,
-                    {"path": "%s/" % self._tmpdir.name},
+                    {"path": "%s" % self._tmpdir.name},
                 )
             ]
         )
@@ -137,6 +137,10 @@ def test_staticfiles_404(self):
             self.fetch("/app/static/"),
             # Access to file outside static directory
             self.fetch("/app/static/../test_file_outside_directory.py"),
+            # Access to file outside static directory with same prefix
+            self.fetch(
+                f"/app/static/{self._tmpdir.name}_foo/test_file_outside_directory.py"
+            ),
             # Access to symlink outside static directory
             self.fetch(f"/app/static/{self._symlink_outside_directory}"),
             # Access to non-existent file

lib/tests/streamlit/web/server/component_request_handler_test.py

@@ -21,7 +21,7 @@
 from streamlit.web.server import ComponentRequestHandler
 
 URL = "http://not.a.real.url:3001"
-PATH = "not/a/real/path"
+PATH = "/not/a/real/path"
 
 
 class ComponentRequestHandlerTest(tornado.testing.AsyncHTTPTestCase):
@@ -79,6 +79,21 @@ def test_outside_component_root_request(self):
         self.assertEqual(403, response.code)
         self.assertEqual(b"forbidden", response.body)
 
+    def test_outside_component_dir_with_same_prefix_request(self):
+        """Tests to ensure a path based on the same prefix but a different
+        directory test folder is forbidden."""
+
+        with mock.patch("streamlit.components.v1.components.os.path.isdir"):
+            # We don't need the return value in this case.
+            declare_component("test", path=PATH)
+
+        response = self._request_component(
+            f"tests.streamlit.web.server.component_request_handler_test.test//{PATH}_really"
+        )
+
+        self.assertEqual(403, response.code)
+        self.assertEqual(b"forbidden", response.body)
+
     def test_relative_outside_component_root_request(self):
         """Tests to ensure a path relative to the component root directory
         (and specifically outside of the component root) is disallowed."""