From 711b02dffb3d58c39ab087e020f184132aad252c Mon Sep 17 00:00:00 2001 From: Rajpreet0 Date: Mon, 14 Sep 2026 16:49:07 +0200 Subject: [PATCH] fix: address code review findings on PWA/push feature - serwist route: force useNativeEsbuild to avoid depending on the esbuild-wasm auto-detection fallback - InstallPrompt/NotificationPrompt: switch to useSyncExternalStore so browser-dependent eligibility no longer causes a hydration mismatch and no longer sets state synchronously inside an effect body - NotificationPrompt: a failed subscribe/server-registration no longer permanently dismisses the prompt (localStorage), keeping an in-app retry path; a granted-but-unregistered subscription is detected via pushManager.getSubscription() and re-offers the prompt - send-push: bound each webpush.sendNotification call with a timeout so an unresponsive endpoint can't leave the settle indefinitely - push-subscription-actions: validate endpoint against an allowlist of known push service origins before persisting (SSRF hardening), and scope unsubscribeFromPush to the authenticated recipient - create-notification: wrap the push side-effect in next/server's after() so a serverless function isn't frozen mid-delivery - DiscountCodeList: disable close/cancel controls while a save is in flight Co-Authored-By: Claude Sonnet 5 --- src/app/serwist/[path]/route.ts | 5 + src/components/InstallPrompt.tsx | 37 +++++--- src/components/NotificationPrompt.tsx | 91 +++++++++++++++---- src/lib/push/send-push.ts | 6 +- .../actions/push-subscription-actions.ts | 45 ++++++++- .../notifications/lib/create-notification.ts | 36 ++++++-- .../ui/components/DiscountCodeList.tsx | 4 + 7 files changed, 180 insertions(+), 44 deletions(-) diff --git a/src/app/serwist/[path]/route.ts b/src/app/serwist/[path]/route.ts index bd64a00..26755f2 100644 --- a/src/app/serwist/[path]/route.ts +++ b/src/app/serwist/[path]/route.ts @@ -9,5 +9,10 @@ export const { dynamic, dynamicParams, revalidate, generateStaticParams, GET } = { swSrc: "src/app/sw.ts", additionalPrecacheEntries: [{ url: "/manifest.json", revision }], + // Default ist nur auf win32 true (siehe @serwist/turbopack), auf + // Linux/macOS sonst esbuild-wasm - explizit erzwungen, damit das + // Verhalten plattformunabhängig gleich ist (esbuild bleibt außerdem als + // direkte Dependency installiert, esbuild-wasm nur als Fallback-Netz). + useNativeEsbuild: true, } ); diff --git a/src/components/InstallPrompt.tsx b/src/components/InstallPrompt.tsx index aab8d9a..568a65d 100644 --- a/src/components/InstallPrompt.tsx +++ b/src/components/InstallPrompt.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useState } from "react"; +import { useEffect, useState, useSyncExternalStore } from "react"; import { X, Share, Plus, Download } from "lucide-react"; import { Button } from "@/src/components/ui/button"; import { @@ -18,18 +18,33 @@ function isIos(): boolean { return /iphone|ipad|ipod/i.test(window.navigator.userAgent); } -// Lazy-Initializer statt Effect: eligible/showIosHint hängen nur von -// Browser-APIs ab, nicht von React-Zustand, der synchronisiert werden müsste. -// Serverseitig (kein window) liefert das konsistent false, identisch zum -// allerersten Client-Render - kein Hydration-Mismatch. -function computeEligible(): boolean { - if (typeof window === "undefined") return false; - return isInstallPromptPending(); +// Kein echtes externes Event zum Abonnieren - subscribe registriert daher +// nie ein Update. Der Witz ist nur getServerSnapshot: React rendert serverseitig +// (und im allerersten Client-Pass vor der Hydration) "false" und ersetzt es +// direkt danach durch den echten Browser-Wert, OHNE die +// react-hooks/set-state-in-effect-Regel zu verletzen und OHNE eine +// Hydration-Mismatch-Warnung auszulösen (dafür ist der Hook gebaut). +function subscribeNever() { + return () => {}; } export function InstallPrompt() { - const [eligible, setEligible] = useState(computeEligible); - const [showIosHint] = useState(() => eligible && isIos()); + // useSyncExternalStore statt Lazy-useState/Effect: getServerSnapshot liefert + // serverseitig UND im allerersten Client-Pass vor der Hydration "false", + // React ersetzt es danach durch den echten Browser-Wert - ohne + // Hydration-Mismatch-Warnung und ohne die react-hooks/set-state-in-effect- + // Regel zu verletzen (kein setState im Effect-Body nötig). + const pending = useSyncExternalStore(subscribeNever, isInstallPromptPending, () => false); + const showIosHint = useSyncExternalStore( + subscribeNever, + () => pending && isIos(), + () => false + ); + // dismiss() muss den Prompt sofort ausblenden können, auch bevor sich der + // zugrunde liegende localStorage-Wert das nächste Mal ändert - pending + // allein kann das nicht (useSyncExternalStore hat kein echtes Update-Event). + const [dismissed, setDismissed] = useState(false); + const eligible = pending && !dismissed; const [deferredPrompt, setDeferredPrompt] = useState(null); useEffect(() => { @@ -45,7 +60,7 @@ export function InstallPrompt() { const dismiss = () => { localStorage.setItem(INSTALL_DISMISSED_KEY, "1"); - setEligible(false); + setDismissed(true); window.dispatchEvent(new Event(INSTALL_PROMPT_SETTLED_EVENT)); }; diff --git a/src/components/NotificationPrompt.tsx b/src/components/NotificationPrompt.tsx index 68255af..c82f498 100644 --- a/src/components/NotificationPrompt.tsx +++ b/src/components/NotificationPrompt.tsx @@ -1,6 +1,6 @@ "use client"; -import { useEffect, useState } from "react"; +import { useEffect, useState, useSyncExternalStore } from "react"; import { X, BellRing } from "lucide-react"; import { Button } from "@/src/components/ui/button"; import { INSTALL_PROMPT_SETTLED_EVENT, isInstallPromptPending } from "@/src/lib/pwa"; @@ -29,14 +29,39 @@ function isPushSupported(): boolean { ); } -// Lazy-Initializer statt Effect (siehe InstallPrompt.tsx): Browser-API-Werte -// werden serverseitig identisch mit dem allerersten Client-Render (false) -// beantwortet, kein Hydration-Mismatch möglich. -function computeEligible(): boolean { +// Basis-Voraussetzungen, die sich zur Laufzeit nicht ändern (API-Support, +// VAPID-Konfiguration) - anders als Notification.permission/localStorage +// unten, die erst nach einem Nutzer-Entscheid entstehen. +function isBaseSupported(): boolean { + return Boolean(process.env.NEXT_PUBLIC_VAPID_PUBLIC_KEY) && isPushSupported(); +} + +// Anders als in InstallPrompt.tsx gibt es hier ein echtes externes Signal, +// auf das reagiert werden muss: computeBaseEligible() hängt (über +// isInstallPromptPending()) vom Install-Prompt-Status ab, der sich NACH +// unserem Mount ändern kann (Nutzer klickt ihn erst später weg) - INSTALL_ +// PROMPT_SETTLED_EVENT signalisiert genau das, useSyncExternalStore ruft dann +// automatisch computeBaseEligible() erneut auf. +function subscribeToInstallPromptSettled(onChange: () => void) { + window.addEventListener(INSTALL_PROMPT_SETTLED_EVENT, onChange); + return () => window.removeEventListener(INSTALL_PROMPT_SETTLED_EVENT, onChange); +} + +/** + * Ob der Prompt (erneut) angezeigt werden soll. Nur eine explizite + * Ablehnung (Permission "denied") oder ein manuelles Wegklicken persistieren + * dauerhaft in localStorage - ein technischer Fehler nach erteilter + * Permission (Subscribe/Server) darf den In-App-Retry-Pfad nicht + * blockieren, sonst bliebe der Nutzer ohne Push, bis er Site-Daten + * zurücksetzt (siehe handleEnable). Deckt NICHT den Fall ab, dass die + * Permission bereits erteilt ist, aber die Server-Registrierung fehlte - + * das kann nur async per pushManager.getSubscription() geprüft werden, + * siehe missingServerRegistration-State in NotificationPrompt. + */ +function computeBaseEligible(): boolean { if (typeof window === "undefined") return false; - if (!process.env.NEXT_PUBLIC_VAPID_PUBLIC_KEY) return false; - if (!isPushSupported()) return false; - if (Notification.permission !== "default") return false; // schon erlaubt oder blockiert + if (!isBaseSupported()) return false; + if (Notification.permission === "denied") return false; // Wartet, bis InstallPrompt nicht mehr sichtbar ist (installiert oder // weggeklickt) - sonst überlappen sich beide "fixed bottom-4"-Banner. if (isInstallPromptPending()) return false; @@ -44,28 +69,55 @@ function computeEligible(): boolean { } export function NotificationPrompt() { - const [eligible, setEligible] = useState(computeEligible); + // useSyncExternalStore statt Lazy-useState/Effect (siehe InstallPrompt.tsx): + // getServerSnapshot liefert server- und im allerersten Client-Pass "false", + // kein Hydration-Mismatch, kein setState im Effect-Body nötig. + const baseEligible = useSyncExternalStore( + subscribeToInstallPromptSettled, + computeBaseEligible, + () => false + ); + // true, wenn Permission bereits erteilt ist, aber der Browser lokal keine + // Subscription (mehr) hat - dann war die vorherige Registrierung vermutlich + // unterbrochen (siehe handleEnable-Kommentar), der Prompt bleibt trotz + // gesetztem Dismissed-Flag sichtbar, damit ein Retry möglich ist. Kann nicht + // Teil von computeBaseEligible/useSyncExternalStore sein, weil + // getSubscription() ein Promise zurückgibt (kein synchroner Snapshot). + const [missingServerRegistration, setMissingServerRegistration] = useState(false); + const [dismissed, setDismissed] = useState(false); const [isSubscribing, setIsSubscribing] = useState(false); + const eligible = (baseEligible || missingServerRegistration) && !dismissed; - // Reagiert auf InstallPrompt, das erst nach unserem Mount weggeklickt wird - // (siehe computeEligible) - ansonsten bliebe eligible dauerhaft false. useEffect(() => { - const handleSettled = () => setEligible(computeEligible()); - window.addEventListener(INSTALL_PROMPT_SETTLED_EVENT, handleSettled); - return () => window.removeEventListener(INSTALL_PROMPT_SETTLED_EVENT, handleSettled); + if (isBaseSupported() && Notification.permission === "granted") { + navigator.serviceWorker.ready + .then((registration) => registration.pushManager.getSubscription()) + .then((subscription) => { + if (!subscription) setMissingServerRegistration(true); + }) + .catch(() => {}); + } }, []); + // Schließen-Button/explizite Ablehnung: dauerhaft in localStorage merken. const dismiss = () => { localStorage.setItem(DISMISSED_KEY, "1"); - setEligible(false); + setDismissed(true); }; + // Technischer Fehler nach erteilter Permission: NUR den Prompt ausblenden, + // ohne localStorage zu setzen, damit der Nutzer es beim nächsten Laden + // erneut versuchen kann. + const hideWithoutPersisting = () => setDismissed(true); + const handleEnable = async () => { setIsSubscribing(true); try { const permission = await Notification.requestPermission(); if (permission !== "granted") { - dismiss(); + // "denied" wird schon von computeBaseEligible() dauerhaft + // ausgeschlossen, kein zusätzliches localStorage-Flag nötig. + hideWithoutPersisting(); return; } @@ -87,7 +139,10 @@ export function NotificationPrompt() { if (!result.success) { // Serverseitig nicht gespeichert - Subscription auf dem Gerät nicht // verwaist zurücklassen, sonst denkt der Browser dauerhaft, er sei - // angemeldet, ohne dass Push je zugestellt werden könnte. + // angemeldet, ohne dass Push je zugestellt werden könnte. Best-effort: + // schlägt das fehl, bleibt eine verwaiste Browser-Subscription zurück, + // die der useEffect-Check oben beim nächsten Laden über + // getSubscription() erkennt und den Prompt trotzdem wieder anbietet. await subscription.unsubscribe().catch(() => {}); throw new Error(result.error); } @@ -95,7 +150,7 @@ export function NotificationPrompt() { dismiss(); } catch (error) { console.error("Push-Aktivierung fehlgeschlagen:", error); - dismiss(); + hideWithoutPersisting(); } finally { setIsSubscribing(false); } diff --git a/src/lib/push/send-push.ts b/src/lib/push/send-push.ts index de258a1..ee949cd 100644 --- a/src/lib/push/send-push.ts +++ b/src/lib/push/send-push.ts @@ -43,7 +43,11 @@ export async function sendPushToRecipient( endpoint: sub.endpoint, keys: { p256dh: sub.p256dh, auth: sub.auth }, }, - body + body, + // web-push reicht timeout nur weiter, wenn es explizit gesetzt ist - + // ohne das kann ein Endpoint, der die Verbindung annimmt aber nie + // antwortet, das Promise unbegrenzt offen halten. + { timeout: 10_000 } ) ) ); diff --git a/src/modules/notifications/actions/push-subscription-actions.ts b/src/modules/notifications/actions/push-subscription-actions.ts index f5a858e..1563506 100644 --- a/src/modules/notifications/actions/push-subscription-actions.ts +++ b/src/modules/notifications/actions/push-subscription-actions.ts @@ -11,6 +11,31 @@ export interface PushSubscriptionInput { }; } +// Bekannte Push-Dienst-Origins der unterstützten Browser. Ohne diese +// Allowlist könnte ein Aufrufer eine beliebige interne HTTPS-URL als +// "Endpoint" einreichen - sendPushToRecipient würde später serverseitig +// Requests dorthin schicken (SSRF). endpoint kommt vom Client und wird nie +// vom Browser selbst gegen eine feste Liste geprüft, daher hier serverseitig. +const ALLOWED_PUSH_ORIGINS = new Set([ + "https://fcm.googleapis.com", // Chrome, Edge, andere Chromium-Browser + "https://updates.push.services.mozilla.com", // Firefox +]); + +function isAllowedPushEndpoint(endpoint: string): boolean { + let url: URL; + try { + url = new URL(endpoint); + } catch { + return false; + } + if (url.protocol !== "https:") return false; + + if (ALLOWED_PUSH_ORIGINS.has(url.origin)) return true; + // Windows/Edge-Legacy-Endpunkte laufen über wechselnde Subdomains + // (wns2-.notify.windows.com), daher Suffix- statt Origin-Check. + return url.hostname.endsWith(".notify.windows.com"); +} + /** * Speichert (oder aktualisiert) die Push-Subscription des eingeloggten * Nutzers. Gilt für alle Rollen (User, Manager, Employee) - anders als das @@ -32,6 +57,10 @@ export async function subscribeToPush( return { success: false, error: "Nicht angemeldet." }; } + if (!isAllowedPushEndpoint(input.endpoint)) { + return { success: false, error: "Ungültiger Push-Endpoint." }; + } + try { await prisma.pushSubscription.upsert({ where: { endpoint: input.endpoint }, @@ -59,15 +88,23 @@ export async function subscribeToPush( /** * Entfernt eine Push-Subscription (z.B. wenn der Nutzer Benachrichtigungen - * im Browser deaktiviert). Kein Auth-Check auf recipientId nötig - der - * Endpoint ist ein unrateables, gerätespezifisches Secret, das der Client - * nur kennt, wenn er es selbst erzeugt hat. + * im Browser deaktiviert). Auf recipientId des eingeloggten Nutzers + * beschränkt, damit niemand mit einem fremden (z.B. erratenen oder aus + * einem Netzwerk-Log kopierten) Endpoint die Push-Zustellung eines anderen + * Geräts abschalten kann. */ export async function unsubscribeFromPush( endpoint: string ): Promise<{ success: true } | { success: false; error: string }> { + let userData; try { - await prisma.pushSubscription.deleteMany({ where: { endpoint } }); + userData = await requireAuthWithData(); + } catch { + return { success: false, error: "Nicht angemeldet." }; + } + + try { + await prisma.pushSubscription.deleteMany({ where: { endpoint, recipientId: userData.id } }); return { success: true }; } catch (error) { console.error("unsubscribeFromPush fehlgeschlagen:", error); diff --git a/src/modules/notifications/lib/create-notification.ts b/src/modules/notifications/lib/create-notification.ts index c3b90d5..ef759c3 100644 --- a/src/modules/notifications/lib/create-notification.ts +++ b/src/modules/notifications/lib/create-notification.ts @@ -14,6 +14,7 @@ * Push aktiviert hat, bekommt dieselben Typen wie in-app). */ +import { after } from "next/server"; import { prisma } from "@/src/lib/prisma"; import { sendPushToRecipient } from "@/src/lib/push/send-push"; import { @@ -68,20 +69,35 @@ export async function createNotification(input: CreateNotificationInput): Promis linkUrl: input.linkUrl ?? null, }, }); - - // Fire-and-forget: Push-Fehler dürfen das erfolgreiche In-App-Insert - // oben nicht rückgängig machen oder den Aufrufer blockieren. - void sendPushToRecipient(input.recipientId, { - title: input.title, - body: input.body, - url: input.linkUrl, - }); - - return true; } catch (error) { console.error("createNotification fehlgeschlagen:", error); return false; } + + // Eigener try/catch AUSSERHALB des obigen Blocks: after() wirft, wenn es + // außerhalb eines Request-Scopes aufgerufen wird (z.B. in Tests) - das darf + // den bereits erfolgreichen Insert oben nicht nachträglich als + // fehlgeschlagen erscheinen lassen (return false), sonst denkt der + // Aufrufer, es sei kein Datensatz geschrieben worden, obwohl er es wurde. + try { + // Läuft erst NACH dem Response-Flush weiter, blockiert den Aufrufer also + // nicht - aber die Serverless-Function bleibt bis zum Abschluss aktiv + // (anders als ein reines void-Fire-and-forget, das eine Vercel-Function + // vor Abschluss eingefroren zurücklassen kann). Push-Fehler dürfen das + // In-App-Insert ohnehin nicht mehr rückgängig machen - + // sendPushToRecipient wirft selbst nie. + after(() => + sendPushToRecipient(input.recipientId, { + title: input.title, + body: input.body, + url: input.linkUrl, + }) + ); + } catch (error) { + console.error("createNotification: Push-Versand konnte nicht eingeplant werden:", error); + } + + return true; } /** diff --git a/src/modules/settings/ui/components/DiscountCodeList.tsx b/src/modules/settings/ui/components/DiscountCodeList.tsx index 573643d..89583ae 100644 --- a/src/modules/settings/ui/components/DiscountCodeList.tsx +++ b/src/modules/settings/ui/components/DiscountCodeList.tsx @@ -270,6 +270,7 @@ const DiscountCodeList = ({ setIsCreating(false); setForm(emptyForm); }} + disabled={isSaving} title="Schließen" > @@ -415,6 +416,7 @@ const DiscountCodeList = ({ setIsCreating(false); setForm(emptyForm); }} + disabled={isSaving} > Abbrechen @@ -507,6 +509,7 @@ const DiscountCodeList = ({ size="icon" className="size-6 text-blue-400 hover:bg-blue-100/70 hover:text-blue-700" onClick={cancelEditing} + disabled={isSaving} title="Schließen" > @@ -590,6 +593,7 @@ const DiscountCodeList = ({ variant="outline" className="bg-white/60" onClick={cancelEditing} + disabled={isSaving} > Abbrechen