Security Code Review

The invoice assistant from Chapter 30 is ready to acquire its first tool. A pull request adds an endpoint that accepts an invoice ID and proposed corrections, asks the policy service whether the caller has the billing-editor role, loads the invoice, and creates a draft. The change is small. Its security behavior is not.

The request body includes tenant_id, even though the server already knows the caller’s tenant. The query loads an invoice by ID before the permission decision. The policy client treats a timeout as permission for anyone with the editor role. Debug logging records the complete correction request. The tests prove that an editor can correct an invoice in the happy path.

Every changed line is plausible in isolation. Read as one path, they allow a caller to name another tenant, probe whether its invoices exist, inherit stale or unavailable authority, and place invoice data in logs. The missing denial tests hide the path rather than close it.

A security review follows that path through the changed behavior and asks for evidence where the path should stop. It is not an ordinary review performed more slowly, and it cannot be delegated to a scanner that does not know what an invoice editor is allowed to do.

Find the security change before reviewing the code

Begin with a one-sentence claim about behavior:

This change lets an authenticated account manager create a draft correction for an invoice in the manager’s tenant, after object-level authorization, without exposing invoice contents or creating an irreversible side effect.

That sentence gives the review something falsifiable. It names the actor, action, object, scope, decision point, and intended effect. If the author and reviewer cannot agree on such a sentence, the diff is not yet ready for a security judgment.

Do not infer scope from directory names. Authentication, authorization, session, cryptography, data-access, parsing, serialization, file-transfer, admin, secret, logging, CI/CD, dependency-loading, and AI tool changes normally require focused review. So do less obvious changes to route registration, middleware order, feature flags, cache keys, generated clients, ORM mappings, policy files, job triggers, model tool schemas, and customer-visible errors. Each can alter who may perform an action, which data the action reaches, or how a denial fails.

Raise the review bar when the path is public, privileged, multi-tenant, hard to reverse, connected to production control, or carrying sensitive data. An “internal” label changes likely entry points; it does not make administrative actions, credentials, or customer data harmless.

The visible patch is only the starting boundary. For the correction tool, the reviewer also needs the route and middleware configuration, the authoritative identity source, the invoice query, the policy contract, the tool registration, the log sink, and the relevant tests. Generated code and configuration belong in scope when they can change runtime behavior. Widening the reading is not scope creep; it is how the reviewer reconstructs the path that the patch changes.

Trace the request, not the file list

Start where untrusted data or delegated authority enters. In an HTTP handler this may be a path, body, cookie, header, or server-established session. Elsewhere it may be a queue message, scheduled job, CLI argument, uploaded archive, generated client, retrieved document, or model tool call. Mark which values come from the caller and which come from trusted server context.

For the invoice endpoint, invoice_id and proposed fields may come from the request. Actor and tenant identity must not. The correction therefore derives its tenant from authenticated context and rejects or ignores any caller-supplied tenant selector. Validation bounds field names, types, lengths, and allowed transitions before those values reach storage, logs, templates, or downstream tools.

Next, follow authority to the object and action that consume it. A route-level role check answers only whether the actor belongs to a broad class. It does not prove that this actor may create this correction for this invoice, in this tenant, for this purpose, now. The policy decision needs trusted actor and tenant identity, the target invoice, the requested action, and any state that changes the decision. If group membership or policy may change, cached and asynchronous work needs an explicit freshness rule.

Data access is part of the authority trace. A lookup can reveal existence, timing, metadata, or contents before a later denial. The corrected path queries within the authenticated tenant and either applies object policy before sensitive fields are returned or uses a store that can enforce the constraint in the query. Caches, search indexes, exports, and replicas must preserve the same scope. “We check before sending the response” is insufficient if unauthorized data has already entered application memory, a prompt, an error, or a log.

Then disturb the path. What happens when the invoice is absent, the policy service times out, membership is stale, validation fails, the database commits but the response times out, or security logging is unavailable? A permission lookup failure must not become permission. Retries need an idempotency key and an outcome lookup so uncertainty does not create a second correction. Errors should reveal no cross-tenant existence or sensitive payload. For a high-risk action, loss of required audit logging may block the action; for other paths, the service may degrade and alert. The choice belongs in policy and must be tested, not improvised in an exception handler.

Finally, trace the effect beyond the function return. The draft-creation credential should be scoped to that reversible action. It should not inherit general billing administration or the user’s entire session authority. Metrics, events, and logs should reconstruct actor, target, action, decision, reason, policy version, and request ID without copying correction contents, tokens, signed URLs, or invoice data unnecessarily.

Ask for evidence at each stopping point

A convincing explanation helps the reviewer understand the intended design. It does not show what the implementation does. Ask for the smallest evidence that can disprove the dangerous path.

For input and parsing, use tests with extra fields, invalid types, oversized values, encoded forms, and stack-specific parser edge cases. For query construction, show parameter binding and a test in which a valid actor supplies another tenant’s object ID. For authorization, exercise the wrong tenant, wrong object, wrong action, revoked membership, stale work item, and policy-service failure. For files and serialization, include traversal, archive expansion, content confusion, and malicious object cases relevant to the actual format.

