Good code: name the hatch and ship the deny test

CWE-79 still ranks first on MITRE’s 2025 CWE Top 25. CWE-89 is second. A loop that adds prices never reviewed GET /invoices/:invoiceId.

Pair this page with the 2026 checklist for the control names, IDOR for the scoped row, and injection for the bind. Stay here when the fight is what “good” is allowed to mean in review.

Style is not the bar

I opened the 2025 CWE Top 25 table. Rank 1 is Cross-site Scripting. Rank 2 is SQL Injection. Rank 4 is Missing Authorization, CWE-862. Rank 24 is Authorization Bypass Through User-Controlled Key, CWE-639. None of those rows score a file on prettier. None of them score a folder on feature-sliced layout.

They help a sprint. They do not tell Postgres which columns belong in WHERE. They do not tell the HTML writer what to do with displayName that contains <em>.

OWASP Top 10:2025 still leads with A01 Broken Access Control. A05 is Injection. A03 is Software Supply Chain Failures. That list is awareness. It is not a merge bar. A merge bar is a symbol the reviewer can search, plus a status the suite can fail on.

Helmet 8.3.0 published on 12 July 2026 writes response headers. Keep it. Do not count it as queryInvoice. Zod 4.4.3 published on 4 May 2026 parses a body. Keep it. A Zod pass that later concatenates displayName into SQL has not finished.

Dangerous work keeps a loud name

Readable, for this page, is not “a junior can guess the cart.” Readable is: a reviewer can tell from the call site whether untrusted data is being treated as data or as grammar. A function named render, handle, format, or sanitize that writes HTML is a quiet hatch. A function named escapeHtml is loud. A function named queryInvoice that takes two keys is loud. A function named load that takes one id is quiet.

Identifiers stay invoiceId, userId, and displayName for every snippet below. If a helper accepts only invoiceId, the name must not pretend it authorized the row. Call it loadInvoiceByIdUnchecked if you truly need the raw fetch for a job that already proved canInvoice. Prefer not needing it. The loud name is the fallback when you cannot delete the one-key load.

// Loud on purpose. Do not alias this to load().
async function loadInvoiceByIdUnchecked(invoiceId) {
  const { rows } = await pool.query(
    "SELECT id, user_id, display_name, cents FROM invoices WHERE id = $1",
    [invoiceId]
  );
  return rows[0] || null;
}

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]);
}

loadInvoiceByIdUnchecked is the named fallback. The suffix is the warning. A wrapper named getInvoice that calls the unchecked load and forgets canInvoice is how a cleanup ships a miss. The handler below calls canInvoice first, then the scoped select, and never the unchecked load.

A green suite that never denies

Tested, for this page, is not coverage. Coverage counts lines the happy path already walks. canInvoice returning false is a branch. If no fixture takes that branch, the badge can still look fine. ASVS 5.0.0 went live on 30 May 2025. Chapter work is a requirement list. The merge bar on a working branch is the status you expect when the miss returns.

Seed two accounts. Seed one invoice each. Log in as user A. Request user B’s invoiceId. Expect 404. Expect no cents field. 404 instead of 403 so the existence of the other row is not a side channel you did not design. If the product already documents 403 for a known id, test 403. Pick one. Do not return 200 with an empty object.

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("profile keeps markup as text", async () => {
  const agentA = await loginAs("userA");
  await setDisplayName(userA, "<em>x</em>");
  const page = await agentA.get("/me");
  expect(page.text).toContain("&lt;em&gt;x&lt;/em&gt;");
  expect(page.text).not.toMatch(/<em>x<\/em>/);
});

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

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 HTML case is the same idea at the other sink: <em> stays text unless that field is the one named HTML sink you already review.

Hatches that look like helpers

A hatch is a path that skips a control you already claimed. The polite ones land in “debug,” “tmp,” “legacy,” and “just for staging.” They survive because they look like tools. They fail closed only if CI can see them.

The common Express shape is a query that impersonates. Another is a header that skips auth when NODE_ENV is not the string you think. Process environment in a container image is often production and still wrong if a build arg leaked development. Do not key a skip off a string you hope is set. Key it off a function that does not exist in the production bundle.

// BAD: ships if anyone forgets to strip it
// app.get("/debug/as", (req, res) => {
//   req.session.userId = String(req.query.user || "");
//   res.redirect("/");
// });

// BAD: NODE_ENV is not a lock
// if (process.env.NODE_ENV !== "production" && req.get("x-skip-auth")) {
//   req.session.userId = req.get("x-skip-auth");
// }

