Skip to content

Commit 62c3a11

Browse files
fix(profile): close PR #87 review nits — math comment + 413 at route
Two actionable observations from the self-review: 1. Comment math was wrong. 10M base64 chars decode to ~7.5 MB binary (4/3 expansion ratio), not ~5.6 MB. Rewrote the comment to make the relationship between the body cap and MAX_AVATAR_SIZE explicit, and to document that the actual binary cap fires below. 2. Decoded-byte size is now checked at the route layer with an explicit 413, instead of relying on _validate_upload deeper in the storage service. Two reasons: - Faster failure: bypassed-frontend callers eat 413 before any Supabase round-trip. - Easier route-test reasoning: the contract ("> MAX_AVATAR_SIZE binary bytes -> 413") lives at the route now. The storage-layer validate_upload check stays as defence in depth (and still fires for the surviving multipart-cosmetic endpoint, which doesn't have the route-level guard). Added test_decoded_payload_over_size_cap_returns_413 that pins the contract: a base64 payload that fits the Pydantic max_length but decodes to MAX_AVATAR_SIZE + 1 bytes is rejected with 413 at the route, AND `upload_avatar` is never called. 5 avatar tests pass. The other two minor observations from the review (PR-body wording about the breaking contract change; URL.createObjectURL heads-up) are not code changes — the first is a PR-description tweak handled separately, the second is informational only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eac1738 commit 62c3a11

2 files changed

Lines changed: 45 additions & 1 deletion

File tree

backend/routes/profile.py

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from fastapi import APIRouter, HTTPException, Request, UploadFile, File, Query
1111
from pydantic import BaseModel, Field
1212

13+
from config import MAX_AVATAR_SIZE
1314
from db.connection import table
1415
from services.encryption import encrypt_if_present, decrypt_if_present
1516
from models import (
@@ -269,7 +270,12 @@ class _AvatarUploadBody(BaseModel):
269270
profile-PATCH endpoint successfully.
270271
"""
271272

272-
file_b64: str = Field(..., max_length=10_000_000) # ~7.5 MB base64 = ~5.6 MB binary
273+
# 10M base64 chars upper-bounds the body before any decode work.
274+
# Base64 expands binary by 4/3, so 10M chars decodes to ~7.5 MB
275+
# binary — comfortably above MAX_AVATAR_SIZE (5 MB) and below any
276+
# sensible JSON body limit. The actual binary cap is enforced
277+
# below after decoding.
278+
file_b64: str = Field(..., max_length=10_000_000)
273279
content_type: str = Field(default="image/png", max_length=64)
274280

275281

@@ -293,6 +299,18 @@ async def upload_user_avatar(user_id: str, body: _AvatarUploadBody, request: Req
293299
except Exception:
294300
raise HTTPException(status_code=400, detail="Invalid base64 payload")
295301

302+
# Enforce the binary size cap at the route layer so the 413 is
303+
# immediate and the response shape matches the multipart endpoint
304+
# the caller might still expect (per `services/storage_service.py
305+
# ::_validate_upload`). Without this, the cap fires one layer
306+
# deeper inside upload_avatar, which is correct but harder to
307+
# reason about from a route-test perspective.
308+
if len(file_bytes) > MAX_AVATAR_SIZE:
309+
raise HTTPException(
310+
status_code=413,
311+
detail=f"File too large. Maximum size is {MAX_AVATAR_SIZE // (1024 * 1024)} MB",
312+
)
313+
296314
avatar_url = upload_avatar(user_id, file_bytes, body.content_type)
297315

298316
table("users").update({"avatar_url": avatar_url}, filters={"id": f"eq.{user_id}"})

backend/tests/test_profile_routes.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -566,6 +566,32 @@ def test_missing_file_b64_returns_422(self):
566566
)
567567
assert r.status_code == 422
568568

569+
def test_decoded_payload_over_size_cap_returns_413(self):
570+
"""The route enforces MAX_AVATAR_SIZE on the *decoded* binary —
571+
so a base64 payload that fits the Pydantic max_length but
572+
decodes to more than 5 MB is rejected at the route layer with
573+
a 413 (not at the storage layer). Mirrors the multipart
574+
endpoint's validate_upload contract."""
575+
import base64
576+
from config import MAX_AVATAR_SIZE
577+
# MAX + 1 binary bytes → just above the cap once decoded.
578+
oversize_bytes = b"\x00" * (MAX_AVATAR_SIZE + 1)
579+
oversize_b64 = base64.b64encode(oversize_bytes).decode("ascii")
580+
581+
with _mock_self(), \
582+
patch("routes.profile.table", side_effect=self._table_factory()), \
583+
patch("routes.profile.upload_avatar") as up:
584+
r = client.post(
585+
f"/api/profile/{USER_ID}/avatar",
586+
json={"file_b64": oversize_b64, "content_type": "image/png"},
587+
)
588+
589+
assert r.status_code == 413
590+
assert "Maximum size" in r.json()["detail"]
591+
# Storage layer must NOT have been called — the route should
592+
# reject before bothering Supabase.
593+
up.assert_not_called()
594+
569595

570596
# ── _get_user_or_404 column contract (issue #75) ────────────────────────────
571597

0 commit comments

Comments
 (0)