fix(profile): address code review feedback - early size check, CSRF helper, test constants

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

app/api/profile.py

@@ -231,6 +231,14 @@ async def upload_avatar(
             detail=f"Unsupported image type '{content_type}'. Allowed: JPEG, PNG, GIF, WebP.",
         )
 
+    # Check declared size first (available when the client sends a Content-Length header)
+    if file.size is not None and file.size > _MAX_AVATAR_BYTES:
+        raise HTTPException(
+            status_code=status.HTTP_413_REQUEST_ENTITY_TOO_LARGE,
+            detail="Avatar image must be 2 MB or smaller.",
+        )
+
+    # Read up to one byte past the limit so we can detect oversized uploads
     raw = await file.read(_MAX_AVATAR_BYTES + 1)
     if len(raw) > _MAX_AVATAR_BYTES:
         raise HTTPException(

frontend/templates/profile.html

@@ -77,8 +77,8 @@
       <!-- Upload controls -->
       <div class="flex-1 space-y-3">
         <p class="text-sm text-gray-600 dark:text-gray-400">
-          Upload a JPEG, PNG, GIF, or WebP image up to 2&thinsp;MB.
+          Upload a JPEG, PNG, GIF, or WebP image up to 2 MB.
-          If no custom picture is set, your <a href="https://gravatar.com" class="underline" target="_blank" rel="noopener noreferrer">Gravatar</a> is shown.
+          If no custom picture is set, your <a href="https://gravatar.com" class="underline" target="_blank" rel="noopener noreferrer" aria-label="Gravatar (opens in new tab)">Gravatar</a> is shown.
         </p>
         <label
           for="avatar-input"
@@ -366,15 +366,20 @@ function profileSettings() {
       }
     },
 
