Download and store article images locally #29
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-21-image-persistence"
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 #21: article images are downloaded at fetch time and served by yunjin itself, so the reader no longer hotlinks origins (which may 403 default clients, die, or rate-limit — see #2).
Storage
media.local_filename+media.downloaded_atcolumns (alembic migration); NULL means not downloadedmedia/directory beside the database file, named by SHA-256 of the URL (deterministic, traversal-safe), extension from the URL path or content typeDownload pipeline
media_downloaderservice, wired into both media-creation paths (feed-entry extraction and full-article extraction)YUNJIN_HTTP_USER_AGENToverride — the #2 lesson), redirects followed, image content-type enforced, 20 MB per-image cap, failures tolerated per image (remote URL stays as fallback)Serving
GET /media/<filename>route (send_from_directory, 1-year cache — names are URL-stable)Also included: the previously lost test for
get_next_aggregate_with_unread(missed in PR #28 staging), with itsuser_idFK fix.247 tests, all 9 nox sessions green.
Closes #21
WuMing
Found 5 issue(s). See inline comments below.
@ -0,0 +33,4 @@"image/gif": ".gif","image/webp": ".webp","image/avif": ".avif","image/svg+xml": ".svg",security [HIGH]
A03: SVG files are allowed and stored unmodified, then served by the media route as
image/svg+xml. SVG can contain scripts, so a malicious feed can create stored XSS if a user opens the local media URL. Disallow SVG, sanitize it, or force download/attachment with a strong CSP.@ -0,0 +121,4 @@downloaded = 0for record in records:try:response = httpx2.get(security [HIGH]
A10: The downloader fetches
record.urlfrom untrusted feed metadata without validating the URL scheme, host, or resolved IP. An attacker-controlled feed can cause server-side GET requests to internal services, localhost, or cloud metadata endpoints (e.g., 169.254.169.254). Validate http/https, use an allow-list, and block redirects to private/loopback/link-local IP addresses.@ -0,0 +140,4 @@logger.debug("Media download for %s is not an image (%s)", record.url, content_type)continuedata = response.contentsecurity [MEDIUM]
A04: The 20 MB per-image cap is enforced only after
response.contenthas buffered the entire response body. A malicious or compromised image URL can cause excessive memory consumption despite the cap. Stream the response and abort when Content-Length or downloaded bytes exceed max_bytes.@ -0,0 +10,4 @@bp = Blueprint("media", __name__, url_prefix="/media")@bp.route("/<path:filename>")security [MEDIUM]
A01: The media serving route is not protected by any authentication or authorization check. Stored article images are served to anyone who knows the hash-based filename, even if reader content is private. Apply the same login/authorization controls used by reader and feed routes.
@ -0,0 +200,4 @@url="https://example.com/done.png",media_type="image/png",)media_db.set_media_local_file(db_conn, media_id=1, local_filename="done.png")tests [LOW]
The test discards the object returned by
media_db.create_media(...)and instead hardcodesmedia_id=1, relying on SQLite AUTOINCREMENT starting at 1 for this fresh temp DB. This is brittle: any change to fixture setup, prior inserts, or ID assignment silently makes the test mark the wrong record as downloaded (or no record at all), soassert_not_called()would pass for the wrong reason. Capture the created media (media = media_db.create_media(...)) and passmedia_id=media.id.RenewEconomy images: extraction fix
Found while reviewing #21 against a real feed: RenewEconomy articles never had images because
extract_media_from_feed_entryonly readmedia_content/media_thumbnail/enclosures— and RenewEconomy's WordPress feed carries its featured image as an ordinary<img>inside the entry summary (verified live: the summary contains the<img>tag, but extraction returned nothing). The stored article content had the same tags — the extractor just never looked.extract_media_from_feed_entrynow also scans the entry summary and content value with the existing_extract_img_tagshelper (media-namespace and enclosure extraction unchanged)WuMing
Found 4 issue(s). See inline comments below.
@ -0,0 +35,4 @@"image/avif": ".avif","image/svg+xml": ".svg",}_URL_SUFFIXES = {".jpg", ".jpeg", ".png", ".gif", ".webp", ".avif", ".svg"}security [HIGH]
A03: Stored XSS via SVG. Allowing
.svgas a URL-derived extension means an attacker-controlled feed URL ending in.svg(returning anyimage/*content type) is stored locally and served inline by/media/<filename>asimage/svg+xmlon the application origin. A malicious SVG can execute script when opened directly, leading to session/account compromise. Disallow SVG or serve it withContent-Disposition: attachmentand a sandboxing CSP.@ -0,0 +121,4 @@downloaded = 0for record in records:try:response = httpx2.get(security [HIGH]
A10: SSRF via user-controlled media URLs.
record.urloriginates from untrusted feed content and is fetched without allow-list validation, scheme restriction, or blocking of private/link-local/metadata targets. Redirects are followed, so an attacker-controlled feed can make the server request internal services or cloud metadata endpoints. Restrict to http/https, validate hosts, block private and reserved IP ranges, and revalidate redirect targets.@ -0,0 +66,4 @@assert downloaded == 1mock_get.assert_called_once()assert mock_get.call_args.kwargs["headers"]["User-Agent"].startswith("Mozilla/5.0")tests [LOW]
This assertion pins the default User-Agent, but
_user_agent()readsYUNJIN_HTTP_USER_AGENTfrom the environment at call time. If that variable is set in the developer's shell or CI (which is exactly what the new override is for), the test fails for an unrelated reason. Isolate the ambient environment (e.g.monkeypatch.delenv("YUNJIN_HTTP_USER_AGENT", raising=False)) in the test, or add a companion test that sets it and asserts the override is used.@ -0,0 +200,4 @@url="https://example.com/done.png",media_type="image/png",)media_db.set_media_local_file(db_conn, media_id=1, local_filename="done.png")tests [LOW]
This test hardcodes
media_id=1instead of using the id of the record returned bymedia_db.create_media(...). The test only passes because the media row happens to be the first one in a fresh database; any change to fixture/setup ordering (e.g. an extra media row created earlier) would make the UPDATE target a non-existent row and the assertion would fail for the wrong reason. Capture the created media and passmedia_id=media.id, matching the pattern used in tests/test_db.py.src/yunjin/services/media_downloader.py✅ Fixed —
_is_safe_media_url()mirrors the feed-fetch guard: http/https only, host resolved and rejected when any address is private/loopback/link-local/reserved/unspecified/multicast, unresolvable hosts fail closed, with the existingYUNJIN_ALLOW_PRIVATE_FEED_HOSTSopt-out for LAN media sources. Tests cover non-http schemes, private literals, unresolvable hosts, and the opt-out. Known limitation documented in the module docstring (TOCTOU vs the client's own resolution; redirects not re-validated) — same accepted posture as the feed guard.src/yunjin/services/media_downloader.py✅ Fixed — storage is now a whitelist of raster types (jpeg/png/gif/webp/avif);
image/svg+xmland.svgURLs are never stored, so no served SVG can carry script. Test asserts an SVG response is skipped and nothing is written.src/yunjin/services/media_downloader.py✅ Fixed — downloads now use
httpx2.stream(): the declaredContent-Lengthis checked before reading, and the body is consumed in 64 KB chunks with the cap enforced during the read (aborting mid-stream). Tests cover both the declared-length rejection (asserting the body is never read) and the mid-stream abort.src/yunjin/web/routes/media.py🔴 Not actioned in this PR — there is no authentication layer anywhere in the app; reader, feeds and settings routes are equally open. Session-derived auth is tracked in #25 (Phase 6), which will apply the same controls project-wide.
tests/test_media_downloader.py✅ Fixed — tests now keep the returned record and assert against its id (
stored.id == record.id).tests/test_media_downloader.py✅ Fixed — the test clears the variable with
monkeypatch.delenv(..., raising=False)for the default assertion and sets it for the override assertion.Duplicates of wave-260 findings (all already fixed)
SSRF·SVG XSS·hardcoded media_idIdentical to the findings answered above: SSRF guard (
_is_safe_media_url+ private-range blocking + opt-out), SVG excluded from the raster whitelist, tests using the record returned bycreate_media. No further action.WuMing
Found 6 issue(s). See inline comments below.
@ -241,0 +295,4 @@"""SELECT id, article_id, url, media_type, created_at, local_filename, downloaded_atFROM mediaWHERE article_id = ? AND local_filename IS NULLcode [MEDIUM]
This query selects all undownloaded media for an article, including non-image enclosures such as audio or video. The downloader only stores raster image content types, so every such item will still be fetched remotely (up to the size cap) just to be rejected. Filter to image media or media with a NULL/unknown type, e.g.
AND (media_type IS NULL OR media_type LIKE 'image/%').@ -0,0 +31,4 @@from pathlib import Pathfrom urllib.parse import urlsplitimport httpx2code [CRITICAL]
This imports a module named
httpx2, but the standard HTTP client library ishttpx. Unless the project intentionally depends on a custom or vendoredhttpx2package, this import will fail and the media downloader will be completely broken. Change toimport httpxand update allhttpx2.*references accordingly.@ -0,0 +162,4 @@Local file name."""suffix = Path(urlsplit(url).path).suffix.lower()code [MEDIUM]
The local filename extension is taken from the URL path first, even though the response
Content-Typeis known and validated. A URL ending in.jpgthat actually servesimage/pngwill be stored with a.jpgextension, andsend_from_directorywill serve it withimage/jpegcontent-type, causing broken or misleading image responses. Prefer the validated response Content-Type extension consistently, or verify that the URL suffix matches the actual content type.@ -0,0 +185,4 @@"GET",record.url,timeout=_TIMEOUT_SECONDS,follow_redirects=True,code [HIGH]
Redirects are followed without re-validating the redirect target. The original URL is checked by
_is_safe_media_url, but a public URL can redirect to a private, loopback, or metadata-service address and bypass the SSRF guard. Revalidate every redirect target with the same host/address checks or disable automatic redirects and implement manual, validated redirect handling.security [HIGH]
The initial media URL is validated by _is_safe_media_url, but the HTTP client is configured with follow_redirects=True and does not re-validate redirect destinations. A malicious public URL can redirect to internal addresses (e.g., 169.254.169.254, localhost, internal services) and bypass the SSRF guard. Remediate by disabling automatic redirects and manually validating each redirect hop with the same scheme/private-address checks before following it.
@ -0,0 +198,4 @@return Nonedeclared_length = response.headers.get("content-length")if declared_length and int(declared_length) > max_bytes:code [HIGH]
Content-Lengthis attacker-controlled. If it is missingint()is skipped, but if it is malformed/non-integer or duplicated into an unexpected string,int(declared_length)raisesValueError, which is not caught by the existingexcept httpx2.HTTPError. A single bad header can abort the whole article download job. Validate and parse the header defensively, or catchValueErrorand treat it as a rejected response.src/yunjin/services/media_downloader.py🔴 Factually incorrect —
httpx2is pydantic's fork of httpx and the project's deliberate HTTP client: AGENTS.md mandates it ("use httpx2 … never requests") and it has been the direct dependency since PR #15, used by fetcher, media_extractor, and llm_aggregator. The import cannot fail;httpxis not a dependency at all. No change.src/yunjin/services/media_downloader.py✅ Fixed — downloads now follow redirects manually (
follow_redirects=False) with a 5-hop limit, re-running_is_safe_media_url(scheme + resolved-address checks) on every hop before any request is made — mirroring the feed-fetch guard from PR #28. Tests: a redirect to169.254.169.254is blocked before the second request (asserting a single stream call), and a redirect to a public CDN is followed and stored.src/yunjin/services/media_downloader.py✅ Fixed — same change as the duplicate finding above: manual redirect handling with per-hop re-validation.
src/yunjin/services/media_downloader.py✅ Fixed — new
_declared_length_exceeds()parses the header defensively: absent or unparseable values return False and fall back to the streaming cap (which still applies during the chunked read). Test covers a junkcontent-length: abcheader with a body above the cap — no exception, nothing stored.src/yunjin/services/media_downloader.py✅ Fixed — the extension now comes solely from the validated response content type, so a
.jpgURL servingimage/pngis stored and served as.png. Test asserts the content type wins over a misleading URL suffix.src/yunjin/db/media.py✅ Fixed —
get_undownloaded_media_for_articlenow filters to image media (media_type IS NULL OR media_type LIKE 'image/%'), matchingget_first_image. Test asserts a video row is not queued while an image row is.