1c8763f30f48aa0ea247cdd1edc1eed2cd1f711d
83
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
1c8763f30f |
feat(passkeys): list, add and revoke from the account page (#40)
The issue calls this the smallest one in the project and the one that makes the rest usable, and that is right: registering a passkey with no way to see or remove it is worse than not offering passkeys at all. Revocation is the row going away. #39 looks a credential up by id on every sign-in, so a deleted one is refused immediately and by construction rather than by a flag something has to remember to check. The delete is scoped to the signed-in customer in the same statement that removes the row, because a credential id is not a secret and the WHERE clause is the only thing making this safe. Reading first and deleting after would leave a window. Both "no such credential" and "not yours" answer 404. The second is the interesting case, and saying so would confirm that some other customer holds that id. The lockout check is written even though it cannot fire. password_hash is NOT NULL, so every customer has a password and removing every passkey still leaves a way in. The issue asks for the check anyway and that is the right call, because it is written against the condition rather than against today's schema — it starts holding on its own the moment the condition changes. #332 is what changes it: social sign-in makes password_hash nullable and creates the first customers with no password, and at that point this branch starts running for real. The list returns name, added and last used, and nothing else. No public key, no credential id, no counter — the customer cannot act on any of them, and the credential id is the one value that identifies an authenticator to anyone holding it. Last used is what actually tells two entries apart when the names are similar: someone about to revoke one needs to know which device they are cutting off, and a creation date does not answer that. The whole ceremony lives in customerApi rather than the component, because it is one operation: options from the server, an attestation from the browser, verification back at the server. A component holding that intermediate state could leave a challenge issued and never answered. Dismissing the browser's prompt rejects, and that is a cancellation rather than a failure. Reporting it as an error would tell a customer something went wrong when they simply changed their mind, so NotAllowedError and AbortError are swallowed and everything else is shown. The server's message on a refused revoke is shown as it arrives too — it says what to do about the last way in, and a generic message would strand the customer on a button that just does not work. The section renders nothing where WebAuthn is unavailable, rather than offering a button that cannot work. Same rule #41 applies to the login form. Verified: tsc clean for src and tests in both workspaces, backend lint at the seven pre-existing warnings with none added, frontend lint clean, 521 unit tests across 36 suites, frontend build green. Not verified: the registration ceremony needs a browser and a real authenticator, which per #41 is a standing limitation of this feature. What CI can prove is the list and revoke endpoints, which are ordinary routes. Closes #40 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
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
|
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
|
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
|
306db81966 |
fix(db): narrow the status column instead of asserting the row (#308)
`.$castTo<AdminItemRow>()` / `.$castTo<PublicItemRow>()` at the two list-query call sites replaced the entire result type with an assertion rather than narrowing the one column that actually disagreed, which meant a projection that silently lost a column would still compile — exactly the failure mode this task exists to close, and the opposite of what the commit body claims. Fixed at the source instead: `itemSelect.ts` now defines `ItemsWithStatus`/`ItemDB`, narrowing `items.status` from the schema mirror's `Generated<string>` (a CHECK-constrained text column, so `kysely-codegen` has no literal union to give it) to `Generated<ItemStatus>`, and builds `ItemContext`, `adminItemQuery()` and `publicItemQuery()` from an `itemDb` typed with `ItemDB` instead of `db`/`DB`. Both `$castTo` calls and their comments are gone; the `AdminItemRow[]` / `PublicItemRow[]` annotations at the two call sites now check for real. Verified by temporarily dropping a column from `adminItemQuery`'s projection: the `AdminItemRow[]` assignment failed to compile as expected, confirming the guarantee actually holds. Also corrected two now-false statements left over from the conversion: `db-kysely/CONVENTIONS.md`'s worked-example section said `buildItemFilterSql` was "still raw `pg`" and that converting it "would put a second copy of a live function in `src/` that nothing calls" — both untrue since #308 shipped it as `itemFilterExpressions`. And two doc comments in `itemSelect.ts` still named the deleted `PUBLIC_ITEM_SELECT`/`ADMIN_ITEM_SELECT` constants instead of the functions that replaced them. 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
|
c71b11e05e |
refactor(db): swap the query builder from Drizzle to Kysely (#305)
One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder. The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing. CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to. The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone. isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile. Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3891f4fd75 |
feat(admin): endpoints to rotate one photo of an item (#301)
Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all. A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another. 204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204. Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written. One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. 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
|
8f35204995 |
feat(admin): remove or restore every background on an item (#293)
Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them. Both answer 200 once the id is valid, even when the sidecar fails, and that is a deliberate departure from the per-photo endpoints in #281. Those act on one image, so the request either worked or it did not and 502 says which. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id. No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background. The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade. Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
71efe6ee6a |
chore(sonarqube): keep interpolation out of query call sites (#294)
The quality gate was red on one condition only — new_security_hotspots_reviewed at 75 against a threshold of 100 — and the outstanding hotspot was admin.ts's `${ADMIN_ITEM_SELECT} WHERE i.id = $1`.
Worth being exact about what was wrong with it, because it was not what it looked like. The value was already parameterized: itemId was bound as $1, travelled through the driver's separate parameter channel, and never entered the query text. What was interpolated was a module constant containing no caller data. S2077 fires on the template literal rather than on the value, because the rule cannot tell a constant from a request field — and neither, at a glance, can a person reading it.
So the fix is not to parameterize something already parameterized. It is to stop interpolating at query call sites at all, which turns a property somebody has to verify into one they can see. Every query whose shape is fixed is now a named constant and every such call passes an identifier: ADMIN_ITEM_BY_ID for the two admin routes, PUBLIC_ITEM_BY_ID, LINK_LIST, and the two draft-queue shapes. Seven interpolating call sites become three.
The three that remain cannot become constants and now say so rather than looking like ones nobody got to. admin.ts and items.ts build their WHERE at run time from buildItemFilterSql, whose fragments are string literals whose only interpolations are placeholder indices; that reasoning was already written down and is unchanged. draftingWorker interpolates a table name, and this is the one query here that genuinely cannot be parameterized in any form — a bound parameter is a value, and Postgres will not accept an identifier as one, so the choice is interpolation or nothing. What makes it safe is the closed 'categories' | 'tags' union, and the comment now says that instead of merely asserting there is nothing to worry about.
Also clears the project's only open Sonar issue, S1854 on adminUploadLinks, which #260 introduced and which I had deferred as a tidiness point. It was more than that: outcome was initialised at its declaration and assigned the same value again in the catch, which made two different failures look like one. A template that will not render, or a stored template that cannot be loaded, is not an SMTP problem, and reporting it as "not configured" sent the admin looking in the wrong place. The SMTP-rejection conflation that was actually agreed stays, and is now the only thing that catch conflates.
Closes #294
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
4e7f255782 |
fix(admin): return contact_email from the revoke query (#260)
The revoke route's RETURNING clause omitted contact_email while its result was typed as UploadLinkRow, which declares the field as present. No user-visible effect since the response was never checked for it, but the type asserted something the query did not actually return. Added the column so the two agree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
b18b3e3a3d |
feat(intake): require an address for an upload link and send the link to it (#260)
Creating a link now requires a valid email address and mails the link to it, which is the whole point: getting a link to a contributor was previously a copy-and-paste into whatever the admin happened to use. The send is awaited and its outcome reported, unlike every other sender in this codebase, which fires and forgets because nobody is waiting on the answer. Here somebody is. The admin is looking at the screen, and whether they now have to send the link by hand is exactly the thing they need to know — and QA blocks delivery to any address outside MAIL_ALLOWLIST by design, so a link that was never emailed would otherwise look precisely like one that was. A send that could not happen does not roll the link back. The token is displayed exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that had succeeded. They end up with a usable link and an honest statement about delivery instead. One inaccuracy left deliberately: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome of its own. The distinction is real but nothing consumes it, and the admin's next action is identical either way. Also updates the other integration tests that created a link with only a label, since an address is now required, and adds the uploadLink template key that GET /api/admin/email-templates was missing from its list — an omission left by the template's addition in the prior commit on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
be6800181d |
fix(admin): let a setting whose default is empty actually be cleared (#280)
intakeNotifyEmail and intakeCeilingResetAt both document empty as their default and as a working configuration — no notification address, and no ceiling reset recorded. The validator refused every empty text value, so either could be set and then never removed through the admin at all; the only way back was a DELETE against admin_settings. An admin who turned intake notifications on could not turn them off. Whether empty is a mistake is a fact about the setting rather than about its type, so it is now declared on the setting, in the DEFINITIONS row that already carries its type and fallback. A new setting states it once, in the place someone adding one is already editing, and nothing else has to know. That is what makes this different from special-casing two names in the validator, which would have left the next such setting to rediscover the same bug. The blanket refusal stays the default, because for a setting with a non-empty fallback an empty value really is a mistake: an empty greeting format renders every greeting as nothing at all, which reads as a broken email rather than as something a person cleared. Both those cases keep their tests. Whitespace is normalised to empty rather than stored. Somebody clearing a field they cannot see the end of leaves spaces behind, and they meant cleared. The tests check that the clearing survives the request rather than only being echoed back — the last one sets a value, clears it, and then reads it again through GET, which is the assertion that would have caught this had it existed. Closes #280 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d5e599a30e |
fix(admin): answer 404 for an unreadable id instead of 500 (#207)
The issue asked for two things and only one had been done. A well-formed but absent id already answered 404 — the PUT route carries a comment saying so. A malformed one still reached Postgres as text, raised 22P02 on an integer column, and surfaced through the route's catch as a 500, telling the admin the server had broken when the truth is that no such item can exist. That half is now closed everywhere rather than on the three routes that happened to have it. Guarded: DELETE an item, DELETE an image, unpublish, and every route in adminItemDrafts — publish, regenerate, discard, restore, and the two background-removal endpoints added by #281. The last of those were flagged in that feature's own final review as sharing this pre-existing shape, so they are fixed with the rest rather than left to be found again. Routes carrying two ids guard both. A route can guard the first and forget the second, and the forgotten one fails exactly as loudly, so there is a case each way for both image endpoints and for DELETE image. DELETE deliberately still answers 204 for a well-formed id that is absent. The method is idempotent and the caller's intent, that the item should not exist, is satisfied either way; what must not happen is a 500. There is a test pinning that so the distinction is a decision rather than an omission. Also replaced the raw req.params.id and Number(req.params.id) uses that sat inside routes which had already computed a validated id. They were safe, because the guard above them made them safe, but a validated id and a raw one side by side in the same handler is how this bug comes back. The test block named "a non-numeric id on every admin item route" covered two routes. It now covers every route that takes an id, which is what makes its name true. Closes #207 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
|
b06eac4640 |
fix(intake): clear remove_background when an admin restores a photo (#281)
item_drafts.remove_background is written once at intake and never updated afterward. Restoring a photo clears original_image_path, which is exactly what makes the row look "never cut out" to removeImageBackground — so a submitter's ticked checkbox, followed by the worker cutting the photo out, followed by an admin restoring a poor result, followed by a click on Regenerate, would silently re-cut the same photo the admin had just put back. Nothing was lost, but the control the design calls "what makes a poor result survivable" was quietly defeated by the button sitting next to it. restoreImageOriginal now swaps the image's paths back and clears item_drafts.remove_background for that item in one transaction, so a restore that succeeds while the flag update fails cannot reintroduce the bug. An admin restoring any photo on an item is treated as overriding the submitter's original request for the whole item — the flag is per-item while the swap is per-photo, so there is no narrower place to record the decision, and turning off the whole item's auto-removal is the conservative direction: the alternative is re-cutting something a person deliberately undid. Added an integration test in draftingBackgroundRemoval.integration.test.ts that drafts a submission with the intent set, cuts it out, restores it, mirrors what the admin's Regenerate button does (state back to queued, attempts cleared), runs the worker again, and asserts the photo is still not cut out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
984e329c47 |
fix(admin): narrow the restore-original catch to the race it exists for (#281)
Round 1 caught every throw from restoreImageOriginal() and reported it as a 404, on the theory that losing the concurrent-restore race is the only way that call fails. But the throw carried nothing to distinguish that race from a genuinely different failure during the same UPDATE — a dropped database connection, a transient outage — so a real failure was now silently reinterpreted as "someone already restored this" instead of surfacing as the loud 500 it was before. backend/src/intake/backgroundRemoval.ts now exports NoOriginalToRestoreError, a named subclass of Error thrown in place of the bare Error restoreImageOriginal previously threw. The message text is unchanged, so backgroundRemoval.integration.test.ts's rejects.toThrow(/no original/) assertion keeps passing without modification. backend/src/routes/adminItemDrafts.ts catches that class specifically in the restore-original handler and rethrows anything else, so a real failure still reaches the app-level error handler and comes back as a 500 instead of being mislabeled as "already done". backend/tests/integration/adminItemDrafts.integration.test.ts adds a test that spies on restoreImageOriginal via jest.spyOn on the module namespace (the project compiles to CommonJS, so the route's call site reads the export off that object at call time, which makes the spy effective without jest.mock) to reject once with a plain Error, and asserts the response is 500 rather than 404 — proving the narrowing changes real behavior, not just internal structure. The spy is restored in a finally block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5129112266 |
fix(admin): answer 404, not 500, when two restores race (#281)
restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct. Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
9ced34ad19 |
feat(admin): remove and restore a photo's background per image (#281)
Adds two admin-gated endpoints on the review queue router: POST /:itemId/images/:imageId/remove-background and POST /:itemId/images/:imageId/restore-original. Both run synchronously and reuse the same backgroundRemoval module the drafting worker uses, so a cut-out obtained either way is identical and either can be undone by Restore.
Ownership is scoped by item as well as by image (imageOfItem selects on id AND item_id), because the image id is a serial and guessing one is easy — a photo belonging to a different submission must not be reachable through another item's URL. A sidecar failure returns 502, not 500, and leaves the row untouched, since removeImageBackground only writes the row after the cut-out file already exists on disk.
GET /api/admin/item-drafts now returns { drafts, backgroundRemoval } instead of { drafts }, and each image in the payload gains original_image_path, which is what the review queue UI will use to decide between "Remove background" and "Restore original". DRAFT_SELECT's images aggregate is extended accordingly, keeping the deliberate column spelling that guards against the upload_links token digest leaking into the response.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
885a78c572 |
feat(intake): record whether the submitter asked for a cut-out (#281)
The public intake POST now reads a `removeBackground` multipart field and stores it on the new `item_drafts.remove_background` column. The checkbox on the submission page is ticked by default, so a client that sends nothing gets `true` — only the exact string `'false'` opts out, so a stray or unexpected value is treated as consent rather than a silent refusal. The GET now also reports `backgroundRemoval: isRembgConfigured()` alongside the label, so the submission page knows up front whether the feature exists in this environment at all. Neither handler calls the sidecar or the AI — this task only records intent for the drafting worker to act on later, and the existing ordering of `requireUsableLink` and `requireCapacity` ahead of `uploadImages` is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
cd2676233c |
chore: clear the six code smells SonarQube reported (#181)
The list finally arrived from the reporting added earlier, and confirmed what #181 could only suspect: these are not the five eslint-plugin-sonarjs warnings that issue lists. Those were fixed under #261 and the count staying at five was a coincidence. It is six now, 25 minutes of debt, and one of them was mine. admin.ts imported '../utils' twice — I added readId in #207 without noticing the file already imported from there. One import now. filters.ts had a redundant `as ItemStatus[]`. TypeScript narrows an array through `.every()` with a type predicate from 5.5, and this project is on 5.9, so the assertion stopped telling the compiler anything. Removed, and the build confirms the narrowing holds without it. adminSettings.ts was the only CRITICAL: cognitive complexity 18 against a limit of 15, almost all of it three near-identical loops differing only in how they validated. Each validation is now a small pure reader returning a refusal rather than sending one, and the handler is one loop over a table. Adding a setting type means adding a row. That refactor is deliberately behaviour-preserving. Two things were left alone on purpose: the blanket rejection of empty text, which is wrong for the two settings whose documented default is empty and is filed as #280 rather than folded in where it would be invisible; and the absence of the `count` settings, which no caller submits and which the admin screen has no control for. I had started adding count validation and reverted it — widening behaviour under cover of a complexity fix is how a refactor stops being reviewable. The three S6478s are render props, not components defined during render. ErrorBoundary's `fallback` is typed `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`, so React only ever sees returned elements and never a new component type — the subtree destruction the rule describes does not happen, and the rule's own message offers `allowAsProps` for this shape, which cannot be set from here. Hoisted rather than suppressed because none of them closes over anything local, so at module level each is one stable function instead of a new closure per render. That is a mild improvement, not a contortion. Verified: 402 backend unit, 358 backend integration, 30 frontend unit, 157 e2e, both lints clean, both builds clean. The e2e run matters most here — storefront-errors.spec.ts exercises all three hoisted fallbacks, and it was run on its own first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
baa5bb4fdf |
fix(intake): wake the worker when a draft is regenerated (#272)
Regenerate set state='queued', cleared attempts and ai_error, and answered 200 — then nothing ran the worker, so the row sat until the five-minute sweeper happened along. From the admin's side that is indistinguishable from a dead button, and the obvious response is to press it again. The submission path has kicked the worker since #223. Both paths put a row into 'queued'; only one asked for it to be drafted. That was an oversight in #225 rather than a decision: the kick was added to the submission path in a later task and the action routes were never revisited. Fixed in both places that re-queue — the admin route and the signed regenerate link from the notification email, which sets the identical state. Fire and forget with a logged catch, exactly as the submission path does: a slow or failing model call must not become a failed request for the admin, and the sweeper is still the backstop if the kick misses. The card still does not update itself once the draft lands, since drafting takes a few seconds and the screen has no way to know it finished. That is a UI question rather than this bug, and is noted on #272. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
58d5925466 |
feat(db): convert routes/adminCategories.ts to Drizzle (#218)
All nine sites, chosen because the file is awkward rather than easy: a recursive CTE consumed two ways, a correlated subquery, an array match, and two error paths keyed on a Postgres SQLSTATE. A file of plain CRUD would have produced a flattering number that does not generalise.
The hand-declared row interfaces are gone. CATEGORY_COLUMNS is written once and the row type is inferred from it, which closes the drift itemSelect.ts documents as "KEPT IN STEP BY HAND". That mapping has to be explicit rather than selecting the table: the mirror names columns in camelCase and this API answers in snake_case, so selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test asserting status codes would have caught it.
strict and noUncheckedIndexedAccess hold with no non-null assertions added. requireRow covers the RETURNING rows and the existing lookup destructures and branches, exactly as before.
Two bugs were introduced and caught by the integration suite, and both are worth recording because neither produced a type error.
Drizzle renders a column reference inside a `sql` template UNQUALIFIED. `${items.categoryId} = ${categories.id}` became `WHERE "category_id" = "id"`, which Postgres resolved against items on both sides — so the item count came back plausible and wrong rather than failing. That is worse than the documented array trap, which at least produces invalid SQL. The fragment is now literal text, which is honest since it binds no values.
And the driver's error code moved. Drizzle wraps errors, so the SQLSTATE that sat on err.code now sits on err.cause.code; the old check compiled, never matched, and turned two 409s into 500s. isUniqueViolation accepts both shapes.
inArray replaced the ANY(...::int[]) match and sidesteps the sql.param trap entirely — there is no template to forget it in, and the builder emits the placeholder list correctly by construction.
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
|
92c04847d6 |
feat(intake): refuse submissions past the daily ceiling, and reset it from the admin (#227)
The check is ordered ahead of uploadImages, for the same reason requireUsableLink is: a refused submission must write zero bytes. Ordering it after would accept the upload, store the files and then throw them away, which is the expensive half of the work the ceiling exists to prevent. A test asserts nothing is stored. 503, not 403. The sender has done nothing wrong, their link is fine, and the condition clears by itself as the window rolls — so the link stays usable and works again the moment there is room. The ceiling never touches the admin upload path, which has its own test. Intake being throttled is an inconvenience; the shop being unable to add its own stock is an outage. reset-ceiling is declared above /:id/revoke because Express matches in order and would otherwise read it as an id and try to revoke a link named "reset-ceiling". That has its own test too. The per-link alert is a named function rather than the inline IIFE the plan wrote. routesAreWrapped.test.ts flags any async inside a route registration not directly wrapped in asyncRoute, and it cannot tell an inner IIFE from an unwrapped handler — nor should it have to. The guard caught this, and the extraction reads better than what it rejected. Backend now 373 unit and 337 integration, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
346e9eae4c |
feat(intake): act on a signed link from the notification (#224)
Mounted publicly, deliberately not behind requireAdminGate. These are clicked from an inbox by someone who is not signed in, which is the whole point; the signature is what protects them. GET confirms and changes nothing, POST acts. Mail scanners and corporate link-rewriting gateways issue a GET against every URL in a message before a human sees it, so a GET that discarded a draft would fire itself on delivery — carrying a valid signature, looking entirely legitimate in the log, and nobody would know to go and recover it. That is the case the split exists for and it has its own test. Forged, replayed, upgraded and expired links are each refused with the same 403. Distinguishing them would tell somebody probing which of those they had achieved. There is no signable publish, and asking for one finds no handler. The two registry guard tests are updated rather than worked around: they assert the full set of settings and template keys, so adding either is exactly what should trip them. Backend now 367 unit and 329 integration, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d8a7588685 |
feat(intake): regenerate, discard and restore a draft (#225)
Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that has already failed three times without clearing them would produce a button that appears to work, does nothing, and leaves nothing anywhere to say why. Discard deletes nothing — not the item, not the photographs. It is one click away in what amounts to an inbox, and the photos are often the only copy of something no longer in the sender's hands, so the destructive reading of the word is deliberately not available here. The item returns to pending, because a discarded submission must not stay on sale. Restore returns a draft at the state its own contents justify rather than unconditionally ready. A submission discarded before it was ever drafted has no copy, and coming back as ready would present an empty draft as a finished one. Judged on whether a name was ever written, because the state held before discarding is not stored. Backend now at 346 unit and 317 integration tests, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
44a9037121 |
feat(intake): publish a reviewed item to the storefront (#225)
The only path from an intake submission to the storefront. It performs what mark-available performs — the status, and clearing the sale and reservation fields — rather than calling that route, because the copy and the publish have to be one transaction: an item published carrying the previous draft's name would be worse than one not published at all. The price rule is applied here rather than trusted from the client. A changed number becomes the admin's; an unchanged one keeps whatever it was, so publishing without touching the field records that nobody chose it. The row is locked for the transaction so two admins publishing the same submission cannot interleave one's price decision with another's name. Whole cents only. A fractional value would round somewhere nobody is looking and put the item on sale at a price no one entered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
81886f84c2 |
feat(intake): list submitted items waiting for review (#225)
Columns are spelled out rather than selected with a wildcard, so a column added to item_drafts later does not silently start reaching the browser. That matters most for the join to upload_links, which carries the token digest — only the label is taken, and a test asserts the digest never appears in a response. Discarded rows are excluded by default rather than deleted. Discard has to be recoverable because it is one click away in what amounts to an inbox, but a discarded row left in the default view would compete for attention with work that still needs doing. The gate goes on the mount in app.ts rather than inside the router, matching every other admin router. Since ADMIN_GATE_SECRET is unset for integration runs the gate is disabled there, so the test that asserts the mount is actually gated sets the secret for its own duration — leaving requireAdminGate off a new mount is otherwise a silent hole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
fe28c97e0f |
chore(sonar): remove the rejected Tinqer spike, clear the lint debt, and report measures in CI (#261)
The standing cleanup, three features behind. Four changes. Report the measures in CI. This is the one that matters, because the rest was only findable by reading the tree. SonarQube here is 9.9 Community: no Bearer auth, so the official MCP cannot connect, and the host is a CI secret, so hotspots, duplication, debt and coverage existed only on a dashboard — which made "reduce the debt" an instruction nobody could act on without a browser open beside them. scripts/summarize-sonar.js queries the measures API with the secrets the workflow already holds and prints the result into the job log. The scanner masks the URL and token; measures are not secret. It polls the compute task before reading. The workflow does not set sonar.qualitygate.wait, so the scan step returns once the report is uploaded and the server computes measures afterwards — reading immediately would return the previous analysis, indistinguishable from this one and quietly wrong. When it cannot confirm, it says so in the output rather than presenting stale numbers as current. It is deliberately not guarded with continue-on-error: it exits 0 on every path, and guarding it would oblige it to appear in the final gate, whose job is to fail the build. Remove the Tinqer spike. #216 evaluated Drizzle against Tinqer and rejected Tinqer, and its closing comment said the throwaway src/db-tinqer/ probe must not reach main. The whole spike commit was merged, so it did. The probe is 71 lines imported by nothing, and @tinqerjs/tinqer, @tinqerjs/pg-promise-adapter and pg-promise were dependencies for a library nobody chose. The condition_note column that warning also named did not reach main. Clear the lint debt, both projects now at zero warnings from six and two. One of these was a real defect rather than tidiness: the third catch block in shippingAddresses.ts rolled back and returned 500 while discarding the error, so a failed default-address change left nothing behind to say why — the two catch blocks above it in the same file already logged, and this one had simply been missed. The Express namespace augmentation is a false positive and is disabled with the reason written beside it, because an interface that must merge into one Express declares inside a namespace has no ES module spelling. Dedupe the extension map. backfillImageReencode.ts kept its own .jpg/.png/.webp table whose comment named uploadTypes.ts as the source of truth, directly above duplicating it. That file rewrites stored images, so the two disagreeing would silently skip files it should re-encode. src/db-drizzle/ deliberately stays. #217 is open to promote exactly those files properly, with tablesFilter and the sql.param() array rule; deleting them here would be doing #217 badly in the wrong issue. Only their unused-symbol warnings are fixed, and if drizzle-kit pull regenerates schema.ts the table warning returns — worth #217 knowing. Hotspots and coverage are untouched because both numbers are still invisible. They are the next pass, once the step above has printed them once. Closes #261 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
e163bb39c6 |
feat(intake): run the drafting worker after a submission and on a sweep (#223)
Two drivers. The call after a successful submission means a draft is usually waiting by the time anybody looks; the five-minute sweep means a restart mid-draft is recoverable rather than a permanently stalled row, and picks up whatever the first call missed. Neither is awaited. A slow or failing model must not become a failed upload for someone who did nothing wrong, which is the whole reason drafting does not happen inline — the cost of a dropped call is a few minutes' delay, not a lost submission. Both catch for themselves. The comment beside the existing schedulers points out that `void` is only safe because those functions handle their own errors, and draftQueued does not: its first query can reject, and an escaping rejection would take the container down. 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> |
||
|
bermudalamb
|
bcecda9122 |
fix(intake): stop a throttled sender being told their link is dead (#222)
Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for. One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it. Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message. Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want. The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions. Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store. The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole. Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings. Ref #222, #241 |
||
|
bermudalamb
|
1fc632598a |
feat(intake): accept photo submissions through a shared link (#222)
The public way in. Photos of one item plus a free-text note, from someone with no account, landing as an `items` row at status 'pending' — already invisible to every public and storefront query since #90, so nothing is live by accident. Every refusal is a 404. Unknown, revoked and exhausted links are indistinguishable from outside, because whether a link exists is not something a stranger needs to be able to learn — the same reasoning uploads.ts applies to files. The link is resolved *before* multer runs, and that ordering is the point rather than an implementation detail. discardUnlessAccepted would delete the files afterwards, but "written then deleted" is materially worse than "never written" on an endpoint the whole internet can reach: it is disk churn an unauthenticated caller controls, and it leans on an unlink that a crash between write and delete would skip. A test asserts the volume is untouched for a bad token, so a future reordering fails loudly instead of quietly handing that control away. The link counter is incremented inside the transaction and guarded on the same conditions as the lookup, so two submissions racing for the last slot of a capped link cannot both succeed. The response carries no item id: the sender has no business knowing about the catalogue and nothing they could do with it. The AI is deliberately not called here. A slow or failing model request must not turn into a failed upload for someone who did nothing wrong, and the photos may be the only copy — the item is often no longer in the sender's hands. The row waits at state 'queued' for #223. The new limiter keys on the caller alone, since a submission carries no email. keyByCallerAndEmail's comment warns that a bare ip bucket is a shared allowance, and that trade is taken knowingly: the link is the per-caller identity and its cap is the per-caller bound, while this limiter does the different job of bounding what one address can throw at an endpoint that writes files. Twenty per fifteen minutes is deliberately looser than the password-reset allowance — somebody photographing a box of stock legitimately submits several in a row, and refusing them costs a consignment. Because the route mounts the shared uploadImages, it inherits the type allowlist, the magic-byte check and #226's EXIF stripping without asking for any of them. A test asserts the stripping specifically, since this is the route where it matters most: the photo comes from a stranger's phone rather than the shop's own camera. Backend: 284 integration (12 new), 309 unit, lint unchanged at 6 pre-existing warnings, build clean. Ref #222 |
||
|
bermudalamb
|
3392f6f10d |
feat(intake): issue and revoke named upload links (#222)
Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature. The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest. An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe. Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and a button that errors on a double-click teaches people to distrust it — which is the last thing wanted on the control that contains a leak. Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts. Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it. Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222 |
||
|
bermudalamb
|
1a5a8b837b |
refactor(uploads): extract the validated image pipeline for a second caller (#222)
A pure move, no behaviour change. #222's public intake endpoint needs the same path from a multipart request to files on the uploads volume that the admin routes use, and the alternative to sharing it is a near-copy that has to reproduce every safety property exactly: the type allowlist, the magic-byte check after the write, names from a CSPRNG rather than from `originalname`, the re-encode that strips EXIF, and the cleanup of whatever a refused request left behind. A copy that drifted on any of those is the gap #95, #103, #180 and #226 exist to close. `stripUploadedImages` moved with the rest, which the plan originally did not say — it was written before #226 added it. Leaving it behind would have given the intake route an upload path that skips EXIF stripping, and no test would have failed to say so, because the intake tests are written against a route that does not exist yet. The refreshed plan added a check with a definite answer, and it now holds: routes/admin.ts no longer imports imageProcessing at all. Every comment came across verbatim. They record why the code is shaped as it is and are the most valuable part of what moved. Lint caught something the compiler did not: MAX_IMAGES_PER_REQUEST was left imported into admin.ts, where its only use — `upload.array('images', MAX_IMAGES_PER_REQUEST)` — had moved away with the middleware. It stays exported from imageUpload for the intake route's caps, but admin.ts does not need it. Verified as a refactor rather than as a change: the four suites that exercise this path hardest were run before the move and after it, 44 tests both times, same suites, same count. Full backend: 263 integration, 302 unit, lint back to its 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
|
aecccef418 |
feat(uploads): strip metadata from every accepted upload (#226)
Hooked into uploadImages rather than into the routes. That middleware is where verifyUploadedImages already runs and is the single choke point every upload path passes through, so the admin create and update routes are both covered and the intake route from #222 will inherit it rather than having to remember. The same reasoning discardUnlessAccepted already gives for being a hook instead of a call. Runs after verification, deliberately: re-encoding a file whose bytes do not match its declared type would be work on something already refused, and sharp's error would replace the clearer message that check produces. A re-encode failure refuses the upload rather than storing the original, because the one case where a photo keeps the coordinates it was taken at should not be the case nobody was told about. The test builds a JPEG carrying GPS tags rather than committing a binary fixture, so what it contains is readable, and it asserts the fixture really carries EXIF before asserting the stored file does not — otherwise the test would pass while proving nothing. GPS tags go in IFD3, which is the GPS IFD as libvips names it; sharp's Exif type has no separate GPS key, and putting them in IFD0 would have produced EXIF without producing the tags this issue is about. Backend suites: 285 unit, 260 integration, lint clean, build clean. One caveat worth recording. Across three full integration runs, `uploadValidation` failed once on "removes the upload when the request is refused for its other fields". It is a pre-existing race rather than a regression: discardUnlessAccepted cleans up in an unawaited `void discardUploads(...)` inside a `res.on('close')` handler, so a test asserting on the directory immediately after the response has always been able to observe the state before the unlink lands. Re-encoding adds enough libvips work to lose that race occasionally where it previously did not. The property still holds in production, where the process keeps running and the unlink completes. Filed separately rather than fixed here. 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> |
||
|
bermudalambandClaude Opus 5
|
41840d4890 |
fix(email): stop a demo purchase telling real customers an item sold (#206)
A demo purchase called `notifyFavoritersOfSale`, which mails everyone who favorited the item through production's configured SMTP: "An item you favorited has been sold to another customer, so it is no longer available… this one will not be restocked." Nobody bought it and nobody is shipping anything, so both halves are false. It is also the only outbound consequence a demo purchase has — everything #195 and #203 fixed is on screen, in front of the person who clicked and who has now been told it is a demo. These recipients never saw the cart. They just get told something they cared about is gone, and while production runs the demo interim (#191) they are real customers on real SMTP. The demo route no longer notifies. The PayPal capture and webhook paths are untouched, because those are sales. The item is still marked `sold`, so the storefront stays truthful about availability and the favoriter who goes looking finds what the database says. Only the claim that somebody bought it goes away. That a demo purchase permanently consumes real production inventory is a larger question than this issue and is left alone. Removing the call broke two tests and quietly hollowed out three more, which is the more interesting half of this change. Five tests in `favorites.integration.test.ts` used the demo purchase as a convenient way to make a sale happen; with the notification gone, the two asserting mail *is* sent failed, and the three asserting it is *not* sent would have passed for the wrong reason for ever. They were always about who gets told rather than about the demo route, so they now call the notifier the way the PayPal routes do — after the purchase, with the sold ids and the buyer. `buyThenNotify` says so at the point of use. Route-level coverage is unaffected: the admin mark-sold path already had its own test, and the new test asserts the demo route notifies nobody. Both halves were mutation-tested rather than assumed. The new test fails without the fix. Dropping the buyer exclusion from `collectFavoriteRecipients` fails "does not tell the buyer their own purchase is unavailable" and "emails every opted-in favoriter except the buyer" — so the restored tests are guarding the logic again rather than passing on an empty inbox. Verified: 255 integration tests pass (the suite needs `--runInBand`; these share one database), 278 unit tests pass, backend build clean. Closes #206 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
bermudalamb
|
61c12fd438 |
refactor: remove the duplicated blocks SonarQube found (#182)
Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182 |
||
|
bermudalamb
|
b616b9f0ab |
fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)
SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one. The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing. That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable. The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads. `verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss. The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it. Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass. Refs #180 |
||
|
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
|
179cbad225 |
refactor(backend): type the remaining query results (#159)
Completes the typing. Every `.query(...)` in backend/src whose rows are read now carries a row type: adminCustomers, adminCategories, shippingAddresses, adminTags, adminEmailTemplates, adminSettings, public, server and the auth middleware. Typed sites go from 49 to 78, and there are no untyped reads left anywhere. Writes and transaction control stay untyped, which is the exemption #159's criteria allow for and the reason is stated in each file: they return nothing anyone reads, and annotating them would bury the ones that matter. The aggregates needed checking rather than guessing, and the answer was not what the shapes suggest. Postgres returns COUNT as bigint and SUM as numeric, and node-postgres hands both back as strings — only an explicit ::int cast arrives as a number. Probed against the real database: COUNT(*) is a string, COUNT(*)::int is a number, SUM() is a string, MAX(timestamptz) is a Date. That makes the admin customer list a mixture. order_count and total_spent_cents are strings; reserved_count, which the query casts, is a number. They are typed as what they are. Which surfaces a mismatch worth knowing about and not fixed here. frontend/src/admin/adminCustomersApi.ts declares both as `number`, and Customers.tsx sorts with `a.order_count - b.order_count` and renders with `(v / 100).toFixed(2)`. Those work, because `-` and `/` coerce a numeric string. The first `+` written against either — a column total, say — will concatenate instead. Nothing is broken today; the types on both sides simply disagree about reality, and one of them is now right. Changing the API to cast would alter the response shape, which is a behaviour change and belongs in its own issue. Two smaller shapes worth a note. shipping_addresses.usps_standardized is jsonb that is only ever handed to the client, so it is `unknown` rather than a guessed object. And `SELECT 1 ... ` used purely for `.length` has no column name of its own — Postgres calls it `?column?` — so it is an index signature with nothing read out of it rather than a fabricated field. Verified: tsc clean, unit 254/254, integration 238/238, backend lint unchanged from main. Closes #159 |
||
|
bermudalamb
|
d43e2d5871 |
refactor(backend): type the admin item queries, and fix the stale status union (#159)
admin.ts has no untyped reads left. Typed sites go from 43 to 49. New ItemRecord in itemSelect.ts for the bare `items` row that `RETURNING *` gives back. Deliberately not AdminItemRow: that describes a select which joins the category and adds images and tags as subqueries, so typing a RETURNING * as it would promise three fields the result does not contain. Three shapes for one table, because three different queries return three different things. The typing found a real defect on its first run, which is the case for doing this at all. `ItemStatus` in types.ts was `'available' | 'reserved' | 'sold'`. The database has four values and defaults to 'pending' — items have arrived pending since #90. itemFilters.ts declared its own copy that had all four and was correct. Two declarations of one union with nothing connecting them: one went stale and nothing said so. It was invisible while query rows were `any`. Typing them turned `if (status === 'pending')` in admin.ts into TS2367, "this comparison appears to be unintentional because the types 'ItemStatus' and '\"pending\"' have no overlap" — a compiler telling us the unpublish route's guard could never be true, against a type that was simply wrong. Confirmed against the database rather than by picking the more plausible of the two declarations: `SELECT DISTINCT status FROM items` returns pending, available, reserved and sold. Fixed by removing the duplication rather than by patching both copies. types.ts now holds the only declaration and itemFilters.ts imports it, re-exporting so its existing importers are unaffected. Patching both would have left the next drift free to happen the same way. Verified: tsc clean, unit 254/254, integration 238/238, and backend lint unchanged — the four warnings it reports are identical to those on main with these changes stashed, so none of them are new. Refs #159 |