+    // ── CSRF helper ────────────────────────────────────────────────────────
+    _getCSRFToken() {
+      return document.cookie
+        .split('; ')
+        .find(row => row.startsWith('csrf_token='))
+        ?.split('=')[1];
+    },
+
     // ── Save general settings ──────────────────────────────────────────────
     async saveProfile() {
       this.saving = true;
       this._hideBanner();
       try {
-        const csrfToken = document.cookie
+        const csrfToken = this._getCSRFToken();
-          .split('; ')
-          .find(row => row.startsWith('csrf_token='))
-          ?.split('=')[1];
 
         const res = await fetch('/api/profile', {
           method: 'PATCH',
@@ -415,10 +420,7 @@ function profileSettings() {
       const formData = new FormData();
       formData.append('file', file);
 
-      const csrfToken = document.cookie
+      const csrfToken = this._getCSRFToken();
-        .split('; ')
-        .find(row => row.startsWith('csrf_token='))
-        ?.split('=')[1];
 
       try {
         const res = await fetch('/api/profile/avatar', {
@@ -446,10 +448,7 @@ function profileSettings() {
     // ── Remove avatar ──────────────────────────────────────────────────────
     async removeAvatar() {
       this._hideBanner();
-      const csrfToken = document.cookie
+      const csrfToken = this._getCSRFToken();
-        .split('; ')
-        .find(row => row.startsWith('csrf_token='))
-        ?.split('=')[1];
 
       try {
         const res = await fetch('/api/profile/avatar', {
@@ -473,10 +472,7 @@ function profileSettings() {
     async changePassword() {
       this.pwSaving = true;
       this._hideBanner();
-      const csrfToken = document.cookie
+      const csrfToken = this._getCSRFToken();
-        .split('; ')
-        .find(row => row.startsWith('csrf_token='))
-        ?.split('=')[1];
 
       try {
         const res = await fetch('/api/profile/change-password', {

tests/test_api_profile.py

@@ -19,6 +19,16 @@ from sqlalchemy.pool import StaticPool
 from app.database import Base, get_db
 from app.models import LocalUser, UserProfile
 
+# ---------------------------------------------------------------------------
+# Test data constants
+# ---------------------------------------------------------------------------
+
+# Minimal valid 1×1 PNG image (base64-encoded) used across avatar upload tests
+_MINIMAL_VALID_PNG_BASE64 = (
+    b"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/Z+hHgAHggJ/PchI6QAAAABJRU5ErkJggg=="
+)
+_MINIMAL_VALID_PNG_BYTES = base64.b64decode(_MINIMAL_VALID_PNG_BASE64)
+
 # ---------------------------------------------------------------------------
 # Fixtures
 # ---------------------------------------------------------------------------
@@ -78,15 +88,15 @@ class TestGravatarUrl:
         from app.api.profile import _gravatar_url
 
         url = _gravatar_url("Test@Example.COM")
-        assert "gravatar.com/avatar/" in url
+        assert url.startswith("https://www.gravatar.com/avatar/")
         assert url.endswith("?d=identicon")
 
     def test_fallback_for_none_email(self):
         from app.api.profile import _gravatar_url
 
         url = _gravatar_url(None)
-        assert "gravatar.com/avatar/" in url
+        assert url.startswith("https://www.gravatar.com/avatar/")
-        assert "?d=identicon" in url
+        assert url.endswith("?d=identicon")
 
 
 @pytest.mark.unit
@@ -164,7 +174,7 @@ class TestGetProfileHandler:
         assert result.display_name == "Alice"
         assert result.preferred_language == "fr"
         assert result.preferred_theme == "dark"
-        assert "gravatar.com" in result.avatar_url
+        assert result.avatar_url.startswith("https://www.gravatar.com/avatar/")
 
     @pytest.mark.asyncio
     async def test_returns_custom_avatar_when_stored(self, prof_session):
@@ -284,13 +294,10 @@ class TestUploadAvatarHandler:
         """upload_avatar stores the image as a data: URI."""
         from app.api.profile import upload_avatar
 
-        png_bytes = base64.b64decode(
-            b"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8/Z+hHgAHggJ/PchI6QAAAABJRU5ErkJggg=="
-        )
-
         upload = MagicMock()
         upload.content_type = "image/png"
-        upload.read = AsyncMock(return_value=png_bytes)
+        upload.size = len(_MINIMAL_VALID_PNG_BYTES)
+        upload.read = AsyncMock(return_value=_MINIMAL_VALID_PNG_BYTES)
 
         req = MagicMock()
         req.session = {"user": {"preferred_username": "avataruser", "email": "av@example.com"}}
@@ -305,6 +312,7 @@ class TestUploadAvatarHandler:
 
         upload = MagicMock()
         upload.content_type = "application/pdf"
+        upload.size = 4
         upload.read = AsyncMock(return_value=b"%PDF")
 
         req = MagicMock()
@@ -319,11 +327,12 @@ class TestUploadAvatarHandler:
         """upload_avatar raises 413 when image exceeds 2 MB."""
         from app.api.profile import upload_avatar
 
-        big_data = b"x" * (2 * 1024 * 1024 + 1)
+        big_size = 2 * 1024 * 1024 + 1
 
         upload = MagicMock()
         upload.content_type = "image/png"
-        upload.read = AsyncMock(return_value=big_data)
+        upload.size = big_size  # triggers early size check
+        upload.read = AsyncMock(return_value=b"x" * big_size)
 
         req = MagicMock()
         req.session = {"user": {"preferred_username": "biguser", "email": "big@example.com"}}
@@ -358,7 +367,7 @@ class TestDeleteAvatarHandler:
         req.session = {"user": {"preferred_username": "delavatar", "email": "del@example.com"}}
 
         result = await delete_avatar(req, prof_session)
-        assert "gravatar.com" in result["avatar_url"]
+        assert result["avatar_url"].startswith("https://www.gravatar.com/avatar/")
 
         row = prof_session.query(UserProfile).filter_by(user_id="delavatar").first()
         assert row.avatar_data is None