diff --git a/studio/backend/routes/models.py b/studio/backend/routes/models.py index d01e94b0c9..9ea113e488 100644 --- a/studio/backend/routes/models.py +++ b/studio/backend/routes/models.py @@ -26,6 +26,22 @@ def _is_valid_repo_id(repo_id: str) -> bool: return bool(_VALID_REPO_ID.fullmatch(repo_id)) +def _safe_is_dir(path) -> bool: + """``Path.is_dir()`` that returns ``False`` instead of raising. + + On Python >= 3.12 ``is_dir()``'s ``os.stat`` only suppresses + "not found"-class errors and now propagates ``PermissionError`` + (EACCES); on Python <= 3.11 it returned ``False``. The folder-scan + endpoints probe well-known system locations (e.g. a root-owned, + mode-700 ``/usr/share/ollama/.ollama/models``) and must treat an + un-stat-able path as "not a directory", never 500. + """ + try: + return Path(path).is_dir() + except OSError: + return False + + # Add backend directory to path backend_path = Path(__file__).parent.parent.parent if str(backend_path) not in sys.path: @@ -882,7 +898,7 @@ async def get_recommended_folders( return if resolved in seen: return - if Path(resolved).is_dir() and os.access(resolved, os.R_OK | os.X_OK): + if _safe_is_dir(resolved) and os.access(resolved, os.R_OK | os.X_OK): seen.add(resolved) folders.append(resolved) @@ -1056,7 +1072,7 @@ def _build_browse_allowlist() -> list[Path]: resolved = p.resolve() except OSError: return - if resolved.is_dir(): + if _safe_is_dir(resolved): candidates.append(resolved) _add(Path.home()) @@ -1389,7 +1405,7 @@ async def browse_folders( return if resolved in seen_sug: return - if Path(resolved).is_dir(): + if _safe_is_dir(resolved): seen_sug.add(resolved) suggestions.append(resolved) diff --git a/studio/backend/tests/test_recommended_folders_permission.py b/studio/backend/tests/test_recommended_folders_permission.py new file mode 100644 index 0000000000..659c3b547d --- /dev/null +++ b/studio/backend/tests/test_recommended_folders_permission.py @@ -0,0 +1,125 @@ +# SPDX-License-Identifier: AGPL-3.0-only +# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0 + +""" +Regression test for the /recommended-folders (and /browse-folders) 500 +caused by an unreadable model directory, e.g. a stock root-owned +``ollama`` install at ``/usr/share/ollama/.ollama/models``. + +Root cause: the folder-scan helpers in ``routes.models`` probed candidate +paths with a bare ``Path(p).is_dir()``. On Python <= 3.11 that returned +``False`` for an unreadable path; on Python >= 3.12 ``is_dir()`` propagates +``PermissionError`` (EACCES), so the endpoint 500-ed through the whole +middleware stack instead of just skipping the directory. The probes now go +through the module-level ``_safe_is_dir`` helper. + +``routes.models`` pulls the full backend dependency tree (fastapi, +structlog, the models package, ...), so rather than stand up the app we +extract the real ``_safe_is_dir`` definition from the source file and +exercise that exact function in isolation. The test therefore stays +dependency-free while still running the shipped code. + +Run: + python -m pytest studio/backend/tests/test_recommended_folders_permission.py -v +""" + +import ast +import os +import sys +from pathlib import Path + +import pytest + +_backend_root = Path(__file__).resolve().parent.parent +_models_src = _backend_root / "routes" / "models.py" + + +def _load_safe_is_dir(): + """Return the real ``_safe_is_dir`` from routes/models.py without + importing the (heavily dependency-laden) module.""" + tree = ast.parse(_models_src.read_text()) + fn = next( + node + for node in tree.body + if isinstance(node, ast.FunctionDef) and node.name == "_safe_is_dir" + ) + module = ast.Module(body = [fn], type_ignores = []) + ns: dict = {"Path": Path, "os": os} + exec(compile(module, f"", "exec"), ns) + return ns["_safe_is_dir"] + + +safe_is_dir = _load_safe_is_dir() + +# Permission bits are bypassed for the superuser, so the chmod-000 setup +# below would not actually deny access when running as root. +_skip_as_root = pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason = "root bypasses filesystem permission bits", +) + + +def test_helper_exists_in_source(): + # Guards against a refactor silently dropping the helper the fix + # depends on (the extractor would then raise StopIteration). + assert callable(safe_is_dir) + + +def test_readable_dir_is_true(tmp_path): + assert safe_is_dir(tmp_path) is True + + +def test_missing_path_is_false(tmp_path): + assert safe_is_dir(tmp_path / "does-not-exist") is False + + +def test_file_is_false(tmp_path): + f = tmp_path / "weights.gguf" + f.write_bytes(b"x") + assert safe_is_dir(f) is False + + +@_skip_as_root +def test_mode000_dir_itself_is_still_a_dir(tmp_path): + """A mode-000 directory is still stat-able via its (traversable) + parent, so _safe_is_dir reports True without raising. Filtering out + dirs we cannot actually *read* is the caller's separate + os.access(R_OK|X_OK) check, not this helper's job.""" + locked = tmp_path / "locked" + locked.mkdir() + os.chmod(locked, 0o000) + try: + assert safe_is_dir(locked) is True # must not raise + finally: + os.chmod(locked, 0o755) + + +@_skip_as_root +def test_path_under_unreadable_parent_returns_false_not_raises(tmp_path): + """The exact production scenario: stat()-ing a child of a mode-700 + system directory, e.g. ``/usr/share/ollama/.ollama/models``.""" + parent = tmp_path / "ollama" + parent.mkdir() + os.chmod(parent, 0o000) + try: + assert safe_is_dir(parent / ".ollama" / "models") is False + finally: + os.chmod(parent, 0o755) + + +@_skip_as_root +@pytest.mark.skipif( + sys.version_info < (3, 12), + reason = "is_dir() only propagates PermissionError on Python >= 3.12", +) +def test_demonstrates_the_underlying_stdlib_regression(tmp_path): + """Documents *why* _safe_is_dir exists: the old bare pattern raises + on the interpreters Studio ships on (3.12+).""" + parent = tmp_path / "ollama" + parent.mkdir() + os.chmod(parent, 0o000) + try: + with pytest.raises(PermissionError): + Path(parent / ".ollama" / "models").is_dir() # pre-fix expr + finally: + os.chmod(parent, 0o755)