fix(security): add path traversal guard and fail-closed error handling to handoff (#408)
Addresses unresolved review comments from PR #407: 1. Path traversal prevention: resolved path must stay within repo_root, raises PodcasterHandoffError if article_path escapes (e.g. ../../secrets). 2. Fail-closed on read errors: if article file exists but cannot be read, raise PodcasterHandoffError instead of silently omitting content (which would reintroduce the placeholder-script failure mode). Adds tests for both cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Juan Manuel Servera committed
Jun 12, 2026 at 12:32 UTC
335eb8e2d8684b55f072c718138d278a23e54024
2 files changed
+48
-4
scripts/podcaster_handoff.py
+15
-4
@@ -149,15 +149,26 @@ def _read_article_content(article_path: str, repo_root: Path = REPO_ROOT) -> tup
149
"""Read article file content and extract title.
150
151
Returns (content, title). Content is truncated to MAX_ARTICLE_CONTENT_CHARS.
152
- Returns (None, None) if the file does not exist or is empty.
152
+ Returns (None, None) if the file does not exist.
153
+ Raises PodcasterHandoffError if the file exists but cannot be read, or if
154
+ the resolved path escapes the repo root (path traversal prevention).
155
"""
154
- resolved = repo_root / article_path
156
+ resolved = (repo_root / article_path).resolve()
157
+ # Prevent path traversal — resolved path must stay within repo_root.
158
+ try:
159
+ resolved.relative_to(repo_root.resolve())
160
+ except ValueError:
161
+ raise PodcasterHandoffError(
162
+ f"article_path resolves outside the repository root: {article_path}"
163
+ )
164
if not resolved.exists():
165
return None, None
166
try:
167
content = resolved.read_text(encoding="utf-8")
159
- except OSError:
160
- return None, None
168
+ except OSError as exc:
169
+ raise PodcasterHandoffError(
170
+ f"Article file exists but could not be read: {resolved} ({exc})"
171
+ )
172
if not content.strip():
173
return None, None
174
title = _extract_title(content)
tests/test_podcaster_handoff.py
+33
@@ -377,6 +377,39 @@ class PodcasterHandoffTests(unittest.TestCase):
377
self.assertNotIn("article_content", payload)
378
self.assertNotIn("article_title", payload)
379
380
+ def test_read_article_content_path_traversal_raises(self) -> None:
381
+ """Path traversal attempts must raise PodcasterHandoffError."""
382
+ with tempfile.TemporaryDirectory() as tmpdir:
383
+ base = Path(tmpdir)
384
+ # Create a file outside repo_root
385
+ outside = base.parent / "secret.txt"
386
+ outside.write_text("secret data", encoding="utf-8")
387
+ try:
388
+ with self.assertRaises(podcaster_handoff.PodcasterHandoffError) as ctx:
389
+ podcaster_handoff._read_article_content("../secret.txt", repo_root=base)
390
+ self.assertIn("outside the repository root", str(ctx.exception))
391
+ finally:
392
+ outside.unlink(missing_ok=True)
393
+
394
+ def test_read_article_content_unreadable_file_raises(self) -> None:
395
+ """An existing but unreadable file must raise, not silently omit content."""
396
+ with tempfile.TemporaryDirectory() as tmpdir:
397
+ base = Path(tmpdir)
398
+ article_dir = base / "content" / "weekly"
399
+ article_dir.mkdir(parents=True)
400
+ article_file = article_dir / "W24.md"
401
+ article_file.write_text("# Test", encoding="utf-8")
402
+ # Make file unreadable
403
+ article_file.chmod(0o000)
404
+ try:
405
+ with self.assertRaises(podcaster_handoff.PodcasterHandoffError) as ctx:
406
+ podcaster_handoff._read_article_content(
407
+ "content/weekly/W24.md", repo_root=base
408
+ )
409
+ self.assertIn("could not be read", str(ctx.exception))
410
+ finally:
411
+ article_file.chmod(0o644)
412
+
413
414
if __name__ == "__main__":
415
unittest.main()