Skip to content

Commit bd0a899

Browse files
authored
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
1 parent ee41c84 commit bd0a899

4 files changed

Lines changed: 24 additions & 9 deletions

File tree

‎lib/streamlit/web/server/app_static_file_handler.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ def validate_absolute_path(self, root: str, absolute_path: str) -> Optional[str]
4545
# we don't want to serve directories, and serve only files
4646
raise tornado.web.HTTPError(404)
4747

48-
if os.path.commonprefix([full_path, root]) != root:
48+
if os.path.commonpath([full_path, root]) != root:
4949
# Don't allow misbehaving clients to break out of the static files directory
5050
_LOGGER.warning(
5151
"Serving files outside of the static directory is not supported"

‎lib/streamlit/web/server/component_request_handler.py‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,14 @@ def get(self, path: str) -> None:
4343
abspath = os.path.realpath(os.path.join(component_root, filename))
4444

4545
# Do NOT expose anything outside of the component root.
46-
if os.path.commonprefix([component_root, abspath]) != component_root or (
47-
not os.path.normpath(abspath).startswith(
48-
component_root
49-
) # this is a recommendation from CodeQL, probably a bit redundant
50-
):
46+
if os.path.commonpath([component_root, abspath]) != component_root:
5147
self.write("forbidden")
5248
self.set_status(403)
5349
return
5450
try:
5551
with open(abspath, "rb") as file:
5652
contents = file.read()
57-
except (OSError) as e:
53+
except OSError as e:
5854
_LOGGER.error(
5955
"ComponentRequestHandler: GET %s read error", abspath, exc_info=e
6056
)

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ def get_app(self):
6969
(
7070
r"/app/static/(.*)",
7171
AppStaticFileHandler,
72-
{"path": "%s/" % self._tmpdir.name},
72+
{"path": "%s" % self._tmpdir.name},
7373
)
7474
]
7575
)
@@ -137,6 +137,10 @@ def test_staticfiles_404(self):
137137
self.fetch("/app/static/"),
138138
# Access to file outside static directory
139139
self.fetch("/app/static/../test_file_outside_directory.py"),
140+
# Access to file outside static directory with same prefix
141+
self.fetch(
142+
f"/app/static/{self._tmpdir.name}_foo/test_file_outside_directory.py"
143+
),
140144
# Access to symlink outside static directory
141145
self.fetch(f"/app/static/{self._symlink_outside_directory}"),
142146
# Access to non-existent file

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
from streamlit.web.server import ComponentRequestHandler
2222

2323
URL = "http://not.a.real.url:3001"
24-
PATH = "not/a/real/path"
24+
PATH = "/not/a/real/path"
2525

2626

2727
class ComponentRequestHandlerTest(tornado.testing.AsyncHTTPTestCase):
@@ -79,6 +79,21 @@ def test_outside_component_root_request(self):
7979
self.assertEqual(403, response.code)
8080
self.assertEqual(b"forbidden", response.body)
8181

82+
def test_outside_component_dir_with_same_prefix_request(self):
83+
"""Tests to ensure a path based on the same prefix but a different
84+
directory test folder is forbidden."""
85+
86+
with mock.patch("streamlit.components.v1.components.os.path.isdir"):
87+
# We don't need the return value in this case.
88+
declare_component("test", path=PATH)
89+
90+
response = self._request_component(
91+
f"tests.streamlit.web.server.component_request_handler_test.test//{PATH}_really"
92+
)
93+
94+
self.assertEqual(403, response.code)
95+
self.assertEqual(b"forbidden", response.body)
96+
8297
def test_relative_outside_component_root_request(self):
8398
"""Tests to ensure a path relative to the component root directory
8499
(and specifically outside of the component root) is disallowed."""

0 commit comments

Comments
 (0)