Fix/framework mechanism audit - #261
Merged
Merged
Conversation
Found by auditing the repository against AdonisJS/Inertia mechanics read out of node_modules, after a 404 on the product-edit form turned out to be a Laravel idiom (`_method` in the request body) that this framework does not honour. Kiosk endpoints accepted any signed-in account. `middleware.kiosk()` diverts kiosk accounts away from the regular UI — it does not require one — and the target `customerId` arrives in the request body, so anybody could order onto somebody else's account. Adds a `kioskOnly` middleware and re-checks the target is a live account in both purchase actions. The e2e seed turned out to insert `kiosk@localhost` with `is_kiosk = false`, so those tests were exercising a path no real kiosk device takes; it is now flagged correctly. Method spoofing is disabled. Shield decides whether to validate CSRF from `request.method()` and runs after the bodyparser, so a cross-site POST carrying `_method=GET` in its body routed as POST while Shield saw "GET" — a verb absent from `csrf.methods` — and skipped validation entirely. Nothing needs spoofing: Inertia issues real verbs. (Adding safe verbs to `csrf.methods` would NOT help; Shield validates exactly the verbs on that list, so every navigation would then demand a token.) Disabled accounts no longer authenticate over the API. `isDisabled` was checked when a token was issued and in the session guard, never when a Bearer token was used, so a token minted before the account was disabled kept working. Disabling an account now also revokes its long-lived credentials. An impersonating admin could mint a permanent API token for the target — and see its raw value — because `createToken` reads `auth.user`. Same guard as the other sensitive actions now applies to create and revoke. Password resets revoke remember-me tokens (2 years) and API tokens; a deliberate password change signs out other devices but keeps API tokens, which the user created on purpose and can see in their profile. Two GETs that changed state are gone: logout is POST-only, and the "add to favourites" link from the purchase email is a signed URL — it has to stay a GET because it is clicked in a mail client, but it is no longer forgeable. Throttle buckets derive from `request.ip()`, which honours `trustProxy`, instead of reading `X-Forwarded-For` directly. Also moves the Inertia middleware inside the session middleware (unavoidably in this commit, since both touch start/kernel.ts): its dispose() reflashes messages when it turns a stale-asset request into a 409, and that only survives if it runs before the session commit. Every fix has a test that fails without it. For the four security guards that was verified by temporarily disabling the guard and watching the test go red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…paths
The systemic one: `errors` was never shared from the Inertia middleware, so
`form.errors` was always empty and the client read a 302-back validation failure
as a success. `onError` was dead code across the whole app, `onSuccess` ran after
rejections, and dialogs closed and reset themselves over input the server had
refused. Adding it makes `:invalid`, inline messages and onError work everywhere
at once.
Inertia mutations no longer answer 4xx with a Location — that makes the client
raise an error modal and swallow the flash, so the user never learns why the
action failed. Plain 302 plus a flash, and `redirect('back', true)` so filters in
the URL and rows in the table stop disagreeing. A shared `listRedirectUrl` helper
replaces the duplicated referer handling.
Validation drift, which shows up as a silent server rejection with no warning:
allergens picked when creating a product were dropped entirely (Inertia serialises
arrays into FormData as indexed string keys, and the validator filtered strings
out — the edit page hid this behind a JSON workaround, so the two pages behaved
differently). Both forms now send the same shape; JSON stays because an empty
array has no FormData representation at all, which is the only way to express
"remove every allergen". Barcode and name collisions return a field-level message
instead of a 500, decided by the unique constraint rather than a pre-flight query
that a concurrent insert can always overtake. Client mirrors added for the profile
and music limits; the bodyparser limit now sits above the validator's, so an
oversized upload is a validation error rather than a bare 413.
`create_product` over MCP could never succeed — the tool promises a product
without an image but `image_path` was NOT NULL. Migrated to nullable.
Data paths: `?page=-1` no longer 500s (PostgreSQL rejects a negative OFFSET),
storno restores stock with an atomic increment instead of a read-modify-write on
an unlocked row, and keypad-id allocation shares one transaction with the insert
it belongs to — `pg_advisory_xact_lock` releases at commit, so allocating
separately handed the same number to concurrent registrations.
CURRENCY finally applies everywhere: 36 strings had the symbol hard-coded, the
client got an ad-hoc patch and the server got nothing, so an instance configured
for EUR still mailed invoices in crowns. One `{currency}` placeholder, one
substitution for both sides.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hanics `npm run check:routes` compares every call in inertia/** and tests/e2e/** against the real route table from `node ace list:routes --json` — not by parsing routes.ts, so group prefixes cannot skew it. Wired into check.sh and the quality workflow. This exists because the bug that started all of this was mechanically detectable and nobody was looking: routes are indexed by method, so a verb mismatch answers 404 rather than 405, which reads like a wrong URL. Verified by temporarily reintroducing the original bug — it was caught, with the line and the reason. CLAUDE.md gains a table of mechanics verified against node_modules rather than recalled: middleware ordering and what is not yet available in the server stack, why a verb miss is a 404, why GET must never be added to csrf.methods, how the Inertia error and redirect contracts work, what FormData cannot express, and the transaction scope of advisory locks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The throttle bucket key is derived from request.ip(), and the peer address is not portable: loopback surfaces as `127.0.0.1` on this machine and as something else on the CI runner. Pre-filling a hard-coded key therefore filled a bucket nothing read, the request under test was never throttled, and CI failed with 200 instead of 429 while the same tests passed locally. The helper now discovers the key with one throwaway request and fills that bucket, and throws with the observed keys if it does not find exactly one — so a future environment difference fails loudly instead of quietly not testing anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.