79a2b606ea4e816771ce368187c289893653ea57
5
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> |
||
 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
|
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
|
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
|
9c9e9c3ded |
feat(backend): check the environment at boot instead of discovering it later (#64)
The backend reads environment variables in a couple of dozen places and validated none of them. A missing or misspelled one was undefined until the first line of code that happened to need it, which could be a long time after the container reported healthy — and several of those failures are silent and customer-visible. DEMO_MODE is the one that mattered most. It was read as "demo unless the value is exactly the string false", so DEMO_MODE=False, DEMO_MODE=0, or any typo meant demo mode stayed on and the shop quietly stopped charging anyone. It is now required and strict: exactly 'true' or 'false', and anything else refuses to start while quoting the value it was given, so the typo is visible in the message rather than inferred. Two requirements are conditional, and that is what makes them expressible at all. PayPal credentials are demanded only when DEMO_MODE=false, because QA runs with none of them on purpose and an unconditional rule would be simply wrong there. PUBLIC_URL is demanded only when SMTP is configured, because its only job is building links in email — an environment that cannot send mail does not need it, and requiring it everywhere would break every existing local setup to prevent nothing. UPLOADS_DIR gets no such reprieve: its fallback is correct inside the container and wrong everywhere else, so inheriting it writes uploads somewhere nobody is looking. Every problem is reported at once rather than one per restart, and the process then exits — the same shape as the container refusing to start on a failed migration rather than serving against a schema it does not match. Warnings are printed but do not stop anything: SMTP absent, the admin gate inactive, or an allowlist missing while mail can be sent. That last one is new and earns its place, since SMTP with no allowlist means the environment can reach real customers, which is what #87 exists to prevent. The admin-gate warning moved here from server.ts, so one place says what this container is and is not configured to do. validateEnv is a pure function of the environment handed to it rather than a reader of process.env, so it is tested exhaustively without booting anything or mutating global state. It is called from server.ts and deliberately not from app.ts: the integration suite imports app directly and would otherwise become a configuration exercise. Its rules are one small function each at module level, because cognitive complexity counts everything declared inside a function and the first version scored 24 against a limit of 15. Verified as a real process, not only in tests. A missing DEMO_MODE, a DEMO_MODE of 'False', real payments with no PayPal credentials, and half-configured SMTP each exit 1 with the problems listed; a valid environment starts and serves. Note the exit codes were checked without a pipe, because $? after `| head` reports head rather than node and had first suggested a clean exit. 141 unit, 169 integration and 94 end-to-end passing, lint unchanged at 0 errors and 8 warnings. Both CI workflows already set all six always-required variables plus DEMO_MODE, so the pipeline is unaffected. Refs #64 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |