c9dccfe4a21a416bd2a1c73a9955ffdaa793550d
15
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
903a1d8b76 |
chore(qa): read the Google credentials from the stack, like every other secret
QA_GOOGLE_CLIENT_ID and QA_GOOGLE_CLIENT_SECRET were set on the QA stack and went nowhere, because #340 hardcoded the container's values empty rather than reading anything. The button stayed missing, correctly, but for a reason the file gave no way to discover: every other secret in it is read from a QA_-prefixed stack variable, and these two were the odd ones out. So they are wired the way the rest of the file works. The deploy that prompted this cost nothing except time, and the next one would have cost the same again. Wiring them is not the same as enabling them, and the comment now leads with that. **Leave both stack variables unset until QA moves off *.bermudalamb.synology.me.** 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 that one, because Synology owns the registrable domain above it — the same wall #285 hit with Cloudflare. Setting them today produces a button that fails at Google with redirect_uri_mismatch, and there is no console entry that could satisfy it. Once #313 moves QA to qa.redefined-designs.com it is three steps and no code: set the two variables, point PUBLIC_URL at the new host, and add the matching callback under Clients in the Google Auth Platform. Production already read its pair from the stack and is unchanged. The QA variable documentation at the top of the file gains an entry, matching the style of the others. Verified: both files still parse as YAML, both substitutions resolve to the intended stack variables, and the compose environment guard passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
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
|
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
|
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
|
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
|
7b4fbbb9e3 |
build(intake): give the containers an INTAKE_ACTION_SECRET (#224)
A gap in the plan rather than in the code: nothing wired the secret into either stack, so the feature would have shipped with its signed links permanently disabled and nothing saying why. Both compose files now interpolate it, with `:-` so an unset variable stays empty rather than failing the deploy. QA takes QA_INTAKE_ACTION_SECRET, its own value rather than production's, for the same reason as every other QA_ prefixed credential — and more sharply here, because a link signed with it acts on a draft without a login. Rotating the secret revokes every outstanding link, which is the intended answer to one leaking. The cutover doc counted fourteen interpolated names and now counts fifteen. That document says it is checked against the file rather than from memory, so it was: fifteen in the compose file, the same fifteen listed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
cbb0090579 |
build(qa): take the SMTP host, port and TLS flag from QA_ stack variables (#258)
QA_SMTP_HOST, QA_SMTP_PORT and QA_SMTP_SECURE were set on the stack but nothing read them: the compose file hardcoded all three, so changing one in Portainer had no effect and gave no sign of that. They now interpolate like the credentials beside them. Each keeps its Brevo value as a default rather than being left to fall through. The mailer's own fallbacks are Gmail's — smtp.gmail.com, 465, implicit TLS — and Brevo is STARTTLS on 587, so an unset variable with no default here would quietly aim QA at Gmail and fail at send time rather than at boot. `:-` supplies the default only when the variable is unset or empty, so setting one still wins. Verified with `docker compose config` both ways: unset resolves to smtp-relay.brevo.com/587/false, and set resolves to the supplied values. The #107 compose guard still passes. Production is deliberately untouched. It hardcodes the same three and nobody has asked to vary them there, and a needless change to the production stack is not worth the deploy. Closes #258 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalamb
|
2424305d43 |
build(intake): give the container an ANTHROPIC_API_KEY (#223)
The drafting worker needs a credential, and a Portainer stack variable alone does not reach the container — stack variables are interpolated into the compose file as ${VAR}, and a service receives exactly what its own environment block lists. That is how UPLOADS_DIR went missing in #118, and both files say so; this adds the line that makes the variable actually arrive.
QA takes it from QA_ANTHROPIC_API_KEY, prefixed like the database and SMTP credentials so production's key cannot be pasted there and silently work. It is also worth a key of its own rather than sharing production's, because this is the only credential in either stack that spends money per call, on a path anybody holding an upload link can trigger.
Absent is a working configuration in both, deliberately, which is why prod's line carries `:-` and neither variable joins the always-required list. A submission still arrives, keeps its photos and waits undrafted. Losing somebody's consignment to an expired key would be far worse than an item arriving without its description written, and the photos may be the only copy of something no longer in the sender's hands. USPS is the existing precedent for a credential whose absence degrades rather than fails.
The comments say plainly that a spend limit belongs on the key in the Anthropic console, since nothing in this repository can enforce one and #227's submission ceiling bounds the volume rather than the bill.
docs/ops/production-stack-cutover.md said the compose file interpolates thirteen names and listed them. It now says fourteen, because that document stakes its usefulness on being checked against the file rather than written from memory — a cutover working from a stale list is how a variable gets left behind, which is the failure the document exists to prevent. Counted from the file: exactly fourteen.
composeEnvironment.test.ts passes, 24 tests. It checks that every deployment sets what the validator requires, so adding a variable ahead of a validator entry cannot break it — the entry itself comes with the worker.
Ref #223
|
||
|
bermudalamb
|
0710cccac1 |
fix(qa): give the container the environment variables it now requires (#107)
QA refuses to start: "UPLOADS_DIR is required and is not set." #64 made UPLOADS_DIR always required, on the reasoning that its fallback of /app/uploads is correct inside the container and wrong everywhere else, so nothing should inherit it silently. That reasoning stands — but docker-compose.qa.yml had never set it either. QA was relying on exactly the fallback that change set out to stop people relying on, so the first deploy after it merged is the first one to fail. That is an incomplete check, and one that looked confident. #64 verified both CI workflows set every always-required variable and said so. CI is not what deploys. The compose file, which is, was never opened. ADMIN_GATE_SECRET was missing for a different reason, and the container reported it unset even after it was added to the Portainer stack. That is not a mistake, it is how compose works: stack variables are substituted into this file as ${VAR}, not handed to the container. A service receives exactly what its environment block lists. #87 already wrote that down; this is the first time it has bitten. The two get different treatment for a reason. UPLOADS_DIR is hardcoded because it is not a secret and because it has to match the right-hand side of the volume mapping — splitting one value across two places in the same file is how they drift apart. ADMIN_GATE_SECRET is interpolated from the stack so the secret itself never enters the repository, and the header comment now lists it among the required stack variables. Verified by feeding the environment docker compose config actually renders into validateEnv, the same function that was rejecting it: zero errors and zero warnings, the absence of warnings confirming the admin gate is now configured rather than merely quiet. Production runs from a stack outside this repository with the same history and will refuse to boot on its next rebuild unless UPLOADS_DIR is set there first. This commit does not fix that. Closes #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
0c90e18205 |
feat: let QA send real email, guarded by a recipient allowlist (#87)
QA has never been able to send mail. The compose file set no SMTP variables and the mailer skips sending when it finds none, which was deliberate — a QA run must not be able to email a real customer if a fixture ever holds a real address. The cost is that four customer-facing flows have never been exercised anywhere but production: verification, password reset, favorite-sold alerts, and the cart-reminder cron that already has a known silent failure mode. MAIL_ALLOWLIST replaces the blanket mute. Unset means unrestricted, which is production and must stay so. Set means only matching recipients are delivered to; anything else is skipped with a [mail-blocked] warning naming the address and subject. An entry is either a full address, which also covers its plus-suffixed variants, or @domain for every mailbox there — plus-addressing is how these tests get written, and nobody should have to edit an allowlist to invent a new suffix mid-run. The guard sits in the mailer, not at the four call sites, so every sender is covered by construction and a fifth added later cannot bypass it by forgetting. It skips rather than throws: three callers already swallow send failures into a log, so throwing would mostly be caught anyway while risking a 500 on the signup path. The flow under test finishes and the log says why no mail arrived, which is exactly what was missing when QA was simply muted. Two details are load-bearing enough to state. Comparison is exact equality on both halves of the address rather than a suffix test, so a lookalike domain ending in an allowed one cannot get through — there is a test for that specifically. And a present-but-empty value refuses everyone rather than allowing everyone: writing MAIL_ALLOWLIST= expresses an intent to restrict, and reading it as "no restriction" would turn a typo into an outbound mail incident. This inverts the failure mode, so the allowlist is hardcoded in docker-compose.qa.yml rather than read from a stack variable. The safety property must not depend on remembering to set something in Portainer, where an omission would mean unrestricted sending from an environment full of fixtures. The comment says removing the line disables the restriction rather than the mail. QA points at Brevo, reusing the existing account rather than a separate QA sender — a deliberate choice that puts QA volume behind production's sending reputation and quota, acceptable for now. Host, port and secure are pinned in the compose because the mailer's fallbacks are Gmail's and Brevo needs 587 with STARTTLS; that mismatch fails at send time rather than at boot, which is #64's territory. Verified: 12 new unit tests on the matching function, which is where a mistake would actually be dangerous — 98 unit and 144 integration passing, lint 0 errors and 8 warnings unchanged, and the compose renders the expected values under docker compose config. Refs #87 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
71cbd142c3 |
fix: address the final review of the error boundaries (#62)
Five findings from a whole-branch review, two of which mattered. The catalogue boundary could not be recovered by the very controls it exists to keep alive. When the item grid threw, the header, filter chips and Clear filters stayed mounted — that was the point — but clicking Clear filters only changed the URL. A boundary does not reset on a client-side state change, so the fallback went on rendering over a catalogue that would by then have loaded perfectly well, and the only offered action reloaded the same failing URL. The shop read as permanently broken, which is the failure this whole change exists to prevent, reintroduced one level down. ErrorBoundary now takes an optional resetKey and clears itself when it changes; the catalogue boundary passes the filter key. The page and modal boundaries deliberately do not take one, because their escapes are hard navigations that remount the tree already — recorded on the prop so nobody completes the pattern by symmetry. The client-error endpoint could fill the disk. It is unauthenticated, each accepted report wrote about 8.7 KB, and Docker's default json-file driver has no size cap — so the rate limiter bounded a render loop, as its comment claims, but not a few hundred cheap source addresses. Stack and component stack now truncate at 1000 rather than 4000, which is still around fifteen frames and cuts the worst case to under 3 KB, and the QA compose file caps and rotates the log. Production is a Portainer stack outside this repository and needs the same option applied there; noted in the design doc rather than left implied. Three smaller things. A falsy thrown value defeated the boundary entirely: throw null is legal, and branching on the error object alone treated it as no error, re-rendered the children, threw again, and would have taken the root down — a blank page, the one outcome this is all here to avoid. The boundary now tracks hasError separately and synthesises a real Error for non-Error throws. The modal fallback announced its title twice to a screen reader, once as the dialog's name and once as the heading inside it, so the redundant dialog title is gone. And the design doc claimed the development-only detail shows the component stack when it only ever showed the message; corrected, with a note that the stack still reaches the server log, which is where it is useful. Verified after all five: backend lint 0 errors, 144 integration tests, frontend lint 0 errors and 31 warnings, 87 end-to-end tests, all against a freshly created database. Refs #62 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5a9ecefeba |
ci: build QA in Portainer from a git stack, drop the build workflow
The QA image is now built by Portainer from this repository rather than by Gitea Actions. Deployed as a Git repository stack, "Pull and redeploy" pulls the repo, builds from the Dockerfile, and recreates the containers in one action. This removes the runner from the loop entirely. Three dispatches failed without ever building: the runner refuses privileged containers, so the dind service was never created. Working around that needed either the host Docker socket mounted into the runner or privileged containers enabled runner-wide, and both hand every workflow on every branch root-equivalent control of the NAS, production included. Portainer already holds the socket — that is how it manages containers — so building there needs no new privilege at all. pull_policy: build is what keeps it honest. Without it the stack reuses whatever is tagged redefined-designs:qa, which is exactly how a redeploy appears to succeed while still serving old code — a failure this project has already hit twice. Deleting qa-build.yml also drops the registry, the REGISTRY_TOKEN and BREVO_API_KEY secrets, and the notification email. The email existed because CI worked asynchronously and had to tell you when it finished; redeploying from Portainer is synchronous, so the browser already does. Losing the per-commit image tags is a real cost — rollback becomes "rebuild from the ref you want" rather than retagging a specific build. README changes for this are deliberately not in this commit: that file also carries uncommitted work of Thom's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2a20c0e05b |
ci: manual workflow to build and publish the QA image (#25)
Adds a workflow_dispatch job that builds a chosen ref, pushes it to the Gitea container registry as :qa plus a commit-sha tag, and emails when it is ready. It deliberately does not restart the QA stack — redeploying stays a human action in Portainer. The build runs against a Docker-in-Docker service rather than the NAS's Docker socket. Mounting the host socket into the runner would give every workflow on every branch root-equivalent control of the NAS, production included; pushing to a registry means the image does not need to survive in the build daemon. The QA stack now pulls that image instead of requiring a local build. The previous arrangement meant the image existed only if someone remembered to build it, which produced two confusing failures already: a Docker Hub "pull access denied" when the tag was missing, and a silent stale-image deploy when the build had not been rerun. Two runner capabilities cannot be verified from here — privileged service containers for dind, and a docker CLI in the runner image. The workflow checks both and fails with an explanation rather than a connection refused, and validates all five required secrets and variables up front rather than part-way through a build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c01e434747 |
fix(qa): build the QA image before deploying the stack (#25)
Deploying the stack first failed with "pull access denied for redefined-designs, repository does not exist or may require docker login". The README had the Portainer stack creation ahead of the build, so no local redefined-designs:qa image existed and Compose fell back to pulling from Docker Hub, where the repository does not exist. The build step now comes first in the one-time setup, and the review workflow says to rebuild before restarting the stack — the stack builds nothing itself and would otherwise run whatever was last tagged :qa. pull_policy: never makes the failure legible: a missing local image now reports itself as missing rather than as a registry authentication problem. Noted as safe to remove if the NAS's Compose ever rejects the key, since it only affects the error message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
47f00a3485 |
feat(qa): add the QA environment stack definition (#25)
A disposable stack for reviewing merged-but-undeployed changes online, running alongside production on the same NAS. Every value that could collide with production is changed: container names, host port (32751), volume paths, database name, and image tag. The Postgres data directory is the critical one — sharing production's would mean QA writing into production's database files. QA deliberately differs from production in three ways. DEMO_MODE=true with no PayPal credentials, so the full cart and checkout path is exercisable with no route to live PayPal. No SMTP configuration, so a QA run cannot email anyone; the mailer already degrades gracefully when unset. And restart: "no", so a NAS reboot does not quietly bring QA back up and leave it running. The stack must be named redefined-designs-qa in Portainer: the stack name becomes the compose project name, and reusing production's would make compose reconcile the two against each other and remove the production containers. README covers the one-time NAS directory setup, the review workflow, and how to reset QA data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |