Reader: featured image selection with site-chrome suppression #36
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-33-featured-image"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements the agreed design from #33: card images are chosen once at ingest instead of whichever media row was stored first.
entry) → page og:image (og, captured during full-text ingest via trafilatura metadata) → in-content images from stored markdown (content). URLs sanitized (fixes the embedded-newline extraction bug), SVGs skipped.rss.png, and sponsor banners.articles.featured_media_id(alembic) and a newdownload_featured_imagewires the previously-unreachable media downloader to fetch exactly the chosen image; the reader serves the local file with remote fallback.Closes #33
WuMing
Found 5 issue(s). See inline comments below.
@ -0,0 +19,4 @@def upgrade() -> None:"""Add the featured media reference to articles."""op.execute("ALTER TABLE articles ADD COLUMN featured_media_id INTEGER REFERENCES media(id)")code [HIGH]
This migration adds the column only for existing databases. The direct schema-creation path used by tests/new installs (e.g. init_db/schema.sql) also needs the featured_media_id column; otherwise the new SELECT/UPDATE statements fail with 'no such column: featured_media_id' on fresh databases.
@ -125,0 +133,4 @@"""cursor = conn.cursor()cursor.execute("UPDATE articles SET featured_media_id = ? WHERE id = ?", (media_id, article_id))code [MEDIUM]
set_featured_media() stores any media_id without verifying that the media row exists and belongs to the same article. A mismatch would let featured_media_id point to another article's image, causing the reader to display the wrong image. Add a check that media.article_id == article_id before updating (or use an UPDATE ... WHERE EXISTS ... and assert rowcount).
@ -68,32 +70,41 @@ def fetch_full_text(url: str) -> str | None:logger.info(f"Full-text fetch skipped for {url}: non-HTML content type {content_type!r}")security [LOW]
A09: User-controlled article URL is logged without sanitization. A malicious feed can include CRLF in the URL to inject forged log lines. Sanitize control characters or use structured logging.
@ -100,0 +107,4 @@)download_featured_image(conn, article.id)except Exception:logger.error(f"Featured image selection failed for {article.url}", exc_info=True)security [LOW]
A09: User-controlled article URL is logged without sanitization. A malicious feed can include CRLF in the URL to inject forged log lines. Sanitize control characters or use structured logging.
@ -348,0 +380,4 @@media_dir = _media_directory(conn)media_dir.mkdir(parents=True, exist_ok=True)filename = _download_one(record, media_dir, max_bytes)code [MEDIUM]
download_featured_image() does not catch exceptions from _download_one() or the preceding mkdir. Since the function is a public bool-returning API, transient network or filesystem errors will propagate instead of returning False, and partial files may be left behind. Wrap the download/filesystem operations in try/except, log, clean up partial output, and return False on failure.
✅ Fixed — featured media assignment is now validated
src/yunjin/db/articles.pyset_featured_medianow uses anUPDATE … WHERE EXISTS (SELECT 1 FROM media WHERE id = ? AND article_id = ?)guard, returnsbool, and has tests for both the cross-article and non-existent-media rejection paths. Fixed in commit676f6e2.✅ Fixed — feed-supplied URLs are sanitized before logging
src/yunjin/services/full_text.py·src/yunjin/services/full_text.pyLegitimate log-injection vector. All URL interpolations in the ingest path now pass through a
_log_safehelper (control characters → spaces), with unit tests. Fixed in commit676f6e2.🔴 Not actioned — fresh installs do get the column
alembic/versions/20260913_add_article_featured_media.pyFactually incorrect: there is no
schema.sql—init_dbcreates fresh databases through the Alembic migration chain (its docstring: "Creates a new database with migrations"), so the migration is the single schema source for new and existing databases alike. The PR's own schema test (test_articles_table_gains_featured_media_column) asserts the column exists on a freshly created database viainit_dband passes, as does the full CI suite.🔴 Not actioned — exception propagation matches the established downloader contract
src/yunjin/services/media_downloader.pyDeliberate: the sole caller (the ingest orchestrator in
full_text.py) wraps the call in per-articletry/exceptwith logging — failure tolerance is preserved end-to-end — and the siblingdownload_article_mediaAPI propagates identically. There is no partial-file window:_download_onewrites bytes only after the response is fully read. Diverging from the sibling API's contract inside this PR would be inconsistency, not hardening.WuMing
Found 4 issue(s). See inline comments below.
@ -125,0 +153,4 @@)conn.commit()return cursor.rowcount > 0code [LOW]
cursor.rowcount reports rows actually modified, not rows matched by the UPDATE. If the article already has the same featured_media_id, this returns False even though the reference is stored. Compare the existing value first or return True when the current value already equals media_id.
@ -61,3 +82,3 @@response = _fetch_feed_response(url)except FetchError as exc:logger.warning(f"Full-text fetch skipped for {url}: {exc}")logger.warning(f"Full-text fetch skipped for {_log_safe(url)}: {exc}")security [MEDIUM]
The URL is sanitised with _log_safe, but the exception object {exc} is logged without sanitisation. FetchError is raised from URL validation/fetch paths and may include the same feed-controlled URL, so CRLF sequences can still inject forged log lines. Sanitise the entire log message or ensure exception text is sanitized before logging (A09).
@ -348,0 +373,4 @@if article is None or article.featured_media_id is None:return Falserecord = media_db.get_media(conn, article.featured_media_id)code [LOW]
The media record is fetched by featured_media_id only, without verifying that record.article_id matches article.id. A corrupt or manually-set featured_media_id could download and record another article's media. Add an ownership check such as: if record.article_id != article.id: return False.
@ -198,0 +209,4 @@assert _log_safe("https://x.example/a\r\nFAKE ERROR b") == "https://x.example/a FAKE ERROR b"def test_log_safe_keeps_normal_urls(self) -> None:assert _log_safe("https://x.example/a?b=1&c=2") == "https://x.example/a?b=1&c=2"tests [LOW]
This assertion has the identical string literal on both sides, so it only exercises the no-op path of
_log_safeand reads as tautological. Keep it if the intent is to pin the identity case, but consider asserting against a named constant, or extend it to inputs that could plausibly be mangled (e.g. leading/trailing control chars, unicode, embedded tabs) so the test can catch a regression in the stripping pattern rather than restating the input.WuMing
Found 2 issue(s). See inline comments below.
@ -138,3 +138,3 @@def _fetch_feed_response(url: str) -> httpx2.Response:def _fetch_feed_response(url: str, headers: dict[str, str] | None = None) -> httpx2.Response:tests [LOW]
The new
headersparameter of_fetch_feed_responseis never exercised by a test: the only new test that touches it (tests/test_full_text.py::test_browser_headers_are_sent) mocks_fetch_feed_responseout entirely, so the non-default branch (headers if headers is not None else {...}) has no coverage. Add a unit test that calls_fetch_feed_responsewith an explicit header dict and asserts the mocked httpx2.get/redirect hops receive those headers (and that the default User-Agent is still used when headers is omitted).@ -96,3 +99,3 @@class TestFetchAndStoreFullText:def test_stores_extracted_markdown_and_timestamp(self, db_conn_with_article, db_conn: sqlite3.Connection) -> None:with patch.object(full_text, "fetch_full_text", return_value="# Wind project approved\n\nBody text."):with patch.object(full_text, "fetch_article_page", return_value=ARTICLE_HTML):tests [MEDIUM]
fetch_and_store_full_textnow also callsfeatured_image_service.choose_and_store_featured_imageandmedia_downloader.download_featured_image, but these tests only patchfetch_article_page, leaving the new collaborators unmocked. The tests therefore run real image selection and (when a candidate is found) perform a realhttpx2.streamdownload and write media rows/files into the fixture database, making them non-hermetic and slow/flaky. Patchfeatured_image_service.choose_and_store_featured_imageandfull_text.download_featured_image(or an autouse fixture) in these tests and assert the call/no-call where relevant.