Skip to content

Commit 29d2fb8

Browse files
authored
Fix Issue 100: Improve Path Validation for Writable Directories (#166)
* fix(models): provide clear error messages for restricted path creation (#100) Signed-off-by: sushant-suse <[email protected]> * fix #100: updated path, removed useless tests Signed-off-by: sushant-suse <[email protected]> --------- Signed-off-by: sushant-suse <[email protected]>
1 parent 2d2078f commit 29d2fb8

4 files changed

Lines changed: 121 additions & 139 deletions

File tree

src/docbuild/models/config/env.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -310,35 +310,37 @@ class EnvPathsConfig(BaseModel):
310310
)
311311
"Path for server root files."
312312

313-
repo_dir: Path = Field(
313+
# --- WRITABLE PATHS START HERE ---
314+
315+
repo_dir: EnsureWritableDirectory = Field(
314316
title="Permanent Repository Directory",
315317
description="The directory where permanent bare Git repositories are stored.",
316318
examples=["/var/cache/docbuild/repos/permanent-full/"],
317319
)
318320
"Path for permanent bare Git repositories."
319321

320-
tmp_repo_dir: Path = Field(
322+
tmp_repo_dir: EnsureWritableDirectory = Field(
321323
title="Temporary Repository Directory",
322324
description="Directory used for temporary working copies cloned from permanent bare repos.",
323325
examples=["/var/cache/docbuild/repos/temporary-branches/"],
324326
)
325327
"Directory for temporary working copies."
326328

327-
base_cache_dir: Path = Field(
329+
base_cache_dir: EnsureWritableDirectory = Field(
328330
title="Base Cache Directory",
329331
description="The root directory for all application-level caches.",
330332
examples=["/var/cache/docserv"],
331333
)
332334
"Base path for all caches."
333335

334-
base_server_cache_dir: Path = Field(
336+
base_server_cache_dir: EnsureWritableDirectory = Field(
335337
title="Base Server Cache Directory",
336338
description="The base directory for server-specific caches.",
337339
examples=["/var/cache/docserv/doc-example-com"],
338340
)
339341
"Base path for server caches."
340342

341-
meta_cache_dir: Path = Field(
343+
meta_cache_dir: EnsureWritableDirectory = Field(
342344
title="Metadata Cache Directory",
343345
description="Cache specifically for repository and deliverable metadata.",
344346
examples=["/var/cache/docbuild/doc-example-com/meta"],

src/docbuild/models/path.py

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -57,22 +57,30 @@ def __get_pydantic_core_schema__(
5757
# --- Validation & Creation Logic ---
5858

5959
@classmethod
60-
def validate_and_create(cls, path: Path) -> Self:
61-
"""Expand user, checks if path exists.
62-
63-
If not, creates it. Then checks permissions.
64-
"""
60+
def validate_and_create(cls: type[Self], path: Path) -> type[Self]:
61+
"""Expand user, check existence/permissions, or check parent for creation."""
6562
# Ensure user expansion happens before any filesystem operations
6663
path = path.expanduser()
6764

68-
# 1. Auto-Creation Logic
65+
# 1. Existence and Creation Logic
6966
if not path.exists():
67+
# Check if the parent is writable so we can create the directory
68+
parent = path.parent
69+
# Find the first existing parent directory to check for write permissions.
70+
# For a path like '/a/b/c', path.parents is ('/a/b', '/a', '/').
71+
# We find the first one in that sequence that exists.
72+
parent = next((p for p in path.parents if p.exists()), path.root)
73+
74+
if not os.access(parent, os.W_OK):
75+
raise ValueError(
76+
f"Cannot create directory '{path}'. "
77+
f"Permission denied: Parent directory '{parent}' is not writable."
78+
)
79+
7080
try:
71-
# parents=True: creates /tmp/a/b even if /tmp/a doesn't exist
72-
# exist_ok=True: prevents race conditions
7381
path.mkdir(parents=True, exist_ok=True)
7482
except OSError as e:
75-
raise ValueError(f"Could not create directory '{path}': {e}") from e
83+
raise ValueError(f"Failed to create directory '{path}': {e.strerror}") from e
7684

7785
# 2. Type Check
7886
if not path.is_dir():
@@ -93,7 +101,6 @@ def validate_and_create(cls, path: Path) -> Self:
93101
f"Missing: {', '.join(missing_perms)}"
94102
)
95103

96-
# Return an instance of the custom type (the __init__ method runs next)
97104
return cls(path)
98105

99106
# --- Usability Methods ---

tests/models/config/test_env.py

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import docbuild.config.app as config_app_mod
1111
from docbuild.models.config.env import EnvConfig
12+
from docbuild.models.path import EnsureWritableDirectory
1213

1314
# --- Fixture Setup ---
1415

@@ -83,7 +84,12 @@ def mock_placeholder_resolution(monkeypatch):
8384

8485
@pytest.fixture
8586
def mock_valid_raw_env_data(tmp_path: Path) -> dict[str, Any]:
86-
"""Provide a minimal, valid dictionary representing env.toml data."""
87+
"""Provide a minimal, valid dictionary representing env.toml data with writable paths."""
88+
89+
# Create a base directory for the test inside the pytest temp folder
90+
base = tmp_path / "docbuild_test"
91+
base.mkdir()
92+
8793
nested_xslt_params = {
8894
"external": {"js": {"onlineonly": "/docserv/res/extra.js"}},
8995
"show": {"edit": {"link": 1}},
@@ -106,18 +112,21 @@ def mock_valid_raw_env_data(tmp_path: Path) -> dict[str, Any]:
106112
"canonical_url_domain": "https://docs.example.com",
107113
},
108114
"paths": {
109-
"config_dir": str(tmp_path / "config"),
110-
"root_config_dir": "/etc/docbuild",
111-
"jinja_dir": "/etc/docbuild/jinja",
112-
"server_rootfiles_dir": "/etc/docbuild/rootfiles",
113-
"base_server_cache_dir": "/var/cache/docserv/server",
114-
"base_tmp_dir": "/var/tmp/docbuild",
115-
"repo_dir": "/var/cache/docbuild/repos/permanent-full/",
116-
"tmp_repo_dir": "/var/cache/docbuild/repos/temporary-branches/",
117-
"base_cache_dir": "/var/cache/docserv",
118-
"meta_cache_dir": "/var/cache/docbuild/doc-example-com/meta",
115+
"config_dir": str(base / "config"),
116+
"root_config_dir": str(base / "etc/docbuild"),
117+
"jinja_dir": str(base / "etc/docbuild/jinja"),
118+
"server_rootfiles_dir": str(base / "etc/docbuild/rootfiles"),
119+
120+
# These use EnsureWritableDirectory - MUST be in tmp_path
121+
"base_cache_dir": str(base / "cache"),
122+
"base_server_cache_dir": str(base / "cache/server"),
123+
"base_tmp_dir": str(base / "var/tmp"),
124+
"repo_dir": str(base / "repos/perm"),
125+
"tmp_repo_dir": str(base / "repos/temp"),
126+
"meta_cache_dir": str(base / "cache/meta"),
127+
119128
"tmp": {
120-
"tmp_base_dir": "/var/tmp/docbuild",
129+
"tmp_base_dir": str(base / "var/tmp"),
121130
"tmp_dir": "{tmp_base_dir}/doc-example-com",
122131
"tmp_deliverable_dir": "{tmp_dir}/deliverable/",
123132
"tmp_build_base_dir": "{tmp_dir}/build",
@@ -129,7 +138,7 @@ def mock_valid_raw_env_data(tmp_path: Path) -> dict[str, Any]:
129138
"target": {
130139
"target_base_dir": "[email protected]:/srv/docs",
131140
"target_dir_dyn": "{{product}}",
132-
"backup_dir": Path("/data/docbuild/external-builds/"),
141+
"backup_dir": str(base / "backups"),
133142
},
134143
},
135144
"build": {
@@ -150,7 +159,7 @@ def test_envconfig_full_success(mock_valid_raw_env_data: dict[str, Any]):
150159
assert isinstance(config, EnvConfig)
151160

152161
# Check type coercion for core types
153-
assert isinstance(config.paths.base_cache_dir, Path)
162+
assert isinstance(config.paths.base_cache_dir, EnsureWritableDirectory)
154163

155164
# Check tmp_dir instead of tmp_path
156165
assert config.paths.tmp.tmp_dir.is_absolute()

tests/models/test_path.py

Lines changed: 74 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -22,39 +22,6 @@ class PathTestModel(BaseModel):
2222
# --- Test Cases ---
2323

2424

25-
def test_writable_directory_success_exists(tmp_path: Path):
26-
"""Test validation succeeds for an existing, writable directory."""
27-
existing_dir = tmp_path / "existing_test_dir"
28-
existing_dir.mkdir()
29-
30-
# Validation should succeed and return the custom type instance
31-
model = PathTestModel(writable_dir=existing_dir) # type: ignore
32-
33-
assert isinstance(model.writable_dir, EnsureWritableDirectory)
34-
assert Path(str(model.writable_dir)).resolve() == existing_dir.resolve()
35-
assert model.writable_dir.is_dir() # Test __getattr__ functionality
36-
37-
38-
def test_writable_directory_success_create_new(tmp_path: Path):
39-
"""Test validation succeeds and creates a new directory.
40-
41-
This test ensures that if the provided path does not exist, it is
42-
automatically created with the necessary parent directories.
43-
"""
44-
new_dir = tmp_path / "non_existent" / "deep" / "path"
45-
46-
# Assert precondition: Path does not exist
47-
assert not new_dir.exists()
48-
49-
# Validation should trigger auto-creation
50-
model = PathTestModel(writable_dir=new_dir) # type: ignore
51-
52-
# Assert postcondition: Path now exists and is a directory
53-
assert model.writable_dir.exists()
54-
assert model.writable_dir.is_dir()
55-
assert Path(str(model.writable_dir)).resolve() == new_dir.resolve()
56-
57-
5825
def test_writable_directory_path_expansion(monkeypatch, tmp_path: Path):
5926
"""Test that the path correctly expands the user home directory (~)."""
6027
# 1. Setup Mock Home Directory
@@ -85,82 +52,6 @@ def test_writable_directory_failure_not_a_directory(tmp_path: Path):
8552
assert "Path exists but is not a directory" in excinfo.value.errors()[0]["msg"]
8653

8754

88-
def test_writable_directory_failure_not_writable(tmp_path: Path, monkeypatch):
89-
"""Test validation fails when the directory lacks write permission.
90-
91-
This test is robust against being run by the root user by patching
92-
``os.access`` to simulate a write permission failure.
93-
"""
94-
read_only_dir = tmp_path / "read_only_dir"
95-
read_only_dir.mkdir()
96-
97-
_original_os_access = os.access
98-
99-
# Patch os.access() to always report write permission is MISSING on this directory
100-
def fake_access(path, mode):
101-
# If we are checking the specific read_only directory for WRITE
102-
# permission, fail it.
103-
if path == read_only_dir.resolve() and mode == os.W_OK:
104-
return False
105-
106-
# Otherwise, call the safely stored original function.
107-
return _original_os_access(path, mode)
108-
109-
monkeypatch.setattr(os, "access", fake_access)
110-
111-
# The actual chmod is now primarily symbolic, the mock forces the logic path
112-
read_only_dir.chmod(0o444)
113-
114-
try:
115-
with pytest.raises(ValidationError) as excinfo:
116-
PathTestModel(writable_dir=read_only_dir) # type: ignore
117-
118-
assert (
119-
"Insufficient permissions for directory" in excinfo.value.errors()[0]["msg"]
120-
)
121-
assert "WRITE" in excinfo.value.errors()[0]["msg"]
122-
finally:
123-
# Restore permissions to ensure cleanup (Crucial for CI)
124-
read_only_dir.chmod(0o777)
125-
126-
127-
def test_writable_directory_failure_not_executable(tmp_path: Path, monkeypatch):
128-
"""Test validation fails when the directory lacks execute permission.
129-
130-
This test is robust against being run by the root user by patching
131-
``os.access`` to simulate an execute/search permission failure.
132-
"""
133-
no_exec_dir = tmp_path / "no_exec_dir"
134-
no_exec_dir.mkdir()
135-
136-
_original_os_access = os.access
137-
138-
# Patch os.access() to always report execute permission is MISSING
139-
def fake_access(path, mode):
140-
if path == no_exec_dir.resolve() and mode == os.X_OK:
141-
return False # Force failure on execute check
142-
143-
# Otherwise, call the safely stored original function.
144-
return _original_os_access(path, mode)
145-
146-
monkeypatch.setattr(os, "access", fake_access)
147-
148-
# The actual chmod is now primarily symbolic, the mock forces the logic path
149-
no_exec_dir.chmod(0o666)
150-
151-
try:
152-
with pytest.raises(ValidationError) as excinfo:
153-
PathTestModel(writable_dir=no_exec_dir) # type: ignore
154-
155-
assert (
156-
"Insufficient permissions for directory" in excinfo.value.errors()[0]["msg"]
157-
)
158-
assert "EXECUTE" in excinfo.value.errors()[0]["msg"]
159-
finally:
160-
# Restore permissions
161-
no_exec_dir.chmod(0o777)
162-
163-
16455
def test_writable_directory_failure_mkdir_os_error(monkeypatch, tmp_path: Path):
16556
"""Test that an OSError during directory creation is handled.
16657
@@ -184,7 +75,7 @@ def mock_mkdir(*args, **kwargs):
18475
# Assert that the error is correctly wrapped in a ValueError/ValidationError
18576
error_msg = excinfo.value.errors()[0]["msg"]
18677
assert "Value error" in error_msg
187-
assert "Could not create directory" in error_msg
78+
assert "Failed to create directory" in error_msg
18879
assert "Simulated permission denied" in error_msg
18980

19081

@@ -229,3 +120,76 @@ def test_fspath_protocol_compatibility(
229120
result = path_consumer(custom_path_obj)
230121
expected = expected_factory(test_dir)
231122
assert result == expected
123+
124+
125+
def test_writable_directory_failure_parent_not_writable(tmp_path: Path, monkeypatch):
126+
"""Test validation fails when the parent directory is not writable.
127+
128+
This simulates the scenario where a user tries to create a directory
129+
in a protected root folder (like /data).
130+
"""
131+
# 1. Setup a "protected" parent and a target child
132+
protected_parent = tmp_path / "protected_parent"
133+
protected_parent.mkdir()
134+
target_dir = protected_parent / "new_child_dir"
135+
136+
# 2. Mock os.access to report the parent as NOT writable
137+
_original_os_access = os.access
138+
139+
def fake_access(path, mode):
140+
# Resolve path to ensure comparison works on all OSs
141+
if str(path) == str(protected_parent.resolve()) and mode == os.W_OK:
142+
return False
143+
return _original_os_access(path, mode)
144+
145+
monkeypatch.setattr(os, "access", fake_access)
146+
147+
# 3. Action & Assertions
148+
with pytest.raises(ValidationError) as excinfo:
149+
PathTestModel(writable_dir=target_dir) # type: ignore
150+
151+
error_msg = excinfo.value.errors()[0]["msg"]
152+
assert "Cannot create directory" in error_msg
153+
assert "Permission denied: Parent directory" in error_msg
154+
assert str(protected_parent.resolve()) in error_msg
155+
156+
157+
@pytest.mark.parametrize(
158+
"permission_to_fail, expected_missing_perm",
159+
[
160+
(os.R_OK, "READ"),
161+
(os.W_OK, "WRITE"),
162+
(os.X_OK, "EXECUTE"),
163+
],
164+
ids=["missing_read", "missing_write", "missing_execute"],
165+
)
166+
def test_writable_directory_permission_failures(
167+
tmp_path: Path, monkeypatch, permission_to_fail, expected_missing_perm
168+
):
169+
"""Test validation fails when a specific permission is missing.
170+
171+
This test is robust against being run by the root user by patching
172+
``os.access`` to simulate a specific permission failure (R, W, or X).
173+
"""
174+
test_dir = tmp_path / "permission_test_dir"
175+
test_dir.mkdir()
176+
177+
original_os_access = os.access
178+
179+
# Patch os.access() to fail only the specified permission check for our test directory.
180+
def fake_access(path, mode):
181+
# Resolve path to ensure comparison works reliably across OSs
182+
if Path(path).resolve() == test_dir.resolve() and mode == permission_to_fail:
183+
return False
184+
185+
# For all other checks, or other paths, use the original behavior.
186+
return original_os_access(path, mode)
187+
188+
monkeypatch.setattr(os, "access", fake_access)
189+
190+
with pytest.raises(ValidationError) as excinfo:
191+
PathTestModel(writable_dir=test_dir) # type: ignore
192+
193+
error_msg = excinfo.value.errors()[0]["msg"]
194+
assert "Insufficient permissions for directory" in error_msg
195+
assert f"Missing: {expected_missing_perm}" in error_msg

0 commit comments

Comments
 (0)