Security-focused code review — what to look for, checklists, pull requests

SAST and DAST automate part of the analysis, but they do not replace human review. An experienced reviewer spots business logic flaws that no tool detects: incorrect authorization checks, exploitable state flows, insecure architectural decisions.

The Security Reviewer’s Mindset

When reviewing code, ask yourself:

• Where does this data come from? Who controls this input?
• What happens if this value is unexpected (null, empty, too large, malformed)?
• Who has permission to perform this action? Is it being checked?
• What does this code expose — data, errors, internal state?
• Is any secret or credential visible?

What to Look for by Category

Authentication and Authorization

# Bad — checks authentication only, not authorization
@app.route("/api/orders/<order_id>")
@login_required
def get_order(order_id):
    return Order.get(order_id)  # any logged-in user can see another's order

# Good — verifies that the resource belongs to the current user
@app.route("/api/orders/<order_id>")
@login_required
def get_order(order_id):
    order = Order.get(order_id)
    if order.user_id != current_user.id:
        abort(403)
    return order.to_dict()

Injection (SQL, Shell, LDAP)

# Bad — string interpolation in query
cursor.execute(f"SELECT * FROM users WHERE email = '{email}'")

# Bad — shell injection
os.system(f"convert {filename} output.pdf")

# Good — parameterized
cursor.execute("SELECT * FROM users WHERE email = %s", (email,))

# Good — argument list, no shell
subprocess.run(["convert", filename, "output.pdf"], check=True)

Session and Token Management

• Do JWT tokens verify the signature before using the payload?
• Is the session invalidated on logout?
• Do cookies have HttpOnly, Secure, and SameSite flags?
• Do password reset tokens expire?

Cryptography

• Are MD5 or SHA1 used for password hashing? (wrong — use bcrypt/argon2)
• Are cryptographic keys hardcoded?
• Is TLS configured correctly (no legacy versions, no weak cipher suites)?

File Uploads

# Bad — accepts any extension
filename = request.files["file"].filename
file.save(f"/uploads/{filename}")

# Good — extension whitelist + server-generated name
ALLOWED = {".jpg", ".png", ".pdf"}
ext = Path(filename).suffix.lower()
if ext not in ALLOWED:
    abort(400)
safe_name = str(uuid.uuid4()) + ext
file.save(f"/uploads/{safe_name}")

Security PR Checklist

Input Handling

• All external input is validated before use
• No input concatenated into queries, commands, or templates
• Uploads validate the real file type (magic bytes), not just extension

Access Control

• Every protected route checks both authentication AND authorization
• Resource references verify ownership (IDOR prevention)
• Administrative functions check roles

Sensitive Data

• No secrets or credentials in the diff
• PII does not appear in logs
• Sensitive data in transit uses TLS

Error Handling

• Exceptions are caught and logged — no stack trace returned to the client
• Failure behavior denies access by default

Dependencies

• No new dependency with a known CVE
• Lock file updated

How to Give PR Feedback

Be specific and objective:

# Bad
"This code has a security issue."

# Good
"Line 47: the query uses string interpolation with `user_id`.
If `user_id` comes from the query string without validation (line 12),
this opens an SQL Injection vector. Use a prepared statement:
  cursor.execute('SELECT * FROM orders WHERE user_id = %s', (user_id,))
Ref: OWASP SQL Injection Prevention Cheat Sheet."

Point out: the line, the concrete risk, and the fix. No judgment of the person — focus on the code.

Supporting Tools for Review

• Semgrep — run locally on the diff before opening the PR.
• CodeClimate / SonarCloud — automated analysis integrated with GitHub.
• GitHub Code Scanning — renders SARIF findings directly inside the PR.

Security code review is not a formal audit. It is the last human line of defense before merge. Make it part of the team culture, not the exception.