The capability check ran for employees and skipped the owner, who chose the site himself

Nothing in this system stopped a till sale being booked against a site that has no till. The stock comes out of a production store that never sells anything, and the sale lands in that store’s day book. I found it reading the middleware rather than from a support ticket, which is the only lucky part of it.

The route was gated. Every sales endpoint carried a middleware whose name says it requires the active site to have the point-of-sale capability, and the middleware works. It reads the site, checks that the capability array contains the required value, and returns 403 when it does not.

It also returns early — with no checks at all — when the caller is an account owner.

What the middleware actually does

The logic is three steps. Look up whether this user is an owner of this organisation. If so, call next() and stop. If not, require a site identifier in a request header, confirm the user is mapped to that site, then confirm the site has the required capability.

The intent behind the early return is reasonable and it is stated in the code: owners are not restricted to one site, so demanding a site header from them would break every listing screen they open. That reasoning is sound for a read. It was applied to every verb.

So for an owner, the middleware sets no site on the request at all. Which raises the question the handler then has to answer: if the request scope was not established by the middleware, where does the sale’s site come from?

From the request body. The handler reads a site identifier out of the posted JSON and falls back to it whenever the middleware did not set one — which, for owners, is always.

That value is never checked against anything. Not against the organisation, not against the capability the route claims to require, not against whether the site exists. It is passed straight into the sale record and into the inventory lookup that decrements stock.

Why it looked safe

I assumed at first that this was a straightforward missing check, and that the fix was to validate the body value the way the header value is validated. It is more than that, and the shape is worth naming because it recurs.

The permission check and the scope resolution were separated, and only one of them ran. The middleware exists to do two jobs at once: decide whether this caller may act here, and establish here. When it short-circuits, it skips both, and the second one is silently taken over by the request body. Nothing in the handler signals that its scope arrived from an untrusted place, because the same variable name holds both cases:

const siteId = req.siteId || bodySiteId;

One of those two is a value the server derived and verified. The other is a value the client sent. After that line they are indistinguishable, and every use downstream is written against the merged variable.

That line appears, in one form or another, in most of the module’s handlers. In the read paths the fallback is a query parameter instead of a body field, with the same property.

The three failures, separately

It is worth separating them, because they have different fixes.

A capability requirement that admits an exception is not a requirement. The site’s capability list is a statement about physical reality — this location has a counter and a card machine, that one has a press. An owner’s authority does not create a till. Seniority is a reason to skip a permission check, never a reason to skip a feasibility check, and the two were implemented in one function so they shared one exit.

Scope must never be resolvable from the request body. If a value determines which rows get written, it has to be derived by the server or validated as thoroughly as if it had been derived — which means, at minimum, confirming it belongs to the caller’s organisation and satisfies the same constraints the route advertises. A fallback that reads req.body is a second, undocumented API for choosing scope, and it bypasses the first one.

An early return in shared middleware is invisible at the call site. A route declares that it requires the till capability. Reading the route file, that is what it says. You have to open the middleware to discover that the statement has an exception, and nobody opens the middleware, because its name reads like a specification.

What replaced it

The middleware was split. One function answers “may this user act in this organisation” and applies to everyone. A second function resolves the site and returns it, refusing to fall back to client-supplied values on any write path — an owner who does not send a site header on a write gets a 400 telling them to choose a site, which is a worse experience for exactly one screen and a correct one everywhere.

The capability check was moved out of the authorisation middleware entirely and into the handler’s validation, next to the check that the products exist and the quantities are positive, because that is what it is. It is a domain rule about the site, not a fact about the user, and putting it in the authorisation layer is what let a rule about users switch it off.

The write paths lost the body fallback. The read paths kept an equivalent of it, deliberately: an owner filtering a list by site is choosing a filter, and a filter that names a site outside their organisation returns nothing, because the organisation predicate is applied first and separately. That distinction — a filter narrows an already-scoped set, it never widens it — is the one that makes the read case safe and the write case not.

The rule

Authorisation decides who; scope decides where; do not let one function do both, because the shortcut in one becomes the hole in the other.

And the one to check in your own code this week: search for every place a scope identifier is read with || against something from the request. Each of those is a line where a verified value and an unverified value became the same variable. In this codebase there were dozens, all written by people being helpful about a genuine usability problem, and not one of them was wrong on the day it was added.