Secure code review: a repeatable method

// A diff reader for the first minutes of a review. It decides nothing. For each of the seven passes it lists what a person
// has to look at in a change, with line numbers, and it ends with the questions it cannot answer itself. The two changes are
// made up, for ShopNest's invoice routes, in the unified diff format that git prints.
const CHANGE_A = `diff --git a/src/routes/invoices.js b/src/routes/invoices.js
--- a/src/routes/invoices.js
+++ b/src/routes/invoices.js
@@ -5,7 +5,10 @@
 const { requireUser } = require('../auth');
 const router = express.Router();
-router.use(requireUser);
-
 router.get('/', async (req, res) => {
   res.json(await db.invoicesFor(req.user.id));
 });
+
+router.get('/download', (req, res) => {
+  const file = path.join(INVOICE_DIR, req.query.name);
+  res.sendFile(file);
+});
`;
const CHANGE_B = `diff --git a/src/routes/invoices.js b/src/routes/invoices.js
--- a/src/routes/invoices.js
+++ b/src/routes/invoices.js
@@ -6,4 +6,3 @@
 const router = express.Router();
-router.use(requireUser);

 router.get('/', async (req, res) => {
diff --git a/src/app.js b/src/app.js
--- a/src/app.js
+++ b/src/app.js
@@ -12,3 +12,3 @@
 app.use(express.json());
-app.use('/invoices', invoicesRouter);
+app.use('/invoices', requireUser, invoicesRouter);
 app.use('/orders', ordersRouter);
`;

