fix(tradein-photos): sanitize uploaded images via Pillow re-encode #511

Merged

lekss361 merged 1 commit from feat/tradein-photo-sanitization into main

2026-05-24 11:32:05 +00:00

lekss361 commented

2026-05-24 11:25:52 +00:00

Owner

Summary

Closes finding #6 from 2026-05-24 audit.

upload_photo previously stored raw uploaded bytes verbatim and served them back with the original image/* content-type. Risks:

EXIF leak: phone photos carried GPS lat/lon, device fingerprint, owner metadata.
Polyglot files: a JPEG with appended PHP/HTML/JS payload passed MIME check, sat in estimate_photos.content forever, and was served back labeled as image — any downstream consumer that sniffs or reuses the bytes could execute the payload.
No decoder safety: malformed images bypassing decoder CVEs (libwebp etc.) had no defense.

Approach

New services/image_sanitizer.py (~70 lines, pure — no DB/I/O):

Image.open() + img.load() forces full decode (surfaces malformed input early).
Convert any mode (RGBA / P / CMYK) → RGB.
Cap longest edge at 2400 px via Image.Resampling.LANCZOS thumbnail.
Re-save as JPEG quality=85, optimize=True. No exif= kwarg → metadata dropped.
Returns (bytes, "image/jpeg"); raises ImageSanitizationError on undecodable input.

upload_photo calls sanitize_image() after the existing 10MB / 12-per-estimate / MIME-whitelist filters. Sanitizer failure → HTTP 400. Stored bytes + content_type are the sanitized ones; log records both original and sanitized sizes for monitoring.

Pin Pillow>=10.3.0 explicitly in pyproject.toml (was transitive via weasyprint — pin defends against a weasyprint upgrade silently dropping it).

No HEIC support added in this PR — Pillow without pillow-heif rejects HEIC with UnidentifiedImageError, surfaced as 400. Separate PR if needed.

Smoke test (local, in worker)

input size: 849
output size: 361 ctype: image/jpeg
EXIF stripped: OK
garbage rejected: not a recognized image format

No existing photo tests in repo → no test fixup needed.

Test plan

After deploy: upload a phone JPEG with GPS EXIF via POST /api/v1/trade-in/estimate/{id}/photos; download via GET /photos/{photo_id}; verify exiftool shows zero EXIF tags.
Upload a polyglot file (JPEG header + appended <?php ?> block); verify stored bytes do not contain the payload.
Upload a 20MB phone photo; verify sanitized bytes are < 2MB and stored size_bytes matches.
Upload text/plain masquerading via filename trick; verify 400 (already caught by MIME whitelist) or 400 from sanitizer.

## Summary Closes finding #6 from 2026-05-24 audit. `upload_photo` previously stored raw uploaded bytes verbatim and served them back with the original `image/*` content-type. Risks: - **EXIF leak**: phone photos carried GPS lat/lon, device fingerprint, owner metadata. - **Polyglot files**: a JPEG with appended PHP/HTML/JS payload passed MIME check, sat in `estimate_photos.content` forever, and was served back labeled as image — any downstream consumer that sniffs or reuses the bytes could execute the payload. - **No decoder safety**: malformed images bypassing decoder CVEs (libwebp etc.) had no defense. ## Approach New `services/image_sanitizer.py` (~70 lines, pure — no DB/I/O): - `Image.open()` + `img.load()` forces full decode (surfaces malformed input early). - Convert any mode (RGBA / P / CMYK) → RGB. - Cap longest edge at **2400 px** via `Image.Resampling.LANCZOS` thumbnail. - Re-save as **JPEG quality=85, optimize=True**. No `exif=` kwarg → metadata dropped. - Returns `(bytes, "image/jpeg")`; raises `ImageSanitizationError` on undecodable input. `upload_photo` calls `sanitize_image()` after the existing 10MB / 12-per-estimate / MIME-whitelist filters. Sanitizer failure → HTTP 400. Stored bytes + content_type are the **sanitized** ones; log records both original and sanitized sizes for monitoring. Pin `Pillow>=10.3.0` explicitly in pyproject.toml (was transitive via weasyprint — pin defends against a weasyprint upgrade silently dropping it). No HEIC support added in this PR — Pillow without `pillow-heif` rejects HEIC with `UnidentifiedImageError`, surfaced as 400. Separate PR if needed. ## Smoke test (local, in worker) ``` input size: 849 output size: 361 ctype: image/jpeg EXIF stripped: OK garbage rejected: not a recognized image format ``` No existing photo tests in repo → no test fixup needed. ## Test plan - [ ] After deploy: upload a phone JPEG with GPS EXIF via `POST /api/v1/trade-in/estimate/{id}/photos`; download via `GET /photos/{photo_id}`; verify `exiftool` shows zero EXIF tags. - [ ] Upload a polyglot file (JPEG header + appended `<?php ?>` block); verify stored bytes do not contain the payload. - [ ] Upload a 20MB phone photo; verify sanitized bytes are < 2MB and stored size_bytes matches. - [ ] Upload `text/plain` masquerading via filename trick; verify 400 (already caught by MIME whitelist) or 400 from sanitizer.

lekss361 added 1 commit 2026-05-24 11:25:53 +00:00

fix(tradein-photos): sanitize uploaded images via Pillow re-encode 9d87f80e7e

upload_photo endpoint stored raw uploaded bytes verbatim — EXIF GPS/device
fingerprint leaked, polyglot files (image + appended payload) survived MIME
check and were served back with image/* content-type.

Add services/image_sanitizer.py: open via Pillow, force full decode, drop
EXIF, normalize to RGB, cap longest edge at 2400px, re-encode as JPEG
quality=85. Pipe upload_photo through it; reject undecodable input with 400.

Pin Pillow>=10.3.0 explicitly (was transitive via weasyprint).

Closes finding #6 from 2026-05-24 trade-in audit.

lekss361 commented

2026-05-24 11:31:56 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 3 (P0: 1 sanitizer + endpoint, P2: pyproject)
Lines: +84 / -4
Risk: low · Reversibility: trivial (revert PR) · Merge window: anytime

Security review (focus)

EXIF stripping — img.save() без exif= kwarg: Pillow не переносит info['exif'] на JPEG output по умолчанию. Подтверждено корректно.
Polyglot files — Image.open + img.load() декодирует только пиксельные данные; img.save() пишет свежий JPEG поток, любой appended PHP/JS payload отбрасывается. Корректно.
Decompression bomb — PIL.Image.MAX_IMAGE_PIXELS по умолчанию ≈89M пикселей (DecompressionBombError ловится в except Exception → 400). В сочетании с upstream _MAX_PHOTO_BYTES = 10 MiB достаточная защита. Явный коммент про MAX_IMAGE_PIXELS можно добавить, но не блокер.
Format detection — magic-bytes Pillow, не spoofable Content-Type header.
Stored content-type теперь всегда image/jpeg — устраняет MIME confusion для serving endpoint (get_photo отдаёт row.content_type как media_type).

Correctness

convert("RGB") для RGBA/P/CMYK — теряет alpha (для фото квартир OK).
img.thumbnail() LANCZOS, preserves aspect ratio.
Multi-frame (анимированный GIF, multipage TIFF) → только первый кадр. Acceptable для apt photos.
db.commit() после sanitize — sanitize fail → HTTPException 400 до любого DB write. Нет partial state.

Performance

🟢 Sync Pillow decode в async endpoint: ~100–500 ms блокирует event loop для 10 MB phone JPEG @ LANCZOS 2400px. Не блокер (rate-limit естественный: 12 photos/estimate), но в будущем можно завернуть в run_in_threadpool. Follow-up.

Conventions

from __future__ import annotations, type hints, module-private constants, logging.getLogger(__name__) — matches backend conventions.
ImageSanitizationError(ValueError) — чистая иерархия.
Bare except Exception обоснован комментом + logging + typed re-raise — defensive для third-party decoder.

Tests (G — minor gap)

🟡 Нет unit-тестов для нового image_sanitizer.py (idiomatic case: valid JPEG → EXIF dropped; garbage → raises ImageSanitizationError; 5000×5000 → resized). PR описывает в-worker smoke test, но не покрыт. Не блокер (PR-овский test plan покрывает integration after deploy), но follow-up ticket разумен.

Vault cross-ref

В vault нет previous decision на tradein photo handling. После merge — /vault-write зафиксировать sanitization policy (JPEG-only re-encode, 2400px cap, q=85, EXIF drop) в decisions/. Task для main session.

Minor / follow-up

🟢 _ALLOWED_IMAGE_TYPES whitelist'ит image/heic, но без pillow-heif сторонней зависимости Pillow вернёт UnidentifiedImageError → 400. Слегка confusing UX (whitelisted MIME, но reject). Либо удалить heic из whitelist, либо добавить pillow-heif. Не блокер.
🟢 Pillow≥10.3.0 явный pin — verified (10.3.0 release notes: фиксы libwebp CVE-2023-4863 cohort уже включены, mode confusion fixes). Pin defends против weasyprint downgrade.

Positive

Чистая separation of concerns: services/image_sanitizer.py pure, без DB/I/O, testable.
Логирование orig_size + sanitized_size + ctype даёт monitoring базу для аудита размеров.
Корректный порядок чек'ов: estimate exists → MIME → count limit → size cap → sanitize → INSERT.
Реализация ровно соответствует описанному в PR подходу — no scope creep, no silent regression.

Recommended next steps

Merge as-is (auto-merge via reviewer).
После deploy: выполнить ручной test plan из PR (exiftool check, polyglot upload, 20MB phone photo).
Follow-up: unit-тесты для sanitize_image (3 кейса) + decision запись в vault.
(Опц.) run_in_threadpool для decode под нагрузкой; убрать image/heic из whitelist либо добавить pillow-heif.

## Deep Code Review — verdict ### Summary - **Status:** APPROVE - **Files:** 3 (P0: 1 sanitizer + endpoint, P2: pyproject) - **Lines:** +84 / -4 - **Risk:** low · **Reversibility:** trivial (revert PR) · **Merge window:** anytime ### Security review (focus) - **EXIF stripping** — `img.save()` без `exif=` kwarg: Pillow не переносит `info['exif']` на JPEG output по умолчанию. Подтверждено корректно. - **Polyglot files** — `Image.open` + `img.load()` декодирует только пиксельные данные; `img.save()` пишет свежий JPEG поток, любой appended PHP/JS payload отбрасывается. Корректно. - **Decompression bomb** — `PIL.Image.MAX_IMAGE_PIXELS` по умолчанию ≈89M пикселей (`DecompressionBombError` ловится в `except Exception` → 400). В сочетании с upstream `_MAX_PHOTO_BYTES = 10 MiB` достаточная защита. Явный коммент про `MAX_IMAGE_PIXELS` можно добавить, но не блокер. - **Format detection** — magic-bytes Pillow, не spoofable `Content-Type` header. - **Stored content-type** теперь всегда `image/jpeg` — устраняет MIME confusion для serving endpoint (`get_photo` отдаёт `row.content_type` как `media_type`). ### Correctness - `convert("RGB")` для RGBA/P/CMYK — теряет alpha (для фото квартир OK). - `img.thumbnail()` LANCZOS, preserves aspect ratio. - Multi-frame (анимированный GIF, multipage TIFF) → только первый кадр. Acceptable для apt photos. - `db.commit()` после sanitize — sanitize fail → HTTPException 400 до любого DB write. Нет partial state. ### Performance - 🟢 Sync Pillow decode в async endpoint: ~100–500 ms блокирует event loop для 10 MB phone JPEG @ LANCZOS 2400px. Не блокер (rate-limit естественный: 12 photos/estimate), но в будущем можно завернуть в `run_in_threadpool`. Follow-up. ### Conventions - `from __future__ import annotations`, type hints, module-private constants, `logging.getLogger(__name__)` — matches backend conventions. - `ImageSanitizationError(ValueError)` — чистая иерархия. - Bare `except Exception` обоснован комментом + logging + typed re-raise — defensive для third-party decoder. ### Tests (G — minor gap) - 🟡 Нет unit-тестов для нового `image_sanitizer.py` (idiomatic case: valid JPEG → EXIF dropped; garbage → raises `ImageSanitizationError`; 5000×5000 → resized). PR описывает в-worker smoke test, но не покрыт. Не блокер (PR-овский test plan покрывает integration after deploy), но follow-up ticket разумен. ### Vault cross-ref - В vault нет previous decision на tradein photo handling. После merge — `/vault-write` зафиксировать sanitization policy (JPEG-only re-encode, 2400px cap, q=85, EXIF drop) в `decisions/`. Task для main session. ### Minor / follow-up - 🟢 `_ALLOWED_IMAGE_TYPES` whitelist'ит `image/heic`, но без `pillow-heif` сторонней зависимости Pillow вернёт `UnidentifiedImageError` → 400. Слегка confusing UX (whitelisted MIME, но reject). Либо удалить heic из whitelist, либо добавить `pillow-heif`. Не блокер. - 🟢 Pillow≥10.3.0 явный pin — verified (10.3.0 release notes: фиксы libwebp CVE-2023-4863 cohort уже включены, mode confusion fixes). Pin defends против weasyprint downgrade. ### Positive - Чистая separation of concerns: `services/image_sanitizer.py` pure, без DB/I/O, testable. - Логирование `orig_size` + `sanitized_size` + `ctype` даёт monitoring базу для аудита размеров. - Корректный порядок чек'ов: estimate exists → MIME → count limit → size cap → sanitize → INSERT. - Реализация ровно соответствует описанному в PR подходу — no scope creep, no silent regression. ### Recommended next steps 1. Merge as-is (auto-merge via reviewer). 2. После deploy: выполнить ручной test plan из PR (exiftool check, polyglot upload, 20MB phone photo). 3. Follow-up: unit-тесты для `sanitize_image` (3 кейса) + decision запись в vault. 4. (Опц.) `run_in_threadpool` для decode под нагрузкой; убрать `image/heic` из whitelist либо добавить `pillow-heif`.