* 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>
125 lines
4.2 KiB
Python
125 lines
4.2 KiB
Python
# 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"<extracted {_models_src}>", "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)
|