Security Code Review

Find exploitable bugs, not style violations. Every finding must answer: how would an attacker actually use this?

When to use

Pull-request review, pre-release audit, triaging a scanner report, reviewing auth or crypto code, assessing a dependency, responding to a pentest finding.

Do not use for: infrastructure/cloud config review, compliance questionnaires, or writing exploits against systems the user does not own.

Hard rules

1. No exploit code for third-party systems. Proof-of-concept is limited to the minimum needed to demonstrate the flaw in the user's own code.
2. Never invent a CVE number or a version range. Mark unverified claims [VERIFY].
3. Do not report noise. A finding with no realistic attacker path is a hardening note, not a vulnerability — label it as such.
4. Redact secrets you discover: show AKIA****EXAMPLE, never the full value.

Workflow

1. Establish the threat model in two minutes

If the user cannot answer, assume internet-facing and untrusted input, and say so.

2. Run the secret and pattern scanners first

Deterministic detection before manual reading:

python3 scripts/scan_secrets.py ./src --format md
python3 scripts/taint_patterns.py ./src --lang auto

Treat every hit as a lead, not a verdict. Confirm by reading the code.

3. Trace taint: source → sink

For each dangerous sink, walk backwards to the input source. A vulnerability exists only when untrusted data reaches a sink without effective sanitisation.

4. Check the access-control model explicitly

Broken access control is the most common critical finding and no scanner sees it.

• Is authorisation checked on the object, not only on the route? (IDOR)
• Can a user change userId, tenantId, role or orderId in the request and reach another tenant's data?
• Are mass-assignment fields filtered on create/update?
• Are admin routes protected server-side, not just hidden in the UI?
• Do list endpoints filter by the caller's tenant in the query, not in code after fetching?
• Are JWTs verified with a pinned algorithm (reject alg: none, reject HS/RS confusion), an audience check and an expiry check?

5. Review crypto and secrets

6. Assess dependencies

Direct and transitive. Flag: known-vulnerable versions [VERIFY against an advisory DB], unmaintained packages, typosquat-looking names, install scripts, and lockfile absence. Recommend pinning + automated updates.

7. Report

One table, sorted by severity. For each finding: ID, title, severity, file:line, attacker path in one sentence, patch, and effort.

Severity uses impact × exploitability:

Worked example

@app.route("/api/invoices/<invoice_id>")
@login_required
def get_invoice(invoice_id):
    row = db.execute(
        f"SELECT * FROM invoices WHERE id = '{invoice_id}'").fetchone()
    return jsonify(dict(row))

F-01 — SQL injection via invoice_id · Critical · api/invoices.py:14 Any authenticated user can send /api/invoices/x' UNION SELECT password_hash,1,1 FROM users-- and read the users table. Authentication is not a mitigation: any account suffices.

F-02 — IDOR: no ownership check · High · same line Even patched against SQLi, the query filters only on id. User A can read user B's invoice by guessing or enumerating the id.

Patch

@app.route("/api/invoices/<invoice_id>")
@login_required
def get_invoice(invoice_id):
    row = db.execute(
        "SELECT id, number, total_cents, currency, status, created_at "
        "FROM invoices WHERE id = ? AND tenant_id = ?",
        (invoice_id, current_user.tenant_id),
    ).fetchone()
    if row is None:
        abort(404)                      # 404, not 403 — do not confirm existence
    return jsonify(dict(row))

Also fixed: SELECT * leaked internal columns to the API response.

Quality checklist

• Every finding names a concrete attacker and a concrete gain
• Severity justified by impact × exploitability, not by CWE class alone
• Each finding has a patch, not just a description
• Access control reviewed at the object level, not only the route
• Secrets redacted in the report; rotation advised for anything committed
• No invented CVEs or version ranges; unverified claims marked [VERIFY]
• False-positive-looking scanner hits explicitly dismissed with a reason
• Hardening notes separated from real vulnerabilities

Failure modes