diff --git a/solara/server/cdn_helper.py b/solara/server/cdn_helper.py index 614cc18ea..69c085a14 100644 --- a/solara/server/cdn_helper.py +++ b/solara/server/cdn_helper.py @@ -1,9 +1,9 @@ import logging -import os import pathlib import shutil import requests +from solara.server.utils import path_is_child_of import solara.settings @@ -14,6 +14,9 @@ def put_in_cache(base_cache_dir: pathlib.Path, path, data: bytes): cache_path = base_cache_dir / path + if not path_is_child_of(cache_path, base_cache_dir): + raise PermissionError("Trying to write outside of cache directory") + pathlib.Path(cache_path.parent).mkdir(parents=True, exist_ok=True) try: logger.info("Writing cache file: %s", cache_path) @@ -27,16 +30,8 @@ def get_from_cache(base_cache_dir: pathlib.Path, path): # Make sure cache_path is a subdirectory of base_cache_dir # so we don't accidentally read files from the parent directory # which is a security risk. - # We use os.path.normpath() because we do not want to follow symlinks - # in editable installs, since some packages are symlinked - if not os.path.normpath(cache_path).startswith(os.path.normpath(base_cache_dir)): - logger.warning( - "Trying to read from outside of cache directory: %s is not a subdir of %s (%s - %s)", - cache_path, - base_cache_dir, - os.path.normpath(cache_path), - os.path.normpath(base_cache_dir), - ) + if not path_is_child_of(cache_path, base_cache_dir): + logger.warning("Trying to read from outside of cache directory: %s is not a subdir of %s", cache_path, base_cache_dir) raise PermissionError("Trying to read from outside of cache directory") try: @@ -74,6 +69,9 @@ def get_path(base_cache_dir: pathlib.Path, path) -> pathlib.Path: store_path = path if len(parts) != 1 else pathlib.Path(path) / "__main.js" cache_path = base_cache_dir / store_path + if not path_is_child_of(cache_path, base_cache_dir): + raise PermissionError("Trying to read from outside of cache directory") + if cache_path.exists(): # before d7eba856f100d5c3c64f4eec22c62390f084cb40 on windows, we could # accidentally write to the cache directory, so we need to check if we still diff --git a/solara/server/flask.py b/solara/server/flask.py index 6b659a55c..d8f48e2be 100644 --- a/solara/server/flask.py +++ b/solara/server/flask.py @@ -199,7 +199,7 @@ def nbext(dir, filename): for directory in server.nbextensions_directories: file = directory / dir / filename if file.exists(): - return send_from_directory(directory / dir, filename) + return send_from_directory(directory, dir + os.path.sep + filename) return flask.Response("not found", status=404) @@ -217,7 +217,10 @@ def cdn(path): if not allowed(): abort(401) cache_directory = settings.assets.proxy_cache_dir - content = cdn_helper.get_data(Path(cache_directory), path) + try: + content = cdn_helper.get_data(Path(cache_directory), path) + except PermissionError: + return flask.Response("not found", status=404) mime = mimetypes.guess_type(path) return flask.Response(content, mimetype=mime[0]) diff --git a/solara/server/starlette.py b/solara/server/starlette.py index 299d3a9a3..a62c4a1d1 100644 --- a/solara/server/starlette.py +++ b/solara/server/starlette.py @@ -3,6 +3,7 @@ import logging import math import os +from pathlib import Path import sys import threading import typing @@ -14,6 +15,8 @@ import uvicorn.server import websockets.legacy.http +from solara.server.utils import path_is_child_of + try: import solara_enterprise @@ -405,6 +408,9 @@ def lookup_path(self, path: str) -> typing.Tuple[str, typing.Optional[os.stat_re original_path = os.path.join(directory, path) full_path = os.path.realpath(original_path) directory = os.path.realpath(directory) + # return early if someone tries to access a file outside of the directory + if not path_is_child_of(Path(original_path), Path(directory)): + return "", None try: return full_path, os.stat(full_path) except (FileNotFoundError, NotADirectoryError): @@ -449,7 +455,6 @@ def lookup_path(self, path: str) -> typing.Tuple[str, typing.Optional[os.stat_re full_path = str(get_path(settings.assets.proxy_cache_dir, path)) except Exception: return "", None - return full_path, os.stat(full_path) diff --git a/solara/server/utils.py b/solara/server/utils.py index 42f1a3d47..2f718942f 100644 --- a/solara/server/utils.py +++ b/solara/server/utils.py @@ -1,6 +1,7 @@ import contextlib import logging import os +from pathlib import Path import pdb import traceback @@ -16,6 +17,12 @@ def start_error(title, msg, exception: Exception = None): os._exit(-1) +def path_is_child_of(path: Path, parent: Path) -> bool: + # We use os.path.normpath() because we do not want to follow symlinks + # in editable installs, since some packages are symlinked + return os.path.normpath(path).startswith(os.path.normpath(parent)) + + @contextlib.contextmanager def pdb_guard(): from . import settings diff --git a/tests/integration/apps/not-allowed b/tests/integration/apps/not-allowed new file mode 100644 index 000000000..d3730bda0 --- /dev/null +++ b/tests/integration/apps/not-allowed @@ -0,0 +1 @@ +not accessible diff --git a/tests/integration/apps/public/test.txt b/tests/integration/apps/public/test.txt new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/integration/apps/public/test.txt @@ -0,0 +1 @@ +test diff --git a/tests/integration/apps/secure/app.py b/tests/integration/apps/secure/app.py new file mode 100644 index 000000000..68986fc0d --- /dev/null +++ b/tests/integration/apps/secure/app.py @@ -0,0 +1,4 @@ +import solara + + +page = solara.Button("Click me") diff --git a/tests/integration/server_test.py b/tests/integration/server_test.py index 93a332070..5126f1274 100644 --- a/tests/integration/server_test.py +++ b/tests/integration/server_test.py @@ -5,8 +5,11 @@ import playwright.sync_api import pytest import reacton.ipywidgets as w +import requests import solara +import solara.server.server +from solara.server import settings HERE = Path(__file__).parent @@ -211,3 +214,100 @@ def test_kernel_asyncio(browser: playwright.sync_api.Browser, solara_server, sol context1.close() context2.close() solara.server.settings.kernel.threaded = threaded + + +def test_cdn_secure(solara_server, solara_app, extra_include_path): + cdn_url = solara_server.base_url + "/_solara/cdn" + assert solara.settings.assets.proxy + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = cdn_url + "/vue-grid-layout@1.0.2/dist/vue-grid-layout.min.js" + response = requests.get(url) + assert response.status_code == 200 + # create a file in /share/solara + test_file = settings.assets.proxy_cache_dir.parent / "not-allowed" + test_file.write_text("test") + url = cdn_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_nbextension_secure(solara_server, solara_app, extra_include_path): + nbextensions_url = solara_server.base_url + "/static/nbextensions" + nbextensions_directories = [k for k in solara.server.server.nbextensions_directories if k.exists()] + assert nbextensions_directories, "we should at least test one directory" + nbextensions_directory = nbextensions_directories[0] + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = nbextensions_url + "/jupyter-vuetify/nodeps.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = nbextensions_directory.parent / "not-allowed" + test_file.write_text("test") + url = nbextensions_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + url = nbextensions_url + "/foo/..%2f..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_assets_secure(solara_server, solara_app, extra_include_path): + assets_url = solara_server.base_url + "/static/assets" + assets_directory = solara.server.server.solara_static.parent / "assets" + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = assets_url + "/theme.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = assets_directory.parent / "__init__.py" + assert test_file.exists() + url = assets_url + "/..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + url = assets_url + "/foo/..%2f..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + +def test_public_secure(solara_server, solara_app, extra_include_path): + public_url = solara_server.base_url + "/static/public" + + with solara_app(str(HERE / "apps/secure/app.py")): + apps = list(solara.server.app.apps.values()) + assert len(apps) == 1 + app = apps[0] + public_directory = app.directory.parent / "public" + url = public_url + "/test.txt" + response = requests.get(url) + assert response.status_code == 200 + test_file = public_directory.parent / "not-allowed" + assert test_file.exists() + url = public_url + "/..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + url = public_url + "/foo/..%2f..%2fnot-allowed" + response = requests.get(url) + assert response.status_code == 404 + + +def test_static_secure(solara_server, solara_app, extra_include_path): + static_url = solara_server.base_url + "/static" + static_directory = solara.server.server.solara_static + + with extra_include_path(HERE), solara_app("server_test:ClickButton"): + url = static_url + "/main.js" + response = requests.get(url) + assert response.status_code == 200 + test_file = static_directory.parent / "__init__.py" + assert test_file.exists() + url = static_url + "/..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404 + + url = static_url + "/foo/..%2f..%2f__init__.py" + response = requests.get(url) + assert response.status_code == 404