Secure coding review: name the hatch, ship the deny test

Secure coding done right is a merge that cannot land without the check.

ASVS 5.0 is a catalog of those checks. A pull request that adds a handler should add the fail-path test in the same diff. A standard that nobody gates is a PDF.

The usual mistake is a training day and a repo that still concatenates SQL on the export route.

This page is how to make ASVS-style requirements visible in the next merge, not in the next audit.

OWASP published ASVS 5.0.0 live on 30 May 2025 at Global AppSec EU Barcelona. A merged pull request that adds GET /invoices/:invoiceId and one Jest happy path has not verified V4 Access Control. User B’s fixture can still return 200 with cents.

Done right on a working branch is a reviewer who can name the miss, plus a test that stays red when the miss returns. Pair this page with the 2026 checklist for the control names, IDOR for the scoped query, injection for the bind, and input validation for the door check the test should also hit.

A green happy path is not done

Most teams already have a test that creates an invoice as user A and reads it back. That test proves the feature. It does not prove the boundary. CWE-639 is Authorization Bypass Through User-Controlled Key. Rank 24 on MITRE’s 2025 CWE Top 25. The happy path never sends user B’s key.

CWE-89 is the same shape on the query string. A test that searches for acme and sees one row has not proven a bind. The SQL log still might show a concatenated displayName. CWE-20 is the door: a body that is an object where you expected a string. Zod 4.4.3 published on 4 May 2026. Parse on the handler. The client copy is UX.

ASVS 5.0.0 is the requirement list I open when a reviewer says "looks fine." Chapter 1 is Encoding and Sanitization. Chapter 8 is Authentication. I am not claiming every team must certify a level. I am claiming a diff that opens a hatch needs the matching deny test before merge.

What the reviewer asks

One hatch per comment thread. Do not dump a 40-item PDF on a three-line change. ASVS how-to-reference note: identifiers look like v5.0.0-1.2.5 so the chapter does not drift. Use that form if you cite a requirement. Use a question if you do not.

1. Which untrusted value did this diff start accepting? Query, body, header, cookie, file name, webhook.
2. Which interpreter or store does that value reach? SQL, HTML, shell, object store, session row.
3. Where is the allowlist or schema? Type, length, enum. A comment that says "sanitized" is not a schema.
4. Where is the authorization predicate? canInvoice(userId, invoiceId) or the same WHERE with both keys.
5. Where is the deny test? 401, 403, 404, or 400. A snapshot of the happy HTML is not that test.

PR GET /invoices/:invoiceId
 + test: user A reads own row 200

ASK which hatch? object id
 which predicate? canInvoice
 which deny? A reads B

FAIL no deny test block merge
PASS canInvoice + 404 fixture merge

Name the miss before you nit the style

Reviewers waste a day arguing labels. A default password on an admin console is CWE-306, Missing Authentication for Critical Function, rank 21 in 2025. An authenticated user reading another tenant’s row is CWE-639. Both are bad. They are not the same patch. The first patch is disable the default and require a factor. The second patch is canInvoice.

Write the miss in the ticket title the way the test will assert it. "A can read B’s invoiceId" is a test. "Improve security" is not. "Add helmet" is a header change. It does not close V4.

Tests that expect 403 or 404

Identifiers stay userA, userB, invoiceId, and canInvoice for the rest of this page. The helper is the named fallback. Every handler that touches an invoice calls it. The test calls the handler, not the helper in isolation, so a future rewrite that skips canInvoice still fails.

async function canInvoice(userId, invoiceId) {
 const { rows } = await pool.query(
 "SELECT 1 FROM invoices WHERE id = $1 AND user_id = $2",
 [invoiceId, userId]
 );
 return Boolean(rows[0]);
}

async function getInvoiceHandler(req, res) {
 const { invoiceId } = req.params;
 const { userId } = req.session;
 if (!userId) {
 res.status(401).send("Unauthorized");
 return;
 }
 if (!(await canInvoice(userId, invoiceId))) {
 res.status(404).send("Not found");
 return;
 }
 const { rows } = await pool.query(
 "SELECT id, cents FROM invoices WHERE id = $1 AND user_id = $2",
 [invoiceId, userId]
 );
 res.json(rows[0]);
}

