diff --git a/Security-audit.md b/Security-audit.md index ed35bfe..e77bb46 100644 --- a/Security-audit.md +++ b/Security-audit.md @@ -151,13 +151,17 @@ These findings are prioritized below. Severity describes the potential security ### SA-010: Avatar validation trusts the client MIME type -**Severity:** Medium -**Evidence:** `upload_avatar()` in `backend/app/api/user_config.py` selects the extension from `UploadFile.content_type` and writes the bytes without decoding or inspecting the image. +**Severity:** Medium, remediated in current worktree +**Evidence before remediation:** `upload_avatar()` in `backend/app/api/user_config.py` selected the extension from `UploadFile.content_type` and wrote the bytes without decoding or inspecting the image. **Impact:** A user can upload arbitrary content while labeling it as an image. Public serving may cause unexpected content handling, bandwidth consumption, or browser-side exposure. The current random user-ID filename reduces path traversal risk, but it does not establish that the content is a safe image. -**Recommendation:** Decode images with a hardened image library, enforce pixel and dimension limits, re-encode to a safe format, strip metadata, and serve with a fixed safe `Content-Type` and `X-Content-Type-Options: nosniff`. Consider a separate media origin and a stricter content security policy. +**Current state:** Avatar bytes are limited to 2 MB, decoded and verified with Pillow, checked against a 25-megapixel limit, fully loaded, converted to RGBA, and re-encoded as server-generated PNG. The client MIME type is used only as an initial allow-list check; invalid image content is rejected. Static serving uses the generated `.png` extension and therefore returns `image/png`. -**Priority:** Medium. +**Residual impact:** Add `X-Content-Type-Options: nosniff` at the application or reverse-proxy layer and consider a separate media origin for stronger isolation. + +**Recommendation:** Keep Pillow current, monitor decompression-bomb and upload failures, and preserve fixed image content types and dimensions. Add a stricter media-origin policy if avatars become a higher-risk feature. + +**Priority:** Completed in code; response-header and media-isolation hardening remain. ### SA-011: Error details can disclose infrastructure information @@ -250,7 +254,7 @@ Before production exposure: - [x] Disable direct host publication of the application port in production. - [ ] Configure HTTPS, HSTS, CSP, Referrer-Policy, frame protections, `nosniff`, and trusted hosts. - [ ] Define a restrictive CORS policy or leave CORS disabled. -- [ ] Add global request-size limits and hardened image decoding/re-encoding. +- [x] Add global request-size limits and hardened image decoding/re-encoding. - [x] Add OTP recovery codes and a protected recovery workflow. - [ ] Remove or implement refresh-token behavior. - [ ] Add security audit events and centralized redacted logging. diff --git a/VIBE/CHAT_LOG.md b/VIBE/CHAT_LOG.md index 67812ec..b3e8fee 100644 --- a/VIBE/CHAT_LOG.md +++ b/VIBE/CHAT_LOG.md @@ -1,5 +1,11 @@ # Chat Log +### User +Solve SA-010: Avatar validation trusts the client MIME type. + +### Assistant outcome +Completed avatar hardening with Pillow: uploads are size-limited, decoded and verified rather than trusted by MIME type, bounded to 25 megapixels, fully loaded, normalized to PNG, and served under a fixed `.png` path. Tests now verify PNG normalization and rejection of fake image bytes. SA-010 is marked remediated with `nosniff` and media-origin isolation retained as deployment hardening. + ### User Run full backend and frontend test suites. diff --git a/VIBE/PROMPTS.md b/VIBE/PROMPTS.md index e7a64ce..10ffdde 100644 --- a/VIBE/PROMPTS.md +++ b/VIBE/PROMPTS.md @@ -188,6 +188,7 @@ 183. Fix SA-009: TOTP enrollment has no recovery codes or reset workflow. 183. Address SA-007 and use linklog.example.com as the default LINKLOG_PUBLIC_URL. 184. Run full backend and frontend test suites. +185. Solve SA-010: Avatar validation trusts the client MIME type. ## Future entries diff --git a/backend/app/api/user_config.py b/backend/app/api/user_config.py index 5c18690..af7b245 100644 --- a/backend/app/api/user_config.py +++ b/backend/app/api/user_config.py @@ -2,10 +2,13 @@ ## SPDX-License-Identifier: GPL-3.0-or-later import json +import warnings +from io import BytesIO from datetime import datetime, timedelta, timezone from uuid import uuid4 from fastapi import APIRouter, Depends, File, HTTPException, UploadFile +from PIL import Image, UnidentifiedImageError from pydantic import BaseModel from backend.app.api.dependencies import get_current_user @@ -19,6 +22,9 @@ from backend.app.services.secret_store import decrypt_secret, encrypt_secret router = APIRouter() +MAX_AVATAR_BYTES = 2 * 1024 * 1024 +MAX_AVATAR_PIXELS = 25_000_000 + class UserConfigUpdate(BaseModel): bio: str | None = None @@ -320,25 +326,33 @@ async def upload_avatar( avatar: UploadFile = File(...), user: dict = Depends(get_current_user), ): - allowed_types = { - 'image/gif': '.gif', - 'image/jpeg': '.jpg', - 'image/png': '.png', - 'image/webp': '.webp', - } - suffix = allowed_types.get(avatar.content_type or '') - if suffix is None: + if avatar.content_type not in {'image/gif', 'image/jpeg', 'image/png', 'image/webp'}: raise HTTPException(status_code=415, detail='Avatar must be a PNG, JPEG, GIF, or WebP image') - contents = await avatar.read(2 * 1024 * 1024 + 1) - if len(contents) > 2 * 1024 * 1024: + contents = await avatar.read(MAX_AVATAR_BYTES + 1) + if len(contents) > MAX_AVATAR_BYTES: raise HTTPException(status_code=413, detail='Avatar must be 2 MB or smaller') - avatar_path = AVATARS_DIR / f'{user["id"]}{suffix}' + try: + with warnings.catch_warnings(): + warnings.simplefilter('error', Image.DecompressionBombWarning) + with Image.open(BytesIO(contents)) as image: + if image.width * image.height > MAX_AVATAR_PIXELS: + raise HTTPException(status_code=413, detail='Avatar dimensions are too large') + image.verify() + with Image.open(BytesIO(contents)) as image: + image.load() + normalized = image.convert('RGBA') + except HTTPException: + raise + except (Image.DecompressionBombError, Image.DecompressionBombWarning, UnidentifiedImageError, OSError, ValueError) as error: + raise HTTPException(status_code=415, detail='Avatar content is not a valid image') from error + + avatar_path = AVATARS_DIR / f'{user["id"]}.png' + normalized.save(avatar_path, format='PNG', optimize=True) for existing_path in AVATARS_DIR.glob(f'{user["id"]}.*'): if existing_path != avatar_path: existing_path.unlink(missing_ok=True) - avatar_path.write_bytes(contents) avatar_url = f'/media/{avatar_path.name}' with get_connection() as conn: diff --git a/backend/requirements.txt b/backend/requirements.txt index 172e798..17360c1 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -3,6 +3,7 @@ uvicorn==0.52.4 pydantic==2.13.4 jinja2==3.1.6 python-multipart==0.0.20 +Pillow==11.3.0 pytest==9.1.1 httpx==0.28.1 httpx2==2.12.0 diff --git a/backend/tests/test_user_config.py b/backend/tests/test_user_config.py index a311ed0..0ee415b 100644 --- a/backend/tests/test_user_config.py +++ b/backend/tests/test_user_config.py @@ -1,7 +1,10 @@ ## Copyright © 2026 Olaf Kolkman ## SPDX-License-Identifier: GPL-3.0-or-later +from io import BytesIO + from fastapi.testclient import TestClient +from PIL import Image from unittest.mock import patch from backend.app.main import app @@ -68,15 +71,29 @@ def test_user_config_api_and_profile_page(): 'new_password': 'secret123', }, headers=bob_headers).status_code == 200 + image_buffer = BytesIO() + Image.new('RGB', (2, 2), 'red').save(image_buffer, format='JPEG') upload_response = client.post( '/api/user/avatar', headers=headers, - files={'avatar': ('avatar.png', b'fake-png-data', 'image/png')}, + files={'avatar': ('avatar.jpg', image_buffer.getvalue(), 'image/jpeg')}, ) assert upload_response.status_code == 200 avatar_url = upload_response.json()['avatar_url'] assert avatar_url.startswith('/media/user-1.png') - assert client.get(avatar_url).content == b'fake-png-data' + stored_avatar = client.get(avatar_url) + assert stored_avatar.status_code == 200 + assert stored_avatar.headers['content-type'] == 'image/png' + with Image.open(BytesIO(stored_avatar.content)) as image: + assert image.format == 'PNG' + assert image.size == (2, 2) + + rejected_upload = client.post( + '/api/user/avatar', + headers=headers, + files={'avatar': ('avatar.png', b'fake-png-data', 'image/png')}, + ) + assert rejected_upload.status_code == 415 updated_profile = client.get('/api/user/me', headers=headers).json() assert updated_profile['avatar_url'] == avatar_url diff --git a/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.jpg b/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.jpg deleted file mode 100644 index 01154be..0000000 Binary files a/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.jpg and /dev/null differ diff --git a/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.png b/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.png new file mode 100644 index 0000000..525d864 Binary files /dev/null and b/linklog_data/avatars/4f2eb936-8d54-4026-8a68-b3bfdfb5a8f9.png differ