Skip to content

fix: reject absolute paths in directory env vars #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
CyberSecDef merged 2 commits into from
Apr 8, 2026
Merged

fix: reject absolute paths in directory env vars #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9731432
Initial plan
Copilot Apr 8, 2026
9a44598
fix: reject absolute paths in directory env vars (SESSION_FILE_DIR, E…
Copilot Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions novelforge/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,17 +233,35 @@ def _parse_llm_providers() -> list[ProviderConfig]:
# Flask secret key – override via SECRET_KEY environment variable in production
SECRET_KEY = os.environ.get("SECRET_KEY", "change-me-in-production")


def _resolve_dir(env_var: str, default: str) -> str:
"""Return the absolute path for a directory env var, anchored to PROJECT_ROOT.

Raises:
ValueError: if the env var is set to an absolute path. All directory
env vars must be relative to *PROJECT_ROOT* to prevent arbitrary
filesystem access.
"""
raw = os.environ.get(env_var, default)
if os.path.isabs(raw):
raise ValueError(
f"{env_var} must be a relative path (got {raw!r}). "
"Directory env vars are resolved relative to the project root."
)
Comment on lines +236 to +250

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raising ValueError here happens at import time (these constants are set during module import). Since novelforge/init.py only catches ConfigurationError around validate_config(), an absolute dir env var will crash app startup with an uncaught exception and bypass the structured validation/logging path. Consider recording this as a config-parse error (similar to get_env_int) and surfacing it from validate_config() as ConfigurationError, or otherwise ensuring create_app/startup handles it cleanly.

Suggested change
def _resolve_dir(env_var: str, default: str) -> str:
"""Return the absolute path for a directory env var, anchored to PROJECT_ROOT.
Raises:
ValueError: if the env var is set to an absolute path. All directory
env vars must be relative to *PROJECT_ROOT* to prevent arbitrary
filesystem access.
"""
raw = os.environ.get(env_var, default)
if os.path.isabs(raw):
raise ValueError(
f"{env_var} must be a relative path (got {raw!r}). "
"Directory env vars are resolved relative to the project root."
)
# Configuration parse problems detected during module import. Callers can
# surface these later via the normal validation/startup path.
_CONFIG_PARSE_ERRORS: list[str] = []
def _resolve_dir(env_var: str, default: str) -> str:
"""Return the absolute path for a directory env var, anchored to PROJECT_ROOT.
Absolute paths are rejected because directory env vars must stay relative
to *PROJECT_ROOT*. To avoid crashing at import time, invalid values are
recorded and logged, and the safe default is used instead.
"""
raw = os.environ.get(env_var, default)
if os.path.isabs(raw):
message = (
f"{env_var} must be a relative path (got {raw!r}). "
"Directory env vars are resolved relative to the project root."
)
_CONFIG_PARSE_ERRORS.append(message)
logging.getLogger(__name__).error(message)
raw = default

Copilot uses AI. Check for mistakes.
return str(PROJECT_ROOT / raw)

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still allows path traversal via relative values like "../.." (e.g., PROJECT_ROOT / "../etc"), which can escape PROJECT_ROOT while remaining non-absolute. If the goal is to prevent redirecting these dirs to arbitrary filesystem locations, normalize the joined path (e.g., resolve(strict=False)) and verify it stays within PROJECT_ROOT (Path.is_relative_to), rejecting anything that escapes.

Copilot uses AI. Check for mistakes.


# Directory where Flask-Session stores server-side session files
SESSION_FILE_DIR = str(PROJECT_ROOT / os.environ.get("SESSION_FILE_DIR", "sessions/flask"))
SESSION_FILE_DIR = _resolve_dir("SESSION_FILE_DIR", "sessions/flask")

# Directory where exported novel files are stored temporarily
EXPORT_DIR = str(PROJECT_ROOT / os.environ.get("EXPORT_DIR", "exports"))
EXPORT_DIR = _resolve_dir("EXPORT_DIR", "exports")

# Directory where novel session JSON files are stored
NOVELS_DIR = str(PROJECT_ROOT / os.environ.get("NOVELS_DIR", "sessions/novels"))
NOVELS_DIR = _resolve_dir("NOVELS_DIR", "sessions/novels")

# Directory for log files
LOGS_DIR = str(PROJECT_ROOT / os.environ.get("LOGS_DIR", "logs"))
LOGS_DIR = _resolve_dir("LOGS_DIR", "logs")


class ConfigurationError(Exception):
Expand Down
44 changes: 44 additions & 0 deletions tests/test_validate_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,50 @@
import novelforge.config as cfg


# ---------------------------------------------------------------------------
# _resolve_dir helper
# ---------------------------------------------------------------------------

class TestResolveDir:
"""Unit tests for the _resolve_dir() helper."""

def test_returns_project_root_relative_path_when_unset(self, monkeypatch):
monkeypatch.delenv("NF_TEST_DIR", raising=False)
result = cfg._resolve_dir("NF_TEST_DIR", "my/subdir")
assert result == str(cfg.PROJECT_ROOT / "my/subdir")

def test_returns_project_root_relative_path_for_relative_env_var(self, monkeypatch):
monkeypatch.setenv("NF_TEST_DIR", "custom/path")
result = cfg._resolve_dir("NF_TEST_DIR", "default/path")
assert result == str(cfg.PROJECT_ROOT / "custom/path")

def test_raises_for_absolute_path(self, monkeypatch):
monkeypatch.setenv("NF_TEST_DIR", "/etc/passwd")
with pytest.raises(ValueError, match="NF_TEST_DIR"):
cfg._resolve_dir("NF_TEST_DIR", "default")

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests cover absolute-path rejection, but they don’t cover traversal/escape cases such as "../outside" (which is relative but can resolve outside PROJECT_ROOT). Adding a regression test for that case would lock in the intended security property if _resolve_dir is tightened to enforce PROJECT_ROOT containment.

Suggested change
def test_raises_for_relative_path_traversal_outside_project_root(self, monkeypatch):
monkeypatch.setenv("NF_TEST_DIR", "../outside")
with pytest.raises(ValueError, match="NF_TEST_DIR"):
cfg._resolve_dir("NF_TEST_DIR", "default")

Copilot uses AI. Check for mistakes.
def test_error_message_contains_bad_value(self, monkeypatch):
monkeypatch.setenv("NF_TEST_DIR", "/absolute/path")
with pytest.raises(ValueError, match="/absolute/path"):
cfg._resolve_dir("NF_TEST_DIR", "default")

def test_raises_for_root_path(self, monkeypatch):
monkeypatch.setenv("NF_TEST_DIR", "/")
with pytest.raises(ValueError):
cfg._resolve_dir("NF_TEST_DIR", "default")

def test_relative_default_accepted_when_env_var_unset(self, monkeypatch):
monkeypatch.delenv("NF_TEST_DIR", raising=False)
# Should not raise; default is relative
result = cfg._resolve_dir("NF_TEST_DIR", "logs")
assert result.endswith("logs")

def test_result_is_string(self, monkeypatch):
monkeypatch.delenv("NF_TEST_DIR", raising=False)
result = cfg._resolve_dir("NF_TEST_DIR", "data")
assert isinstance(result, str)


# ---------------------------------------------------------------------------
# get_env_int helper
# ---------------------------------------------------------------------------
Expand Down
Loading