code Review/security/Injection, Secrets, and Broken Access Control: Security Defects You Can Catch in a Diff

security

Injection, Secrets, and Broken Access Control: Security Defects You Can Catch in a Diff

Trace untrusted input to its sink to find SQL injection, missing ownership checks, hardcoded secrets, and timing leaks — the security defects that show up in ordinary pull requests.

Reviewer Detection Checklist

0 of 13 verified (0%)

Defect Patterns & Fixes

SQL injection through a concatenated identifier

python

Scenario: A payment worker marks a transaction as settled after a successful retry.

def mark_success(self, transaction_id: str) -> None:
    cursor = self.db.cursor()
    query = ("UPDATE payments SET status = 'SUCCESS' "
             "WHERE id = '" + transaction_id + "'")
    cursor.execute(query)

Why this is defective: transaction_id originates from a webhook payload, so it is attacker-influenced even though it looks like an internal identifier. A value of x' OR '1'='1 settles every payment in the table. Reviewers often wave this through on the grounds that the id 'comes from our own system' — but it entered the system from outside, and the point where it is concatenated has no way to know that. Parameterisation is not a hardening measure here; it is the only correct way to write the statement.

How to Spot It in Reviews:

•Any +, %, .format(), or f-string appearing between a quote character and a variable in a query.
•Trace the variable back to its origin — 'internal id' usually means 'came from a request three functions ago'.
•ORM code is not automatically safe: check raw(), execute(), @Query(nativeQuery = true), and string-built WHERE fragments.
•If the value is a column or table name, parameters will not help — the fix is an allowlist, and that difference is worth stating in the comment.

Missing ownership check on a request-supplied id

csharp

Scenario: An authenticated endpoint returns an invoice by id.

[Authorize]
[HttpGet("invoices/{id}")]
public async Task<IActionResult> Get(Guid id)
{
    var invoice = await _db.Invoices.FindAsync(id);
    if (invoice is null) return NotFound();
    return Ok(invoice);
}

Why this is defective: [Authorize] proves the caller is *somebody*; it says nothing about whether this invoice is theirs. Any logged-in user can read every invoice in the system by changing the id, and every request is authenticated, well-formed, and invisible to a scanner or a WAF. The ownership predicate belongs in the query rather than in an if after the fetch, so there is no path that loads the row without the filter. Returning NotFound rather than Forbid for someone else's record also avoids confirming that the id exists.

How to Spot It in Reviews:

•Any handler parameter that is an entity id, then a lookup by that id alone.
•FindAsync(id), findById(id), objects.get(pk=id) in a request path — the tenant or owner predicate is missing by construction.
•Ask where the caller's identity is read. If the session or token is never consulted in the handler, authorisation is not happening.
•Sequential or guessable ids raise severity but are not the defect; a UUID does not make an unauthorised read authorised.

Live secret committed as a default

python

Scenario: A configuration module supplies a fallback signing key so local development works out of the box.

# services/config.py
DEFAULT_API_SECRET = "sk_live_998822331144556677889900aabbcc"
MAX_ATTEMPTS = 3


def get_secret() -> str:
    return os.getenv("PAYMENT_API_SECRET", DEFAULT_API_SECRET)

Why this is defective: The sk_live_ prefix makes this a production credential, and committing it means it is compromised in every clone, fork, CI log, and backup — deleting the line in a later commit does not undo that. The fallback is also a correctness trap: an instance that is missing its environment variable starts successfully and signs requests with the wrong key, failing at the far end where the cause is invisible. Failing fast at startup turns a silent misconfiguration into an obvious one. Note that the review comment must include rotation: the code fix alone leaves the key valid.

How to Spot It in Reviews:

•Recognisable prefixes and shapes: sk_live_, AKIA, -----BEGIN PRIVATE KEY-----, long base64 or hex literals.
•Any getenv(NAME, <literal>) where the literal is a credential rather than a harmless default.
•Check test fixtures, docker-compose.yml, and .env files added in the same PR — secrets hide there more often than in application code.
•The comment is incomplete without 'rotate this key'; the committed value is already public within the repository's blast radius.

