be6800181d66592e68f6003fa128a25f239be201
87
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
b616b9f0ab |
fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)
SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one. The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing. That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable. The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads. `verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss. The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it. Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass. Refs #180 |
||
|
bermudalamb
|
e3842b1a4c |
ci: fail at the end rather than part way through, so the scan still runs (#174)
`sonarqube.yml` already had a documented design for this: run every suite, produce coverage, scan, summarise, then fail at the end from recorded step outcomes. The comments on the gate spell it out and #142 fixed it once already. The integration suite was never wired into it — no `id`, no `continue-on-error`, and absent from the gate, where the unit and end-to-end suites had all three. So it was the one suite whose failure aborted the job. On every push since #154 started, step 9 failed and steps 10 through 15 were skipped, which means there has been no SonarQube analysis at all for the duration — not a degraded one, none. The end-to-end suite has not run in CI either, which is separately why #116 cannot be verified: the step that would demonstrate its fix is skipped rather than failing. Fixing only that step would have left the same shape in four other places, so every step from the first suite to the scan is now guarded and named in the gate: starting the backend, installing browsers, merging frontend coverage, and the scan itself, which until now took the summaries down with it. The preconditions before the suites — checkout, installs, build checks, migrations — still fail hard, because when they fail there is genuinely nothing to analyse. The job still fails. It fails at the end, having produced everything it could. The integration suite also gains the summary the other two already had. It was the only suite without one, so the failure this workflow has been stuck on presented as 36 assertion errors about categories and price filters rather than as a count — #154 records how expensive that misdirection was to read. `summarize-jest.js` already takes a label, so this is reuse. `tests/unit/workflowGate.test.ts` asserts the pairing that makes the design work: a step carries `continue-on-error` so it cannot abort the job, and the gate names it so it can still fail the job. Both halves are needed and nothing connected them, which is how this happened — and the other direction is worse, since a step guarded but unnamed cannot fail the job at all. The test checks the invariant rather than a list of names, for the same reason `composeEnvironment.test.ts` reads the real list rather than a copy. Verified by mutation: dropping the integration suite from the gate, un-guarding it, and removing `always()` each fail it. Confirmed by running the new flag combination rather than assuming it: the integration suite writes `integration-results.json`, the summariser reads it and exits 0, and both that file and `coverage/integration/lcov.info` are still written when the suite fails — which is what makes scanning with a failing suite produce real coverage rather than a fabricated regression. Closes #174 |
||
|
bermudalamb
|
cf1680dbfb |
feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)
The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103 |
||
|
bermudalamb
|
faf38be91a |
feat(filters): make the storefront filter panel searchable and multi-select (#139)
The filter drawer did not scale with the taxonomy behind it. Categories were a bare antd `Tree` rendered at whatever depth it had grown to, with no search and single selection, and tags were a wall of every tag in the system. Neither said what was selected except through highlighting and chip colour. Both are now searchable multi-selects. Categories keep their hierarchy in a `TreeSelect`, matching the admin's `CategoryTreeSelect` so the two screens behave alike; tags become a multiple `Select` whose selected pills keep their colours, which is the only place a tag's colour was load-bearing. Several categories combine as OR. A customer picking Furniture and Decor wants both, not the empty intersection, and each selected id still expands to its descendants, so the answer is the union of the subtrees. That is deliberately the opposite of the tag rule, which stays AND, and both headings now state their rule rather than leaving it to be discovered. `ItemFilters.categoryId` becomes `categoryIds` end to end. The recursive CTE is seeded with `= ANY($n::int[])` rather than one id, which walks every selected root in one recursion and gives the OR for free; matching on `IN` keeps it a set test, so an item under two selected branches still appears once. The query parameter keeps its singular name and becomes comma-separated, the shape `tags` and `status` already use, so every `?category=1` link written before this still parses as a list of one. A list containing anything unreadable is still a 400, per decision 9 — honouring the readable half would answer a narrower question than the one asked and look indistinguishable from a filter that worked. The admin's inventory filter stays single-select, since it asks what is in a category rather than in any of several, but reads and writes a list of at most one so there is one shared filter type rather than two that drift. Closes #139 |
||
|
bermudalamb
|
f32913ef51 |
refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)
Indexing an array now yields `T | undefined`, which is what it always did — the compiler simply says so. Enabled in backend/tsconfig.json and frontend/tsconfig.json, and in tsconfig.sonar.json alongside it so the drift guard stays satisfied. The sequencing this issue insisted on was right, and the numbers show why. Before #159 typed the query results, `rows[0]` was `any` and `any` indexes to `any`: the flag would have found close to nothing and the count would have changed completely afterwards. With the rows typed it finds 57 — 50 in the backend, 4 in the frontend, 3 in the Playwright suite — and they fall into three kinds. Thirty are `rows[0]` after a `rows.length` guard. TypeScript cannot connect the two, and rewriting them as `const [row] = rows; if (!row) …` makes the guard and the use the same check, which is better code independently of the flag. Ten are rows a statement guarantees — `INSERT … RETURNING`, or a lookup for an id the session middleware has already matched. These get `requireRow(rows, what)`, a new helper in db.ts that throws naming the query. A thrown error rather than a non-null assertion: if the assumption is ever wrong, an assertion hands `undefined` to the next line and fails somewhere unrelated, whereas this fails at the query and says which. asyncRoute turns it into a 500, which is the right answer for "the database did not do what the statement says it does". It also states the assumption once instead of ten times. The rest is ordinary indexing the compiler cannot prove: a regex capture group that the pattern guarantees, `split('+')[0]`, a modulo kept in range, `hasOwnProperty` failing to narrow an index signature, and Express typing route params as an index signature so `req.params.itemId` is `string | undefined` on a route that cannot match without it. One correction to this issue's premise, which matters for what it was expected to find. The body says "in a handful it does not guard at all", and the unguarded-500 risk it describes was not found. Every `rows[0]` either sits behind a length check or behind a statement that guarantees a row. What the flag actually bought was the ten places where that guarantee was real but unstated, and those now say so. Two changes worth calling out because they are not mechanical. TAG_COLORS is typed `[string, ...string[]]` in both copies rather than `as const` — the first attempt used `as const`, which narrowed the elements to literals and broke adminTags, so the annotation keeps `string` while telling the compiler index 0 exists. And the filter drawer's slider falls back to the bounds it was given rather than to null, because null there reads as "no filter" and would widen the results rather than leave them unchanged. Test files needed changes too, since ts-jest compiles them against the same config: a regex destructure in the compose guard, and ten `mock.calls[0][0]` reads where the surrounding assertions already establish the call happened. Verified: tsc clean over backend, frontend src and the Playwright suite; unit 254/254; integration 238/238; frontend build clean; lint unchanged in both workspaces. Closes #101 |
||
|
bermudalamb
|
3fde6fc6bf |
fix(deploy): commit production's compose and bring it under the drift guard (#118)
Production refused to boot with "UPLOADS_DIR is required and is not set" while UPLOADS_DIR was set in Portainer's stack variables. Both statements were true at once. Portainer substitutes stack variables into the compose file rather than handing them to the container, so a variable with no line in the file never reaches the app — behaviour the QA compose already warns about at its ADMIN_GATE_SECRET entry, hit in production where nothing was watching for it.
Nothing could have caught it. The drift guard reads docker-compose.qa.yml, and production ran from a Portainer stack outside the repository that no test could see. That is worse than an even gap: UPLOADS_DIR is in ALWAYS_REQUIRED and the test was green, so the natural reading was that the deploying environments set it. QA did. Production did not.
So production's compose is now a file in the repository, deployed as a git repository stack rather than pasted into the web editor — otherwise the committed copy and the running copy drift apart again, which is the whole problem.
Values are hardcoded rather than interpolated wherever they are not secrets. Only a secret has a reason to stay out of the repository, and every interpolation is another chance for the failure above. UPLOADS_DIR in particular has to agree with the volume mapping, and splitting it across two files is how they drift.
The guard now runs over every deployment rather than QA alone, and checks each by handing its parsed entries to validateEnv itself rather than restating the rules. A restatement is one more copy to drift; running the real validator means the file is checked against exactly what the container checks at boot. Interpolated ${SECRET} values count as present, which is right — what is being guarded is that the line exists, since that is what decides whether the value reaches the container.
Environments differ on purpose, so the expectations are registered per file rather than shared: QA is demo mode with a mail allowlist and no PayPal credentials, production is the reverse of all three. A root-level compose file that is not registered fails the last test, so adding an environment forces the decision instead of silently inheriting whatever the loop asserted.
Verified by removing the UPLOADS_DIR line from the production file and confirming three tests fail, one of them reproducing the exact boot error. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot.
Two things found while writing this and deliberately not changed here. RESERVATION_MINUTES is set in QA's compose and read nowhere in the code — drift in the opposite direction, which #118's scan half should catch. And production publishes 32750 on every interface exactly as QA does; that is #117, and the reasoning is recorded in a comment at the ports block rather than acted on, since changing it needs the proxy host entry repointed in the same pass.
Refs #118
|
||
|
bermudalamb
|
2f7268704a |
feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)
Giving the customer emails a tab of their own (#135) made a family of related holes visible: an admin could edit the wording of every customer email, but not the facts that wording asserted, and two templates could not address the customer at all. Both token lifetimes were hardcoded twice. VERIFY_TOKEN_TTL_MS sat in routes/customers.ts while the verification body separately said "This link expires in 24 hours", and RESET_TOKEN_TTL_MS sat beside a body separately saying "one hour". The prose was not derived from the constant, it was a second hand-written copy of the same fact — so making the constants configurable without addressing that would have made things worse, not better: the setting moves to two hours and the email keeps confidently promising one. Both are now settings, and both templates state their lifetime through an {{expiresIn}} placeholder rendered from the setting. The cart reminder gains {{holdDuration}} for the same reason. Per-item deadlines were already inside {{itemList}}, but there was no way to write a sentence about the hold itself without hardcoding a number the cart expiry setting could change underneath the author. All three durations render through one formatDuration(), so the reset email and the cart reminder say "one hour" the same way rather than in two authors' phrasing. A fractional hour drops to minutes, because "0.5 hours" reads badly and "1.5 hours" reads worse in a sentence a customer is meant to act on. Every template now offers greeting, firstName and lastName. favoriteSold and favoriteWithdrawn previously offered only itemName and siteUrl and could not address anyone — the query behind them never selected a name, so it does now. The greeting itself is two settings, a format and a fallback, rather than the wording baked into greeting(). The fallback is separate rather than the format with the name edited out: that editing is guesswork that has to be right every time, and getting it wrong ships "Hi ," to everyone who registered while first names were still optional (#106). Those customers exist, which is why greeting() guarded the case in the first place. The admin preview renders the durations and the greeting from the live settings rather than from a static sample. The preview exists so an admin sees the email that will be sent, and a sample reading "one hour" while the setting says two is the precise failure these placeholders were added to remove. Settings are read through a new adminSettings accessor. cart_expiry_hours was previously read by an inline query in two places, each with its own `|| '24'`; with five settings and read sites across three routes and the cron job, a default written twice is a default that will eventually disagree with itself. Values are stored as text, so each definition declares how to read it back — numbers were the only kind until the greeting format arrived. The new placeholders are available but never required, so every template an admin has already saved keeps rendering and keeps sending. Also fixes updateAdminSettings announcing success for a save the server refused: it returned the 400 body as though it were the saved settings, so the form reported "Settings saved" either way. Closes #136 |
||
 bermudalambandClaude Opus 5
|
b8549e9c72 |
feat: let a customer resend their own verification email (#110)
A verification email was sent once, at registration. If it was lost, filtered, or arrived after the 24-hour token had expired, the customer had no route back except registering again with a different address. POST /api/customers/resend-verification mints a fresh token and sends the mail, and the account page grows a "Send it again" button beside the warning that was already there. The button only exists while the address is unverified: on a verified account it would be a control whose only outcome is a refusal, and the endpoint refuses that case rather than sending a pointless email. The mint-token-and-send sequence now lives in one place. Registration and the email change already did the same three steps - supersede any outstanding token, mint a new one, send it - and this would have been a third copy. The step most likely to be dropped in a copy is the supersede, and it is the one that matters: without it an older message still sitting in the inbox goes on verifying. Anything that makes the server send mail on request is an abuse vector, so this is rate limited to three an hour, keyed on the customer id. That is tighter than either existing limiter and sidesteps #84's IPv6 problem entirely, since a signed-in caller has an identity better than an address to count against and cannot escape the bucket by moving within a delegated prefix. The refusal says the mail probably did send and to check the spam folder, which is both more useful and more honest than a bare 429. The claim that keying on customer id also solved test isolation was wrong, and the tests caught it. resetDb truncates with RESTART IDENTITY, so every integration test's first customer is id 1: three tests that each sent once left the fourth starting at its limit, and two tests failed on a 429 they never asked for. A "fresh customer per test" is a fresh row with a recycled identity. The limiter now has an explicit exported store the suite clears between tests, and the comment that claimed otherwise has been corrected rather than left to mislead the next reader. Verification: seven integration tests covering the unauthenticated refusal, the send, the new token invalidating the old one - asserted through the endpoint that would honour the stale link rather than by counting rows - the new link actually verifying, the already-verified refusal, the allowance stopping the fourth send rather than merely reporting it, and one customer's exhausted allowance leaving another's intact. Three end-to-end tests for the button, its confirmation and the message on the fourth click. The 33 integration tests across the three suites this touched all pass, as do the 199 backend unit tests. tsc clean on both sides, ESLint no errors. Closes #110 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
32b3f616d9 |
feat: filter by Sold / Not sold / All on the storefront and the admin (#105)
Three decisions were taken before any code, and are recorded on the issue. The status filter is generalised to accept several values rather than gaining a second `sold` dimension beside it. "Not sold" is not a status: it is available-or-reserved on the storefront and includes pending in the admin, neither of which is one value. `?status=available,reserved` and `i.status = ANY($n::text[])` express that with one concept, so there is no way to write a contradiction like `?status=sold&sold=no`. A single status still parses to a list of one, which is how the admin's existing `?status=sold` keeps working untouched. The storefront now defaults to Not sold. That is a change in what every customer sees, not just a new control: the black SOLD ribbons leave the default view on a catalogue where they were evidence the shop sells things, and every storefront link shared so far quietly changes meaning. Accepted deliberately, with the default named in STOREFRONT_DEFAULT_STATUSES rather than implied by the absence of a parameter. The admin's four-way status dropdown is replaced rather than joined. That gives up isolating a single status: there is no longer a way to view only Reserved, or only Pending, and Not sold folds pending in with the rest. The pending workflow from #90 is the likeliest thing to miss it, and if it does, the fix is to put isolation back beside the preset rather than to remove the preset. The e2e test that covered "which is how Reserved is reached" is renamed and narrowed to what survives, rather than deleted. One thing the issue did not anticipate, found by a test rather than by reading. The favorites view deliberately showed sold favorites - "a favorite that has just sold is often exactly what the customer came to look at", and they have just been emailed to say so. Defaulting the storefront to Not sold reversed that silently and broke the test asserting it. Favorites therefore keep their own default of everything, on the server and in the control's displayed position, while an explicit ?status= still wins. That interaction is the kind a single-feature change quietly breaks, and it was caught only because the previous decision had been written down as an assertion. "All" still means different things in the two places, as the issue set out: available + reserved + sold on the storefront, all four in the admin. Pending remains unreachable from every public read - the storefront's unconditional exclusion clause is untouched - and the pending guard now checks every requested status rather than a single one, so `?status=available,pending` is refused for naming pending at all rather than accepted because the first name happened to be allowed. The control sits in the filter bar rather than in the drawer, since the default now hides sold pieces and a customer who never opens the drawer would otherwise have no way to know they exist. It is consequently excluded from the "Filters (N)" count, which describes the drawer, while still counting toward hasActiveFilters so that an empty result reads as "no items match these filters" with a way out rather than as an empty shop. Verification: 13 integration tests covering the default, each preset, the favorites exception and its override, and pending's unreachability under every accepted combination; 38 parser unit tests including multi-value parsing, an unknown name in a list being refused rather than dropped, and a list that names nothing; 6 new end-to-end tests for the storefront control, its URL round-trip, and the default staying out of the URL. 204 backend unit tests and 76 integration tests across the four affected suites pass. Two full end-to-end runs: 110 and 111 passing against the same 3 pre-existing failures, one run also showing a pending-publish failure that passes in isolation and did not recur - the cross-suite database contention filed as #116. tsc, ESLint and the production build are clean. Closes #105 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1fa723bd19 |
feat: tabs and a rendered preview for the email templates (#119)
Follow-up to #92, which shipped the editable templates as a column of stacked cards. With six templates the cart reminder sat below five editors, so reaching it meant scrolling past all of them and which one you were editing was knowable only from a card title you had already scrolled past. They are tabs now, and the Default/Customised tag moves onto the tab label, so which templates have been changed is visible without opening each one. The larger gap was that there was no way to see what the email would look like. The editor is a markdown textarea; what gets sent is rendered HTML with placeholders substituted and, for the two favorite templates, a consent footer appended by the server. An admin editing copy could not tell whether the result read correctly. POST /api/admin/email-templates/:key/preview renders the draft in the editor rather than what is stored, so the effect of an edit is visible before committing to it. It renders on the server deliberately: renderTemplate is the only thing in the system that turns this markdown into HTML, and markdown-it is configured there with html: false, which is the control that stops an admin putting script into a customer's inbox. A renderer in the browser would be a second implementation of both, and a preview that disagreed with the mailer would be worse than none. It does not enforce required placeholders - saving refuses a body that dropped one, and previewing it is how the admin sees what they have done. The preview renders into a sandboxed iframe rather than through dangerouslySetInnerHTML. The markup is safe by construction, but an email is its own styling context: rendered inline, the admin theme's CSS would change how it looks and the preview would lie about the result. Sample values live beside the template definitions rather than in the route, so adding a placeholder puts the missing sample next to the change that needs it. A unit test asserts every available placeholder has one, because a missing sample renders a literal {{placeholder}} into the preview and teaches the admin their copy is broken when it is not. This also fixes a test that has been failing on main. email-templates.spec.ts located the Save button by filtering .ant-card for the template name, which matched an outer card containing every template's Save button - six of them - and died on a strict mode violation, taking two more tests with it as unrun. Only the active tab's editor is mounted now, so the labels are unambiguous and the filter is gone. Verification: eight end-to-end tests, four for editing and four for the preview, covering the draft being previewed rather than the stored copy, sample values replacing placeholders, raw HTML being escaped exactly as the mailer escapes it, and the consent footer appearing on a favorite template and not on a password reset. The full suite goes from 100 passed / 3 failed / 2 unrun to 112 passed / 2 failed / 0 unrun; the two that remain are the pre-existing password-reset failures that need a database on port 55432 and fail identically on main. 38 backend unit tests pass, tsc and ESLint are clean. Closes #119 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7c3463650d |
feat(backend): let a customer change their own name, password and email (#111)
SonarQube Analysis / sonarqube (pull_request) Failing after 12m11s
Two of the three already existed on the backend and had no caller. PUT /api/customers/me updated the name; POST /api/customers/change-password already demanded the current password and enforced the eight-character minimum. Neither was reachable from the frontend, which is why the gap was easy to miss — the API looked finished. The name endpoint accepted empty values and wrote nulls, letting a customer clear fields registration refuses to let them skip. That is the same rule disagreeing with itself, so it now refuses each by name exactly as registration does. Changing a password now ends other sessions and keeps the one making the change. Reset already deleted every session for the customer, on the reasoning that a password is changed precisely when the old one may be known to someone else — change reached the opposite conclusion for no recorded reason, and a session opened with a leaked password outlived the change meant to lock it out. The current session is spared so the change does not eject the person making it. Changing the email address is new. It asks for the current password, because swapping the address a password reset goes to is how an account is taken over and a live session alone is not enough; that also matches what change-password already required. The address is normalised and validated, an address another account holds is refused with the same 409 as registration, and on success the row is marked unverified and any outstanding verification token superseded — one already sitting in the old inbox must not be able to verify the new address. Two emails then go out, to different places. Verification to the new address, and a notice to the old one naming what the address was changed to. The notice is the only thing that tells a real owner their account was taken, and one that does not say where the address went is nearly useless to someone checking whether it was them. Both sends happen after the row is written, never before, so a change that failed cannot produce mail saying it succeeded. That notice is a sixth template in #92's system, which cost a definition and a default body. The unit tests iterate every template, so its defaults were checked against its own required placeholder without writing a new test. Verified: 199 unit and 208 integration passing. The session test signs in on a second agent, changes the password on the first, and asserts the second is refused while the first still works — the property being claimed rather than the code path being executed. One of my own assertions was wrong on the way: /me answers an unauthenticated caller with 401 and an error body, not an empty one, and the frontend is what turns that into null. Refs #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4ed9513ad2 |
feat(backend): make the five customer emails editable copy (#92)
Every customer email was a template literal in the route that sent it, so changing a word meant a code change, a review and a deploy. All five now render from markdown that an admin can edit: verification, password reset, favorite sold, favorite withdrawn, and the cart reminder. Five, not the four the issue counted — the favorite alerts have separate copy for sold and withdrawn.
markdown-it runs with html disabled, which is its default and the reason for choosing it over marked. Raw HTML in a stored body is escaped rather than passed through, so editing copy cannot put script into a customer's inbox. That is a stronger guarantee than sanitising output, because there is no output to sanitise.
Values are substituted into the markdown before it renders, which means a value that should become a list has to arrive as markdown. The cart reminder previously built li elements by hand; those would now be escaped and shown to the customer as literal angle brackets, so it emits a markdown list instead. The greeting is one placeholder rather than a bare name, so a template author writes {{greeting}} instead of "Hi {{firstName}}," — which reads as "Hi ," for anyone who registered before first names were required.
Saving is refused when a body has dropped a placeholder it needs, naming all of them rather than the first. This is the rule that separates a convenience from a way to break password resets from a settings screen: a reset email with no link still sends, still looks correct in the log, and is useless to everyone who receives it.
The favorite alerts' consent sentence is appended by the server and is not editable. It explains why the customer is receiving the mail, which is a compliance artifact rather than copy, and editing wording should not be able to delete it.
Unset templates fall back to the built-in defaults, so an install that never touches the settings screen behaves exactly as it did. The API reports an uncustomised template as null rather than as its default text, so "never edited" stays distinguishable from "edited to something identical", and DELETE restores the default by forgetting the row rather than writing the default into it.
Two problems surfaced during verification, both worth recording.
Five favorite-alert tests failed with no error and no mail. The cause was not this code: resetDb does not truncate admin_settings, so a subject of "Gone" stored by the new template tests survived into a later suite and changed the mail it was asserting on. Cleaning up inside the template tests would have fixed only that pairing, so resetDb now clears stored templates for every suite — template rows are test data like any other, and one outliving the suite that wrote it makes a failure appear somewhere unrelated.
The withdrawal notification then failed on timing. Loading copy from the database made the sender async, and the removal path was fire-and-forget, so the response could beat the mail out of the door. Dispatch was previously synchronous even though the sends themselves were not awaited; that is now restored by awaiting it.
Verified: 197 unit and 195 integration passing, lint unchanged at 4 warnings. The admin screen for editing these follows in the next commit.
Refs #92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
b287c07747 |
feat: capture first and last name so emails can greet informally (#106)
Registration collected one optional Name, so every greeting had only a whole name to use: "Hi Thom Lamb," rather than "Hi Thom,". Both parts are now captured, and the cart reminder greets by first name. Both are required of anyone new, refused individually rather than as a single "name is required" so a form that filled one and not the other is told which. The columns are nullable even so, and that is deliberate. Marking them NOT NULL would mean backfilling legacy rows with empty strings, which asserts that every customer has a name — untrue of anyone who registered while the field was optional. The table records what is actually the case; the rule that new registrations must supply both lives in the route, where a missing field can produce a message naming it. The backfill splits on the first space, and it is lossy in a way no version of this avoids. "Thom Lamb" becomes Thom and Lamb; "Mary Jane Smith" gets a last name of "Jane Smith"; names that are not two parts fare worse. It was chosen over leaving the columns empty because nothing currently lets a customer correct their own name — PUT /api/customers/me exists but no frontend calls it — so empty would have meant permanently unpersonalised for every existing customer. The migration says so, so nobody later reads backfilled values as data the customer supplied in that shape. Verified against a seeded database rather than reasoned about, because this is the part that cannot be covered by the suite: migrations run in globalSetup before any test, and the old column is gone afterwards. Six representative rows through the real migration gave Thom/Lamb, Mary/"Jane Smith", Cher/null, " Padded Name " trimmed to Padded/Name, and null and whitespace-only names left as null on both. The down migration rejoins the parts and returns all six to their original strings. The old column is dropped rather than kept alongside, so there is one source of truth instead of two that drift. The admin keeps receiving a single composed display name. It only ever shows one — the list cell and the drawer title — and never edits one, so giving it both parts plus the joining logic would be work for no reader. Churn was the bulk of this: 14 backend registrations and 10 end-to-end registration forms. A first attempt at the backend fixtures also added names to login and password-reset payloads, which would still have passed since the server ignores unknown fields, but a login test implying login takes a name is a small lie; that was reverted and redone against register calls only. Verified: 172 unit, 183 integration and 95 end-to-end passing, lint unchanged at 4 backend and 27 frontend warnings. Not covered: the cart reminder itself, which runs from a cron and had no test before this either. The greeting change is a one-line substitution in that query. Refs #106 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
f76db6c8fe |
fix(test): make the compose guard survive a CRLF checkout (#107)
The guard added for #107 finds nothing on a checkout with CRLF line endings, which is every fresh clone on Windows. Splitting on a bare newline leaves a trailing carriage return, the end-of-line anchor in the entry pattern then cannot match, and all ten assertions in the file fail together. Worth being precise about why this shipped, because the process that was supposed to prevent it ran and did not. That guard was fired deliberately before committing: the UPLOADS_DIR line was removed, two tests failed, the line was restored, ten passed. What the exercise never varied was the file's line endings — and by then the working copy happened to be LF, because the backup-and-restore used to fire the guard had rewritten it that way. So the deliberate firing proved the guard catches a missing variable, on a file shaped exactly as the test run had shaped it, and proved nothing about the shape it meets in a clean clone. The failure mode is the one the file already worried about: parsing that matches nothing makes every other assertion vacuously true. Here it failed loudly instead only because the "parsed some entries at all" case exists — which is the case that turned a silent pass into a visible failure, and is the reason this was noticed at all rather than sitting green and checking nothing. Splitting on an optional carriage return fixes it. 172 unit tests pass on the CRLF checkout that was failing. Found while verifying #106, whose branch could not go green until this was fixed, which is why the fix lands there rather than on its own. Refs #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
a2500a9901 |
test(backend): fail the build when the compose file lacks a required variable (#107)
The one-line compose fix in the previous commit unblocks QA. This is the part that stops it happening again, and it is the more useful half. The failure was not really a missing variable. It was that nothing connected two files: envValidation.ts gained a required variable, docker-compose.qa.yml did not set it, and nothing noticed until a container refused to boot on deploy. CI passed the whole time, because CI supplies its own environment and never reads the compose file — which is exactly why "CI is green" was the wrong evidence to have offered. So a unit test now reads the compose file and asserts it sets everything the validator demands. It imports ALWAYS_REQUIRED rather than restating it, which is the only version of this test worth having: a copied list would pass forever while the next variable added to the validator went unguarded in precisely the same way. Two further assertions earn their place. UPLOADS_DIR is hardcoded rather than taken from a stack variable on the grounds that it must agree with the volume mapping, so the test checks it against the mount rather than leaving that a claim in a comment. And ADMIN_GATE_SECRET must be present as an interpolation rather than a literal, since a secret in the repository would defeat the point of having one. There is also a test guarding the test: a regex that matched nothing would make every other assertion in the file vacuously true, so one case asserts that parsing found entries at all. Fired deliberately rather than assumed. Removing the UPLOADS_DIR line reproduces the original failure as two failing tests; restoring it returns to ten passing. A guard that has only ever been observed passing is not known to guard anything. What this cannot do is check production, which runs from a Portainer stack outside this repository. That gap is now written into the README beside the validation rules, along with the reason a variable set only in Portainer's stack UI never reaches the container: stack variables are interpolated into the compose file, not handed to the service. 172 unit tests pass, lint unchanged at 4 warnings. Refs #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
35b242a66f |
feat(backend): accept only real images in the inventory upload (#95)
The upload bounded size and count and nothing else: POST /api/admin/items would take a PDF, a zip or an executable and store it as an item image, under an extension copied from whatever the caller named their file. Those files are served by express.static from the application's own origin, so a stored .html came back as text/html and a .svg as image/svg+xml — both able to run script as the site. Three types are accepted: JPEG, PNG and WebP. SVG is excluded deliberately even though it is an image, because it executes script when navigated to directly, which is the exposure #103 describes; a photograph of a one-of-a-kind item is never a vector drawing, so nothing real is lost. GIF is excluded as simply not wanted for product stills. Validation happens twice, because once is not enough. The declared content type is checked in multer's fileFilter, before a byte is written — that catches picking a PDF by accident, which is most of what goes wrong. But file.mimetype is whatever the caller wrote in the multipart headers, so the bytes are checked too: each stored file's leading bytes must match the format it claimed. That is what stops evil.html renamed to photo.jpg and declared image/jpeg, which an allowlist on the declared type alone waves straight through. The byte check cannot live in fileFilter — that runs before multer has read the stream, so there is nothing to look at yet. It runs after the write instead, and a failure removes every file from the request rather than only the offending one: accepting the good half of a refused upload would leave files on the volume that nothing references. Handles are closed before anything is unlinked, because an open handle makes the unlink fail on Windows. The stored name now takes its extension from the validated type rather than from path.extname(file.originalname), so the name on disk cannot disagree with what the file is. The random UUID is unchanged — that was already right, and its comment explains why. The picker offers exactly those three types rather than image/*, so a choice the API will refuse is not on the menu in the first place. That is a convenience, not a control: the operating system's All files option remains, drag-and-drop ignores accept, and anything calling the API directly never sees it. The server is the control. Nine integration tests, and they are the first in this project to upload real file content — which is why none of this was noticed. They cover a genuine PNG accepted, a PDF refused, SVG refused, HTML wearing image/jpeg refused, nothing left on the volume after a refusal, a mixed request discarding its valid file too, and no item created when the upload fails. Plus 21 unit tests on the pure signature checks, including a RIFF container that is not WebP. Verified: 162 unit, 178 integration, 94 end-to-end on a fresh container. Backend lint holds at 4 warnings — it caught the now-unused path import, which is exactly what it is for. Refs #95 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
9c9e9c3ded |
feat(backend): check the environment at boot instead of discovering it later (#64)
The backend reads environment variables in a couple of dozen places and validated none of them. A missing or misspelled one was undefined until the first line of code that happened to need it, which could be a long time after the container reported healthy — and several of those failures are silent and customer-visible. DEMO_MODE is the one that mattered most. It was read as "demo unless the value is exactly the string false", so DEMO_MODE=False, DEMO_MODE=0, or any typo meant demo mode stayed on and the shop quietly stopped charging anyone. It is now required and strict: exactly 'true' or 'false', and anything else refuses to start while quoting the value it was given, so the typo is visible in the message rather than inferred. Two requirements are conditional, and that is what makes them expressible at all. PayPal credentials are demanded only when DEMO_MODE=false, because QA runs with none of them on purpose and an unconditional rule would be simply wrong there. PUBLIC_URL is demanded only when SMTP is configured, because its only job is building links in email — an environment that cannot send mail does not need it, and requiring it everywhere would break every existing local setup to prevent nothing. UPLOADS_DIR gets no such reprieve: its fallback is correct inside the container and wrong everywhere else, so inheriting it writes uploads somewhere nobody is looking. Every problem is reported at once rather than one per restart, and the process then exits — the same shape as the container refusing to start on a failed migration rather than serving against a schema it does not match. Warnings are printed but do not stop anything: SMTP absent, the admin gate inactive, or an allowlist missing while mail can be sent. That last one is new and earns its place, since SMTP with no allowlist means the environment can reach real customers, which is what #87 exists to prevent. The admin-gate warning moved here from server.ts, so one place says what this container is and is not configured to do. validateEnv is a pure function of the environment handed to it rather than a reader of process.env, so it is tested exhaustively without booting anything or mutating global state. It is called from server.ts and deliberately not from app.ts: the integration suite imports app directly and would otherwise become a configuration exercise. Its rules are one small function each at module level, because cognitive complexity counts everything declared inside a function and the first version scored 24 against a limit of 15. Verified as a real process, not only in tests. A missing DEMO_MODE, a DEMO_MODE of 'False', real payments with no PayPal credentials, and half-configured SMTP each exit 1 with the problems listed; a valid environment starts and serves. Note the exit codes were checked without a pipe, because $? after `| head` reports head rather than node and had first suggested a clean exit. 141 unit, 169 integration and 94 end-to-end passing, lint unchanged at 0 errors and 8 warnings. Both CI workflows already set all six always-required variables plus DEMO_MODE, so the pipeline is unaffected. Refs #64 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
89fc7c5c1b |
feat(backend): add an application-layer gate to the admin API (#63)
Authorization for the admin panel and the admin API has lived entirely in one auth_request regex in an Nginx Proxy Manager config outside this repository. That control is real and it works — nothing is publicly exposed today — but it is invisible from the code, untested here, and not reviewed when this code changes. Three things follow from that, and the first is the one worth the change. An admin route added at a path the regex does not match is unprotected the moment it is written, and nothing in Express indicates that. Anything reaching the published container port directly bypasses authentik entirely. And locally there is no gate at all, so no developer ever sees the boundary being enforced. requireAdminGate is attached to each admin router rather than to a path prefix, which is what makes it useful rather than merely redundant with the proxy. An admin router added later at some other path inherits the gate; because the proxy only injects the header on paths its regex matches, that router refuses on its first request instead of being quietly public. A 403 in that situation is the boundary reporting that it has drifted. The gate is optional, and unset means exactly today's behaviour. That keeps local development and all 113 existing admin test call sites working untouched, and means shipping the image before configuring the proxy cannot take the admin panel down. What it does not do is stay silent about it: the server warns at boot when the gate is inactive, naming what is unprotected. This project has been bitten repeatedly by controls that report success while doing nothing, and an unconfigured gate should be a visible choice rather than an invisible one. An empty value is treated as unset rather than as a secret, because enforcing an empty secret would admit any caller sending an empty header. Comparison is timing-safe over SHA-256 digests of both sides: timingSafeEqual throws on buffers of unequal length, so comparing raw values would turn a short header into a 500 rather than a 403, and a length check first would leak the secret's length. Turning it on requires the secret in two places at once — the stack environment and a proxy_set_header line on the gated location in NPM. Setting only one gives 403s until the other catches up. That coupling, and the three consequences above, are now written into the README beside the deployment section, since none of it is visible from the code. Verified over real HTTP as well as in tests. Booting without the secret logs the warning and serves admin normally; booting with it returns 403 for a missing header, 403 for a wrong one, 200 for the right one, and leaves the public storefront at 200 throughout, with each refusal logged distinguishably and without echoing the value it was sent. 8 new unit tests, 9 new integration tests covering every admin router separately — a correct middleware nobody mounted would pass the unit tests and protect nothing. 106 unit and 153 integration passing, lint 0 errors and 8 warnings unchanged. Refs #63 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ecc2219fa5 |
feat: stage new items as pending until an admin publishes them (#90)
An item used to be live on the storefront the instant it was created. Now it starts pending, and a customer sees it only once it is published. The migration changes the column default and nothing else. Backfilling would un-publish the entire live catalogue, which is the one thing it must not do. Hiding a pending item took four separate changes, not one, and that is the part worth knowing. The storefront's item routes had no status filter at all — sold items are listed and rendered with a Sold badge deliberately — so pending could not be expressed as one more optional filter. GET /api/items now carries an exclusion the caller cannot opt out of; GET /api/items/:id carries the same, because hiding an item from the list while still serving it by id would leave it reachable to anyone who kept a link; and GET /api/filters excludes pending from both aggregates it computes. That last one is the least obvious: a pending item would have inflated its tags' counts, so a customer would read "Rare (1)", filter by it, and be told nothing matches — and its price would have stretched the slider to a range no visible item occupies. The tag count is computed over the joined items rather than filtered with a WHERE. A WHERE would have dropped the row for a tag whose only item is pending, and the tag would have vanished from the drawer instead of showing zero. There is a test for exactly that, because the first version of this query had that bug. parseItemFilters is shared by the storefront and admin routes, so 'pending' parses on both. The public route refuses it explicitly rather than answering with an empty list, which would read as "no items match" instead of "you may not ask that". The storefront's URL reader is deliberately left not accepting it either, with a comment saying so, since a request guaranteed to fail is not worth constructing. Publishing is the existing mark-available: same transition, same UPDATE, so the admin UI labels that button "Publish" when the item is pending rather than adding a second endpoint that does the same thing. Unpublish is new and is not symmetrical — it is refused for a reserved item, which someone is holding in their cart right now, and for a sold one, which is a record of something that happened rather than a draft. Both refusals name their reason, and the buttons are hidden in those states so the refusal is not how you find out. Changing a column default has reach, and it surfaced eight test fixtures that silently depended on it. Each is now explicit about the status it wants rather than inheriting one — better practice regardless, and immune to the next default change. Two tests also used 'pending' as their example of an *unknown* status; both would have quietly become tautologies, so they now use one that is genuinely unknown. Verified: 98 unit, 160 integration and 94 end-to-end passing, the last on a freshly created container. One earlier run showed a single failure in favorites.spec.ts; it passes in isolation and on a clean container, and is the cross-spec interference already recorded against the suite rather than anything from this change. Refs #90 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
0c90e18205 |
feat: let QA send real email, guarded by a recipient allowlist (#87)
QA has never been able to send mail. The compose file set no SMTP variables and the mailer skips sending when it finds none, which was deliberate — a QA run must not be able to email a real customer if a fixture ever holds a real address. The cost is that four customer-facing flows have never been exercised anywhere but production: verification, password reset, favorite-sold alerts, and the cart-reminder cron that already has a known silent failure mode. MAIL_ALLOWLIST replaces the blanket mute. Unset means unrestricted, which is production and must stay so. Set means only matching recipients are delivered to; anything else is skipped with a [mail-blocked] warning naming the address and subject. An entry is either a full address, which also covers its plus-suffixed variants, or @domain for every mailbox there — plus-addressing is how these tests get written, and nobody should have to edit an allowlist to invent a new suffix mid-run. The guard sits in the mailer, not at the four call sites, so every sender is covered by construction and a fifth added later cannot bypass it by forgetting. It skips rather than throws: three callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 on the signup path. The flow under test finishes and the log says why no mail arrived, which is exactly what was missing when QA was simply muted. Two details are load-bearing enough to state. Comparison is exact equality on both halves of the address rather than a suffix test, so a lookalike domain ending in an allowed one cannot get through — there is a test for that specifically. And a present-but-empty value refuses everyone rather than allowing everyone: writing MAIL_ALLOWLIST= expresses an intent to restrict, and reading it as "no restriction" would turn a typo into an outbound mail incident. This inverts the failure mode, so the allowlist is hardcoded in docker-compose.qa.yml rather than read from a stack variable. The safety property must not depend on remembering to set something in Portainer, where an omission would mean unrestricted sending from an environment full of fixtures. The comment says removing the line disables the restriction rather than the mail. QA points at Brevo, reusing the existing account rather than a separate QA sender — a deliberate choice that puts QA volume behind production's sending reputation and quota, acceptable for now. Host, port and secure are pinned in the compose because the mailer's fallbacks are Gmail's and Brevo needs 587 with STARTTLS; that mismatch fails at send time rather than at boot, which is #64's territory. Verified: 12 new unit tests on the matching function, which is where a mistake would actually be dangerous — 98 unit and 144 integration passing, lint 0 errors and 8 warnings unchanged, and the compose renders the expected values under docker compose config. Refs #87 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
2f6e855596 |
fix(backend): stop IPv6 callers bypassing the password-reset rate limit (#84)
The QA stack has been logging ERR_ERL_KEY_GEN_IPV6 at every boot, and express-rate-limit was right to complain. keyByCallerAndEmail built its key from req.ip raw. For an IPv4 caller that is one address and the limiter worked as intended. For an IPv6 caller it is the full 128 bits — and a residential IPv6 customer is delegated an entire prefix and can source every request from a different address inside it at no cost. Keyed that way, each request counted as a new caller and the allowance of five per fifteen minutes never bound at all. That matters more here than it would elsewhere, because of what this limiter is for. Its own comment says it: without one, anyone can make the server send unlimited mail to any address they choose. For IPv6 clients there effectively was no limiter, while the code read as though there were. The caller half of the key now goes through express-rate-limit's ipKeyGenerator, which groups IPv6 by prefix and returns IPv4 unchanged. The helper's default is /56 rather than /64, and that default is kept deliberately: /56 covers a whole delegated site, so an attacker cannot escape their bucket by moving within their own allocation. It does mean several households behind one delegation share an allowance — acceptable only because the key also carries the email address, so they collide just when targeting the same account. The reasoning sits next to the code, because a future reader tightening it to /64 would silently reopen the hole. keyByCallerAndEmail is now exported so it can be tested directly. The limiter's allowance is still not asserted anywhere, and should not be: its store is process-wide, so a test that exhausts it leaks into every later test from the same address and fails something unrelated later. The key function is pure, and it is where the bug was. Verified by firing the guard rather than reasoning about it: building main and loading the module reproduces the ValidationError, and the same load with this change is silent. Seven new unit tests cover an IPv4 caller unchanged, two addresses in one delegation collapsing to a single key, separate delegations staying apart, an IPv4-mapped address keying the same as the plain IPv4 one, email normalisation, a non-string email, and a request with no address at all. 86 unit tests pass, 144 integration, lint 0 errors and 8 warnings — unchanged. Closes #84 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
71cbd142c3 |
fix: address the final review of the error boundaries (#62)
Five findings from a whole-branch review, two of which mattered. The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry. The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied. Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful. Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7d227507b0 |
fix(backend): stop a client error report forging log lines (#62)
Review of the endpoint found that truncating the report's fields is not enough. It is unauthenticated and reachable without the frontend, so a caller could embed a newline in any field and forge what reads as a second [client-error] record in the shared server log. Every field is now stripped of CR, LF and the other C0 control characters, plus DEL, each replaced by a single space, so one report is always exactly one log record. Sanitising happens before truncation rather than after. The substitution is 1-for-1, so it cannot change the string's length and clipping the sanitised value still guarantees the stored result never exceeds the limit. An escaping scheme that expanded a control character into several visible ones would need the opposite order to keep that guarantee, so the two are not interchangeable — recorded in a comment next to the code rather than left for someone to rediscover by reversing it. The check is a numeric code-point comparison rather than a regex over a control-character class. That is not style: the first attempt used one, and the hex escapes were corrupted into raw control bytes on the way into the file. Written this way the source never has to contain an escape sequence or a raw control character at all, and the file is verified free of both. The review also found the truncation boundary was never exercised — the only test sent 5000 characters against a 500 limit. Tests now cover a string of exactly the limit passing through untouched, one character over truncating, truncation of stack and componentStack rather than message alone, and a report full of newlines producing a single log line. Verified: 10 integration tests pass, up from 4, and lint reports no new warnings. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c693672051 |
feat(backend): log client-side render errors to the server (#62)
The frontend's error boundaries need somewhere to report to. A boundary that only shows a customer a message leaves nobody knowing it happened, which is the failure shape this project has designed against three times already. POST /api/client-errors takes a report, truncates its fields, logs it with a [client-error] prefix and returns 204. No storage: the container log is where this project's operational visibility already lives, and a table with a retention policy and an admin screen is a subsystem larger than the issue. An unrecognised context is a 400 rather than a log line under a guessed label, following parseItemFilters, which refuses a malformed filter instead of coercing it. Oversized fields go the other way and are truncated rather than refused, because an over-long report is still the only record of the failure. The endpoint gets its own rate limiter rather than reusing passwordResetRequestLimiter, whose comment already warns that its caller-and-email key collapses every caller into one shared bucket on an endpoint without an email. The new one takes the default key generator, which also avoids the ERR_ERL_KEY_GEN_IPV6 warning the custom key produces. Verified: 138 integration tests pass, 4 of them new, and 79 unit. The unit count rose by one without a test being written — routesAreWrapped.test.ts runs describe.each over the files in src/routes, so a new route file generates a case. The handler is synchronous and needs no asyncRoute wrapper. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1d7aba2d60 |
fix(backend): route every async handler through the error middleware (#59)
Express 4 does not forward a rejected promise from an async handler, so an unwrapped async route never responds at all — the request hangs until the client gives up, nothing reaches the error middleware, and monitoring sees an open connection rather than a 500. That silence is the shape of the 2026-08-17 incident, where an unhandled rejection left every item query hanging and the storefront rendered it as an empty shop. `asyncRoute` was written in response, but it was only applied to some routes: 30 handlers added afterwards were still bare, including register, login, the whole cart, and PayPal checkout. Wraps all 30, plus two the issue's inventory missed. `attachCustomer` is a bare async middleware mounted globally in app.ts, so a rejection in its session lookup would hang every request in the application — including the 25 handlers that were already wrapped correctly, which meant the guarantee did not actually hold anywhere. The PayPal webhook registers on a second router named `webhookRouter`, so an audit grepping for `router.` walked straight past it. Adds a unit test that scans the route sources and fails on any registration whose handler is not wrapped. A convention already half-forgotten once will be forgotten again, and enforcement is what the issue asked for; ESLint would be the better home for it but there is no ESLint in this repo yet (#60). The test walks parens rather than lines, so it also catches a handler whose `async` sits on its own line, and it matches any `*Router` name rather than just `router` — the two ways the existing bare handlers escaped notice. It is deleted along with `asyncRoute` if the project moves to Express 5, which forwards rejections natively. No behaviour changes on the success path; the failure path turns a hung request into a logged 500. Closes #59 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
d4e2abe743 |
feat: filter the storefront by favorited items (#35)
Adds favorites as another dimension of the existing storefront filter rather than a separate view, so it lives in the URL, shows up as a removable chip, and combines with category, tags, and price by AND like everything else. A customer can ask for "my favorites under $500 in Furniture" instead of only "my favorites". Sold favorites are included. The storefront shows sold items everywhere else, and a favorite that has just sold is often exactly what the customer came back to look at after being emailed about it in #34. Hiding them would make items disappear from a list the customer curated themselves. Anyone wanting only what they can still buy can combine the toggle with the status filter. Which customer "my favorites" means comes from the session, never from the query string, so a hand-edited URL cannot name someone else's favorites. A signed-out visitor sees the toggle and gets the same inline register/login prompt the heart button and Add to Cart already use; signing in resolves the gate and the filter applies on its own. A bookmarked favorites link whose session has expired says so rather than rendering an empty grid, which would tell the visitor they have no favorites instead of that we do not know who they are. The API answers 401 for the same reason, and the admin inventory refuses the filter outright rather than ignoring it. The shared SQL builder now requires callers to say whose favorites they mean, even when that is nobody, and throws instead of dropping the clause — a future caller that forgets the guard fails loudly rather than quietly returning the whole catalogue. Verified with 59 unit tests, 134 backend integration tests, and 70 end-to-end tests, all passing, with type checking clean on both sides. |
||
|
bermudalambandClaude Opus 5
|
c4fb47853f |
feat: tell favoriters when an item is withdrawn (#34)
Deleting an item cascades its favorites away, so anyone watching it lost the record silently and heard nothing. Recipients are now collected before the delete, since after it there is nobody left to look up, and the mail is sent only once the delete has succeeded so nobody hears about a withdrawal that did not happen. Items that had already sold are excluded. Their favoriters were told at the point of sale, and a second "no longer available" for the same item reads as a duplicate rather than news. The wording differs from the sale notification — withdrawn rather than sold — because the customer did not lose out to another buyer and saying so would be untrue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
f626f27e75 |
feat: favorite items and notify when a favorite is sold (#34)
Customers can favorite and unfavorite items from the storefront, opt in to being told when a favorite is sold to someone else, and manage that preference from their account page. The opt-in is a consent of its own rather than the existing marketing flag. Being told that a specific item you asked about has gone is a narrower thing than agreeing to marketing, and folding one into the other would leave marketing_consent_text no longer describing what was actually agreed to. It is recorded the same way as the marketing consent — flag, timestamp, and the exact wording shown — and accepting it does not set marketing_consent. The prompt appears only after a customer has actually favorited something, so the reason for asking is concrete rather than an abstract marketing ask, and it says plainly that it is separate from marketing email. Declining keeps the favorite. Notifications fire when an item reaches sold, either through checkout or an admin marking it sold, and never to the buyer — telling someone the item they just bought is unavailable reads as a bug. Reserved is deliberately not a trigger: reservations expire and get released, so a "gone" email would often be about an item still for sale. Disabled accounts are excluded, per #33. completeCheckout now returns the sold item ids and the buyer so its three call sites can notify after COMMIT. Sending inside the transaction would email people about a sale that then rolled back, and would hold the transaction open for SMTP. Each message is sent independently so one bad address cannot stop the rest, and the sale has already succeeded regardless. Favoriting while signed out opens the existing inline register/login modal, exactly as Add to Cart does, and completes the favorite on success. Also fixes a latent bug in the same component: while the session was still resolving, `customer` is null for a signed-in visitor too, so clicking Add to Cart or the new heart in that window prompted them to sign in again. Both now ignore clicks until the session has resolved, and the control shows as loading meanwhile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
13c010ff51 |
feat(admin): disable and re-enable customer accounts (#33)
Adds customers.disabled_at, admin disable/enable endpoints, a Status column and toggle on the Customers tab, and enforcement across every path that authenticates. Enforcement lives in attachCustomer, which previously validated only the session token and its expiry and never read the customer row. Register, login and password reset all mint sessions, so a single check in the middleware covers every path rather than three separate ones — and it means an existing rd_session cookie stops working at once instead of at its 30-day expiry. Disabling also deletes the sessions outright, so eviction does not wait for the next request. Disabling releases the items the customer was holding, in the same transaction. A disabled account cannot check out, so leaving its reservations would keep one-of-a-kind stock off the storefront for up to the cart expiry window for no purpose. Guarded on 'reserved' so a sold item is never resurrected. Re-enabling restores sign-in but does not give the items back — they may since have sold. Sign-in returns an explicit 403 rather than a generic credential failure. That does confirm the address has an account, which sits awkwardly beside the deliberately non-enumerating reset in #32; the trade was made the other way because a disabled customer told "invalid email or password" resets their password, succeeds, is still locked out, and concludes the site is broken. The check runs only after the password verifies, so it is not a bulk membership oracle, and /register already reveals existence. A reset token issued before the disable no longer mints a session, and no new tokens are issued for a disabled account — while still answering 200, so that endpoint stays non-enumerating. Self-service GDPR export and deletion are blocked along with everything else, so those requests now need servicing by hand. Worth checking the privacy policy does not promise unconditional self-service. Also fixes an unrelated bug the e2e run surfaced: the admin inventory fired a request per keystroke in the price fields with no sequencing, so an older response could land after a newer one and repaint stale rows. Only the most recently issued request may now set state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
db7c61c89d |
feat: customer password reset via email round-trip (#32)
Adds "Forgot password?" to the login page, a request page, and a reset page reached by a one-hour, single-use token delivered by email. Reuses customer_tokens with a new password_reset kind alongside verify_email. The request endpoint always answers 200, whether or not the address has an account, so it cannot be used to test addresses for membership. Note /register still reveals existence through its 409 on a duplicate, so this protection is currently partial; closing that is its own change. Completing a reset deletes every session for that customer. A reset prompted by a compromise has to evict the intruder, and leaving a 30-day cookie alive would defeat the point. It also marks the address verified, since receiving the mail is exactly what verification proves, and supersedes any outstanding token so an older link in the inbox cannot be resurrected. Introduces the first rate limiting in the codebase, on the request endpoint only. The limiter is keyed on caller *and* submitted address: keying on IP alone would let one person lock out everyone behind the same proxy, and everything arrives via Nginx Proxy Manager. Applying that same limiter to the reset endpoint, which carries no address, collapsed every caller into one shared bucket -- so that endpoint is deliberately unlimited instead, protected by a 32-byte single-use token whose bcrypt work only runs after the token matches. The e2e tests read the issued token directly from Postgres rather than through a test-support endpoint. An endpoint returning a reset token for an arbitrary address is account takeover for every customer if it is ever reachable, and an environment gate is thin protection against that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
f537314259 |
fix(admin): theme, American English, and inventory/reservation tooling (#27)
Seven reported items, of which the first four had two root causes. The active tab was invisible in dark mode because colorPrimary was hardcoded to #1a1a1a in both themes. The accent now inverts with the theme, and colorTextLightSolid inverts with it, or a near-white accent would get antd's default white label and disappear. The Category tab, Tag tab, and item-form category selector ignored the theme entirely. antd declares main: lib/index.js and module: es/index.js, so importing from 'antd' resolves to the ES build while 'antd/lib/...' loads the CommonJS one — two copies, two React contexts, and no ConfigProvider for anything deep-imported. Switching those files to antd/es/* keeps the deep-import convention and shares the instance. This was introduced by my own use of the lib path; es is correct under Vite. Two storefront components had the same latent bug. "Colour" is now "Color". The Customers tab shows how many items each customer is holding, as a link opening the item list with a Release button. Release mirrors the customer's own cart removal — drop the cart row, return the item to available, guarded on 'reserved' so it can never resurrect a sold item — and deliberately sends no email about an action the customer did not take. The count is a subquery rather than another join, which would have multiplied rows and inflated order_count and total_spent_cents. The Inventory tab filters by category, tags, price, and status, reusing the storefront's parser and query builder so the two cannot drift. Reserved is one option in a Status filter rather than a standalone toggle. Also fixes two defects the screenshots exposed: the reserved-count link bubbled to the row handler and opened the customer drawer behind the dialog, and .admin-category-node had no CSS at all, so the tree node name, item count, and actions ran together as one string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c77fdad2b9 |
fix: run migrations on boot and stop failures rendering as empty (#23)
The storefront showed no inventory after deploying the categories/tags release. No data was lost: the code queried categories/item_tags/ items.category_id against a database where the migration had not been run, and that failure was invisible at every layer. Three changes, each addressing one layer: Migrations now run at container start, so deployed code cannot be ahead of the schema and the easily-forgotten manual `docker exec migrate.js up` step disappears. migrate.js waits for Postgres to accept connections first, since the NAS brings the DB container up slower than the app, and still exits non-zero so a bad migration stops the container rather than serving a half-migrated schema. Express 4 does not forward a rejected async handler, and no error middleware was mounted, so a failing query never responded at all. Async routes are now wrapped and an error middleware guarantees a 500. A hung request is indistinguishable from an empty result in the UI, which is how a schema mismatch came to read as "the store has no items". The storefront now separates "request failed" from "no items" and offers a retry. fetchItems/fetchFilterOptions throw on a non-OK response rather than returning the parsed error body, which would have been set as the item list and crashed the grid on .map. Also restores the project-context update from 7fb5764, which was left out of PR #24 and ended up dangling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
9222e97deb |
feat(api): categories, tags, and storefront item filters (#23)
Adds a self-referencing categories tree, a tag registry with deterministic colours, and item_tags, plus admin CRUD for both. GET /api/items now accepts category, tags, min_price and max_price. Category matching walks the subtree with a recursive CTE so selecting a parent includes everything filed beneath it; tags match with AND via a count check, since ANY() alone would return items carrying only one of them. Malformed filter params return 400 rather than being ignored, so a broken link doesn't quietly list the whole catalogue. GET /api/filters serves the drawer its tree, tags, and price bounds in one request. Item image/tag aggregation moves from LEFT JOIN + GROUP BY to scalar subqueries. Joining two one-to-many relations multiplies their rows, so an item with 2 images and 3 tags would have repeated every image three times once tags were added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
1c46632e8f | fix: remove stale single-item demo-checkout tests, add cart integration tests | ||
|
bermudalamb
|
7f4479605a | chore: replace manual SQL migrations with node-pg-migrate | ||
|
bermudalamb
|
81118245ce | fix: sonarqube denial of service issue | ||
|
bermudalamb
|
921022c658 |
Add backend unit/integration tests, Playwright e2e tests, README, cookie Secure fix
SonarQube Analysis / sonarqube (push) Successful in 4m20s
|