From caed8543eabe29926af54ee7dbbfa62e7ae4ae54 Mon Sep 17 00:00:00 2001 From: Lukas Remis Date: Mon, 28 Sep 2026 08:09:09 +0200 Subject: [PATCH] Avatars: a cached avatar or miss failed on every later request The shared Redis client decodes replies as UTF-8 text, but the avatar cache holds raw image bytes and a b"none" miss marker: an image could not be read back at all, and a miss came back as str and crashed on .partition(b"\n"). Only a sender's first request worked; after that, a 500. The cache now goes through a binary-safe client, get_redis_bytes(). Co-Authored-By: Claude Opus 5.5 --- backend/app/api/avatar.py | 5 +- backend/app/redis_client.py | 21 +++++++- backend/tests/api/test_avatar.py | 85 ++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 backend/tests/api/test_avatar.py diff --git a/backend/app/api/avatar.py b/backend/app/api/avatar.py index 7f97a95..0c75375 100644 --- a/backend/app/api/avatar.py +++ b/backend/app/api/avatar.py @@ -17,7 +17,7 @@ from fastapi import APIRouter, Query, Response from app.accounts.ssrf import SsrfError, guarded_request from app.config import get_settings from app.identity.deps import CurrentUser -from app.redis_client import get_redis +from app.redis_client import get_redis_bytes router = APIRouter(prefix="/avatar", tags=["mail"]) @@ -66,7 +66,8 @@ async def sender_avatar(_user: CurrentUser, email: str = Query(...)) -> Response if not get_settings().sender_avatars_enabled or "@" not in email: return Response(status_code=404) - redis = get_redis() + # Binary-safe client: the cache holds raw image bytes (and a b"none" marker). + redis = get_redis_bytes() md5 = _email_md5(email) key = f"avatar:{md5}" headers = {"Cache-Control": "private, max-age=86400"} diff --git a/backend/app/redis_client.py b/backend/app/redis_client.py index b78025c..bbebbb6 100644 --- a/backend/app/redis_client.py +++ b/backend/app/redis_client.py @@ -20,6 +20,7 @@ def aw(result: Any) -> Awaitable[Any]: _client: aioredis.Redis | None = None +_bytes_client: aioredis.Redis | None = None def get_redis() -> aioredis.Redis: @@ -34,9 +35,25 @@ def get_redis() -> aioredis.Redis: return _client +def get_redis_bytes() -> aioredis.Redis: + """Return the process-wide Redis client for binary values. + + ``get_redis()`` decodes every reply as UTF-8 text, which fails on (or + silently changes the type of) raw bytes such as a cached image. Anything that + stores bytes goes through this one instead. + """ + global _bytes_client + if _bytes_client is None: + _bytes_client = aioredis.Redis.from_url(get_settings().redis_url, decode_responses=False) + return _bytes_client + + async def close_redis() -> None: - """Close the Redis client (used on app shutdown).""" - global _client + """Close the Redis clients (used on app shutdown).""" + global _client, _bytes_client if _client is not None: await _client.aclose() _client = None + if _bytes_client is not None: + await _bytes_client.aclose() + _bytes_client = None diff --git a/backend/tests/api/test_avatar.py b/backend/tests/api/test_avatar.py new file mode 100644 index 0000000..0ad4eac --- /dev/null +++ b/backend/tests/api/test_avatar.py @@ -0,0 +1,85 @@ +"""Sender avatar cache (SPEC §9.1): a cached hit or miss must be served again. + +The shared Redis client decodes replies as text, so the cache of raw image +bytes broke on the second request for any sender — a 500 instead of the image +(or instead of the 404 that means "draw initials"). +""" + +from __future__ import annotations + +import uuid +from collections.abc import AsyncIterator + +import httpx +import pytest +import pytest_asyncio +from httpx import ASGITransport +from sqlalchemy import delete +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from sqlalchemy.pool import NullPool + +from app.api import avatar +from app.config import get_settings +from app.identity.passwords import hash_password +from app.identity.sessions import create_session +from app.main import create_app +from app.models import User + +# A PNG signature: not valid UTF-8, exactly what a text-decoding client chokes on. +PNG = b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\xff\xfe" + + +@pytest_asyncio.fixture +async def client() -> AsyncIterator[httpx.AsyncClient]: + engine = create_async_engine(get_settings().database_url, poolclass=NullPool) + session = async_sessionmaker(engine, expire_on_commit=False)() + user = User(handle=f"av{uuid.uuid4().hex[:8]}@test.local", password_hash=hash_password("pw")) + session.add(user) + await session.commit() + token = await create_session(user.id, "pytest") + async with httpx.AsyncClient( + transport=ASGITransport(app=create_app()), + base_url="http://testserver", + cookies={"session": token}, + ) as c: + yield c + await session.execute(delete(User).where(User.id == user.id)) + await session.commit() + await session.close() + await engine.dispose() + + +async def test_cached_avatar_is_served_again( + client: httpx.AsyncClient, monkeypatch: pytest.MonkeyPatch +) -> None: + calls = 0 + + async def fake_resolve(email: str, md5: str) -> tuple[str, bytes] | None: + nonlocal calls + calls += 1 + return "image/png", PNG + + monkeypatch.setattr(avatar, "_resolve", fake_resolve) + for _ in range(2): + resp = await client.get("/api/avatar", params={"email": "alice@example.com"}) + assert resp.status_code == 200 + assert resp.content == PNG + assert resp.headers["content-type"] == "image/png" + assert calls == 1 # the second came from the cache + + +async def test_cached_miss_is_a_404_again( + client: httpx.AsyncClient, monkeypatch: pytest.MonkeyPatch +) -> None: + calls = 0 + + async def fake_resolve(email: str, md5: str) -> tuple[str, bytes] | None: + nonlocal calls + calls += 1 + return None + + monkeypatch.setattr(avatar, "_resolve", fake_resolve) + for _ in range(2): + resp = await client.get("/api/avatar", params={"email": "nobody@example.com"}) + assert resp.status_code == 404 + assert calls == 1