function rejectHatch(req, res, next) {
  if (req.path.startsWith("/debug")) {
    res.status(404).send("Not found");
    return;
  }
  if (req.get("x-skip-auth")) {
    res.status(404).send("Not found");
    return;
  }
  next();
}

app.use(rejectHatch);

rejectHatch is the named fallback when you cannot prove the debug file is gone. Prefer deleting the route. The function exists so a reviewer can grep one symbol. Pair it with a CI line that fails the job if /debug, x-skip-auth, or asUser comes back.

# ci-hatch-greps.sh
set -e
if rg -n "/debug/as|x-skip-auth|asUser|skipAuth" \
  --glob '!node_modules' --glob '!**/*.test.*' --glob '!ci-hatch-greps.sh'; then
  echo "hatch string in app code"
  exit 1
fi

A03:2025 is the supply-chain row. A floating caret on a request-path package is a hatch you did not write. npm ci in CI. Pin the lockfile. I opened the public timeline for the 8 to 9 September 2025 chalk, debug, and strip-ansi incident. The lesson here is a lockfile pin you can fail the build on, not a feeling about “trusted maintainers.”

Three symbols in one handler

Readable, tested, no hatches collapse into one request. Parse the id. Prove the row belongs to the session. Bind the select. Encode the name if the response is HTML. There is no fourth slogan.

GET /invoices/:invoiceId
Cookie: __Host-session

HATCH  if query.debug: set userId
       SELECT * WHERE id = invoiceId
       send displayName raw
         |
OK     parse invoiceId
       canInvoice(userId, invoiceId) or 404
       queryInvoice binds both keys
       escapeHtml(displayName)
         |
OUT    404 or text, never raw markup

function escapeHtml(s) {
  return String(s)
    .replaceAll("&", "&")
    .replaceAll("<", "&lt;")
    .replaceAll(">", "&gt;")
    .replaceAll('"', "&quot;");
}

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

app.get("/invoices/:invoiceId", async (req, res) => {
  const { userId } = req.session;
  const { invoiceId } = req.params;
  if (!userId) {
    res.status(401).send("Unauthorized");
    return;
  }
  if (!(await canInvoice(userId, invoiceId))) {
    res.status(404).send("Not found");
    return;
  }
  const row = await queryInvoice(invoiceId, userId);
  if (!row) {
    res.status(404).send("Not found");
    return;
  }
  res.set("Content-Type", "text/html; charset=utf-8");
  res.send(`<p>${escapeHtml(row.display_name)}</p>`);
});

queryInvoice does two jobs on purpose. The placeholders stop CWE-89. The user_id column stops CWE-639. A UUID in the URL is not a lock. ORDER BY still cannot take a bind. Map a token such as cents to a column you wrote. sortColumn is that map.

const SORT = { cents: "cents", created: "created_at" };

function sortColumn(token) {
  return SORT[token] || "created_at";
}

async function listInvoices(userId, token) {
  const col = sortColumn(token);
  const { rows } = await pool.query(
    `SELECT id, display_name, cents FROM invoices WHERE user_id = $1 ORDER BY ${col} ASC`,
    [userId]
  );
  return rows;
}

The template literal interpolates col only after sortColumn returned a key you wrote. The client string never reaches SQL. If you cannot point at sortColumn next to every ORDER BY, you do not have that control yet.

Prove the definition this week

You are not running a pentest. You are proving the symbols exist and that a miss returns 404. Work on your own origin with fixtures you own.

1. Search the repo for escapeHtml, textContent, or the template engine’s default escape. A raw string-built <p> without that call is the XSS ticket.
2. Search for canInvoice or WHERE id = $1 AND user_id = $2. A handler that loads by invoiceId alone is the IDOR ticket.
3. Search for /debug, x-skip-auth, and asUser. A hit in app code is the hatch ticket.
4. Run the three tests above in CI. A missing file is a red job, not a skip.
5. Copy as cURL a read you already make. Swap in the other fixture’s id. Keep the cookie on your machine. Expect 404.

rg -n "innerHTML|dangerouslySetInnerHTML|Sequelize\\.literal|whereRaw|/debug/as|x-skip-auth" \
  --glob '!node_modules'

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

A 200 on that replay means canInvoice never ran or returned true for the wrong pair. That is the ticket. The sibling guides are how you finish the sink. This page only answers whether the work is allowed to be called good.