e933cd19f94b006373ea9585eb69c00063045574
29
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
c704c07b89 |
docs(security): put the SQL injection invariant where it is enforced (#202)
#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) <noreply@anthropic.com> |
||
|
bermudalamb
|
6104ebb459 |
feat(ops): read DEMO_MODE from the stack rather than the compose file (#190)
Reverses the position the previous commit took. Hardcoding it made a value that gets flipped without a code change require a commit and a merge to flip, which is backwards — and it is the operator's call, not the file's.
No default, deliberately. `${DEMO_MODE:-false}` is the obvious form and the wrong one: a default decides whether the shop takes money on the operator's behalf, silently, whichever way it points. Having none is safe rather than fragile because `checkDemoMode` is strict — an unset stack variable substitutes to an empty string, and anything that is not exactly `true` or `false` refuses to boot naming DEMO_MODE. That strictness is the whole reason interpolating this one is defensible, so the line says so.
The compose guard had to learn the difference. It hands each deploying file's entries to the real validator, and a literal `${DEMO_MODE}` is not a value `checkDemoMode` accepts, so both the DEMO_MODE assertion and the validateEnv check failed the moment the file stopped holding a literal. Each deployment now declares the stack variables it supplies, and a bare `${VAR}` named there resolves to the declared value before the file is validated. Every other bare `${VAR}` stays opaque exactly as before — those are secrets, and what is checked of them is that the line exists.
Be clear about what that guard can prove. It cannot see Portainer, so it does not verify the stack actually holds `true`; nothing in this repository can. What it does is keep the intent beside the file and make the pair inseparable — hardcode the compose line and the registry disagrees, change the registry and it no longer describes the file. The runtime half is the boot check, which fails loudly rather than falling back. Verified by mutation: hardcoding `false` fails the DEMO_MODE assertion, and deleting the line fails that and `validateEnv`.
Restoring real payments is now two Portainer values and a redeploy, with no commit — which is what #190 asks for.
|
||
|
bermudalamb
|
12b2f09d79 |
docs(ops): point the demo-mode notes at the right issue (#190)
The tracking issue was filed as #190; the compose banner and the guard test both said #191, guessing the number before it existed. A note that points at the wrong issue is worse than no note when the thing it tracks is production not taking money. |
||
|
bermudalamb
|
0a830cad1f |
feat(ops): put production in demo mode to complete the cutover (#191)
Production could not boot during the cutover to the committed compose file: `DEMO_MODE` is false there, which makes the three PayPal secrets required, and they were not available. Demo mode is the interim the compose file's own header sanctions for exactly this — the whole cart and checkout flow works and nobody is ever charged.
Two things made this cost more than it should have, and both are now written down rather than left to be rediscovered.
`DEMO_MODE` is hardcoded rather than interpolated, so setting a `DEMO_MODE` stack variable in Portainer does nothing at all — there is no `${...}` for it to substitute into and the file's value wins silently. That hardcoding is right: the one value deciding whether the shop takes money should not be flippable from a web UI without a commit anybody can read. But the failure mode reads as "I set it and it ignored me", so the line now says so.
Declaring `PAYPAL_CLIENT_ID=` with an empty value is identical to not declaring it. `isPresent` rejects a blank string deliberately, because set-to-nothing is a mistake rather than a value.
The state is loud in both places that can see it. The compose file leads with a banner saying production is taking no money, and `composeEnvironment.test.ts` asserts `DEMO_MODE` is `true` — that assertion is the guard, not a formality: it fails the moment the file and the expectation disagree, in either direction, so this cannot be switched back quietly and cannot be left on unnoticed.
#191 restores it.
|
||
|
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
|
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>
|
||
|
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
|
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
|
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
|
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
|