fix(security): normalize changed-file paths + recurse D20 lint into subpackages #93

Merged

pdurlej merged 1 commit from claude/security-hardening-wave into main

2026-06-17 23:29:51 +02:00

claude commented

2026-06-17 23:22:54 +02:00

Collaborator

What

Two defense-in-depth hardening fixes surfaced by the 2026-06-16 opportunity audit (security-governance finder). Both are M2-permitted (D21 defense hardening), stdlib-only, with regression tests.

1. Path-traversal bypass in `classify_files` (`pr_check.py`)

classify_files matched policy prefixes against raw paths via startswith. A changed-files entry like docs/../secrets/.env would startswith("docs/") and classify as safe_docs_status (eligible_clean) instead of secrets — bypassing the manual gate.

Reachability (honest scope): git normalizes tree paths, so this is not reachable through a normal Forgejo diff. It is reachable via the offline-metadata pipeline (#27), which accepts hand-crafted changed_files. So: real defense-in-depth gap, not a live RCE.
Fix: posixpath.normpath each path before classification → it's classified by its true target (docs/../secrets/.env → secrets/.env → secrets). Any path that escapes the repo root (absolute, or .. after normalization) fails closed to unknown (a sensitive class).

2. D20 boundary lint was flat (`test_d20_architectural_boundary.py`)

_src_module_paths used SRC_DIR.glob("*.py") — top-level only. A future subpackage (e.g. src/patchwarden/adapters/forgejo.py) calling a merge endpoint would slip past the build-time D20 check — the exact Swiss-Cheese gap the lint exists to close.

Fix: rglob (recursive), parametrized root, plus a recursion self-test that proves a nested merge_pull_request( call is caught and __init__.py stays excluded.

Tests

+7 (6 path-traversal/normalization in test_pr_check, 1 recursion in test_d20).
Full suite 303 green (PYTHONPATH=src python3 -m unittest discover tests).

Deliberately NOT included (needs your design call — reported separately)

effective_cloud_review() is defined + unit-tested but never called in the live review_run._findings_from_ollama dispatch path. A sensitive-classification PR could therefore have its diff sent to a cloud Ollama endpoint (OLLAMA_BASE_URL). The fix needs a design decision on the enforcement layer (review_run vs cli vs a dedicated guard), so I did not bundle a possibly-wrong-layer change here.

From the 2026-06-16 opportunity audit. Do not merge — operator merge only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

## What Two **defense-in-depth** hardening fixes surfaced by the 2026-06-16 opportunity audit (security-governance finder). Both are M2-permitted (**D21 defense hardening**), stdlib-only, with regression tests. ### 1. Path-traversal bypass in `classify_files` (`pr_check.py`) `classify_files` matched policy prefixes against **raw** paths via `startswith`. A changed-files entry like `docs/../secrets/.env` would `startswith("docs/")` and classify as **`safe_docs_status` (eligible_clean)** instead of `secrets` — bypassing the manual gate. - **Reachability (honest scope):** git normalizes tree paths, so this is **not** reachable through a normal Forgejo diff. It **is** reachable via the offline-metadata pipeline (#27), which accepts hand-crafted `changed_files`. So: real defense-in-depth gap, not a live RCE. - **Fix:** `posixpath.normpath` each path before classification → it's classified by its **true target** (`docs/../secrets/.env` → `secrets/.env` → `secrets`). Any path that **escapes the repo root** (absolute, or `..` after normalization) **fails closed to `unknown`** (a sensitive class). ### 2. D20 boundary lint was flat (`test_d20_architectural_boundary.py`) `_src_module_paths` used `SRC_DIR.glob("*.py")` — **top-level only**. A future subpackage (e.g. `src/patchwarden/adapters/forgejo.py`) calling a merge endpoint would slip past the build-time D20 check — the exact Swiss-Cheese gap the lint exists to close. - **Fix:** `rglob` (recursive), parametrized `root`, plus a recursion **self-test** that proves a nested `merge_pull_request(` call is caught and `__init__.py` stays excluded. ## Tests - **+7** (6 path-traversal/normalization in `test_pr_check`, 1 recursion in `test_d20`). - Full suite **303 green** (`PYTHONPATH=src python3 -m unittest discover tests`). ## Deliberately NOT included (needs your design call — reported separately) `effective_cloud_review()` is **defined + unit-tested but never called** in the live `review_run._findings_from_ollama` dispatch path. A sensitive-classification PR could therefore have its diff sent to a cloud Ollama endpoint (`OLLAMA_BASE_URL`). The fix needs a **design decision on the enforcement layer** (review_run vs cli vs a dedicated guard), so I did not bundle a possibly-wrong-layer change here. From the 2026-06-16 opportunity audit. **Do not merge — operator merge only.** Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Rows
Columns