Reader: featured image selection with site-chrome suppression #36

Manually merged
marvin8 merged 7 commits from feat/issue-33-featured-image into main 2026-09-13 02:01:00 +00:00
Collaborator

Implements the agreed design from #33: card images are chosen once at ingest instead of whichever media row was stored first.

  • Ranked candidates: feed entry media (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.
  • Resize-variant collapse: candidates dedupe on query-stripped base URL, keeping the largest declared dimensions (40 ABC rows collapse to 7 real images; no perceptual hashing needed).
  • Site-chrome suppression: candidates whose base URL appears on ≥ 3 same-feed articles are skipped — verified against the live db to remove the onestepoffthegrid logos, rss.png, and sponsor banners.
  • Persisted + downloaded once: articles.featured_media_id (alembic) and a new download_featured_image wires the previously-unreachable media downloader to fetch exactly the chosen image; the reader serves the local file with remote fallback.
  • Existing articles keep the legacy first-image behaviour.

Closes #33

Implements the agreed design from #33: card images are chosen once at ingest instead of whichever media row was stored first. - **Ranked candidates**: feed entry media (`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. - **Resize-variant collapse**: candidates dedupe on query-stripped base URL, keeping the largest declared dimensions (40 ABC rows collapse to 7 real images; no perceptual hashing needed). - **Site-chrome suppression**: candidates whose base URL appears on ≥ 3 same-feed articles are skipped — verified against the live db to remove the onestepoffthegrid logos, `rss.png`, and sponsor banners. - **Persisted + downloaded once**: `articles.featured_media_id` (alembic) and a new `download_featured_image` wires the previously-unreachable media downloader to fetch exactly the chosen image; the reader serves the local file with remote fallback. - Existing articles keep the legacy first-image behaviour. Closes #33
Serve the featured image on reader cards
All checks were successful
/ gitleaks (pull_request) Successful in 20s
/ checks (pull_request) Successful in 1m57s
/ pr-review (pull_request) Successful in 3m3s
4878e82fc7
forgejo-actions left a comment

WuMing

Found 5 issue(s). See inline comments below.

## 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.

**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. <!-- wuming:sha256:a5d350ace589b5b369598131e46f5b7ddd180e99d7415a4e017c0f8d49a10026 -->
marvin8 marked this conversation as resolved
@ -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).

**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). <!-- wuming:sha256:f8a08989205a7ac1455ad771a5b38d5b166e3aa7bb096f483ea773ed20437b9f -->
@ -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.

**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. <!-- wuming:sha256:6e442363a237ed7f6038e453e4609ee2326f171484a4c9b7acd953a1f3e6d1a8 -->
@ -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.

**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. <!-- wuming:sha256:5ea01c0a21f5dbda75a8908cbdb3a01ba3cb78ec82f4dd1504bb2bd7f44d8260 -->
@ -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.

**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. <!-- wuming:sha256:661d71fee83bc2fb828eaa8adda572536c5aaddb3d044d7ced91328c3cd5bac5 -->
marvin8 marked this conversation as resolved
🔒 Guard featured media assignment and sanitize ingest logs
All checks were successful
/ gitleaks (pull_request) Successful in 24s
/ pr-review (pull_request) Successful in 2m9s
/ checks (pull_request) Successful in 2m40s
676f6e2922
Author
Collaborator

src/yunjin/db/articles.py

set_featured_media() stores any media_id without verifying that the media row exists and belongs to the same article…

set_featured_media now uses an UPDATE … WHERE EXISTS (SELECT 1 FROM media WHERE id = ? AND article_id = ?) guard, returns bool, and has tests for both the cross-article and non-existent-media rejection paths. Fixed in commit 676f6e2.


Fixed — feed-supplied URLs are sanitized before logging

src/yunjin/services/full_text.py · src/yunjin/services/full_text.py

A09: User-controlled article URL is logged without sanitization. A malicious feed can include CRLF in the URL…

Legitimate log-injection vector. All URL interpolations in the ingest path now pass through a _log_safe helper (control characters → spaces), with unit tests. Fixed in commit 676f6e2.


🔴 Not actioned — fresh installs do get the column

alembic/versions/20260913_add_article_featured_media.py

The direct schema-creation path used by tests/new installs (e.g. init_db/schema.sql) also needs the featured_media_id column…

Factually incorrect: there is no schema.sqlinit_db creates 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 via init_db and passes, as does the full CI suite.


🔴 Not actioned — exception propagation matches the established downloader contract

src/yunjin/services/media_downloader.py

download_featured_image() does not catch exceptions from _download_one()… transient network or filesystem errors will propagate instead of returning False…

Deliberate: the sole caller (the ingest orchestrator in full_text.py) wraps the call in per-article try/except with logging — failure tolerance is preserved end-to-end — and the sibling download_article_media API propagates identically. There is no partial-file window: _download_one writes bytes only after the response is fully read. Diverging from the sibling API's contract inside this PR would be inconsistency, not hardening.

