diff --git a/post_parser.py b/post_parser.py index 5d9223d..32ca6f1 100644 --- a/post_parser.py +++ b/post_parser.py @@ -526,7 +526,8 @@ class PostParser: sanitize: when True, run the html body and footer through a single bleach pass each. Single-post HTML and JSON need this (there is no whole-feed pass on those paths). Feed generation passes False and - relies on the final whole-feed sanitize in rss_generator, so no + relies on the final sanitize in rss_generator (per-post for RSS, + whole-feed for HTML), so no fragment is sanitized more than once. """ # Compute html body once — avoids triple _generate_html_body calls. @@ -715,8 +716,8 @@ class PostParser: content_body.append(f'
') # NOTE: sanitize is NOT applied here. Sanitization happens exactly once per - # output boundary (process_message for single-post/JSON, final whole-feed pass - # in rss_generator for feeds). See the sanitize coverage map (4.4). + # output boundary (process_message for single-post/JSON; in rss_generator the + # per-post pass for RSS and the whole-feed pass for HTML). See the map (4.4). html_body = '\n'.join(content_body) return html_body @@ -879,7 +880,7 @@ class PostParser: content_footer.append('
' + flags_html) # Not sanitized here — sanitized once at the output boundary (4.4 coverage map): - # process_message for single-post/JSON, final whole-feed pass for feeds. + # process_message for single-post/JSON; per-post (RSS) / whole-feed (HTML) for feeds. html_footer = '\n'.join(content_footer) return html_footer diff --git a/rss_generator.py b/rss_generator.py index c2af83b..0c6f7ea 100644 --- a/rss_generator.py +++ b/rss_generator.py @@ -14,6 +14,7 @@ import logging import asyncio import re import time +from html import escape as html_escape from datetime import datetime, timezone from types import SimpleNamespace from typing import Optional @@ -23,7 +24,6 @@ from pyrogram.types import Message from post_parser import PostParser from config import get_settings from tg_throttle import tg_rpc_bounded -from file_io import upsert_media_file_ids_bulk_sync, DB_PATH from bleach.css_sanitizer import CSSSanitizer from bleach import clean as HTMLSanitizer @@ -487,8 +487,13 @@ async def generate_channel_rss(channel: str | int, ) sanitized_html = await asyncio.to_thread(_sanitize_sync, post['html']) except Exception as e: + # FAIL CLOSED: since 4.4 removed the per-fragment passes, post['html'] is + # raw channel-controlled HTML, and this is its ONLY sanitize. If bleach + # itself throws (e.g. RecursionError on pathological nested HTML), do NOT + # emit the raw payload (that would be stored XSS) — html.escape it so the + # content survives as inert text. logger.error(f"rss_html_sanitization_error: channel {channel}, message_id {post['message_id']}, error {str(e)}") - sanitized_html = post['html'] + sanitized_html = html_escape(post['html']) fe.content(content=sanitized_html, type='CDATA') if post['date'] is not None: @@ -693,7 +698,11 @@ async def generate_channel_html(channel: str | int, ) html = await asyncio.to_thread(_sanitize_sync, html) except Exception as e: + # FAIL CLOSED (see the RSS path): `html` is now the raw concatenated feed + # (4.4 removed the per-fragment passes). If bleach throws, html.escape the + # whole feed rather than returning raw channel HTML/JS to the client. logger.error(f"html_final_sanitization_error: channel {channel}, error {str(e)}") + html = html_escape(html) html_gen_elapsed = time.time() - html_gen_start_time logger.debug(f"html_generation_timing: channel {channel}, HTML generated in {html_gen_elapsed:.3f} seconds") diff --git a/tests/test_stage4_eventloop.py b/tests/test_stage4_eventloop.py index 1b091a2..c0648ec 100644 --- a/tests/test_stage4_eventloop.py +++ b/tests/test_stage4_eventloop.py @@ -365,3 +365,107 @@ async def test_xss_in_media_caption_stripped_in_feeds(monkeypatch): monkeypatch.setattr("tg_cache.cached_get_chat_history", fake_get_history2, raising=False) html_feed = await generate_channel_html("testchan", client=SimpleNamespace(), limit=5) _assert_clean(html_feed, "html feed (media caption)") + + +# --------------------------------------------------------------------------- # +# Review round-1: the new bulk-upsert SQL executed for real (not mocked). +# --------------------------------------------------------------------------- # +def test_bulk_upsert_media_file_ids_real_sql(tmp_path): + import sqlite3 + from file_io import upsert_media_file_ids_bulk_sync, init_db_sync + + db = str(tmp_path / "t.db") + init_db_sync(db) + + # Empty list is a no-op (no crash). + upsert_media_file_ids_bulk_sync(db, []) + + # Multi-row insert. + upsert_media_file_ids_bulk_sync(db, [ + ("chA", 1, "fidA", 100.0), + ("chB", 2, "fidB", 200.0), + ]) + conn = sqlite3.connect(db) + rows = dict(((c, p, f), a) for c, p, f, a in + conn.execute("SELECT channel, post_id, file_unique_id, added FROM media_file_ids")) + assert rows[("chA", 1, "fidA")] == 100.0 + assert rows[("chB", 2, "fidB")] == 200.0 + + # Re-upsert the SAME key updates `added` (ON CONFLICT ... DO UPDATE SET added=excluded.added). + upsert_media_file_ids_bulk_sync(db, [("chA", 1, "fidA", 999.0)]) + a = conn.execute( + "SELECT added FROM media_file_ids WHERE channel='chA' AND post_id=1 AND file_unique_id='fidA'" + ).fetchone()[0] + assert a == 999.0 + conn.close() + + +# --------------------------------------------------------------------------- # +# Review round-1: media ids collected before a render exception are still +# flushed (the flush is in a finally). Removing the finally must break this. +# --------------------------------------------------------------------------- # +@pytest.mark.asyncio +async def test_pending_media_flushed_on_render_exception(monkeypatch): + async def fake_get_chat(client, channel): + return SimpleNamespace(title="Test", username="testchan", id=-1001234567890) + + async def fake_get_history(client, channel, limit=20): + return [make_message(40, text="hi")] + + monkeypatch.setattr("tg_cache.cached_get_chat", fake_get_chat, raising=False) + monkeypatch.setattr("tg_cache.cached_get_chat_history", fake_get_history, raising=False) + + # A render pipeline that collects a pending id then raises mid-render. + def boom_pipeline(messages, post_parser, *a, **k): + post_parser._pending_media_ids.append(("chZ", 9, "fidZ", 1.0)) + raise RuntimeError("render blew up") + + monkeypatch.setattr(rss_module, "_render_pipeline", boom_pipeline) + + flushed = {} + async def fake_bulk(db, entries): + flushed["entries"] = list(entries) + monkeypatch.setattr("post_parser.upsert_media_file_ids_bulk_sync", + lambda db, entries: flushed.__setitem__("entries", list(entries)), + raising=False) + + with pytest.raises(Exception): + await generate_channel_rss("testchan", client=SimpleNamespace(), limit=5) + + # The collected id was persisted despite the render exception (flush in finally). + assert flushed.get("entries") == [("chZ", 9, "fidZ", 1.0)] + + +# --------------------------------------------------------------------------- # +# Review round-1 [security]: if the ONLY sanitize pass throws, the feed must +# FAIL CLOSED (html.escape the raw content), never emit the raw XSS payload. +# --------------------------------------------------------------------------- # +@pytest.mark.asyncio +async def test_rss_fails_closed_when_sanitizer_raises(monkeypatch): + async def fake_get_chat(client, channel): + return SimpleNamespace(title="Test", username="testchan", id=-1001234567890) + + async def fake_get_history(client, channel, limit=20): + return [make_message(41, text=XSS_PAYLOAD)] + + monkeypatch.setattr("tg_cache.cached_get_chat", fake_get_chat, raising=False) + monkeypatch.setattr("tg_cache.cached_get_chat_history", fake_get_history, raising=False) + + # Force bleach to blow up (e.g. the RecursionError class already seen in prod). + # rss_generator imports it as `from bleach import clean as HTMLSanitizer`. + def boom(*a, **k): + raise RecursionError("bleach exploded") + monkeypatch.setattr(rss_module, "HTMLSanitizer", boom, raising=True) + + rss = await generate_channel_rss("testchan", client=SimpleNamespace(), limit=5) + chunks = re.findall(r"", rss, re.DOTALL) + assert chunks, "expected CDATA content" + for chunk in chunks: + # Fail-closed: the raw payload was html.escaped, so NO live tag survived — every + # `<` became `<`. (The letters "javascript:" still appear, but as inert text + # inside an escaped "…", not a live href.) + assert " reached the feed" + assert " reached the feed" + assert " reached the feed" + # ...and the escaping actually ran (payload present as escaped text, not dropped). + assert "<script>" in chunk, "expected the payload html-escaped, not dropped"