Signature verified with a variable-time comparison

Scenario: An inbound webhook is authenticated by comparing the provider's HMAC header against a locally computed digest.

func verify(body []byte, header string, secret []byte) bool {
    mac := hmac.New(sha256.New, secret)
    mac.Write(body)
    expected := hex.EncodeToString(mac.Sum(nil))
    return header == expected          // short-circuits on first differing byte
}

Why this is defective: String equality returns as soon as two bytes differ, so the time taken depends on how many leading bytes were correct. An attacker who can send many requests and measure response time recovers the expected signature one byte at a time, then forges arbitrary webhooks. The measurement is noisy over the internet but entirely practical from a co-located network, and the whole point of the HMAC is to be the only thing standing between an attacker and a forged payment notification. hmac.Equal (and hmac.compare_digest, CryptographicOperations.FixedTimeEquals, MessageDigest.isEqual) exist for exactly this comparison.

How to Spot It in Reviews:

•==, .equals, strcmp, or != applied to a signature, HMAC, token, session id, or password hash.
•Any comparison of a secret-derived value where one side came from the request.
•Also check what happens on a malformed header: decoding errors must fail closed, never fall through to true.
•Verify the signature is computed over the raw body, not a re-serialised struct — re-encoding changes bytes and breaks verification in ways people 'fix' by disabling it.

Why security defects belong in ordinary code review

Most exploited vulnerabilities are not clever. They are a string concatenated into a query, a secret committed to a repository, a missing ownership check on an id that came from the URL, and a comparison operator that leaks timing. Every one of them is visible in a diff to a reviewer who knows the shape.

Security review is not a separate discipline bolted on before release. A penetration test happens quarterly against a deployed system; code review happens on every change, before the vulnerability exists. The economics are not close: a missing authorisation check caught in review costs a comment, and caught after a breach costs disclosure, forensics, and regulatory notification.

What it costs in production

Injection. One concatenated parameter can read every row of every table, and in many databases can write files or execute commands. There is no partial version of this failure.
Broken access control. An id from the request used without an ownership check lets any authenticated user enumerate every other user's records. This is consistently the most common serious finding in real applications, and it is invisible to scanners because every request is technically well-formed and authenticated.
Leaked secrets. A key committed to a repository is compromised permanently, including in every fork, clone, and CI log. Rotation is the only remedy, and rotation is an incident.
Timing and oracle leaks. A non-constant-time comparison of a token or signature leaks it byte by byte to a patient attacker. A verbose error that distinguishes "no such user" from "wrong password" hands over a user enumeration tool.
Supply chain and deserialisation. Untrusted input turned into objects (pickle, readObject, YAML load, unsafe binders) is remote code execution, not a parsing bug.
Logging PII and credentials. Logs travel further than the database: aggregation services, backups, screenshots in tickets. A token in a log line is a token in a dozen systems with weaker access controls.

How to spot it in review

Follow the data. Every security defect is untrusted input reaching a place that trusts it.

Step 1 — mark the untrusted sources in the diff. Request bodies, query strings, path parameters, headers, cookies, webhook payloads, queue messages, uploaded filenames, and anything from a third-party API. Data from your own database counts too if a user put it there.

Step 2 — follow each to a sink. The sinks that matter:

Query construction. Any query built by concatenation or interpolation, including ORMs' raw-SQL escape hatches, and ORDER BY clauses (which parameter binding cannot cover — allowlist the column instead).
Command and path construction. os.system, exec, Process.Start, and any file path joined from user input (../../ traversal).
Deserialisation and templating. Untrusted bytes into pickle, native serialisation, or a template engine's raw-render mode.
Outbound requests. A URL supplied by the user and fetched by the server is server-side request forgery — internal metadata endpoints are the classic target.
Response rendering. Unescaped output, and dangerouslySetInnerHTML-style APIs.

