From 6d6226ffe611509a08d1e70ddefb6960574dc08c Mon Sep 17 00:00:00 2001 From: Jose-Gael-Cruz-Lopez Date: Mon, 15 Jun 2026 01:51:32 -0400 Subject: [PATCH 1/3] fix(security): route issue-report screenshots through an auth-gated backend upload (#231 Phase 2a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/routes/feedback.py | 55 +++++++++++++++++++- backend/tests/test_issue_screenshot_auth.py | 57 +++++++++++++++++++++ frontend/src/components/ReportIssueFlow.tsx | 25 +++++---- 3 files changed, 125 insertions(+), 12 deletions(-) create mode 100644 backend/tests/test_issue_screenshot_auth.py diff --git a/backend/routes/feedback.py b/backend/routes/feedback.py index 3987c013..21fbc511 100644 --- a/backend/routes/feedback.py +++ b/backend/routes/feedback.py @@ -1,10 +1,22 @@ -from fastapi import APIRouter +import uuid -from db.connection import table +import httpx +from fastapi import APIRouter, File, HTTPException, Request, UploadFile + +from db.connection import SUPABASE_URL, SUPABASE_KEY, table from models import SubmitFeedbackBody, SubmitIssueReportBody +from services.auth_guard import get_session_user_id +from services.request_limits import read_within_limit router = APIRouter() +# #231: issue-report screenshots upload here. They used to be written by the +# frontend with the public anon key; this endpoint moves the write server-side +# (service role) so the bucket can be made private. +ISSUE_BUCKET = "issues-media-files" +MAX_SCREENSHOT_BYTES = 5 * 1024 * 1024 # 5 MB +ALLOWED_SCREENSHOT_TYPES = {"image/png", "image/jpeg", "image/webp", "image/gif"} + @router.post("/feedback") def submit_feedback(body: SubmitFeedbackBody): @@ -29,3 +41,42 @@ def submit_issue_report(body: SubmitIssueReportBody): "screenshot_urls": body.screenshot_urls, }) return {"ok": True} + + +@router.post("/issue-reports/screenshot") +async def upload_issue_screenshot(request: Request, file: UploadFile = File(...)): + """Auth-gated, server-side upload for issue-report screenshots (#231). + + Replaces the frontend's direct anon-key upload to issues-media-files so the + bucket can be made private (Phase 2b). Validates type + size (the #220/#229 + pattern), uploads with the service role, and returns the storage PATH (not a + public URL); the path is stored in issue_reports.screenshot_urls and reviewed + via the dashboard / a signed URL. + """ + user_id = get_session_user_id(request) # 401 if unauthenticated + if (file.content_type or "") 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" + ) + path = f"{user_id}/{uuid.uuid4()}.{ext}" + url = f"{SUPABASE_URL}/storage/v1/object/{ISSUE_BUCKET}/{path}" + 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() + return {"path": path} diff --git a/backend/tests/test_issue_screenshot_auth.py b/backend/tests/test_issue_screenshot_auth.py new file mode 100644 index 00000000..ede9c004 --- /dev/null +++ b/backend/tests/test_issue_screenshot_auth.py @@ -0,0 +1,57 @@ +""" +Regression tests for #231 Phase 2a: POST /api/issue-reports/screenshot. + +This endpoint moves issue-report screenshot uploads off the frontend's public +anon-key storage client and onto an auth-gated, service-role backend upload so +the issues-media-files bucket can be made private. The negative tests assert it +requires auth (401) and bounds type (415) / size (413); all fail on pre-fix code +(the endpoint didn't exist → 404). +""" +from unittest.mock import patch + +from fastapi.testclient import TestClient + +from main import app + +client = TestClient(app) + + +def _png(nbytes: int = 16): + return {"file": ("shot.png", b"\x89PNG\r\n\x1a\n" + b"0" * nbytes, "image/png")} + + +class TestIssueScreenshotUpload: + def test_unauthenticated_returns_401(self): + from services import auth_guard + + with patch("routes.feedback.get_session_user_id", auth_guard._real_get_session_user_id), \ + patch.object(auth_guard, "_decode_session", auth_guard._real_decode_session): + r = client.post("/api/issue-reports/screenshot", files=_png()) + assert r.status_code == 401 + + def test_wrong_content_type_returns_415(self): + with patch("routes.feedback.httpx") as hx: + r = client.post( + "/api/issue-reports/screenshot", + files={"file": ("notes.txt", b"hello", "text/plain")}, + ) + assert r.status_code == 415 + hx.put.assert_not_called() + + def test_oversize_returns_413(self, monkeypatch): + monkeypatch.setattr("routes.feedback.MAX_SCREENSHOT_BYTES", 100) + with patch("routes.feedback.httpx") as hx: + r = client.post("/api/issue-reports/screenshot", files=_png(nbytes=200)) + assert r.status_code == 413 + hx.put.assert_not_called() + + def test_valid_upload_returns_path_scoped_to_user(self): + with patch("routes.feedback.httpx") as hx: + hx.put.return_value.raise_for_status.return_value = None + r = client.post("/api/issue-reports/screenshot", files=_png()) + assert r.status_code == 200 + path = r.json()["path"] + # Path is scoped under the authenticated user_id (conftest stub: user_andres). + assert path.startswith("user_andres/") + assert path.endswith(".png") + hx.put.assert_called_once() diff --git a/frontend/src/components/ReportIssueFlow.tsx b/frontend/src/components/ReportIssueFlow.tsx index 8f434101..aa7ea45a 100644 --- a/frontend/src/components/ReportIssueFlow.tsx +++ b/frontend/src/components/ReportIssueFlow.tsx @@ -6,13 +6,11 @@ import { Pill } from "./Pill"; import { useToast } from "./ToastProvider"; import { useUser } from "@/context/UserContext"; import { useBodyScrollLock } from "@/lib/useBodyScrollLock"; -import { supabase } from "@/lib/supabase"; import { submitIssueReport, IS_LOCAL_MODE } from "@/lib/api"; const TOPICS = ["Bug", "Feature", "Polish", "Content", "Other"] as const; type Topic = typeof TOPICS[number]; -const BUCKET = "issues-media-files"; const MAX_SCREENSHOTS = 5; const MAX_BYTES = 5 * 1024 * 1024; @@ -21,14 +19,21 @@ interface ReportIssueFlowProps { onClose: () => void; } -async function uploadScreenshot(userId: string, file: File): Promise { +// #231: upload via the auth-gated backend endpoint (service-role) instead of the +// public anon storage client, so issues-media-files can be made private. Returns +// the storage path (stored in screenshot_urls), not a public URL. +async function uploadScreenshot(file: File): Promise { if (IS_LOCAL_MODE) return URL.createObjectURL(file); - const ext = file.name.split(".").pop() || "png"; - const path = `${userId}/${Date.now()}-${Math.random().toString(36).slice(2, 8)}.${ext}`; - const { error } = await supabase.storage.from(BUCKET).upload(path, file, { cacheControl: "3600", upsert: false }); - if (error) throw error; - const { data } = supabase.storage.from(BUCKET).getPublicUrl(path); - return data.publicUrl; + const form = new FormData(); + form.append("file", file); + const res = await fetch("/api/issue-reports/screenshot", { + method: "POST", + credentials: "include", + body: form, + }); + if (!res.ok) throw new Error(`Screenshot upload failed (${res.status})`); + const data = (await res.json()) as { path: string }; + return data.path; } export function ReportIssueFlow({ open, onClose }: ReportIssueFlowProps) { @@ -93,7 +98,7 @@ export function ReportIssueFlow({ open, onClose }: ReportIssueFlowProps) { setSubmitting(true); try { const urls = await Promise.all( - screenshots.map(s => uploadScreenshot(userId, s.file).catch(err => { + screenshots.map(s => uploadScreenshot(s.file).catch(err => { toast.error(`Screenshot upload failed: ${err?.message || "unknown"}`); return null; })), From 1807a911ea367ff71aaa3a2c234446b2b77b5474 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Tue, 16 Jun 2026 21:16:32 -0400 Subject: [PATCH 2/3] Fix #231 Phase 2a screenshot upload: API_URL prefix, 502 error mapping, timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- backend/routes/feedback.py | 20 +++++++++++++++++++- backend/tests/test_issue_screenshot_auth.py | 20 +++++++++++++++++++- frontend/src/components/ReportIssueFlow.tsx | 4 ++-- 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/backend/routes/feedback.py b/backend/routes/feedback.py index 21fbc511..c519a313 100644 --- a/backend/routes/feedback.py +++ b/backend/routes/feedback.py @@ -1,3 +1,4 @@ +import logging import uuid import httpx @@ -8,6 +9,8 @@ from services.auth_guard import get_session_user_id from services.request_limits import read_within_limit +logger = logging.getLogger(__name__) + router = APIRouter() # #231: issue-report screenshots upload here. They used to be written by the @@ -77,6 +80,21 @@ async def upload_issue_screenshot(request: Request, file: UploadFile = File(...) "Authorization": f"Bearer {SUPABASE_KEY}", "Content-Type": file.content_type or "application/octet-stream", }, + timeout=30.0, ) - r.raise_for_status() + if r.status_code not in (200, 201): + # Surface the real Supabase response so the failure is debuggable + # without server access (mirrors storage_service.upload_avatar). We + # only show the truncated upstream body — never the URL or headers + # (the latter contains the service-role key). + body_text = (r.text or "").strip()[:500] + logger.warning( + "upload_issue_screenshot: Supabase storage rejected upload " + "user=%s status=%d body=%s", + user_id, r.status_code, body_text, + ) + raise HTTPException( + status_code=502, + detail=f"Screenshot upload failed (Supabase {r.status_code}): {body_text or 'no body'}", + ) return {"path": path} diff --git a/backend/tests/test_issue_screenshot_auth.py b/backend/tests/test_issue_screenshot_auth.py index ede9c004..a06de51e 100644 --- a/backend/tests/test_issue_screenshot_auth.py +++ b/backend/tests/test_issue_screenshot_auth.py @@ -47,7 +47,7 @@ def test_oversize_returns_413(self, monkeypatch): def test_valid_upload_returns_path_scoped_to_user(self): with patch("routes.feedback.httpx") as hx: - hx.put.return_value.raise_for_status.return_value = None + hx.put.return_value.status_code = 200 r = client.post("/api/issue-reports/screenshot", files=_png()) assert r.status_code == 200 path = r.json()["path"] @@ -55,3 +55,21 @@ def test_valid_upload_returns_path_scoped_to_user(self): assert path.startswith("user_andres/") assert path.endswith(".png") hx.put.assert_called_once() + + def test_supabase_failure_returns_502_not_500(self): + # When Supabase Storage rejects the upload (e.g. bucket missing / + # RLS denied), the endpoint maps it to a 502 with the truncated + # upstream body — never a generic 500. (#231 review fix.) + with patch("routes.feedback.httpx") as hx: + hx.put.return_value.status_code = 404 + hx.put.return_value.text = '{"statusCode":"404","error":"Bucket not found"}' + r = client.post("/api/issue-reports/screenshot", files=_png()) + assert r.status_code == 502 + detail = r.json()["detail"] + # Upstream status + body are surfaced for debuggability... + assert "404" in detail + assert "Bucket not found" in detail + # ...but the service-role key (in headers) and upload URL must not leak. + assert "Authorization" not in detail + assert "storage/v1/object" not in detail + hx.put.assert_called_once() diff --git a/frontend/src/components/ReportIssueFlow.tsx b/frontend/src/components/ReportIssueFlow.tsx index aa7ea45a..3ba82603 100644 --- a/frontend/src/components/ReportIssueFlow.tsx +++ b/frontend/src/components/ReportIssueFlow.tsx @@ -6,7 +6,7 @@ import { Pill } from "./Pill"; import { useToast } from "./ToastProvider"; import { useUser } from "@/context/UserContext"; import { useBodyScrollLock } from "@/lib/useBodyScrollLock"; -import { submitIssueReport, IS_LOCAL_MODE } from "@/lib/api"; +import { submitIssueReport, IS_LOCAL_MODE, API_URL } from "@/lib/api"; const TOPICS = ["Bug", "Feature", "Polish", "Content", "Other"] as const; type Topic = typeof TOPICS[number]; @@ -26,7 +26,7 @@ async function uploadScreenshot(file: File): Promise { if (IS_LOCAL_MODE) return URL.createObjectURL(file); const form = new FormData(); form.append("file", file); - const res = await fetch("/api/issue-reports/screenshot", { + const res = await fetch(`${API_URL}/api/issue-reports/screenshot`, { method: "POST", credentials: "include", body: form, From 500c0255be21eb343773f3b049d27d58ec7e4f71 Mon Sep 17 00:00:00 2001 From: Jose Cruz Date: Tue, 16 Jun 2026 21:46:48 -0400 Subject: [PATCH 3/3] style(feedback): sort db.connection import members (ruff isort) Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/routes/feedback.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/routes/feedback.py b/backend/routes/feedback.py index c519a313..e4f63f02 100644 --- a/backend/routes/feedback.py +++ b/backend/routes/feedback.py @@ -4,7 +4,7 @@ import httpx from fastapi import APIRouter, File, HTTPException, Request, UploadFile -from db.connection import SUPABASE_URL, SUPABASE_KEY, table +from db.connection import SUPABASE_KEY, SUPABASE_URL, table from models import SubmitFeedbackBody, SubmitIssueReportBody from services.auth_guard import get_session_user_id from services.request_limits import read_within_limit