Spaghetti code: extract canInvoice before IDOR hides

Spaghetti code becomes a security bug when the authorization check is in some paths and not others.

A form rewrite that still concatenates SQL in one branch is injection. A user-controlled key in one of twelve handlers is IDOR. The tangle hides the skip.

The usual mistake is a rewrite for cleanliness that never adds the test that would have failed on the skipped branch.

This page is how to see the skipped branch, and why CWE-639 and CWE-89 keep pairing with messy control flow.

CWE-639 sits at rank 24 on MITRE’s 2025 CWE Top 25. CWE-89 is still rank 2.A form rewrite that still concatenates displayName and loads by invoiceId alone has not moved the risk.

Open the IDOR guide when the predicate is the whole ticket. Open the injection guide when the interpreter is the ticket. Open input validation when the body type is the ticket. This page is only the extract.

A tangle is a missed check

Spaghetti, on a security review, is not “the file is long.” It is a control that exists in one branch and vanishes in another because the reader cannot hold the whole path. CWE-862, Missing Authorization, is rank 4 in 2025. CWE-863, Incorrect Authorization, is rank 17. Both show up when the same resource is loaded two ways and only one way remembers userId.

Broken Access Control is still first. A05 Injection is fifth. Those two are the ones a tangle hides best. The check is not missing from the company wiki. It is missing from the third copy of the handler, the CSV export, the webhook, or the “quick” admin script that calls loadById.

A rewrite for taste can wait. A rewrite that creates one function the other paths must call is a control. Bridge patterns and folder moves are not that function. They can help you find it. They do not replace it.

How the object id disappears

The happy path looks authorized because the UI only links to your own rows. The tangle adds a second door: a search, a PDF, a “related invoices” include, a batch job that takes a list of ids. Each door starts as a copy. Copies drift. One copy keeps AND user_id = $2. The next copy “simplifies.”

Identifiers stay invoiceId, userId, and displayName below. The miss is CWE-639: the client chose the key, the server did not prove ownership. Rank 24 is not “rare.” It is “the other path.”

// Typical tangle: two doors, one check
async function getInvoicePage(req, res) {
 const { userId } = req.session;
 const { invoiceId } = req.params;
 if (!userId) return res.status(401).send("Unauthorized");
 const { rows } = await pool.query(
 "SELECT id, display_name, cents FROM invoices WHERE id = $1 AND user_id = $2",
 [invoiceId, userId]
 );
 if (!rows[0]) return res.status(404).send("Not found");
 res.json(rows[0]);
}

// Copied six months later for the PDF. The AND is gone.
async function getInvoicePdf(req, res) {
 const { invoiceId } = req.params;
 const row = await loadById(invoiceId);
 if (!row) return res.status(404).send("Not found");
 res.set("Content-Type", "application/pdf");
 res.send(renderPdf(row));
}

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

getInvoicePage looks fine in review. getInvoicePdf is the door the reviewer never opened because it lived under jobs/ or exports/. The extract is not “add a comment.” It is delete loadById from the request path or force it through canInvoice.

How the concat sits three files away

Injection hides the same way. The handler looks parameterized. The search helper two folders down builds a string. Teams call that helper “just a filter builder.” It is text templating. A real bind sends the statement and the values as separate messages. String add is not that, even if you wrap quotes.

Substituting an escaped value into query text is not what a client library does when it parameterizes. The control is still queryInvoice with placeholders, not a smarter concatenate.

// Hiding in util/search.js
function buildInvoiceSearch(displayName) {
 return (
 "SELECT id, display_name, cents FROM invoices WHERE display_name ILIKE '%" +
 displayName +
 "%'"
 );
}

// Looks clean at the call site
async function searchInvoices(req, res) {
 const sql = buildInvoiceSearch(req.query.q);
 const { rows } = await pool.query(sql);
 res.json(rows);
}

The call site has no $1. The util has no userId either, so this is both tickets at once. Zod 4.4.3 on 4 May 2026 can reject a giant q. That is the door. The sink still needs a placeholder and a tenant column.

async function searchInvoicesForUser(userId, q) {
 const { rows } = await pool.query(
 "SELECT id, display_name, cents FROM invoices WHERE user_id = $1 AND display_name ILIKE $2",
 [userId, "%" + q + "%"]
 );
 return rows;
}

