diff --git a/backend/routes/feedback.py b/backend/routes/feedback.py index 3987c013..e4f63f02 100644 --- a/backend/routes/feedback.py +++ b/backend/routes/feedback.py @@ -1,10 +1,25 @@ -from fastapi import APIRouter +import logging +import uuid -from db.connection import table +import httpx +from fastapi import APIRouter, File, HTTPException, Request, UploadFile + +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 + +logger = logging.getLogger(__name__) 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 +44,57 @@ 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", + }, + timeout=30.0, + ) + 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 new file mode 100644 index 00000000..a06de51e --- /dev/null +++ b/backend/tests/test_issue_screenshot_auth.py @@ -0,0 +1,75 @@ +""" +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.status_code = 200 + 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() + + 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 8f434101..3ba82603 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"; +import { submitIssueReport, IS_LOCAL_MODE, API_URL } 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_URL}/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; })),