Security Code Review for APIs
A repeatable method that finds real issues in an hour, instead of skimming a diff and hoping.
Unstructured review finds what the reviewer already knows to look for. A method finds the categories they would have skipped on a Friday afternoon.
Start with the diff's shape, not its content
Three questions before reading any code:
- Does this add a new way to reach existing data? Exports, search, bulk endpoints, GraphQL resolvers and webhooks all need their own authorization, and they usually inherit none.
- Does this add a new input? A new field, parameter or header is a new place to validate.
- Does this change who can do what? Any diff touching roles, scopes or middleware ordering deserves close reading.
Then walk the seven questions per endpoint
Authentication, object authorization, function authorization, input validation, output shaping, resource bounds, and outbound calls. Every unanswered one is a finding.
The patterns worth grepping for
Some findings are mechanical. findById(req.params.id) with no ownership filter. res.json(model) with no serializer. db.raw with no placeholder. jwt.decode. req.body passed straight into an update. Automate these and spend the review time on logic.
Read the tests as part of the diff
A new endpoint with no authorization test is incomplete. "Does another user get a 404?" is a question the test suite should already answer.
Say what and why
"This is vulnerable to BOLA" is a label. "Alice can read Bob's invoice by changing the id, because the query is not scoped to req.user.id — move the check into the where clause" is a review comment someone can act on.
Example
# Mechanical findings — automate these, then review the logic.
# Unscoped lookups by id
grep -rnE "find(ById|One|First)\(.*(params|body|query)\." src/ \
| grep -vE "user_?[Ii]d|tenant_?[Ii]d|owner"
# Models serialised directly
grep -rnE "res\.json\((?!.*Serializer|.*serialize|.*\{)" src/
# Raw SQL with no placeholder
grep -rn "raw(\|DB::raw\|queryRawUnsafe" src/ | grep -v "?\|:param\|\$1"
# Decoding a token instead of verifying it
grep -rn "jwt.decode\|decodeJwt" src/
# Request body passed straight into a write
grep -rnE "\.(update|create|insert)\(.*req\.body\)" src/When to use it
- A review catches a new CSV export that reuses a query predating the tenancy model, exposing other customers' rows.
- A grep in CI flags an unscoped findById the day it is written, before it reaches review at all.
- A pull request adding an endpoint is returned because it has no test asserting that another user receives a 404.
More examples
The review checklist, applied to a real diff
Checking project membership rather than only tenant membership is the finding a checklist surfaces and intuition usually misses — tenant scoping feels like enough.
// The pull request:
app.get('/api/projects/:projectId/documents', auth, async (req, res) => {
const documents = await db('documents')
.where({ project_id: req.params.projectId })
.limit(req.query.limit || 100);
res.json(documents);
});
// ── The seven questions ──────────────────────────────────────────────
//
// 1. AUTHENTICATION ✅ auth middleware present
//
// 2. OBJECT AUTHORIZATION ❌ FINDING
// projectId comes from the URL and is never checked against the caller.
// Any authenticated user reads any project's documents.
//
// 3. FUNCTION AUTHORIZATION ⚠️ should reading documents require a role or
// scope? Not stated. Ask.
//
// 4. INPUT VALIDATION ❌ FINDING
// projectId is unvalidated (is it a uuid?), and `limit` is a raw string
// from the query — `?limit=abc` and `?limit=1e9` both reach the database.
//
// 5. OUTPUT SHAPING ❌ FINDING
// The model is serialised whole. Whatever columns `documents` has today,
// and whatever is added next year, is published.
//
// 6. RESOURCE BOUNDS ❌ FINDING
// `limit` is client-controlled with no cap. `?limit=10000000`.
//
// 7. OUTBOUND ✅ none
//
// PLUS the question that is not about this code:
// Is there another path to `documents`? A search index? An export?
// A webhook payload? Do THEY scope by project and by owner?
// ── The corrected version ────────────────────────────────────────────
const listDocsQuery = z.object({
limit: z.coerce.number().int().min(1).max(100).default(20),
cursor: z.string().max(200).optional(),
}).strict();
const paramsSchema = z.object({ projectId: z.string().uuid() });
app.get('/api/projects/:projectId/documents',
auth,
validate({ params: paramsSchema, query: listDocsQuery }),
async (req, res) => {
// 2. The PARENT is authorized first, from the credential.
const project = await db('projects')
.where({ id: req.params.projectId, tenant_id: req.user.tenant })
.first();
if (!project) return res.status(404).json({ error: 'not_found' });
// 3. And membership of that project, not merely of the tenant.
const member = await db('project_members')
.where({ project_id: project.id, user_id: req.user.id }).first();
if (!member) return res.status(404).json({ error: 'not_found' });
// 6. Bounded, and the child is scoped to the checked parent.
const documents = await db('documents')
.where({ project_id: project.id })
.orderBy('created_at', 'desc')
.limit(req.query.limit);
// 5. Explicit shape.
res.json({ data: documents.map(DocumentSerializer.forMember) });
});
// Four findings in seven lines of code, found by asking seven questions.Automating the mechanical findings
Encoding review findings as lint rules is how a one-off review becomes a permanent standard — the next person cannot reintroduce the pattern without a build failure.
// Reviewers should spend their attention on logic. Let a linter find the rest.
// .eslintrc.js
module.exports = {
plugins: ['security'],
rules: {
'security/detect-unsafe-regex': 'error', // ReDoS
'security/detect-object-injection': 'warn',
'security/detect-non-literal-fs-filename': 'error', // path traversal
'security/detect-child-process': 'error', // command injection
'no-restricted-syntax': ['error',
{
selector: "CallExpression[callee.property.name='json'] > Identifier",
message: 'Do not serialise a model directly — use a Serializer view.',
},
{
selector: "CallExpression[callee.property.name='decode'][callee.object.name='jwt']",
message: 'jwt.decode does not verify. Use jwt.verify with pinned options.',
},
{
selector: "CallExpression[callee.property.name=/^(update|create|insert)$/] " +
"> MemberExpression[object.name='req'][property.name='body']",
message: 'Do not pass req.body into a write — use an explicit field allowlist.',
},
],
'no-restricted-properties': ['error',
{ object: 'req', property: 'originalUrl',
message: 'Use req.path in logs — originalUrl carries the query string.' },
],
},
};
// A CI script for the patterns a linter cannot express
// scripts/security-grep.sh
FINDINGS=0
check() { # check <description> <grep args...>
local desc="$1"; shift
if out=$(grep -rnE "$@" src/ 2>/dev/null); then
echo "⚠️ $desc"; echo "$out" | sed 's/^/ /'; FINDINGS=1
fi
}
check "Unscoped lookup by id" \
"find(ById|One)\(.*req\.(params|body|query)"
check "Raw SQL without a placeholder" \
"(db|knex)\.raw\([^?]*\\\$\{"
check "jwt.verify without pinned options" \
"jwt\.verify\([^,]+,[^,]+\)"
check "CORS reflecting the request origin" \
"Allow-Origin.*req\.(headers\.origin|get\('origin'\))"
check "Substring origin matching" \
"origin\.(includes|endsWith|startsWith)\("
check "Logging a whole request object" \
"logger\.\w+\(\{?\s*(req|request)[,}]"
exit $FINDINGS
// Each of these took two minutes to write and finds a real class of bug on
// every large codebase.A review template that produces actionable comments
Citing an existing correct example in the codebase is the most effective part of a review comment — it turns an abstract instruction into a pattern to copy.
## Security review: <PR title>
### Shape of the change
- [ ] Adds a new path to existing data? (export / search / bulk / GraphQL / webhook)
→ if yes: does the NEW path have the same authorization as the old one?
- [ ] Adds a new input? (field / parameter / header / upload)
→ if yes: is it validated, bounded and allowlisted?
- [ ] Changes who can do what? (roles / scopes / middleware order)
→ if yes: read very carefully; test both directions
### Per endpoint
| Question | Answer |
|---|---|
| Reachable unauthenticated? | |
| Object authorization — WHERE is the check? | |
| Is it inside the query or after the fetch? | |
| Foreign object → 404 (not 403)? | |
| Function authorization — role/scope, on the route? | |
| Body validated, unknown fields REJECTED? | |
| Response shaped explicitly? | |
| limit / depth / size bounded? | |
| Outbound calls — timeout, size, redirects? | |
### Tests
- [ ] Another user gets 404
- [ ] Another tenant gets 404
- [ ] A rejected write did not happen (assert the row is unchanged)
- [ ] Non-writable fields are ignored
- [ ] Page size is capped
### Writing the comment
❌ "This has an IDOR."
A label. The author now has to work out what and where.
✅ "Line 12: `findById(req.params.id)` is not scoped to the caller, so Alice
can read Bob's invoice by changing the id. Move the check into the query:
`findOne({ id, userId: req.user.id })`, and return 404 rather than 403 so
we do not confirm the record exists. Same pattern as `invoices.js:44`."
States: WHERE, WHAT an attacker does, HOW to fix it, and a precedent in
the codebase. That comment gets fixed correctly the first time.
### Severity, so the author can prioritise
CRITICAL cross-tenant or cross-user data access; auth bypass; RCE
HIGH privilege escalation; injection; sensitive data exposure
MEDIUM missing bounds; verbose errors; weak configuration
LOW defence in depth; hardening; consistency
Discussion