From 7dc0bb556c38fc618bdb7a3fc4bac207a5de41d1 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Sat, 25 Jul 2026 13:44:02 +0700 Subject: [PATCH] fix: address code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - imaging: downscale to 1280px before JPEG quality-stepping so real camera photos fit the 200KB cap (was quality-only → rejected typical photos) - billing: save_prescription/save_checkup_services refuse to modify a PAID invoice, so recorded payment can't diverge from the invoice total - staff: set_staff_role/remove_staff RPCs hold an advisory lock while enforcing the last-admin invariant — race-safe vs the prior check-then-act - imaging delete: remove the object at the row's stored path (DB lookup), not a client-supplied path - invoice PDF: filter deleted=false like every other checkup view - sign-in: skip rate limit when no client IP is resolvable instead of bucketing all requests under a shared "unknown" key - reminders: bound to a [today-30d, today+7d] window so stale overdue rows don't accumulate and bury upcoming ones --- app/[locale]/(app)/admin/staff/actions.ts | 53 ++---- .../(app)/checkups/[id]/imaging/actions.ts | 17 +- .../checkups/[id]/imaging/image-capture.tsx | 38 +++-- .../(app)/checkups/[id]/invoice/route.ts | 7 +- app/[locale]/(app)/reminders/page.tsx | 5 +- app/[locale]/(auth)/sign-in/actions.ts | 28 ++-- .../20260725140000_bsk_review_fixes.sql | 153 ++++++++++++++++++ types/supabase-bsk.ts | 8 + 8 files changed, 240 insertions(+), 69 deletions(-) create mode 100644 supabase/migrations/20260725140000_bsk_review_fixes.sql diff --git a/app/[locale]/(app)/admin/staff/actions.ts b/app/[locale]/(app)/admin/staff/actions.ts index 6696931..92474bf 100644 --- a/app/[locale]/(app)/admin/staff/actions.ts +++ b/app/[locale]/(app)/admin/staff/actions.ts @@ -2,20 +2,19 @@ /** * Staff management Server Actions (admin only). The invite flow remains the - * create path; here an admin can change a member's role or revoke access - * (delete the bsk.app_users enrollment — auth.users is untouched). + * create path; here an admin can change a member's role or revoke access. * - * Writes use the admin (service_role) client, consistent with the invite flow - * and the app_users least-privilege posture (authenticated has no direct write). - * Guards: an admin cannot change/remove their OWN row, and the LAST admin - * cannot be demoted or removed (avoids locking everyone out). + * Mutations go through the set_staff_role / remove_staff RPCs (SECURITY DEFINER, + * gated on the caller's current_role() via the USER client) which hold an + * advisory lock while enforcing the "keep >= 1 admin" and "no self-change" + * invariants — race-safe, unlike a check-then-act in app code. */ import { getLocale } from "next-intl/server"; import { revalidatePath } from "next/cache"; import { getServerSession } from "@/lib/auth/get-server-session"; -import { createSupabaseAdminClient } from "@/lib/supabase/admin"; +import { createSupabaseServerClient } from "@/lib/supabase/server"; import { isAppRole } from "@/lib/db/roles"; async function revalidateStaff() { @@ -23,40 +22,18 @@ async function revalidateStaff() { revalidatePath(`/${locale}/admin/staff`); } -/** True if removing/demoting this user would leave zero admins. */ -async function isLastAdmin( - admin: ReturnType, - userId: string, -): Promise { - const { data: target } = await admin - .from("app_users") - .select("role") - .eq("user_id", userId) - .maybeSingle(); - if (target?.role !== "admin") return false; - - const { count } = await admin - .from("app_users") - .select("user_id", { count: "exact", head: true }) - .eq("role", "admin"); - return (count ?? 0) <= 1; -} - export async function updateStaffRoleAction(formData: FormData): Promise { const session = await getServerSession(); if (session?.role !== "admin") return; const userId = String(formData.get("userId") ?? ""); const role = String(formData.get("role") ?? ""); - if (!userId || !isAppRole(role)) return; - if (userId === session.user.id) return; // no self role-change + if (!userId || !isAppRole(role) || userId === session.user.id) return; - const admin = createSupabaseAdminClient(); - if (role !== "admin" && (await isLastAdmin(admin, userId))) return; // keep >=1 admin - - const { error } = await admin.from("app_users").update({ role }).eq("user_id", userId); + const supabase = await createSupabaseServerClient(); + const { error } = await supabase.rpc("set_staff_role", { p_user_id: userId, p_role: role }); if (!error) { - await admin.rpc("log_audit", { + await supabase.rpc("log_audit", { p_action: "staff.role_change", p_entity: "app_users", p_entity_id: userId, @@ -71,14 +48,12 @@ export async function removeStaffAction(formData: FormData): Promise { if (session?.role !== "admin") return; const userId = String(formData.get("userId") ?? ""); - if (!userId || userId === session.user.id) return; // no self removal + if (!userId || userId === session.user.id) return; - const admin = createSupabaseAdminClient(); - if (await isLastAdmin(admin, userId)) return; // never remove the last admin - - const { error } = await admin.from("app_users").delete().eq("user_id", userId); + const supabase = await createSupabaseServerClient(); + const { error } = await supabase.rpc("remove_staff", { p_user_id: userId }); if (!error) { - await admin.rpc("log_audit", { + await supabase.rpc("log_audit", { p_action: "staff.remove", p_entity: "app_users", p_entity_id: userId, diff --git a/app/[locale]/(app)/checkups/[id]/imaging/actions.ts b/app/[locale]/(app)/checkups/[id]/imaging/actions.ts index e842e65..eb3a18f 100644 --- a/app/[locale]/(app)/checkups/[id]/imaging/actions.ts +++ b/app/[locale]/(app)/checkups/[id]/imaging/actions.ts @@ -81,6 +81,17 @@ export async function deleteImageAction( const supabase = await createSupabaseServerClient(); + // Look up the row's ACTUAL storage_path (scoped by id + checkup_id) — never + // trust the client-supplied path, so a scripted call can't remove another + // checkup's object. + const { data: row } = await supabase + .from("checkup_images") + .select("storage_path") + .eq("id", parsed.data.imageId) + .eq("checkup_id", parsed.data.checkupId) + .maybeSingle(); + if (!row) return { status: "error", message: t("errorGeneric") }; + const { error: updateError } = await supabase .from("checkup_images") .update({ deleted: true }) @@ -89,9 +100,9 @@ export async function deleteImageAction( if (updateError) return { status: "error", message: t("errorGeneric") }; // Best-effort object removal — metadata is already soft-deleted so the - // gallery hides it regardless; a future retention sweep (Phase 7) reconciles - // any orphaned object left behind by a failed removal here. - await supabase.storage.from(CHECKUP_MEDIA_BUCKET).remove([parsed.data.storagePath]); + // gallery hides it regardless; the nightly retention sweep reconciles any + // orphaned object left behind by a failed removal here. + await supabase.storage.from(CHECKUP_MEDIA_BUCKET).remove([row.storage_path]); await supabase.rpc("log_audit", { p_action: "image.delete", diff --git a/app/[locale]/(app)/checkups/[id]/imaging/image-capture.tsx b/app/[locale]/(app)/checkups/[id]/imaging/image-capture.tsx index 9253532..951d0aa 100644 --- a/app/[locale]/(app)/checkups/[id]/imaging/image-capture.tsx +++ b/app/[locale]/(app)/checkups/[id]/imaging/image-capture.tsx @@ -25,6 +25,22 @@ import { CHECKUP_MEDIA_BUCKET, MAX_IMAGE_BYTES, buildStoragePath } from "@/lib/i import { recordImageAction } from "./actions"; const QUALITY_STEPS = [0.9, 0.8, 0.7, 0.6, 0.5, 0.4] as const; +// Downscale the longest side to this before quality-stepping — a full-res phone +// photo (e.g. 4000×3000) never fits 200KB on JPEG quality alone, so we must +// reduce resolution first. 1280px keeps ultrasound/clinic detail legible. +const MAX_DIMENSION = 1280; + +/** Draw a source (video frame or image) onto a canvas scaled to fit MAX_DIMENSION. */ +function makeScaledCanvas(source: CanvasImageSource, w: number, h: number): HTMLCanvasElement | null { + const scale = Math.min(1, MAX_DIMENSION / Math.max(w, h)); + const canvas = document.createElement("canvas"); + canvas.width = Math.max(1, Math.round(w * scale)); + canvas.height = Math.max(1, Math.round(h * scale)); + const ctx = canvas.getContext("2d"); + if (!ctx) return null; + ctx.drawImage(source, 0, 0, canvas.width, canvas.height); + return canvas; +} // getUserMedia support never changes after mount, so there's nothing to // subscribe to — this is purely a way to read a browser-only value without @@ -55,7 +71,6 @@ export function ImageCapture({ checkupId }: { checkupId: number }) { const router = useRouter(); const videoRef = useRef(null); - const canvasRef = useRef(null); const streamRef = useRef(null); // Browser-only feature detection without an Effect: getServerSnapshot @@ -138,12 +153,11 @@ export function ImageCapture({ checkupId }: { checkupId: number }) { function snapshot() { const video = videoRef.current; if (!video || !video.videoWidth) return; - const canvas = canvasRef.current ?? document.createElement("canvas"); - canvas.width = video.videoWidth; - canvas.height = video.videoHeight; - const ctx = canvas.getContext("2d"); - if (!ctx) return; - ctx.drawImage(video, 0, 0, canvas.width, canvas.height); + const canvas = makeScaledCanvas(video, video.videoWidth, video.videoHeight); + if (!canvas) { + setError(t("errorGeneric")); + return; + } void uploadFromCanvas(canvas); } @@ -155,16 +169,12 @@ export function ImageCapture({ checkupId }: { checkupId: number }) { const img = new Image(); const objectUrl = URL.createObjectURL(file); img.onload = () => { - const canvas = document.createElement("canvas"); - canvas.width = img.naturalWidth; - canvas.height = img.naturalHeight; - const ctx = canvas.getContext("2d"); + const canvas = makeScaledCanvas(img, img.naturalWidth, img.naturalHeight); URL.revokeObjectURL(objectUrl); - if (!ctx) { + if (!canvas) { setError(t("errorGeneric")); return; } - ctx.drawImage(img, 0, 0); void uploadFromCanvas(canvas); }; img.onerror = () => { @@ -220,8 +230,6 @@ export function ImageCapture({ checkupId }: { checkupId: number }) { {uploading &&

{t("uploading")}

} - - {error && (

{error} diff --git a/app/[locale]/(app)/checkups/[id]/invoice/route.ts b/app/[locale]/(app)/checkups/[id]/invoice/route.ts index 71d368d..d21d48a 100644 --- a/app/[locale]/(app)/checkups/[id]/invoice/route.ts +++ b/app/[locale]/(app)/checkups/[id]/invoice/route.ts @@ -23,7 +23,12 @@ export async function GET(_req: Request, { params }: { params: Promise<{ locale: const supabase = await createSupabaseServerClient(); const [{ data: c }, { data: clinic }] = await Promise.all([ - supabase.from("checkups").select("id, customer_id, queue_number, checkup_date").eq("id", checkupId).maybeSingle(), + supabase + .from("checkups") + .select("id, customer_id, queue_number, checkup_date") + .eq("id", checkupId) + .eq("deleted", false) + .maybeSingle(), supabase.from("clinic_settings").select("name, address, phone").eq("id", true).maybeSingle(), ]); if (!c) return new Response("Not found", { status: 404 }); diff --git a/app/[locale]/(app)/reminders/page.tsx b/app/[locale]/(app)/reminders/page.tsx index 3b23aa8..c1d9573 100644 --- a/app/[locale]/(app)/reminders/page.tsx +++ b/app/[locale]/(app)/reminders/page.tsx @@ -16,12 +16,15 @@ export default async function RemindersPage({ params }: { params: Promise<{ loca const now = new Date(); const today = fmt.format(now); const horizon = fmt.format(new Date(now.getTime() + 7 * 86_400_000)); + // Lower bound so ancient overdue rows drop off instead of accumulating forever + // and burying this-week reminders past the row limit. + const floor = fmt.format(new Date(now.getTime() - 30 * 86_400_000)); const supabase = await createSupabaseServerClient(); const { data: checkups } = await supabase .from("checkups") .select("id, customer_id, recheck_date") - .not("recheck_date", "is", null) + .gte("recheck_date", floor) .lte("recheck_date", horizon) .eq("deleted", false) .order("recheck_date", { ascending: true }) diff --git a/app/[locale]/(auth)/sign-in/actions.ts b/app/[locale]/(auth)/sign-in/actions.ts index 0dac238..c10ea6a 100644 --- a/app/[locale]/(auth)/sign-in/actions.ts +++ b/app/[locale]/(auth)/sign-in/actions.ts @@ -61,17 +61,25 @@ export async function signInAction( // XFF hop (appended by the trusted proxy). const hdrs = await headers(); const xff = hdrs.get("x-forwarded-for"); - const ip = hdrs.get("x-real-ip")?.trim() || xff?.split(",").at(-1)?.trim() || "unknown"; - try { - const { success: withinLimit } = await signInLimiter.limit(ip); - if (!withinLimit) { - return { status: "error", fieldErrors: {}, formError: t("tooManyAttempts") }; + const ip = hdrs.get("x-real-ip")?.trim() || xff?.split(",").at(-1)?.trim() || null; + // Only rate-limit when a real client IP is resolvable. If neither header is + // set (misconfigured proxy / non-Vercel host), skip rather than bucket every + // request under a shared "unknown" key — which would lock out the whole clinic + // after 5 attempts/min. Logged so the missing-IP case is visible. + if (!ip) { + console.warn("[sign-in] no client IP header; skipping rate limit"); + } else { + try { + const { success: withinLimit } = await signInLimiter.limit(ip); + if (!withinLimit) { + return { status: "error", fieldErrors: {}, formError: t("tooManyAttempts") }; + } + } catch (err) { + // Shared Redis unavailable: fail OPEN so a sibling app's outage can't lock + // doctors out of the clinic. Logged so the gap in brute-force protection + // is alertable rather than silent. + console.warn("[sign-in] rate limiter unavailable, failing open:", err); } - } catch (err) { - // Shared Redis unavailable: fail OPEN so a sibling app's outage can't lock - // doctors out of the clinic. Logged so the gap in brute-force protection - // is alertable rather than silent. - console.warn("[sign-in] rate limiter unavailable, failing open:", err); } const supabase = await createSupabaseServerClient(); diff --git a/supabase/migrations/20260725140000_bsk_review_fixes.sql b/supabase/migrations/20260725140000_bsk_review_fixes.sql new file mode 100644 index 0000000..64458f8 --- /dev/null +++ b/supabase/migrations/20260725140000_bsk_review_fixes.sql @@ -0,0 +1,153 @@ +-- BSK — code-review fixes. +-- +-- (2) Lock prescriptions/services once an invoice is paid: save_prescription / +-- save_checkup_services now refuse to modify a checkup whose medicine_orders +-- row is 'paid', so a recorded payment can never diverge from the invoice. +-- (4) Race-safe staff mutations: set_staff_role / remove_staff hold an advisory +-- lock while checking the "keep >= 1 admin" invariant, so two concurrent +-- demote/remove operations can't both pass the guard and orphan the clinic. + +-- ─── (2) save_prescription — refuse when paid ─────────────────────────────── +CREATE OR REPLACE FUNCTION bsk.save_prescription(p_checkup_id bigint, p_items jsonb) + RETURNS void + LANGUAGE plpgsql + VOLATILE + SECURITY DEFINER + SET search_path = bsk, pg_catalog +AS $$ +DECLARE + v_role bsk.app_role := bsk.current_role(); +BEGIN + IF v_role IS NULL OR v_role NOT IN ('admin', 'receptionist', 'doctor', 'nurse') THEN + RAISE EXCEPTION 'not authorized to save a prescription'; + END IF; + + IF EXISTS (SELECT 1 FROM bsk.medicine_orders WHERE checkup_id = p_checkup_id AND payment_status = 'paid') THEN + RAISE EXCEPTION 'cannot modify a paid invoice'; + END IF; + + DELETE FROM bsk.order_items WHERE checkup_id = p_checkup_id; + + INSERT INTO bsk.order_items (checkup_id, medicine_id, quantity, dosage, unit_price, line_total, notes) + SELECT + p_checkup_id, + (elem->>'medicine_id')::bigint, + (elem->>'quantity')::integer, + NULLIF(elem->>'dosage', ''), + m.sale_price, + m.sale_price * (elem->>'quantity')::integer, + NULLIF(elem->>'notes', '') + FROM jsonb_array_elements(COALESCE(p_items, '[]'::jsonb)) AS elem + JOIN bsk.medicines m ON m.id = (elem->>'medicine_id')::bigint; + + INSERT INTO bsk.medicine_orders (checkup_id) VALUES (p_checkup_id) + ON CONFLICT (checkup_id) DO NOTHING; +END +$$; + +-- ─── (2) save_checkup_services — refuse when paid ─────────────────────────── +CREATE OR REPLACE FUNCTION bsk.save_checkup_services(p_checkup_id bigint, p_items jsonb) + RETURNS void + LANGUAGE plpgsql + VOLATILE + SECURITY DEFINER + SET search_path = bsk, pg_catalog +AS $$ +DECLARE + v_role bsk.app_role := bsk.current_role(); +BEGIN + IF v_role IS NULL OR v_role NOT IN ('admin', 'receptionist', 'doctor', 'nurse') THEN + RAISE EXCEPTION 'not authorized to save checkup services'; + END IF; + + IF EXISTS (SELECT 1 FROM bsk.medicine_orders WHERE checkup_id = p_checkup_id AND payment_status = 'paid') THEN + RAISE EXCEPTION 'cannot modify a paid invoice'; + END IF; + + DELETE FROM bsk.checkup_services WHERE checkup_id = p_checkup_id; + + INSERT INTO bsk.checkup_services (checkup_id, service_id, quantity, unit_price, line_total) + SELECT + p_checkup_id, + (elem->>'service_id')::bigint, + (elem->>'quantity')::integer, + s.price, + s.price * (elem->>'quantity')::integer + FROM jsonb_array_elements(COALESCE(p_items, '[]'::jsonb)) AS elem + JOIN bsk.services s ON s.id = (elem->>'service_id')::bigint; +END +$$; + +-- ─── (4) Race-safe staff mutations ────────────────────────────────────────── +-- Both take pg_advisory_xact_lock so the last-admin check-then-act is serialized. +-- SECURITY DEFINER (write app_users) but gated on the CALLER's current_role(). + +CREATE OR REPLACE FUNCTION bsk.set_staff_role(p_user_id uuid, p_role bsk.app_role) + RETURNS void + LANGUAGE plpgsql + VOLATILE + SECURITY DEFINER + SET search_path = bsk, pg_catalog +AS $$ +DECLARE + v_target_role bsk.app_role; +BEGIN + IF bsk.current_role() <> 'admin' THEN + RAISE EXCEPTION 'not authorized to change staff roles'; + END IF; + IF p_user_id = auth.uid() THEN + RAISE EXCEPTION 'cannot change your own role'; + END IF; + + PERFORM pg_advisory_xact_lock(hashtext('bsk:staff')::bigint); + + SELECT role INTO v_target_role FROM bsk.app_users WHERE user_id = p_user_id; + IF v_target_role IS NULL THEN + RAISE EXCEPTION 'user not enrolled'; + END IF; + + -- Demoting the last admin is forbidden. + IF v_target_role = 'admin' AND p_role <> 'admin' + AND (SELECT count(*) FROM bsk.app_users WHERE role = 'admin') <= 1 THEN + RAISE EXCEPTION 'cannot demote the last admin'; + END IF; + + UPDATE bsk.app_users SET role = p_role WHERE user_id = p_user_id; +END +$$; + +CREATE OR REPLACE FUNCTION bsk.remove_staff(p_user_id uuid) + RETURNS void + LANGUAGE plpgsql + VOLATILE + SECURITY DEFINER + SET search_path = bsk, pg_catalog +AS $$ +DECLARE + v_target_role bsk.app_role; +BEGIN + IF bsk.current_role() <> 'admin' THEN + RAISE EXCEPTION 'not authorized to remove staff'; + END IF; + IF p_user_id = auth.uid() THEN + RAISE EXCEPTION 'cannot remove yourself'; + END IF; + + PERFORM pg_advisory_xact_lock(hashtext('bsk:staff')::bigint); + + SELECT role INTO v_target_role FROM bsk.app_users WHERE user_id = p_user_id; + IF v_target_role IS NULL THEN + RETURN; -- already gone + END IF; + + IF v_target_role = 'admin' + AND (SELECT count(*) FROM bsk.app_users WHERE role = 'admin') <= 1 THEN + RAISE EXCEPTION 'cannot remove the last admin'; + END IF; + + DELETE FROM bsk.app_users WHERE user_id = p_user_id; +END +$$; + +GRANT EXECUTE ON FUNCTION bsk.set_staff_role(uuid, bsk.app_role) TO authenticated; +GRANT EXECUTE ON FUNCTION bsk.remove_staff(uuid) TO authenticated; diff --git a/types/supabase-bsk.ts b/types/supabase-bsk.ts index 4431f07..f999905 100644 --- a/types/supabase-bsk.ts +++ b/types/supabase-bsk.ts @@ -512,6 +512,14 @@ export type Database = { }; Returns: number; }; + set_staff_role: { + Args: { p_user_id: string; p_role: Database["bsk"]["Enums"]["app_role"] }; + Returns: undefined; + }; + remove_staff: { + Args: { p_user_id: string }; + Returns: undefined; + }; save_prescription: { Args: { p_checkup_id: number; p_items: Json }; Returns: undefined;