Reviewing a pull request for access control

Start with the rule, not the diff

A diff shows lines. The thing being changed is a rule: who may touch this object, and where that is enforced. Open the rule before the diff. Which route serves the request, which middleware or dependency gate sits in front of it, which query fetches the object, and where the actor id comes from. Then read the change against that map. A one-line edit to a query is small; the rule it rewires may not be.

Two questions carry most of the weight. On this route, who is the actor and where does the gate for it live? On this lookup, who names the key, the session or the request? The first is function-level access control, the second is object-level. The dangerous diffs are usually the ones that quietly move the answer to one of the two.

Four shapes worth stopping for

New handler, no visible gate. A route or RPC added without a role check or middleware in front of it. The gate may live in a file the diff does not show, which is exactly why the question comes before the approval: if you cannot point at the gate, treat it as absent. This is the function-level shape, under-guarded routes, within the broader CWE-284 category, Improper Access Control, and the OWASP API5:2023 entry, Broken Function Level Authorization.

A lookup the caller names. An id in the path or body reaching a query with no owner scoping attached, so the object belongs to another tenant and the request reads it anyway. This is the object-level shape, the OWASP API1:2023 entry, Broken Object Level Authorization, whose related weaknesses include CWE-639, Authorization Bypass Through User-Controlled Key. The tell is boring and everywhere: a fetch by id with the tenant binding in a different clause, or not in the query at all.

A gate that moved. Middleware dropped from a mount, a role list grown by one, a check that used to sit in a dependency now duplicated in one caller and skipped in another. None of those lines looks wrong in isolation; each widens who passes. The question is not whether the line is correct, it is whether the rule after the diff is the same rule the route enforced before it.

A new trust flow. An internal endpoint re-exposed through a public handler, a service-to-service call now reachable from a user session, a debug route shipped alongside the feature. Trust boundaries never arrive in a diff marked as such; they arrive as an actor type that changed.

Evidence before approval

The approval should be able to state three things, and so should any comment that raises this surface: the rule in question (who may touch the object, and where that is enforced), the evidence (file and lines), and the actor (which role or session the route assumes). If one of the three cannot be written down, the review is not finished. It is the same standard a finding has to meet to be worth a human's time.

The bar applies to the fix, too. A check added inline in the handler can be correct today and wrong after the next refactor, if the middleware was the real enforcement point. A fix expectation should name the enforcement location, not only the missing check.

When the diff is agent-generated and the queue is long

Coding agents produce clean-looking diffs at a pace review budgets do not match, and access-control bugs ride in on well-formatted pull requests: tests pass, style is consistent, and the ownership check the old code had is simply gone. The procedure above is what scarce human review time is for: gates, tenant bindings, object references, trust flows.

The fix prompt closes the loop: the evidence, the rule, the boundary, and a human review before merge, so the agent fixes the gate and stops. Aevral's opt-in PR security review add-on applies the same reading to pull-request events and leaves at most two findings per review, advisory and never blocking, because a review signal that floods its reader stops being read. The whole-repo scan applies the same reading across the repository, not only the diff.