fix(server): harden auth, SSE, state, scraping

- Fix SSE streams not terminating on successful jobs (C-2) - Anchor data/session paths to BASE_DIR instead of CWD (C-3) - Guard TelegramAuthManager state with RLock (H-1) - Replace millisecond job ids with uuid4 (H-2) - Always redact api_id/api_hash on export, drop include_secrets (H-3) - Enforce JSON content-type + same-origin on mutating requests (H-4) - Rate-limit auth attempts and phone-code requests (H-5) - Deep-copy StateStore.load() on all paths (H-6) - Cap FloodWait retries in forward_message (H-7) - De-duplicate forwarding handler registration (H-8) - Validate continuous channels at ingest, join scrape thread on account removal, fix refresh_config status under lock, cap SSE streams and JSON body size (M-5, M-6, M-17) - Add regression tests (33 passing) and REVIEW.md
2026-09-07 11:54:13 +02:00
parent 59824940c6
commit a2468a2a2c
8 changed files with 973 additions and 186 deletions
@@ -14,6 +14,7 @@ They verify:
  - load_messages() pagination with search filter
 """

+import io
 import os
 import shutil
 import sqlite3
@@ -124,6 +125,29 @@ def create_channel_db(data_dir: Path, account_id: Optional[str], channel_id: str
    conn.close()


+def _make_ws_handler(headers=None, body=b""):
+    """Build a bare webui_server request handler for unit-level checks.
+
+    Avoids touching sockets - read_json_body()/_check_same_origin() only use
+    headers/rfile/send_error_json, which are stubbed here.
+    """
+    import webui_server as ws_module
+
+    hdrs = dict(headers or {})
+    hdrs.setdefault("Content-Length", str(len(body)))
+    handler = object.__new__(ws_module.TelegramScraperRequestHandler)
+    handler.headers = hdrs
+    handler.rfile = io.BytesIO(body)
+    handler.wfile = io.BytesIO()
+    handler.path = "/api/test"
+    handler.command = "POST"
+    handler.client_address = ("127.0.0.1", 4321)
+    handler.server = MagicMock()
+    handler.send_error_json = MagicMock()
+    handler.send_json = MagicMock()
+    return handler
+
+
 # ── Fixture setup / teardown ────────────────────────────────────────────────


@@ -305,11 +329,29 @@ class TestInputSafety:
        redacted = ws_module.export_account_state(state)
        assert redacted["api_hash"] is None
        assert redacted["api_hash_present"] is True
+        # api_id is sensitive too and must never be exported
+        assert redacted["api_id"] is None
+        assert redacted["api_id_present"] is True
        assert state["api_hash"] == "secret"
+        assert state["api_id"] == 123

-        full = ws_module.export_account_state(state, include_secrets=True)
-        assert full["api_hash"] == "secret"
-        assert "api_hash_present" not in full
+    def test_clean_continuous_channels_drops_invalid_and_normalizes(self):
+        """Continuous config ingest must reject path-traversal entries."""
+        import webui_server as ws_module
+
+        cleaned, dropped = ws_module.clean_continuous_channels([
+            "@valid_name", "123", "nested/channel", r"nested\channel", "../escape", ".", "..", "",
+        ])
+        # Valid entries pass through normalized (leading '@' stripped, numbers kept)
+        assert cleaned == ["valid_name", "123"]
+        # Invalid entries are dropped (not persisted), preserving order of appearance
+        assert dropped == ["nested/channel", r"nested\channel", "../escape", ".", "..", ""]
+
+    def test_clean_continuous_channels_non_list_input(self):
+        import webui_server as ws_module
+
+        assert ws_module.clean_continuous_channels(None) == ([], [])
+        assert ws_module.clean_continuous_channels("not-a-list") == ([], [])


 class TestContinuousOrchestrator:
@@ -396,6 +438,74 @@ class TestContinuousOrchestrator:
        finally:
            self._restore_ws_data_dir()

+    def test_stop_and_join_waits_for_thread_then_flips_running(self):
+        """stop()+join() must let status['running'] become False only after the
+        worker thread truly exits, and join must be safe/idempotent on a
+        short-lived thread."""
+        PerAccountContinuousScrapeManager, _, ws = self._import_orch_classes()
+        self._setup_ws_data_dir()
+        try:
+            aid = make_account_id()
+            create_account(TEST_DATA, aid, continuous_scraping={
+                "enabled": True, "interval_minutes": 60, "channels": [], "run_all_tracked": True,
+            })
+            mgr = PerAccountContinuousScrapeManager(aid)
+            # Keep the loop from doing anything slow: skip config reload and
+            # make auth check see a non-authorized account so it just waits on
+            # the stop event briefly.
+            mgr.refresh_config = lambda: None
+            mgr.join  # ensure attribute exists
+            mgr.start()
+            assert mgr.status["running"] is True
+            assert mgr.thread is not None and mgr.thread.is_alive()
+
+            mgr.stop()
+            # stop() only requests; running stays True until the thread exits.
+            assert mgr.status["running"] is True
+
+            # Second stop() must be safe (idempotent).
+            mgr.stop()
+
+            assert mgr.join(timeout=5.0) is True, "worker thread did not exit"
+            # After join, the thread's finally has flipped running to False.
+            assert mgr.status["running"] is False
+
+            # join() on a dead/never-started thread is a no-op success.
+            fresh = PerAccountContinuousScrapeManager(aid)
+            assert fresh.join(timeout=1.0) is True
+        finally:
+            self._restore_ws_data_dir()
+
+    def test_refresh_config_disable_requests_stop_without_lying_about_running(self):
+        """refresh_config() disabling the account must request a stop but must
+        NOT set status['running']=False (the worker thread owns that)."""
+        PerAccountContinuousScrapeManager, _, ws = self._import_orch_classes()
+        self._setup_ws_data_dir()
+        try:
+            aid = make_account_id()
+            create_account(TEST_DATA, aid, continuous_scraping={
+                "enabled": True, "interval_minutes": 60, "channels": [], "run_all_tracked": True,
+            })
+            mgr = PerAccountContinuousScrapeManager(aid)
+            # Simulate a live worker by faking the running state and an enabled
+            # in-memory config, then flip the on-disk config to disabled.
+            with mgr.lock:
+                mgr.status["running"] = True
+                mgr.config["enabled"] = True
+            create_account(TEST_DATA, aid, continuous_scraping={
+                "enabled": False, "interval_minutes": 60, "channels": [], "run_all_tracked": True,
+            })
+            mgr.refresh_config()
+            assert mgr.stop_event.is_set(), "refresh_config should request stop"
+            # running is NOT touched by refresh_config — the loop's finally sets it.
+            assert mgr.status["running"] is True
+            # After the worker would exit, running flips to False (simulated here).
+            with mgr.lock:
+                mgr.status["running"] = False
+            assert mgr.status["running"] is False
+        finally:
+            self._restore_ws_data_dir()
+

 class TestMessageSearch:
    """load_messages() SQL correctness with pagination and search."""
@@ -576,7 +686,6 @@ class TestJobDeduplicationSchema:
        runner = JobRunner()
        runner.queue.put = lambda job: None
        j1 = runner.create_job("scrape_all", "First", {"account_id": "acc1"})
-        time.sleep(0.002)  # ensure different timestamp -> different job_id
        j2 = runner.create_job("scrape_all", "Second", {"account_id": "acc2"})
        assert j1.job_id != j2.job_id
        runner.shutdown(timeout=0)
@@ -587,7 +696,6 @@ class TestJobDeduplicationSchema:
        runner.queue.put = lambda job: None
        j1 = runner.create_job("scrape_all", "First", {"account_id": "acc1"})
        j1.status = "done"
-        time.sleep(0.002)  # ensure different timestamp -> different job_id
        j2 = runner.create_job("scrape_all", "Second", {"account_id": "acc1"})
        assert j1.job_id != j2.job_id
        runner.shutdown(timeout=0)
@@ -626,6 +734,179 @@ class TestAccountHealthSummary:
        assert "media_count" in health


+class TestSecurityHardening:
+    """Regression tests for the security / hardening fixes.
+
+    - StateStore.load() must never hand callers a reference to its internal
+      cache (H-6)
+    - read_json_body() size cap + content-type rejection (H-4)
+    - same-origin enforcement on state-mutating / SSE endpoints (H-4)
+    - auth rate limiter: lockout on failures + cooldown on code requests (H-5)
+    - terminal job statuses include "done" so SSE streams terminate (C-2)
+    """
+
+    def _ws_module(self):
+        import webui_server as ws_module
+        return ws_module
+
+    def _clear_auth_attempts(self):
+        ws = self._ws_module()
+        with ws._auth_attempts_lock:
+            ws._auth_attempts.clear()
+
+    def test_state_store_load_returns_independent_copies(self):
+        from app_state import StateStore
+
+        defaults = {"accounts": [], "nested": {"x": 1}}
+        state_path = TEST_DATA / "indep-copy" / "state.json"
+        state_path.parent.mkdir(parents=True, exist_ok=True)
+        if state_path.exists():
+            state_path.unlink()
+
+        # File-not-exists path -> defaults; mutating the first result must not
+        # poison the second load() within the TTL window.
+        store = StateStore(state_path, defaults=defaults)
+        first = store.load()
+        first["accounts"].append("mutated")
+        first["nested"]["x"] = 999
+        assert store.load()["accounts"] == []
+        assert store.load()["nested"]["x"] == 1
+
+        # Merge path (real file); use a fresh instance to bypass the TTL cache.
+        state_path.write_text(
+            '{"accounts": ["a"], "nested": {"x": 2, "y": 3}}', encoding="utf-8"
+        )
+        store = StateStore(state_path, defaults=defaults)
+        third = store.load()
+        assert third["accounts"] == ["a"]
+        third["nested"]["y"] = 999
+        reloaded = store.load()
+        assert reloaded["accounts"] == ["a"]
+        assert reloaded["nested"] == {"x": 2, "y": 3}
+
+        # Invalid JSON -> defaults fallback must also return an independent copy.
+        state_path.write_text("{not valid json", encoding="utf-8")
+        store = StateStore(state_path, defaults=defaults)
+        fallback = store.load()
+        fallback["nested"]["x"] = 500
+        reloaded = store.load()
+        assert reloaded["nested"]["x"] == 1
+
+    def test_read_json_body_rejects_non_json_and_oversized(self):
+        ws = self._ws_module()
+
+        # non-JSON Content-Type -> sentinel (do_POST maps it to 415)
+        handler = _make_ws_handler(
+            headers={"Content-Type": "text/plain"}, body=b"hello"
+        )
+        assert handler.read_json_body() is ws._JSON_CONTENT_TYPE_REJECTED
+
+        # declared Content-Length over the cap -> sentinel (do_POST maps to 413)
+        oversized = str(ws.MAX_JSON_BODY_BYTES + 1)
+        handler = _make_ws_handler(
+            headers={"Content-Length": oversized, "Content-Type": "application/json"},
+            body=b"{}",
+        )
+        assert handler.read_json_body() is ws._JSON_BODY_TOO_LARGE
+
+        # valid application/json body parses normally
+        payload = b'{"a": 1}'
+        handler = _make_ws_handler(
+            headers={"Content-Type": "application/json; charset=utf-8"},
+            body=payload,
+        )
+        assert handler.read_json_body() == {"a": 1}
+
+        # unparseable Content-Length -> treated as no body (no 500)
+        handler = _make_ws_handler(
+            headers={"Content-Length": "garbage", "Content-Type": "application/json"},
+            body=b"{}",
+        )
+        assert handler.read_json_body() == {}
+
+    def test_check_same_origin_rejects_cross_origin(self):
+        ws = self._ws_module()
+
+        # cross-origin Origin header -> rejected (403 response sent)
+        h = _make_ws_handler(
+            headers={"Host": "localhost:8080", "Origin": "http://evil.example"}
+        )
+        assert h._check_same_origin() is False
+        h.send_error_json.assert_called_once()
+
+        # Sec-Fetch-Site: cross-site -> rejected
+        h2 = _make_ws_handler(
+            headers={"Host": "localhost:8080", "Sec-Fetch-Site": "cross-site"}
+        )
+        assert h2._check_same_origin() is False
+        h2.send_error_json.assert_called_once()
+
+        # same-origin Origin + Sec-Fetch-Site -> allowed
+        h3 = _make_ws_handler(
+            headers={
+                "Host": "localhost:8080",
+                "Origin": "http://localhost:8080",
+                "Sec-Fetch-Site": "same-origin",
+            }
+        )
+        assert h3._check_same_origin() is True
+
+        # no Origin / Sec-Fetch-Site headers (curl, same-origin) -> allowed
+        h4 = _make_ws_handler(headers={"Host": "localhost:8080"})
+        assert h4._check_same_origin() is True
+
+    def test_auth_rate_limiter_lockout_and_code_cooldown(self):
+        ws = self._ws_module()
+        self._clear_auth_attempts()
+        try:
+            # Lockout after AUTH_MAX_FAILED_ATTEMPTS failures.
+            ip, acc = "10.0.0.5", "acc1"
+            for _ in range(ws.AUTH_MAX_FAILED_ATTEMPTS):
+                ws._record_auth_failure(ip, acc)
+            assert ws._check_auth_throttle(ip, acc) is False
+
+            # Lockout expires -> attempts allowed again.
+            with ws._auth_attempts_lock:
+                ws._auth_attempts[(ip, acc)]["locked_until"] = time.time() - 1
+            assert ws._check_auth_throttle(ip, acc) is True
+
+            # A successful code request starts a cooldown for (ip, account).
+            ip2 = "10.0.0.6"
+            ws._record_auth_code_request(ip2, acc)
+            assert ws._check_auth_code_cooldown(ip2, acc) is False
+            # Different client IP is unaffected.
+            assert ws._check_auth_code_cooldown("10.0.0.7", acc) is True
+
+            # Cooldown expires -> allowed again.
+            with ws._auth_attempts_lock:
+                ws._auth_attempts[(ip2, acc)]["cooldown_until"] = time.time() - 1
+            assert ws._check_auth_code_cooldown(ip2, acc) is True
+        finally:
+            self._clear_auth_attempts()
+
+    def test_auth_attempts_sweep_evicts_expired_entries(self):
+        ws = self._ws_module()
+        self._clear_auth_attempts()
+        try:
+            with ws._auth_attempts_lock:
+                # Grow past the cap with only already-expired lockouts.
+                for i in range(ws.AUTH_ATTEMPTS_MAX_ENTRIES + 2):
+                    ws._auth_attempts[("10.99.0.1", str(i))] = {
+                        "failures": 0,
+                        "locked_until": time.time() - 30,
+                        "cooldown_until": None,
+                    }
+            assert len(ws._auth_attempts) > ws.AUTH_ATTEMPTS_MAX_ENTRIES
+            ws._check_auth_throttle("10.99.0.1", "0")  # triggers the sweep
+            assert len(ws._auth_attempts) < ws.AUTH_ATTEMPTS_MAX_ENTRIES
+        finally:
+            self._clear_auth_attempts()
+
+    def test_terminal_job_statuses_include_done(self):
+        ws = self._ws_module()
+        assert "done" in ws.TERMINAL_JOB_STATUSES
+
+
 # ── Cleanup all temp data ──────────────────────────────────────────────────