404 instead of 403 so the existence of user B’s row is not a side channel you did not design. If your product already documents 403 for a known id, keep that contract and test 403. Pick one. Do not return 200 with an empty object.

Two accounts, one deny

Seed two accounts. Seed one invoice each. The cross-account case is eight lines if the harness can log in.

test("user A cannot read user B invoice", async () => {
 const agentA = await loginAs("userA");
 const denied = await agentA.get("/invoices/" + invoiceIdOfB);
 expect(denied.status).toBe(404);
 expect(denied.body.cents).toBeUndefined();
});

test("search binds displayName", async () => {
 const reader = await loginAs("userA");
 const found = await reader.get("/search").query({ q: "O'Brien" });
 expect(found.status).toBe(200);
 const sql = lastQueryText(); // from your pg logger in test
 expect(sql).toMatch(/\$1/);
 expect(sql).not.toMatch(/O'Brien/);
});

test("object where string expected is 400", async () => {
 const writer = await loginAs("userA");
 const badType = await writer.post("/invoices").send({ displayName: { $gt: "" } });
 expect(badType.status).toBe(400);
});

The bind test needs a query logger you only enable in test. If you cannot hook the driver, assert that a quote in displayName still returns the row and does not 500. That is weaker. Prefer the log. The object-for-string case is the Mongo operator story on a JSON body. Reject before it becomes a filter. The input-validation sibling is that parse.

For HTML, render a profile whose displayName is <em>x</em>. Expect escaped text in the body, not italics, unless that field is the one named HTML sink with a sanitizer you already review.

CI greps that fail the build

Reviewers miss hatches that land in a file nobody opened. A grep in CI is the cheap net. It is not the audit. Semgrep or a one-line rg that fails the job is enough to make the hatch visible.

# ci-deny-greps.sh fail the job on a hit in app code
set -e
rg -n "Sequelize\\.literal|whereRaw|queryRawUnsafe|dangerouslySetInnerHTML" \
 --glob '!node_modules' --glob '!**/*.test.*' && exit 1 || true
rg -n "app\\.get\\(.*/:invoiceId" --glob '!node_modules' | while read -r line; do
 echo "$line"
done
# Human still checks canInvoice on each of those routes.
# The grep only makes the hatch list.

A hit on literal is not an automatic vulnerability. It is an automatic review comment: show the allowlist. A hit on dangerouslySetInnerHTML is the same: show the sanitizer and the test that keeps <em> inert if that field is not the HTML sink.

npm ci in the same job. A floating latest on a request-path package is A03:2025. The September 2025 chalk and debug incident is why a lockfile pin is a review item.

A review that only reads the TypeScript types has not read canInvoice. Types say invoiceId is a string. They do not say it belongs to userId. Put the predicate in the handler, then put the deny status in the suite. If the team uses a shared request helper, make that helper attach the test user’s cookie. A helper that only hits happy URLs will hide the miss forever.

This is not a pentest ticket

A reviewer is not a red team. You do not need a foreign lab string. You need seeded accounts and your own status codes. Copy as cURL from DevTools on a request you already make. Replay against your own origin. Change invoiceId to the other fixture. Expect 404. That is a test you can check in.

Do not paste attack strings into a customer tenant. Do not ask a junior to "try SQL injection" on production. The bind assertion above is the proof. The cross-account case is the other proof. If you want an external review later, that is a scoped engagement with written limits. This page is the merge bar.

curl -sS -D - -o /tmp/b.json \
 -H "Cookie: __Host-session=USER_A_SID" \
 "https://your-app.example/invoices/${INVOICE_ID_OF_B}"
# Expect: HTTP/2 404
# Expect: no cents field in /tmp/b.json

Run that from the same machine you develop on. Keep the cookie in your shell history off shared logs. Rotate the fixture password when you are done if the sid was a real session.