From 2a2b4d02baa2c984e2a04ed8ed84db858ce8be38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=A0indel=C3=A1=C5=99?= Date: Tue, 11 Aug 2026 16:12:44 +0200 Subject: [PATCH 1/4] fix(security): close authorization and credential-lifetime holes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- app/auth/api_or_entra_guard.ts | 20 ++- app/controllers/api/auth_controller.ts | 6 + app/controllers/web/kiosk_controller.ts | 22 ++- .../web/password_reset_controller.ts | 5 + app/controllers/web/profile_controller.ts | 46 +++-- app/controllers/web/shop_controller.ts | 79 ++++---- app/middleware/kiosk_only_middleware.ts | 30 ++++ app/middleware/throttle_middleware.ts | 7 +- app/services/admin_service.ts | 19 +- app/services/credential_revocation.ts | 29 +++ app/services/notification_service.ts | 14 +- app/services/password_reset_service.ts | 5 + config/app.ts | 11 +- config/shield.ts | 6 + inertia/layouts/AppLayout.vue | 3 +- inertia/pages/kiosk/index.vue | 11 +- start/kernel.ts | 10 +- start/routes.ts | 24 ++- tests/e2e/global_setup.ts | 17 +- .../functional/api/api_or_entra_guard.spec.ts | 72 ++++++++ tests/functional/api/mcp.spec.ts | 42 ++++- tests/functional/rate_limit.spec.ts | 60 +++++-- .../functional/web/account_lifecycle.spec.ts | 84 +++++++++ tests/functional/web/csrf.spec.ts | 73 ++++++++ tests/functional/web/impersonation.spec.ts | 69 ++++++- tests/functional/web/kiosk_basket.spec.ts | 169 ++++++++++++++++++ tests/functional/web/shop.spec.ts | 58 +++++- 27 files changed, 884 insertions(+), 107 deletions(-) create mode 100644 app/middleware/kiosk_only_middleware.ts create mode 100644 app/services/credential_revocation.ts create mode 100644 tests/functional/web/csrf.spec.ts diff --git a/app/auth/api_or_entra_guard.ts b/app/auth/api_or_entra_guard.ts index d898fc13..b3048534 100644 --- a/app/auth/api_or_entra_guard.ts +++ b/app/auth/api_or_entra_guard.ts @@ -3,6 +3,7 @@ import type { ApplicationService, ConfigProvider } from '@adonisjs/core/types' import { AccessTokensGuard } from '@adonisjs/auth/access_tokens' import type { GuardConfigProvider } from '@adonisjs/auth/types' import type { AccessTokensUserProviderContract } from '@adonisjs/auth/types/access_tokens' +import { errors as authErrors } from '@adonisjs/auth' import { entraIdJwtVerifier } from '#services/entra_id_jwt_verifier' import { looksLikeJwt } from '#utils/bearer_token' @@ -29,6 +30,19 @@ class ApiOrEntraGuard< this.#httpContext = args[1] } + /** + * Disabled accounts must not authenticate. Tokens outlive the account state, so this + * is checked on every request rather than only at issuance — otherwise a token minted + * before an account was disabled would keep working indefinitely. + */ + #rejectIfDisabled(user: unknown) { + if ((user as { isDisabled?: boolean } | null)?.isDisabled) { + throw new authErrors.E_UNAUTHORIZED_ACCESS('Unauthorized access', { + guardDriverName: this.driverName, + }) + } + } + async authenticate() { const authHeader = this.#httpContext.request.header('authorization') if (authHeader?.startsWith('Bearer ')) { @@ -36,6 +50,7 @@ class ApiOrEntraGuard< if (looksLikeJwt(token)) { const user = await entraIdJwtVerifier.resolveUser(token) if (user) { + this.#rejectIfDisabled(user) this.authenticationAttempted = true this.isAuthenticated = true // Entra-resolved users have no currentAccessToken; API controllers only @@ -46,7 +61,10 @@ class ApiOrEntraGuard< // A JWT that doesn't resolve to a linked user falls through and fails below. } } - return super.authenticate() + + const user = await super.authenticate() + this.#rejectIfDisabled(user) + return user } } diff --git a/app/controllers/api/auth_controller.ts b/app/controllers/api/auth_controller.ts index 9b52ebe4..58bdc45e 100644 --- a/app/controllers/api/auth_controller.ts +++ b/app/controllers/api/auth_controller.ts @@ -41,6 +41,12 @@ export default class AuthController { return response.unauthorized({ error: 'User not found or disabled.' }) } + // Honour the user's own "disable keypad sign-in" preference on both identifiers — + // the keypad and the card are the two ways this endpoint identifies somebody. + if (user.keypadDisabled) { + return response.unauthorized({ error: 'Keypad sign-in is disabled for this user.' }) + } + const token = await User.accessTokens.create(user, ['*'], { name: 'kiosk-token', expiresIn: '24h', diff --git a/app/controllers/web/kiosk_controller.ts b/app/controllers/web/kiosk_controller.ts index 39783bb5..a6ff0a00 100644 --- a/app/controllers/web/kiosk_controller.ts +++ b/app/controllers/web/kiosk_controller.ts @@ -53,6 +53,7 @@ export default class KioskController { const customer = await User.query() .where('keypadId', normalizedKeypadId) .where('isDisabled', false) + .where('keypadDisabled', false) .first() if (!customer) { @@ -99,6 +100,12 @@ export default class KioskController { async purchaseBasket({ request, response }: HttpContext) { const { customerId, items } = await request.validateUsing(purchaseBasketValidator) + // The customer id comes from the request, so re-check the target is a live account. + const customer = await User.query().where('id', customerId).where('isDisabled', false).first() + if (!customer) { + return response.status(404).json({ ok: false, error: 'customer_not_found' }) + } + const orderService = new OrderService() try { @@ -140,6 +147,7 @@ export default class KioskController { const customer = await User.query() .where('keypadId', keypadId) .where('isDisabled', false) + .where('keypadDisabled', false) .first() if (!customer) { @@ -194,6 +202,12 @@ export default class KioskController { return response.redirect('/kiosk') } + // The customer id comes from the request, so re-check the target is a live account. + const customer = await User.query().where('id', customerId).where('isDisabled', false).first() + if (!customer) { + return response.redirect('/kiosk') + } + const orderService = new OrderService() try { @@ -204,13 +218,9 @@ export default class KioskController { logger.error({ err }, 'Failed to send purchase confirmation email') }) - const customer = await User.find(customerId) - return response.redirect(`/kiosk/shop?keypadId=${customer?.keypadId ?? ''}&success=1`) + return response.redirect(`/kiosk/shop?keypadId=${customer.keypadId ?? ''}&success=1`) } catch { - const customer = await User.find(customerId) - return response.redirect( - `/kiosk/shop?keypadId=${customer?.keypadId ?? ''}&error=out_of_stock` - ) + return response.redirect(`/kiosk/shop?keypadId=${customer.keypadId ?? ''}&error=out_of_stock`) } } } diff --git a/app/controllers/web/password_reset_controller.ts b/app/controllers/web/password_reset_controller.ts index ea11ef6e..7f75bc47 100644 --- a/app/controllers/web/password_reset_controller.ts +++ b/app/controllers/web/password_reset_controller.ts @@ -11,6 +11,7 @@ import PasswordResetService from '#services/password_reset_service' import AuthModeService from '#services/auth_mode_service' import ReauthStepupService from '#services/reauth_stepup_service' import { isDomainError } from '#services/domain_error' +import { revokeLongLivedCredentials } from '#services/credential_revocation' export default class PasswordResetController { private resets = new PasswordResetService() @@ -169,6 +170,10 @@ export default class PasswordResetController { user.password = data.newPassword await user.save() + // Sign out every other device. API tokens survive — the user created those on purpose + // and can revoke them from the profile page. + await revokeLongLivedCredentials(user.id, { includeApiTokens: false }) + await AuditService.log(user.id, 'user.password_changed', 'user', user.id, null, { via: 'profile', ip: request.ip(), diff --git a/app/controllers/web/profile_controller.ts b/app/controllers/web/profile_controller.ts index e695dbd1..47452170 100644 --- a/app/controllers/web/profile_controller.ts +++ b/app/controllers/web/profile_controller.ts @@ -6,7 +6,6 @@ import { DateTime } from 'luxon' import { updateProfileValidator, toggleColorModeValidator, - updateExcludedAllergensValidator, updatePreferencesValidator, } from '#validators/user' import { @@ -556,6 +555,17 @@ export default class ProfileController { async createToken({ request, auth, response, session, i18n }: HttpContext) { const user = auth.user! + + // `auth.user` is the impersonated target here, so an admin could otherwise mint a + // permanent credential for somebody else's account — and see its raw value. + if (this.isImpersonating(session)) { + session.flash('alert', { + type: 'danger', + message: i18n.t('messages.sensitive_action_blocked_while_impersonating'), + }) + return response.redirect('/profile') + } + const data = await request.validateUsing(createApiTokenValidator) const expiresIn = data.expiresInDays ? `${data.expiresInDays} days` : undefined @@ -581,6 +591,15 @@ export default class ProfileController { async revokeToken({ params, auth, response, session, i18n }: HttpContext) { const user = auth.user! + + if (this.isImpersonating(session)) { + session.flash('alert', { + type: 'danger', + message: i18n.t('messages.sensitive_action_blocked_while_impersonating'), + }) + return response.redirect('/profile') + } + const tokenId = Number(params.id) // Verify the token belongs to this user before deleting @@ -605,31 +624,6 @@ export default class ProfileController { return response.redirect('/profile') } - async updateExcludedAllergens({ request, auth, response }: HttpContext) { - const user = auth.user! - const before = await this.getExcludedAllergenIds(user.id) - const data = await request.validateUsing(updateExcludedAllergensValidator) - await this.syncExcludedAllergenIds(user, data.excludedAllergenIds) - - const after = await this.getExcludedAllergenIds(user.id) - if (before.length !== after.length || before.some((id, index) => id !== after[index])) { - const allergenIds = [...new Set([...before, ...after])] - const allergenRows = - allergenIds.length > 0 - ? await Allergen.query().whereIn('id', allergenIds).select('id', 'name') - : [] - const namesById = new Map(allergenRows.map((a) => [a.id, a.name])) - const toLabel = (ids: number[]) => - ids.map((id) => namesById.get(id) ?? `#${id}`).join(', ') || '—' - - await AuditService.log(user.id, 'profile.updated', 'user', user.id, null, { - excludedAllergens: { from: toLabel(before), to: toLabel(after) }, - }) - } - - return response.redirect().back() - } - async toggleFavorite({ params, auth, response }: HttpContext) { const user = auth.user! const productId = Number(params.id) diff --git a/app/controllers/web/shop_controller.ts b/app/controllers/web/shop_controller.ts index 384420ec..a6874d40 100644 --- a/app/controllers/web/shop_controller.ts +++ b/app/controllers/web/shop_controller.ts @@ -10,39 +10,10 @@ import Product from '#models/product' import PageView from '#models/page_view' export default class ShopController { - async index({ inertia, auth, request, response, session, i18n }: HttpContext) { + async index({ inertia, auth, request }: HttpContext) { const shopService = new ShopService() const user = auth.user! - // Handle ?add_favorite=X param (used in purchase confirmation email links) - const addFavoriteRaw = request.input('add_favorite') - if (addFavoriteRaw) { - const productId = Number(addFavoriteRaw) - if (!Number.isNaN(productId) && productId > 0) { - const product = await Product.find(productId) - if (product) { - const existing = await user - .related('favoriteProducts') - .query() - .where('products.id', productId) - .first() - if (!existing) { - await user.related('favoriteProducts').attach([productId]) - await AuditService.log(user.id, 'favorite.added', 'product', productId, null, { - name: product.displayName, - }) - session.flash('alert', { type: 'success', message: i18n.t('messages.favorite_added') }) - } else { - session.flash('alert', { - type: 'info', - message: i18n.t('messages.favorite_already_added'), - }) - } - } - } - return response.redirect('/shop') - } - // Fire-and-forget page view tracking PageView.create({ userId: user.id, channel: 'web' }).catch((err) => { logger.error({ err }, 'Failed to record page view') @@ -79,11 +50,55 @@ export default class ShopController { return inertia.render('shop/index', { products, categories, - filters: { category: categoryId ?? '' }, - excludeAllergens, + // Both live under `filters` — that is where the page reads them from, and it is + // also what the partial reload on filter changes asks for. + filters: { category: categoryId ?? '', excludeAllergens }, }) } + /** + * "Add to favourites" link from the purchase-confirmation email. + * + * A GET that writes to the database can be triggered cross-site (an tag suffices), + * so the link is signed: the email carries a signature bound to this product and user, + * and anything without a valid one is refused. It has to stay a GET — it is clicked from + * a mail client, which cannot POST. + */ + async addFavorite({ params, request, auth, response, session, i18n }: HttpContext) { + if (!request.hasValidSignature('add-favorite')) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.action_failed') }) + return response.redirect('/shop') + } + + const user = auth.user! + const productId = Number(params.productId) + const product = Number.isInteger(productId) ? await Product.find(productId) : null + + if (!product) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.not_found') }) + return response.redirect('/shop') + } + + const existing = await user + .related('favoriteProducts') + .query() + .where('products.id', productId) + .first() + + if (existing) { + session.flash('alert', { type: 'info', message: i18n.t('messages.favorite_already_added') }) + return response.redirect('/shop') + } + + await user.related('favoriteProducts').attach([productId]) + await AuditService.log(user.id, 'favorite.added', 'product', productId, null, { + name: product.displayName, + }) + session.flash('alert', { type: 'success', message: i18n.t('messages.favorite_added') }) + + return response.redirect('/shop') + } + async purchase({ request, auth, response, session, i18n }: HttpContext) { const { deliveryId } = await request.validateUsing(purchaseValidator) const orderService = new OrderService() diff --git a/app/middleware/kiosk_only_middleware.ts b/app/middleware/kiosk_only_middleware.ts new file mode 100644 index 00000000..9723064c --- /dev/null +++ b/app/middleware/kiosk_only_middleware.ts @@ -0,0 +1,30 @@ +import type { HttpContext } from '@adonisjs/core/http' +import type { NextFn } from '@adonisjs/core/types/http' + +/** + * Kiosk-only middleware restricts the kiosk endpoints to the dedicated kiosk account. + * + * These endpoints act on behalf of an arbitrary customer — the customer id travels + * in the request payload — so authentication alone is not enough: any signed-in user + * could otherwise order onto somebody else's account. + * + * This is the counterpart of KioskMiddleware, which keeps kiosk accounts *out* of the + * regular UI. That one does not imply this one. + */ +export default class KioskOnlyMiddleware { + async handle(ctx: HttpContext, next: NextFn) { + if (!ctx.auth.user?.isKiosk) { + if (ctx.request.accepts(['html', 'json']) === 'json') { + return ctx.response.status(403).json({ error: 'forbidden' }) + } + + ctx.session.flash('alert', { + type: 'danger', + message: ctx.i18n.t('messages.unauthorized'), + }) + return ctx.response.redirect('/') + } + + return next() + } +} diff --git a/app/middleware/throttle_middleware.ts b/app/middleware/throttle_middleware.ts index d2f01573..ccc3b5b2 100644 --- a/app/middleware/throttle_middleware.ts +++ b/app/middleware/throttle_middleware.ts @@ -83,9 +83,10 @@ export default class ThrottleMiddleware { const userId = ctx.auth?.user?.id if (userId) return `throttle:user:${userId}` - const ip = ctx.request.header('x-forwarded-for')?.split(',')[0]?.trim() ?? ctx.request.ip() - - return `throttle:ip:${ip}` + // request.ip() resolves X-Forwarded-For only through the configured trustProxy tier. + // Reading the header directly let any client pick its own bucket and rotate the header + // to get an unlimited number of them. + return `throttle:ip:${ctx.request.ip()}` } private shouldUseInertiaFlashResponse(ctx: HttpContext): boolean { diff --git a/app/services/admin_service.ts b/app/services/admin_service.ts index 0e5d023c..37ab636c 100644 --- a/app/services/admin_service.ts +++ b/app/services/admin_service.ts @@ -5,6 +5,7 @@ import Category from '#models/category' import Allergen from '#models/allergen' import db from '@adonisjs/lucid/services/db' import InvoiceService from '#services/invoice_service' +import { revokeLongLivedCredentials } from '#services/credential_revocation' import { DateTime } from 'luxon' export default class AdminService { @@ -195,7 +196,17 @@ export default class AdminService { if (data.isKiosk !== undefined) user.isKiosk = data.isKiosk if (data.keypadId !== undefined) user.keypadId = data.keypadId + const nowDisabled = user.$dirty.isDisabled === true + await user.save() + + // Disabling must take effect immediately: long-lived credentials would otherwise + // keep the account usable (remember-me cookies live 2 years, API tokens can be + // issued without an expiry). + if (nowDisabled) { + await revokeLongLivedCredentials(user.id) + } + return user } @@ -433,9 +444,11 @@ export default class AdminService { throw new Error('ORDER_ALREADY_INVOICED') } - // Restore stock - order.delivery.amountLeft += 1 - await order.delivery.useTransaction(trx).save() + // Restore stock with an atomic increment rather than read-modify-write. The preloaded + // delivery is not row-locked (forUpdate() applies to `orders`), so a concurrent + // purchase or storno on the same delivery would otherwise overwrite the other's + // amountLeft and quietly lose a unit of stock. + await trx.from('deliveries').where('id', order.deliveryId).increment('amount_left', 1) // Delete the order await order.useTransaction(trx).delete() diff --git a/app/services/credential_revocation.ts b/app/services/credential_revocation.ts new file mode 100644 index 00000000..1ef087e9 --- /dev/null +++ b/app/services/credential_revocation.ts @@ -0,0 +1,29 @@ +import db from '@adonisjs/lucid/services/db' +import type { TransactionClientContract } from '@adonisjs/lucid/types/database' + +/** + * Revoke the credentials of a user that outlive a session. + * + * Sessions expire on their own, but remember-me tokens (2 years) and personal API tokens + * (optional expiry) do not — so whenever an account is disabled or its password changes, + * they have to go, or the old credential keeps granting access. + * + * `includeApiTokens` separates the two situations: after a password *reset* or an account + * being disabled every credential is suspect, while a user deliberately changing their own + * password should keep the API tokens they created on purpose and can see in their profile. + * + * Pass `trx` when the caller already has a transaction open, so the revocation commits or + * rolls back together with the change that triggered it. + */ +export async function revokeLongLivedCredentials( + userId: number, + options: { trx?: TransactionClientContract; includeApiTokens?: boolean } = {} +) { + const client = options.trx ?? db + + await client.from('remember_me_tokens').where('tokenable_id', userId).delete() + + if (options.includeApiTokens !== false) { + await client.from('auth_access_tokens').where('tokenable_id', userId).delete() + } +} diff --git a/app/services/notification_service.ts b/app/services/notification_service.ts index bad3127f..d86d6216 100644 --- a/app/services/notification_service.ts +++ b/app/services/notification_service.ts @@ -1,4 +1,5 @@ import mail from '@adonisjs/mail/services/main' +import router from '@adonisjs/core/services/router' import i18nManager from '@adonisjs/i18n/services/main' import env from '#start/env' import type { DateTime } from 'luxon' @@ -43,7 +44,18 @@ export default class NotificationService { .where('product_id', productId) .first() - const addFavoriteUrl = isFavorite ? null : `${this.appUrl}/shop?add_favorite=${productId}` + // Signed link: the target writes to the database on a GET, so it must not be forgeable. + // Expires well after the mail is useful, but not forever. + const addFavoriteUrl = isFavorite + ? null + : this.appUrl + + router.makeSignedUrl( + '/shop/favorites/:productId', + { productId }, + // disableRouteLookup: mails are also sent from the scheduler (console + // environment), where the router is never committed and a lookup by name throws. + { expiresIn: '30 days', purpose: 'add-favorite', disableRouteLookup: true } + ) await mail.send((message) => { message diff --git a/app/services/password_reset_service.ts b/app/services/password_reset_service.ts index 4ded5f35..ff38f6e7 100644 --- a/app/services/password_reset_service.ts +++ b/app/services/password_reset_service.ts @@ -5,6 +5,7 @@ import env from '#start/env' import User from '#models/user' import PasswordResetToken from '#models/password_reset_token' import { DomainError } from '#services/domain_error' +import { revokeLongLivedCredentials } from '#services/credential_revocation' export default class PasswordResetService { private normalizeEmail(email: string) { @@ -78,6 +79,10 @@ export default class PasswordResetService { user.password = newPassword await user.save() + // A reset is how a user recovers a compromised account, so every credential that + // outlives the session has to die with the old password. + await revokeLongLivedCredentials(user.id, { trx }) + token.usedAt = DateTime.utc() await token.save() diff --git a/config/app.ts b/config/app.ts index 34e571ef..be991a5c 100644 --- a/config/app.ts +++ b/config/app.ts @@ -43,7 +43,16 @@ export function parseTrustProxy( */ export const http = defineConfig({ generateRequestId: true, - allowMethodSpoofing: true, + + /** + * Method spoofing is OFF on purpose. Nothing in the app needs it — Inertia issues + * real PUT/PATCH/DELETE requests — and leaving it on is a CSRF hazard: spoofing is + * resolved from `request.method()`, which Shield also consults when deciding whether + * a request needs a CSRF token. Because the bodyparser runs before Shield (but after + * routing), a cross-site POST carrying `_method=GET` in its body routes as POST while + * Shield sees "GET" — a method absent from `csrf.methods` — and skips validation. + */ + allowMethodSpoofing: false, /** * Trust the X-Forwarded-* headers from the configured proxy tier. diff --git a/config/shield.ts b/config/shield.ts index dd648b72..16b8160c 100644 --- a/config/shield.ts +++ b/config/shield.ts @@ -27,6 +27,12 @@ const shieldConfig = defineConfig({ ) }, enableXsrfCookie: true, + /** + * Unsafe verbs only — Shield validates exactly the methods listed here, so adding + * GET/HEAD/OPTIONS would demand a token on every navigation. That makes the list a + * gate keyed on `request.method()`, which is why method spoofing stays disabled in + * config/app.ts: a spoofed verb would otherwise pick which side of this gate it lands on. + */ methods: ['POST', 'PUT', 'PATCH', 'DELETE'], }, diff --git a/inertia/layouts/AppLayout.vue b/inertia/layouts/AppLayout.vue index ba7fb731..e35d1006 100644 --- a/inertia/layouts/AppLayout.vue +++ b/inertia/layouts/AppLayout.vue @@ -136,7 +136,8 @@ const menuItems = computed(() => { }) function logout() { - window.location.assign('/logout') + // POST, not a plain navigation: signing out is a state change and must carry a CSRF token. + router.post('/logout') } function stopImpersonation() { diff --git a/inertia/pages/kiosk/index.vue b/inertia/pages/kiosk/index.vue index 135ee0f4..c5eed836 100644 --- a/inertia/pages/kiosk/index.vue +++ b/inertia/pages/kiosk/index.vue @@ -161,6 +161,9 @@ function playLoginTone(type: 'success' | 'error') { // ── Basket ──────────────────────────────────────────────────────────────────── +// Server-side cap per basket line (app/validators/order.ts). +const MAX_LINE_QUANTITY = 99 + const basket = ref([]) const checkoutLoading = ref(false) const outOfStockDeliveryId = ref(null) @@ -270,7 +273,10 @@ function addToBasket(product: ProductItem) { const existing = basket.value.find((i) => i.deliveryId === nextLot.deliveryId) if (existing) { - if (existing.quantity >= existing.maxStock) { + // MAX_LINE_QUANTITY mirrors the server cap in app/validators/order.ts. Without it the + // basket happily grows past 99 and the whole checkout is then rejected with a generic + // error the customer cannot act on. + if (existing.quantity >= Math.min(existing.maxStock, MAX_LINE_QUANTITY)) { toast.add({ severity: 'warn', summary: t('kiosk.max_stock_reached'), life: 2000 }) return } @@ -436,7 +442,7 @@ async function onKeypadSubmit(keypadId: string) { const data = await res.json() if (data.action === 'logout') { - window.location.assign('/logout') + router.post('/logout') return } @@ -507,6 +513,7 @@ async function submitBasket() { method: 'POST', headers: { 'Content-Type': 'application/json', + 'Accept': 'application/json', 'X-XSRF-TOKEN': getCsrfToken(), }, body: JSON.stringify({ diff --git a/start/kernel.ts b/start/kernel.ts index 36dfd84e..8802f9ee 100644 --- a/start/kernel.ts +++ b/start/kernel.ts @@ -29,7 +29,6 @@ server.use([ () => import('@adonisjs/cors/cors_middleware'), () => import('@adonisjs/vite/vite_middleware'), () => import('@adonisjs/static/static_middleware'), - () => import('#middleware/inertia_middleware'), ]) /** @@ -39,6 +38,14 @@ server.use([ router.use([ () => import('@adonisjs/core/bodyparser_middleware'), () => import('@adonisjs/session/session_middleware'), + /** + * Sits *inside* the session middleware on purpose. Its dispose() step reflashes + * messages when it turns a stale-asset request into a 409, and that only survives if + * it runs before the session is committed. Registered in the server stack (where the + * adapter's docs put it) dispose() would run after the commit and the reflash would be + * a silent no-op, losing the flash message across the forced reload. + */ + () => import('#middleware/inertia_middleware'), () => import('#middleware/cache_guard_middleware'), () => import('@adonisjs/shield/shield_middleware'), () => import('@adonisjs/auth/initialize_auth_middleware'), @@ -57,5 +64,6 @@ export const middleware = router.named({ emailVerified: () => import('#middleware/email_verified_middleware'), role: () => import('#middleware/role_middleware'), kiosk: () => import('#middleware/kiosk_middleware'), + kioskOnly: () => import('#middleware/kiosk_only_middleware'), throttle: () => import('#middleware/throttle_middleware'), }) diff --git a/start/routes.ts b/start/routes.ts index 86608771..6002b864 100644 --- a/start/routes.ts +++ b/start/routes.ts @@ -69,8 +69,17 @@ const McpOauthRegisterController = () => import('#controllers/web/mcp_oauth_regi const McpOauthAuthorizeController = () => import('#controllers/web/mcp_oauth_authorize_controller') const McpOauthTokenController = () => import('#controllers/web/mcp_oauth_token_controller') -const authThrottleLimit = process.env.NODE_ENV === 'test' ? 1000 : 10 -const mcpThrottleLimit = process.env.NODE_ENV === 'test' ? 1000 : 120 +/** + * Throttle limits. The test default is high so unrelated suites are not rate-limited, but + * it is overridable so a test can actually exhaust the limit with real requests — pinning + * a bucket into the store proves the response shape and nothing about the counting. + */ +const authThrottleLimit = Number( + process.env.AUTH_THROTTLE_LIMIT ?? (process.env.NODE_ENV === 'test' ? 1000 : 10) +) +const mcpThrottleLimit = Number( + process.env.MCP_THROTTLE_LIMIT ?? (process.env.NODE_ENV === 'test' ? 1000 : 120) +) /* |-------------------------------------------------------------------------- @@ -227,8 +236,8 @@ router.get('/auth/:provider/callback', [OidcController, 'callback']) router.get('/email/verify/:token', [EmailVerificationController, 'verify']) router.get('/profile/iban/verify/:token', [IbanChangeController, 'verify']) -router.get('/logout', [LoginController, 'destroy']).use(middleware.auth()).as('logout.get') -router.post('/logout', [LoginController, 'destroy']).use(middleware.auth()).as('logout.post') +// POST only — a GET logout is triggerable cross-site and Shield does not guard safe verbs. +router.post('/logout', [LoginController, 'destroy']).use(middleware.auth()).as('logout') // Stop impersonation — requires auth only (impersonation middleware has already run) router.post('/impersonate/stop', [AdminImpersonationController, 'destroy']).use(middleware.auth()) @@ -244,6 +253,10 @@ router // Shop router.get('/shop', [ShopController, 'index']) router.post('/shop/purchase', [ShopController, 'purchase']) + // Signed GET — clicked from the purchase-confirmation email, verified in the controller. + router + .get('/shop/favorites/:productId', [ShopController, 'addFavorite']) + .as('shop.favorites.add') // Orders router.get('/orders', [OrdersController, 'index']) @@ -258,7 +271,6 @@ router router.get('/profile', [ProfileController, 'show']) router.put('/profile', [ProfileController, 'update']) router.put('/profile/preferences', [ProfileController, 'updatePreferences']) - router.put('/profile/excluded-allergens', [ProfileController, 'updateExcludedAllergens']) router.post('/profile/color-mode', [ProfileController, 'toggleColorMode']) router.post('/profile/favorites/:id', [ProfileController, 'toggleFavorite']) @@ -391,7 +403,7 @@ router router.get('/kiosk/shop', [KioskController, 'shop']) router.post('/kiosk/purchase', [KioskController, 'purchase']) }) - .use(middleware.auth()) + .use([middleware.auth(), middleware.kioskOnly()]) /* |-------------------------------------------------------------------------- diff --git a/tests/e2e/global_setup.ts b/tests/e2e/global_setup.ts index 32bb5a2e..5e948b0d 100644 --- a/tests/e2e/global_setup.ts +++ b/tests/e2e/global_setup.ts @@ -157,6 +157,11 @@ export default async function globalSetup() { await client.query('BEGIN') for (const user of users) { + // The kiosk device really has to be flagged as one: the /kiosk endpoints act on behalf + // of an arbitrary customer, so they are restricted to kiosk accounts. Seeding it as a + // plain customer would test a path no real kiosk takes. + const isKiosk = user.email === 'kiosk@localhost' + await client.query( ` INSERT INTO users ( @@ -166,12 +171,20 @@ export default async function globalSetup() { created_at, updated_at ) VALUES ( $1, $2, $3, $4, $5, $6, - false, false, false, true, + $7, false, false, true, true, 'dark', false, NULL, NOW(), NOW(), NOW() ) `, - [user.email, user.display_name, user.password, user.role, user.keypad_id, user.iban] + [ + user.email, + user.display_name, + user.password, + user.role, + user.keypad_id, + user.iban, + isKiosk, + ] ) } diff --git a/tests/functional/api/api_or_entra_guard.spec.ts b/tests/functional/api/api_or_entra_guard.spec.ts index 7c7c3e77..ab784293 100644 --- a/tests/functional/api/api_or_entra_guard.spec.ts +++ b/tests/functional/api/api_or_entra_guard.spec.ts @@ -76,3 +76,75 @@ test.group('api_or_entra guard', (group) => { response.assertStatus(401) }) }) + +test.group('api guard rejects disabled accounts', (group) => { + const originalResolveUser = entraIdJwtVerifier.resolveUser.bind(entraIdJwtVerifier) + + group.each.setup(async () => { + throttleStore.clear() + await cleanAll() + }) + group.each.teardown(async () => { + entraIdJwtVerifier.resolveUser = originalResolveUser + await cleanAll() + }) + + test('an opaque token issued before the account was disabled stops working', async ({ + client, + }) => { + const user = await UserFactory.create() + const token = await User.accessTokens.create(user, ['*'], { name: 'before-disable' }) + const raw = token.value!.release() + + // Sanity: the token works while the account is live. + const before = await client.get('/api/v1/products').header('authorization', `Bearer ${raw}`) + before.assertStatus(200) + + user.isDisabled = true + await user.save() + + const after = await client.get('/api/v1/products').header('authorization', `Bearer ${raw}`) + after.assertStatus(401) + }) + + test('an Entra JWT resolving to a disabled user is rejected', async ({ client }) => { + const user = await UserFactory.apply('disabled').create() + entraIdJwtVerifier.resolveUser = async (token: string) => (token === JWT_SHAPED ? user : null) + + const response = await client + .get('/api/v1/products') + .header('authorization', `Bearer ${JWT_SHAPED}`) + + response.assertStatus(401) + }) + + test('disabling an account revokes its API tokens and remember-me tokens', async ({ + client, + assert, + }) => { + const admin = await UserFactory.apply('admin').create() + const user = await UserFactory.create() + await User.accessTokens.create(user, ['*'], { name: 'doomed' }) + await db.table('remember_me_tokens').insert({ + tokenable_id: user.id, + hash: 'deadbeef', + created_at: new Date(), + updated_at: new Date(), + expires_at: new Date(Date.now() + 86_400_000), + }) + + const response = await client + .put(`/admin/users/${user.id}`) + .form({ isDisabled: true }) + .loginAs(admin) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + + const apiTokens = await db.from('auth_access_tokens').where('tokenable_id', user.id) + const rememberTokens = await db.from('remember_me_tokens').where('tokenable_id', user.id) + assert.lengthOf(apiTokens, 0) + assert.lengthOf(rememberTokens, 0) + }) +}) diff --git a/tests/functional/api/mcp.spec.ts b/tests/functional/api/mcp.spec.ts index b572d554..e1619989 100644 --- a/tests/functional/api/mcp.spec.ts +++ b/tests/functional/api/mcp.spec.ts @@ -10,6 +10,8 @@ import { CategoryFactory } from '#database/factories/category_factory' import { store as throttleStore } from '#middleware/throttle_middleware' import User from '#models/user' import McpOauthClient from '#models/mcp_oauth_client' +import Category from '#models/category' +import Product from '#models/product' const MCP_URL = '/mcp' @@ -214,12 +216,15 @@ test.group('API MCP - Authentication', (group) => { response.assertStatus(403) }) - test('rejects disabled users with 403', async ({ client }) => { + test('rejects disabled users at authentication', async ({ client }) => { const disabled = await UserFactory.apply('disabled').create() const token = await createToken(disabled) + // 401, not 403: the api guard refuses to authenticate a disabled account at all, so + // the request never reaches the MCP role check. Tokens outlive the account state, so + // this has to be enforced per request rather than only at issuance. const response = await mcpPost(client, token, MCP_INIT) - response.assertStatus(403) + response.assertStatus(401) }) test('answers OPTIONS preflight with permissive CORS headers', async ({ client, assert }) => { @@ -640,3 +645,36 @@ test.group('API MCP - OAuth server', (group) => { response.assertStatus(400) }) }) + +test.group('API MCP - create_product', (group) => { + group.each.setup(async () => { + throttleStore.clear() + await cleanAll() + }) + group.each.teardown(cleanAll) + + test('a supplier can create a product without an image', async ({ client, assert }) => { + const supplier = await UserFactory.apply('supplier').create() + const token = await createToken(supplier) + const category = await Category.create({ name: 'MCP kategorie', color: '#123456' }) + + const response = await mcpPost(client, token, { + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { + name: 'create_product', + arguments: { + displayName: 'Produkt bez obrázku', + description: 'Vytvořeno přes MCP', + categoryId: category.id, + }, + }, + }) + + response.assertStatus(200) + const product = await Product.query().where('displayName', 'Produkt bez obrázku').first() + assert.isNotNull(product) + assert.isNull(product!.imagePath) + }) +}) diff --git a/tests/functional/rate_limit.spec.ts b/tests/functional/rate_limit.spec.ts index c780aad6..8fab6a27 100644 --- a/tests/functional/rate_limit.spec.ts +++ b/tests/functional/rate_limit.spec.ts @@ -4,6 +4,16 @@ import { store as throttleStore } from '#middleware/throttle_middleware' import { UserFactory } from '#database/factories/user_factory' import db from '@adonisjs/lucid/services/db' +/** + * The bucket key comes from request.ip(), which resolves X-Forwarded-For only through the + * configured trustProxy tier. In tests requests arrive from loopback with trustProxy at + * its default, so every request lands in the same bucket regardless of the header — which + * is exactly the property the last test here pins down. + */ +const LOOPBACK_KEY = 'throttle:ip:::ffff:127.0.0.1' + +const loopbackKeys = () => [...throttleStore.keys()].filter((key) => key.startsWith('throttle:ip:')) + test.group('Rate Limit Middleware', (group) => { group.each.setup(async () => { throttleStore.clear() @@ -14,17 +24,15 @@ test.group('Rate Limit Middleware', (group) => { test('Inertia web requests are redirected back with flash when throttled', async ({ client }) => { await UserFactory.apply('admin').create() - const ip = '198.51.100.11' - throttleStore.set(`throttle:ip:${ip}`, { - count: 10, - resetAt: Date.now() + 30_000, - }) + // Pre-fill whichever loopback bucket this environment resolves to. + for (const key of [LOOPBACK_KEY, 'throttle:ip:127.0.0.1']) { + throttleStore.set(key, { count: 10_000, resetAt: Date.now() + 30_000 }) + } const response = await client .post('/login') .header('X-Inertia', 'true') .header('X-Inertia-Version', '1') - .header('X-Forwarded-For', ip) .header('Referer', '/login') .form({ email: 'someone@example.com', @@ -38,17 +46,45 @@ test.group('Rate Limit Middleware', (group) => { }) test('API requests still return JSON 429 when throttled', async ({ client, assert }) => { - const ip = '198.51.100.12' - throttleStore.set(`throttle:ip:${ip}`, { - count: 60, - resetAt: Date.now() + 30_000, - }) + for (const key of [LOOPBACK_KEY, 'throttle:ip:127.0.0.1']) { + throttleStore.set(key, { count: 10_000, resetAt: Date.now() + 30_000 }) + } - const response = await client.get('/api/v1/health').header('X-Forwarded-For', ip) + const response = await client.get('/api/v1/health') response.assertStatus(429) assert.exists(response.header('retry-after')) assert.equal(response.body().error, 'Too many requests') assert.isNumber(response.body().retryAfter) }) + + test('the counter really increments per request', async ({ client, assert }) => { + await client.get('/api/v1/health') + const afterFirst = loopbackKeys() + assert.lengthOf(afterFirst, 1) + const countAfterFirst = throttleStore.get(afterFirst[0])!.count + + await client.get('/api/v1/health') + const countAfterSecond = throttleStore.get(afterFirst[0])!.count + + assert.equal(countAfterSecond, countAfterFirst + 1) + }) + + /** + * Buckets follow request.ip(), which resolves X-Forwarded-For only through the configured + * trustProxy tier — the middleware no longer reads the header itself. Verified by running + * this suite with TRUST_PROXY=false, where every forwarded address collapses into the + * single loopback bucket instead of minting one per header value. + */ + test('a trusted proxy chain still separates real clients', async ({ client, assert }) => { + // trustProxy defaults to "loopback", and tests arrive from loopback, so a well-formed + // forwarded address is honoured — which is what keeps per-client limits meaningful + // behind the reverse proxy in production. + await client.get('/api/v1/health').header('X-Forwarded-For', '203.0.113.1') + await client.get('/api/v1/health').header('X-Forwarded-For', '203.0.113.2') + + const keys = loopbackKeys() + assert.includeMembers(keys, ['throttle:ip:203.0.113.1', 'throttle:ip:203.0.113.2']) + assert.equal(throttleStore.get('throttle:ip:203.0.113.1')!.count, 1) + }) }) diff --git a/tests/functional/web/account_lifecycle.spec.ts b/tests/functional/web/account_lifecycle.spec.ts index 012f01d7..2df493ca 100644 --- a/tests/functional/web/account_lifecycle.spec.ts +++ b/tests/functional/web/account_lifecycle.spec.ts @@ -370,3 +370,87 @@ test.group('Web Auth - Registration and Password Lifecycle', (group) => { assert.exists(token) }) }) + +test.group('Password change revokes long-lived credentials', (group) => { + const cleanAll = async () => { + await db.from('audit_logs').delete() + await db.from('password_reset_tokens').delete() + await db.from('auth_access_tokens').delete() + await db.from('remember_me_tokens').delete() + await db.from('users').delete() + } + + group.each.setup(async () => { + throttleStore.clear() + await cleanAll() + }) + group.each.teardown(cleanAll) + + const seedLongLivedCredentials = async (user: User) => { + await User.accessTokens.create(user, ['*'], { name: 'integration' }) + await db.table('remember_me_tokens').insert({ + tokenable_id: user.id, + hash: `remember-${user.id}`, + created_at: new Date(), + updated_at: new Date(), + expires_at: new Date(Date.now() + 86_400_000), + }) + } + + test('a password reset kills remember-me cookies and API tokens', async ({ client, assert }) => { + const user = await UserFactory.merge({ email: 'reset-revoke@example.com' }).create() + await seedLongLivedCredentials(user) + + const payload = await new PasswordResetService().createToken(user.email) + assert.exists(payload) + const token = payload!.resetUrl.split('/').pop()! + + const response = await client + .post(`/reset-password/${token}`) + .form({ password: 'brand-new-pass-1', passwordConfirmation: 'brand-new-pass-1' }) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + response.assertHeader('location', '/login') + + const apiTokens = await db.from('auth_access_tokens').where('tokenable_id', user.id) + const rememberTokens = await db.from('remember_me_tokens').where('tokenable_id', user.id) + assert.lengthOf(apiTokens, 0) + assert.lengthOf(rememberTokens, 0) + + // And the new password really is in effect. + await user.refresh() + assert.isTrue(await hash.verify(user.password!, 'brand-new-pass-1')) + }) + + test('a deliberate password change signs out other devices but keeps API tokens', async ({ + client, + assert, + }) => { + const user = await UserFactory.merge({ email: 'change-revoke@example.com' }).create() + await seedLongLivedCredentials(user) + + const response = await client + .put('/profile/password') + .loginAs(user) + .form({ + currentPassword: 'password123', + newPassword: 'another-new-pass-1', + newPasswordConfirmation: 'another-new-pass-1', + }) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + + const rememberTokens = await db.from('remember_me_tokens').where('tokenable_id', user.id) + assert.lengthOf(rememberTokens, 0) + + const apiTokens = await db.from('auth_access_tokens').where('tokenable_id', user.id) + assert.lengthOf(apiTokens, 1) + + await user.refresh() + assert.isTrue(await hash.verify(user.password!, 'another-new-pass-1')) + }) +}) diff --git a/tests/functional/web/csrf.spec.ts b/tests/functional/web/csrf.spec.ts new file mode 100644 index 00000000..52ad8ed8 --- /dev/null +++ b/tests/functional/web/csrf.spec.ts @@ -0,0 +1,73 @@ +import '#tests/test_context' +import { test } from '@japa/runner' +import db from '@adonisjs/lucid/services/db' +import { UserFactory } from '#database/factories/user_factory' + +const cleanAll = async () => { + await db.from('audit_logs').delete() + await db.from('auth_access_tokens').delete() + await db.from('users').delete() +} + +/** + * Shield decides whether a request needs a CSRF token by looking at `request.method()`, + * and only the verbs listed in `config/shield.ts` are validated. With method spoofing + * enabled, a cross-site POST carrying `_method=GET` in its body would route as POST while + * Shield saw "GET" — a verb absent from that list — and skip validation entirely. + * + * Spoofing is therefore off (config/app.ts). These tests pin both halves of that: the + * gate cannot be talked out of validating, and a spoofed verb cannot reach another route. + */ +test.group('CSRF cannot be bypassed through method spoofing', (group) => { + group.each.setup(cleanAll) + group.each.teardown(cleanAll) + + test('a state-changing POST without a CSRF token is rejected', async ({ client, assert }) => { + const user = await UserFactory.create() + + const response = await client + .post('/profile/tokens') + .form({ name: 'no-csrf' }) + .loginAs(user) + .redirects(0) + + assert.notEqual(response.status(), 200) + + const tokens = await db.from('auth_access_tokens').where('tokenable_id', user.id) + assert.lengthOf(tokens, 0) + }) + + test('_method=GET in the body does not talk Shield out of validating', async ({ + client, + assert, + }) => { + const user = await UserFactory.create() + + const response = await client + .post('/profile/tokens') + .form({ _method: 'GET', name: 'spoofed-past-shield' }) + .loginAs(user) + .redirects(0) + + assert.notEqual(response.status(), 200) + + const tokens = await db.from('auth_access_tokens').where('tokenable_id', user.id) + assert.lengthOf(tokens, 0) + }) + + test('_method in the body cannot reach a route registered for another verb', async ({ + client, + }) => { + const user = await UserFactory.create() + + // PUT /profile exists; POST /profile does not. + const response = await client + .post('/profile') + .form({ _method: 'PUT', displayName: 'Spoofed Name' }) + .loginAs(user) + .withCsrfToken() + .redirects(0) + + response.assertStatus(404) + }) +}) diff --git a/tests/functional/web/impersonation.spec.ts b/tests/functional/web/impersonation.spec.ts index 8c762c27..a0694509 100644 --- a/tests/functional/web/impersonation.spec.ts +++ b/tests/functional/web/impersonation.spec.ts @@ -2,9 +2,12 @@ import '#tests/test_context' import { test } from '@japa/runner' import { UserFactory } from '#database/factories/user_factory' import db from '@adonisjs/lucid/services/db' +import hash from '@adonisjs/core/services/hash' +import User from '#models/user' const cleanAll = async () => { await db.from('audit_logs').delete() + await db.from('auth_access_tokens').delete() await db.from('users').delete() } @@ -167,9 +170,10 @@ test.group('Admin Impersonation', (group) => { assert.equal(target.iban, 'CZ6508000000192000145399') }) - test('impersonating admin cannot change target password', async ({ client }) => { + test('impersonating admin cannot change target password', async ({ client, assert }) => { const admin = await UserFactory.apply('admin').create() const target = await UserFactory.create() + const originalHash = target.password const response = await client .put('/profile/password') @@ -191,6 +195,69 @@ test.group('Admin Impersonation', (group) => { response.assertStatus(302) response.assertHeader('location', '/profile') + + // The redirect alone proves nothing — it is also what success returns. Assert the effect. + await target.refresh() + assert.equal(target.password, originalHash) + assert.isFalse(await hash.verify(target.password!, 'new-secret-123')) + + const passwordAudit = await db + .from('audit_logs') + .where('user_id', target.id) + .where('action', 'user.password_changed') + assert.lengthOf(passwordAudit, 0) + }) + + test('impersonating admin cannot mint an API token for the target', async ({ + client, + assert, + }) => { + const admin = await UserFactory.apply('admin').create() + const target = await UserFactory.create() + + const response = await client + .post('/profile/tokens') + .loginAs(admin) + .withSession({ + __impersonation: { + byId: admin.id, + asId: target.id, + asName: target.displayName, + }, + }) + .form({ name: 'stolen-token' }) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + response.assertHeader('location', '/profile') + + const tokens = await db.from('auth_access_tokens').where('tokenable_id', target.id) + assert.lengthOf(tokens, 0) + }) + + test('impersonating admin cannot revoke the target API token', async ({ client, assert }) => { + const admin = await UserFactory.apply('admin').create() + const target = await UserFactory.create() + const token = await User.accessTokens.create(target, ['*'], { name: 'targets-own' }) + + const response = await client + .delete(`/profile/tokens/${token.identifier}`) + .loginAs(admin) + .withSession({ + __impersonation: { + byId: admin.id, + asId: target.id, + asName: target.displayName, + }, + }) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + + const tokens = await db.from('auth_access_tokens').where('tokenable_id', target.id) + assert.lengthOf(tokens, 1) }) test('impersonating admin cannot start OIDC link flow', async ({ client }) => { diff --git a/tests/functional/web/kiosk_basket.spec.ts b/tests/functional/web/kiosk_basket.spec.ts index 2fee7e23..a6397e15 100644 --- a/tests/functional/web/kiosk_basket.spec.ts +++ b/tests/functional/web/kiosk_basket.spec.ts @@ -426,3 +426,172 @@ test.group('GET /kiosk/customer', (group) => { assert.notInclude(body.recommendedIds, blockedProduct.id) }) }) + +test.group('Kiosk endpoints are restricted to the kiosk account', (group) => { + group.each.setup(cleanAll) + group.each.teardown(cleanAll) + + const stockedDelivery = async () => { + const supplier = await UserFactory.apply('supplier').create() + const category = await CategoryFactory.create() + const product = await ProductFactory.merge({ categoryId: category.id }).create() + return DeliveryFactory.merge({ + supplierId: supplier.id, + productId: product.id, + amountLeft: 5, + price: 15, + }).create() + } + + test('a plain customer cannot order onto another account via purchase-basket', async ({ + client, + assert, + }) => { + const attacker = await UserFactory.create() + const victim = await UserFactory.create() + const delivery = await stockedDelivery() + + const response = await client + .post('/kiosk/purchase-basket') + .header('accept', 'application/json') + .json({ customerId: victim.id, items: [{ deliveryId: delivery.id, quantity: 1 }] }) + .loginAs(attacker) + .withCsrfToken() + + response.assertStatus(403) + + const orders = await Order.query().where('buyerId', victim.id) + assert.lengthOf(orders, 0) + }) + + test('a supplier cannot order onto another account via the single-item endpoint', async ({ + client, + assert, + }) => { + const attacker = await UserFactory.apply('supplier').create() + const victim = await UserFactory.create() + const delivery = await stockedDelivery() + + const response = await client + .post('/kiosk/purchase') + .form({ customerId: victim.id, deliveryId: delivery.id }) + .loginAs(attacker) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + assert.equal(response.header('location'), '/') + + const orders = await Order.query().where('buyerId', victim.id) + assert.lengthOf(orders, 0) + }) + + test('a plain customer cannot enumerate customers via the identify endpoint', async ({ + client, + }) => { + const attacker = await UserFactory.create() + const victim = await UserFactory.create() + + const response = await client + .get(`/kiosk/customer?keypadId=${victim.keypadId}`) + .header('accept', 'application/json') + .loginAs(attacker) + + response.assertStatus(403) + }) + + test('the kiosk refuses to order onto a disabled account', async ({ client, assert }) => { + const kioskDevice = await UserFactory.apply('kiosk').create() + const disabled = await UserFactory.apply('disabled').create() + const delivery = await stockedDelivery() + + const response = await client + .post('/kiosk/purchase-basket') + .header('accept', 'application/json') + .json({ customerId: disabled.id, items: [{ deliveryId: delivery.id, quantity: 1 }] }) + .loginAs(kioskDevice) + .withCsrfToken() + + response.assertStatus(404) + + const orders = await Order.query().where('buyerId', disabled.id) + assert.lengthOf(orders, 0) + }) +}) + +test.group('POST /kiosk/purchase (single item)', (group) => { + group.each.setup(cleanAll) + group.each.teardown(cleanAll) + + test('the kiosk can buy a single item for a customer', async ({ client, assert }) => { + const kioskDevice = await UserFactory.apply('kiosk').create() + const customer = await UserFactory.create() + const supplier = await UserFactory.apply('supplier').create() + const category = await CategoryFactory.create() + const product = await ProductFactory.merge({ categoryId: category.id }).create() + const delivery = await DeliveryFactory.merge({ + supplierId: supplier.id, + productId: product.id, + amountLeft: 2, + price: 25, + }).create() + + const response = await client + .post('/kiosk/purchase') + .form({ customerId: customer.id, deliveryId: delivery.id }) + .loginAs(kioskDevice) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + assert.include(response.header('location')!, 'success=1') + + const orders = await Order.query().where('buyerId', customer.id) + assert.lengthOf(orders, 1) + + await delivery.refresh() + assert.equal(delivery.amountLeft, 1) + }) + + test('an out-of-stock item redirects with an error and creates no order', async ({ + client, + assert, + }) => { + const kioskDevice = await UserFactory.apply('kiosk').create() + const customer = await UserFactory.create() + const supplier = await UserFactory.apply('supplier').create() + const category = await CategoryFactory.create() + const product = await ProductFactory.merge({ categoryId: category.id }).create() + const delivery = await DeliveryFactory.merge({ + supplierId: supplier.id, + productId: product.id, + amountLeft: 0, + price: 25, + }).create() + + const response = await client + .post('/kiosk/purchase') + .form({ customerId: customer.id, deliveryId: delivery.id }) + .loginAs(kioskDevice) + .withCsrfToken() + .redirects(0) + + response.assertStatus(302) + assert.include(response.header('location')!, 'error=out_of_stock') + + const orders = await Order.query().where('buyerId', customer.id) + assert.lengthOf(orders, 0) + }) + + test('a keypad-disabled customer cannot be identified at the kiosk', async ({ client }) => { + const kioskDevice = await UserFactory.apply('kiosk').create() + const customer = await UserFactory.merge({ keypadDisabled: true }).create() + + const response = await client + .get(`/kiosk/customer?keypadId=${customer.keypadId}`) + .header('accept', 'application/json') + .loginAs(kioskDevice) + + response.assertStatus(404) + }) +}) diff --git a/tests/functional/web/shop.spec.ts b/tests/functional/web/shop.spec.ts index 0b97190e..6daa59ce 100644 --- a/tests/functional/web/shop.spec.ts +++ b/tests/functional/web/shop.spec.ts @@ -1,5 +1,6 @@ import '#tests/test_context' import { test } from '@japa/runner' +import router from '@adonisjs/core/services/router' import { UserFactory } from '#database/factories/user_factory' import { ProductFactory } from '#database/factories/product_factory' import { DeliveryFactory } from '#database/factories/delivery_factory' @@ -370,11 +371,19 @@ test.group('Web Shop - purchase', (group) => { }) }) -test.group('Web Shop - add_favorite', (group) => { +test.group('Web Shop - add to favourites from email', (group) => { group.each.setup(cleanAll) group.each.teardown(cleanAll) - test('?add_favorite adds product to favorites and redirects to /shop', async ({ + /** Builds the same signed link the purchase-confirmation email carries. */ + const signedFavoriteUrl = (productId: number) => + router.makeSignedUrl( + '/shop/favorites/:productId', + { productId }, + { expiresIn: '30 days', purpose: 'add-favorite', disableRouteLookup: true } + ) + + test('a signed link adds the product to favourites and redirects to /shop', async ({ client, assert, }) => { @@ -382,7 +391,7 @@ test.group('Web Shop - add_favorite', (group) => { const category = await CategoryFactory.create() const product = await ProductFactory.merge({ categoryId: category.id }).create() - const response = await client.get(`/shop?add_favorite=${product.id}`).loginAs(user).redirects(0) + const response = await client.get(signedFavoriteUrl(product.id)).loginAs(user).redirects(0) response.assertStatus(302) assert.equal(response.header('location'), '/shop') @@ -403,7 +412,42 @@ test.group('Web Shop - add_favorite', (group) => { assert.equal(metadata?.name, product.displayName) }) - test('?add_favorite when already a favorite does not duplicate and redirects', async ({ + test('an unsigned link writes nothing — a GET that mutates must not be forgeable', async ({ + client, + assert, + }) => { + const user = await UserFactory.create() + const category = await CategoryFactory.create() + const product = await ProductFactory.merge({ categoryId: category.id }).create() + + const response = await client.get(`/shop/favorites/${product.id}`).loginAs(user).redirects(0) + + response.assertStatus(302) + + const count = await db.from('user_favorites').where('user_id', user.id).count('* as total') + assert.equal(Number(count[0].total), 0) + }) + + test('a tampered signature writes nothing', async ({ client, assert }) => { + const user = await UserFactory.create() + const category = await CategoryFactory.create() + const product = await ProductFactory.merge({ categoryId: category.id }).create() + const other = await ProductFactory.merge({ categoryId: category.id }).create() + + // Signature issued for one product, replayed against another. + const signature = signedFavoriteUrl(other.id).split('signature=')[1] + const response = await client + .get(`/shop/favorites/${product.id}?signature=${signature}`) + .loginAs(user) + .redirects(0) + + response.assertStatus(302) + + const rows = await db.from('user_favorites').where('user_id', user.id) + assert.lengthOf(rows, 0) + }) + + test('a signed link for an already-favourite product does not duplicate', async ({ client, assert, }) => { @@ -415,7 +459,7 @@ test.group('Web Shop - add_favorite', (group) => { .table('user_favorites') .insert({ user_id: user.id, product_id: product.id, created_at: new Date() }) - const response = await client.get(`/shop?add_favorite=${product.id}`).loginAs(user).redirects(0) + const response = await client.get(signedFavoriteUrl(product.id)).loginAs(user).redirects(0) response.assertStatus(302) @@ -423,13 +467,13 @@ test.group('Web Shop - add_favorite', (group) => { assert.equal(Number(count[0].total), 1) }) - test('?add_favorite with non-existent product redirects gracefully', async ({ + test('a signed link for a non-existent product redirects gracefully', async ({ client, assert, }) => { const user = await UserFactory.create() - const response = await client.get('/shop?add_favorite=999999').loginAs(user).redirects(0) + const response = await client.get(signedFavoriteUrl(999999)).loginAs(user).redirects(0) response.assertStatus(302) assert.equal(response.header('location'), '/shop') From d59eb4772859e16a83548f81e99ee1bd14b072f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20=C5=A0indel=C3=A1=C5=99?= Date: Tue, 11 Aug 2026 16:13:19 +0200 Subject: [PATCH 2/4] fix(app): honour the Inertia protocol, align validation, harden data paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- adonisrc.ts | 1 + .../web/admin/allergens_controller.ts | 12 +- app/controllers/web/admin/audit_controller.ts | 3 +- .../web/admin/categories_controller.ts | 12 +- .../web/admin/invoices_controller.ts | 3 +- .../web/admin/orders_controller.ts | 3 +- .../web/admin/storno_controller.ts | 5 +- app/controllers/web/admin/users_controller.ts | 35 ++-- app/controllers/web/audit_controller.ts | 3 +- app/controllers/web/bootstrap_controller.ts | 28 ++- app/controllers/web/invoices_controller.ts | 12 +- app/controllers/web/login_controller.ts | 2 +- app/controllers/web/oidc_controller.ts | 37 ++-- app/controllers/web/orders_controller.ts | 3 +- app/controllers/web/ratings_controller.ts | 35 ++-- app/controllers/web/register_controller.ts | 29 ++- .../web/supplier/deliveries_controller.ts | 3 +- .../web/supplier/invoice_controller.ts | 5 +- .../web/supplier/payments_controller.ts | 6 +- .../web/supplier/products_controller.ts | 54 ++++-- .../web/supplier/stock_controller.ts | 3 +- app/helpers/list_redirect.ts | 23 +++ app/helpers/pagination.ts | 12 ++ app/mcp/tools/supplier_tools.ts | 35 ++-- app/middleware/email_verified_middleware.ts | 2 +- app/middleware/inertia_middleware.ts | 26 ++- app/services/currency_service.ts | 23 +++ app/services/product_keypad_id.ts | 28 +++ app/services/product_service.ts | 40 +++-- app/services/unique_violation.ts | 18 ++ app/validators/product.ts | 7 +- config/bodyparser.ts | 10 +- config/inertia.ts | 8 + ...81000000005_make_product_image_nullable.ts | 28 +++ database/schema.ts | 2 +- inertia/composables/use_inline_edit.ts | 9 + inertia/pages/admin/allergens/index.vue | 1 + inertia/pages/admin/categories/index.vue | 1 + inertia/pages/admin/music/index.vue | 51 +++++- inertia/pages/admin/users/index.vue | 4 + inertia/pages/profile/show.vue | 69 ++++++- inertia/pages/ratings/feed.vue | 10 +- inertia/pages/supplier/products/create.vue | 22 ++- inertia/pages/supplier/products/edit.vue | 8 +- resources/lang/cs/admin.json | 14 +- resources/lang/cs/common.json | 6 +- resources/lang/cs/emails.json | 42 ++--- resources/lang/cs/messages.json | 6 +- resources/lang/cs/profile.json | 4 +- resources/lang/en/admin.json | 14 +- resources/lang/en/common.json | 6 +- resources/lang/en/emails.json | 42 ++--- resources/lang/en/messages.json | 6 +- resources/lang/en/profile.json | 4 +- start/env.ts | 7 + start/i18n.ts | 23 +++ tests/functional/web/admin.spec.ts | 93 ++++++++++ .../functional/web/email_verification.spec.ts | 70 ++++++++ tests/functional/web/inertia_errors.spec.ts | 40 +++++ tests/functional/web/invoices.spec.ts | 49 +++++ tests/functional/web/profile.spec.ts | 111 ++++++++++++ tests/functional/web/ratings.spec.ts | 163 ++++++++++++++++- tests/functional/web/supplier.spec.ts | 170 +++++++++++++++++- 63 files changed, 1325 insertions(+), 276 deletions(-) create mode 100644 app/helpers/list_redirect.ts create mode 100644 app/helpers/pagination.ts create mode 100644 app/services/product_keypad_id.ts create mode 100644 app/services/unique_violation.ts create mode 100644 database/migrations/1781000000005_make_product_image_nullable.ts create mode 100644 start/i18n.ts create mode 100644 tests/functional/web/inertia_errors.spec.ts diff --git a/adonisrc.ts b/adonisrc.ts index d8e7e5b2..d29e6950 100644 --- a/adonisrc.ts +++ b/adonisrc.ts @@ -79,6 +79,7 @@ export default defineConfig({ preloads: [ () => import('#start/routes'), () => import('#start/kernel'), + () => import('#start/i18n'), { file: () => import('#start/scheduler'), environment: ['console'], diff --git a/app/controllers/web/admin/allergens_controller.ts b/app/controllers/web/admin/allergens_controller.ts index e70a2abd..c6550dd5 100644 --- a/app/controllers/web/admin/allergens_controller.ts +++ b/app/controllers/web/admin/allergens_controller.ts @@ -2,6 +2,7 @@ import type { HttpContext } from '@adonisjs/core/http' import AdminService from '#services/admin_service' import { createAllergenValidator, updateAllergenValidator } from '#validators/allergen' import AuditService from '#services/audit_service' +import { isUniqueViolation } from '#services/unique_violation' import Allergen from '#models/allergen' export default class AllergensController { @@ -26,7 +27,16 @@ export default class AllergensController { const data = await request.validateUsing(createAllergenValidator) const service = new AdminService() - const allergen = await service.createAllergen(data.name) + let allergen: Allergen + try { + allergen = await service.createAllergen(data.name) + } catch (err) { + if (isUniqueViolation(err, 'name')) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.name_taken') }) + return response.redirect().back() + } + throw err + } await AuditService.log(auth.user!.id, 'allergen.created', 'allergen', allergen.id, null, { name: allergen.name, diff --git a/app/controllers/web/admin/audit_controller.ts b/app/controllers/web/admin/audit_controller.ts index fe2fb452..5f3d2726 100644 --- a/app/controllers/web/admin/audit_controller.ts +++ b/app/controllers/web/admin/audit_controller.ts @@ -1,10 +1,11 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import AuditService from '#services/audit_service' import User from '#models/user' export default class AdminAuditController { async index({ inertia, request }: HttpContext) { - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const action = request.input('action') const entityType = request.input('entityType') const userId = request.input('userId') diff --git a/app/controllers/web/admin/categories_controller.ts b/app/controllers/web/admin/categories_controller.ts index 924c73cc..d8b92898 100644 --- a/app/controllers/web/admin/categories_controller.ts +++ b/app/controllers/web/admin/categories_controller.ts @@ -2,6 +2,7 @@ import type { HttpContext } from '@adonisjs/core/http' import AdminService from '#services/admin_service' import { createCategoryValidator, updateCategoryValidator } from '#validators/category' import AuditService from '#services/audit_service' +import { isUniqueViolation } from '#services/unique_violation' import Category from '#models/category' export default class CategoriesController { @@ -27,7 +28,16 @@ export default class CategoriesController { const data = await request.validateUsing(createCategoryValidator) const service = new AdminService() - const category = await service.createCategory(data.name, data.color) + let category: Category + try { + category = await service.createCategory(data.name, data.color) + } catch (err) { + if (isUniqueViolation(err, 'name')) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.name_taken') }) + return response.redirect().back() + } + throw err + } const metadata = { name: category.name, diff --git a/app/controllers/web/admin/invoices_controller.ts b/app/controllers/web/admin/invoices_controller.ts index 4625a02d..d3f5462d 100644 --- a/app/controllers/web/admin/invoices_controller.ts +++ b/app/controllers/web/admin/invoices_controller.ts @@ -1,10 +1,11 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import AdminService from '#services/admin_service' import db from '@adonisjs/lucid/services/db' export default class InvoicesController { async index({ inertia, request }: HttpContext) { - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const status = request.input('status') const buyerId = request.input('buyerId') const supplierId = request.input('supplierId') diff --git a/app/controllers/web/admin/orders_controller.ts b/app/controllers/web/admin/orders_controller.ts index a3187089..4d7e6aa9 100644 --- a/app/controllers/web/admin/orders_controller.ts +++ b/app/controllers/web/admin/orders_controller.ts @@ -1,10 +1,11 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import AdminService from '#services/admin_service' import db from '@adonisjs/lucid/services/db' export default class OrdersController { async index({ inertia, request }: HttpContext) { - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const channel = request.input('channel') const invoiced = request.input('invoiced') const buyerId = request.input('buyerId') diff --git a/app/controllers/web/admin/storno_controller.ts b/app/controllers/web/admin/storno_controller.ts index 19696d1b..88f767c9 100644 --- a/app/controllers/web/admin/storno_controller.ts +++ b/app/controllers/web/admin/storno_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import { listRedirectUrl } from '#helpers/list_redirect' import AdminService from '#services/admin_service' import AuditService from '#services/audit_service' import NotificationService from '#services/notification_service' @@ -6,7 +7,7 @@ import Order from '#models/order' import logger from '@adonisjs/core/services/logger' export default class StornoController { - async store({ params, response, session, i18n, auth }: HttpContext) { + async store({ params, request, response, session, i18n, auth }: HttpContext) { const service = new AdminService() try { @@ -50,6 +51,6 @@ export default class StornoController { session.flash('alert', { type: 'danger', message }) } - return response.redirect('/admin/orders') + return response.redirect(listRedirectUrl(request, '/admin/orders')) } } diff --git a/app/controllers/web/admin/users_controller.ts b/app/controllers/web/admin/users_controller.ts index 63841ca5..0c828ae4 100644 --- a/app/controllers/web/admin/users_controller.ts +++ b/app/controllers/web/admin/users_controller.ts @@ -1,4 +1,6 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' +import { listRedirectUrl } from '#helpers/list_redirect' import { DateTime } from 'luxon' import AdminService from '#services/admin_service' import InvoiceService from '#services/invoice_service' @@ -12,25 +14,10 @@ import RegistrationPolicyService from '#services/registration_policy_service' import PasswordResetService from '#services/password_reset_service' import InvitationService from '#services/invitation_service' -/** - * Return the referer URL if it points to /admin/users (preserving active filters), - * otherwise fall back to /admin/users without filters. - */ -function usersUrl(request: HttpContext['request']): string { - const referer = request.header('referer') ?? '' - try { - const { pathname, search } = new URL(referer) - if (pathname === '/admin/users') return pathname + search - } catch { - // invalid URL — use fallback - } - return '/admin/users' -} - export default class UsersController { async index({ inertia, request }: HttpContext) { - const page = request.input('page', 1) - const invitePage = Number(request.input('invitePage', 1)) + const page = resolvePage(request.input('page', 1)) + const invitePage = resolvePage(request.input('invitePage', 1)) const role = request.input('role') const userId = request.input('userId') const disabled = request.input('disabled') @@ -134,17 +121,17 @@ export default class UsersController { type: 'danger', message: i18n.t('messages.last_active_admin_required'), }) - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } if (err instanceof Error && err.message === 'USER_HAS_UNINVOICED_ORDERS') { - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } if (err instanceof Error && err.message === 'KEYPAD_ID_TAKEN') { session.flash('alert', { type: 'danger', message: i18n.t('messages.keypad_id_taken'), }) - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } throw err } @@ -170,7 +157,7 @@ export default class UsersController { message: i18n.t('messages.user_updated', { name: user.displayName }), }) - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } async generateInvoice({ params, request, response, session, i18n, auth }: HttpContext) { @@ -202,7 +189,7 @@ export default class UsersController { } } - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } async sendPasswordReset({ params, request, response, session, i18n, auth }: HttpContext) { @@ -213,7 +200,7 @@ export default class UsersController { const payload = await resetService.createToken(user.email) if (!payload) { session.flash('alert', { type: 'danger', message: i18n.t('messages.action_failed') }) - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } const notificationService = new NotificationService() @@ -235,6 +222,6 @@ export default class UsersController { type: 'success', message: i18n.t('messages.password_reset_email_sent'), }) - return response.redirect(usersUrl(request)) + return response.redirect(listRedirectUrl(request, '/admin/users')) } } diff --git a/app/controllers/web/audit_controller.ts b/app/controllers/web/audit_controller.ts index 69668a0f..c1e4ff8b 100644 --- a/app/controllers/web/audit_controller.ts +++ b/app/controllers/web/audit_controller.ts @@ -1,9 +1,10 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import AuditService from '#services/audit_service' export default class AuditController { async index({ inertia, auth, request }: HttpContext) { - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const action = request.input('action') const sortOrder = request.input('sortOrder') diff --git a/app/controllers/web/bootstrap_controller.ts b/app/controllers/web/bootstrap_controller.ts index b5e05f49..e22f1b27 100644 --- a/app/controllers/web/bootstrap_controller.ts +++ b/app/controllers/web/bootstrap_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import db from '@adonisjs/lucid/services/db' import logger from '@adonisjs/core/services/logger' import { DateTime } from 'luxon' import User from '#models/user' @@ -49,16 +50,25 @@ export default class BootstrapController { return response.redirect('/setup/bootstrap') } - const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId() + // Allocation and insert share one transaction — the keypad-id advisory lock is + // transaction-scoped and would otherwise be released before the row exists. + const user = await db.transaction(async (trx) => { + const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId( + trx as unknown as Parameters[0] + ) - const user = await User.create({ - displayName: data.displayName, - email: data.email.trim().toLowerCase(), - password: data.password, - keypadId: nextKeypadId, - role: 'admin', - emailVerifiedAt: DateTime.utc(), - pendingEmail: null, + return User.create( + { + displayName: data.displayName, + email: data.email.trim().toLowerCase(), + password: data.password, + keypadId: nextKeypadId, + role: 'admin', + emailVerifiedAt: DateTime.utc(), + pendingEmail: null, + }, + { client: trx } + ) }) await auth.use('web').login(user, true) diff --git a/app/controllers/web/invoices_controller.ts b/app/controllers/web/invoices_controller.ts index c2392b74..5d69de0e 100644 --- a/app/controllers/web/invoices_controller.ts +++ b/app/controllers/web/invoices_controller.ts @@ -1,4 +1,6 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' +import { listRedirectUrl } from '#helpers/list_redirect' import InvoiceService from '#services/invoice_service' import QrPaymentService from '#services/qr_payment_service' import NotificationService from '#services/notification_service' @@ -8,7 +10,7 @@ import logger from '@adonisjs/core/services/logger' export default class InvoicesController { async index({ inertia, auth, request }: HttpContext) { const invoiceService = new InvoiceService() - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const status = request.input('status') const sortBy = request.input('sortBy') const sortOrder = request.input('sortOrder') @@ -43,7 +45,7 @@ export default class InvoicesController { }) } - async requestPaid({ params, auth, response, session, i18n }: HttpContext) { + async requestPaid({ params, request, auth, response, session, i18n }: HttpContext) { const invoiceService = new InvoiceService() try { @@ -65,10 +67,10 @@ export default class InvoicesController { } } - return response.redirect('/invoices') + return response.redirect(listRedirectUrl(request, '/invoices')) } - async cancelPaid({ params, auth, response, session, i18n }: HttpContext) { + async cancelPaid({ params, request, auth, response, session, i18n }: HttpContext) { const invoiceService = new InvoiceService() try { @@ -91,7 +93,7 @@ export default class InvoicesController { } } - return response.redirect('/invoices') + return response.redirect(listRedirectUrl(request, '/invoices')) } async qrcode({ params, auth, response, i18n }: HttpContext) { diff --git a/app/controllers/web/login_controller.ts b/app/controllers/web/login_controller.ts index 75fd8d9b..db47c51f 100644 --- a/app/controllers/web/login_controller.ts +++ b/app/controllers/web/login_controller.ts @@ -74,7 +74,7 @@ export default class LoginController { }) if (this.verifications.shouldBlockAppAccess(user)) { session.flash('alert', { - type: 'warning', + type: 'warn', message: i18n.t('messages.email_verification_required'), }) return response.redirect('/profile') diff --git a/app/controllers/web/oidc_controller.ts b/app/controllers/web/oidc_controller.ts index cb0654f8..b747a3d4 100644 --- a/app/controllers/web/oidc_controller.ts +++ b/app/controllers/web/oidc_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import db from '@adonisjs/lucid/services/db' import logger from '@adonisjs/core/services/logger' import { DateTime } from 'luxon' import User from '#models/user' @@ -208,7 +209,7 @@ export default class OidcController { if (externalProvider.accessDenied()) { logger.warn({ provider }, 'External login cancelled by user') - session.flash('alert', { type: 'warning', message: i18n.t('messages.login_cancelled') }) + session.flash('alert', { type: 'warn', message: i18n.t('messages.login_cancelled') }) return response.redirect('/login') } @@ -432,19 +433,27 @@ export default class OidcController { } } - // Auto-register new user - const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId() + // Auto-register new user. Allocation and insert share one transaction — the + // keypad-id advisory lock is transaction-scoped. + user = await db.transaction(async (trx) => { + const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId( + trx as unknown as Parameters[0] + ) - user = await User.create({ - email, - displayName: displayName || email.split('@')[0], - phone, - role: allowBootstrapRegistration - ? 'admin' - : (invitation?.role ?? (hasAnyAdmin ? 'customer' : 'admin')), - keypadId: nextKeypadId, - emailVerifiedAt: DateTime.utc(), - pendingEmail: null, + return User.create( + { + email, + displayName: displayName || email.split('@')[0], + phone, + role: allowBootstrapRegistration + ? 'admin' + : (invitation?.role ?? (hasAnyAdmin ? 'customer' : 'admin')), + keypadId: nextKeypadId, + emailVerifiedAt: DateTime.utc(), + pendingEmail: null, + }, + { client: trx } + ) }) if (invitation) { @@ -524,7 +533,7 @@ export default class OidcController { }) if (this.verifications.shouldBlockAppAccess(user)) { session.flash('alert', { - type: 'warning', + type: 'warn', message: i18n.t('messages.email_verification_required'), }) return response.redirect('/profile') diff --git a/app/controllers/web/orders_controller.ts b/app/controllers/web/orders_controller.ts index 5292fdb5..45b44337 100644 --- a/app/controllers/web/orders_controller.ts +++ b/app/controllers/web/orders_controller.ts @@ -1,10 +1,11 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import OrderService from '#services/order_service' export default class OrdersController { async index({ inertia, auth, request }: HttpContext) { const orderService = new OrderService() - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const channel = request.input('channel') const invoiced = request.input('invoiced') const sortBy = request.input('sortBy') diff --git a/app/controllers/web/ratings_controller.ts b/app/controllers/web/ratings_controller.ts index 684469e4..80f7a8c6 100644 --- a/app/controllers/web/ratings_controller.ts +++ b/app/controllers/web/ratings_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import app from '@adonisjs/core/services/app' import ProductRatingService from '#services/product_rating_service' import { createOrUpdateRatingValidator } from '#validators/rating' @@ -30,7 +31,7 @@ export default class RatingsController { const canSeePrivate = viewerCanSeePrivate(user) const publicFeedEnabled = isPublicFeedEnabled() - const page = Number(request.input('page', 1)) || 1 + const page = resolvePage(request.input('page', 1)) const productIdRaw = request.input('productId') const visibilityRaw = request.input('visibility') as string | undefined const onlyMineRaw = request.input('onlyMine') as string | undefined @@ -123,7 +124,7 @@ export default class RatingsController { const productId = Number(productIdRaw) if (!productId) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_invalid') }) - return response.redirect('back') + return response.redirect('back', true) } const visibility = this.resolveVisibility(payload.visibility, auth.user) @@ -137,15 +138,15 @@ export default class RatingsController { visibility, }) session.flash('alert', { type: 'success', message: i18n.t('rating.flash_saved') }) - return response.redirect('back') + return response.redirect('back', true) } catch (error: unknown) { if (isDomainError(error, 'RATING_NOT_ORDERED')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_not_ordered') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } if (isDomainError(error, 'RATING_WINDOW_CLOSED')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_window_closed') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } throw error } @@ -156,11 +157,11 @@ export default class RatingsController { const rating = await ProductRating.find(Number(params.id)) if (!rating) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_not_found') }) - return response.status(404).redirect('back') + return response.redirect('back', true) } if (rating.userId !== auth.user!.id) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_forbidden') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } const visibility = this.resolveVisibility(payload.visibility, auth.user) @@ -174,15 +175,15 @@ export default class RatingsController { visibility, }) session.flash('alert', { type: 'success', message: i18n.t('rating.flash_saved') }) - return response.redirect('back') + return response.redirect('back', true) } catch (error: unknown) { if (isDomainError(error, 'RATING_NOT_ORDERED')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_not_ordered') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } if (isDomainError(error, 'RATING_WINDOW_CLOSED')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_window_closed') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } throw error } @@ -195,15 +196,15 @@ export default class RatingsController { try { await ProductRatingService.deleteRating(Number(params.id), user.id, isAdmin) session.flash('alert', { type: 'success', message: i18n.t('rating.flash_deleted') }) - return response.redirect('back') + return response.redirect('back', true) } catch (error: unknown) { if (isDomainError(error, 'RATING_FORBIDDEN')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_forbidden') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } if (isDomainError(error, 'RATING_NOT_FOUND')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_not_found') }) - return response.status(404).redirect('back') + return response.redirect('back', true) } throw error } @@ -218,19 +219,19 @@ export default class RatingsController { publicFeedEnabled: isPublicFeedEnabled(), viewerCanSeePrivate: canSeePrivate, }) - return response.redirect('back') + return response.redirect('back', true) } catch (error: unknown) { if (isDomainError(error, 'RATING_SELF_UPVOTE')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_self_upvote') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } if (isDomainError(error, 'RATING_UPVOTE_DISABLED')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_forbidden') }) - return response.status(403).redirect('back') + return response.redirect('back', true) } if (isDomainError(error, 'RATING_NOT_FOUND')) { session.flash('alert', { type: 'danger', message: i18n.t('rating.flash_not_found') }) - return response.status(404).redirect('back') + return response.redirect('back', true) } throw error } diff --git a/app/controllers/web/register_controller.ts b/app/controllers/web/register_controller.ts index 9447e8b3..3483ce27 100644 --- a/app/controllers/web/register_controller.ts +++ b/app/controllers/web/register_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import db from '@adonisjs/lucid/services/db' import logger from '@adonisjs/core/services/logger' import User from '#models/user' import AuditService from '#services/audit_service' @@ -69,16 +70,26 @@ export default class RegisterController { return response.redirect('/register') } - const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId() + // The keypad-id advisory lock is transaction-scoped, so the allocation and the insert + // have to share one transaction — otherwise the lock is gone before the row exists and + // two concurrent registrations collide on the unique index. + const user = await db.transaction(async (trx) => { + const nextKeypadId = await this.keypadIds.getNextAvailableUserKeypadId( + trx as unknown as Parameters[0] + ) - const user = await User.create({ - displayName: data.displayName.trim(), - email: normalizedEmail, - password: data.password, - keypadId: nextKeypadId, - role: 'customer', - emailVerifiedAt: null, - pendingEmail: null, + return User.create( + { + displayName: data.displayName.trim(), + email: normalizedEmail, + password: data.password, + keypadId: nextKeypadId, + role: 'customer', + emailVerifiedAt: null, + pendingEmail: null, + }, + { client: trx } + ) }) await auth.use('web').login(user, true) diff --git a/app/controllers/web/supplier/deliveries_controller.ts b/app/controllers/web/supplier/deliveries_controller.ts index 633e86a5..d697c885 100644 --- a/app/controllers/web/supplier/deliveries_controller.ts +++ b/app/controllers/web/supplier/deliveries_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import DeliveryService from '#services/delivery_service' import NotificationService from '#services/notification_service' import { createDeliveryValidator } from '#validators/delivery' @@ -21,7 +22,7 @@ function deliveryReturnUrl(request: HttpContext['request']): string { export default class DeliveriesController { async index({ inertia, auth, request }: HttpContext) { const service = new DeliveryService() - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const productId = request.input('productId') const sortBy = request.input('sortBy') const sortOrder = request.input('sortOrder') diff --git a/app/controllers/web/supplier/invoice_controller.ts b/app/controllers/web/supplier/invoice_controller.ts index a051b1e6..103ae822 100644 --- a/app/controllers/web/supplier/invoice_controller.ts +++ b/app/controllers/web/supplier/invoice_controller.ts @@ -1,4 +1,5 @@ import type { HttpContext } from '@adonisjs/core/http' +import User from '#models/user' import InvoiceService from '#services/invoice_service' import NotificationService from '#services/notification_service' import logger from '@adonisjs/core/services/logger' @@ -48,10 +49,12 @@ export default class InvoiceController { message: i18n.t('messages.invoice_no_orders_for_buyer'), }) } else { + // The message reads "invoice generated for {name}", so pass the name — not the id. + const buyer = await User.find(buyerId) session.flash('alert', { type: 'success', message: i18n.t('supplier.invoice_generated_for_buyer', { - name: String(buyerId), + name: buyer?.displayName ?? String(buyerId), }), }) diff --git a/app/controllers/web/supplier/payments_controller.ts b/app/controllers/web/supplier/payments_controller.ts index c62ce706..7739166c 100644 --- a/app/controllers/web/supplier/payments_controller.ts +++ b/app/controllers/web/supplier/payments_controller.ts @@ -1,4 +1,6 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' +import { listRedirectUrl } from '#helpers/list_redirect' import InvoiceService from '#services/invoice_service' import NotificationService from '#services/notification_service' import { paymentActionValidator } from '#validators/invoice' @@ -9,7 +11,7 @@ import Invoice from '#models/invoice' export default class PaymentsController { async index({ inertia, auth, request }: HttpContext) { const invoiceService = new InvoiceService() - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const status = request.input('status') const sortBy = request.input('sortBy') const sortOrder = request.input('sortOrder') @@ -88,6 +90,6 @@ export default class PaymentsController { } } - return response.redirect('/supplier/payments') + return response.redirect(listRedirectUrl(request, '/supplier/payments')) } } diff --git a/app/controllers/web/supplier/products_controller.ts b/app/controllers/web/supplier/products_controller.ts index 229d303e..bb42514c 100644 --- a/app/controllers/web/supplier/products_controller.ts +++ b/app/controllers/web/supplier/products_controller.ts @@ -1,13 +1,15 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import ProductService from '#services/product_service' import { createProductValidator, updateProductValidator } from '#validators/product' import AuditService from '#services/audit_service' import { normalizeImagePath } from '#helpers/image_url' +import { isUniqueViolation } from '#services/unique_violation' export default class ProductsController { async index({ inertia, request }: HttpContext) { const service = new ProductService() - const page = request.input('page', 1) + const page = resolvePage(request.input('page', 1)) const search = request.input('search') const categoryId = request.input('categoryId') @@ -56,14 +58,23 @@ export default class ProductsController { const data = await request.validateUsing(createProductValidator) const service = new ProductService() - const product = await service.createProduct({ - displayName: data.displayName, - description: data.description, - categoryId: data.categoryId, - barcode: data.barcode, - image: data.image, - allergenIds: data.allergenIds, - }) + let product: Awaited> + try { + product = await service.createProduct({ + displayName: data.displayName, + description: data.description, + categoryId: data.categoryId, + barcode: data.barcode, + image: data.image, + allergenIds: data.allergenIds, + }) + } catch (err) { + if (isUniqueViolation(err, 'barcode')) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.barcode_taken') }) + return response.redirect().back() + } + throw err + } await AuditService.log(auth.user!.id, 'product.created', 'product', product.id, null, { name: product.displayName, @@ -110,14 +121,23 @@ export default class ProductsController { const service = new ProductService() const beforeProduct = await service.getProduct(Number(params.id)) - const product = await service.updateProduct(Number(params.id), { - displayName: data.displayName, - description: data.description, - categoryId: data.categoryId, - barcode: data.barcode, - image: data.image, - allergenIds: data.allergenIds, - }) + let product: Awaited> + try { + product = await service.updateProduct(Number(params.id), { + displayName: data.displayName, + description: data.description, + categoryId: data.categoryId, + barcode: data.barcode, + image: data.image, + allergenIds: data.allergenIds, + }) + } catch (err) { + if (isUniqueViolation(err, 'barcode')) { + session.flash('alert', { type: 'danger', message: i18n.t('messages.barcode_taken') }) + return response.redirect().back() + } + throw err + } await product.load('category') await product.load('allergens') diff --git a/app/controllers/web/supplier/stock_controller.ts b/app/controllers/web/supplier/stock_controller.ts index 5b5cea88..525c5c25 100644 --- a/app/controllers/web/supplier/stock_controller.ts +++ b/app/controllers/web/supplier/stock_controller.ts @@ -1,11 +1,12 @@ import type { HttpContext } from '@adonisjs/core/http' +import { resolvePage } from '#helpers/pagination' import DeliveryService from '#services/delivery_service' import Category from '#models/category' export default class StockController { async index({ inertia, auth, request }: HttpContext) { const service = new DeliveryService() - const page = Number(request.input('page', 1)) + const page = resolvePage(request.input('page', 1)) const categoryId = request.input('categoryId') ? Number(request.input('categoryId')) : undefined const sortBy = request.input('sortBy') || undefined const sortOrder = request.input('sortOrder') || undefined diff --git a/app/helpers/list_redirect.ts b/app/helpers/list_redirect.ts new file mode 100644 index 00000000..3e4b628f --- /dev/null +++ b/app/helpers/list_redirect.ts @@ -0,0 +1,23 @@ +import type { HttpContext } from '@adonisjs/core/http' + +/** + * Where to send the browser back to after a mutation performed from a filtered list. + * + * Redirecting to the bare list path drops the query string, so the page reloads + * unfiltered on page 1 while the filter bar still shows the previous filters — the URL + * and the table end up describing different things. Reusing the referer keeps them in + * step, but only when it really points at the list in question; anything else (a foreign + * host, a crafted referer, a direct API call) falls back to the plain path. + */ +export function listRedirectUrl(request: HttpContext['request'], listPath: string): string { + const referer = request.header('referer') ?? '' + + try { + const { pathname, search } = new URL(referer, `${request.protocol()}://${request.host()}`) + if (pathname === listPath) return pathname + search + } catch { + // Unparseable referer — fall through to the plain list path. + } + + return listPath +} diff --git a/app/helpers/pagination.ts b/app/helpers/pagination.ts new file mode 100644 index 00000000..c0c5f1f3 --- /dev/null +++ b/app/helpers/pagination.ts @@ -0,0 +1,12 @@ +/** + * Turn a `?page=` query value into a page number Lucid's paginate() will accept. + * + * Anything non-numeric, zero or negative collapses to page 1. Without this a crafted + * `?page=-1` reaches paginate() and PostgreSQL rejects the resulting negative OFFSET, + * so every paginated screen answers 500 instead of showing the first page. + */ +export function resolvePage(input: unknown): number { + const parsed = Number(input) + if (!Number.isFinite(parsed)) return 1 + return Math.max(1, Math.trunc(parsed)) +} diff --git a/app/mcp/tools/supplier_tools.ts b/app/mcp/tools/supplier_tools.ts index 698944d7..f5c6ee25 100644 --- a/app/mcp/tools/supplier_tools.ts +++ b/app/mcp/tools/supplier_tools.ts @@ -7,6 +7,7 @@ import InvoiceService from '#services/invoice_service' import ProductService from '#services/product_service' import AuditService from '#services/audit_service' import Product from '#models/product' +import { withProductKeypadId } from '#services/product_keypad_id' import { ok, fail, mapDomainError, pageMeta } from '#mcp/tools/helpers' /** @@ -174,10 +175,10 @@ export function registerSupplierTools(server: McpServer, user: User) { 'in the web UI under Supplier → Products). A keypad ID is assigned automatically. ' + 'Idempotent by name: if a product with the same name already exists, it is returned instead.', { - displayName: z.string().min(1).max(100).describe('Product name shown in the shop'), - description: z.string().max(500).optional().describe('Short description'), + displayName: z.string().min(1).max(255).describe('Product name shown in the shop'), + description: z.string().max(1000).optional().describe('Short description'), categoryId: z.number().int().positive().describe('Category ID (see list_categories)'), - barcode: z.string().max(64).optional().describe('EAN barcode'), + barcode: z.string().max(100).optional().describe('EAN barcode'), allergenIds: z.array(z.number().int().positive()).optional().describe('Allergen IDs'), }, async ({ @@ -198,20 +199,24 @@ export function registerSupplierTools(server: McpServer, user: User) { }) } - const maxKeypad = await Product.query().max('keypad_id as max').first() - const nextKeypadId = (maxKeypad?.$extras.max ?? 0) + 1 + const product = await withProductKeypadId(async (trx, nextKeypadId) => { + const created = await Product.create( + { + keypadId: nextKeypadId, + displayName, + description: description ?? '', + categoryId, + barcode: barcode || null, + }, + { client: trx } + ) - const product = await Product.create({ - keypadId: nextKeypadId, - displayName, - description: description ?? '', - categoryId, - barcode: barcode || null, - }) + if (allergenIds && allergenIds.length > 0) { + await created.related('allergens').attach(allergenIds, trx) + } - if (allergenIds && allergenIds.length > 0) { - await product.related('allergens').attach(allergenIds) - } + return created + }) await AuditService.log(user.id, 'product.created', 'product', product.id, null, { displayName, diff --git a/app/middleware/email_verified_middleware.ts b/app/middleware/email_verified_middleware.ts index 5ef5e278..7a1765e6 100644 --- a/app/middleware/email_verified_middleware.ts +++ b/app/middleware/email_verified_middleware.ts @@ -26,7 +26,7 @@ export default class EmailVerifiedMiddleware { if (this.verifications.shouldBlockAppAccess(user)) { ctx.session.flash('alert', { - type: 'warning', + type: 'warn', message: ctx.i18n.t('messages.email_verification_required'), }) return ctx.response.redirect('/profile') diff --git a/app/middleware/inertia_middleware.ts b/app/middleware/inertia_middleware.ts index eceb7a7e..f7816e9a 100644 --- a/app/middleware/inertia_middleware.ts +++ b/app/middleware/inertia_middleware.ts @@ -6,7 +6,11 @@ import type { InferSharedProps } from '@adonisjs/inertia/types' import db from '@adonisjs/lucid/services/db' import env from '#start/env' import { readFileSync, readdirSync } from 'node:fs' -import { getCurrencyCode, getCurrencyDisplay } from '#services/currency_service' +import { + applyCurrencyPlaceholder, + getCurrencyCode, + getCurrencyDisplay, +} from '#services/currency_service' type ImpersonationSession = { byId: number; asId: number; asName: string } @@ -20,7 +24,6 @@ function loadTranslations(locale: string): Record const langDir = app.languageFilesPath(locale) const appName = env.get('APP_NAME', 'Small Business Fridge') - const currencyDisplay = getCurrencyDisplay(locale) try { const files = readdirSync(langDir).filter((file) => file.endsWith('.json')) @@ -28,15 +31,14 @@ function loadTranslations(locale: string): Record for (const file of files) { const namespace = file.replace('.json', '') - translations[namespace] = JSON.parse(readFileSync(`${langDir}/${file}`, 'utf-8')) + // The `{currency}` placeholder is resolved from config here, so the client never has + // to pass a currency to t() — the same substitution runs server-side in start/i18n.ts. + translations[namespace] = applyCurrencyPlaceholder( + JSON.parse(readFileSync(`${langDir}/${file}`, 'utf-8')), + locale + ) if (namespace === 'common') { translations[namespace].app_name = appName - translations[namespace].currency = currencyDisplay - translations[namespace].price_with_currency = `{price} ${currencyDisplay}` - const pieceUnit = translations[namespace].pieces ?? '' - translations[namespace].per_piece = pieceUnit - ? `{price} ${currencyDisplay}/${pieceUnit}` - : `{price} ${currencyDisplay}` } } @@ -94,6 +96,12 @@ export default class InertiaMiddleware extends BaseInertiaMiddleware { : undefined ), flash: ctx.inertia.always(ctx.session?.flashMessages.all() ?? {}), + /** + * Without this, `form.errors` is always empty, so Inertia treats a 302-back + * validation failure as a success: onError never fires, onSuccess does, and + * forms happily reset themselves over rejected input. + */ + errors: ctx.inertia.always(this.getValidationErrors(ctx)), impersonation: ctx.inertia.always( impersonation ? { asName: impersonation.asName } : undefined ), diff --git a/app/services/currency_service.ts b/app/services/currency_service.ts index 5dcab94c..62b8aead 100644 --- a/app/services/currency_service.ts +++ b/app/services/currency_service.ts @@ -47,3 +47,26 @@ export function getCurrencyCode(): string { export function getCurrencyDisplay(locale: string): string { return resolveCurrencyDisplay(getCurrencyCode(), locale) } + +/** + * Substitute the `{currency}` placeholder in a set of translations. + * + * The placeholder exists because the currency symbol comes from configuration, not from + * the language: hard-coding "Kč" in the strings meant a deployment with CURRENCY=EUR still + * mailed out amounts in crowns. Doing the substitution once, at load time, keeps every + * consumer working — server-side i18n, Edge mail templates and the client alike — without + * every t() call having to remember to pass the currency. + */ +export function applyCurrencyPlaceholder( + translations: Record, + locale: string +): Record { + const display = getCurrencyDisplay(locale) + const result: Record = {} + + for (const [key, value] of Object.entries(translations)) { + result[key] = typeof value === 'string' ? value.replaceAll('{currency}', display) : value + } + + return result +} diff --git a/app/services/product_keypad_id.ts b/app/services/product_keypad_id.ts new file mode 100644 index 00000000..da7dceaa --- /dev/null +++ b/app/services/product_keypad_id.ts @@ -0,0 +1,28 @@ +import db from '@adonisjs/lucid/services/db' +import type { TransactionClientContract } from '@adonisjs/lucid/types/database' + +/** Distinct from the users' keypad-allocation lock key in KeypadIdService. */ +const PRODUCT_KEYPAD_LOCK_KEY = 8999902 + +/** + * Allocate the next product keypad ID. + * + * `MAX(keypad_id) + 1` on its own is a read-modify-write: two suppliers creating a product + * at the same time read the same maximum and the second insert dies on the unique index. + * The advisory lock is transaction-scoped, so the caller must pass the transaction that + * also performs the insert — otherwise the lock is released before the row exists and the + * race is back. + */ +export async function allocateProductKeypadId(trx: TransactionClientContract): Promise { + await trx.rawQuery('SELECT pg_advisory_xact_lock(?)', [PRODUCT_KEYPAD_LOCK_KEY]) + + const result = await trx.from('products').max('keypad_id as max').first() + return Number(result?.max ?? 0) + 1 +} + +/** Convenience wrapper for callers that have no transaction of their own yet. */ +export async function withProductKeypadId( + handler: (trx: TransactionClientContract, keypadId: number) => Promise +): Promise { + return db.transaction(async (trx) => handler(trx, await allocateProductKeypadId(trx))) +} diff --git a/app/services/product_service.ts b/app/services/product_service.ts index 9321b249..e06c13c4 100644 --- a/app/services/product_service.ts +++ b/app/services/product_service.ts @@ -4,6 +4,7 @@ import Allergen from '#models/allergen' import type { MultipartFile } from '@adonisjs/core/bodyparser' import app from '@adonisjs/core/services/app' import { randomUUID } from 'node:crypto' +import { withProductKeypadId } from '#services/product_keypad_id' export default class ProductService { /** @@ -17,27 +18,30 @@ export default class ProductService { image: MultipartFile allergenIds?: number[] }): Promise { - // Auto-assign next keypad ID - const maxKeypad = await Product.query().max('keypad_id as max').first() - const nextKeypadId = (maxKeypad?.$extras.max ?? 0) + 1 - + // Save the upload before opening the transaction — it is slow I/O and would hold the + // keypad-id lock for its duration. const imagePath = await this.saveImage(data.image) - const product = await Product.create({ - keypadId: nextKeypadId, - displayName: data.displayName, - description: data.description, - imagePath, - categoryId: data.categoryId, - barcode: data.barcode || null, + return withProductKeypadId(async (trx, nextKeypadId) => { + const product = await Product.create( + { + keypadId: nextKeypadId, + displayName: data.displayName, + description: data.description, + imagePath, + categoryId: data.categoryId, + barcode: data.barcode || null, + }, + { client: trx } + ) + + const ids = data.allergenIds ?? [] + if (ids.length > 0) { + await product.related('allergens').attach(ids, trx) + } + + return product }) - - const ids = data.allergenIds ?? [] - if (ids.length > 0) { - await product.related('allergens').attach(ids) - } - - return product } /** diff --git a/app/services/unique_violation.ts b/app/services/unique_violation.ts new file mode 100644 index 00000000..4614047c --- /dev/null +++ b/app/services/unique_violation.ts @@ -0,0 +1,18 @@ +/** + * Recognise a PostgreSQL unique-constraint violation (SQLSTATE 23505). + * + * Checking the database error rather than pre-querying for a duplicate is deliberate: + * a "does it already exist?" query can always be overtaken between the check and the + * insert, so the constraint is the only reliable arbiter. Callers translate this into a + * field-level message instead of letting it surface as a 500. + * + * `constraintContains` narrows the match when one table has several unique constraints + * (e.g. products has both keypad_id and barcode), so each gets its own message. + */ +export function isUniqueViolation(error: unknown, constraintContains?: string): boolean { + const candidate = error as { code?: unknown; constraint?: unknown } | null + if (candidate?.code !== '23505') return false + if (!constraintContains) return true + + return String(candidate.constraint ?? '').includes(constraintContains) +} diff --git a/app/validators/product.ts b/app/validators/product.ts index 3e759510..762d665e 100644 --- a/app/validators/product.ts +++ b/app/validators/product.ts @@ -6,7 +6,12 @@ function parseAllergenIds() { .optional() .transform((v) => { if (v === undefined || v === null) return [] - if (Array.isArray(v)) return v.filter((n): n is number => typeof n === 'number' && n > 0) + // FormData stringifies everything, so an array arrives as ['3', '7'] from the browser + // and as [3, 7] from JSON/MCP. Coerce instead of filtering the strings out — that + // silently dropped every allergen picked in the create form. + if (Array.isArray(v)) { + return v.map(Number).filter((n) => Number.isInteger(n) && n > 0) + } if (typeof v === 'string') { try { const p = JSON.parse(v) as unknown diff --git a/config/bodyparser.ts b/config/bodyparser.ts index f3d1ead9..970cf101 100644 --- a/config/bodyparser.ts +++ b/config/bodyparser.ts @@ -44,10 +44,14 @@ const bodyParserConfig = defineConfig({ processManually: [], /** - * Maximum limit of data to parse including all files - * and fields + * Maximum limit of data to parse including all files and fields. + * + * Must stay ABOVE the largest per-file validator limit (music uploads allow 20mb, see + * app/validators/music_track.ts). At an equal limit the bodyparser aborts the request + * with a bare 413 before validation runs, so the user gets an untranslated error page + * instead of a field-level message — and the validator's own limit is unreachable. */ - limit: '20mb', + limit: '25mb', types: ['multipart/form-data'], }, }) diff --git a/config/inertia.ts b/config/inertia.ts index b0170b70..0efc885d 100644 --- a/config/inertia.ts +++ b/config/inertia.ts @@ -1,10 +1,18 @@ import { defineConfig } from '@adonisjs/inertia' +import env from '#start/env' const inertiaConfig = defineConfig({ /** * Path to the Edge view that will be used as the root view for Inertia responses */ rootView: 'inertia_layout', + + /** + * Left undefined by default so the version is hashed from the Vite manifest and a + * deploy pushes clients onto the new bundle. Set ASSETS_VERSION to pin it — useful in + * CI, where a stale `public/assets` manifest can otherwise be picked up by test runs. + */ + assetsVersion: env.get('ASSETS_VERSION') || undefined, }) export default inertiaConfig diff --git a/database/migrations/1781000000005_make_product_image_nullable.ts b/database/migrations/1781000000005_make_product_image_nullable.ts new file mode 100644 index 00000000..fc23923b --- /dev/null +++ b/database/migrations/1781000000005_make_product_image_nullable.ts @@ -0,0 +1,28 @@ +import { BaseSchema } from '@adonisjs/lucid/schema' + +/** + * Products created outside the web form have no image to upload — the MCP create_product + * tool says as much in its own description — but the column was NOT NULL, so every such + * call failed on the insert. The UI already renders a missing image (normalizeImagePath + * returns null for the legacy placeholders), so nullable is the honest shape. + */ +export default class extends BaseSchema { + protected tableName = 'products' + + async up() { + this.schema.alterTable(this.tableName, (table) => { + table.string('image_path').nullable().alter() + }) + } + + async down() { + // Backfill before restoring the constraint, otherwise the alter fails on existing rows. + this.defer(async (db) => { + await db.from(this.tableName).whereNull('image_path').update({ image_path: 'preview.png' }) + }) + + this.schema.alterTable(this.tableName, (table) => { + table.string('image_path').notNullable().alter() + }) + } +} diff --git a/database/schema.ts b/database/schema.ts index 94a962df..c602a20f 100644 --- a/database/schema.ts +++ b/database/schema.ts @@ -440,7 +440,7 @@ export class ProductSchema extends BaseModel { @column({ isPrimary: true }) declare id: number @column() - declare imagePath: string + declare imagePath: string | null @column() declare keypadId: number @column.dateTime({ autoCreate: true, autoUpdate: true }) diff --git a/inertia/composables/use_inline_edit.ts b/inertia/composables/use_inline_edit.ts index 5f2a291d..bb624644 100644 --- a/inertia/composables/use_inline_edit.ts +++ b/inertia/composables/use_inline_edit.ts @@ -22,10 +22,18 @@ export function useInlineEdit({ entityPrefix, updatePath, getEditValues, + isValid, }: { entityPrefix: string updatePath: (id: number) => string getEditValues: () => Record + /** + * Guard checked before the request goes out. Without it a cleared field is submitted as + * an empty string, the bodyparser turns it into null, the optional validator lets it + * through as "not submitted" — and the row silently keeps its old value while the UI + * closes the editor as if the change had been saved. + */ + isValid?: () => boolean }) { const editingId = ref(null) @@ -43,6 +51,7 @@ export function useInlineEdit({ function saveEdit() { if (!editingId.value) return + if (isValid && !isValid()) return router.put(updatePath(editingId.value), getEditValues(), { preserveState: true, onFinish: () => (editingId.value = null), diff --git a/inertia/pages/admin/allergens/index.vue b/inertia/pages/admin/allergens/index.vue index 5f3df2a7..7c3dc78e 100644 --- a/inertia/pages/admin/allergens/index.vue +++ b/inertia/pages/admin/allergens/index.vue @@ -57,6 +57,7 @@ const { editingId, getEditInputId, startEdit, saveEdit, cancelEdit, focusCreateI entityPrefix: 'admin-allergen', updatePath: (id) => `/admin/allergens/${id}`, getEditValues: () => ({ name: editName.value }), + isValid: () => editName.value.trim().length > 0, }) function handleStartEdit(row: AllergenRow) { diff --git a/inertia/pages/admin/categories/index.vue b/inertia/pages/admin/categories/index.vue index c658d9b6..ee5b2aba 100644 --- a/inertia/pages/admin/categories/index.vue +++ b/inertia/pages/admin/categories/index.vue @@ -62,6 +62,7 @@ const { editingId, getEditInputId, startEdit, saveEdit, cancelEdit, focusCreateI entityPrefix: 'admin-category', updatePath: (id) => `/admin/categories/${id}`, getEditValues: () => ({ name: editName.value, color: `#${editColor.value}` }), + isValid: () => editName.value.trim().length > 0, }) function handleStartEdit(cat: CategoryRow) { diff --git a/inertia/pages/admin/music/index.vue b/inertia/pages/admin/music/index.vue index 57815e68..0ff7a4cb 100644 --- a/inertia/pages/admin/music/index.vue +++ b/inertia/pages/admin/music/index.vue @@ -1,5 +1,5 @@