Code Review

# ===== 1) Frame the review =====
# Question: 'Where does any user-controlled value reach SQL in this PR?'
# If nothing, skip the SQLi review. If something, follow it.

# ===== 2) Grep for sinks =====

# Raw queries / escape hatches
git diff main..HEAD | grep -nE 'whereRaw|selectRaw|DB::statement|DB::raw|sequelize\.query|\$queryRawUnsafe|FromSqlRaw|cursor\.execute|conn\.query'

# String concatenation that LOOKS like SQL
git diff main..HEAD | grep -nE 'SELECT.*\+\s*|WHERE.*\$\{|ORDER BY.*\$\{'

# Dynamic stored proc execution
git diff main..HEAD | grep -nE 'EXECUTE.*\|\|.*|sp_executesql.*\+'

# ===== 3) Trace upstream =====
# For every sink:
# - Where does the value come from? (req.body, req.query, headers, DB)
# - Is it parameterised end-to-end?
# - If it is an identifier (column, table, direction), is it whitelisted?

# ===== 4) Verify the parameterisation =====
# Parameterised — safe shape:
$stmt = $pdo->prepare('SELECT id FROM users WHERE email = ?');
$stmt->execute([$email]);

# Unparameterised — bug:
$rows = $pdo->query("SELECT id FROM users WHERE email = '$email'");

# Identifier whitelist for ORDER BY:
SORTS = { newest: 'created_at DESC', price: 'price_cents ASC' }
order = SORTS.get(req.query.sort, SORTS['newest'])

# LIKE pattern: escape wildcards on input BEFORE binding
safe = input.replace(/[%_]/g, c => '\\' + c)
query('SELECT id FROM articles WHERE title LIKE $1 ESCAPE "\\"', ['%' + safe + '%'])

# ===== 5) ORM-specific checks =====

# Eloquent (Laravel)
# - safe:    where(), whereIn(), whereBetween()
# - unsafe:  whereRaw with concatenation
# Example fix:
# - whereRaw("status = '$status'")
# + where('status', $status)

# Sequelize (Node)
# - safe:    findAll({ where: { id } }), parameterised raw queries with replacements
# - unsafe:  sequelize.query('... ' + value)
# Example fix:
# - sequelize.query('SELECT * FROM users WHERE id = ' + id);
# + sequelize.query('SELECT * FROM users WHERE id = :id',
#                  { replacements: { id }, type: QueryTypes.SELECT });

# Prisma
# - safe:    $queryRaw\\`tagged template\\`
# - unsafe:  $queryRawUnsafe(stringConcat)

# Django ORM
# - safe:    Order.objects.filter(status=status)
# - unsafe:  raw('SELECT * FROM orders WHERE status = %s' % status)
# Use raw('... WHERE status = %s', [status]) -- parameterised
# Or .extra(where=['status = %s'], params=[status]) (legacy; prefer Q objects)

# ===== 6) Defence in depth checks =====
# Even if the query looks safe, ask:
# - Is the DB role least-privileged?
# - WAF in front of the app?
# - Query log alerts on UNION / sleep / information_schema?
# These don't replace parameterisation, but make a missed bug less catastrophic.

# ===== 7) Suggest tests with the fix =====
# Authors absorb the lesson better when they ship a regression test.
# Example tests:
# - 'send email=\' OR 1=1-- as the email; assert response is 401 or 400'
# - 'list orders sorted by a value not in the whitelist; assert defaults to newest'
# - 'search for a string containing %; assert the LIKE escape works'

# ===== 8) Reviewer comments =====
# Specific, line-anchored:
# 'Line 23: whereRaw with concatenation. Replace with where('status', $status) or
#  whereRaw('status = ?', [$status]) to parameterise.'
# Avoid 'SQLi bad'; the author already knows that.

# Suggest a one-line patch where possible. Reviewers who give exact suggestions
# see the patch land in the same PR; reviewers who explain abstractly see a
# follow-up issue that quietly closes.

# ===== 9) Document the review =====
# 'Reviewed for SQLi: N sinks, K traced upstream, J fixes requested.'
# 'Pending follow-up: introduce app_readonly role for analytics endpoints.'
# Visible reviewer standard improves the next author's PR.

# ===== 10) Common misses =====
# - String concatenation hidden in a helper module
# - Dynamic ORDER BY from a 'safe-looking' enum that turns out to come from req
# - ESM imports of legacy code that bypasses the ORM
# - Test fixtures that turn off the ORM safe defaults
# - Stored procedures called via raw EXEC with concatenated args

# ===== Common false positives =====
# - Static SQL strings with no user input (still parameterise for consistency)
# - Internal admin tools where the threat model is different (still parameterise)
# - Generated SQL from a query builder you trust (verify it actually parameterises)

// Look for:
// - String concatenation / template strings inside .query / .execute
// - .raw() / whereRaw() / fromRaw() / .literal() etc.
// - User input baked into ORDER BY / LIMIT / table names
// - Stored procs that EXECUTE dynamic SQL