fix: the provenance panel hotlinked the CDN for images already on disk (3965)
CI / lint (push) Successful in 8s
CI / extension-version (push) Successful in 9s
Build images / sign-extension (push) Successful in 9s
Build images / build-agent (push) Successful in 17s
CI / frontend-build (push) Successful in 35s
CI / backend-lint-and-test (push) Successful in 1m18s
Build images / build-web (push) Successful in 1m22s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m19s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m38s
CI / lint (push) Successful in 8s
CI / extension-version (push) Successful in 9s
Build images / sign-extension (push) Successful in 9s
Build images / build-agent (push) Successful in 17s
CI / frontend-build (push) Successful in 35s
CI / backend-lint-and-test (push) Successful in 1m18s
Build images / build-web (push) Successful in 1m22s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 2m19s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m38s
Two surfaces render a post's HTML body. get_post did _localize_inline_images(sanitize_post_html(...)); provenance_service._post_dict called sanitize_post_html alone. So opening the Provenance panel fetched images from Patreon's CDN for files FC had already downloaded - the archive reaching out to the platform to display what it had archived, which is the thing 830 Phase 2 set out to stop. The same bodies also break when a CDN URL expires or a post is removed, while the identical local copy sits unused. The fix is the shape, not the call. Those were two separately-callable halves and only the first looked mandatory, so a second caller was always going to do half of it. render_post_body in the new services/post_body.py is the whole pipeline in one call, and sanitizing-without-localizing is no longer a reachable operation. _localize_inline_images moves there verbatim; post_feed_service loses five imports that went with it. _post_dict becomes async and takes the session. Both call sites are already inside async methods, so for_image's list comprehension awaits per entry - fine, because localization issues ZERO queries for a body with no inline <img>, which is most of them. Recorded that early exit in the module so the loop isn't "optimized" into a batch without a measurement. Deliberately NOT fixed: provenance still names the same columns url/title/date where the feed says post_url/post_title/post_date, and its description_translated is full text where the feed truncates to DESCRIPTION_LIMIT. That is 3965's wider half - a breaking payload change for ProvenancePanel with no second reason to spend it today. Noted in _post_dict's docstring so the next reader knows it was seen and left. Four regression tests on the provenance path, covering both entry points plus the two refusals the feed already pins: an uncaptured image stays hotlinked (a broken local path is worse than an intact remote one), and a filehash owned by another artist never leaks in. Reverting render_post_body to a bare sanitize fails the first two. Recorded as snippet 3968. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SHQB1YukL3VyvMK8rcbmV9
This commit is contained in:
@@ -0,0 +1,93 @@
|
||||
"""Rendering a post's stored HTML body for display — sanitized, and pointing
|
||||
at our own copies of the images rather than the platform's.
|
||||
|
||||
## Why this is one function and not two
|
||||
|
||||
Two surfaces render a post body: the post detail view
|
||||
(`PostFeedService.get_post`) and the provenance panel (`ProvenanceService`).
|
||||
Until issue #3965 the detail view did `_localize_inline_images(sanitize(...))`
|
||||
and provenance did `sanitize(...)` alone — so the provenance panel hotlinked
|
||||
the platform CDN for images FC had already downloaded, which is exactly what
|
||||
#830 Phase 2 set out to stop.
|
||||
|
||||
That bug was available because the two halves were separately callable and
|
||||
only one of them looked mandatory. `render_post_body` is therefore the whole
|
||||
pipeline in a single call, and it is the only thing callers are meant to
|
||||
reach for: sanitizing without localizing is not a supported operation, so it
|
||||
should not be a reachable one. `localize_inline_images` stays public only
|
||||
because a caller that already holds sanitized HTML needs it.
|
||||
|
||||
## Cost
|
||||
|
||||
`localize_inline_images` issues ZERO queries for a body with no inline
|
||||
`<img>`, which is most of them — it returns before touching the session if
|
||||
the body is empty, has no image tags, or has none carrying a parseable CDN
|
||||
filehash. That early exit is why callers may run this per post in a loop
|
||||
rather than needing a batched form; do not "optimize" it into one without a
|
||||
measurement saying the loop is actually hot.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from html import unescape
|
||||
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from ..models import ImageRecord
|
||||
from ..utils.html_sanitize import extract_img_srcs, rewrite_img_srcs, sanitize_post_html
|
||||
from ..utils.paths import filehash_from_url
|
||||
from .gallery_service import image_url
|
||||
|
||||
|
||||
async def render_post_body(
|
||||
session: AsyncSession, description: str | None, artist_id: int | None,
|
||||
) -> str | None:
|
||||
"""A post's body, ready to put in front of someone.
|
||||
|
||||
Sanitize, then repoint inline images at local copies. Use this rather than
|
||||
calling either half on its own — see the module docstring.
|
||||
"""
|
||||
return await localize_inline_images(
|
||||
session, sanitize_post_html(description), artist_id,
|
||||
)
|
||||
|
||||
|
||||
async def localize_inline_images(
|
||||
session: AsyncSession, html: str | None, artist_id: int | None,
|
||||
) -> str | None:
|
||||
"""Rewrite a post body's inline `<img src=CDN>` to locally-served copies.
|
||||
|
||||
The join key is the CDN filehash the downloader persisted on each
|
||||
ImageRecord (source_filehash): for every body image whose filehash maps
|
||||
to a stored image of THIS artist, swap the src to /images/<path>. Images
|
||||
we never captured (or pre-Phase-2 rows with no filehash) are left as-is —
|
||||
they keep hotlinking, which is the prior behavior. Scoped to the post's
|
||||
artist so one creator's body never resolves to another's file.
|
||||
"""
|
||||
if not html or artist_id is None:
|
||||
return html
|
||||
srcs = extract_img_srcs(html)
|
||||
if not srcs:
|
||||
return html
|
||||
# filehash -> the raw (as-in-HTML) src strings carrying it. A body can
|
||||
# repeat the same image; keep every raw form so each is substituted.
|
||||
by_hash: dict[str, list[str]] = {}
|
||||
for raw in srcs:
|
||||
fh = filehash_from_url(unescape(raw))
|
||||
if fh:
|
||||
by_hash.setdefault(fh, []).append(raw)
|
||||
if not by_hash:
|
||||
return html
|
||||
rows = (await session.execute(
|
||||
select(ImageRecord.source_filehash, ImageRecord.path)
|
||||
.where(
|
||||
ImageRecord.artist_id == artist_id,
|
||||
ImageRecord.source_filehash.in_(list(by_hash)),
|
||||
)
|
||||
)).all()
|
||||
replace: dict[str, str] = {}
|
||||
for fh, path in rows:
|
||||
for raw in by_hash.get(fh, ()):
|
||||
replace[raw] = image_url(path)
|
||||
return rewrite_img_srcs(html, replace)
|
||||
@@ -11,8 +11,6 @@ attachments from PostAttachment) so the API layer can jsonify directly.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from html import unescape
|
||||
|
||||
from sqlalchemy import and_, func, or_, select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
@@ -26,15 +24,10 @@ from ..models import (
|
||||
Source,
|
||||
attachment_download_url,
|
||||
)
|
||||
from ..utils.html_sanitize import (
|
||||
extract_img_srcs,
|
||||
rewrite_img_srcs,
|
||||
sanitize_post_html,
|
||||
)
|
||||
from ..utils.paths import filehash_from_url
|
||||
from ..utils.text import html_to_plain, truncate_at_word
|
||||
from .gallery_service import image_url, thumbnail_url
|
||||
from .gallery_service import thumbnail_url
|
||||
from .pagination import decode_cursor, encode_cursor
|
||||
from .post_body import render_post_body
|
||||
|
||||
DESCRIPTION_LIMIT = 280
|
||||
THUMBNAIL_LIMIT = 6
|
||||
@@ -250,54 +243,18 @@ class PostFeedService:
|
||||
item["description_full"] = html_to_plain(post.description)
|
||||
# Full (uncapped) translated description for the detail view (#143).
|
||||
item["description_translated_full"] = post.description_translated
|
||||
# Sanitized HTML body for faithful (semantic) rendering in the post view;
|
||||
# detail-only (the feed list stays lightweight plain text). None when the
|
||||
# post has no body. Inline `<img>` sources are remapped to locally-served
|
||||
# copies (#830 Phase 2) so the body never hotlinks the public CDN.
|
||||
item["description_html"] = await self._localize_inline_images(
|
||||
sanitize_post_html(post.description), post.artist_id,
|
||||
# Rendered body for faithful (semantic) display in the post view;
|
||||
# detail-only, which is what keeps the feed list lightweight plain text
|
||||
# (measured: note #3962). None when the post has no body. What
|
||||
# "rendered" involves — sanitize, then repoint inline images at our own
|
||||
# copies — belongs to `render_post_body`, which the provenance panel
|
||||
# shares so the two surfaces cannot drift apart again (#3965).
|
||||
item["description_html"] = await render_post_body(
|
||||
self.session, post.description, post.artist_id,
|
||||
)
|
||||
item["external_links"] = await self._external_links_for(post.id)
|
||||
return item
|
||||
|
||||
async def _localize_inline_images(
|
||||
self, html: str | None, artist_id: int | None,
|
||||
) -> str | None:
|
||||
"""Rewrite a post body's inline `<img src=CDN>` to locally-served copies.
|
||||
|
||||
The join key is the CDN filehash the downloader persisted on each
|
||||
ImageRecord (source_filehash): for every body image whose filehash maps
|
||||
to a stored image of THIS artist, swap the src to /images/<path>. Images
|
||||
we never captured (or pre-Phase-2 rows with no filehash) are left as-is —
|
||||
they keep hotlinking, which is the prior behavior. Scoped to the post's
|
||||
artist so one creator's body never resolves to another's file."""
|
||||
if not html or artist_id is None:
|
||||
return html
|
||||
srcs = extract_img_srcs(html)
|
||||
if not srcs:
|
||||
return html
|
||||
# filehash -> the raw (as-in-HTML) src strings carrying it. A body can
|
||||
# repeat the same image; keep every raw form so each is substituted.
|
||||
by_hash: dict[str, list[str]] = {}
|
||||
for raw in srcs:
|
||||
fh = filehash_from_url(unescape(raw))
|
||||
if fh:
|
||||
by_hash.setdefault(fh, []).append(raw)
|
||||
if not by_hash:
|
||||
return html
|
||||
rows = (await self.session.execute(
|
||||
select(ImageRecord.source_filehash, ImageRecord.path)
|
||||
.where(
|
||||
ImageRecord.artist_id == artist_id,
|
||||
ImageRecord.source_filehash.in_(list(by_hash)),
|
||||
)
|
||||
)).all()
|
||||
replace: dict[str, str] = {}
|
||||
for fh, path in rows:
|
||||
for raw in by_hash.get(fh, ()):
|
||||
replace[raw] = image_url(path)
|
||||
return rewrite_img_srcs(html, replace)
|
||||
|
||||
async def _external_links_for(self, post_id: int) -> list[dict]:
|
||||
"""Off-platform file-host links recorded for a post (detail-only). Each
|
||||
carries its host, full url, label, and download status so the post view
|
||||
|
||||
@@ -18,17 +18,32 @@ from ..models import (
|
||||
Source,
|
||||
attachment_download_url,
|
||||
)
|
||||
from ..utils.html_sanitize import sanitize_post_html
|
||||
from .post_body import render_post_body
|
||||
|
||||
|
||||
def _post_dict(p: Post) -> dict:
|
||||
async def _post_dict(session: AsyncSession, p: Post) -> dict:
|
||||
"""One provenance entry's post.
|
||||
|
||||
NOTE: the key names here deliberately differ from
|
||||
`PostFeedService._to_dict` (`url`/`title`/`date` vs
|
||||
`post_url`/`post_title`/`post_date`, `attachment_count` vs `attachments`),
|
||||
and `description_translated` is the FULL text here where the feed truncates
|
||||
it to DESCRIPTION_LIMIT. That divergence is issue #3965's wider half and is
|
||||
deliberately NOT addressed here — renaming is a breaking payload change for
|
||||
ProvenancePanel with no second reason to spend it.
|
||||
|
||||
What IS fixed here is the body: this used to call `sanitize_post_html`
|
||||
alone, so provenance bodies hotlinked the platform CDN for images already
|
||||
on disk while the post detail view served local copies. `render_post_body`
|
||||
is the whole pipeline, so the two surfaces cannot drift apart again.
|
||||
"""
|
||||
return {
|
||||
"id": p.id,
|
||||
"external_post_id": p.external_post_id,
|
||||
"url": p.post_url,
|
||||
"title": p.post_title,
|
||||
"date": p.post_date.isoformat() if p.post_date else None,
|
||||
"description_html": sanitize_post_html(p.description),
|
||||
"description_html": await render_post_body(session, p.description, p.artist_id),
|
||||
"attachment_count": p.attachment_count,
|
||||
# Translation (#143): the English title/description shown by default when
|
||||
# a translation exists; the UI toggles to the original. Source lang labels
|
||||
@@ -131,7 +146,7 @@ class ProvenanceService:
|
||||
"provenance_id": ip.id,
|
||||
"captured_at": ip.captured_at.isoformat()
|
||||
if ip.captured_at else None,
|
||||
"post": _post_dict(post),
|
||||
"post": await _post_dict(self.session, post),
|
||||
"source": _source_dict(src) if src is not None else None,
|
||||
"artist": _artist_dict(art),
|
||||
}
|
||||
@@ -154,7 +169,7 @@ class ProvenanceService:
|
||||
return None
|
||||
post, src, art = row
|
||||
return {
|
||||
"post": _post_dict(post),
|
||||
"post": await _post_dict(self.session, post),
|
||||
"source": _source_dict(src) if src is not None else None,
|
||||
"artist": _artist_dict(art),
|
||||
"attachments": await self._attachments_for_posts([post.id]),
|
||||
|
||||
Reference in New Issue
Block a user