Merge pull request 'Security hardening: auth, CSRF, path traversal, XSS, tests' (#34) from fix/security-and-tests into main

tests/test_path_handling.py

@@ -1,107 +1,77 @@
 #!/usr/bin/env python3
-"""Unit tests for storage_manager path handling."""
+"""Unit tests for path handling and security."""
 
+import sys
+import tempfile
 from pathlib import Path
 from urllib.parse import unquote
 
-# Use actual ARCHIVE_DIR path
-ARCHIVE_DIR = Path("/Volumes/playground/NewsArchiver/archival_data")
+sys.path.insert(0, str(Path(__file__).parent.parent))
 
 
-def test_relative_path_format():
-    """Test the relative path format after save_article."""
-    # Simulate what save_article does
-    archive_file_path = (
-        ARCHIVE_DIR / "websites/404 Media/html/2024-01-15/article_001.html"
-    )
+def test_path_traversal_blocked():
+    """Test that path traversal is blocked in archive routes."""
+    archive_dir = Path(tempfile.mkdtemp())
 
-    # This is what we store in the database
-    stored_path = str(archive_file_path.relative_to(ARCHIVE_DIR))
+    def validate_archive_path(archive_path: str):
+        decoded = unquote(archive_path)
+        try:
+            archive_file = (archive_dir / decoded).resolve()
+            if not str(archive_file).startswith(str(archive_dir)):
+                return None
+            return archive_file
+        except (ValueError, OSError):
+            return None
 
-    # Verify stored path is relative
-    assert not Path(stored_path).is_absolute()
-    assert stored_path == "websites/404 Media/html/2024-01-15/article_001.html"
+    for bad_path in [
+        "../../../etc/passwd",
+        "..%2F..%2F..%2Fetc%2Fpasswd",
+        "../secret.txt",
+        "websites/../../etc/passwd",
+    ]:
+        result = validate_archive_path(bad_path)
+        assert result is None, f"Traversal should be blocked: {bad_path}"
+        print(f"  PASS: blocked '{bad_path}'")
 
-    # Simulate what get_article does when retrieving
-    retrieved_path = Path(stored_path)
-    if not retrieved_path.is_absolute():
-        full_path = ARCHIVE_DIR / retrieved_path
-    else:
-        full_path = retrieved_path
-
-    # Verify full path is correct
-    assert str(full_path) == str(archive_file_path)
-
-    # This is what we return for the web interface
-    web_path = str(full_path.relative_to(ARCHIVE_DIR))
-    assert web_path == stored_path
-
-    print(f"Test passed! Stored: {stored_path}, Web: {web_path}")
+    valid = validate_archive_path("websites/Test/html/2024-01-01/article.html")
+    assert valid is not None
+    assert str(valid).startswith(str(archive_dir))
+    print(f"  PASS: allowed valid path")
 
 
-def test_multiple_sources():
-    """Test that different sources get correct paths."""
-    sources = ["404 Media", "TestSource", "Another Source"]
+def test_path_traversal_file_access():
+    """Test that path traversal cannot read files outside archive dir."""
+    archive_dir = Path(tempfile.mkdtemp())
+    secret_file = archive_dir.parent / "secret.txt"
+    secret_file.write_text("top secret")
 
-    for source in sources:
-        archive_file_path = (
-            ARCHIVE_DIR / f"websites/{source}/html/2024-01-15/article_001.html"
-        )
-        stored_path = str(archive_file_path.relative_to(ARCHIVE_DIR))
+    def validate_archive_path(archive_path: str):
+        decoded = unquote(archive_path)
+        try:
+            archive_file = (archive_dir / decoded).resolve()
+            if not str(archive_file).startswith(str(archive_dir)):
+                return None
+            return archive_file
+        except (ValueError, OSError):
+            return None
 
-        # Verify path structure
-        parts = Path(stored_path).parts
-        assert parts[0] == "websites"
-        assert parts[1] == source
-        assert parts[2] == "html"
-
-        print(f"Source '{source}': {stored_path}")
+    blocked = validate_archive_path(f"../secret.txt")
+    assert blocked is None
+    print(f"  PASS: file access traversal blocked")
+    secret_file.unlink()
 
 
-def test_archive_file_url_generation():
-    """Test that the URL for archived files is correct."""
-    # Simulate what the template does
+def test_url_encoding():
+    """Test URL encoding/decoding in archive paths."""
     archive_file_path = "websites/404 Media/html/2024-01-15/article_001.html"
-
-    # This is what the template generates
-    url = f"/archive-file/{archive_file_path}"
-
-    # Verify URL format
-    assert url == "/archive-file/websites/404 Media/html/2024-01-15/article_001.html"
-
-    # Simulate what the route handler does
-    decoded_path = unquote(archive_file_path)
-    full_path = ARCHIVE_DIR / decoded_path
-
-    # Verify the full path is correct
-    expected_path = ARCHIVE_DIR / "websites/404 Media/html/2024-01-15/article_001.html"
-    assert str(full_path) == str(expected_path)
-
-    print(f"URL: {url}")
-    print(f"Full path: {full_path}")
-
-
-def test_old_absolute_path_handling():
-    """Test handling of old absolute paths from different servers."""
-    # Old absolute path from a different server
-    old_absolute_path = Path(
-        "/home/user/playground/NewsArchiver/archival_data/websites/404 Media/html/2024-01-15/article_001.html"
-    )
-
-    # Check if it's absolute
-    assert old_absolute_path.is_absolute()
-
-    # The code handles this by checking is_absolute() first
-    # If the path is absolute but not under ARCHIVE_DIR, we can still try to extract
-    # the relative part by checking if ARCHIVE_DIR is in the path
-    if old_absolute_path.is_absolute():
-        # For this test, we just verify the logic
-        print("Old absolute path handling verified")
+    encoded = archive_file_path.replace(" ", "%20")
+    decoded = unquote(encoded)
+    assert decoded == archive_file_path
+    print(f"  PASS: encoding round-trip")
 
 
 if __name__ == "__main__":
-    test_relative_path_format()
-    test_multiple_sources()
-    test_archive_file_url_generation()
-    test_old_absolute_path_handling()
-    print("\nAll tests passed!")
+    test_path_traversal_blocked()
+    test_path_traversal_file_access()
+    test_url_encoding()
+    print("\nAll path handling tests passed!")

tests/test_web_interface.py

@@ -0,0 +1,71 @@
+#!/usr/bin/env python3
+"""Unit tests for web_interface.py security features."""
+
+import subprocess
+import sys
+import tempfile
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+
+def test_validate_archive_path_blocks_traversal():
+    """Test validate_archive_path function blocks traversal."""
+    from web_interface import validate_archive_path, ARCHIVE_DIR
+
+    for bad in ["../../../etc/passwd", "..%2Fetc%2Fpasswd", "../secret.txt"]:
+        result = validate_archive_path(bad)
+        assert result is None, f"Should block: {bad}"
+    print("  PASS: traversal blocked")
+
+
+def test_xss_safe_filter_removed():
+    """Test that article.html no longer uses |safe filter."""
+    result = subprocess.run(
+        ["grep", "-rn", "|safe", "templates/article.html"],
+        capture_output=True, text=True,
+    )
+    assert result.returncode != 0, "article.html should not contain |safe"
+    print("  PASS: no |safe in article.html")
+
+
+def test_security_headers_present():
+    """Test security headers are added to responses."""
+    from web_interface import app
+    client = app.test_client()
+    resp = client.get("/")
+    assert resp.headers.get("X-Content-Type-Options") == "nosniff"
+    assert resp.headers.get("X-Frame-Options") == "DENY"
+    assert resp.headers.get("X-XSS-Protection") == "1; mode=block"
+    assert resp.headers.get("Referrer-Policy") == "strict-origin-when-cross-origin"
+    print("  PASS: security headers present")
+
+
+def test_server_url_not_hardcoded():
+    """Test that SERVER_URL comes from env, not hardcoded."""
+    result = subprocess.run(
+        ["grep", "-n", "192\\.168", "web_interface.py"],
+        capture_output=True, text=True,
+    )
+    assert result.returncode != 0, "No hardcoded internal IPs"
+    print("  PASS: no hardcoded IPs")
+
+
+def test_no_safe_filter_in_rss():
+    """Test RSS/Atom feeds escape content properly."""
+    result = subprocess.run(
+        ["grep", "-rn", "|safe", "templates/rss.xml", "templates/atom.xml"],
+        capture_output=True, text=True,
+    )
+    assert result.returncode != 0, "No |safe in RSS templates"
+    print("  PASS: no |safe in RSS/Atom")
+
+
+if __name__ == "__main__":
+    test_validate_archive_path_blocks_traversal()
+    test_xss_safe_filter_removed()
+    test_security_headers_present()
+    test_server_url_not_hardcoded()
+    test_no_safe_filter_in_rss()
+    print("\nAll web interface tests passed!")

web_interface.py

@@ -17,7 +17,7 @@ from datetime import datetime, timezone
 from functools import wraps
 from pathlib import Path
 from typing import Optional
-from urllib.parse import quote, urlparse
+from urllib.parse import quote, unquote, urlparse
 
 from flask import (
     Flask,
@@ -302,7 +302,9 @@ def articles(slug: str):
 
 
 def validate_archive_path(archive_path: str) -> Optional[Path]:
-    """Validate that an archive path is within ARCHIVE_DIR (prevents path traversal).
+    """Validate archive path is within ARCHIVE_DIR (prevents path traversal).
+
+    URL-decodes path first to block encoded traversal (..%2F).
 
     Args:
         archive_path: Requested path component
@@ -310,8 +312,9 @@ def validate_archive_path(archive_path: str) -> Optional[Path]:
     Returns:
         Resolved Path if valid, None if traversal detected
     """
+    decoded = unquote(archive_path)
     try:
-        archive_file = (ARCHIVE_DIR / archive_path).resolve()
+        archive_file = (ARCHIVE_DIR / decoded).resolve()
         if not str(archive_file).startswith(str(ARCHIVE_DIR)):
             logger.warning("Path traversal attempt blocked: %s", archive_path)
             return None
@@ -334,10 +337,7 @@ def serve_archive(archive_path):
 @login_required
 def serve_archive_file(encoded_path):
     """Serve archived HTML file from encoded path."""
-    import urllib.parse
-
-    archive_path = urllib.parse.unquote(encoded_path)
-    archive_file = validate_archive_path(archive_path)
+    archive_file = validate_archive_path(encoded_path)
     if archive_file and archive_file.exists():
         return archive_file.read_text(encoding="utf-8")
     return "Archive not found", 404