Security and Performance Review

The two specialist lenses a reviewer switches on for a diff — thinking like an attacker to catch the injection and the missing authorization check, and thinking like production to catch the N+1 query — while knowing exactly where the human eye stops and a scanner, profiler, or load test has to take over.

By Pratik Dhanave · · 17 min read
#Code Review#Security#Performance#Quality#Engineering Practices
Part 6 of 8 Code Review
← Part 5: The Author's Craft: Making Code Easy to Review Part 7: Code Review at Scale →

Most of a code review is design, correctness, tests, and clarity — the general-purpose pass covered earlier in this series. But two categories reward a different kind of attention, and a reviewer who does not deliberately switch into them will scroll right past the two most expensive bugs a diff can carry: a missing authorization check, and a query inside a loop.

Neither is about whether the code works. The injection-vulnerable handler works perfectly — for well-behaved input. The N+1 query works perfectly — on the twelve rows in your test database. Both pass review, both pass tests, and both surface later as an incident: one as a breach, the other as a 2 a.m. page when the table hits a million rows. The security and performance lenses exist to catch the class of bug that looks fine and isn’t.

This post is about how to switch those lenses on, what to look for in an actual diff, and — just as important — where the human reviewer’s usefulness ends and the tools take over. You cannot fully security-review or performance-review a change by eye. What you can do is flag the smells the tools miss and know when to demand a SAST run, a profiler trace, or a load test before approval.

This complements the site’s dedicated API Security series, which goes deep on each vulnerability class. Here the focus is narrower: how to spot these things in a pull request, in the minutes you have.

The security lens: assume the input is hostile

The whole security mindset compresses to one instruction: assume every input is hostile, and follow the data. When a diff introduces a new input — a query parameter, a request body field, an uploaded file, a header, a URL the user supplies — trace it forward. Where does it land? If it reaches a sink — a database query, a shell command, a file path, an HTTP call, a deserializer — without being validated or parameterized on the way, you have found something.

That is the entire technique: identify sources, identify sinks, and check every path between them. Here is the catalog of what to look for.

Untrusted input reaching a sink (injection)

The classic. User input concatenated into a query string, a shell command, or an HTML template. The fix is almost always “parameterize” — let the driver separate code from data.

- rows, err := db.Query(
-     "SELECT * FROM invoices WHERE customer = '" + name + "'")
+ rows, err := db.Query(
+     "SELECT * FROM invoices WHERE customer = $1", name)

Review comment: “name comes straight from the query string and is being concatenated into SQL — a value of x' OR '1'='1 reads every customer’s invoices. Use a parameterized query ($1) so the driver treats it as data, not code. Same pattern applies to the os/exec call on line 88 — build the argument list, don’t shell out with a string.”

The tell in a diff is string concatenation or interpolation reaching anything that executes: SQL, exec, eval, a template rendered as raw HTML, an LDAP filter, a file path joined from user input.

A missing authorization check on a new endpoint or object

This one deserves top billing because it is the highest-value catch a human can make. When a diff adds an endpoint that loads or mutates an object by ID, ask the question a scanner cannot: is this user allowed to touch this particular object? Authentication (“who are you”) is not authorization (“may you do this to that”). A logged-in user is still not allowed to read another user’s invoice.

  func (h *InvoiceHandler) Get(w http.ResponseWriter, r *http.Request) {
      id := r.PathValue("id")
-     inv, err := h.store.FindInvoice(id)
+     inv, err := h.store.FindInvoice(id)
      if err != nil {
          http.Error(w, "not found", 404)
          return
      }
+     if inv.OwnerID != currentUser(r).ID {
+         http.Error(w, "not found", 404)  // 404, not 403 — don't confirm it exists
+         return
+     }
      writeJSON(w, inv)
  }

Review comment: “This looks up the invoice by ID but never checks that it belongs to the caller — any authenticated user can read any invoice by guessing or enumerating IDs. This is Broken Object Level Authorization (BOLA). We need an ownership check after the lookup; returning 404 rather than 403 also avoids confirming the record exists to someone probing. The authorization post in the API Security series covers the pattern.”

