Review this design. Every one of 40 API endpoints checks that the caller owns the requested resource by fetching it and comparing an owner field in the controller. A penetration test found one endpoint missing the check. What would you change and what would you leave alone?
Show the full answer Hide the answer
What is actually required
Every request that names a resource must establish that the caller may act on that resource. The requirement is not controversial. The design's problem is that it satisfies it 40 separate times, which means it is satisfied 39 times and the 40th is a vulnerability with no signal attached to it.
Broken object-level authorisation has sat at or near the top of the OWASP API security risks since the list was first published in 2019, for exactly this structural reason: it is an omission, and omissions do not throw exceptions. The cost of the current design is paid in review time on every new endpoint, forever.
What I would change
Move the check below the controller, into the data access path. The layer that loads a resource takes the caller's identity and applies the ownership predicate as part of the query, so an endpoint that forgets returns nothing rather than returning somebody else's row. In a relational store this is a mandatory tenant or owner predicate, optionally enforced by row-level security in the database so even ad-hoc access obeys it.
The property that matters: the insecure version must be the one that fails. A developer who forgets should get an empty result and a failing test, not a working endpoint.
Add a test that enumerates routes. A test that walks the route table and asserts every route is either annotated as public or covered by an authorisation check turns "we reviewed it" into a build failure. This is what catches endpoint 41, which will be added next month by someone who has never read the pen test.
Make identifiers unguessable as defence in depth. Sequential integers make enumeration trivial; random identifiers do not authorise anything but they remove the cheap attack. This is a supplement, never the control.
What I would remove
Remove the fetch-then-compare pattern itself. Loading the object and then deciding means the object was already read, which matters when the read is itself the sensitive action, and it invites the mistake of returning part of the object in an error message.
What I would leave alone
Leave the per-endpoint checks in place during the migration. Removing 40 checks in favour of a new mechanism, in one change, is how a whole class of authorisation is lost at once. Run both, log where they disagree, and remove the controller checks only after the disagreement log is empty for a full release.
Leave role-based checks where they are. Roles answer "may this caller call this endpoint at all", which is a different question from "may they touch this object", and it belongs at the edge. Conflating the two is how role explosion starts.
How I would argue this in the review
With the arithmetic of coverage. 40 hand-written checks with a 97.5% success rate is one vulnerability, and the rate does not improve as the API grows; it degrades, because later endpoints are written under more time pressure by people with less context. A single enforced path has one place to get right and one place to test. Then the concrete ask: one route-enumeration test, merged this week, regardless of when the larger refactor happens.
When not to centralise the check
For a handful of endpoints in a service with one resource type and one team, explicit per-endpoint checks are readable and fine. Choose the enforced path when the number of endpoints exceeds what one person can hold in their head, or when more than one team adds routes - at that point the omission becomes a matter of time rather than of care.