fix: close remaining TOCTOU races, self-correct a regression from the prior fix
Second silent-failure-hunter pass (stock/auth/admin-action focus,
explicitly scoped away from the payment paths fixed in the previous
commit) plus a self-review of that previous commit's own diff.
Self-correction: my prior fix to fulfillOrder's NEU->BEZAHLT guard
replaced the original atomic `updateMany({status:"NEU"})` claim with a
plain findUnique+update to let a retry backfill Payment/Invoice/email
after BEZAHLT. That accidentally dropped the atomicity the original
code relied on to prevent double-counting a discount code on two
concurrent webhook deliveries. Restored it: the NEU->BEZAHLT
transition is an atomic updateMany claim again (count===1 decides
"firstTime", which now also gates the discount-code increment); a
losing/retry call separately checks for "already BEZAHLT" to still
allow the backfill.
Three further real gaps, confirmed by hand before fixing:
- failOrder (same webhook file) and admin's updateOrderStatusAction
had the identical TOCTOU pattern: findUnique then unconditional
update then restockOrderItems. Two concurrent webhook deliveries
(Stripe documents at-least-once delivery) or two admin tabs
double-clicking "Stornieren" could both pass the stale read before
either write landed, silently double-crediting inventory. Both now
use the same atomic-claim pattern as fulfillOrder.
- 2FA backup-code verification used findFirst({usedAt:null}) then a
separate update — two concurrent submissions of the same code could
both pass the check and both authenticate, defeating single-use.
Replaced with one atomic updateMany claim.
- Two silent no-ops: an invalid/stale order-status update returned
with zero admin-visible feedback despite revalidatePath still
firing; discount-code deletion had no existence check, so a
double-submitted delete threw an unhandled Prisma P2025 (500)
instead of a clear message. Both now use the existing fail() helper.
Verified: tsc clean, 151/151 existing tests still pass.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
1 parent
8b5d5d095c
commit
3627f4e5c1
4 files changed
+80
-33
No files matched your search
@@ -82,7 +82,15 @@ export async function deleteDiscountCodeAction(formData: FormData): Promise<void
|
||||
const admin = await requireAdmin();
|
||||
const id = str(formData, "id");
|
||||
|
||||
const deleted = await prisma.discountCode.delete({ where: { id } });
|
||||
// Doppeltes Absenden (zwei Tabs, Retry nach Timeout) darf nicht als
|
||||
// unbehandelter Prisma-P2025-Fehler (500) enden, wenn der Gutschein beim
|
||||
// ersten Versuch schon geloescht wurde.
|
||||
let deleted;
|
||||
try {
|
||||
deleted = await prisma.discountCode.delete({ where: { id } });
|
||||
} catch {
|
||||
fail("/admin/gutscheine", "Der Gutschein wurde nicht gefunden — möglicherweise bereits gelöscht.");
|
||||
}
|
||||
|
||||
await logAdminAction(admin, {
|
||||
action: "discount_code.delete",
|
||||
|
||||
@@ -8,7 +8,7 @@ import { shipmentEmail, pickupReadyEmail } from "@/lib/email-templates";
|
||||
import { getCarrierTrackingUrl } from "@/lib/shipment-tracking";
|
||||
import { restockOrderItems } from "@/lib/inventory";
|
||||
import { isLocale, defaultLocale } from "@/i18n/config";
|
||||
import { requireAdmin, str, strOrNull } from "./_shared";
|
||||
import { requireAdmin, str, strOrNull, fail } from "./_shared";
|
||||
|
||||
const ORDER_STATUSES = [
|
||||
"NEU",
|
||||
@@ -25,7 +25,10 @@ export async function updateOrderStatusAction(formData: FormData): Promise<void>
|
||||
const admin = await requireAdmin();
|
||||
const orderId = str(formData, "orderId");
|
||||
const status = str(formData, "status") as (typeof ORDER_STATUSES)[number];
|
||||
if (!ORDER_STATUSES.includes(status)) return;
|
||||
const detailPath = `/admin/bestellungen/${orderId}`;
|
||||
if (!ORDER_STATUSES.includes(status)) {
|
||||
fail(detailPath, "Ungültiger Status.");
|
||||
}
|
||||
|
||||
const result = await prisma.$transaction(async (tx) => {
|
||||
const previous = await tx.order.findUnique({
|
||||
@@ -41,13 +44,25 @@ export async function updateOrderStatusAction(formData: FormData): Promise<void>
|
||||
});
|
||||
if (!previous) return null;
|
||||
|
||||
await tx.order.update({ where: { id: orderId }, data: { status } });
|
||||
|
||||
// Beim Checkout reservierter Bestand wird bei Stornierung zurueckgebucht
|
||||
// (genau einmal — nur beim Wechsel VON einem nicht-stornierten Status).
|
||||
if (status === "STORNIERT" && previous.status !== "STORNIERT") {
|
||||
if (status === "STORNIERT") {
|
||||
// Atomarer Claim statt einfachem update(): verhindert, dass zwei
|
||||
// schnell aufeinanderfolgende Stornierungs-Versuche (z.B. zwei offene
|
||||
// Admin-Tabs auf derselben Bestellung) beide restockOrderItems
|
||||
// ausloesen und den Lagerbestand doppelt gutschreiben. Ein getrenntes
|
||||
// findUnique+update koennte das nicht verhindern, da beide Versuche
|
||||
// noch denselben "nicht schon storniert"-Stand lesen wuerden, bevor
|
||||
// einer von ihnen schreibt — die WHERE-Klausel im UPDATE selbst wird
|
||||
// dagegen atomar gegen den aktuellen Zeilenstand ausgewertet.
|
||||
const claim = await tx.order.updateMany({
|
||||
where: { id: orderId, status: { not: "STORNIERT" } },
|
||||
data: { status },
|
||||
});
|
||||
if (claim.count === 1) {
|
||||
await restockOrderItems(tx, previous.items);
|
||||
}
|
||||
} else {
|
||||
await tx.order.update({ where: { id: orderId }, data: { status } });
|
||||
}
|
||||
|
||||
let shipment: { carrier: string | null; trackingNumber: string | null } | null = null;
|
||||
if (status === "VERSENDET") {
|
||||
@@ -76,6 +91,10 @@ export async function updateOrderStatusAction(formData: FormData): Promise<void>
|
||||
return { ...previous, shipment };
|
||||
});
|
||||
|
||||
if (!result) {
|
||||
fail(detailPath, "Die Bestellung wurde nicht gefunden — möglicherweise wurde sie zwischenzeitlich gelöscht.");
|
||||
}
|
||||
|
||||
if (result) {
|
||||
await logAdminAction(admin, {
|
||||
action: "order.status_change",
|
||||
|
||||
@@ -90,16 +90,25 @@ async function fulfillOrder(session: Stripe.Checkout.Session): Promise<void> {
|
||||
// Mal stillschweigend abweisen und Payment/Invoice/E-Mail blieben fuer
|
||||
// immer unvollstaendig, obwohl die Bestellung laengst bezahlt ist.
|
||||
const order = await prisma.$transaction(async (tx) => {
|
||||
const existing = await tx.order.findUnique({
|
||||
where: { id: orderId },
|
||||
select: { status: true },
|
||||
// Atomarer Versuch, von "NEU" auf "BEZAHLT" zu wechseln: ein einzelnes
|
||||
// UPDATE mit "status: NEU" in der WHERE-Klausel laesst bei zwei parallel
|
||||
// laufenden Webhook-Zustellungen fuer dieselbe Bestellung garantiert nur
|
||||
// eine davon gewinnen (count === 1) — anders als ein getrenntes
|
||||
// findUnique+update, bei dem beide Transaktionen den alten Status noch
|
||||
// lesen koennten, bevor eine von ihnen schreibt, und der Gutschein-Zaehler
|
||||
// unten doppelt inkrementiert wuerde.
|
||||
const claim = await tx.order.updateMany({
|
||||
where: { id: orderId, status: "NEU" },
|
||||
data: { status: "BEZAHLT" },
|
||||
});
|
||||
if (!existing || (existing.status !== "NEU" && existing.status !== "BEZAHLT")) {
|
||||
return null;
|
||||
}
|
||||
const firstTime = claim.count === 1;
|
||||
|
||||
if (existing.status === "NEU") {
|
||||
await tx.order.update({ where: { id: orderId }, data: { status: "BEZAHLT" } });
|
||||
if (!firstTime) {
|
||||
// Entweder ein Retry, der die Bestellung bereits "BEZAHLT" vorfindet
|
||||
// (siehe Kommentar oben bei fulfillOrder), oder ein anderer Endstatus
|
||||
// (z.B. "STORNIERT") — im zweiten Fall nichts weiter tun.
|
||||
const existing = await tx.order.findUnique({ where: { id: orderId }, select: { status: true } });
|
||||
if (!existing || existing.status !== "BEZAHLT") return null;
|
||||
}
|
||||
|
||||
const updated = await tx.order.findUniqueOrThrow({
|
||||
@@ -107,10 +116,10 @@ async function fulfillOrder(session: Stripe.Checkout.Session): Promise<void> {
|
||||
include: { items: true },
|
||||
});
|
||||
|
||||
// Nur beim allerersten Durchlauf zaehlen, nicht bei einem Retry, der die
|
||||
// Bestellung bereits "BEZAHLT" vorfindet — sonst wuerde derselbe
|
||||
// Gutschein bei jedem erneut zugestellten Webhook nochmal gezaehlt.
|
||||
if (existing.status === "NEU" && updated.discountCodeId) {
|
||||
// Nur beim Gewinner des obigen atomaren Claims zaehlen, nie bei einem
|
||||
// Retry — sonst wuerde derselbe Gutschein bei jedem erneut zugestellten
|
||||
// Webhook nochmal gezaehlt.
|
||||
if (firstTime && updated.discountCodeId) {
|
||||
await tx.discountCode.update({
|
||||
where: { id: updated.discountCodeId },
|
||||
data: { usedCount: { increment: 1 } },
|
||||
@@ -192,10 +201,21 @@ async function failOrder(session: Stripe.Checkout.Session): Promise<void> {
|
||||
if (!orderId) return;
|
||||
|
||||
const result = await prisma.$transaction(async (tx) => {
|
||||
const previous = await tx.order.findUnique({
|
||||
// Atomarer Claim (gleiches Muster wie oben bei fulfillOrder): ein
|
||||
// getrenntes findUnique+update wuerde bei zwei parallel zugestellten
|
||||
// "expired"/"async_payment_failed"-Webhooks fuer dieselbe Bestellung
|
||||
// beide Kopien noch "NEU" lesen lassen, bevor eine von ihnen schreibt —
|
||||
// restockOrderItems liefe dann zweimal und der Lagerbestand wuerde
|
||||
// stillschweigend doppelt gutgeschrieben.
|
||||
const claim = await tx.order.updateMany({
|
||||
where: { id: orderId, status: "NEU" },
|
||||
data: { status: "STORNIERT" },
|
||||
});
|
||||
if (claim.count === 0) return null;
|
||||
|
||||
const previous = await tx.order.findUniqueOrThrow({
|
||||
where: { id: orderId },
|
||||
select: {
|
||||
status: true,
|
||||
orderNumber: true,
|
||||
email: true,
|
||||
firstName: true,
|
||||
@@ -203,12 +223,6 @@ async function failOrder(session: Stripe.Checkout.Session): Promise<void> {
|
||||
items: { select: { productId: true, quantity: true } },
|
||||
},
|
||||
});
|
||||
// Nur von "NEU" aus stornieren: verhindert doppelte Bestandsrueckbuchung,
|
||||
// falls Stripe denselben Webhook erneut zustellt, und laesst bereits
|
||||
// bezahlte/stornierte Bestellungen unangetastet.
|
||||
if (!previous || previous.status !== "NEU") return null;
|
||||
|
||||
await tx.order.update({ where: { id: orderId }, data: { status: "STORNIERT" } });
|
||||
|
||||
// Beim Checkout reservierten Bestand zurueckbuchen (gleiches Muster wie
|
||||
// bei manueller Stornierung im Admin-Bereich, siehe admin/actions.ts).
|
||||
|
||||
@@ -123,13 +123,19 @@ export async function verifyTwoFactorAction(formData: FormData): Promise<void> {
|
||||
|
||||
if (!authenticated && code.length > 0) {
|
||||
const codeHash = hashBackupCode(code.toUpperCase());
|
||||
const backupCode = await prisma.twoFactorBackupCode.findFirst({
|
||||
// Atomarer Claim statt findFirst+update: ein getrenntes Lesen und
|
||||
// Schreiben liesse zwei parallele Anfragen mit demselben (z.B.
|
||||
// abgefangenen) Backup-Code beide noch den "unbenutzt"-Stand sehen,
|
||||
// bevor eine von ihnen schreibt — beide wuerden dann authentifiziert
|
||||
// und die Einmal-Verwendbarkeit des Codes waere ausgehebelt. Die
|
||||
// WHERE-Klausel im UPDATE selbst wird dagegen atomar gegen den
|
||||
// aktuellen Zeilenstand ausgewertet, sodass hoechstens ein Aufruf
|
||||
// "gewinnt" (count === 1).
|
||||
const claim = await prisma.twoFactorBackupCode.updateMany({
|
||||
where: { customerId: customer.id, codeHash, usedAt: null },
|
||||
data: { usedAt: new Date() },
|
||||
});
|
||||
if (backupCode) {
|
||||
await prisma.twoFactorBackupCode.update({ where: { id: backupCode.id }, data: { usedAt: new Date() } });
|
||||
authenticated = true;
|
||||
}
|
||||
authenticated = claim.count === 1;
|
||||
}
|
||||
|
||||
if (!authenticated) {
|
||||
|
||||
Reference in new issue
Block a user