From 502ba864f9765b5ec5597893a57562c9bf01327e Mon Sep 17 00:00:00 2001 From: Aswinmcw Date: Thu, 3 Sep 2026 09:21:11 +0000 Subject: [PATCH] worker: claim a paid quote atomically, count the whole pipeline as revenue, validate images on product update, match the crawler grid to the API order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - handleQuotePaid read the quote, then wrote an order and marked the quote in one batch. Two deliveries of the same payment_link.paid — Razorpay retries, and the event-id header is not guaranteed — could both read order_id = NULL and both create an order, receipt, invoice and pair of emails. The quote is now claimed first with UPDATE … WHERE order_id IS NULL; only the delivery that changes a row goes on to write the order. A failed order write releases the claim so the retry can convert. - /api/admin/stats counted only 'paid' and 'shipped', so revenue dropped the moment an order went to "in production" and came back when it shipped. Every stage from paid to delivered counts; cancelled/refunded/pending do not. - updateProduct accepted any string for image/images — an external URL, a traversal, a typo — where createProduct checks the manifest. Same check now. - The homepage's server-rendered grid used ORDER BY sort, name; the API orders pinned → buyable → quote-only. A crawler indexed one order and the visitor watched it reshuffle when main.js ran. Same order on both. Co-authored-by: Cursor --- src/admin.js | 38 ++++++++++++++++++++++----- src/index.js | 13 +++++++--- src/orders.js | 69 +++++++++++++++++++++++++++++++------------------ test/admin.mjs | 34 +++++++++++++++++++++--- test/orders.mjs | 66 ++++++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 180 insertions(+), 40 deletions(-) diff --git a/src/admin.js b/src/admin.js index 57d96b6..ddda7e2 100644 --- a/src/admin.js +++ b/src/admin.js @@ -610,12 +610,34 @@ export async function updateProduct(env, id, body) { if ("personalise_required" in body) { put("personalise_required", body.personalise_required ? 1 : 0); } - if ("image" in body) { - const image = clip(body.image, MAXLEN.image); - if (!image) return bad("Image path cannot be empty."); - put("image", image); + // Same check as createProduct: the client names a file, the manifest decides + // whether it exists. This path used to accept any string — an external URL, + // a traversal, a typo — and the shop would render whatever it was told. + if ("image" in body || "images" in body) { + const manifest = await readManifest(env); + if (!manifest) return bad("Image manifest unavailable — run `npm run images` and deploy.", 503); + const known = new Set(manifest.images.map((i) => i.file)); + + if ("image" in body) { + const image = clip(body.image, MAXLEN.image); + if (!image) return bad("Image path cannot be empty."); + const named = image.replace(/^.*\//, ""); + if (!known.has(named) || /^https?:|^\/\/|\.\./i.test(image)) { + return bad("That image is not in assets/images. Push the photo, run `npm run images`, and try again."); + } + put("image", `assets/images/${named}`); + } + if ("images" in body) { + const extras = []; + for (const part of clip(body.images, MAXLEN.images).split(",")) { + const f = part.trim().replace(/^.*\//, ""); + if (!f) continue; + if (!known.has(f)) return bad(`Extra image "${f}" is not in assets/images.`); + extras.push(`assets/images/${f}`); + } + put("images", extras.join(",")); + } } - if ("images" in body) put("images", clip(body.images, MAXLEN.images)); if ("category" in body) put("category", clip(body.category, MAXLEN.category)); if ("visible" in body) put("visible", body.visible ? 1 : 0); // Leads the catalogue. A toggle, so coerced rather than validated — there is @@ -1080,9 +1102,13 @@ export async function refundOrder(env, id, body) { // ── dashboard summary ───────────────────────────────────────────── export async function stats(env) { + // Every stage from paid onwards is money that has arrived. This counted only + // 'paid' and 'shipped', so the moment an order was marked "in production" it + // fell out of the revenue figure and came back when it shipped — the number on + // the dashboard went DOWN as work progressed. const paid = await env.DB.prepare( `SELECT COUNT(*) AS orders, COALESCE(SUM(total_paise),0) AS revenue - FROM orders WHERE status IN ('paid','shipped')` + FROM orders WHERE status IN ('paid','in_production','ready','shipped','delivered')` ).first(); const pending = await env.DB.prepare( `SELECT COUNT(*) AS n FROM orders WHERE status = 'pending'` diff --git a/src/index.js b/src/index.js index 17585fc..e9c26d9 100644 --- a/src/index.js +++ b/src/index.js @@ -167,10 +167,14 @@ export default { && (request.method === "GET" || request.method === "HEAD")) { const page = await env.ASSETS.fetch(request); try { + // The SAME order listProducts() in shop.js produces, or the grid a + // crawler indexes (and a visitor sees for the first moment) is a + // different grid from the one main.js draws a second later. const [{ results }, promo] = await Promise.all([ env.DB.prepare( - `SELECT slug, name, description, price_paise, image - FROM products WHERE visible = 1 ORDER BY sort ASC, name ASC` + `SELECT slug, name, description, price_paise, image, pinned + FROM products WHERE visible = 1 + ORDER BY pinned DESC, created_at DESC, (sort = 0), sort ASC, name ASC` ).all(), // Non-fatal: a banner that fails to render server-side just appears // the old way, from /api/products, once main.js runs. @@ -179,8 +183,11 @@ export default { return null; }), ]); + const ordered = (results || []).slice().sort((a, b) => + Number(Boolean(b.pinned)) - Number(Boolean(a.pinned)) || + Number(!(a.price_paise > 0)) - Number(!(b.price_paise > 0))); - const rendered = rewriteHome(env, page, results || [], url, promo); + const rendered = rewriteHome(env, page, ordered, url, promo); // Edge-cached, because this added a D1 query to the hot path. // diff --git a/src/orders.js b/src/orders.js index d134437..ac177be 100644 --- a/src/orders.js +++ b/src/orders.js @@ -400,35 +400,54 @@ async function handleQuotePaid(env, ctx, evt) { const orderId = uid(); const orderReceipt = "AP-" + orderId.replace(/-/g, "").slice(0, 8).toUpperCase(); const t = now(); + const priorStatus = quote.status; + + // CLAIM THE QUOTE FIRST, conditionally. The read above and the writes below + // are two round trips, and two deliveries of the same payment — Razorpay + // retries, and the event-id header is not guaranteed — could both read + // order_id = NULL and both build an order. `AND order_id IS NULL` makes the + // claim atomic: exactly one delivery changes a row; the other sees 0 and + // stops, before it has created anything. + const claim = await env.DB.prepare( + `UPDATE quotes SET status = 'paid', order_id = ?, updated_at = ? + WHERE id = ? AND order_id IS NULL` + ).bind(orderId, t, quote.id).run(); + if (claim.meta?.changes === 0) return; // The address is deliberately blank: a quote request never asks for one. The // dashboard flags this order as needing an address, and Aswin is already in an // email thread with the customer by the time it is paid. - await env.DB.batch([ - env.DB.prepare( - `INSERT INTO orders (id, receipt, rzp_order_id, rzp_payment_id, status, - subtotal_paise, shipping_paise, total_paise, currency, - delivery, cust_name, cust_email, cust_phone, - notes, created_at, paid_at) - VALUES (?,?,?,?,'paid',?,0,?,'INR','ship',?,?,?,?,?,?)` - ).bind(orderId, orderReceipt, rzpOrder.id || payment.order_id || null, - payment.id || null, paid, paid, - quote.cust_name, quote.cust_email, quote.cust_phone, - `Quotation ${quote.receipt}`, t, t), - - // product_id NULL — the column is nullable for exactly this: a line that - // never came from the catalogue. - env.DB.prepare( - `INSERT INTO order_items - (id, order_id, product_id, name, price_paise, qty, personalisation, pos) - VALUES (?,?,NULL,?,?,1,?,0)` - ).bind(uid(), orderId, `Custom print — ${quote.receipt}`, paid, - String(quote.description || "").slice(0, 500)), - - env.DB.prepare( - `UPDATE quotes SET status = 'paid', order_id = ?, updated_at = ? WHERE id = ?` - ).bind(orderId, t, quote.id), - ]); + try { + await env.DB.batch([ + env.DB.prepare( + `INSERT INTO orders (id, receipt, rzp_order_id, rzp_payment_id, status, + subtotal_paise, shipping_paise, total_paise, currency, + delivery, cust_name, cust_email, cust_phone, + notes, created_at, paid_at) + VALUES (?,?,?,?,'paid',?,0,?,'INR','ship',?,?,?,?,?,?)` + ).bind(orderId, orderReceipt, rzpOrder.id || payment.order_id || null, + payment.id || null, paid, paid, + quote.cust_name, quote.cust_email, quote.cust_phone, + `Quotation ${quote.receipt}`, t, t), + + // product_id NULL — the column is nullable for exactly this: a line that + // never came from the catalogue. + env.DB.prepare( + `INSERT INTO order_items + (id, order_id, product_id, name, price_paise, qty, personalisation, pos) + VALUES (?,?,NULL,?,?,1,?,0)` + ).bind(uid(), orderId, `Custom print — ${quote.receipt}`, paid, + String(quote.description || "").slice(0, 500)), + ]); + } catch (e) { + // Release the claim so Razorpay's retry can convert the quote, rather than + // leaving it marked paid and pointing at an order that was never written. + await env.DB.prepare( + `UPDATE quotes SET status = ?, order_id = NULL, updated_at = ? + WHERE id = ? AND order_id = ?` + ).bind(priorStatus, now(), quote.id, orderId).run().catch(() => {}); + throw e; + } const order = await env.DB.prepare(`SELECT * FROM orders WHERE id = ?`).bind(orderId).first(); const { results: items } = await env.DB.prepare( diff --git a/test/admin.mjs b/test/admin.mjs index 97010d7..9e85680 100644 --- a/test/admin.mjs +++ b/test/admin.mjs @@ -349,7 +349,11 @@ function makeDB(seed = {}) { return { meta: { changes: o ? 1 : 0 } }; } if (s.startsWith("SELECT COUNT(*) AS orders, COALESCE(SUM(total_paise),0) AS revenue")) { - const rows = db.orders.filter((o) => ["paid", "shipped"].includes(o.status)); + // Parse the IN (...) list out of the SQL rather than hardcoding it, so + // the fake cannot agree with the test while disagreeing with the query. + const list = (s.match(/status IN \(([^)]*)\)/) || [])[1] || ""; + const statuses = list.split(",").map((x) => x.trim().replace(/^'|'$/g, "")); + const rows = db.orders.filter((o) => statuses.includes(o.status)); return { first: { orders: rows.length, revenue: rows.reduce((n, o) => n + o.total_paise, 0) } }; } if (s.startsWith("SELECT COUNT(*) AS n FROM orders WHERE status = 'pending'")) { @@ -442,6 +446,22 @@ section("admin products — list includes hidden rows"); ok("exposes the visible flag", out.products.some((p) => p.visible === 0)); } +section("admin products — update checks the image against the manifest"); +{ + const env = envDB({ products: [PRODUCT] }); + for (const image of ["https://evil.example/x.jpg", "//evil.example/x.jpg", "../../etc/passwd", "not-in-manifest.jpg"]) { + const [status] = await read(await updateProduct(env, PRODUCT.id, { image })); + ok(`rejects ${image}`, status === 400, String(status)); + } + const [status] = await read(await updateProduct(env, PRODUCT.id, { image: "dragon.jpg", images: "extra1.jpg, assets/images/extra2.jpg" })); + ok("accepts a file that exists", status === 200, String(status)); + const row = env.DB._db.products[0]; + ok("stores the canonical path", row.image === "assets/images/dragon.jpg", row.image); + ok("extras are canonical too", row.images === "assets/images/extra1.jpg,assets/images/extra2.jpg", row.images); + const [bad] = await read(await updateProduct(env, PRODUCT.id, { images: "extra1.jpg,ghost.jpg" })); + ok("an unknown extra is refused", bad === 400); +} + section("admin products — price validation"); { for (const [label, price] of [ @@ -1304,13 +1324,19 @@ section("admin stats"); { ...ORDER, id: "o2", status: "shipped", total_paise: 100000 }, { ...ORDER, id: "o3", status: "pending", total_paise: 50000 }, { ...ORDER, id: "o4", status: "cancelled", total_paise: 70000 }, + // Mid-pipeline. These used to drop out of revenue the moment an order was + // marked "in production" and reappear when it shipped. + { ...ORDER, id: "o5", status: "in_production", total_paise: 20000 }, + { ...ORDER, id: "o6", status: "ready", total_paise: 10000 }, + { ...ORDER, id: "o7", status: "delivered", total_paise: 5000 }, + { ...ORDER, id: "o8", status: "refunded", total_paise: 90000 }, ], products: [PRODUCT, { ...PRODUCT, id: "p2", visible: 0 }], }); const [, s] = await read(await stats(env)); - ok("counts paid + shipped as revenue", s.revenue_paise === 144800, String(s.revenue_paise)); - ok("excludes pending and cancelled from revenue", s.revenue_paise !== 264800); - ok("paid order count", s.paid_orders === 2); + ok("counts every stage from paid to delivered as revenue", s.revenue_paise === 179800, String(s.revenue_paise)); + ok("excludes pending, cancelled and refunded from revenue", s.revenue_paise !== 264800 && s.revenue_paise < 269800); + ok("paid order count spans the pipeline", s.paid_orders === 5, String(s.paid_orders)); ok("pending count", s.pending_orders === 1); ok("product totals", s.products_total === 2 && s.products_visible === 1); } diff --git a/test/orders.mjs b/test/orders.mjs index d542427..acc4aa9 100644 --- a/test/orders.mjs +++ b/test/orders.mjs @@ -57,9 +57,19 @@ function makeDB() { return { first: db.quotes.find((q) => q.receipt === args[0]) || null }; } if (s.startsWith("UPDATE quotes SET status = 'paid'")) { + // The conditional claim. `AND order_id IS NULL` is what makes a second + // delivery a no-op, so the fake has to honour it or the race test below + // passes for the wrong reason. const q = db.quotes.find((x) => x.id === args[2]); - if (q) { q.status = "paid"; q.order_id = args[0]; q.updated_at = args[1]; } - return { meta: { changes: q ? 1 : 0 } }; + if (!q || (s.includes("order_id IS NULL") && q.order_id)) return { meta: { changes: 0 } }; + q.status = "paid"; q.order_id = args[0]; q.updated_at = args[1]; + return { meta: { changes: 1 } }; + } + if (s.startsWith("UPDATE quotes SET status = ?, order_id = NULL")) { + const q = db.quotes.find((x) => x.id === args[2] && x.order_id === args[3]); + if (!q) return { meta: { changes: 0 } }; + q.status = args[0]; q.order_id = null; q.updated_at = args[1]; + return { meta: { changes: 1 } }; } if (s.startsWith("SELECT * FROM orders WHERE id = ?")) { return { first: db.orders.find((o) => o.id === args[0]) || null }; @@ -872,6 +882,58 @@ section("payment_link.paid — a redelivery does nothing twice"); ok("exactly one line", env.DB._db.order_items.length === 1); } +section("payment_link.paid — two deliveries with NO event id race for one quote"); +{ + // No x-razorpay-event-id header at all, so webhook_events cannot help, and the + // two deliveries interleave: both read the quote before either has written. + // The conditional UPDATE is the only thing between this and two orders, two + // receipts and two invoices for one payment. + const env = ENV(); stubFetch(); + env.DB._db.quotes.push({ ...QUOTE_ROW }); + const raw = linkPaidBody(); + const sig = await hmacHex(raw, WEBHOOK_SECRET); + + const c1 = makeCtx(), c2 = makeCtx(); + const noId = (r) => { const h = new Headers(r.headers); h.delete("x-razorpay-event-id"); return new Request(r.url, { method: "POST", headers: h, body: raw }); }; + await Promise.all([ + razorpayWebhook(noId(webhookReq(raw, sig, "")), env, c1), + razorpayWebhook(noId(webhookReq(raw, sig, "")), env, c2), + ]); + await settle(c1); await settle(c2); + + ok("exactly one order despite the race", env.DB._db.orders.length === 1, String(env.DB._db.orders.length)); + ok("exactly one line", env.DB._db.order_items.length === 1); + ok("the quote points at the order that exists", + env.DB._db.quotes[0].order_id === env.DB._db.orders[0]?.id); +} + +section("payment_link.paid — a failed order write releases the claim"); +{ + const env = ENV(); stubFetch(); + env.DB._db.quotes.push({ ...QUOTE_ROW }); + const raw = linkPaidBody(); + const sig = await hmacHex(raw, WEBHOOK_SECRET); + + // D1 falls over on the batch, once. + const realBatch = env.DB.batch.bind(env.DB); + let failed = false; + env.DB.batch = async (stmts) => { + if (!failed) { failed = true; throw new Error("D1 hiccup"); } + return realBatch(stmts); + }; + const c1 = makeCtx(); + let threw = false; + try { await razorpayWebhook(webhookReq(raw, sig, "evt_pl_fail1"), env, c1); } catch { threw = true; } + ok("the first delivery fails loudly so Razorpay retries", threw); + ok("no order was written", env.DB._db.orders.length === 0); + ok("the quote is NOT left marked paid", env.DB._db.quotes[0].status !== "paid" && env.DB._db.quotes[0].order_id == null, + JSON.stringify(env.DB._db.quotes[0])); + + const c2 = makeCtx(); + await razorpayWebhook(webhookReq(raw, sig, "evt_pl_fail2"), env, c2); await settle(c2); + ok("the retry converts the quote", env.DB._db.orders.length === 1 && env.DB._db.quotes[0].status === "paid"); +} + section("payment_link.paid — an unknown reference is refused quietly"); { const env = ENV(); stubFetch();