The gotcha: the single highest-value security catch in a review is a missing authorization check on a new object or endpoint — and it is exactly the bug automated scanners are worst at. A SAST tool sees a database lookup and a response; it has no idea that this user shouldn’t see that row, because the rule lives in your domain, not in the code’s syntax. BOLA tops the API security risk lists precisely because it is invisible to machines and obvious to a human who asks “whose data is this?” Every time a diff adds an endpoint that takes an ID, that is the question.

Secrets committed, and sensitive data in logs

Two of the cheapest catches, and both permanent once they land. A hard-coded API key, a database password, a private key, a token in a config file — even if deleted in a later commit, it lives in git history forever and must be rotated, not just removed. And the quieter cousin: sensitive data written to logs.

- log.Printf("auth attempt: user=%s password=%s token=%s", u, pw, tok)
+ log.Printf("auth attempt: user=%s result=%s", u, result)

Review comment: “This logs the raw password and token at info level — they’ll land in log aggregation, get indexed, and sit there readable by anyone with log access. Drop the secret fields; log the outcome, not the credentials. Also flagging the apiKey = \"sk-live-...\" on line 12 — that needs to move to config/secret storage and the exposed key rotated, since it’s now in git history.”

The diff tells: a string that looks like a key (sk-, AKIA, a long base64 blob, -----BEGIN), and any log line that formats a password, token, full card number, or personal data.

Unsafe deserialization, SSRF, and missing rate limits

Three more that hide in ordinary-looking additions:

  func FetchPreview(userURL string) (*http.Response, error) {
-     return http.Get(userURL)   // SSRF: userURL may be an internal address
+     if err := validatePublicURL(userURL); err != nil {  // allowlist scheme+host, block private ranges
+         return nil, err
+     }
+     return safeClient.Get(userURL)
  }

Review comment: “userURL is user-controlled and we fetch it server-side — that’s SSRF. Someone can point it at the cloud metadata endpoint or an internal service the firewall trusts. We need to validate the scheme and host against an allowlist and reject private/link-local ranges before the request, and ideally use a client that doesn’t follow redirects into those ranges.”

A new dependency is new attack surface

When a diff adds a package to go.mod, package.json, or pyproject.toml, that is not a free line. You are importing someone else’s code, and its transitive dependencies, into your trust boundary and your build.

Review comment: “This adds left-pad-ultra for one string-padding call — do we need the dependency, or is this ten lines of our own? If we do keep it: is it maintained (last release, open CVEs, download count), and does it pull in a large transitive tree? A one-line helper isn’t worth a new supply-chain surface.”

The gotcha: a new dependency is new attack surface, not just a convenience — every package you add can execute code at install time, ship a future compromised version, or drag in a tree of transitive packages you never chose. Review why it is needed and whether it is maintained, the same way you would review code written in-house, because at runtime that is exactly what it is. Trivial functionality is cheaper to own than to import. This ties directly into supply-chain security: the dependency you wave through today is the one in the incident report tomorrow.

Security smells checklist

A quick scan list for the security lens:

The performance lens: think like production, not like your laptop

The performance mindset is a shift of scale: read the diff as if the data were a thousand times bigger and a hundred requests were hitting it at once. Almost every performance bug that survives review does so because it was tested against a tiny dataset where the problem is genuinely invisible.

The N+1 query — the classic diff smell

If you learn to spot one performance problem in a diff, make it this one. A loop that runs a query per iteration turns one logical operation into one query plus N more — hence “N+1.”

  orders, _ := db.Query("SELECT id, customer_id FROM orders WHERE status='open'")
  for _, o := range orders {
-     // one query per order — N+1
-     cust, _ := db.Query(
-         "SELECT name FROM customers WHERE id = $1", o.CustomerID)
-     results = append(results, render(o, cust))
+     ids = append(ids, o.CustomerID)
  }
+ // one query for all customers, then join in memory
+ custByID, _ := loadCustomers(db, ids)  // SELECT ... WHERE id = ANY($1)
+ for _, o := range orders {
+     results = append(results, render(o, custByID[o.CustomerID]))
+ }

Review comment: “This runs one customer query per order — with 500 open orders that’s 501 round trips, and it scales linearly with order volume. It won’t show up in the test with three orders, but it will melt in production. Can we collect the customer IDs and fetch them in a single WHERE id = ANY(...), then look them up from a map in the loop? If an ORM is doing the loading, this is where eager-loading / a JOIN belongs.”

