36bbaf99e47ed870d7637831d013a2c3e42f23b8
18
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
f95013850b |
feat(passkeys): authentication ceremony (#39)
A customer signs in with a registered passkey. Usernameless: they are never asked who they are, the browser offers whichever accounts it holds for this Relying Party, and the assertion says which credential answered. #38 requested discoverable credentials so this would work. That choice does more than improve the experience. This issue requires that failures not reveal whether an email has an account or has passkeys registered, and with no email ever sent to the endpoint there is nothing to reveal. The email-first alternative would have had to answer identically for a known and an unknown address, in every branch, forever. Session creation is shared rather than reimplemented, which is the requirement stated most sharply here: a second, subtly different session path is how auth bugs get in. setSessionCookie and createSession move to customerSession.ts and both paths import them. Two implementations that agree today are two that can be changed one at a time, and the one that would be forgotten is whichever is not the password path, because that is the one every manual test exercises. Social sign-in will use the same module when #332 lands. The signature counter policy #37 deferred is decided here, and both halves matter. Requiring an increase from every authenticator refuses synced passkeys, which report zero forever by design and are what most customers actually use. Requiring it from none discards the only signal that a hardware credential has been cloned. So zero against zero is accepted and anything else must strictly increase — and the asymmetry is deliberate, because an authenticator that has ever reported a real counter is held to the strict rule from then on and cannot downgrade itself to zero to escape it. A disabled account is refused, read from the same row as the credential rather than a second query that could disagree. Enforcing that only on the password path would have left passkeys as a way around it. Every refusal answers the same way. No such credential, a disabled account, a bad assertion and a stalled counter are all that did not work to the caller; saying which would turn the endpoint into an oracle for whether a credential exists and whether its account is in good standing. The stalled counter is logged, because the customer cannot act on it and the person who can is reading the logs. The challenge is spent by deleting it, with the expiry in the same statement, so a replay finds nothing to delete and a stale challenge fails the same way. It is passed to the library as a predicate rather than a value, which is what makes a usernameless flow possible at all — the challenge is not known until the assertion names it. That predicate is a named function rather than an inline callback, and it was inline first. routesAreWrapped.test.ts reads the text of each router.post looking for an async that no asyncRoute covers, and an async callback nested inside a wrapped handler looks exactly like an unwrapped one to it. The guard caught it, and hoisting the function out was the better fix: the code reads more clearly and the guard keeps its teeth rather than learning another exception. Verified: tsc clean for src and tests, lint back to the seven pre-existing warnings with none added, 521 unit tests across 36 suites — seven new, covering the counter policy in both directions. Not verified: the ceremony cannot be exercised without a browser and a real authenticator, which per #41 is a standing limitation of this feature rather than a gap here. CI can prove the routes exist, are wrapped, and refuse a caller with no credential. Closes #39 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
955049eac9 |
fix(privacy): separate analytics consent from email consent (#56)
The previous commit widened the marketing consent sentence to cover the Brevo tracker, so one checkbox carried both purposes. That is the specific pattern GDPR rejects: consent has to be granular, and current EDPB guidance treats bundling tracking consent with subscription consent as invalid because the customer cannot accept one purpose and refuse the other. Quebec's Law 25 s.8.1 is stricter again — profiling technology has to be off until the person switches it on, with no pre-ticked box and no consent inherited from agreeing to something else. Building to both standards was the decision, since the storefront is publicly reachable and anyone can register. So the marketing sentence is restored to exactly what it was, which leaves every existing email consent valid and untouched, and analytics gets its own column, its own sentence, its own checkbox at registration, its own toggle in the account page and its own endpoint. A customer can now hold either, both, or neither, and withdrawing one does not disturb the other. The migration defaults analytics_consent to false, which is both the honest answer — none of the existing customers was ever asked — and what Law 25 requires. Nothing about this change opts anybody in. Two details that are compliance requirements rather than wording preferences. The sentence names Brevo instead of saying "our email provider", because informed consent means the customer can tell who receives their data and a description they cannot act on is not disclosure. And the account toggle is as prominent and as easy to switch off as it is to switch on, because withdrawal has to be as easy as consenting. The analytics endpoint is separate from the marketing one rather than a second field on it, so that a single call cannot change an answer the customer did not touch — the bundling problem moved from the form into the API. The unit tests now assert the two consents stay apart in both directions, including that the marketing sentence still says nothing about tracking, because re-bundling them would otherwise pass silently and is the mistake this project already made once. Verified: backend tsc clean, both lint suites 0 errors with no new warnings, 478 unit tests passing across 33 suites, frontend production build green. Not verified: the migration has not been run against a database, and integration and e2e need a Node this machine does not have active. None of this is legal advice and the wording is worth a lawyer's eye before it ships. Refs #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
ac3f6e91f5 |
feat(analytics): report consenting customers' activity to Brevo (#56)
Loads Brevo's web tracker for a signed-in customer who has consented, reports route changes as page views, and tracks the three events the issue asked for: added_to_cart, favorited, and checkout_completed. The four design questions were settled on the issue in August and this implements those answers. The consent gate is the part worth reading. The decision recorded on the issue was "gate it behind consent", but the sentence customers actually agreed to named only email: "I want to receive occasional emails about new one-of-a-kind items". Gating a tracker on `marketing_consent` while that was the stored wording would have treated "email me about new items" as authorisation to send someone's browsing to a third party, which it does not say — and this project stores the wording verbatim against each customer precisely so that a record says what the customer saw. So the sentence is widened here, and the tracker is gated on `analytics_consent`, a field the server computes by comparing the wording stored against a customer with the current constant. Changing the sentence therefore does not retroactively widen anybody's consent: everyone who agreed to the old text keeps their email consent and is not tracked until they re-consent through the account page. A boolean alone could not tell those two populations apart, which is the whole reason the text is stored per customer. `analyticsConsent` is exported and has its own unit test, because "agreeing to the old wording does not authorise tracking" is the rule that silently tracks people if it regresses — their flag really is true. QA stays out of the live Brevo account by construction rather than by remembering. The key is per-environment, the tracker never loads without one, and `docker-compose.qa.yml` sets an empty literal with no stack variable behind it, so nothing can inherit a value from the host or be pasted in from production's stack. Same reasoning as QA_DB_PASSWORD and the QA_SMTP_ names beside it. Events are reported from the API layer rather than the UI call sites, so no caller can add to the cart or favorite an item without it being counted, and each fires only after the response was accepted — a refused add is not reported as one. The two checkout completions each name their processor, because a demo purchase charges nothing and counting it as a sale would overstate revenue. The privacy policy gains an analytics section in this change rather than a follow-up, since the published policy previously described none of this and would otherwise have lagged the code. It is deliberate about the limits: withdrawing consent stops further reporting, but anything already sent stays with Brevo, and a script already injected cannot be un-injected — `stopBrevoTracking` stops calls, it does not unload sa.js. That is said in the code too, because "tracking stops" reads as a stronger promise than any web tracker can make. Verified: backend tsc clean, both lint suites 0 errors with no new warnings, 474 unit tests passing across 33 suites, and the frontend production build green including the compose-environment guard. Not verified: integration and e2e, which need a database and a Node this machine does not have active, and no real Brevo key was exercised — the tracker has never been observed reporting to an actual account. Closes #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
 bermudalamb
|
d7dacffa11 |
test(perf): stop hashing test passwords at production cost (#242)
The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way. The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed. Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict. `hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless. Closes #242 |
||
|
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
|
c5fe84fba5 |
refactor(backend): type the customer query results (#159)
The largest file, 43 query sites, now with none of its reads untyped. Typed sites across the backend go from 22 to 43. CustomerRecord extends the existing CustomerRow rather than restating it, because that is the relationship that actually holds. CustomerRow was already there and is not a table row — it is the subset safe to return to the customer, written that way so adding a column could not silently start being echoed back by a `...c` downstream. The full row read by `SELECT *` is that subset plus seven fields that are deliberately not on it, password_hash among them. Extending keeps the two connected: adding a column to the table means adding it to CustomerRecord and deciding at that moment whether it belongs in CustomerRow, which is exactly the decision the older comment is about. The column list came from the live schema rather than from reading migrations, since the migrations are additive and reconstructing the current shape from six files invites getting a nullability wrong. Typing the data export surfaced something worth a decision, and it is recorded in the code rather than quietly changed. `GET /me/export` runs `SELECT * FROM orders` and sends every column verbatim, including raw_event — the processor's entire capture payload. That is defensible for a GDPR export, since it is the customer's own transaction, but it is a decision rather than an accident, and it is now visible in a type instead of hidden behind `any`. The order-history route two functions above deliberately selects six named columns instead, which is the contrast that makes the export's behaviour worth confirming. No behaviour changed here; #159 is about types. Nullability follows the schema rather than optimism: orders.amount_cents, status, item_id, customer_id and checkout_id are all nullable in Postgres, and customers.first_name and last_name are nullable despite registration requiring them, because customers who registered while the field was optional genuinely have none. Verified: tsc clean, and the full integration suite passes 238/238 across 17 suites. Refs #159 |
||
|
bermudalamb
|
2f7268704a |
feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)
Giving the customer emails a tab of their own (#135) made a family of related holes visible: an admin could edit the wording of every customer email, but not the facts that wording asserted, and two templates could not address the customer at all. Both token lifetimes were hardcoded twice. VERIFY_TOKEN_TTL_MS sat in routes/customers.ts while the verification body separately said "This link expires in 24 hours", and RESET_TOKEN_TTL_MS sat beside a body separately saying "one hour". The prose was not derived from the constant, it was a second hand-written copy of the same fact — so making the constants configurable without addressing that would have made things worse, not better: the setting moves to two hours and the email keeps confidently promising one. Both are now settings, and both templates state their lifetime through an {{expiresIn}} placeholder rendered from the setting. The cart reminder gains {{holdDuration}} for the same reason. Per-item deadlines were already inside {{itemList}}, but there was no way to write a sentence about the hold itself without hardcoding a number the cart expiry setting could change underneath the author. All three durations render through one formatDuration(), so the reset email and the cart reminder say "one hour" the same way rather than in two authors' phrasing. A fractional hour drops to minutes, because "0.5 hours" reads badly and "1.5 hours" reads worse in a sentence a customer is meant to act on. Every template now offers greeting, firstName and lastName. favoriteSold and favoriteWithdrawn previously offered only itemName and siteUrl and could not address anyone — the query behind them never selected a name, so it does now. The greeting itself is two settings, a format and a fallback, rather than the wording baked into greeting(). The fallback is separate rather than the format with the name edited out: that editing is guesswork that has to be right every time, and getting it wrong ships "Hi ," to everyone who registered while first names were still optional (#106). Those customers exist, which is why greeting() guarded the case in the first place. The admin preview renders the durations and the greeting from the live settings rather than from a static sample. The preview exists so an admin sees the email that will be sent, and a sample reading "one hour" while the setting says two is the precise failure these placeholders were added to remove. Settings are read through a new adminSettings accessor. cart_expiry_hours was previously read by an inline query in two places, each with its own `|| '24'`; with five settings and read sites across three routes and the cron job, a default written twice is a default that will eventually disagree with itself. Values are stored as text, so each definition declares how to read it back — numbers were the only kind until the greeting format arrived. The new placeholders are available but never required, so every template an admin has already saved keeps rendering and keeps sending. Also fixes updateAdminSettings announcing success for a save the server refused: it returned the 400 body as though it were the saved settings, so the form reported "Settings saved" either way. Closes #136 |
||
|
bermudalambandClaude Opus 5
|
b8549e9c72 |
feat: let a customer resend their own verification email (#110)
A verification email was sent once, at registration. If it was lost, filtered, or arrived after the 24-hour token had expired, the customer had no route back except registering again with a different address. POST /api/customers/resend-verification mints a fresh token and sends the mail, and the account page grows a "Send it again" button beside the warning that was already there. The button only exists while the address is unverified: on a verified account it would be a control whose only outcome is a refusal, and the endpoint refuses that case rather than sending a pointless email. The mint-token-and-send sequence now lives in one place. Registration and the email change already did the same three steps - supersede any outstanding token, mint a new one, send it - and this would have been a third copy. The step most likely to be dropped in a copy is the supersede, and it is the one that matters: without it an older message still sitting in the inbox goes on verifying. Anything that makes the server send mail on request is an abuse vector, so this is rate limited to three an hour, keyed on the customer id. That is tighter than either existing limiter and sidesteps #84's IPv6 problem entirely, since a signed-in caller has an identity better than an address to count against and cannot escape the bucket by moving within a delegated prefix. The refusal says the mail probably did send and to check the spam folder, which is both more useful and more honest than a bare 429. The claim that keying on customer id also solved test isolation was wrong, and the tests caught it. resetDb truncates with RESTART IDENTITY, so every integration test's first customer is id 1: three tests that each sent once left the fourth starting at its limit, and two tests failed on a 429 they never asked for. A "fresh customer per test" is a fresh row with a recycled identity. The limiter now has an explicit exported store the suite clears between tests, and the comment that claimed otherwise has been corrected rather than left to mislead the next reader. Verification: seven integration tests covering the unauthenticated refusal, the send, the new token invalidating the old one - asserted through the endpoint that would honour the stale link rather than by counting rows - the new link actually verifying, the already-verified refusal, the allowance stopping the fourth send rather than merely reporting it, and one customer's exhausted allowance leaving another's intact. Three end-to-end tests for the button, its confirmation and the message on the fourth click. The 33 integration tests across the three suites this touched all pass, as do the 199 backend unit tests. tsc clean on both sides, ESLint no errors. Closes #110 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7c3463650d |
feat(backend): let a customer change their own name, password and email (#111)
SonarQube Analysis / sonarqube (pull_request) Failing after 12m11s
Two of the three already existed on the backend and had no caller. PUT /api/customers/me updated the name; POST /api/customers/change-password already demanded the current password and enforced the eight-character minimum. Neither was reachable from the frontend, which is why the gap was easy to miss — the API looked finished. The name endpoint accepted empty values and wrote nulls, letting a customer clear fields registration refuses to let them skip. That is the same rule disagreeing with itself, so it now refuses each by name exactly as registration does. Changing a password now ends other sessions and keeps the one making the change. Reset already deleted every session for the customer, on the reasoning that a password is changed precisely when the old one may be known to someone else — change reached the opposite conclusion for no recorded reason, and a session opened with a leaked password outlived the change meant to lock it out. The current session is spared so the change does not eject the person making it. Changing the email address is new. It asks for the current password, because swapping the address a password reset goes to is how an account is taken over and a live session alone is not enough; that also matches what change-password already required. The address is normalised and validated, an address another account holds is refused with the same 409 as registration, and on success the row is marked unverified and any outstanding verification token superseded — one already sitting in the old inbox must not be able to verify the new address. Two emails then go out, to different places. Verification to the new address, and a notice to the old one naming what the address was changed to. The notice is the only thing that tells a real owner their account was taken, and one that does not say where the address went is nearly useless to someone checking whether it was them. Both sends happen after the row is written, never before, so a change that failed cannot produce mail saying it succeeded. That notice is a sixth template in #92's system, which cost a definition and a default body. The unit tests iterate every template, so its defaults were checked against its own required placeholder without writing a new test. Verified: 199 unit and 208 integration passing. The session test signs in on a second agent, changes the password on the first, and asserts the second is refused while the first still works — the property being claimed rather than the code path being executed. One of my own assertions was wrong on the way: /me answers an unauthenticated caller with 401 and an error body, not an empty one, and the frontend is what turns that into null. Refs #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4ed9513ad2 |
feat(backend): make the five customer emails editable copy (#92)
Every customer email was a template literal in the route that sent it, so changing a word meant a code change, a review and a deploy. All five now render from markdown that an admin can edit: verification, password reset, favorite sold, favorite withdrawn, and the cart reminder. Five, not the four the issue counted — the favorite alerts have separate copy for sold and withdrawn.
markdown-it runs with html disabled, which is its default and the reason for choosing it over marked. Raw HTML in a stored body is escaped rather than passed through, so editing copy cannot put script into a customer's inbox. That is a stronger guarantee than sanitising output, because there is no output to sanitise.
Values are substituted into the markdown before it renders, which means a value that should become a list has to arrive as markdown. The cart reminder previously built li elements by hand; those would now be escaped and shown to the customer as literal angle brackets, so it emits a markdown list instead. The greeting is one placeholder rather than a bare name, so a template author writes {{greeting}} instead of "Hi {{firstName}}," — which reads as "Hi ," for anyone who registered before first names were required.
Saving is refused when a body has dropped a placeholder it needs, naming all of them rather than the first. This is the rule that separates a convenience from a way to break password resets from a settings screen: a reset email with no link still sends, still looks correct in the log, and is useless to everyone who receives it.
The favorite alerts' consent sentence is appended by the server and is not editable. It explains why the customer is receiving the mail, which is a compliance artifact rather than copy, and editing wording should not be able to delete it.
Unset templates fall back to the built-in defaults, so an install that never touches the settings screen behaves exactly as it did. The API reports an uncustomised template as null rather than as its default text, so "never edited" stays distinguishable from "edited to something identical", and DELETE restores the default by forgetting the row rather than writing the default into it.
Two problems surfaced during verification, both worth recording.
Five favorite-alert tests failed with no error and no mail. The cause was not this code: resetDb does not truncate admin_settings, so a subject of "Gone" stored by the new template tests survived into a later suite and changed the mail it was asserting on. Cleaning up inside the template tests would have fixed only that pairing, so resetDb now clears stored templates for every suite — template rows are test data like any other, and one outliving the suite that wrote it makes a failure appear somewhere unrelated.
The withdrawal notification then failed on timing. Loading copy from the database made the sender async, and the removal path was fire-and-forget, so the response could beat the mail out of the door. Dispatch was previously synchronous even though the sends themselves were not awaited; that is now restored by awaiting it.
Verified: 197 unit and 195 integration passing, lint unchanged at 4 warnings. The admin screen for editing these follows in the next commit.
Refs #92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
b287c07747 |
feat: capture first and last name so emails can greet informally (#106)
Registration collected one optional Name, so every greeting had only a whole name to use: "Hi Thom Lamb," rather than "Hi Thom,". Both parts are now captured, and the cart reminder greets by first name. Both are required of anyone new, refused individually rather than as a single "name is required" so a form that filled one and not the other is told which. The columns are nullable even so, and that is deliberate. Marking them NOT NULL would mean backfilling legacy rows with empty strings, which asserts that every customer has a name — untrue of anyone who registered while the field was optional. The table records what is actually the case; the rule that new registrations must supply both lives in the route, where a missing field can produce a message naming it. The backfill splits on the first space, and it is lossy in a way no version of this avoids. "Thom Lamb" becomes Thom and Lamb; "Mary Jane Smith" gets a last name of "Jane Smith"; names that are not two parts fare worse. It was chosen over leaving the columns empty because nothing currently lets a customer correct their own name — PUT /api/customers/me exists but no frontend calls it — so empty would have meant permanently unpersonalised for every existing customer. The migration says so, so nobody later reads backfilled values as data the customer supplied in that shape. Verified against a seeded database rather than reasoned about, because this is the part that cannot be covered by the suite: migrations run in globalSetup before any test, and the old column is gone afterwards. Six representative rows through the real migration gave Thom/Lamb, Mary/"Jane Smith", Cher/null, " Padded Name " trimmed to Padded/Name, and null and whitespace-only names left as null on both. The down migration rejoins the parts and returns all six to their original strings. The old column is dropped rather than kept alongside, so there is one source of truth instead of two that drift. The admin keeps receiving a single composed display name. It only ever shows one — the list cell and the drawer title — and never edits one, so giving it both parts plus the joining logic would be work for no reader. Churn was the bulk of this: 14 backend registrations and 10 end-to-end registration forms. A first attempt at the backend fixtures also added names to login and password-reset payloads, which would still have passed since the server ignores unknown fields, but a login test implying login takes a name is a small lie; that was reverted and redone against register calls only. Verified: 172 unit, 183 integration and 95 end-to-end passing, lint unchanged at 4 backend and 27 frontend warnings. Not covered: the cart reminder itself, which runs from a cron and had no test before this either. The greeting change is a one-line substitution in that query. Refs #106 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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 |