Best Practices

Security Code Review Best Practices

How to make code reviews an effective security checkpoint without turning every PR into a week-long security audit.

James
Senior DevOps Engineer
6 min read

Code review is the most underutilized security control in most organizations. Every PR goes through review, but reviewers focus almost exclusively on functionality, code style, and architecture. Security gets a passing glance at best. This is a missed opportunity because code review is the one point where a human with context reads every line of production code before it ships.

You do not need every reviewer to be a security expert. You need reviewers who know what to look for and have a systematic way to check for it.

The Mindset Shift

Functional code review asks: "Does this code work correctly?" Security code review asks: "Can this code be made to work incorrectly?"

The difference is adversarial thinking. When reviewing an API endpoint, a functional reviewer checks if it returns the right data. A security-minded reviewer asks: What happens if I send unexpected input? Can I access data I should not? What if I call this 10,000 times per second?

This mindset does not come naturally to most developers. It needs to be taught and reinforced through checklists and practice.

The Security Review Checklist

Keep this short enough that reviewers actually use it. I have seen 50-item checklists that nobody follows. Here is one that covers the highest-impact issues:

Input Handling

  • [ ] All external input is validated (type, length, format, range).
  • [ ] Validation happens server-side, not just client-side.
  • [ ] Database queries use parameterized statements, not string concatenation.
  • [ ] User input is not passed to shell commands, eval(), or similar execution functions.
  • [ ] HTML output is properly encoded to prevent XSS.
  • [ ] File uploads validate file type, size, and content (not just extension).

Authentication and Authorization

  • [ ] All endpoints require authentication unless explicitly public.
  • [ ] Authorization checks verify the current user can access the specific resource.
  • [ ] No reliance on client-side authorization (hidden fields, client-side role checks).
  • [ ] Session tokens are generated with cryptographic randomness.
  • [ ] Password handling uses proper hashing (bcrypt, Argon2).

Data Protection

  • [ ] Sensitive data is not logged (passwords, tokens, PII, credit cards).
  • [ ] Error responses do not expose stack traces or internal details.
  • [ ] Secrets are loaded from environment variables or a secrets manager, not hardcoded.
  • [ ] Sensitive data in transit uses TLS.
  • [ ] Personally identifiable information is handled per your data classification policy.

Dependencies

  • [ ] New dependencies are from reputable sources with active maintenance.
  • [ ] Dependency versions are pinned in the lock file.
  • [ ] No dependencies with known critical vulnerabilities.
  • [ ] License is compatible with the project's license requirements.

Logic and Design

  • [ ] Race conditions are addressed in concurrent operations.
  • [ ] Resource limits are set (timeouts, pagination, file sizes).
  • [ ] Failure modes are handled securely (fail closed, not open).
  • [ ] Cryptographic operations use standard libraries and current algorithms.

How to Actually Review for Security

Step 1: Understand the Change

Before reading code, read the PR description. Understand what the change does, what data it touches, and what trust boundaries it crosses. A PR that adds a new internal logging helper needs less security scrutiny than a PR that adds a new public API endpoint handling user file uploads.

Step 2: Follow the Data

Trace data flow from input to output. Where does data enter the system? How is it transformed? Where is it stored? Where is it displayed? Each transition point is a potential vulnerability.

For a typical web API endpoint:

HTTP Request → Router → Middleware → Handler → Service → Database
                                                        ↓
HTTP Response ← Serializer ← Service ← Handler ← Result

Check security controls at each boundary: input validation at the handler, authorization in middleware, parameterized queries at the database layer, output encoding in the serializer.

Step 3: Check the Negative Cases

What happens when input is:

  • Empty or null?
  • Extremely long?
  • The wrong type (string where number expected)?
  • Containing special characters (<script>, '; DROP TABLE--, ../../../etc/passwd)?
  • A valid value but for a resource the user should not access?

If the code does not handle these cases, flag it.

Step 4: Look at What Changed Around the Code

git diff shows you what changed, but the context around the change matters. If a new function uses a shared utility, check whether that utility is secure for this use case. A string formatting function that is safe for logging might be unsafe for building SQL queries.

Step 5: Check Configuration Changes

Changes to infrastructure configs, CI pipelines, and deployment manifests deserve security scrutiny:

  • New environment variables that might contain secrets.
  • Changed permissions or access controls.
  • Modified network rules or port exposures.
  • New external service integrations.
  • Changes to authentication or CORS configuration.

Making Security Review Sustainable

Do not require security review on every PR. Use risk-based routing. PRs that touch authentication, authorization, data handling, dependency manifests, or infrastructure configuration get a security-focused review. PRs that update copy text or fix CSS do not.

Automate what can be automated. SAST tools catch pattern-based issues (SQL injection, XSS, hardcoded secrets) faster and more consistently than humans. Reserve human review for logic-level issues that tools miss.

Rotate security review duty. If the same person reviews every PR for security, they burn out and their attention degrades. Rotate security review responsibility across the team weekly.

Time-box the review. A security-focused review of a typical PR should take 15-30 minutes on top of the functional review. If it consistently takes longer, the PRs are too large. Break them down.

Build a findings library. Document common security issues you find in reviews with explanations and fix examples. Over time, this becomes training material that reduces the occurrence of those issues.

Common Patterns to Watch For

The "trust the frontend" pattern. Backend code that assumes input is valid because "the frontend validates it." Always re-validate on the server.

The "admin backdoor" pattern. Special parameters or headers that bypass authentication for testing. These must never reach production.

The "log everything" pattern. Debug logging that captures request bodies, including passwords, tokens, and sensitive data. Common in early development, dangerous in production.

The "TODO: add auth later" pattern. Endpoints shipped without authentication with a comment promising to add it later. Later never comes.

The "copy-paste from Stack Overflow" pattern. Code copied from examples that works functionally but uses insecure patterns (like disabling TLS verification or using deprecated crypto functions).

How Safeguard.sh Helps

Safeguard.sh complements manual security code reviews by automating dependency and vulnerability checks on every pull request. While your reviewers focus on logic-level security issues that require human judgment, Safeguard.sh handles the automated detection of vulnerable dependencies, license violations, and policy breaches. Together, human review and automated scanning create a comprehensive security checkpoint that catches both the subtle logic flaws and the known vulnerability patterns.

Never miss an update

Weekly insights on software supply chain security, delivered to your inbox.