### ✅ Fixed — featured media assignment is now validated [`src/yunjin/db/articles.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/36#issuecomment-2977) > set_featured_media() stores any media_id without verifying that the media row exists and belongs to the same article… `set_featured_media` now uses an `UPDATE … WHERE EXISTS (SELECT 1 FROM media WHERE id = ? AND article_id = ?)` guard, returns `bool`, and has tests for both the cross-article and non-existent-media rejection paths. Fixed in commit `676f6e2`. --- ### ✅ Fixed — feed-supplied URLs are sanitized before logging [`src/yunjin/services/full_text.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/36#issuecomment-2979) · [`src/yunjin/services/full_text.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/36#issuecomment-2980) > A09: User-controlled article URL is logged without sanitization. A malicious feed can include CRLF in the URL… Legitimate log-injection vector. All URL interpolations in the ingest path now pass through a `_log_safe` helper (control characters → spaces), with unit tests. Fixed in commit `676f6e2`. --- ### 🔴 Not actioned — fresh installs do get the column [`alembic/versions/20260913_add_article_featured_media.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/36#issuecomment-2976) > The direct schema-creation path used by tests/new installs (e.g. init_db/schema.sql) also needs the featured_media_id column… Factually incorrect: there is no `schema.sql` — `init_db` creates 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 via `init_db` and passes, as does the full CI suite. --- ### 🔴 Not actioned — exception propagation matches the established downloader contract [`src/yunjin/services/media_downloader.py`](https://forge.marvin8.zone/marvin8/yunjin/pulls/36#issuecomment-2978) > download_featured_image() does not catch exceptions from _download_one()… transient network or filesystem errors will propagate instead of returning False… Deliberate: the sole caller (the ingest orchestrator in `full_text.py`) wraps the call in per-article `try/except` with logging — failure tolerance is preserved end-to-end — and the sibling `download_article_media` API propagates identically. There is no partial-file window: `_download_one` writes bytes only after the response is fully read. Diverging from the sibling API's contract inside this PR would be inconsistency, not hardening.
forgejo-actions left a comment

WuMing

Found 4 issue(s). See inline comments below.

## WuMing Found **4** issue(s). See inline comments below.
@ -125,0 +153,4 @@
)
conn.commit()
return cursor.rowcount > 0

code [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.

**code** [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. <!-- wuming:sha256:bf081bd114ec79e8698c5a1cd92f224ba70e11ca4bb3579b22f24057d494d4e3 -->
marvin8 marked this conversation as resolved
@ -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).

**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). <!-- wuming:sha256:92c552adb87a02cf99756544abeb79b3a558f85db982017efb672a0a2763afe5 -->
marvin8 marked this conversation as resolved
@ -348,0 +373,4 @@
if article is None or article.featured_media_id is None:
return False
record = 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.

**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. <!-- wuming:sha256:de07780f9c81885e378293463fdaaea68a0c98b24f0328a2bc80330ceb124e90 -->
marvin8 marked this conversation as resolved
@ -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_safe and 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.

**tests** [LOW] This assertion has the identical string literal on both sides, so it only exercises the no-op path of `_log_safe` and 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:sha256:4f68bd9cc4f1c3dc5c88dd65aceed1246e58c24a71dc12a15676508ced92de9d -->
🐛 Resolve the media directory absolutely when serving
All checks were successful
/ gitleaks (pull_request) Successful in 21s
/ checks (pull_request) Successful in 1m52s
/ pr-review (pull_request) Successful in 2m55s
7d4759bbbb
🐛 Send browser headers and reject error pages in article fetches
All checks were successful
/ gitleaks (pull_request) Successful in 21s
/ checks (pull_request) Successful in 1m50s
/ pr-review (pull_request) Successful in 2m54s
37ba95dd44
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## 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 headers parameter of _fetch_feed_response is 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_response out entirely, so the non-default branch (headers if headers is not None else {...}) has no coverage. Add a unit test that calls _fetch_feed_response with 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).

**tests** [LOW] The new `headers` parameter of `_fetch_feed_response` is 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_response` out entirely, so the non-default branch (`headers if headers is not None else {...}`) has no coverage. Add a unit test that calls `_fetch_feed_response` with 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). <!-- wuming:sha256:fc409a975d2f92d86bbaaf7cde7f5bfa2d85db6cacbc91c570b061661bf97672 -->
marvin8 marked this conversation as resolved
@ -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_text now also calls featured_image_service.choose_and_store_featured_image and media_downloader.download_featured_image, but these tests only patch fetch_article_page, leaving the new collaborators unmocked. The tests therefore run real image selection and (when a candidate is found) perform a real httpx2.stream download and write media rows/files into the fixture database, making them non-hermetic and slow/flaky. Patch featured_image_service.choose_and_store_featured_image and full_text.download_featured_image (or an autouse fixture) in these tests and assert the call/no-call where relevant.

**tests** [MEDIUM] `fetch_and_store_full_text` now also calls `featured_image_service.choose_and_store_featured_image` and `media_downloader.download_featured_image`, but these tests only patch `fetch_article_page`, leaving the new collaborators unmocked. The tests therefore run real image selection and (when a candidate is found) perform a real `httpx2.stream` download and write media rows/files into the fixture database, making them non-hermetic and slow/flaky. Patch `featured_image_service.choose_and_store_featured_image` and `full_text.download_featured_image` (or an autouse fixture) in these tests and assert the call/no-call where relevant. <!-- wuming:sha256:a341bf089869d433ff8b2bd2f92b3891301fe8d51043b46a2bc020d6c2bcd8f8 -->
🐛 Retry blocked article fetches with the extractor identity
All checks were successful
/ gitleaks (pull_request) Successful in 19s
/ checks (pull_request) Successful in 1m57s
/ pr-review (pull_request) Successful in 2m56s
bca5355645
marvin8 approved these changes 2026-09-13 01:59:37 +00:00
marvin8 manually merged commit b5980fdad7 into main 2026-09-13 02:01:00 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
marvin8/yunjin!36
No description provided.