Refactoring for security: make every hatch grep-able

A security refactor is moving a repeated check into one place so the next route cannot skip it.

Six handlers that each compare invoice.ownerId will grow a seventh that forgets. The bug is not the comparison. The bug is the copy-paste.

The usual mistake is a rewrite for style that leaves the authorization inline, or a rewrite that centralizes it and then adds a raw export path beside the new helper.

This page is how to see that seventh handler coming, and the tests that must travel with the helper.

Six handlers in invoice.js each tested invoice.ownerId === actor.userId inline. The seventh, exportInvoice, never got the copy. Bob’s GET returned Alice’s cents. A refactor that does not put that test in one symbol is how the next hatch ships the same miss.

This page is the move that makes the next review a search. Pair it with the IDOR guide for the helper body, the injection guide for the hatch list, and the secure coding checklist for the control name you write on the ticket.

Six copies, one missing line

CWE-639 is Authorization Bypass Through User-Controlled Key. The happy path never sends Bob’s key. The scattered path sends it to the one handler you refactored last. Duplication is not a style nit here. It is a seventh door.

That story hid the real miss: the check lived in too many places, so one place forgot it. You do not need a superclass to get that wrong. You need getInvoice, patchInvoice, deleteInvoice, listLines, voidInvoice, sendInvoice, and then exportInvoice added on a Friday.

// BAD: the seventh handler never got the paste
async function exportInvoice(req, res) {
 const actor = requireUser(req);
 if (!actor) return res.status(401).end();
 const invoice = await findById(req.params.invoiceId);
 if (!invoice) return res.status(404).end();
 return res.send(await renderCsv(invoice.id));
}

A reviewer who reads only the new function sees a 404 for a missing row. They do not see that Alice’s row is present and Bob is the actor. The test that would have caught it is the one that logs in as Bob and asks for Alice. That test cannot be written seven times and stay honest. It wants one helper.

Find the scatter before you invent a new name. Grep the comparisons, then the loaders.

rg -n "ownerId\\s*===|invoice\\.userId|req\\.params\\.invoiceId" src
rg -n "findById\\(|pool\\.query\\(|Sequelize\\.literal|whereRaw" src

Every hit that is not already canInvoice or queryInvoice is a candidate for the move. A worker under src/workers that loads by id is in the same list. A billing webhook that trusts payload.invoiceId is in the same list. If the search returns twenty files, you still move to one symbol. You do not schedule a rewrite because the hit count is large.

Pull the string hatch into one name

The other scatter is SQL. Prisma Client, Sequelize, Knex, and TypeORM parameterize the builder path. The failure is the method you reach for when the builder cannot express the query. Those methods have names. They grep after you stop wrapping them in local helpers that hide the string.

A string that pastes an escaped value into SQL is still a template. A client that sends the statement and the values on separate wires is a bind. A shop that builds "ORDER BY " + sort inside exportInvoice and again inside listInvoices will patch one and miss the other. One queryInvoiceList with an allowlist is the refactor.

// src/db/queryInvoiceList.js
const SORT = {
 created: "created_at",
 cents: "cents",
};

async function queryInvoiceList(actor, sortToken) {
 const column = SORT[sortToken] || SORT.created;
 const { rows } = await pool.query(
 `SELECT id, owner_id AS "ownerId", cents
 FROM invoices
 WHERE owner_id = $1
 ORDER BY ${column} ASC`,
 [actor.userId]
 );
 return rows;
}

column comes from a map you wrote. The token never reaches SQL. Admin list views that must see every row call a second named function, queryInvoiceListAll, that still uses the map and still binds nothing from the client into grammar. Do not hide Sequelize.literal(req.query.sort) inside smartSort. That is the opposite of grep-able.

One canInvoice, not seven owner tests

Move the object rule first. It is the miss that never greps as literal. After the move, every handler that loads an invoice calls the same symbol. A new export that forgets the call is a review comment, not a hunt through six files.

// src/auth/canInvoice.js
function canInvoice(actor, invoice) {
 if (!actor || !invoice) return false;
 if (actor.role === "admin") return true;
 return invoice.ownerId === actor.userId;
}

module.exports = { canInvoice };

// src/routes/invoice.js
const { canInvoice } = require("../auth/canInvoice");
const { queryInvoice } = require("../db/queryInvoice");

async function exportInvoice(req, res) {
 const actor = requireUser(req);
 if (!actor) return res.status(401).end();
 const invoice = await queryInvoice(req.params.invoiceId);
 if (!canInvoice(actor, invoice)) return res.status(404).end();
 return res.send(await renderCsv(invoice.id));
}

async function getInvoice(req, res) {
 const actor = requireUser(req);
 if (!actor) return res.status(401).end();
 const invoice = await queryInvoice(req.params.invoiceId);
 if (!canInvoice(actor, invoice)) return res.status(404).end();
 return res.json(invoice);
}

// src/db/queryInvoice.js
async function queryInvoice(invoiceId) {
 const { rows } = await pool.query(
 "SELECT id, owner_id AS \"ownerId\", cents FROM invoices WHERE id = $1",
 [invoiceId]
 );
 return rows[0] || null;
}

