903a1d8b76ab3d5961933634d3562f859d77aae8
13
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
9cc82002b8 |
feat(auth): groundwork for signing in with Google (#340)
Nothing a customer can see. The schema change and the configuration land on their own so the widest-reaching edit in the project can be reviewed for what it is rather than buried inside a feature. The password hash becomes nullable. That is one line and it is not the work; the work is that every read of the column is now a question rather than a fact. Three places compared against it with bcrypt, and all three now ask first through one shared function. That function exists because the alternative is worse than a wrong answer. bcrypt.compare throws on a null hash rather than returning false, so any call site that forgot the check would answer a sign-in attempt with a 500 instead of a refusal. On the login route that is also an oracle, because it would happen for exactly the accounts that have no password. One function rather than a null check repeated three times means the question is asked identically everywhere and a fourth site cannot forget to ask it. Nothing writes a null yet. The first accounts without a password arrive with the sign-up path, which is why this is landed ahead of them. The identities table is a table rather than columns on customers, because one customer may eventually hold more than one. Columns would make a second provider a migration and a third an embarrassment. Its important column is the provider subject, and the comment on it is the whole security posture of the feature in one place: never the email. An email is a display value its owner can change and a provider may reassign; a subject is opaque and stable for the life of the account. Matching on the email would strand a customer who changed theirs and, far worse, hand their account to whoever inherited the old address. Unique across the provider and subject together, not the subject alone. The down migration drops the table and deliberately does not restore the NOT NULL. Re-adding it fails outright once a passwordless customer exists, and a down migration that destroys accounts to satisfy a constraint is far worse than a column that is merely more permissive than it needs to be. The redirect URI is derived from PUBLIC_URL, the same single source the WebAuthn Relying Party ID uses and for the same reason: Google compares it as an exact string and answers a mismatch with a message that says nothing about which half is wrong. Deriving it means the value is correct by construction anywhere the email links already are. The tests are mostly about what must not end up in it, since a trailing slash on PUBLIC_URL is an easy way to produce a URI that is one character from the registered one. The config also reports whether it is enabled at all, so a developer without credentials gets a storefront that works and simply does not offer the button, rather than one that offers it and fails. Absent rather than disabled, the same choice made for a browser without WebAuthn. Environment validation refuses to boot on one credential without the other, matching how the SMTP pair is handled. Half-configured is the case worth catching because the failure otherwise arrives at the moment a customer presses the button. The QA compose file sets both to empty, and the comment there says why at length rather than leaving it to look like an oversight. Google refuses a redirect URI whose host is not under a domain whose ownership has been proved by DNS, and nobody can prove ownership of anything under bermudalamb.synology.me because Synology owns the registrable domain above it. That is the same wall #285 hit with Cloudflare. So QA cannot run this at all until #313 moves it to a subdomain of the real domain, at which point it is two stack variables and one console entry, with no code change either way. Also corrects the record in #332, which lists account deletion as confirming with a password. It does not; the route takes none and the confirmation is a modal in the account page. Deletion needed no change here. Verified: backend tsc clean for src and tests, 550 unit tests pass including new coverage of the config derivation, the null-hash comparison and the environment rules; lint clean apart from warnings that predate this branch. The integration suite needs a database this machine has no Docker for. Closes #340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
90e372d6bd |
feat(admin): move a customer's account to an address they can reach (#337)
The third step of the only recovery route a customer who has lost their mailbox has. The first two are contacting the shop and being verified against order history. The third had no implementation, so the answer was a hand-written database edit that left no record of who did it or why. The thing to say plainly, because everything here follows from it: this operation and an account takeover are the same operation. They differ only in whether the verification was sound, and nothing in the software can check that. What the software can do is make the change recorded, announced, and complete in its effects. Recorded. The endpoint refuses without a written reason, and the reason is stored against the account. That row is the only thing that tells a genuine recovery from a takeover afterwards, which is why a hand edit was never acceptable and why the field is required by the server rather than merely collected by the form. It is never shown to the customer: it is a note about how somebody was verified and can name things the customer should not be handed back. There is no column for who did it. Admin access is one shared gate secret in front of a single operator, so such a column could only ever hold a constant, and a constant dressed up as an identity is worse than an honest absence. Announced, to the address being replaced. If the recovery was sound that reaches nobody and costs nothing. If it was not, it reaches the real owner, who is the only person in the world who can say so, and that is the only reason this endpoint is safe to have at all. Its own template rather than the self-service one, because that copy says to contact us if you did not make this change, and here somebody already did — the sentence would be addressed to the customer who just did the thing it asks for, while the person who needs to act on it did nothing. The new address is marked unverified and sent a confirmation link. Somebody reading an address out over the phone has not demonstrated they can receive mail at it, and that is the commonest way this goes wrong harmlessly. Complete in its effects. The move signs the customer out everywhere, removes every passkey, and cancels reset links already sent. That is the conclusion #42 reached for password reset, and it applies here with more force: somebody the system cannot identify asked for this change, so a session or a credential surviving it is one the new owner cannot see and cannot revoke, and a reset link sitting in the mailbox being taken away would let whoever still reads it take the account straight back. The password is left alone. What the customer lost was the mailbox, so demanding a new one adds a step for no gain. The verification-email helper moved out of the customers route into its own module, for the reason session creation moved out for passkeys: two implementations that agree today are two that can be changed one at a time, and the one that gets forgotten is whichever the manual testing does not exercise. This path runs perhaps once a year, so it is exactly the one that would rot. The admin drawer gains the action next to the address rather than among the account controls, because it is a thing done to that field by someone already looking at it. It leads with the warning instead of burying it. The history of moves sits on the same drawer and renders nothing at all for the overwhelming majority of customers, who have never been moved. Verified: backend tsc clean for src and tests, 526 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. It also cannot be proven by CI right now — run 917 has been hung since it started and 24 runs are queued behind it, which is the same hang #154 identifies as the source of the leftover Postgres containers. Closes #337 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
|
2bc9440b38 |
test(ci): record which Postgres actually answered (#154)
This adds the measurement #154 has needed twice and never had. It does not fix the failure and does not guess at it: the issue has been wrong twice from reasoning ahead of evidence, and the point here is to make the next occurrence answer the question rather than reopen it. The observation that rules out every explanation so far is that the schema comes back. A suite fails because orders does not exist, and a later suite truncating that same table passes. A dropped database does not un-drop itself, so this was never one database losing its schema. More than one server answering to one name produces exactly this, and Docker embedded DNS round-robins every container sharing an alias, so a leftover service container from an earlier run fits every observation including the empty dmesg that killed the OOM theory. globalSetup now logs every address the database host resolves to. More than one is the answer outright. One address means this reading is wrong too, and the next suspect is a single container restarted with a fresh data directory. Alongside it, both globalSetup and a failing assertSchemaPresent record which server actually answered. pg_postmaster_start_time is what settles that and needs no special rights: two Postgres instances cannot share one, so differing values within a single run are proof, where a differing inet_server_addr alone could be argued to be one container that moved. The failure message now says to compare the two rather than leaving the reader to know that is the interesting comparison. Logged on a passing run as well as a failing one, deliberately. A failing run's addresses mean nothing without a passing run's to compare them against, and this issue has twice suffered from having only the failure to look at. Neither can throw. A diagnostic that fails the run it was added to explain is worse than no diagnostic, so both are wrapped and both degrade to a printed reason. The failure path costs one extra round trip, taken only when the schema is already known to be missing. assertSchemaPresent is not on the hot path — resetDb calls it only when its TRUNCATE has already failed. Verified: tsc clean, typecheck:tests clean, lint 0 errors with no new warnings, and the four message patterns schemaLoss.integration.test.ts asserts on are all still present. The probe reads only pg_catalog functions, so it still answers against the dropped schema that suite creates. Refs #154 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
 bermudalambandClaude Opus 5
|
0b6cc85c4f |
fix(lint): bring the backend test suites into scope (#298)
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
813216f971 |
test(integration): say so when the database loses its schema (#154)
When the integration run lost its schema partway through, it presented as 36 assertion errors about categories, price filters and favourite notifications. The real message — `relation "items" does not exist` — was further down the same log, and hours went into chasing the assertions instead. The cause is still open and needs runner-side evidence this cannot reach: whether the Postgres service container is being recreated mid-run, which would come back with an empty data directory. This is the half that can be fixed from here — whatever the cause, the next occurrence reads as "the database lost its schema" on the first line, names which tables are gone, and says that nothing in the suite drops tables so the database was replaced underneath the run. globalSetup asserts once after migrating, which establishes the fact the rest of the run depends on. Without it, a run that never had a schema and one that lost it midway are indistinguishable from the failures they produce. resetDb checks only when its TRUNCATE fails, rather than on every reset. It runs in a beforeEach several hundred times a suite, and an extra round trip each time to guard against a rare event would be paying continuously for it. When the schema is fine, an unrelated failure is passed through untouched rather than dressed up as a schema problem. The diagnostic is tested against a database that has actually lost its schema, not reasoned about. An earlier attempt dropped the schema before the run and proved nothing — globalSetup re-migrates, so it repaired itself and every test passed. The suite drops and rebuilds around each case and restores in afterAll; the full integration suite was then run twice to confirm the restore holds for everything ordered after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
22215f32eb |
feat(intake): settings for the submission ceiling (#227)
A count type beside the existing hours, text and choice readers. resolveHours would have worked — it is parseFloat with a positive guard — but calling a submission ceiling an "hours" setting is a lie in the type name that every later reader has to decode. Whole numbers only, so a ceiling of 12.5 is a typo rather than a preference, and a malformed value falls back rather than yielding a NaN that compares false against everything and silently disables the limit. resetDb is widened to clear intake_ settings as well as email_ ones. It deliberately does not truncate admin_settings, so a ceiling of 1 left behind by one suite would make every later suite's submissions refuse with a 503, in files that never mention a ceiling. The comment there already records that exact failure happening once with an email template subject, which reached the favorite-alert tests and failed five of them somewhere else entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
6df32af784 |
feat(intake): add upload_links and item_drafts, and default an item's price (#222)
The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later. `upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it. `item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not. `items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person. The number lives in the migration rather than in configuration. Changing a default price is a rare, deliberate act that deserves a record; an environment variable would let it drift silently between environments, and a wrong default is invisible until something has already sold at it. Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000. Backend: 263 integration, 302 unit, all passing against the new schema. Ref #222 |
||
|
bermudalambandClaude Opus 5
|
4ed9513ad2 |
feat(backend): make the five customer emails editable copy (#92)
Every customer email was a template literal in the route that sent it, so changing a word meant a code change, a review and a deploy. All five now render from markdown that an admin can edit: verification, password reset, favorite sold, favorite withdrawn, and the cart reminder. Five, not the four the issue counted — the favorite alerts have separate copy for sold and withdrawn.
markdown-it runs with html disabled, which is its default and the reason for choosing it over marked. Raw HTML in a stored body is escaped rather than passed through, so editing copy cannot put script into a customer's inbox. That is a stronger guarantee than sanitising output, because there is no output to sanitise.
Values are substituted into the markdown before it renders, which means a value that should become a list has to arrive as markdown. The cart reminder previously built li elements by hand; those would now be escaped and shown to the customer as literal angle brackets, so it emits a markdown list instead. The greeting is one placeholder rather than a bare name, so a template author writes {{greeting}} instead of "Hi {{firstName}}," — which reads as "Hi ," for anyone who registered before first names were required.
Saving is refused when a body has dropped a placeholder it needs, naming all of them rather than the first. This is the rule that separates a convenience from a way to break password resets from a settings screen: a reset email with no link still sends, still looks correct in the log, and is useless to everyone who receives it.
The favorite alerts' consent sentence is appended by the server and is not editable. It explains why the customer is receiving the mail, which is a compliance artifact rather than copy, and editing wording should not be able to delete it.
Unset templates fall back to the built-in defaults, so an install that never touches the settings screen behaves exactly as it did. The API reports an uncustomised template as null rather than as its default text, so "never edited" stays distinguishable from "edited to something identical", and DELETE restores the default by forgetting the row rather than writing the default into it.
Two problems surfaced during verification, both worth recording.
Five favorite-alert tests failed with no error and no mail. The cause was not this code: resetDb does not truncate admin_settings, so a subject of "Gone" stored by the new template tests survived into a later suite and changed the mail it was asserting on. Cleaning up inside the template tests would have fixed only that pairing, so resetDb now clears stored templates for every suite — template rows are test data like any other, and one outliving the suite that wrote it makes a failure appear somewhere unrelated.
The withdrawal notification then failed on timing. Loading copy from the database made the sender async, and the removal path was fire-and-forget, so the response could beat the mail out of the door. Dispatch was previously synchronous even though the sends themselves were not awaited; that is now restored by awaiting it.
Verified: 197 unit and 195 integration passing, lint unchanged at 4 warnings. The admin screen for editing these follows in the next commit.
Refs #92
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
f626f27e75 |
feat: favorite items and notify when a favorite is sold (#34)
Customers can favorite and unfavorite items from the storefront, opt in to being told when a favorite is sold to someone else, and manage that preference from their account page. The opt-in is a consent of its own rather than the existing marketing flag. Being told that a specific item you asked about has gone is a narrower thing than agreeing to marketing, and folding one into the other would leave marketing_consent_text no longer describing what was actually agreed to. It is recorded the same way as the marketing consent — flag, timestamp, and the exact wording shown — and accepting it does not set marketing_consent. The prompt appears only after a customer has actually favorited something, so the reason for asking is concrete rather than an abstract marketing ask, and it says plainly that it is separate from marketing email. Declining keeps the favorite. Notifications fire when an item reaches sold, either through checkout or an admin marking it sold, and never to the buyer — telling someone the item they just bought is unavailable reads as a bug. Reserved is deliberately not a trigger: reservations expire and get released, so a "gone" email would often be about an item still for sale. Disabled accounts are excluded, per #33. completeCheckout now returns the sold item ids and the buyer so its three call sites can notify after COMMIT. Sending inside the transaction would email people about a sale that then rolled back, and would hold the transaction open for SMTP. Each message is sent independently so one bad address cannot stop the rest, and the sale has already succeeded regardless. Favoriting while signed out opens the existing inline register/login modal, exactly as Add to Cart does, and completes the favorite on success. Also fixes a latent bug in the same component: while the session was still resolving, `customer` is null for a signed-in visitor too, so clicking Add to Cart or the new heart in that window prompted them to sign in again. Both now ignore clicks until the session has resolved, and the control shows as loading meanwhile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
9222e97deb |
feat(api): categories, tags, and storefront item filters (#23)
Adds a self-referencing categories tree, a tag registry with deterministic colours, and item_tags, plus admin CRUD for both. GET /api/items now accepts category, tags, min_price and max_price. Category matching walks the subtree with a recursive CTE so selecting a parent includes everything filed beneath it; tags match with AND via a count check, since ANY() alone would return items carrying only one of them. Malformed filter params return 400 rather than being ignored, so a broken link doesn't quietly list the whole catalogue. GET /api/filters serves the drawer its tree, tags, and price bounds in one request. Item image/tag aggregation moves from LEFT JOIN + GROUP BY to scalar subqueries. Joining two one-to-many relations multiplies their rows, so an item with 2 images and 3 tags would have repeated every image three times once tags were added. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
7f4479605a | chore: replace manual SQL migrations with node-pg-migrate | ||
|
bermudalamb
|
921022c658 |
Add backend unit/integration tests, Playwright e2e tests, README, cookie Secure fix
SonarQube Analysis / sonarqube (push) Successful in 4m20s
|