Step 3 — check authorisation separately from authentication. For every handler that takes an identifier, ask: authenticated as someone is established, but is this user allowed this record? Look for the query that filters by id alone rather than by id and owner.

Step 4 — check the credentials and the crypto. Hardcoded keys and passwords; secrets in default values, tests, fixtures, and compose files; == used to compare tokens, signatures, or HMACs; a hash chosen for speed (MD5, SHA-1, plain SHA-256) where a password hash (bcrypt, scrypt, Argon2) is required; a nonce or salt that is constant, sequential, or seeded from a non-cryptographic random.

Warning

The single highest-yield question in an application code review is: "this id comes from the request — where is the check that it belongs to the caller?" Broken object-level authorisation outnumbers injection in modern codebases and no tool reliably finds it.

Fixing it without breaking something else

Security fixes are behaviour changes, and a fix that breaks legitimate users gets reverted, which leaves you with the vulnerability plus a rollback.

Parameterising a query changes type handling. Values previously coerced by string formatting are now bound with types; date and numeric edge cases can shift. LIKE patterns need their wildcards handled deliberately, and identifiers still cannot be bound.
Adding an ownership filter can break admin and support paths that legitimately read other users' records. Find them before you ship the filter, or you break the support desk.
Rotating a leaked secret is a coordinated deploy. Every consumer needs the new value, and the old value must be revoked — not just removed from the code. Deleting the line from the current commit does nothing; the value stays in history.
Switching to a stronger password hash requires a migration path. Rehash on next successful login and keep verifying the old format until the tail is drained.
Tightening output escaping can break rendering where markup was intentional. That is real work: an allowlist sanitiser, not a blanket escape.
A stricter validator will reject data already in the database. Historic rows that predate the rule are now unreadable by their own service.
Making errors generic reduces debuggability. Keep the detail in the log with a correlation id; return the generic message to the client.

How not to over-engineer the fix

Security is where over-engineering is easiest to justify and most damaging, because complexity is itself an attack surface.

Use the platform primitive, not your own. Parameterised queries, the framework's CSRF token, the standard hmac.compare_digest/CryptographicOperations.FixedTimeEquals, a maintained password hasher. Hand-rolled crypto in a diff is a finding on its own — the correct review comment is "use the library", never a suggested implementation.
Do not build a policy engine for two roles. An ABAC evaluator with a rule DSL is the wrong answer to a missing WHERE user_id = ?.
Do not add a WAF rule instead of the fix. Input filtering in front of an injection is a mitigation, not a remedy, and it makes the underlying defect permanent.
Do not sanitise everywhere. Escaping on input as well as on output produces double-encoded data and a codebase where nobody knows what is safe. Validate at the boundary, encode at the sink, once each.
Do not introduce a secrets platform mid-PR. Moving a key to an environment variable is the proportionate fix; adopting a vault is a project with its own review.
Prefer removing the capability. If a filename does not need to be user-supplied, generate it. The best fix for an injection sink is often deleting the sink.

Comment templates that an agent can act on

""... WHERE id = '" + txId + "'" is SQL injection. Use a parameterised query: cursor.execute("UPDATE payments SET status = ? WHERE id = ?", (status, tx_id))."
"DEFAULT_API_SECRET is a live key in source. Read it from the environment and rotate the committed value — it is already compromised."
"This loads the order by id with no ownership check; any authenticated user can read any order. Filter by customerId from the session."
"signature == expected is not constant time and leaks the HMAC. Use hmac.compare_digest."
"The uploaded filename is joined into the storage path — ../ escapes the directory. Generate a server-side name and keep the original for display only."

Previous Guide

Swallowed Exceptions and Silent Failures: Reviewing Error Handling That Hides Defects

Next Guide

N+1 Queries, Unbounded Memory, and Hot Loops: Spotting Performance Defects in Review

Interactive Practice

Review Defective Code in the Workbench

Test your ability to spot this defect in our interactive Monaco-powered PR code editor.

Open Workbench