Failure evidence should cover partial and uncertain outcomes, not only clean rejections. Force dependency timeouts, unavailable keys, malformed downstream responses, logging failure, rollback, and retry after an ambiguous result. Assert both the client-visible response and the side effects that did—or did not—occur.

Operational evidence matters when code changes a production control. Inspect the emitted security event, redaction behavior, alert ownership, rollback path, and any migration or feature flag that can bypass the new rule. A screenshot of a green pipeline is useful only when the reviewer can identify the exact commit, test, policy, configuration, and artifact it represents.

Automated checks can supply some of this evidence. Chapter 32 explains how to place and govern them. The code reviewer still decides whether the tests correspond to the claim. A clean scanner cannot establish business authorization, and a test named forbidden proves little unless its setup and assertions reproduce the blocked attack path.

Keep a local catalog of dangerous capabilities

A dangerous API catalog is a review-routing aid, not a universal banned list. Give it stack-specific names and an owner, then attach a required question and evidence expectation to each entry.

• Execution and interpretation: shell processes, dynamic imports, template evaluators, expression engines, deserialization hooks, and generated code. Which input can influence the program or arguments? Where are allow-lists, isolation, destination-aware escaping, and malicious-input tests?
• Queries and object locations: raw SQL, query builders, search DSLs, cache keys, and object-storage paths. Where are parameters bound, tenant and object constraints enforced, and denial behavior tested?
• Web output and navigation: HTML or Markdown rendering, redirects, response headers, cookies, and URL fetchers. Which output context applies? Are destinations, active content, cookie attributes, and server-side request targets constrained?
• Files and media: upload, download, archive extraction, image or document processing, and temporary files. How are type, size, expansion, path, parser isolation, malware handling, lifetime, and access controlled?
• Cryptography and credentials: signing, encryption, hashing, token generation, key loading, and signed URLs. Is the primitive approved for this purpose? Who owns the key, lifetime, rotation, audience, and misuse tests?
• Privileged automation: CI jobs, infrastructure changes, admin scripts, migrations, and model tools. Which identity executes the action? How narrow are its credentials, inputs, destinations, approval, logs, and rollback?

The catalog should point into the codebase rather than pretending that a family name is enough. A Java deserializer, a Rust unsafe boundary, a Go template, and a JavaScript child process require different language knowledge. Put concrete functions, framework defaults, safe wrappers, and test fixtures in maintained stack appendices or team guidance. In the review itself, verify versions and configuration; “the framework handles it” is a hypothesis, not evidence.

Write comments that can be resolved

“Is this safe?” transfers the reviewer’s work to the author. “Add more tests” does not say which failure threatens approval. A useful blocking comment names the path, the missing proof, and the requirement that closes it.

For the invoice query:

tenant_id comes from the request, and findInvoice(id) runs before object authorization. A caller can therefore select another tenant and distinguish an existing invoice from a missing one. Derive tenant identity from the authenticated context, constrain the lookup by tenant, and add a regression test in which a northwind user requests a southridge invoice and receives the same denial as an unknown object.

For the policy fallback:

A policy timeout enters the role-only branch, so loss of the policy service widens access. The correction requirement is object-and-action authorization. Return a denial or service-unavailable result without creating a draft, and show that behavior in a test that forces the policy client to time out.

For the log statement:

This debug event serializes the correction payload, which may contain billing data. Record actor, tenant, invoice reference, action, decision, reason, and request ID instead. Add a redaction assertion so future fields cannot silently enter the log.

The author may close a comment with code, a test or trace, a precise policy citation, or an approved exception. An exception names the exposed path, owner, compensating control, expiration, and follow-up. Confidence, familiarity, and “internal only” are not closure evidence.

Decide from the reconstructed path

Before approving, the reviewer should be able to answer these questions without filling gaps from memory:

1. What asset, actor, action, data class, and trust boundary does the change affect?
2. Where can untrusted input or authority enter, including routes, jobs, generated artifacts, configuration, and model tools?
3. Which server-established identity and policy authorize the exact object and action, and how fresh must that decision be?
4. Which queries, caches, indexes, exports, prompts, and logs can observe the data before and after the decision?
5. What occurs when validation, policy, key access, storage, a downstream call, or required logging fails?
6. Which negative test reproduces the most credible attack path, and what side effects does it assert are absent?
7. What privilege performs the effect, what evidence records it, and how can the change be disabled or reversed?

Return to the pull request’s one-sentence claim. The corrected implementation derives tenant identity from authenticated context, constrains the invoice lookup, authorizes the object and action without a permissive timeout path, limits the tool to draft creation, records metadata rather than payload, and proves cross-tenant denial and dependency failure. Now the reviewer can approve a behavior, not merely a collection of changed lines.

If any part of the path still rests on an assumption, turn that assumption into a test, a policy or configuration artifact, or a time-bound exception. That record is what allows the next reviewer—and the team responding to a failure months later—to see why the change was allowed to ship.