a0238ea4edb68f1286839783647901766b19347a
66
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
|
88f76926d5 |
feat(passkeys): registration ceremony (#38)
A signed-in customer can register a passkey. Signing in with one is #39 and the management screen is #40, so nothing reads these credentials yet. The name column arrives here rather than in #37. That issue listed the columns the ceremony needs and this one is for the person: #40 shows a list and offers to revoke from it, and "phone" against "laptop" is the only thing that makes two rows tellable apart. Without it a customer revoking a credential is choosing between identical entries. The customer may name it, and otherwise it is derived from the authenticator's transports — a hint rather than a fact, so the defaults are deliberately vague. "This device" is honest about a platform authenticator in a way that guessing at a model name would not be. Single use is enforced by deleting the challenge and treating the delete as the check, in one statement with the expiry condition. A replayed response finds nothing to delete and is refused, and two requests racing cannot both see the row and both proceed. Beginning a second registration replaces any in-flight challenge for that customer, so pressing the button twice cannot leave the first one usable. The challenge is consumed before the response is verified, deliberately. A failed attempt must not leave one available for a second try, so an invalid response costs the ceremony rather than merely failing it. Never started, already used and expired all answer the same way. From the server they are one condition — no challenge this customer may still complete — and distinguishing them would tell someone guessing which guess was closest. excludeCredentials stops the same authenticator being enrolled twice, but it is a hint the browser may ignore, so the unique constraint on credential_id is what actually holds. Hitting it answers 409: the credential is already registered, which is not a failure of anything. userID is the customer id rather than the email. A userID is meant to be stable and opaque, and an email is neither — a customer changing theirs would otherwise look like a different person to their own authenticator. Discoverable credentials are requested as preferred rather than required, because #39 wants sign-in without the customer first saying who they are, and an authenticator that cannot store one should still be usable here. Mounted before /api/customers, like the addresses router: Express matches mounts in order and the broader prefix would otherwise swallow these. Verified: tsc clean for src and tests, lint 0 errors with no new warnings, 514 unit tests across 35 suites — twenty new, covering the naming rules. The schema mirror gains the name column, placed where kysely-codegen would put it. Not verified: neither migration has been run against a database, and the ceremony itself cannot be exercised without a browser and an authenticator. What CI can prove is that the routes exist, are wrapped, and refuse an unauthenticated caller; what it cannot prove is a real attestation, which needs a device. Closes #38 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
6d320fd867 |
feat(passkeys): schema, dependency and per-environment Relying Party (#37)
Groundwork only. Nothing reads any of this yet, and no behaviour changes. The Relying Party ID is derived from PUBLIC_URL rather than written down, because it is the one value in this feature that cannot be corrected afterwards: a credential is bound to it permanently, and a wrong one surfaces only as a customer unable to sign in with a passkey that no longer matches anything. PUBLIC_URL is what every customer-facing link is already built from, so the ID is correct wherever those links are, and wrong only where they were already wrong. hostname rather than host, so a port cannot reach an ID that must not contain one. Local development is the exception the issue's table did not cover. envValidation requires PUBLIC_URL only when SMTP is configured, so a local setup that cannot send mail legitimately has none and falls back to localhost, which browsers treat as a secure context. Two origins there rather than one: the app is served by Vite on 5173 during development and by Express on 3000 once built, and those differ only by port, which is not part of the RP ID. The challenge table is separate from customer_tokens, and the reason is structural rather than preference. customer_tokens.customer_id is NOT NULL, and an authentication challenge is issued before anyone is identified — a discoverable-credential sign-in has no customer to attach to at the moment the challenge exists. Storing it there would mean making that column nullable for every other kind of token. Two of the issue's open decisions are deliberately not made here, because they belong to the ceremony that enforces them rather than to the schema. What to do when the signature counter fails to increase is #39's: many synced passkeys report zero forever, so treating a non-increase as cloning is wrong for them and right for a hardware key, and this only has to hold the value. Whether a disabled account can authenticate is also #39's, and the schema takes the position that it should not cost the customer their devices: credentials survive disabling and are refused at the ceremony, so re-enabling does not mean re-registering everything. Deletion is different and is settled here — credentials cascade with the customer, since one outliving its owner could authenticate as an account that no longer exists. signature_counter is BIGINT because the spec allows a 32-bit unsigned value, which overflows a signed INTEGER at half its range. That is the first bigint column in this schema, so the generated mirror gains the Int8 alias with it. Both tables are added to resetDb's TRUNCATE list and to REQUIRED_TABLES, and the schema mirror is updated by hand to match what kysely-codegen emits — placement and all, so a real regenerate produces no diff. Skipping either is how #56 turned a green local run into a red main; the mirror drift guard exists precisely to catch it, and schemaLoss's count moves from 18 to 20 with them. Verified: tsc clean for src and tests, lint 0 errors with no new warnings, 494 unit tests across 34 suites including nine new ones for the RP derivation, frontend build green, and the migration parses. Not verified: the migration has not been run against a database, and the integration suite needs one this machine cannot provide. Worth knowing before this goes further: #313 changes the domain, and every passkey registered before that cutover stops working at it. This code needs no change — it follows PUBLIC_URL — but the credentials do not survive. That is free while production is not live and nobody holds one, and it stops being free the day the shop opens. Closes #37 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
8956b9f122 |
fix(items): read the id strictly, and stop the error test needing a route that does not (#307)
Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207. It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart. Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own. readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed. readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing. The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently. Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say. Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at. Refs #307 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> |
||
 bermudalambandClaude Opus 5
|
36e6057c6e |
fix(tests): close the route guard's factory hole and two regex warnings (#307)
Three of the items on the cleanup issue, and the first is the one that mattered.
routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.
Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.
The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.
The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.
An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
abe8ac8184 |
test(filters): merge the duplicate itemFilters import (#308)
backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ec1020891f |
refactor(db): build the item queries through Kysely (#308)
The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in. All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one. The second thing this buys may matter more than the first. pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one. Closes #308 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3659608abe |
test(images): release the sharp handle before teardown (#301)
libvips caches input file mappings in memory after a pipeline finishes. On Windows, this keeps an open handle on the input file, and the OS refuses to delete a file with an open handle. The animated-WebP test triggers a pipeline rejection (correctly refusing a multi-page rotation), so the mapping stays in cache and afterEach cannot remove the test directory. Disabling the cache costs these tests nothing: each file is read exactly once during its test, so there is no reuse to cache. With caching disabled, Windows can delete the input files and afterEach succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5c0c7186ff |
feat(images): turn a stored photo a quarter turn (#301)
The file half of the remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because the tag that said which way up the pixels went is gone. Those photos need a person to look at each one and turn it. Rewrites the pixels rather than recording an angle, because an angle obliges every consumer to honour it — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar is not ours to teach. Left is anticlockwise and right is clockwise, which is rotate(-90) and rotate(90); sharp reads a positive angle as clockwise. The direction test asserts a pixel rather than a dimension, because dimensions swap whichever way the turn goes — a reversed sign would pass every size assertion and ship a control that does the opposite of its label. The animated WebP case is the one that could destroy someone's file quietly. Reading such a file without the animated flag succeeds and hands back the first frame alone, so a rotation that omitted it would write a still back over the animation and report success. Passing the same flag reencodeInPlace passes makes sharp refuse instead — multi-page images turn only by 180° — which is the honest answer and leaves the file untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
445c9c4a22 |
fix(backgrounds): report a partial restore instead of throwing (#293)
restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ba26ee99f0 |
fix(mail-templates): add sample values for the upload-link placeholders (#260)
submitUrl, label and submissionsAllowed were added to the uploadLink template but SAMPLE_VALUES had no entries for any of them, so an admin opening Email templates, Upload link for a contributor, Preview saw literal {{submitUrl}} in the body — the preview being the only way to check an edit before saving.
The unit test meant to catch exactly this, in emailTemplates.test.ts, iterated a hardcoded KEYS array that predated intakeDraft and uploadLink, so it never checked either template's samples. KEYS is now Object.keys(TEMPLATES) as TemplateKey[], so the guard covers every template automatically and cannot go stale the same way again. intakeDraft already had samples for all its placeholders and passes once included, as expected.
One other test in the same file, "every template can address the customer", asserts that available contains greeting/firstName/lastName — a real invariant of the six customer-facing templates, but not of intakeDraft or uploadLink, which notify the shop and a contributor rather than a customer with a name on file. Switching that test to the new all-templates KEYS would have made it fail for both, so it now uses its own explicit CUSTOMER_FACING_KEYS list instead. That is a deliberate, commented exception: a hardcoded list is correct there because the claim itself does not extend to every template, whereas the SAMPLE_VALUES guard's claim does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
eaecc43379 |
feat(intake): record where an upload link was sent, and how to say it (#260)
Adds upload_links.contact_email and the uploadLink mail template. The column is nullable on purpose. Links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered rather than backfilled with something untrue; the requirement belongs in the create route, which is where new links are actually made. The template requires submitUrl, the same guard verification has on verifyUrl. An email inviting somebody to send in photos, with no way for them to do it, sends perfectly happily and looks fine in the log — it is the one failure here worth making impossible, and a test asserts the default body satisfies the guard it declares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
823796b92a |
test(mail): stop the allowlist test from reaching real Gmail (#260)
Review of the Task 1 commit approved the implementation but flagged the third test in mailOutcome.test.ts as a defect carried over from the brief: it set SMTP_USER, SMTP_PASSWORD, and an allowlist the recipient satisfied, so sendMail fell through both early-return guards and reached the real transporter, opening a live TLS connection to smtp.gmail.com:465. The .catch(() => 'threw') wrapper hid a fast auth rejection, a slow timeout, or an accidental real send equally, and on a restricted CI runner it would hang to the Jest timeout rather than fail fast. The fix deletes that test rather than replacing it. What it was trying to prove — that an allowlisted recipient is not blocked — is already covered hermetically by backend/tests/unit/mailAllowlist.test.ts, which exercises isAllowedRecipient directly across exact matches, plus-suffixes, domains, and refusals. The other two tests in mailOutcome.test.ts are untouched; they cover the two paths that return early, which is the entire point of the change, and neither one reaches the transporter. The file's top doc comment is updated to match: it now says only the two skip paths are covered here, names mailAllowlist.test.ts as where the allowlist's own behaviour is tested, and spells out why a third test that reaches the transporter does not belong in this file, so nobody adds one back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8a35c213fc |
feat(mail): have sendMail say what it actually did (#260)
It returned Promise<void> and returned early in two cases that were indistinguishable from success at the call site: no SMTP credentials, and a recipient outside MAIL_ALLOWLIST. A caller could therefore report that it had emailed somebody a message nobody would ever receive, and in QA — which restricts delivery deliberately, as its entire safety property — that is the normal case rather than an edge one. It now returns a MailOutcome saying which of the three happened. No existing caller changes: there are seven and every one ignores the result, so this is additive. Re-deriving the answer at a second site would have duplicated isAllowedRecipient and the SMTP check, which is exactly the drift the guard-in-one-place comment above them exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4e2357356b |
fix(scripts): make the database the e2e suite reads a recorded fact (#273)
The end-to-end helper opens its own connection to read a password-reset token, and nothing guaranteed it pointed at the database the application was actually using. With -E2eDb the app runs on redefined_e2e at 55501 while the helper kept its redefined_local default at 55500, so the app wrote to one database and the suite read another — and the specs failed for a reason that had nothing to do with them. start-local.ps1 now records the coordinates it chose in .local/database.json, and run-tests.ps1 reads them into TEST_PGHOST, TEST_PGPORT, TEST_PGUSER, TEST_PGPASSWORD and TEST_PGDATABASE before Playwright starts. The answer now comes from one place, written by the thing that made the decision at the moment it made it. It is written after Start-Database rather than before, so the file never names a database that failed to come up, and removed by -Stop, so a stopped stack does not leave a record pointing at a container that is gone. Setting all five closes the second fault in the same change. Invoke-IntegrationSuite sets TEST_PGPORT and PowerShell keeps it for the rest of the session, so a -Suite all run leaked the integration port into the e2e run that followed — with none of the matching credentials, leaving the helper offering redefined_local's password to the integration database. Overwriting every one of them is what makes that leak harmless. A missing record throws rather than falling back. A default is what produced both faults in the first place: always plausible, silently wrong, and it fails in ways that look like application bugs rather than configuration. The guard test is the point of the change as much as the fix is. This is the third instance today of two files having to agree with nothing comparing them — #107 and #118 were envValidation against a compose file, #287 was the workflow against start-local.ps1, and this is three files rather than two. The test pins the whole chain: that the writer records the five settings, that the runner reads each of them from the record rather than a default, and that the helper reads no connection variable the runner does not set. Removing a single line from the runner fails four of its assertions, which was checked rather than assumed. What it cannot do is run PowerShell, so these are text assertions against the scripts. That is weaker than executing them and still catches the drift that actually happened. Closes #273 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dfd900aadd |
fix(admin): stop reporting every removeImageBackground failure as a sidecar failure (#281)
The remove-background route's catch block turned every throw from removeImageBackground into a 502 "the background-removal service did not answer". But removeImageBackground also throws for an unrecognised file extension (a legacy .jpeg), for a file missing from the uploads volume, and when REMBG_URL is not set at all — none of which involve contacting the sidecar. The admin was told to retry a service that was never reached, while the real reason existed only in the server log. Added SidecarRequestError in rembgClient.ts, following the NoOriginalToRestoreError pattern already in backgroundRemoval.ts. It is thrown only for failures that happen after actually attempting to reach the sidecar: the fetch call itself throwing (now wrapped in a try/catch, covering unreachable and timed-out), a non-2xx response, or a response that is not a PNG. It is deliberately not thrown for "REMBG_URL is not set", since that path never attempts contact at all. The remove-background handler now checks err instanceof SidecarRequestError before answering 502; everything else answers 500 with a message that says what actually went wrong. Added a unit test pairing (rembgClient.test.ts) asserting the sidecar-contacted failures are SidecarRequestError and the unconfigured case is not, and an integration test (adminItemDrafts.integration.test.ts) proving a missing upload file answers something other than 502 with a message that does not claim the service did not answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8d12cb2f2d |
feat(intake): swap a photo for a cut-out, keeping the original (#281)
Adds backend/src/intake/backgroundRemoval.ts, the shared module the drafting worker and the admin endpoints both call so a cut-out obtained either way is undoable the same way. cutoutPathFor is pure and writes a new file beside the original rather than overwriting it, which is what keeps the original restorable and makes the JPEG-to-PNG change free. removeImageBackground only points the row at the new file after it is already on disk, and is idempotent via the original_image_path IS NOT NULL check — load-bearing twice, since it also stops a second pass from recording the cut-out as the original and losing the real one for good. restoreImageOriginal swaps the paths back and deliberately leaves the cut-out file on disk. Extends the Task 1 integration test file with a stub sidecar bound to an ephemeral port and covers the no-op-on-repeat case plus three failure modes (500, non-image body, unreachable), asserting the row is left untouched in every failure case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
61e9c239d3 |
feat(intake): talk to the rembg sidecar, always naming u2net (#281)
Adds the client that will let the intake path remove backgrounds from submitted photos via the rembg sidecar over HTTP. isRembgConfigured() reports whether REMBG_URL is set (unconfigured is a normal, working state, not a failure), and removeBackground() posts a file to /api/remove and resolves with the PNG bytes it gets back, rejecting on every failure — unconfigured, unreachable, a non-2xx response, or a body that fails the same magic-byte PNG check the upload path already uses. The one hard rule: every request names model=u2net explicitly and this is never configurable. The sidecar's default model, reached simply by omitting the parameter, is bria-rmbg, which is licensed non-commercial — a licensing problem that a shop cannot silently ship, and one that would produce a perfectly good image with nothing in it to reveal the mistake. The test that posts against a real stub HTTP server and asserts model=u2net appears on the wire is the only thing guarding against that regressing. Wires REMBG_URL into both docker-compose.qa.yml and docker-compose.prod.yml as an optional variable, right after ANTHROPIC_WORKSPACE_ID, following the existing style in each file's environment block and header comment. It is deliberately left out of envValidation.ts's ALWAYS_REQUIRED — requiring it would make an environment with no sidecar refuse to boot, which is exactly the failure mode this feature is designed to avoid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
a02214108a |
feat(build): pass the commit to the image as a build arg (#248)
The admin version stamp has reported commit "unknown" everywhere. #233 read it out of .git during the build and #235 removed that, because Portainer's build context has no repository history and the COPY failed every deploy — the version stamp became the thing that stopped deployments. #237 then established that building in Gitea Actions does not help: the Dockerfile no longer copies .git, so where the build runs is irrelevant. So the builder hands the commit over rather than the build going to look for it. ARG GIT_COMMIT, empty by default, passed through to writeBuildInfo, which prefers it and still falls back to reading .git so a local build stamps itself with no argument needed. An empty value is treated as absent rather than stamped. `--build-arg GIT_COMMIT=` is what an unset shell variable expands to, and a blank commit reads as one that happens to be empty rather than one nobody supplied. Nothing regresses for Portainer. It cannot pass the argument, so its images keep saying "unknown" exactly as today, and they still deploy — the property #235 was bought with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6c6aaa46eb |
fix(intake): send the workspace id an identity-linked key requires (#271)
Every draft in QA failed with a 400: "anthropic-workspace-id is required when authenticating with an identity-linked API key". A key issued against a workspace, rather than standing alone, is refused unless the request names the workspace it acts in — and the client was constructed with an API key and nothing else. Nothing about a key's shape says which kind it is, so no amount of configuration checking would have caught this. Only a real call would, which is exactly what #223's task 8 existed to make. Sent only when ANTHROPIC_WORKSPACE_ID is set. Plenty of keys need no workspace, and sending an empty header would turn the ordinary case into a different error rather than leaving it working. Both compose files carry it with an empty default so an unset variable cannot fail a deploy, and the cutover doc goes from fifteen interpolated names to sixteen — checked against the file, and every name in the list now matches one in the compose. The failure handling needed no change and got none. The submission kept its photos, the draft recorded ai_error, and the review queue showed the reason. A model call failing must never lose somebody's consignment, and it did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
f52976dec9 |
fix(admin): answer 404 for an item id that does not exist (#207)
Three routes in admin.ts answered a miss with a success. PUT /items/:id ran an UPDATE that matched nothing, committed happily, selected nothing back and replied 200 with an empty body — a success the admin client could do nothing with, and no record anywhere that the item was not found. mark-sold and mark-available did the same. The create route beside them has always used requireRow for exactly this, which is why this reads as an oversight rather than a decision.
A garbage id was worse in a different direction. Number('abc') is NaN, the driver sends it to Postgres as the text "NaN", Postgres raises 22P02 for an integer column, and the catch turned that into a 500 — so a caller asking for an item that cannot exist was told the server broke. Both now answer 404, because from the caller's side "/items/abc" identifies no item in exactly the way "/items/999999" does.
readId is shared rather than repeated, and rejects zero, negatives and fractions as well as text: every id in this schema is a positive serial, so anything else identifies nothing.
mark-sold now notifies favouriters only after the row is known to exist, so nobody is told about a sale that did not happen.
The issue asked for the same shape to be checked across the other admin routes. It was: unpublish already looks the item up and 404s, and the tags and categories PUT routes both do an existence check before their UPDATE, so their rows[0] is guaranteed. items.ts already guards the public read. These three were the only ones lying about a miss.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
0872cad4df |
fix(ci): stop pull request scans overwriting the dashboard's picture of main (#197)
SonarQube Community has no branch analysis. Every scan published under a project key replaces that project's single analysis, whatever revision it came from — so each pull request, and each push to one, overwrote the dashboard's analysis of main with the branch. The new-code period, the gate result, the coverage percentages and the hotspot list then all described whatever was scanned last, with nothing on the dashboard saying which revision that was. A gate that went green on a feature branch read exactly like a gate that went green on main. It was caught only by luck: #180's hotspots reported line numbers that landed on a comment and a blank line in main, which is the kind of nonsense a person notices. Everything else it misreported would have looked fine. scripts/scan-local.sh has always refused to do this, defaulting to a scratch key, and its header says why in as many words. CI walked into the hazard that script guards against. Now the two tell the same story. The suites still run on pull requests, which is where their value is — only publishing is restricted. The measures report is skipped alongside the scan, because with nothing published it would print main's numbers into a pull request's log, which is noise at best and misread as the branch's own at worst. A test asserts both steps carry the restriction and that the three suites do not, because the failure leaves no trace and the `if:` is one line for somebody to drop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
55cb7eaa13 |
feat(intake): count submissions across every link, and alert on abnormal volume (#227)
The count comes from item_drafts rather than a tally. Every submission creates exactly one row in the same transaction that creates the item, so the rows are the truth — a separate counter would be a second thing that can disagree with them, and the one that disagrees silently is always the counter. windowStart is the part worth testing on its own and the part most likely to be quietly wrong. A reset older than the window must not widen it, which would make the ceiling stricter over time rather than rolling; a reset in the future must not disable it; and a malformed value must not produce an Invalid Date, which compares false against everything and would silently switch off the limit it was set to impose. Each is a test. Alerts go through sendMail directly rather than the editable templates. An abuse alert is not copy anyone will want to reword, and making it editable means it can be broken — a required placeholder removed from an alert nobody reads until an incident is a poor way to discover the validation. The throttle is in memory, so a restart during an incident can send one extra alert. That beats writing to admin_settings from the request path on every refused submission; a replicated deployment would have to move it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
0517faca60 |
feat(intake): add the submission notification template (#224)
Editable from the settings screen like every other template. reviewUrl is the only required placeholder: a notification with no link in it still sends, still looks fine in the log, and is useless to whoever receives it, which is what the required-placeholder validation exists to catch. The two signed links are deliberately optional. They are absent whenever INTAKE_ACTION_SECRET is unset, and a template demanding them would leave an unconfigured environment unable to send this at all. A test asserts the template offers no way to publish. That the email cannot publish is what bounds the risk taken by pricing items on arrival, and it is a property of the copy as much as of the routes — a publish link in the body would be one nobody reviewed. Where the notification goes is an admin setting rather than an environment variable, for the same reason drafting_model is one: it is changed by whoever runs the shop, not by whoever deploys it. Empty is the default and means do not notify, which is a working configuration. INTAKE_ACTION_SECRET warns rather than fails at boot, like the drafting key. Being told an item arrived matters far more than being able to discard it in one click. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
acc1b406d8 |
feat(intake): sign the two actions an email may take (#224)
Signed over the item, the action and the expiry together. Signing any subset would let a link be replayed against a different item or upgraded to a different action, and leaving the expiry out of the payload would let anyone holding an expired link extend it by editing the timestamp in the URL. Each of those is a test. Compared through a second digest rather than directly, because timingSafeEqual throws when the buffers differ in length, and a truncated link is an ordinary thing to receive rather than an exception. Same idiom as the admin gate. actionUrl returns null rather than throwing when there is no secret or no PUBLIC_URL. An unconfigured environment still sends the notification with its review link — being told an item arrived matters far more than the shortcuts do — and a link that could not be verified must never be offered in the first place. Uses the shared trimTrailingSlashes rather than a trailing-slash regex, which is what utils.ts exports it for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7522826bd0 |
feat(intake): record who chose an item's price (#225)
Pure and separately tested because the failure it guards is silent. Items are priced on arrival, so the schema no longer stops a number nobody chose reaching the storefront — the review queue does, by showing that nobody chose it, and an item selling at a default price looks exactly like one selling at a chosen price. Editing the number is the only thing that confirms it. Publishing an untouched field deliberately does not, because that would record "I did not look at this" as "I approved this". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
eae2c15f1a |
feat(intake): draft a listing from photos and a note (#223)
The client is a parameter rather than a module import, so every test passes a stub. A test that reaches the real API is a defect in the test: this runs on a route a stranger with a link can trigger, and each call costs money. getAnthropicClient returns null rather than throwing when there is no key. An unconfigured environment is a working one, and the worker treats null exactly as it treats a failed call — one path rather than two. parsed_output is guarded, not asserted. The SDK returns null there when the answer did not satisfy the schema, which is what a model replying in prose looks like; failing cleanly leaves the submission queued for a retry, where asserting would crash the worker mid-loop. Absent usage figures are treated as zero for the same reason: undercounting a cost is survivable, throwing away a draft that actually succeeded is not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d27dcae62b |
feat(admin): choose the drafting model from Settings (#223)
The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting. A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse. One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have. Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for. The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5988f4a545 |
feat(intake): tell the model to describe rather than invent (#223)
Pure and separately tested, because this is where the correctness of every draft is decided. Nothing downstream can distinguish an observed detail from an invented one — the description arrives as prose either way — so the instruction is the only place that distinction can be enforced, and the tests assert it is actually present. On a one-of-a-kind item an invented "1930s hand-thrown stoneware" is not a cosmetic error but a false claim on a public shop, and the shop answers for it rather than the model. Visible damage is called out for the same reason in reverse: a buyer finding a chip on arrival is worse than reading about it beforehand. Categories and tags are listed rather than described, so the model chooses from what exists instead of inventing a taxonomy the storefront filters know nothing about, and declining is explicitly allowed so a model with no matching option does not pick the closest wrong one. The note is quoted and labelled as the sender's rather than merged into the instruction: it is untrusted text from an unauthenticated stranger and should read as evidence to weigh, not as something the shop asserts. A whitespace-only note counts as no note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
22910ce9e2 |
feat(intake): constrain what a draft may contain (#223)
The SDK validates the model's response against this before any of it reaches the database, so a model that answers in prose or invents a field becomes a caught error rather than a row full of nonsense. Everything the model may decline to answer is nullable, because it is told to say nothing rather than guess. A null category is a better answer than a wrong one, and resolving it is what the review queue is for. The name and description are not nullable: a draft without them is not a partial success worth storing. The price is an integer, bounded at both ends. A fractional, negative or absurd figure reaching the review queue is a number somebody has to notice is wrong, and being trustworthy at a glance is that queue's whole job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1b87e08262 |
build(intake): add the Anthropic SDK and warn when its key is absent (#223)
Both packages go in dependencies rather than devDependencies. The final Docker stage installs with --omit=dev, so the wrong section produces a container that fails on the first submission and nowhere else — which is how sharp went wrong in #226. ANTHROPIC_API_KEY is a warning, not a requirement. Absent, the container still boots and a submission still arrives, keeps its photos and waits in the queue undrafted. The photos are often the only copy of an item no longer in the sender's hands, so losing a consignment to an expired key would be a worse outcome than an item arriving without its description written. Silence would be wrong too: an operator who believes drafting is on and finds every item undrafted has nothing to tell them why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
2b2cbe119e |
feat(intake): generate and hash upload link tokens (#222)
The token is the entire access control on an endpoint the whole internet can reach, so both halves are pure and tested directly rather than through a request — the same reasoning that has uploadTypes.ts and keyByCallerAndEmail exported for their tests. 32 bytes of CSPRNG output, base64url so the value survives being pasted into a URL, a chat message or a QR code without escaping. That matters for something a person is handed rather than something a machine reads. The collision test runs a thousand generations rather than asserting the obvious, because a repeat would mean one person's link opening another's. SHA-256 rather than bcrypt, and the reasoning inverts the one that governs passwords. A password hash is slow on purpose because a human password carries little entropy and must survive an offline dictionary attack. This is 256 bits from a CSPRNG: there is no dictionary, so slowing the hash buys nothing. Meanwhile the digest is computed on every submission to an unauthenticated endpoint, where a deliberately slow hash would be a denial-of-service surface — #242 is the local proof that cost-12 hashing on a request path is enough to push it past a timeout under load. No timing-safe comparison, deliberately: the lookup is an indexed equality match on the digest rather than a byte-by-byte compare of the secret, and an attacker who could mount a timing attack against a 256-bit random value would still need the value. Backend: 307 unit tests, lint unchanged at 6 pre-existing warnings, build clean. Ref #222 |
||
|
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
|
44328d0b5c |
feat(admin): show the deployed commit and build time in the admin (#233)
There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code.
The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above.
The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container.
Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy.
Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail.
Verified in the built image rather than argued: the stamp inside it reads
|
||
|
bermudalamb
|
e85be0f970 |
feat(uploads): re-encode images to strip metadata and bound dimensions (#226)
Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. The same reasoning that makes uploadTypes.ts an allowlist rather than a denylist. `needsProcessing` is pure and separately tested because it is the whole of the backfill's idempotency argument: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Being wrong there degrades every image a little more on every run. Anything sharp cannot describe is processed rather than skipped, since a file we understand least is not one to assume is safe. Verified end to end on a real image before wiring anything up: 3000x2000 with EXIF present became 2000x1333 with EXIF absent, and no temporary file was left behind. Corrects something this README claimed an hour ago. Installing under a Node below 20.9.0 does produce a broken sharp, because npm skips the optional platform binary when the engine check fails and still reports success. But once that binary is present sharp loads and runs fine on 18.16.1 — `engines` is enforced at install time, not at require time. The README said the runtime was blocked, which would have sent someone switching Node versions to fix a problem that only the install created. Ref #226 |
||
|
bermudalambandClaude Opus 5
|
c704c07b89 |
docs(security): put the SQL injection invariant where it is enforced (#202)
#180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI". The three call-site comments are restored, with two corrections a review of the original found. They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added. This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker. `items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against. The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that. Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile. Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only. Verified: backend build clean, 280 unit tests pass. Closes #202 Refs #180 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
6104ebb459 |
feat(ops): read DEMO_MODE from the stack rather than the compose file (#190)
Reverses the position the previous commit took. Hardcoding it made a value that gets flipped without a code change require a commit and a merge to flip, which is backwards — and it is the operator's call, not the file's.
No default, deliberately. `${DEMO_MODE:-false}` is the obvious form and the wrong one: a default decides whether the shop takes money on the operator's behalf, silently, whichever way it points. Having none is safe rather than fragile because `checkDemoMode` is strict — an unset stack variable substitutes to an empty string, and anything that is not exactly `true` or `false` refuses to boot naming DEMO_MODE. That strictness is the whole reason interpolating this one is defensible, so the line says so.
The compose guard had to learn the difference. It hands each deploying file's entries to the real validator, and a literal `${DEMO_MODE}` is not a value `checkDemoMode` accepts, so both the DEMO_MODE assertion and the validateEnv check failed the moment the file stopped holding a literal. Each deployment now declares the stack variables it supplies, and a bare `${VAR}` named there resolves to the declared value before the file is validated. Every other bare `${VAR}` stays opaque exactly as before — those are secrets, and what is checked of them is that the line exists.
Be clear about what that guard can prove. It cannot see Portainer, so it does not verify the stack actually holds `true`; nothing in this repository can. What it does is keep the intent beside the file and make the pair inseparable — hardcode the compose line and the registry disagrees, change the registry and it no longer describes the file. The runtime half is the boot check, which fails loudly rather than falling back. Verified by mutation: hardcoding `false` fails the DEMO_MODE assertion, and deleting the line fails that and `validateEnv`.
Restoring real payments is now two Portainer values and a redeploy, with no commit — which is what #190 asks for.
|
||
|
bermudalamb
|
12b2f09d79 |
docs(ops): point the demo-mode notes at the right issue (#190)
The tracking issue was filed as #190; the compose banner and the guard test both said #191, guessing the number before it existed. A note that points at the wrong issue is worse than no note when the thing it tracks is production not taking money. |
||
|
bermudalamb
|
0a830cad1f |
feat(ops): put production in demo mode to complete the cutover (#191)
Production could not boot during the cutover to the committed compose file: `DEMO_MODE` is false there, which makes the three PayPal secrets required, and they were not available. Demo mode is the interim the compose file's own header sanctions for exactly this — the whole cart and checkout flow works and nobody is ever charged.
Two things made this cost more than it should have, and both are now written down rather than left to be rediscovered.
`DEMO_MODE` is hardcoded rather than interpolated, so setting a `DEMO_MODE` stack variable in Portainer does nothing at all — there is no `${...}` for it to substitute into and the file's value wins silently. That hardcoding is right: the one value deciding whether the shop takes money should not be flippable from a web UI without a commit anybody can read. But the failure mode reads as "I set it and it ignored me", so the line now says so.
Declaring `PAYPAL_CLIENT_ID=` with an empty value is identical to not declaring it. `isPresent` rejects a blank string deliberately, because set-to-nothing is a mistake rather than a value.
The state is loud in both places that can see it. The compose file leads with a banner saying production is taking no money, and `composeEnvironment.test.ts` asserts `DEMO_MODE` is `true` — that assertion is the guard, not a formality: it fails the moment the file and the expectation disagree, in either direction, so this cannot be switched back quietly and cannot be left on unnoticed.
#191 restores it.
|
||
|
bermudalamb
|
e3842b1a4c |
ci: fail at the end rather than part way through, so the scan still runs (#174)
`sonarqube.yml` already had a documented design for this: run every suite, produce coverage, scan, summarise, then fail at the end from recorded step outcomes. The comments on the gate spell it out and #142 fixed it once already. The integration suite was never wired into it — no `id`, no `continue-on-error`, and absent from the gate, where the unit and end-to-end suites had all three. So it was the one suite whose failure aborted the job. On every push since #154 started, step 9 failed and steps 10 through 15 were skipped, which means there has been no SonarQube analysis at all for the duration — not a degraded one, none. The end-to-end suite has not run in CI either, which is separately why #116 cannot be verified: the step that would demonstrate its fix is skipped rather than failing. Fixing only that step would have left the same shape in four other places, so every step from the first suite to the scan is now guarded and named in the gate: starting the backend, installing browsers, merging frontend coverage, and the scan itself, which until now took the summaries down with it. The preconditions before the suites — checkout, installs, build checks, migrations — still fail hard, because when they fail there is genuinely nothing to analyse. The job still fails. It fails at the end, having produced everything it could. The integration suite also gains the summary the other two already had. It was the only suite without one, so the failure this workflow has been stuck on presented as 36 assertion errors about categories and price filters rather than as a count — #154 records how expensive that misdirection was to read. `summarize-jest.js` already takes a label, so this is reuse. `tests/unit/workflowGate.test.ts` asserts the pairing that makes the design work: a step carries `continue-on-error` so it cannot abort the job, and the gate names it so it can still fail the job. Both halves are needed and nothing connected them, which is how this happened — and the other direction is worse, since a step guarded but unnamed cannot fail the job at all. The test checks the invariant rather than a list of names, for the same reason `composeEnvironment.test.ts` reads the real list rather than a copy. Verified by mutation: dropping the integration suite from the gate, un-guarding it, and removing `always()` each fail it. Confirmed by running the new flag combination rather than assuming it: the integration suite writes `integration-results.json`, the summariser reads it and exits 0, and both that file and `coverage/integration/lcov.info` are still written when the suite fails — which is what makes scanning with a failing suite produce real coverage rather than a fabricated regression. Closes #174 |
||
|
bermudalamb
|
cf1680dbfb |
feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)
The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103 |
||
|
bermudalamb
|
faf38be91a |
feat(filters): make the storefront filter panel searchable and multi-select (#139)
The filter drawer did not scale with the taxonomy behind it. Categories were a bare antd `Tree` rendered at whatever depth it had grown to, with no search and single selection, and tags were a wall of every tag in the system. Neither said what was selected except through highlighting and chip colour. Both are now searchable multi-selects. Categories keep their hierarchy in a `TreeSelect`, matching the admin's `CategoryTreeSelect` so the two screens behave alike; tags become a multiple `Select` whose selected pills keep their colours, which is the only place a tag's colour was load-bearing. Several categories combine as OR. A customer picking Furniture and Decor wants both, not the empty intersection, and each selected id still expands to its descendants, so the answer is the union of the subtrees. That is deliberately the opposite of the tag rule, which stays AND, and both headings now state their rule rather than leaving it to be discovered. `ItemFilters.categoryId` becomes `categoryIds` end to end. The recursive CTE is seeded with `= ANY($n::int[])` rather than one id, which walks every selected root in one recursion and gives the OR for free; matching on `IN` keeps it a set test, so an item under two selected branches still appears once. The query parameter keeps its singular name and becomes comma-separated, the shape `tags` and `status` already use, so every `?category=1` link written before this still parses as a list of one. A list containing anything unreadable is still a 400, per decision 9 — honouring the readable half would answer a narrower question than the one asked and look indistinguishable from a filter that worked. The admin's inventory filter stays single-select, since it asks what is in a category rather than in any of several, but reads and writes a list of at most one so there is one shared filter type rather than two that drift. Closes #139 |
||
|
bermudalamb
|
f32913ef51 |
refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)
Indexing an array now yields `T | undefined`, which is what it always did — the compiler simply says so. Enabled in backend/tsconfig.json and frontend/tsconfig.json, and in tsconfig.sonar.json alongside it so the drift guard stays satisfied. The sequencing this issue insisted on was right, and the numbers show why. Before #159 typed the query results, `rows[0]` was `any` and `any` indexes to `any`: the flag would have found close to nothing and the count would have changed completely afterwards. With the rows typed it finds 57 — 50 in the backend, 4 in the frontend, 3 in the Playwright suite — and they fall into three kinds. Thirty are `rows[0]` after a `rows.length` guard. TypeScript cannot connect the two, and rewriting them as `const [row] = rows; if (!row) …` makes the guard and the use the same check, which is better code independently of the flag. Ten are rows a statement guarantees — `INSERT … RETURNING`, or a lookup for an id the session middleware has already matched. These get `requireRow(rows, what)`, a new helper in db.ts that throws naming the query. A thrown error rather than a non-null assertion: if the assumption is ever wrong, an assertion hands `undefined` to the next line and fails somewhere unrelated, whereas this fails at the query and says which. asyncRoute turns it into a 500, which is the right answer for "the database did not do what the statement says it does". It also states the assumption once instead of ten times. The rest is ordinary indexing the compiler cannot prove: a regex capture group that the pattern guarantees, `split('+')[0]`, a modulo kept in range, `hasOwnProperty` failing to narrow an index signature, and Express typing route params as an index signature so `req.params.itemId` is `string | undefined` on a route that cannot match without it. One correction to this issue's premise, which matters for what it was expected to find. The body says "in a handful it does not guard at all", and the unguarded-500 risk it describes was not found. Every `rows[0]` either sits behind a length check or behind a statement that guarantees a row. What the flag actually bought was the ten places where that guarantee was real but unstated, and those now say so. Two changes worth calling out because they are not mechanical. TAG_COLORS is typed `[string, ...string[]]` in both copies rather than `as const` — the first attempt used `as const`, which narrowed the elements to literals and broke adminTags, so the annotation keeps `string` while telling the compiler index 0 exists. And the filter drawer's slider falls back to the bounds it was given rather than to null, because null there reads as "no filter" and would widen the results rather than leave them unchanged. Test files needed changes too, since ts-jest compiles them against the same config: a regex destructure in the compose guard, and ten `mock.calls[0][0]` reads where the surrounding assertions already establish the call happened. Verified: tsc clean over backend, frontend src and the Playwright suite; unit 254/254; integration 238/238; frontend build clean; lint unchanged in both workspaces. Closes #101 |
||
|
bermudalamb
|
3fde6fc6bf |
fix(deploy): commit production's compose and bring it under the drift guard (#118)
Production refused to boot with "UPLOADS_DIR is required and is not set" while UPLOADS_DIR was set in Portainer's stack variables. Both statements were true at once. Portainer substitutes stack variables into the compose file rather than handing them to the container, so a variable with no line in the file never reaches the app — behaviour the QA compose already warns about at its ADMIN_GATE_SECRET entry, hit in production where nothing was watching for it.
Nothing could have caught it. The drift guard reads docker-compose.qa.yml, and production ran from a Portainer stack outside the repository that no test could see. That is worse than an even gap: UPLOADS_DIR is in ALWAYS_REQUIRED and the test was green, so the natural reading was that the deploying environments set it. QA did. Production did not.
So production's compose is now a file in the repository, deployed as a git repository stack rather than pasted into the web editor — otherwise the committed copy and the running copy drift apart again, which is the whole problem.
Values are hardcoded rather than interpolated wherever they are not secrets. Only a secret has a reason to stay out of the repository, and every interpolation is another chance for the failure above. UPLOADS_DIR in particular has to agree with the volume mapping, and splitting it across two files is how they drift.
The guard now runs over every deployment rather than QA alone, and checks each by handing its parsed entries to validateEnv itself rather than restating the rules. A restatement is one more copy to drift; running the real validator means the file is checked against exactly what the container checks at boot. Interpolated ${SECRET} values count as present, which is right — what is being guarded is that the line exists, since that is what decides whether the value reaches the container.
Environments differ on purpose, so the expectations are registered per file rather than shared: QA is demo mode with a mail allowlist and no PayPal credentials, production is the reverse of all three. A root-level compose file that is not registered fails the last test, so adding an environment forces the decision instead of silently inheriting whatever the loop asserted.
Verified by removing the UPLOADS_DIR line from the production file and confirming three tests fail, one of them reproducing the exact boot error. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot.
Two things found while writing this and deliberately not changed here. RESERVATION_MINUTES is set in QA's compose and read nowhere in the code — drift in the opposite direction, which #118's scan half should catch. And production publishes 32750 on every interface exactly as QA does; that is #117, and the reasoning is recorded in a comment at the ports block rather than acted on, since changing it needs the proxy host entry repointed in the same pass.
Refs #118
|
||
|
bermudalamb
|
2f7268704a |
feat(admin): make the token lifetimes, cart hold and greeting configurable (#136)
Giving the customer emails a tab of their own (#135) made a family of related holes visible: an admin could edit the wording of every customer email, but not the facts that wording asserted, and two templates could not address the customer at all. Both token lifetimes were hardcoded twice. VERIFY_TOKEN_TTL_MS sat in routes/customers.ts while the verification body separately said "This link expires in 24 hours", and RESET_TOKEN_TTL_MS sat beside a body separately saying "one hour". The prose was not derived from the constant, it was a second hand-written copy of the same fact — so making the constants configurable without addressing that would have made things worse, not better: the setting moves to two hours and the email keeps confidently promising one. Both are now settings, and both templates state their lifetime through an {{expiresIn}} placeholder rendered from the setting. The cart reminder gains {{holdDuration}} for the same reason. Per-item deadlines were already inside {{itemList}}, but there was no way to write a sentence about the hold itself without hardcoding a number the cart expiry setting could change underneath the author. All three durations render through one formatDuration(), so the reset email and the cart reminder say "one hour" the same way rather than in two authors' phrasing. A fractional hour drops to minutes, because "0.5 hours" reads badly and "1.5 hours" reads worse in a sentence a customer is meant to act on. Every template now offers greeting, firstName and lastName. favoriteSold and favoriteWithdrawn previously offered only itemName and siteUrl and could not address anyone — the query behind them never selected a name, so it does now. The greeting itself is two settings, a format and a fallback, rather than the wording baked into greeting(). The fallback is separate rather than the format with the name edited out: that editing is guesswork that has to be right every time, and getting it wrong ships "Hi ," to everyone who registered while first names were still optional (#106). Those customers exist, which is why greeting() guarded the case in the first place. The admin preview renders the durations and the greeting from the live settings rather than from a static sample. The preview exists so an admin sees the email that will be sent, and a sample reading "one hour" while the setting says two is the precise failure these placeholders were added to remove. Settings are read through a new adminSettings accessor. cart_expiry_hours was previously read by an inline query in two places, each with its own `|| '24'`; with five settings and read sites across three routes and the cron job, a default written twice is a default that will eventually disagree with itself. Values are stored as text, so each definition declares how to read it back — numbers were the only kind until the greeting format arrived. The new placeholders are available but never required, so every template an admin has already saved keeps rendering and keeps sending. Also fixes updateAdminSettings announcing success for a save the server refused: it returned the 400 body as though it were the saved settings, so the form reported "Settings saved" either way. Closes #136 |
||
|
bermudalambandClaude Opus 5
|
32b3f616d9 |
feat: filter by Sold / Not sold / All on the storefront and the admin (#105)
Three decisions were taken before any code, and are recorded on the issue. The status filter is generalised to accept several values rather than gaining a second `sold` dimension beside it. "Not sold" is not a status: it is available-or-reserved on the storefront and includes pending in the admin, neither of which is one value. `?status=available,reserved` and `i.status = ANY($n::text[])` express that with one concept, so there is no way to write a contradiction like `?status=sold&sold=no`. A single status still parses to a list of one, which is how the admin's existing `?status=sold` keeps working untouched. The storefront now defaults to Not sold. That is a change in what every customer sees, not just a new control: the black SOLD ribbons leave the default view on a catalogue where they were evidence the shop sells things, and every storefront link shared so far quietly changes meaning. Accepted deliberately, with the default named in STOREFRONT_DEFAULT_STATUSES rather than implied by the absence of a parameter. The admin's four-way status dropdown is replaced rather than joined. That gives up isolating a single status: there is no longer a way to view only Reserved, or only Pending, and Not sold folds pending in with the rest. The pending workflow from #90 is the likeliest thing to miss it, and if it does, the fix is to put isolation back beside the preset rather than to remove the preset. The e2e test that covered "which is how Reserved is reached" is renamed and narrowed to what survives, rather than deleted. One thing the issue did not anticipate, found by a test rather than by reading. The favorites view deliberately showed sold favorites - "a favorite that has just sold is often exactly what the customer came to look at", and they have just been emailed to say so. Defaulting the storefront to Not sold reversed that silently and broke the test asserting it. Favorites therefore keep their own default of everything, on the server and in the control's displayed position, while an explicit ?status= still wins. That interaction is the kind a single-feature change quietly breaks, and it was caught only because the previous decision had been written down as an assertion. "All" still means different things in the two places, as the issue set out: available + reserved + sold on the storefront, all four in the admin. Pending remains unreachable from every public read - the storefront's unconditional exclusion clause is untouched - and the pending guard now checks every requested status rather than a single one, so `?status=available,pending` is refused for naming pending at all rather than accepted because the first name happened to be allowed. The control sits in the filter bar rather than in the drawer, since the default now hides sold pieces and a customer who never opens the drawer would otherwise have no way to know they exist. It is consequently excluded from the "Filters (N)" count, which describes the drawer, while still counting toward hasActiveFilters so that an empty result reads as "no items match these filters" with a way out rather than as an empty shop. Verification: 13 integration tests covering the default, each preset, the favorites exception and its override, and pending's unreachability under every accepted combination; 38 parser unit tests including multi-value parsing, an unknown name in a list being refused rather than dropped, and a list that names nothing; 6 new end-to-end tests for the storefront control, its URL round-trip, and the default staying out of the URL. 204 backend unit tests and 76 integration tests across the four affected suites pass. Two full end-to-end runs: 110 and 111 passing against the same 3 pre-existing failures, one run also showing a pending-publish failure that passes in isolation and did not recur - the cross-suite database contention filed as #116. tsc, ESLint and the production build are clean. Closes #105 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
1fa723bd19 |
feat: tabs and a rendered preview for the email templates (#119)
Follow-up to #92, which shipped the editable templates as a column of stacked cards. With six templates the cart reminder sat below five editors, so reaching it meant scrolling past all of them and which one you were editing was knowable only from a card title you had already scrolled past. They are tabs now, and the Default/Customised tag moves onto the tab label, so which templates have been changed is visible without opening each one. The larger gap was that there was no way to see what the email would look like. The editor is a markdown textarea; what gets sent is rendered HTML with placeholders substituted and, for the two favorite templates, a consent footer appended by the server. An admin editing copy could not tell whether the result read correctly. POST /api/admin/email-templates/:key/preview renders the draft in the editor rather than what is stored, so the effect of an edit is visible before committing to it. It renders on the server deliberately: renderTemplate is the only thing in the system that turns this markdown into HTML, and markdown-it is configured there with html: false, which is the control that stops an admin putting script into a customer's inbox. A renderer in the browser would be a second implementation of both, and a preview that disagreed with the mailer would be worse than none. It does not enforce required placeholders - saving refuses a body that dropped one, and previewing it is how the admin sees what they have done. The preview renders into a sandboxed iframe rather than through dangerouslySetInnerHTML. The markup is safe by construction, but an email is its own styling context: rendered inline, the admin theme's CSS would change how it looks and the preview would lie about the result. Sample values live beside the template definitions rather than in the route, so adding a placeholder puts the missing sample next to the change that needs it. A unit test asserts every available placeholder has one, because a missing sample renders a literal {{placeholder}} into the preview and teaches the admin their copy is broken when it is not. This also fixes a test that has been failing on main. email-templates.spec.ts located the Save button by filtering .ant-card for the template name, which matched an outer card containing every template's Save button - six of them - and died on a strict mode violation, taking two more tests with it as unrun. Only the active tab's editor is mounted now, so the labels are unambiguous and the filter is gone. Verification: eight end-to-end tests, four for editing and four for the preview, covering the draft being previewed rather than the stored copy, sample values replacing placeholders, raw HTML being escaped exactly as the mailer escapes it, and the consent footer appearing on a favorite template and not on a password reset. The full suite goes from 100 passed / 3 failed / 2 unrun to 112 passed / 2 failed / 0 unrun; the two that remain are the pre-existing password-reset failures that need a database on port 55432 and fail identically on main. 38 backend unit tests pass, tsc and ESLint are clean. Closes #119 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
7c3463650d |
feat(backend): let a customer change their own name, password and email (#111)
SonarQube Analysis / sonarqube (pull_request) Failing after 12m11s
Two of the three already existed on the backend and had no caller. PUT /api/customers/me updated the name; POST /api/customers/change-password already demanded the current password and enforced the eight-character minimum. Neither was reachable from the frontend, which is why the gap was easy to miss — the API looked finished. The name endpoint accepted empty values and wrote nulls, letting a customer clear fields registration refuses to let them skip. That is the same rule disagreeing with itself, so it now refuses each by name exactly as registration does. Changing a password now ends other sessions and keeps the one making the change. Reset already deleted every session for the customer, on the reasoning that a password is changed precisely when the old one may be known to someone else — change reached the opposite conclusion for no recorded reason, and a session opened with a leaked password outlived the change meant to lock it out. The current session is spared so the change does not eject the person making it. Changing the email address is new. It asks for the current password, because swapping the address a password reset goes to is how an account is taken over and a live session alone is not enough; that also matches what change-password already required. The address is normalised and validated, an address another account holds is refused with the same 409 as registration, and on success the row is marked unverified and any outstanding verification token superseded — one already sitting in the old inbox must not be able to verify the new address. Two emails then go out, to different places. Verification to the new address, and a notice to the old one naming what the address was changed to. The notice is the only thing that tells a real owner their account was taken, and one that does not say where the address went is nearly useless to someone checking whether it was them. Both sends happen after the row is written, never before, so a change that failed cannot produce mail saying it succeeded. That notice is a sixth template in #92's system, which cost a definition and a default body. The unit tests iterate every template, so its defaults were checked against its own required placeholder without writing a new test. Verified: 199 unit and 208 integration passing. The session test signs in on a second agent, changes the password on the first, and asserts the second is refused while the first still works — the property being claimed rather than the code path being executed. One of my own assertions was wrong on the way: /me answers an unauthenticated caller with 401 and an error body, not an empty one, and the frontend is what turns that into null. Refs #111 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |