Uh oh!
There was an error while loading. Please reload this page.
fix(security): issue-report screenshots via auth-gated backend upload (#231 Phase 2a) - #239
Conversation
…ackend upload (#231 Phase 2a) Moves issue-report screenshot uploads off the frontend's public anon-key storage client onto an auth-gated, service-role backend endpoint so issues-media-files can be made private (Phase 2b). - Backend: POST /api/issue-reports/screenshot — get_session_user_id (401), content-type allowlist (415) + bounded read (413) via request_limits, uploads with the service role, returns the storage PATH. - Frontend: ReportIssueFlow uploads via the endpoint (credentials:include) and stores the path; drops the supabase anon storage client (removes the last live frontend anon-storage path). - Test: unauth 401, wrong-type 415, oversize 413, valid 200 (path scoped to the authed user). All fail pre-fix (endpoint didn't exist). Note: issue screenshots have NO in-app reader (write-only, like résumés), so no signed-URL read endpoint is included — it would have no consumer; review is via the dashboard. screenshot_urls now stores paths for new reports.
Warning Review limit reached
More reviews will be available in 29 minutes and 46 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughMoves issue-report screenshot uploads from direct Supabase client-side uploads to a new authenticated server-side FastAPI endpoint ( ChangesScreenshot Upload via Backend Endpoint
Sequence DiagramsequenceDiagram
participant Browser as Browser (ReportIssueFlow)
participant API as FastAPI /issue-reports/screenshot
participant Supabase as Supabase Storage
Browser->>API: POST /api/issue-reports/screenshot (UploadFile + session cookie)
API->>API: decode session → user_id (401 if missing)
API->>API: check content_type vs allowlist (415 on mismatch)
API->>API: read body ≤ 5 MB (413 on oversize, 400 if empty)
API->>Supabase: PUT {user_id}/screenshots/{uuid}.{ext} with service role key
Supabase-->>API: 200 OK
API-->>Browser: {"path": "{user_id}/screenshots/{uuid}.{ext}"}
Browser->>Browser: use path as screenshot_urls in issue report submission
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 500c025 | Commit Preview URL Branch Preview URL | Jun 17 2026, 01:48 AM |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/routes/feedback.py (1)
78-78: 💤 Low valueDead-code fallback:
file.content_typeis already validated non-None.Line 57 rejects requests where
(file.content_type or "")is not inALLOWED_SCREENSHOT_TYPES, so by the time line 78 executes,file.content_typeis guaranteed to be a valid string from the allowlist. Theor "application/octet-stream"fallback is unreachable.🧹 Simplify to reflect the invariant
- "Content-Type": file.content_type or "application/octet-stream",+ "Content-Type": file.content_type,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/feedback.py` at line 78, The `or "application/octet-stream"` fallback in the Content-Type header assignment is unreachable dead code because the validation at line 57 already ensures that `file.content_type` is a non-None value from the ALLOWED_SCREENSHOT_TYPES allowlist. Remove the fallback operator and simplify the expression to use `file.content_type` directly, reflecting the guaranteed invariant at that point in the code.backend/tests/test_issue_screenshot_auth.py (1)
48-57: 💤 Low valueTest will break if the async httpx fix is applied.
If the endpoint is updated to use
httpx.AsyncClient(as suggested for the blocking I/O issue), this mock will need adjustment sincepatch("routes.feedback.httpx")mocks the synchronous module-level usage.For an
AsyncClientrefactor, you'd mock the async context manager and its methods. No action needed now, but keep this in mind when applying the async fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_issue_screenshot_auth.py` around lines 48 - 57, The current test mocks the synchronous httpx module-level usage. When the endpoint is refactored to use httpx.AsyncClient, this test will need to be updated to properly mock the async context manager and its async methods. Specifically, update the patch target from the module-level httpx to the AsyncClient class, mock the async context manager's __aenter__ and __aexit__ methods, and ensure the mocked put method is set up as a coroutine or async mock that can be awaited within the async context. This adjustment will be necessary once the blocking I/O issue is addressed and httpx.AsyncClient is introduced in the routes.feedback module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/routes/feedback.py`:
- Around line 72-81: Replace the synchronous `httpx.put()` call with an async
equivalent to prevent blocking the event loop. Use `httpx.AsyncClient()` with
`await` to make the call non-blocking, and add a timeout parameter to the
request to prevent indefinite hangs if Supabase Storage is unresponsive. You can
either create a new AsyncClient for each request or follow the module-level
client pattern (similar to the sync `_client` pattern mentioned in the comment)
by instantiating an async client at import time and reusing it across requests
for better performance.
---
Nitpick comments:
In `@backend/routes/feedback.py`:
- Line 78: The `or "application/octet-stream"` fallback in the Content-Type
header assignment is unreachable dead code because the validation at line 57
already ensures that `file.content_type` is a non-None value from the
ALLOWED_SCREENSHOT_TYPES allowlist. Remove the fallback operator and simplify
the expression to use `file.content_type` directly, reflecting the guaranteed
invariant at that point in the code.
In `@backend/tests/test_issue_screenshot_auth.py`:
- Around line 48-57: The current test mocks the synchronous httpx module-level
usage. When the endpoint is refactored to use httpx.AsyncClient, this test will
need to be updated to properly mock the async context manager and its async
methods. Specifically, update the patch target from the module-level httpx to
the AsyncClient class, mock the async context manager's __aenter__ and __aexit__
methods, and ensure the mocked put method is set up as a coroutine or async mock
that can be awaited within the async context. This adjustment will be necessary
once the blocking I/O issue is addressed and httpx.AsyncClient is introduced in
the routes.feedback module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10aca5a2-e177-41c4-a2f0-6de68c683b63
📒 Files selected for processing (3)
backend/routes/feedback.pybackend/tests/test_issue_screenshot_auth.pyfrontend/src/components/ReportIssueFlow.tsx
| r = httpx.put( | ||
| url, | ||
| content=content, | ||
| headers={ | ||
| "apikey": SUPABASE_KEY, | ||
| "Authorization": f"Bearer {SUPABASE_KEY}", | ||
| "Content-Type": file.content_type or "application/octet-stream", | ||
| }, | ||
| ) | ||
| r.raise_for_status() |
There was a problem hiding this comment.
Synchronous httpx.put blocks the async event loop.
httpx.put() is a synchronous call inside an async def function. Under concurrent load, each upload blocks the entire event loop until the Supabase round-trip completes, starving other requests.
Additionally, no timeout is specified—if Supabase Storage is slow or unresponsive, the call can hang indefinitely.
🔧 Proposed fix using async httpx with timeout
- r = httpx.put(- url,- content=content,- headers={- "apikey": SUPABASE_KEY,- "Authorization": f"Bearer {SUPABASE_KEY}",- "Content-Type": file.content_type or "application/octet-stream",- },- )- r.raise_for_status()+ async with httpx.AsyncClient(timeout=30.0) as http:+ r = await http.put(+ url,+ content=content,+ headers={+ "apikey": SUPABASE_KEY,+ "Authorization": f"Bearer {SUPABASE_KEY}",+ "Content-Type": file.content_type, # already validated non-None+ },+ )+ r.raise_for_status()Alternatively, if you prefer to reuse a module-level async client (similar to the sync _client pattern in connection.py), you could instantiate one at import time and reuse it across requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/feedback.py` around lines 72 - 81, Replace the synchronous
`httpx.put()` call with an async equivalent to prevent blocking the event loop.
Use `httpx.AsyncClient()` with `await` to make the call non-blocking, and add a
timeout parameter to the request to prevent indefinite hangs if Supabase Storage
is unresponsive. You can either create a new AsyncClient for each request or
follow the module-level client pattern (similar to the sync `_client` pattern
mentioned in the comment) by instantiating an async client at import time and
reusing it across requests for better performance.
…g, timeout Address PR #239 review: - ReportIssueFlow.tsx: prefix screenshot upload fetch with API_URL (frontend and backend are separate origins; bare path hit the frontend host). - routes/feedback.py: replace r.raise_for_status() with the established storage_service.upload_avatar pattern — check status, log, and raise HTTPException(502) with a truncated upstream body (no URL/headers, which carry the service-role key) instead of a generic 500. - routes/feedback.py: add timeout=30.0 to the httpx.put (matches db client). - test_issue_screenshot_auth.py: add Supabase-failure test asserting 502 (not 500) and that the key/URL don't leak; update the success-path mock to set status_code=200 now that raise_for_status is no longer used. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/routes/feedback.py (3)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDerive the object extension from the validated MIME type.
file.filenameis user-controlled; copying its suffix can produce mismatched paths or URL-significant characters. Since the MIME type is already allowlisted, map it to a known extension.🐛 Proposed fix
-ALLOWED_SCREENSHOT_TYPES = {"image/png", "image/jpeg", "image/webp", "image/gif"}+SCREENSHOT_EXTENSIONS = {+ "image/png": "png",+ "image/jpeg": "jpg",+ "image/webp": "webp",+ "image/gif": "gif",+}+ALLOWED_SCREENSHOT_TYPES = set(SCREENSHOT_EXTENSIONS)- if (file.content_type or "") not in ALLOWED_SCREENSHOT_TYPES:+ content_type = file.content_type or ""+ if content_type not in ALLOWED_SCREENSHOT_TYPES: raise HTTPException( status_code=415, detail="Unsupported image type. Allowed: PNG, JPEG, WEBP, GIF.", ) content = await read_within_limit(file, MAX_SCREENSHOT_BYTES) # 413 if oversize if not content: raise HTTPException(status_code=400, detail="Empty file.") - ext = (- file.filename.rsplit(".", 1)[-1]- if file.filename and "." in file.filename- else "png"- )+ ext = SCREENSHOT_EXTENSIONS[content_type]Also applies to: 60-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/feedback.py` at line 21, The issue is that file.filename is user-controlled and using its extension directly can produce mismatched file paths or unexpected characters. Since the MIME type is already validated against the ALLOWED_SCREENSHOT_TYPES allowlist, create a mapping from MIME types to their corresponding file extensions (e.g., image/png to .png, image/jpeg to .jpg, etc.). Then, wherever the file extension is used (in the code around lines 60-73 where file saving occurs), derive it from the validated MIME type using this mapping instead of extracting it from the user-provided file.filename.
74-83: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftKeep service-role storage writes behind the Supabase access boundary.
This route composes the Supabase Storage URL and sends
SUPABASE_KEYdirectly from route code. Move the upload into the approved DB/storage boundary, or add an approved helper there, so service-role handling stays centralized.As per coding guidelines, "All Supabase access must go through
backend/db/connection.py::table(). Do not instantiatehttpxclients or importsupabasedirectly elsewhere."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/feedback.py` around lines 74 - 83, The direct httpx.put call to SUPABASE_URL with SUPABASE_KEY in the feedback route violates the approved Supabase access pattern. Move the file upload logic (the httpx.put call that constructs the Supabase Storage URL and sends the SUPABASE_KEY) from this route into a dedicated helper function in backend/db/connection.py, similar to the table() interface. Then replace the httpx.put code in the feedback route with a call to this new helper function, ensuring all service-role storage operations are centralized and go through the approved connection boundary.Source: Coding guidelines
75-99:⚠️ Potential issue | 🟠 MajorMap transport failures to 502 too.
timeout=30.0now bounds the call, but timeout/connect/DNS failures raise beforerexists, bypassing the status-code mapping and surfacing as 500s.🐛 Proposed fix
- r = httpx.put(- url,- content=content,- headers={- "apikey": SUPABASE_KEY,- "Authorization": f"Bearer {SUPABASE_KEY}",- "Content-Type": file.content_type or "application/octet-stream",- },- timeout=30.0,- )+ try:+ r = httpx.put(+ url,+ content=content,+ headers={+ "apikey": SUPABASE_KEY,+ "Authorization": f"Bearer {SUPABASE_KEY}",+ "Content-Type": file.content_type or "application/octet-stream",+ },+ timeout=30.0,+ )+ except httpx.RequestError as exc:+ logger.warning(+ "upload_issue_screenshot: Supabase storage request failed "+ "user=%s error=%s",+ user_id,+ exc.__class__.__name__,+ )+ raise HTTPException(+ status_code=502,+ detail="Screenshot upload failed: storage service unavailable.",+ ) from exc if r.status_code not in (200, 201):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/feedback.py` around lines 75 - 99, The current error handling in the upload screenshot endpoint only catches HTTP status code failures from the httpx.put call, but transport-level failures such as timeouts, connection errors, or DNS failures will raise exceptions before the response object r is created, bypassing the error mapping and surfacing as 500 errors instead of 502s. Wrap the httpx.put call in a try-except block to catch httpx transport exceptions (such as TimeoutException, ConnectError, and similar) and raise the same HTTPException with status_code 502, following the same pattern as the existing status code check, to ensure all Supabase storage failures consistently map to 502 responses.frontend/src/components/ReportIssueFlow.tsx (1)
63-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the backend screenshot MIME allowlist in the picker.
The UI accepts any
image/*, but the backend only accepts PNG, JPEG, WEBP, and GIF. SVG/HEIC/BMP files pass client validation and then fail during submit with 415.🐛 Proposed fix
const MAX_SCREENSHOTS = 5; const MAX_BYTES = 5 * 1024 * 1024; +const ALLOWED_SCREENSHOT_TYPES = new Set(["image/png", "image/jpeg", "image/webp", "image/gif"]);+const ACCEPTED_SCREENSHOT_TYPES = "image/png,image/jpeg,image/webp,image/gif";- if (!f.type.startsWith("image/")) {- toast.error(`${f.name} isn't an image`);+ if (!ALLOWED_SCREENSHOT_TYPES.has(f.type)) {+ toast.error(`${f.name} must be PNG, JPEG, WEBP, or GIF`); return; }- accept="image/*"+ accept={ACCEPTED_SCREENSHOT_TYPES}Also applies to: 240-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/ReportIssueFlow.tsx` around lines 63 - 77, The addFiles function currently validates image files too broadly by accepting any f.type that starts with "image/", but the backend only accepts PNG, JPEG, WEBP, and GIF formats. Update the MIME type validation check (currently checking f.type.startsWith("image/")) to instead validate against a specific allowlist of only the backend-supported formats: image/png, image/jpeg, image/webp, and image/gif. This will prevent non-supported image types like SVG, HEIC, and BMP from passing client validation and subsequently failing with a 415 error during submission. Apply the same fix to the additional validation mentioned at lines 240-246.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/routes/feedback.py`:
- Line 21: The issue is that file.filename is user-controlled and using its
extension directly can produce mismatched file paths or unexpected characters.
Since the MIME type is already validated against the ALLOWED_SCREENSHOT_TYPES
allowlist, create a mapping from MIME types to their corresponding file
extensions (e.g., image/png to .png, image/jpeg to .jpg, etc.). Then, wherever
the file extension is used (in the code around lines 60-73 where file saving
occurs), derive it from the validated MIME type using this mapping instead of
extracting it from the user-provided file.filename.
- Around line 74-83: The direct httpx.put call to SUPABASE_URL with SUPABASE_KEY
in the feedback route violates the approved Supabase access pattern. Move the
file upload logic (the httpx.put call that constructs the Supabase Storage URL
and sends the SUPABASE_KEY) from this route into a dedicated helper function in
backend/db/connection.py, similar to the table() interface. Then replace the
httpx.put code in the feedback route with a call to this new helper function,
ensuring all service-role storage operations are centralized and go through the
approved connection boundary.
- Around line 75-99: The current error handling in the upload screenshot
endpoint only catches HTTP status code failures from the httpx.put call, but
transport-level failures such as timeouts, connection errors, or DNS failures
will raise exceptions before the response object r is created, bypassing the
error mapping and surfacing as 500 errors instead of 502s. Wrap the httpx.put
call in a try-except block to catch httpx transport exceptions (such as
TimeoutException, ConnectError, and similar) and raise the same HTTPException
with status_code 502, following the same pattern as the existing status code
check, to ensure all Supabase storage failures consistently map to 502
responses.
In `@frontend/src/components/ReportIssueFlow.tsx`:
- Around line 63-77: The addFiles function currently validates image files too
broadly by accepting any f.type that starts with "image/", but the backend only
accepts PNG, JPEG, WEBP, and GIF formats. Update the MIME type validation check
(currently checking f.type.startsWith("image/")) to instead validate against a
specific allowlist of only the backend-supported formats: image/png, image/jpeg,
image/webp, and image/gif. This will prevent non-supported image types like SVG,
HEIC, and BMP from passing client validation and subsequently failing with a 415
error during submission. Apply the same fix to the additional validation
mentioned at lines 240-246.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8035ad70-4236-4eb7-afef-d338421399e4
📒 Files selected for processing (3)
backend/routes/feedback.pybackend/tests/test_issue_screenshot_auth.pyfrontend/src/components/ReportIssueFlow.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/test_issue_screenshot_auth.py
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Phase 2a of #231 storage hardening — the app change that must deploy before flipping
issues-media-filesprivate (2b).What
POST /api/issue-reports/screenshot:get_session_user_id(401 unauth) + content-type allowlist (415) + bounded read (413,request_limits.read_within_limit), uploads with the service role, returns the storage path.ReportIssueFlow: uploads via the endpoint (credentials:'include'), stores the path; drops thesupabaseanon storage client — removing the last live frontend anon-storage path.test_issue_screenshot_auth.py: 401 / 415 / 413 / 200(path scoped to user). All fail pre-fix (endpoint didn't exist).While building, I confirmed there is no in-app reader for issue screenshots —
feedback.pyonly INSERTs, no GET/admin view, nothing rendersscreenshot_urls. It's write-only collected, exactly like résumés. So I did not build a signed-URL read endpoint: it would have no consumer (dead code). Review is via the Supabase dashboard (works on private buckets).That changes the gate you set: there are no in-app screenshots to "render," so the pre-2b check isn't "confirm screenshots render" — it's "confirm a new issue-report submission with a screenshot succeeds via the backend in prod." If you'd rather have an in-app admin review view now (which would give the signed-URL read a real consumer), say so and I'll add it; otherwise this matches the résumé decision (dashboard review, deferred admin view).
Sequence
issues-media-filesprivate + drop its two{public}policies). Existing 2 rows' public URLs stop resolving → dashboard review (same as résumés).Summary by CodeRabbit