From 39c82ff3a4a44fab9f82994b833b621b739e7075 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 28 Aug 2026 12:22:39 -0500 Subject: [PATCH 1/4] fix(orders): mark a demo order in the history rather than leaving it to read as real (#205) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #195 and #203 made the cart say a demo order is a demo order. That message is an antd toast lasting about three seconds, after which the cart empties and the card unmounts. Order history is what the customer comes back to when they wonder where their item is, and it said nothing. A demo row was a real row: item name, `$80.00`, status `completed` rendered as a neutral tag because `STATUS_COLORS` has no `completed` key, and `demo` printed raw under a heading reading "Processor". That is not an explanation — a customer has no reason to read `demo` as "this did not happen", and "processor" is not a word they have any reason to know. Two things now say it, for the same reason the cart needed two. The `demo` cell renders as a tag reading "Demo (not charged)", which marks *which* order. A notice above the table, shown only when there is one, says what that means — a tag reading "Demo" still assumes the reader knows what a demo order is, and what they actually want to know is whether to expect a parcel. Nothing changes on the backend: `orders.processor = 'demo'` was already written at checkout and already selected for this page. The row is a real row in a real table and stays visible, because hiding it would be its own kind of lie — the customer did do something, and it did have an effect on the catalogue. Written test-first against a browser: the new case drives a real demo purchase through the cart, opens `/orders`, and failed on both assertions before the change. Verified: 4 end-to-end tests in this spec pass, the 5 existing order-history tests still pass, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #205 Co-Authored-By: Claude Opus 5 (1M context) --- frontend/src/customer/Orders.tsx | 46 ++++++++++++++++++------ frontend/tests/e2e/demo-checkout.spec.ts | 26 ++++++++++++++ frontend/tests/e2e/pages/OrdersPage.ts | 4 +++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/frontend/src/customer/Orders.tsx b/frontend/src/customer/Orders.tsx index 1bd22ea..b70774c 100644 --- a/frontend/src/customer/Orders.tsx +++ b/frontend/src/customer/Orders.tsx @@ -38,7 +38,16 @@ const COLUMNS = [ dataIndex: 'status', render: (v: string) => {v} }, - { title: 'Processor', dataIndex: 'processor' }, + { + title: 'Processor', + dataIndex: 'processor', + // `demo` used to render as a raw column value, which is not an explanation: + // a customer has no reason to read it as "this did not happen", and the row + // was otherwise identical to a real one — real price, `completed` status, + // same neutral tag. The cart says it is a demo (#195, #203) for about three + // seconds; this is the record they come back to. See #205. + render: (v: string) => (v === 'demo' ? Demo (not charged) : v) + }, { title: 'Date', dataIndex: 'created_at', render: (v: string) => new Date(v).toLocaleDateString() } ]; @@ -69,6 +78,12 @@ function OrdersBody({ loading, error, orders, onRetry }: BodyProps) { ); } + // Shown only when there is one to explain. The per-row tag says which order, + // this says what it means — a tag reading "Demo" still assumes the reader + // knows what a demo order is, and the thing they actually want to know is + // whether to expect a parcel. + const hasDemoOrder = orders.some(o => o.processor === 'demo'); + if (orders.length === 0) { return ( @@ -79,15 +94,26 @@ function OrdersBody({ loading, error, orders, onRetry }: BodyProps) { } return ( - + <> + {hasDemoOrder && ( + + )} +
+ ); } diff --git a/frontend/tests/e2e/demo-checkout.spec.ts b/frontend/tests/e2e/demo-checkout.spec.ts index 333d398..70ab312 100644 --- a/frontend/tests/e2e/demo-checkout.spec.ts +++ b/frontend/tests/e2e/demo-checkout.spec.ts @@ -61,6 +61,32 @@ test.describe('Demo mode says so to the customer', () => { await expect(cart.demoNotice).toBeVisible({ timeout: 20000 }); }); + // The toast is three seconds; this is the record the customer comes back to + // when they wonder where their item is. It showed `demo` as a raw value under + // a "Processor" heading, beside a real price and a neutral `completed` tag — + // nothing a customer would read as "this did not happen". See #205. + test('order history marks the demo order rather than showing it as a real one', async ({ + page, + customer, + cart, + orders + }) => { + const name = `Demo h${uniqueSuffix()}`; + const item = await createItem(page.request, { name, price: '80' }); + expect((await page.request.post(`/api/cart/items/${item.id}`)).status()).toBe(201); + expect((await page.request.post('/api/customers/me/addresses', { data: ADDRESS })).ok()).toBe(true); + + await cart.goto(); + await expect(cart.checkoutButton).toBeVisible({ timeout: 20000 }); + await cart.checkoutButton.click(); + await expect(page.getByText(/nothing was charged/i)).toBeVisible({ timeout: 20000 }); + + await orders.goto(); + + await expect(orders.demoOrderMarker).toBeVisible({ timeout: 20000 }); + await expect(orders.demoNotice).toBeVisible(); + }); + test('the confirmation says nothing was charged', async ({ page, customer, cart }) => { const name = `Demo p${uniqueSuffix()}`; const item = await createItem(page.request, { name, price: '80' }); diff --git a/frontend/tests/e2e/pages/OrdersPage.ts b/frontend/tests/e2e/pages/OrdersPage.ts index b13357e..cc49341 100644 --- a/frontend/tests/e2e/pages/OrdersPage.ts +++ b/frontend/tests/e2e/pages/OrdersPage.ts @@ -14,6 +14,8 @@ export class OrdersPage { readonly continueShoppingButton: Locator; readonly backToShopButton: Locator; readonly anyDialog: Locator; + readonly demoNotice: Locator; + readonly demoOrderMarker: Locator; constructor(private readonly page: Page) { this.heading = page.getByRole('heading', { name: 'Order History' }); @@ -21,6 +23,8 @@ export class OrdersPage { this.continueShoppingButton = page.getByRole('button', { name: 'Continue Shopping' }); this.backToShopButton = page.getByRole('button', { name: 'Back to Shop' }); this.anyDialog = page.getByRole('dialog'); + this.demoNotice = page.getByText('Some of these are demo orders'); + this.demoOrderMarker = page.getByText('Demo (not charged)'); } async goto(): Promise { From 70d04186b15f32b9ae2aa8d5997617921922c4c8 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 28 Aug 2026 11:53:41 -0500 Subject: [PATCH 2/4] docs(ops): stop the compose header contradicting itself, and name the four variables step 2 dropped (#204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three corrections a review of #196 found, all in text #196 itself rewrote. **The compose header asserted something it falsified three lines later.** "All must be set in Portainer for this stack. All are secrets except DEMO_MODE" — but three PayPal entries are unused while `DEMO_MODE` is `true`, three more are marked Optional, and `SMTP_FROM` is not a secret. The runbook sends the operator to that exact block as authoritative, so an operator cutting over during the demo interim reads "all must be set", has no live PayPal credentials — which is the whole reason the interim exists — and either stops or invents a `BACKUP_PASSPHRASE`, which is how you get archives nobody can decrypt. The blanket claim is gone; each entry already says whether it is required and when. **Step 2 enumerated nine of the thirteen interpolated names.** `USPS_CLIENT_ID`, `USPS_CLIENT_SECRET`, `UPLOADS_BASE_URL` and `BACKUP_PASSPHRASE` were missing. The USPS pair is the one that matters, and it now gets a sentence of its own: losing it is the only silent failure in this step. Address validation is skipped when those are empty rather than failing, so checkout keeps working and quietly stops validating addresses, with no crash loop and nothing in step 7 that would notice. The list also now says to take everything the stack holds rather than working from the list, because an enumeration reads as a checklist however it is introduced. **The `DB_PASSWORD` failure was described wrongly, and its error names a variable the operator never typed.** It does not fail to authenticate against its data directory — the app never reaches a connection attempt, `checkAlwaysRequired` refuses at boot, and the message says `PGPASSWORD` because the compose file injects it as `PGPASSWORD=${DB_PASSWORD}`. An operator grepping for `DB_PASSWORD` finds nothing. That is now stated. The crash-loop examples were reordered to match the case the section claims to be about. It headlines "the likeliest outcome of a missed step 2", but the only block shown was the PayPal triple, which cannot occur during the demo interim, and #196 replaced it with a `DEMO_MODE`-only block that is not what a missed step 2 produces either. A wholesale miss is two problems led by `PGPASSWORD`; that is now first, with the `DEMO_MODE`-only and mistyped-value forms after it. Every quoted line was verified by running the real validator against production's entry set rather than by reading the source — `2 problem(s)` and `1 problem(s)` counts included — and all four now match character for character. The mistyped-value message is quoted in full rather than truncated, which was the point of the complaint that produced it. `DEMO_MODE` is no longer called "the only one that is not a secret", which was false of `SMTP_FROM` and `UPLOADS_BASE_URL`. It is the only one that is a setting rather than a credential, which is true and a better hook. Verified: 278 backend unit tests pass, including the compose guard that parses this file. Closes #204 Refs #196 Co-Authored-By: Claude Opus 5 (1M context) --- docker-compose.prod.yml | 6 ++++-- docs/ops/production-stack-cutover.md | 28 +++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 8b4ec8a..7c8341a 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -57,8 +57,10 @@ # (#190). Everything else is written out, so there is one place to look and one # thing that can be wrong. # -# Required stack environment variables. All must be set in Portainer for this -# stack. All are secrets except DEMO_MODE: +# The stack environment variables this file reads. Each entry says whether it is +# required and when — there is no blanket rule, because three are unused while +# DEMO_MODE is `true`, three are optional, and DEMO_MODE and SMTP_FROM are not +# secrets at all: # # DEMO_MODE `true` or `false`, exactly. Whether real payments are # taken. Not a secret — it is here rather than written diff --git a/docs/ops/production-stack-cutover.md b/docs/ops/production-stack-cutover.md index 43e31a1..81dedc3 100644 --- a/docs/ops/production-stack-cutover.md +++ b/docs/ops/production-stack-cutover.md @@ -60,9 +60,13 @@ Everything here is lost when the stack is deleted, and the rollback in step 8 is **The stack name**, exactly as Portainer shows it. If it is not `redefined-designs`, note that — the new stack must be created with that name, because the stack name becomes the compose project name and reusing QA's would make Compose reconcile the two against each other. -**Every stack environment variable, name and value.** They belong to the stack, and deleting it discards them. This is the step whose omission is felt hardest: the compose file interpolates `DEMO_MODE`, `DB_PASSWORD`, `SMTP_USER`, `SMTP_PASSWORD`, `SMTP_FROM`, `ADMIN_GATE_SECRET`, `PAYPAL_CLIENT_ID`, `PAYPAL_CLIENT_SECRET` and `PAYPAL_WEBHOOK_ID`, and an unset one substitutes to an empty string rather than failing. A missing `DB_PASSWORD` cannot authenticate against its own data directory, and none of this is recoverable from anything in this repository. +**Every stack environment variable, name and value.** They belong to the stack, and deleting it discards them. This is the step whose omission is felt hardest. The compose file interpolates thirteen names — `DEMO_MODE`, `DB_PASSWORD`, `SMTP_USER`, `SMTP_PASSWORD`, `SMTP_FROM`, `ADMIN_GATE_SECRET`, `PAYPAL_CLIENT_ID`, `PAYPAL_CLIENT_SECRET`, `PAYPAL_WEBHOOK_ID`, `USPS_CLIENT_ID`, `USPS_CLIENT_SECRET`, `UPLOADS_BASE_URL` and `BACKUP_PASSPHRASE` — and an unset one substitutes to an empty string rather than failing. None of it is recoverable from anything in this repository. Take everything the stack holds rather than working from this list; it is here to say how much there is, and it is checked against the file rather than from memory. -`DEMO_MODE` is the odd one out and the easiest to miss, because it is the only one that is not a secret. It must be exactly `true` or `false`. There is no default in the compose file, deliberately (#190) — a default would decide whether the shop takes money on the operator's behalf, silently, whichever way it pointed — so an unset `DEMO_MODE` refuses to boot rather than guessing. Production is `true` for now, the interim from #191; setting it to `false` is what restores real payments, and doing that without all three PayPal secrets present crash-loops the container. +`USPS_CLIENT_ID` and `USPS_CLIENT_SECRET` deserve naming because losing them is the one failure here that is completely silent. Address validation is skipped when they are empty rather than failing, so checkout keeps working and quietly stops validating addresses. Nothing in step 7 catches it, and there is no crash loop to notice. + +A missing `DB_PASSWORD` does not fail the way you would expect either: the app never reaches a connection attempt. It refuses at boot, and the message names `PGPASSWORD` rather than the variable you set, because the compose file injects it as `PGPASSWORD=${DB_PASSWORD}`. Grep the log for the name in the error, not the name in Portainer. + +`DEMO_MODE` is the odd one out and the easiest to miss, because it is the only one that is a setting rather than a credential. It must be exactly `true` or `false`. There is no default in the compose file, deliberately (#190) — a default would decide whether the shop takes money on the operator's behalf, silently, whichever way it pointed — so an unset `DEMO_MODE` refuses to boot rather than guessing. Production is `true` for now, the interim from #191; setting it to `false` is what restores real payments, and doing that without all three PayPal secrets present crash-loops the container. Copy them somewhere before you delete anything. @@ -221,14 +225,28 @@ Migration output must appear *before* `listening on 3000`, and `listening on 300 **What a crash loop looks like, and it is the likeliest outcome of a missed step 2.** A `[config] refusing to start` block, then the whole boot sequence again, repeating. -While production is in the demo interim, the likeliest one is `DEMO_MODE` itself: +Miss step 2 wholesale and it is two problems, led by a name you never typed: + +``` +[config] refusing to start — 2 problem(s) with the environment: +[config] - PGPASSWORD is required and is not set. +[config] - DEMO_MODE is required and must be exactly 'true' or 'false'. It decides whether real payments are taken, so it has to be stated rather than inherited. +``` + +Carry `DB_PASSWORD` across and only `DEMO_MODE` is left, which is the single likeliest form during the demo interim: ``` [config] refusing to start — 1 problem(s) with the environment: -[config] - DEMO_MODE is required and must be exactly 'true' or 'false'. +[config] - DEMO_MODE is required and must be exactly 'true' or 'false'. It decides whether real payments are taken, so it has to be stated rather than inherited. ``` -The other form appears once `DEMO_MODE` is `false` and real payments are on: +A value that was set but mistyped reads differently, and the quotes are the only thing that distinguishes `true ` with a trailing space from `true`: + +``` +[config] - DEMO_MODE must be exactly 'true' or 'false', but is 'True'. Anything else used to be read as demo mode, which meant a typo here quietly stopped the shop charging anyone. +``` + +The PayPal form appears once `DEMO_MODE` is `false` and real payments are on: ``` [config] refusing to start — 3 problem(s) with the environment: From 41840d4890e8a2ab3c278db5f4345bfecd68b635 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 28 Aug 2026 12:16:29 -0500 Subject: [PATCH 3/4] fix(email): stop a demo purchase telling real customers an item sold (#206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A demo purchase called `notifyFavoritersOfSale`, which mails everyone who favorited the item through production's configured SMTP: "An item you favorited has been sold to another customer, so it is no longer available… this one will not be restocked." Nobody bought it and nobody is shipping anything, so both halves are false. It is also the only outbound consequence a demo purchase has — everything #195 and #203 fixed is on screen, in front of the person who clicked and who has now been told it is a demo. These recipients never saw the cart. They just get told something they cared about is gone, and while production runs the demo interim (#191) they are real customers on real SMTP. The demo route no longer notifies. The PayPal capture and webhook paths are untouched, because those are sales. The item is still marked `sold`, so the storefront stays truthful about availability and the favoriter who goes looking finds what the database says. Only the claim that somebody bought it goes away. That a demo purchase permanently consumes real production inventory is a larger question than this issue and is left alone. Removing the call broke two tests and quietly hollowed out three more, which is the more interesting half of this change. Five tests in `favorites.integration.test.ts` used the demo purchase as a convenient way to make a sale happen; with the notification gone, the two asserting mail *is* sent failed, and the three asserting it is *not* sent would have passed for the wrong reason for ever. They were always about who gets told rather than about the demo route, so they now call the notifier the way the PayPal routes do — after the purchase, with the sold ids and the buyer. `buyThenNotify` says so at the point of use. Route-level coverage is unaffected: the admin mark-sold path already had its own test, and the new test asserts the demo route notifies nobody. Both halves were mutation-tested rather than assumed. The new test fails without the fix. Dropping the buyer exclusion from `collectFavoriteRecipients` fails "does not tell the buyer their own purchase is unavailable" and "emails every opted-in favoriter except the buyer" — so the restored tests are guarding the logic again rather than passing on an empty inbox. Verified: 255 integration tests pass (the suite needs `--runInBand`; these share one database), 278 unit tests pass, backend build clean. Closes #206 Co-Authored-By: Claude Opus 5 (1M context) --- backend/src/routes/cartCheckout.ts | 15 +++++- .../integration/favorites.integration.test.ts | 50 ++++++++++++++++--- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/backend/src/routes/cartCheckout.ts b/backend/src/routes/cartCheckout.ts index b585a29..ec708ed 100644 --- a/backend/src/routes/cartCheckout.ts +++ b/backend/src/routes/cartCheckout.ts @@ -246,9 +246,20 @@ router.post('/demo/purchase', requireCustomer, asyncRoute(async (req: Request, r const opened = await openCheckout(client, req.customerId as number, shippingAddressId, 'demo', `demo-${Date.now()}`); if (!opened.ok) { await client.query('ROLLBACK'); return res.status(400).json({ error: opened.error }); } - const sold = await completeCheckout(client, opened.checkoutId, 'demo', null, { demo: true }); + await completeCheckout(client, opened.checkoutId, 'demo', null, { demo: true }); await client.query('COMMIT'); - await notifyFavoritersOfSale(sold.itemIds, sold.buyerId); + + // Deliberately no notifyFavoritersOfSale here, unlike the PayPal capture and + // webhook paths above. A demo purchase is not a sale. The item really is + // marked sold, so the storefront stays truthful about availability, but the + // `favoriteSold` copy says the item "has been sold to another customer" and + // "will not be restocked" — and both are false when nobody bought anything. + // + // This is the only outbound consequence a demo purchase has. Everything else + // it does is visible to the person who clicked, who has been told it is a + // demo (#195, #203); these recipients never saw the cart and have no way to + // know. While production runs the demo interim (#191) they are real + // customers on real SMTP. See #206. res.json({ status: 'completed' }); } catch (err) { await client.query('ROLLBACK'); diff --git a/backend/tests/integration/favorites.integration.test.ts b/backend/tests/integration/favorites.integration.test.ts index 24e3b56..58aa054 100644 --- a/backend/tests/integration/favorites.integration.test.ts +++ b/backend/tests/integration/favorites.integration.test.ts @@ -7,6 +7,7 @@ jest.mock('../../src/mailer', () => ({ sendMail: jest.fn().mockResolvedValue(undefined) })); import { sendMail } from '../../src/mailer'; +import { notifyFavoritersOfSale } from '../../src/favoriteAlerts'; const sentMail = sendMail as jest.MockedFunction; beforeEach(async () => { @@ -168,36 +169,69 @@ describe('notifying when a favorited item sells', () => { expect(purchase.status).toBeLessThan(400); } + // The demo route deliberately notifies nobody (#206) and the PayPal path has + // no integration coverage, so the tests below call the notifier the way the + // PayPal capture and webhook routes do — after the purchase, with the sold + // ids and the buyer. That keeps them about *who* gets told, which is what + // they were testing all along; whether the demo route itself notifies is + // asserted separately, above. + async function buyThenNotify( + agent: ReturnType, + itemId: number, + buyerId: number | null + ) { + await buyViaDemo(agent, itemId); + await notifyFavoritersOfSale([itemId], buyerId); + } + it('emails a favoriter who opted in', async () => { const itemId = await createItem('Wanted item'); const { agent: watcher } = await register('watcher@example.com'); await watcher.post(`/api/customers/me/favorites/${itemId}`); await watcher.put('/api/customers/me/favorite-alerts').send({ enabled: true }); - const { agent: buyer } = await register('buyer@example.com'); - await buyViaDemo(buyer, itemId); + const { agent: buyer, id: buyerId } = await register('buyer@example.com'); + await buyThenNotify(buyer, itemId, buyerId); expect(soldNotificationsTo()).toEqual(['watcher@example.com']); }); + // A demo purchase is not a sale. The item really is marked sold, so the + // storefront is telling the truth about availability, but nobody bought + // anything and nobody is shipping anything — and while production runs the + // demo interim (#191) this mail reaches real favoriters through real SMTP, + // telling them an item "has been sold to another customer" and "will not be + // restocked". Both are false. See #206. + it('sends nothing when the purchase was a demo', async () => { + const itemId = await createItem('Demo bought'); + const { agent: watcher } = await register('watcher-demo@example.com'); + await watcher.post(`/api/customers/me/favorites/${itemId}`); + await watcher.put('/api/customers/me/favorite-alerts').send({ enabled: true }); + + const { agent: buyer } = await register('demo-buyer@example.com'); + await buyViaDemo(buyer, itemId); + + expect(soldNotificationsTo()).toEqual([]); + }); + it('does not email a favoriter who never opted in', async () => { const itemId = await createItem('Wanted item'); const { agent: watcher } = await register('silent@example.com'); await watcher.post(`/api/customers/me/favorites/${itemId}`); - const { agent: buyer } = await register('buyer2@example.com'); - await buyViaDemo(buyer, itemId); + const { agent: buyer, id: buyerId } = await register('buyer2@example.com'); + await buyThenNotify(buyer, itemId, buyerId); expect(soldNotificationsTo()).toEqual([]); }); it('does not tell the buyer their own purchase is unavailable', async () => { const itemId = await createItem('Self bought'); - const { agent: buyer } = await register('selfbuy@example.com'); + const { agent: buyer, id: buyerId } = await register('selfbuy@example.com'); await buyer.post(`/api/customers/me/favorites/${itemId}`); await buyer.put('/api/customers/me/favorite-alerts').send({ enabled: true }); - await buyViaDemo(buyer, itemId); + await buyThenNotify(buyer, itemId, buyerId); expect(soldNotificationsTo()).toEqual([]); }); @@ -210,10 +244,10 @@ describe('notifying when a favorited item sells', () => { await agent.put('/api/customers/me/favorite-alerts').send({ enabled: true }); } - const { agent: buyer } = await register('c@example.com'); + const { agent: buyer, id: buyerId } = await register('c@example.com'); await buyer.post(`/api/customers/me/favorites/${itemId}`); await buyer.put('/api/customers/me/favorite-alerts').send({ enabled: true }); - await buyViaDemo(buyer, itemId); + await buyThenNotify(buyer, itemId, buyerId); expect(soldNotificationsTo().sort()).toEqual(['a@example.com', 'b@example.com']); }); From c704c07b89d8deca42fc47a780032b911a97ded5 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 28 Aug 2026 11:43:55 -0500 Subject: [PATCH 4/4] docs(security): put the SQL injection invariant where it is enforced (#202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI". The three call-site comments are restored, with two corrections a review of the original found. They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added. This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker. `items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against. The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that. Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile. Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only. Verified: backend build clean, 280 unit tests pass. Closes #202 Refs #180 Co-Authored-By: Claude Opus 5 (1M context) --- backend/src/itemFilters.ts | 16 +++++++++ backend/src/routes/admin.ts | 20 +++++++++++ backend/src/routes/items.ts | 8 +++++ backend/tests/unit/itemFilters.test.ts | 49 ++++++++++++++++++++++++++ 4 files changed, 93 insertions(+) diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index ad98c98..ab43eee 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -239,6 +239,22 @@ export function parseItemFilters(query: Record): ItemFilters { // Returns WHERE fragments plus their parameters, with placeholders numbered // from `startIndex` so the caller can splice these in after its own params. // +// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses +// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and +// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable +// without signing in. So the only thing that may ever be interpolated into a +// string pushed onto `clauses` is a placeholder index: `$${next}`, or +// `$${next + 1}` in the tags clause. Every value goes onto `params` and is +// bound by the driver. Interpolating a filter value here would be SQL injection +// at both call sites, and `parseItemFilters` refusing malformed input is not +// what prevents it — these literals would be safe with no parser at all. +// +// Stated here rather than only at the call sites because this is where the rule +// is enforced and where a seventh clause would be added. SonarQube raised S2077 +// on the call sites and they are marked Reviewed/Safe (#180); that marking does +// not re-raise when this file changes, so this comment and the two tests over +// it are what stand between that edit and a live injection. See #202. +// // `favoritesCustomerId` is required rather than optional so a caller has to say // whose favorites it means, even when it means nobody's. Both routes already // reject a favorites filter they cannot satisfy, so reaching the throw below is diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index dd3cfd8..34a66ce 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -340,6 +340,18 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => { return res.status(400).json({ error: 'favorites is not a valid inventory filter' }); } + // S2077 flags every query below that assembles its SQL as a template literal, + // and this is the one where that is more than a formality: `where` really is + // built at run time. What makes it safe is that buildItemFilterSql composes + // only string literals written in itemFilters.ts. The only interpolations + // inside any of them are placeholder indices — `$${next}`, and `$${next + 1}` + // in the tags clause — numbers, seeded from the startIndex argument and + // incremented locally. Neither is ever derived from a filter value. + // + // So a caller chooses which of six fixed fragments are joined, and supplies + // every value in `params`, and neither of those becomes SQL. parseItemFilters + // rejects malformed input above, but that is defence in depth rather than the + // reason this holds — the clause literals would be safe without it. const { clauses, params } = buildItemFilterSql(filters, 1, null); const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : ''; const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params); @@ -367,6 +379,11 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons await setItemTags(client, item.id, await resolveTagIds(client, tagNames)); } await client.query('COMMIT'); + // S2077 again, and here the template is a module constant plus a literal: + // ADMIN_ITEM_SELECT interpolates nothing of its own, and the id is bound as + // $1 rather than formatted in. Same shape as the update route below, where + // the bound value is caller-supplied — which is precisely why it is a + // parameter. const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [item.id]); res.json(requireRow(full, 'the item just inserted')); } catch (err) { @@ -415,6 +432,9 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp await insertItemImages(client, Number(req.params.id), files, nextSort); } await client.query('COMMIT'); + // S2077, the same constant-plus-$1 shape as the create route above. + // req.params.id is caller-controlled and goes through the driver as a bound + // parameter; it never reaches the query text. const { rows: full } = await pool.query(`${ADMIN_ITEM_SELECT} WHERE i.id = $1`, [req.params.id]); res.json(full[0]); } catch (err) { diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index 290c9ff..ee54a29 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -73,6 +73,14 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { }; const { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null); + // The same construct SonarQube flagged as S2077 in admin.ts and which is + // marked Reviewed/Safe there (#180) — and this is the copy reachable without + // signing in, so it is worth saying here too rather than relying on the + // reader having seen the other one. It holds for the same reason: the clauses + // are literals from buildItemFilterSql carrying only placeholder indices, and + // EXCLUDE_PENDING is a module constant. Joining with AND cannot weaken + // EXCLUDE_PENDING either, because no fragment contains a top-level OR for the + // join to re-associate against. const where = [EXCLUDE_PENDING, ...clauses].join(' AND '); const { rows } = await pool.query( `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`, diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 845aaf6..5dce722 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -256,3 +256,52 @@ describe('buildItemFilterSql', () => { expect(sql).toContain('$3'); }); }); + +// Both callers splice these clauses straight into query text, so a value +// reaching the clause string is SQL injection rather than a style problem. The +// comment on buildItemFilterSql says so; these two make it fail a build instead +// of relying on someone reading it. See #202, and #180 for the S2077 review. +describe('buildItemFilterSql keeps every value out of the SQL text', () => { + // Deliberately built by hand rather than through parseItemFilters, because + // the claim is that the clause literals are safe with no parser at all. These + // values could never survive parsing, which is the point: the parser is + // defence in depth, not the reason this holds. + const HOSTILE = "1); DROP TABLE items; --"; + const hostileFilters = { + categoryIds: [HOSTILE], + tagIds: [HOSTILE], + minPriceCents: HOSTILE, + maxPriceCents: HOSTILE, + status: [HOSTILE], + favoritesOnly: true + } as unknown as Parameters[0]; + + it('never lets a filter value reach a clause, even one the parser would reject', () => { + const built = buildItemFilterSql(hostileFilters, 1, HOSTILE as unknown as number); + const sql = built.clauses.join(' AND '); + + expect(sql).not.toContain(HOSTILE); + expect(sql).not.toContain('DROP TABLE'); + // Every value still arrives, bound, where it can do nothing. + expect(built.params).toContain(HOSTILE); + }); + + // The structural version of the same claim, and the one that catches a value + // which happens not to look hostile: the SQL text must not depend on the + // values at all. Two disjoint sets of inputs, byte-identical clauses. + it('produces byte-identical SQL for two completely different filter sets', () => { + const a = buildItemFilterSql( + parseItemFilters({ category: '4', tags: '7,8', min_price: '100', max_price: '900', status: 'sold' }), + 1, + 42 + ); + const b = buildItemFilterSql( + parseItemFilters({ category: '99', tags: '11,12', min_price: '5', max_price: '6', status: 'available' }), + 1, + 7 + ); + + expect(a.clauses).toEqual(b.clauses); + expect(a.params).not.toEqual(b.params); + }); +});