The % wrap is still a value, not grammar. ILIKE metacharacters in q can widen a search. That is a product bug if you need literal percent signs. It is not CWE-89 if the driver sent q separately. Escape % and _ in q if literal search is the contract. Do that in a named helper, likePattern.

function likePattern(q) {
 return "%" + String(q).replaceAll("\\", "\\\\").replaceAll("%", "\\%").replaceAll("_", "\\_") + "%";
}

Extract the control, then move the rest

Refactor as a control means the first commit adds the function and switches callers. It does not mean a six-week rewrite. If a caller cannot switch today, wrap it and fail the build on new uses of the old name.

TANGLE
 pages.js getInvoicePage has userId
 pdf.js getInvoicePdf loadById only
 util.js buildInvoiceSearch string add
 jobs.js exportCsv copy of pdf

EXTRACT
 canInvoice(userId, invoiceId)
 queryInvoice(invoiceId, userId)
 searchInvoicesForUser(userId, q)
 |
 pages, pdf, csv, search all call these
 |
 loadById gone from request path

ASVS 5.0.0, 30 May 2025, is the requirement list if a reviewer wants a number on the ticket. You do not need a full level. You need V4 on every door that takes invoiceId, and encoding or a bind on every door that takes displayName.

The handler after the extract

One module owns the row. HTTP handlers stay thin. The PDF job calls the same module. The CSV job calls the same module. There is no “export exception.”

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

async function requireInvoice(userId, invoiceId) {
 if (!userId) {
 const err = new Error("unauthorized");
 err.status = 401;
 throw err;
 }
 if (!(await canInvoice(userId, invoiceId))) {
 const err = new Error("not found");
 err.status = 404;
 throw err;
 }
 const row = await queryInvoice(invoiceId, userId);
 if (!row) {
 const err = new Error("not found");
 err.status = 404;
 throw err;
 }
 return row;
}

app.get("/invoices/:invoiceId", async (req, res) => {
 try {
 const row = await requireInvoice(req.session.userId, req.params.invoiceId);
 res.json(row);
 } catch (err) {
 res.status(err.status || 500).send(err.status ? err.message : "Error");
 }
});

app.get("/invoices/:invoiceId/pdf", async (req, res) => {
 try {
 const row = await requireInvoice(req.session.userId, req.params.invoiceId);
 res.set("Content-Type", "application/pdf");
 res.send(renderPdf(row));
 } catch (err) {
 res.status(err.status || 500).send(err.status ? err.message : "Error");
 }
});

requireInvoice is the named fallback for handlers that should not each retype the 401 and 404. The PDF route can no longer “simplify.” A future admin script that imports loadById will fail the grep. Delete loadById or rename it loadInvoiceByIdUnchecked and keep it off HTTP.

test("pdf door uses the same deny as json", async () => {
 const agentA = await loginAs("userA");
 const denied = await agentA.get("/invoices/" + invoiceIdOfB + "/pdf");
 expect(denied.status).toBe(404);
});

test("search does not concatenate displayName", async () => {
 const agentA = await loginAs("userA");
 await agentA.get("/search").query({ q: "O'Brien" });
 const sql = lastQueryText();
 expect(sql).toMatch(/\$2/);
 expect(sql).not.toMatch(/O'Brien/);
});

If the PDF test is missing, the extract is a story. The JSON test alone will stay green while getInvoicePdf is still the old copy. Write one deny per door you found in the first grep.

Prove the extract this week

You are mapping doors, not attacking a tenant. Stay on fixtures you own.

1. List routes and jobs that take an invoice key: rg -n "invoiceId|:invoiceId|invoices/" --glob '!node_modules'.
2. For each hit, ask whether canInvoice or the two-key WHERE is in that file. If the file only calls loadById, that is the ticket.
3. List string adds into SQL: rg -n "ILIKE|' \\+|whereRaw|Sequelize\\.literal" --glob '!node_modules'.
4. Add requireInvoice. Switch the JSON door and the PDF door in the same pull request.
5. Copy as cURL a PDF or export you already make. Swap the id to the other fixture. Expect 404.

rg -n "function loadById|WHERE id = \\$1[^\n]*$" --glob '!node_modules'

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

A 200 with a PDF body means the extract missed a door. That is the next commit, not a reason to revert the JSON fix. Keep shipping per door until the grep is quiet.