4ed9513ad23f91e175e6ae6165ef7773bfdfa77f
32
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  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
|
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> |
||
|
bermudalambandClaude Opus 5
|
a700597440 |
refactor: standardise antd imports and remove the avoidable any (#65)
Stage 1 of #65: this issue's original two lists. The type-checked gate it also owns follows in later stages. Nine files imported antd from the barrel while the rest of the codebase used deep imports from antd/es. Both resolve to the same modules under antd v5 and Vite, so this is not the tree-shaking problem it would have been under v4 — the cost was that a documented convention had two spellings, and nobody reading a file could tell whether its style was deliberate or just old. Eighty-two imports converted, and every antd/es path was checked to exist before generating any of them rather than trusting a name-mangling rule. The three `client: any` parameters in cartCheckout are now PoolClient. These functions run inside a transaction, and `any` removed exactly the check that would catch a pool-versus-client mix-up — which in this codebase means a query silently running outside the transaction it was meant to be part of, on the path that takes money. publicCustomer took `any` and now takes a CustomerRow describing what it actually reads. Typed as its own shape rather than the whole table so that adding a column later — a password hash, a token, an internal note — cannot quietly start being echoed back to a customer. The three `(window as any).paypal` casts are replaced by a declared interface for the injected SDK. It is deliberately narrow: it describes the three things this app calls, not the whole SDK, because a wider guess would be fiction and a wrong shape typed confidently is worse than an honest cast. The property is optional, since the SDK is absent until its script has loaded — which is the check both call sites already make. Verified: 141 unit, 169 integration and 94 end-to-end passing. The end-to-end run is the one that matters here — an antd import migration can build cleanly and still break at runtime through styles or context, so a green tsc proves less than it appears to. Lint drops from 8 warnings to 4 in the backend and 30 to 27 in the frontend, all of them the no-explicit-any this change removed. As a side effect the no-unsafe count that later stages exist to clear falls from 259 to 216 in the backend and 73 to 67 in the frontend, measured rather than estimated. Refs #65 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
|
5f9172512c |
refactor: clear the 85 minutes of technical debt (#81)
Thirteen of SonarQube's fourteen code smells, with the fourteenth argued as a false positive rather than coded around. Four of these were not stylistic. The Remove button in Cart's List.Item actions array carried no key, so React could not match it across renders and rebuilt it on every cart render. The cart, customer-auth and favorites providers each passed a freshly allocated object as their context value, which re-renders every consumer whenever the provider renders, cart or session unchanged — and these three wrap the entire storefront, so the fan-out was the whole tree. Two of them also rebuilt a Set on every render for good measure. All four now memoized on the values they actually derive from. The four cognitive-complexity findings wanted real restructuring rather than suppression. parseItemFilters splits into one helper per field, leaving the function with the order those helpers run in and the single rule that spans two fields; the order is preserved exactly, because a query wrong in two ways reports the first field and rearranging the calls would change which error a caller sees. adminCategories' PUT extracts the parent resolution — the existence check and the cycle check — into a resolver that returns the refusal rather than sending it. App's four-way render chain becomes a Catalogue component, which also removes two of the nested ternaries. Customers' confirm dialog moves to module level, since a function's cognitive complexity counts everything declared inside it and that dialog branches on `disabling` five times. The rest were mechanical: two more nested ternaries — a status-colour lookup and a pluralisation helper — and one type assertion that asserted the type the expression already had. Left alone: S6478 on CategoryTreeSelect's popupRender. That is antd's render prop, called as a function and spliced in, never mounted as a component type, so the destroy-the-subtree failure the rule describes cannot happen. Marked false positive in SonarQube with that reasoning rather than contorting the component around a rule that misread it. Verified rather than assumed. Backend unit 78 pass, integration 134 pass, end-to-end 83 pass, both workspaces build clean. ESLint warnings drop from 35 to 31 with no new file warning — the same React and SonarJS rules #60 turned on are what surfaced this backlog in the first place. Refs #81 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
c058b3ed2e |
feat(ci): add ESLint to both workspaces with a defect-only CI gate (#60)
TypeScript's strict mode checks types and nothing else, so nothing enforced the React hook rules, the SonarJS rules, or unhandled-promise detection. Adds a flat config per workspace, a lint script in each, and a lint job in tests.yml. The rule selection is the substance of this change and is measured rather than guessed. A full-strength config reports 435 violations across 50 files, but 325 of those are the no-unsafe-* family from recommendedTypeChecked, every one downstream of pool.query() returning any rows and untyped fetch responses. Typing those boundaries is the whole of #65, so enabling the rules here would ship a linter whose output is three-quarters another issue's backlog — the reliable way to teach everyone to ignore lint output. This enables recommended plus the two type-aware rules that catch defects rather than describe type debt, which leaves 110 findings. Both configs downgrade every preset to a warning and then list the error rules explicitly at the bottom, so the CI gate is readable in one place instead of inferred from four presets' defaults. Errors are no-floating-promises, no-misused-promises, rules-of-hooks, exhaustive-deps and jsx-a11y/alt-text; everything else warns. No --max-warnings flag is needed because ESLint already exits non-zero on errors and zero on warnings. no-misused-promises runs with checksVoidReturn.attributes false, since onClick={async () => ...} is idiomatic React and safe when the handler catches its own errors — at the default it flags every antd button in the admin screens, 25 of its 28 hits, and a rule that is 89% noise gets switched off within a week. The 37 errors this surfaced were mostly not the mechanical fix they looked like. The plan assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, which was true of the one sampled when writing the design and false for most of the rest: Admin, Categories, Customers, Tags, Settings, Account and CustomerAuthContext all had no rejection handling at all, so `void` on them would have hidden real failures rather than annotated deliberate ones. Each of those loaders now catches and surfaces the failure before the call site voids it. The CustomerAuthContext one was a live bug — a rejected fetchMe left loading true forever, rendering as a permanent spinner instead of a signed-out page. Admin's load became a useCallback so its effect can name it honestly rather than suppress the dependency, Categories' drop handler was split so the function antd receives returns void as its type says, and Cart's effect now names refreshCartContext, which is a useCallback with an empty dependency list and so cannot re-run it. The only disable added is in asyncRoute, where returning a promise where Express expects void is the entire point of the wrapper and the promise cannot reject. Two of the issue's premises did not survive measurement, both recorded in the spec: exhaustive-deps flags 2 cases rather than the 10 inferred from empty dependency arrays, and the backend was already clean on the defect rules because #59 wrapped every async route. Verified: lint, build, 78 unit, 134 integration and 83 e2e all pass in both workspaces, and the CI gate was confirmed to fail by introducing a deliberate violation rather than by assuming the job is wired correctly. Closes #60 |
||
|
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> |
||
|
bermudalambandClaude Opus 5
|
977489cbc0 |
refactor: delete superseded checkout routes and dedupe cart checkout
routes/paypal.ts and routes/demo.ts were the pre-cart single-item checkout flow. Nothing has imported them since the cart flow landed: app.ts mounts only cartCheckout, the frontend calls /api/checkout/cart/*, and no test touches them. They duplicated PAYPAL_BASE, getAccessToken, and a second handler for the /webhooks/paypal mount. Also extract openCheckout() from /paypal/create and /demo/purchase in cartCheckout.ts, which repeated the same address-ownership check, cart lock, and checkouts/checkout_items inserts. It returns a discriminated union so callers keep control of the transaction and the response. Add CartItem/LockedCart interfaces, dropping the (it: any) casts. Note: paypal.ts was the only writer of items.reserved_until and items.paypal_order_id. Those columns are now write-dead; the schema is left alone for a separate migration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ab1024a1b6 |
fix: resolve SonarQube hotspots for upload size and X-Powered-By
- S5693: MAX_IMAGE_BYTES was 8 * 1024 * 1024 (8,388,608), just over the 8,000,000-byte ceiling the rule treats as safe, so the hotspot on the multer storage config never cleared. Use 8_000_000. - S5689: Express advertises its stack in X-Powered-By by default, which tells an attacker what to aim exploits at. Disable the header. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
b73b472d80 | fix: TypeScript type mismatch between UspsValidationResult and local fallback object | ||
|
bermudalamb
|
9ab689e624 | feat: add customer cart with expiry, shipping addresses with USPS validation, and multi-item PayPal checkout | ||
|
bermudalambandClaude Opus 5
|
aba47c81e3 |
fix: bound admin image uploads and use a CSPRNG for stored filenames
SonarQube flagged three hotspots in the admin router: multer was configured with no content length limits, and stored filenames were derived from Date.now() plus Math.random(). - Cap the multipart body on every dimension: 6 files, 8 MiB per image, 8 fields, 64 KiB per field. Without limits a single request could fill the uploads volume. - Generate stored filenames with crypto.randomUUID() so paths are not predictable. Image ordering is unaffected; sort_order already drives it. - Wrap the upload middleware to translate MulterError into 413/400 JSON. The app mounts no error handler, so a limit rejection would otherwise surface as an HTML 500. 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
|
||
|
bermudalamb
|
9be4986dd3 | Initial commit: redefined-designs storefront |