I pointed an AI reviewer at a 2019 order service: 63 comments, and none of them was the IDOR

Last month I got read access to an order service that had been running since 2019. Node, Express, Postgres, around 64k lines, three developers in its history and none of them still at the company. The team wanted a second opinion before a batch of changes to the checkout flow.

I ran an AI reviewer over the branch first. Half curiosity, half because everybody has been arguing about whether AI code review is a bubble, and I wanted a number of my own instead of a take.

It came back with 63 comments across 12 files.

Sorting the 63

38 were naming, formatting, or "consider extracting this into a helper"
11 were missing null checks, and 9 of those could not happen, because the value came from a NOT NULL column
8 suggested wrapping code in a try/catch that was already inside a try/catch
4 were about actual behavior, and 2 of those I would have written myself: a date compared as a string, and a retry loop with no ceiling

So something around 3% of the output was worth a human's attention. That ratio alone is not damning. Linters have terrible ratios too and nobody cares, because a linter costs nothing to skim.

The part that bothered me is what it stayed quiet about.

The comment it did not write

Here is the endpoint, cleaned up and renamed:

// src/routes/orders.js
router.get('/orders/:id', requireAuth, async (req, res) => {
  const order = await db.oneOrNone(
    'select * from orders where id = $1',
    [req.params.id]
  );

  if (!order) return res.status(404).json({ error: 'not_found' });
  return res.json(order);
});

requireAuth resolves the session and puts req.user in place. It never asks whether that order belongs to the caller. The ids are sequential integers. Three curl calls with a valid cookie and a different number and you are reading another customer's address, items and total.

The AI's comment on that exact file was: "Consider extracting the 404 response into a shared helper for consistency." Accurate. Useless.

I checked how widespread it was with ripgrep before doing anything else:

$ rg -n "where id = \$1" --glob '!test/**' src/
src/routes/orders.js:41:    'select * from orders where id = $1',
src/routes/invoices.js:88:   'select * from invoices where id = $1',
src/jobs/reconcile.js:23:    'select * from orders where id = $1',

Two of the three were reachable from the browser. The third runs in a job with no request context, so it is fine.

Why a diff reviewer cannot see this

The reviewer reasons about lines that exist. A broken authorization check is an absence. There is no bad line to point at, and the thing that makes it dangerous lives somewhere else entirely: in the fact that ids are sequential, in what requireAuth chose not to do, in a middleware file the branch never touched.

Same story with the two other findings from that week. A query sitting inside a for loop that only hurts when a customer has more than a handful of shipments. A /health route that returns 200 without checking anything, which means the load balancer keeps sending traffic to a process that lost its database pool. Both are invisible in a diff and obvious when you run the system.

The fix I tried that did not work

My first move was the obvious one: give it more context. Whole file instead of hunk, then a repo map, then a system prompt that spelled it out, roughly "check every route for broken object level authorization, ids are sequential".

Told explicitly what to hunt for, it found the orders endpoint. It also flagged 7 endpoints in total, and 5 of those were wrong. They loaded data through a repository function that already scopes by customer_id, one call deeper than the route file. The model could not tell the difference between an endpoint with no ownership filter and an endpoint whose filter is two hops away.

Which is the honest summary: it runs a checklist over text. It does not know who owns what in your system, and the prompt that makes it paranoid enough to catch the real one also makes it cry wolf five times.

What sits in CI now

I stopped asking the reviewer for security opinions and wrote a test with two tenants:

test('an order is not readable by another customer', async () => {
  const alice = await seedCustomer();
  const bob = await seedCustomer();
  const order = await seedOrder(bob, { total: 1290 });

  const res = await request(app)
    .get(`/orders/${order.id}`)
    .set('Authorization', bearer(alice));

  expect(res.status).toBe(404);
});

404 and not 403, so the endpoint does not confirm that the id exists.

One test only covers one route, and the point was the other 40. So there is a second script that walks the Express router stack and prints every path that never touches an owner filter:

$ node scripts/audit-routes.js
GET   /orders/:id              no owner filter
GET   /invoices/:id            no owner filter
POST  /orders/:id/cancel       no owner filter
GET   /orders                  scoped (customer_id)
...
3 of 41 routes unscoped

It is crude. It matches on the repository functions we consider safe, so a fourth way of loading an order would slip past it until someone adds it to the list. But it is deterministic, it runs in about 2 seconds, and it never invents a problem to look useful.

Where I land on the bubble argument

I don't think these tools are worthless. Two of those 63 comments were real, and one of them was a bug I would probably have shipped.

What I think is broken is measuring them by comments produced. Every comment spends attention, and attention runs out. After the twentieth "consider extracting", the human reviewer starts scrolling, and the one comment that mattered scrolls by with the rest. The tool got cheaper and moved the cost onto the person approving the merge.

So the number I care about now is not how much it finds. It is how much noise a real finding has to survive.

How are you drawing that line? Specifically, has anyone gotten an AI reviewer to reason about authorization across files without drowning in false positives, or did you also give up and write the boring two-tenant test?

This audit came out of client work at Revin.

DEV Community