What Is Wrong With This Code? A Security Review Checklist

When you ask "what is wrong with this code" and it compiles, runs, and passes its tests, the answer is usually a security or correctness flaw the happy path never exercised. Bugs that crash announce themselves. The ones that matter most in a review are the quiet ones: code that works perfectly for well-behaved input and hands an attacker the keys when the input is not well-behaved. If you have ever pasted a snippet somewhere and typed "whats wrong with my code," this is the mental checklist that tends to find the real issue.

The trick is to stop reading the code as what it does and start reading it as what an adversary could make it do.

Start with the inputs

Almost every serious flaw traces back to trusting input that should not be trusted. Before anything else, find every place data enters the function — arguments, request bodies, query parameters, headers, file contents, environment — and ask what happens if each one is hostile.

app.get('/user', (req, res) => {
  const query = "SELECT * FROM users WHERE id = " + req.query.id;
  db.execute(query);
});

What is wrong with this code: req.query.id is attacker-controlled and concatenated straight into SQL. Passing id=1 OR 1=1 returns every user; worse payloads do worse things. The test suite passed because the tests always sent a clean integer. The fix is a parameterized query, never string concatenation:

db.execute("SELECT * FROM users WHERE id = ?", [req.query.id]);

The same class of bug shows up as command injection when input reaches a shell, path traversal when it reaches a filesystem path, and cross-site scripting when it reaches HTML output. Once you learn to spot "untrusted value flows into dangerous operation," you find it everywhere.

Look at error handling, especially the empty parts

The second thing to scan is what happens when things go wrong, because that is where security decisions quietly get skipped.

try {
    boolean valid = verifyToken(token);
    if (!valid) return unauthorized();
} catch (Exception e) {
    // ignore
}
proceedAsAuthenticated();

Whats wrong with this code: the empty catch swallows any failure from verifyToken, and execution falls through to proceedAsAuthenticated(). A malformed token that throws instead of returning false becomes an authentication bypass. An empty catch block is one of the highest-value things to flag in any review. Errors on a security path should fail closed — deny access — not fall through.

Check the secrets and the crypto

A quick scan for hardcoded credentials pays off constantly:

API_KEY = "sk_live_51H8xY2..."   # committed to the repo

What is wrong with my code here is that the key now lives in version control history forever, readable by anyone with repo access, and rotation means a code change. Secrets belong in environment variables or a secrets manager, never in source.

Crypto smells are close behind: MD5 or SHA1 used for anything security-relevant, a hardcoded IV, Math.random() used to generate a token, or an equality check on a secret using == instead of a constant-time comparison (which leaks information through timing). None of these throw errors. They just quietly do the wrong thing.

Watch for access control that is not there

Some of the most damaging bugs are things the code fails to do. An endpoint that fetches a record by ID but never checks that the record belongs to the requesting user is a broken-access-control flaw:

app.get('/invoice/:id', (req, res) => {
  // no check that req.user owns invoice :id
  res.json(getInvoice(req.params.id));
});

Changing the ID in the URL walks straight through everyone else's invoices. This one is invisible in a code read unless you are specifically asking "who is allowed to do this, and where is that enforced?" Add that question to your checklist.

Reason about concurrency and state

Code that is correct when run once can be wrong when run twice at the same time. A check-then-act sequence — read a balance, then deduct — can be raced by two simultaneous requests so both see the old balance. These bugs pass every single-threaded test and only surface under load, often as a way to spend the same credit twice. If a snippet reads shared state and then modifies it, ask whether two callers could interleave.

The part your eyes cannot cover

Manual review is essential, but it does not scale and it does not see everything. Two categories in particular need tooling:

Your own code, systematically. Static analysis (SAST) reasons about data flow the way the checklist above does, but exhaustively. It traces tainted input to sinks across files in ways a human reviewer misses on a large diff. Run it in CI so every pull request gets the same scan.

Code you did not write. Here is the uncomfortable truth: most of what runs in a modern app is third-party dependencies, and "what is wrong with this code" often has nothing to do with your code at all. The flaw is a known CVE in a library three levels down that you never opened. No amount of reviewing your own snippet finds it. That is the job of software composition analysis, which inventories your dependencies and matches them against known vulnerabilities; an SCA tool such as Safeguard can flag a vulnerable package even when it arrived transitively.

A checklist you can actually use

When you next stare at a snippet and ask what is wrong with this code, run through this in order:

1. Where does input enter, and what if it is hostile? (injection, traversal, XSS)
2. What happens on error — does it fail open or closed? (empty catches, fall-through)
3. Are there secrets in the source, or weak crypto?
4. Who is allowed to do this, and where is that enforced? (access control)
5. Can two callers interleave on shared state? (race conditions)
6. What about the dependencies I did not write?

Most quiet security bugs fall into one of those six buckets. Working the list turns a vague "something feels off" into a specific finding. The Academy has worked examples for each category if you want to drill the pattern recognition.

FAQ

How do I figure out what is wrong with my code from a security standpoint?

Stop reading it as what it does and read it as what an attacker could make it do. Check every input for injection, check error handling for fail-open behavior, scan for hardcoded secrets and weak crypto, verify access control, and consider concurrency.

Why does buggy code still pass all my tests?

Tests usually exercise the happy path with well-behaved input. Security flaws live in the paths tests do not cover: malformed input, error conditions, concurrent access, and unauthorized requests. Passing tests is not evidence the code is secure.

What is the most common security bug in code reviews?

Untrusted input flowing into a dangerous operation — SQL injection, command injection, and cross-site scripting all share this shape. Empty catch blocks that cause security checks to fail open are a close second.

Can a tool tell me what is wrong with my code automatically?

Partly. Static analysis (SAST) systematically traces input to dangerous sinks in your own code, and software composition analysis finds known vulnerabilities in your dependencies. Together they catch far more than manual review, though human judgment is still needed for logic and access-control flaws.