Identifiers stay canInvoice, queryInvoice, queryInvoiceList, and exportInvoice. Do not invent checkAccess in one file and assertOwner in another. Two names is how the seventh handler calls neither.

Mass assignment is the same shape. If PATCH accepted ownerId in six places, pull a parseInvoicePatch that strips it. Input validation is the door. The refactor is making that door one function.

Grep after the move, not before

The grep is useless while the check is a comparison pasted by hand. After canInvoice exists, the job can fail a handler that loads an invoice and never mentions the helper. That second script is the named fallback for object rules, which have no method name of their own.

# tools/hatch_grep.sh
set -euo pipefail
PAT='Sequelize\.literal|whereRaw|\$queryRawUnsafe|knex\.raw\('
rg -n -e "$PAT" --glob '!node_modules' --glob '!allow-hatches.txt' src \
 | sort > /tmp/hatch-hits.txt
touch allow-hatches.txt
sort -u allow-hatches.txt > /tmp/hatch-allow.txt
if ! comm -13 /tmp/hatch-allow.txt /tmp/hatch-hits.txt | grep -q.; then
 exit 0
fi
echo "new hatch string. add a reviewed allow line or remove the call"
comm -13 /tmp/hatch-allow.txt /tmp/hatch-hits.txt
exit 1

# tools/check_can_invoice.py
import pathlib, re, sys

root = pathlib.Path("src/routes")
missing = []
for path in root.rglob("*.js"):
 text = path.read_text()
 if "queryInvoice(" not in text:
 continue
 if "canInvoice(" not in text:
 missing.append(str(path))
if missing:
 print("queryInvoice without canInvoice:", missing)
 sys.exit(1)

check_can_invoice.py is the named fallback. It exits 1 when a route file talks to queryInvoice and never calls canInvoice. A worker that loads invoices must live under src/routes or you extend the walk. Do not exclude exportInvoice because it is a CSV. That was the seventh door.

SCATTER ownerId tests in 6 handlers
 exportInvoice has none
 |
 v
NAME canInvoice(actor, invoice)
 queryInvoice(id) uses $1
 |
 v
GREP canInvoice
 Sequelize.literal whereRaw
 miss -> PR red
 |
 v
TEST asBob export -> 404

OWASP ASVS 5.0.0 shipped on 30 May 2025. The object chapter is the requirement you write on the ticket that created canInvoice. The query chapter is the ticket that created queryInvoiceList. A 40-file rewrite with no those two names is still a scatter, just a newer one.

When a rewrite is the honest call

Be specific. Rewrite when you cannot name the helper because the ownership rule is not in the tree. If ownerId is sometimes a user, sometimes a tenant, and sometimes a string in a JSON blob, stop pasting. Write one rule, then point every loader at it. That may delete files. It is still a refactor of the rule, not a greenfield app.

Do not rewrite because the class diagram is ugly. Do not rewrite because a 2012 piracy number felt persuasive. The 2012 BSA piracy figure is not a control. Do not quote it as a reason to rewrite. The control is whether rg canInvoice src hits every loader.

Admin overrides belong inside canInvoice, not as a second comparison in the handler. If support needs a break-glass role, add it to the helper and test it. A comment that says “admin skipped here” next to exportInvoice is how the seventh door comes back. The helper is allowed to be ten lines. It is not allowed to be seven copies.

Bridge patterns that wrap a third-party pay API are fine when the new code is the only caller of markPaid and the old trigger is deleted in the same pull request. A bridge that leaves both triggers live is two copies again. Delete the old call in the same diff or do not claim the move landed.

Prove the move yourself

You are proving the seventh handler cannot ship without the helper, and that a new literal still turns the job red. Use a repo you own.

1. Commit canInvoice.js, queryInvoice.js, hatch_grep.sh, and check_can_invoice.py.
2. Open a PR that adds exportInvoice using queryInvoice and no canInvoice. Expect the Python script to exit 1.
3. Add the call. Log in as Bob. Request Alice’s export. Expect 404.
4. Add Sequelize.literal(req.query.sort) in listInvoices. Expect hatch_grep.sh to exit 1.
5. Replace it with queryInvoiceList and the SORT map. Expect green, then grep the names.

rg -n "canInvoice|queryInvoice|queryInvoiceList|Sequelize\\.literal" \
 --glob '!node_modules'

If allow-hatches.txt grows a smartRaw line every week, the move failed. If the Python check is skipped on src/workers, the queue is the eighth door. Identifiers stay canInvoice, queryInvoice, queryInvoiceList, and hatch_grep.sh.

Reviewers ask two questions on a hatch diff. Which symbol did you add. Which search will turn red if the next person pastes a string. If the answer is “we cleaned up the classes,” send the card back. Cleanup that does not change the search is furniture. Furniture is fine on a tidy-up ticket. It is not the security move this page is for. A pull request that only renames Roles to StaffRole and leaves seven owner lines is an inheritance rename. Decline it.