The gotcha: an N+1 query is completely invisible in a small test dataset and melts production the moment the table grows — the test with ten rows does eleven fast queries and passes green, while the same code does ten thousand and one against the real table. You cannot catch this by watching it run in CI; you catch it by recognizing the shape in the diff — a loop with a query (or an ORM lazy-load, or a network call) inside it. Train your eye on the pattern, because the profiler only finds it after it has already shipped.

Unbounded queries and missing pagination

A query with no LIMIT and no pagination is a landmine planted on a schedule: fine today, an out-of-memory crash the day the result set grows past what one response can hold.

- rows, _ := db.Query("SELECT * FROM events WHERE user_id = $1", uid)
+ rows, _ := db.Query(
+     "SELECT * FROM events WHERE user_id = $1 ORDER BY id LIMIT $2 OFFSET $3",
+     uid, pageSize, offset)

Review comment: “This loads every event for a user into memory with no bound. For an active user that’s unbounded growth — a slow response now and an OOM later. Can we paginate (limit + cursor/offset) and have the endpoint return a page? Also selecting * when we render three columns pulls more than we need over the wire.”

Work in a loop that should be hoisted, and missing indexes

Two more that read straight off the diff:

- for _, line := range lines {
-     re := regexp.MustCompile(`^\d{4}-\d{2}-\d{2}`)  // recompiled every iteration
-     if re.MatchString(line) { ... }
- }
+ re := regexp.MustCompile(`^\d{4}-\d{2}-\d{2}`)      // compile once
+ for _, line := range lines {
+     if re.MatchString(line) { ... }
+ }

Review comment: “The regex is compiled on every iteration — hoist it above the loop and compile once. Separately, the new WHERE tenant_id = $1 AND created_at > $2 query: is there an index covering (tenant_id, created_at)? Without one this is a sequential scan that gets slower as the table grows — worth an accompanying migration.”

Hot-path allocations, blocking calls, caching, and O(n²)

Rounding out the performance lens:

The discipline: don’t optimize prematurely, but don’t ship the obvious incident

There is a real tension here, and getting it right is the skill. “Premature optimization is the root of all evil” is correct about micro-performance — do not rewrite a clean function into an unreadable one to save nanoseconds nobody will notice, and do not demand hand-tuned code where a readable version runs fine. Guessed performance is usually wrong; the honest answer to “is this fast enough?” is often “measure it.”

The gotcha: “optimize later” is the right call for micro-performance and exactly the wrong call for an unbounded query or a missing index — those are not optimizations, they are latent incidents you are choosing to ship. The distinction is algorithmic and structural versus constant-factor. An N+1 query, a missing LIMIT, an unindexed hot query, an accidental O(n²) — these change how the code scales, and “later” means “during the outage.” Constant-factor tuning genuinely can wait for a profiler. Catch the scaling bugs now; defer the micro-tuning until something proves it matters.

Performance smells checklist

Know where your eyes stop

The most important thing to internalize about both lenses is their limit. You cannot fully security-review or performance-review a change by reading it. A human reviewer catches the smells — the pattern, the missing check, the loop-with-a-query — and that is high-value work no tool does well. But depth belongs to instruments built for it.

For security, the diff-level catch is the start; behind it sits SAST (static analysis over the whole codebase), DAST (probing the running app), dependency scanners, and secret scanners in CI. Some classes — a taint path across many files, a vulnerable transitive dependency, a runtime injection — are things a scanner finds and an eyeball won’t. A reviewer’s job is to catch the human-obvious BOLA the scanner misses, and to insist the scanners run for everything else.

For performance, the diff-level catch is the N+1 and the unbounded query; the depth is a profiler on a representative dataset and a load test at expected concurrency. If a change touches a hot path and you cannot tell whether it is fast enough, the right review outcome is not “approve and hope” or “reject on a guess” — it is “please attach a benchmark or a profile before we merge.” Ask for the measurement; don’t invent it.

Know where your eyes stop
Lens What the human reviewer catches by eye What you defer to tools
Security Missing authz/BOLA, obvious injection, committed secrets, secrets in logs, unjustified dependency SAST/DAST, dependency & secret scanners, penetration testing
Performance N+1, unbounded query, missing index, loop-invariant work, obvious O(n²) Profiler on real data, load/stress test, APM traces

Key takeaways

Further reading

Written by Pratik Dhanave ← All posts

Find me on GitHub · LinkedIn · Acknowledgments