From e7e02230e32189d12be1c840bed9bbc5ee7ef7e4 Mon Sep 17 00:00:00 2001 From: DoubleMathew Date: Sun, 17 May 2026 15:16:14 -0500 Subject: [PATCH] Fix /recommended-folders 500 on unreadable model directories (Python 3.12+) (#5523) * Fix /recommended-folders 500 on unreadable model dirs (Python 3.12+) get_recommended_folders probed candidate paths with a bare Path(p).is_dir(). On Python <= 3.11 that returned False for an unreadable path, but on Python 3.12+ is_dir() propagates PermissionError (EACCES) instead, so a stock root-owned ollama install at /usr/share/ollama/.ollama/models (mode 700) made the endpoint 500 through the entire middleware stack. Move the directory-accessibility check into a stdlib-only helper utils.fs_access.is_accessible_dir that swallows OSError and keeps the existing os.access(R_OK|X_OK) filter, restoring the pre-3.12 "unreadable path is simply not a candidate" behaviour. Add a dependency-free regression test. * Address review: inline helper, no new file, cover all probe sites - Drop studio/backend/utils/fs_access.py and the cross-module import; the guard is now a small module-level _safe_is_dir() in models.py (also moots the import-placement / ModuleNotFoundError feedback, since there is no longer an import to misplace). - Apply _safe_is_dir to every system-location probe with the vulnerable bare is_dir() pattern, not just /recommended-folders: _build_browse_allowlist._add and the /browse-folders _add_sug helper, so the same Python 3.12+ PermissionError cannot 500 those endpoints either. Each site keeps its exact prior semantics (recommended-folders retains its os.access(R_OK|X_OK) filter); the only behavioural change is "no longer crashes". - Rewrite the regression test to extract the real _safe_is_dir from source via ast, keeping it dependency-free without standing up the FastAPI app, and correct the mode-000 case. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Roland Tannous <115670425+rolandtannous@users.noreply.github.com> --- studio/backend/routes/models.py | 22 ++- .../test_recommended_folders_permission.py | 125 ++++++++++++++++++ 2 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 studio/backend/tests/test_recommended_folders_permission.py 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)