const INPUT = /\breq\.(?:query|params|body|headers|cookies)(?:\.\w+)?/g;
const SINKS = [['path.join', /\bpath\.join\(/], ['res.sendFile', /\bres\.sendFile\(/], ['a database query', /\.query\(/], ['exec', /\bexec\(/], ['innerHTML', /\.innerHTML\b/], ['res.redirect', /\bres\.redirect\(/], ['fetch', /\bfetch\(/]];
const CHECK = /requireUser|requireAdmin|authorize|csrf|helmet|rateLimit|escape|sanitize|validate/;
const ROUTE = /router\.(get|post|put|patch|delete)\(\s*'([^']+)'/;
const CONFIG = /(^|\/)(package(-lock)?\.json|\.env[\w.]*|Dockerfile|[\w.-]*config[\w.-]*|\.github\/.*)$/;
const TEST = /(^|\/)(tests?|__tests__)\/|\.(test|spec)\.[jt]s$/;

function read(diff) {
  const files = [];
  let file = null;
  let inHunk = false;
  let oldNo = 0;
  let newNo = 0;
  for (const line of diff.split('\n')) {
    if (line.startsWith('diff --git ')) { file = { name: '', added: [], removed: [] }; files.push(file); inHunk = false; continue; }
    if (!inHunk && line.startsWith('+++ b/')) { file.name = line.slice(6); continue; }
    const hunk = /^@@ -(\d+),?\d* \+(\d+),?\d* @@/.exec(line);
    if (hunk) { oldNo = Number(hunk[1]); newNo = Number(hunk[2]); inHunk = true; continue; }
    if (!inHunk) continue;
    if (line.startsWith('+')) file.added.push({ no: newNo++, text: line.slice(1) });
    else if (line.startsWith('-')) file.removed.push({ no: oldNo++, text: line.slice(1) });
    else { oldNo++; newNo++; }
  }
  return files;
}

function sheet(title, diff) {
  const files = read(diff);
  const added = files.flatMap((f) => f.added.map((l) => ({ ...l, file: f.name })));
  const removed = files.flatMap((f) => f.removed.map((l) => ({ ...l, file: f.name })));
  const questions = [];
  const routes = added.map((l) => ROUTE.exec(l.text)).filter(Boolean).map((m) => `${m[1].toUpperCase()} ${m[2]}`);
  console.log(`== ${title}: ${files.length} file(s), +${added.length} -${removed.length}`);
  console.log(`pass 1 change   ${routes.length ? `adds ${routes.join(', ')}` : 'adds no route'}; ${files.map((f) => f.name).join(', ')}`);
  const tainted = new Map();
  const inputs = [];
  const flows = [];
  for (const l of added) {
    const here = [...l.text.matchAll(INPUT)].map((m) => m[0]);
    here.forEach((i) => inputs.push(`line ${l.no} ${i}`));
    const via = [...tainted.keys()].filter((v) => new RegExp(`\\b${v}\\b`).test(l.text));
    const sink = SINKS.find(([, re]) => re.test(l.text));
    if (sink && (here.length || via.length)) flows.push({ no: l.no, what: here[0] ?? `${via[0]} (from line ${tainted.get(via[0])})`, sink: sink[0] });
    const assign = /\b(?:const|let|var)\s+(\w+)\s*=/.exec(l.text);
    if (assign && (here.length || via.length)) tainted.set(assign[1], l.no);
  }
  console.log(`pass 2 inputs   ${inputs.join('; ') || 'none added'}`);
  console.log(`pass 3 sinks    ${flows.map((f) => `line ${f.no}: ${f.what} reaches ${f.sink}`).join('; ') || 'no input reaches a sink in the added lines'}`);
  const dropped = removed.filter((l) => CHECK.test(l.text));
  const checkNames = (t) => CHECK.exec(t)?.[0];
  for (const l of dropped) {
    const moved = added.find((a) => a.text.includes(checkNames(l.text) ?? '\0'));
    console.log(`pass 4 access   removes "${l.text.trim()}" (${l.file} old line ${l.no})${moved ? `; the word is added again at ${moved.file} line ${moved.no}` : ''}`);
    questions.push(moved ? `Does ${moved.file} line ${moved.no} cover the routes that ${l.file} used to protect?` : `Is "${l.text.trim()}" applied somewhere else, and where?`);
  }
  for (const r of added.filter((a) => ROUTE.test(a.text) && !CHECK.test(a.text))) {
    console.log(`pass 4 access   ${ROUTE.exec(r.text)[2]} (line ${r.no}) names no check of its own`);
  }
  if (!dropped.length) console.log('pass 4 access   no check removed');
  for (const f of flows) questions.push(`Line ${f.no}: is ${f.what} checked before it reaches ${f.sink}?`);
  const handlers = added.filter((a) => ROUTE.test(a.text)).length;
  const withError = added.filter((a) => /catch|next\(|try\b/.test(a.text)).length;
  console.log(`pass 5 errors   ${handlers && !withError ? `${handlers} new handler(s), no error path in the added lines` : 'nothing to flag'}`);
  console.log(`pass 6 config   ${files.filter((f) => CONFIG.test(f.name)).map((f) => f.name).join(', ') || 'no configuration or dependency file changed'}`);
  const tests = files.filter((f) => TEST.test(f.name)).length;
  console.log(`pass 7 tests    ${tests ? `${tests} test file(s) changed` : 'no test file changed'}`);
  if (routes.length && !tests) questions.push('Where is the test that tries to break each new route?');
  console.log(`for a person: ${questions.length} question(s)`);
  questions.forEach((q, i) => console.log(`  ${i + 1}. ${q}`));
}

sheet('change A, a new download route', CHANGE_A);
sheet('change B, a check moved to the app', CHANGE_B);

== change A, a new download route: 1 file(s), +5 -2
pass 1 change   adds GET /download; src/routes/invoices.js
pass 2 inputs   line 12 req.query.name
pass 3 sinks    line 12: req.query.name reaches path.join; line 13: file (from line 12) reaches res.sendFile
pass 4 access   removes "router.use(requireUser);" (src/routes/invoices.js old line 7)
pass 4 access   /download (line 11) names no check of its own
pass 5 errors   1 new handler(s), no error path in the added lines
pass 6 config   no configuration or dependency file changed
pass 7 tests    no test file changed
for a person: 4 question(s)
  1. Is "router.use(requireUser);" applied somewhere else, and where?
  2. Line 12: is req.query.name checked before it reaches path.join?
  3. Line 13: is file (from line 12) checked before it reaches res.sendFile?
  4. Where is the test that tries to break each new route?
== change B, a check moved to the app: 2 file(s), +1 -2
pass 1 change   adds no route; src/routes/invoices.js, src/app.js
pass 2 inputs   none added
pass 3 sinks    no input reaches a sink in the added lines
pass 4 access   removes "router.use(requireUser);" (src/routes/invoices.js old line 7); the word is added again at src/app.js line 13
pass 5 errors   nothing to flag
pass 6 config   no configuration or dependency file changed
pass 7 tests    no test file changed
for a person: 1 question(s)
  1. Does src/app.js line 13 cover the routes that src/routes/invoices.js used to protect?

// A search for smells finds where to read, and the reading decides. Seven rules run over a made-up route file, and each hit
// prints the question it raises. The reader's verdict on each hit, written down after reading the lines around it, comes last.
const FILE = `const express = require('express');
const { exec } = require('node:child_process');
const https = require('node:https');
const router = express.Router();

router.get('/go', (req, res) => {
  res.redirect(req.query.next);
});

router.post('/report', (req, res) => {
  exec('convert logo.png -resize 50% out.png', () => res.send('ok'));
});

router.post('/reset', async (req, res) => {
  const token = Math.random().toString(36).slice(2);
  await saveToken(req.body.email, token);
  res.send('sent');
});

router.get('/card', (req, res) => {
  box.innerHTML = '<b>' + req.query.name + '</b>';
});

router.patch('/me', (req, res) => {
  Object.assign(req.user, req.body);
  res.json(req.user);
});

setTimeout(refresh, 1000 + Math.random() * 500);
const agent = new https.Agent({ rejectUnauthorized: false });
`;

const RULES = [
  { id: 'redirect', re: /res\.redirect\(\s*req\./, ask: 'Where can this send the browser, and is the target checked?' },
  { id: 'exec', re: /\bexec\(/, ask: 'Does any part of the command come from outside?' },
  { id: 'random', re: /Math\.random\(/, ask: 'Is the value a token or an id that must not be guessed?' },
  { id: 'inner-html', re: /\.innerHTML\s*=/, ask: 'Does outside text reach the page without encoding?' },
  { id: 'assign-body', re: /Object\.assign\([^,]+,\s*req\.body/, ask: 'Which fields may the client set?' },
  { id: 'tls-off', re: /rejectUnauthorized:\s*false/, ask: 'Why is certificate checking off, and where does this run?' },
  { id: 'jwt-decode', re: /jwt\.decode\(/, ask: 'Is the signature verified anywhere?' },
];
// The reader's verdicts by line: fix (a flaw), fine (it reads safely) or depends (the answer lies outside this file).
const READING = { 7: 'fix', 11: 'fine', 15: 'fix', 21: 'fix', 25: 'fix', 29: 'fine', 30: 'depends' };
const WHY = {
  7: 'the target is the query string, unchecked',
  11: 'the command is a constant string',
  15: 'a reset token made by Math.random can be guessed',
  21: 'the name is pasted into markup',
  25: 'every field the client sends is copied onto the user',
  29: 'a random delay for a retry, not a secret',
  30: 'harmless in a test, serious in production: ask where it runs',
};

const lines = FILE.split('\n');
const hits = [];
lines.forEach((text, i) => {
  for (const rule of RULES) if (rule.re.test(text)) hits.push({ no: i + 1, rule, text: text.trim() });
});
for (const h of hits) {
  console.log(`line ${String(h.no).padStart(2)}  ${h.rule.id.padEnd(11)} ${h.rule.ask}`);
  console.log(`         read: ${READING[h.no].padEnd(7)} ${WHY[h.no]}`);
}
const count = (verdict) => hits.filter((h) => READING[h.no] === verdict).length;
console.log(`${hits.length} hits: ${count('fix')} to fix, ${count('fine')} fine on reading, ${count('depends')} that depends on where the code runs`);

const routes = lines.filter((l) => /^router\.(get|post|patch|put|delete)\(/.test(l));
const guarded = routes.filter((l) => /requireUser|requireAdmin/.test(l));
console.log(`${routes.length} routes, ${guarded.length} name a check: no rule can report a line that is not there`);

line  7  redirect    Where can this send the browser, and is the target checked?
         read: fix     the target is the query string, unchecked
line 11  exec        Does any part of the command come from outside?
         read: fine    the command is a constant string
line 15  random      Is the value a token or an id that must not be guessed?
         read: fix     a reset token made by Math.random can be guessed
line 21  inner-html  Does outside text reach the page without encoding?
         read: fix     the name is pasted into markup
line 25  assign-body Which fields may the client set?
         read: fix     every field the client sends is copied onto the user
line 29  random      Is the value a token or an id that must not be guessed?
         read: fine    a random delay for a retry, not a secret
line 30  tls-off     Why is certificate checking off, and where does this run?
         read: depends harmless in a test, serious in production: ask where it runs
7 hits: 4 to fix, 2 fine on reading, 1 that depends on where the code runs
5 routes, 0 name a check: no rule can report a line that is not there

"""Review comments that get fixed. A finding is a record: where it is, which weakness it is (a CWE and an ASVS requirement),
how bad it is and why, the fix, and the test that will prove the fix. This program checks five findings about the invoice
change of the lesson and builds the report from the ones that are ready. Standard library only."""
import re

FILES = {"src/routes/invoices.js": 14}  # the files of the change and their length in lines
SEVERITIES = ["critical", "high", "medium", "low"]
VAGUE = re.compile(r"(?i)^(be careful|sanitize( the)? input|validate input|fix (this|it)|improve security)\b")


def problems(f):
    """What stops a finding from being sent, in words the author of the change can act on."""
    out = []
    if f.get("kind") not in ("must-fix", "should-fix", "question"):
        out.append("kind is must-fix, should-fix or question")
    if f.get("file") not in FILES:
        out.append("file is not part of the change")
    elif not isinstance(f.get("line"), int) or not 1 <= f["line"] <= FILES[f["file"]]:
        out.append(f"line {f.get('line')} is outside the file (1 to {FILES[f['file']]})")
    if len(f.get("why", "").split()) < 6:
        out.append("why needs a sentence of at least six words")
    if f.get("kind") == "question":
        return out  # a question needs a place and the question itself
    if not re.fullmatch(r"CWE-\d{1,4}", f.get("cwe", "")):
        out.append("cwe looks like CWE-22")
    if not re.fullmatch(r"v5\.0\.0-\d{1,2}\.\d{1,2}\.\d{1,2}", f.get("asvs", "")):
        out.append("asvs looks like v5.0.0-5.3.2")
    if f.get("severity") not in SEVERITIES:
        out.append("severity is critical, high, medium or low")
    if VAGUE.match(f.get("fix", "")) or len(f.get("fix", "").split()) < 6:
        out.append("fix names the change to make, in at least six words")
    if len(f.get("test", "").split()) < 6:
        out.append("test says what the failing check does, in at least six words")
    return out


FINDINGS = [
    dict(id="F1", kind="must-fix", file="src/routes/invoices.js", line=12, cwe="CWE-22", asvs="v5.0.0-5.3.2", severity="high",
         why="The file name comes from the query string, so a request can name a file outside the invoices folder.",
         fix="Resolve the name under INVOICE_DIR and refuse any result that does not start with that folder.",
         test="A request for ../secret.txt and for a missing name answer 404, and an existing name answers 200."),
    dict(id="F2", kind="must-fix", file="src/routes/invoices.js", line=7, cwe="CWE-862", asvs="v5.0.0-8.2.1", severity="high",
         why="The change removes router.use(requireUser), so the list and the download answer without a signed-in user.",
         fix="Put requireUser back on the router, or on each route, and refuse a request that has no user.",
         test="A request with no session to / and to /download answers 401 and returns no body."),
    dict(id="F3", kind="should-fix", file="src/routes/invoices.js", line=12, cwe="path traversal", asvs="", severity="bad",
         why="unsafe", fix="sanitize input", test=""),
    dict(id="F4", kind="must-fix", file="src/routes/invoices.js", line=99, cwe="CWE-209", asvs="v5.0.0-16.5.1", severity="medium",
         why="A failed download sends the server's error text and path back to the browser.",
         fix="Answer with one generic message and write the detail to the log with a request id.",
         test="A request for a missing invoice returns 404 and a body without any path in it."),
    dict(id="F5", kind="question", file="src/routes/invoices.js", line=7,
         why="Is requireUser applied to this router in src/app.js? If it is, say so in the pull request."),
]

ready = []
for f in FINDINGS:
    found = problems(f)
    print(f"{f['id']} {f['kind']:10} {f['file']}:{f['line']:<3} {'ready' if not found else f'{len(found)} to fix'}")
    for line in found:
        print(f"    - {line}")
    if not found:
        ready.append(f)

print(f"\n{len(ready)} of {len(FINDINGS)} findings are ready. The report, most severe first:")
for f in sorted(ready, key=lambda f: (SEVERITIES.index(f["severity"]) if "severity" in f else 9, f["line"])):
    if f["kind"] == "question":
        print(f"  QUESTION  {f['file']}:{f['line']}  {f['why']}")
    else:
        print(f"  {f['severity'].upper():8}  {f['file']}:{f['line']}  {f['cwe']}  {f['asvs']}  [{f['kind']}]")
        print(f"            {f['why']}")
        print(f"            Fix: {f['fix']}")
        print(f"            Test: {f['test']}")

F1 must-fix   src/routes/invoices.js:12  ready
F2 must-fix   src/routes/invoices.js:7   ready
F3 should-fix src/routes/invoices.js:12  6 to fix
    - why needs a sentence of at least six words
    - cwe looks like CWE-22
    - asvs looks like v5.0.0-5.3.2
    - severity is critical, high, medium or low
    - fix names the change to make, in at least six words
    - test says what the failing check does, in at least six words
F4 must-fix   src/routes/invoices.js:99  1 to fix
    - line 99 is outside the file (1 to 14)
F5 question   src/routes/invoices.js:7   ready

3 of 5 findings are ready. The report, most severe first:
  HIGH      src/routes/invoices.js:7  CWE-862  v5.0.0-8.2.1  [must-fix]
            The change removes router.use(requireUser), so the list and the download answer without a signed-in user.
            Fix: Put requireUser back on the router, or on each route, and refuse a request that has no user.
            Test: A request with no session to / and to /download answers 401 and returns no body.
  HIGH      src/routes/invoices.js:12  CWE-22  v5.0.0-5.3.2  [must-fix]
            The file name comes from the query string, so a request can name a file outside the invoices folder.
            Fix: Resolve the name under INVOICE_DIR and refuse any result that does not start with that folder.
            Test: A request for ../secret.txt and for a missing name answer 404, and an existing name answers 200.
  QUESTION  src/routes/invoices.js:7  Is requireUser applied to this router in src/app.js? If it is, say so in the pull request.