Private
Public Access
Expand direct image links instead of showing nothing (#43 follow-up)
A URL that points straight at an image file (Content-Type: image/*) has no HTML to scrape Open Graph tags from, so the fetch found nothing and the message showed no preview at all -- reported against https://imgs.xkcd.com/comics/creepy.png. link_preview_service now recognizes an allowed image content-type (same list storage.py uses for uploads) before falling through to the HTML/og: path, and returns the URL itself as the preview (LinkPreview.is_image). No need to download the body -- the already-SSRF-validated URL is the image. The frontend renders that case as a real expandable image (message-image + lightbox, same as an actual attachment) instead of the small title+description card, which would have nothing to show anyway. Verified end-to-end against the reported URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,35 @@
|
||||
"""link previews flag direct image links
|
||||
|
||||
Revision ID: f3ec1c1d1992
|
||||
Revises: d00f93766fa5
|
||||
Create Date: 2026-08-17 17:56:19.078087
|
||||
|
||||
"""
|
||||
from typing import Sequence, Union
|
||||
|
||||
from alembic import op
|
||||
import sqlalchemy as sa
|
||||
|
||||
|
||||
# revision identifiers, used by Alembic.
|
||||
revision: str = 'f3ec1c1d1992'
|
||||
down_revision: Union[str, Sequence[str], None] = 'd00f93766fa5'
|
||||
branch_labels: Union[str, Sequence[str], None] = None
|
||||
depends_on: Union[str, Sequence[str], None] = None
|
||||
|
||||
|
||||
def upgrade() -> None:
|
||||
"""Upgrade schema."""
|
||||
# NOT NULL with a *constant* server_default (unlike a volatile one such
|
||||
# as now()/clock_timestamp()) doesn't force a table rewrite in Postgres
|
||||
# 11+ -- it's a metadata-only change, safe and instant regardless of
|
||||
# table size.
|
||||
op.add_column(
|
||||
"link_previews",
|
||||
sa.Column("is_image", sa.Boolean(), nullable=False, server_default=sa.false()),
|
||||
)
|
||||
|
||||
|
||||
def downgrade() -> None:
|
||||
"""Downgrade schema."""
|
||||
op.drop_column("link_previews", "is_image")
|
||||
@@ -23,6 +23,12 @@ class LinkPreview(Base):
|
||||
description: Mapped[str | None] = mapped_column(String(1000))
|
||||
image_url: Mapped[str | None] = mapped_column(String(2048))
|
||||
site_name: Mapped[str | None] = mapped_column(String(200))
|
||||
# A direct link to an image file (Content-Type: image/*, no HTML to
|
||||
# scrape og: tags from) gets image_url = the URL itself and no
|
||||
# title/description/site_name -- this flags that case so the frontend
|
||||
# renders it as an actual expanded image rather than the small
|
||||
# title+description unfurl card.
|
||||
is_image: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False)
|
||||
# Cached separately from a "no row yet" state so a URL that genuinely
|
||||
# doesn't unfurl (no title, fetch error, blocked by SSRF checks) isn't
|
||||
# re-fetched on every message that references it within the TTL.
|
||||
|
||||
@@ -27,6 +27,7 @@ class LinkPreviewInfo(BaseModel):
|
||||
description: str | None
|
||||
image_url: str | None
|
||||
site_name: str | None
|
||||
is_image: bool
|
||||
|
||||
|
||||
class MessageRead(BaseModel):
|
||||
|
||||
@@ -11,6 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
|
||||
from app.database import async_session_factory
|
||||
from app.models import LinkPreview
|
||||
from app.services.ssrf import UnsafeUrlError, validate_target_url
|
||||
from app.storage import ALLOWED_IMAGE_CONTENT_TYPES
|
||||
from app.ws.broadcaster import Broadcaster
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
@@ -99,8 +100,21 @@ async def _fetch_preview_data(url: str) -> dict | None:
|
||||
continue
|
||||
if response.status_code >= 400:
|
||||
return None
|
||||
content_type = response.headers.get("content-type", "")
|
||||
if "text/html" not in content_type:
|
||||
content_type = response.headers.get("content-type", "").split(";")[0].strip()
|
||||
# A direct link to an image file -- render the image
|
||||
# itself rather than trying to scrape og: tags from
|
||||
# nonexistent HTML. No need to read the body at all;
|
||||
# the URL (now fully resolved through any redirects,
|
||||
# each already SSRF-validated above) *is* the preview.
|
||||
if content_type in ALLOWED_IMAGE_CONTENT_TYPES:
|
||||
return {
|
||||
"title": None,
|
||||
"description": None,
|
||||
"image_url": current_url,
|
||||
"site_name": None,
|
||||
"is_image": True,
|
||||
}
|
||||
if content_type != "text/html":
|
||||
return None
|
||||
body = b""
|
||||
async for chunk in response.aiter_bytes():
|
||||
@@ -128,6 +142,7 @@ async def _fetch_preview_data(url: str) -> dict | None:
|
||||
"description": (parser.og.get("og:description") or "").strip()[:1000] or None,
|
||||
"image_url": parser.og.get("og:image") or None,
|
||||
"site_name": (parser.og.get("og:site_name") or "").strip()[:200] or None,
|
||||
"is_image": False,
|
||||
}
|
||||
|
||||
|
||||
@@ -148,6 +163,7 @@ async def _get_or_fetch(db: AsyncSession, url: str) -> LinkPreview | None:
|
||||
existing.description = data["description"]
|
||||
existing.image_url = data["image_url"]
|
||||
existing.site_name = data["site_name"]
|
||||
existing.is_image = data["is_image"]
|
||||
existing.fetched_at = datetime.now(timezone.utc)
|
||||
await db.commit()
|
||||
return None if data is None else existing
|
||||
@@ -155,7 +171,16 @@ async def _get_or_fetch(db: AsyncSession, url: str) -> LinkPreview | None:
|
||||
row = LinkPreview(
|
||||
url=url,
|
||||
fetch_failed=data is None,
|
||||
**(data or {"title": None, "description": None, "image_url": None, "site_name": None}),
|
||||
**(
|
||||
data
|
||||
or {
|
||||
"title": None,
|
||||
"description": None,
|
||||
"image_url": None,
|
||||
"site_name": None,
|
||||
"is_image": False,
|
||||
}
|
||||
),
|
||||
)
|
||||
db.add(row)
|
||||
await db.commit()
|
||||
@@ -192,6 +217,7 @@ async def fetch_and_broadcast_link_preview(
|
||||
"description": preview.description,
|
||||
"image_url": preview.image_url,
|
||||
"site_name": preview.site_name,
|
||||
"is_image": preview.is_image,
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
@@ -93,7 +93,7 @@ _OG_HTML = (
|
||||
)
|
||||
|
||||
|
||||
def _fake_client_factory(html=_OG_HTML, status_code=200, call_log=None):
|
||||
def _fake_client_factory(html=_OG_HTML, status_code=200, call_log=None, content_type="text/html"):
|
||||
class _FakeAsyncClient:
|
||||
def __init__(self, *args, **kwargs):
|
||||
pass
|
||||
@@ -107,7 +107,7 @@ def _fake_client_factory(html=_OG_HTML, status_code=200, call_log=None):
|
||||
def stream(self, method, url, headers=None):
|
||||
if call_log is not None:
|
||||
call_log.append(url)
|
||||
return _FakeStreamCtx(_FakeResponse(status_code, {"content-type": "text/html"}, html))
|
||||
return _FakeStreamCtx(_FakeResponse(status_code, {"content-type": content_type}, html))
|
||||
|
||||
return _FakeAsyncClient
|
||||
|
||||
@@ -155,6 +155,36 @@ def test_ws_message_with_url_triggers_link_preview_broadcast(ws_client, monkeypa
|
||||
assert preview["description"] == "A description of the article."
|
||||
assert preview["image_url"] == "https://8.8.8.8/image.png"
|
||||
assert preview["site_name"] == "Example"
|
||||
assert preview["is_image"] is False
|
||||
|
||||
|
||||
def test_ws_message_with_direct_image_url_expands_the_image(ws_client, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"app.services.link_preview_service.httpx.AsyncClient",
|
||||
_fake_client_factory(html=b"", content_type="image/png"),
|
||||
)
|
||||
url = _unique_url() + ".png"
|
||||
|
||||
username = _unique("alice")
|
||||
_register_ws(ws_client, username=username)
|
||||
room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json()
|
||||
|
||||
with ws_client.websocket_connect("/ws/chat") as ws:
|
||||
ws.send_json({"type": "join", "room_id": room["id"]})
|
||||
assert _recv(ws)["type"] == "joined"
|
||||
|
||||
ws.send_json({"type": "message", "room_id": room["id"], "content": f"lol {url}"})
|
||||
message = _recv(ws)
|
||||
assert message["link_preview"] is None
|
||||
|
||||
preview = _recv(ws)
|
||||
assert preview["type"] == "link_preview"
|
||||
assert preview["url"] == url
|
||||
assert preview["image_url"] == url
|
||||
assert preview["is_image"] is True
|
||||
assert preview["title"] is None
|
||||
assert preview["description"] is None
|
||||
assert preview["site_name"] is None
|
||||
|
||||
|
||||
def test_ws_message_with_private_url_gets_no_preview(ws_client, monkeypatch):
|
||||
|
||||
Reference in New Issue
Block a user