A one-line change to an authorization check can matter more than a hundred lines of interface code. Review effort should follow security impact, not diff size. If a patch changes who can read a record, where a secret is stored, or which network destination receives a request, it deserves a deliberate security review even if the diff looks simple.
Secure code review asks whether a change preserves the system’s intended security properties. That is different from checking formatting, maintainability, or a passing happy-path test. The reviewer needs to understand what the code trusts, what it protects, and what happens when input or a dependency behaves unexpectedly. This work belongs in ordinary development and authorized testing; it does not require probing systems outside your team’s scope.
Give the reviewer a change they can understand
A useful review starts before anyone opens the diff. The author should describe the behavior being changed, identify the relevant entry points, and state any security assumptions. “Update document endpoint” tells a reviewer little. “Let document owners grant read access; existing viewers must not grant access to others” gives them an authorization rule to check against the code.
Keep security-sensitive changes as focused as practical. Combining an access-control rewrite, a dependency upgrade, and a large formatting pass makes it harder to spot the lines that affect behavior. If the work cannot be separated, explain which commits are mechanical and which need close examination. Generated files and lockfiles may need checking, but they should not bury the human-written code that makes trust decisions.
- State the intended rule: Who may perform the operation, on which resource, and under what conditions?
- Name affected surfaces: Include API routes, background jobs, configuration, database migrations, and shared helpers when relevant.
- Provide evidence: Point to tests and explain any meaningful coverage gap.
- Flag deployment assumptions: Mention required secrets, permissions, feature flags, and rollout steps without putting secret values in the review.
The reviewer should still verify those claims. The author’s description is a map, not proof that every route through the code follows it.
Prioritize by what could go wrong
Not every pull request needs the same depth of security attention. Changes to authentication, authorization, payment or personal data, file handling, command execution, cryptography, and infrastructure permissions usually warrant a closer pass. So do new public entry points and changes that move data across a trust boundary. A small edit to shared middleware may affect more users than a large change to a private utility.
Identify the asset and the possible failure, then inspect the code most likely to cause it. For a new export feature, the asset might be other users’ records; the failure might be a query that selects rows before applying tenant restrictions. For an image-processing job, check whether uploaded content can consume excessive memory or whether output could be written somewhere unintended.
Prioritizing risk does not mean ignoring low-risk changes. A quick baseline pass can still catch committed secrets, disabled checks, and surprising configuration changes. It does mean spending more time on a new permission rule than on a renamed local variable.
Review the behavior, not only the edited lines
A diff shows what changed, but security properties often depend on code outside it. Trace a request through its entry point, identity handling, validation, business logic, storage, and response. Check who calls a modified helper and whether its return value means the same thing to every caller. If a function changes from “deny unless explicitly allowed” to “allow unless explicitly denied,” unchanged callers may become vulnerable.
For each sensitive path, ask:
- Where does the input come from, and which parts can an external user control?
- How is the caller identified, and where is permission for this specific action checked?
- Which data or operation becomes reachable after that check?
- What happens on missing data, malformed input, timeouts, and dependency failures?
- Can another route, job, or older client reach the same operation without equivalent checks?
This matters especially when an application has both HTTP handlers and background workers. Validating a field in one route does not protect a worker that reads the same field from a message queue. A user interface that hides a button is not an authorization control for the server operation behind it, either.
Check the location of the security decision
A permission check must happen before the protected action and use an authoritative source of identity and ownership. A request containing a document ID should not become authorized just because the client also supplied an owner ID matching its own account. The server should read the document’s owner or access grants from trusted storage and compare them with the authenticated caller. Check that every path, including early returns and alternate lookup methods, reaches that decision.
Look for assumptions that changed elsewhere
Renaming a field from userId to accountId may look mechanical until a caller starts treating an account identifier as an individual user identifier. Inspect the relevant types, database constraints, and tests. If the code relies on unique ownership, confirm that the migration or schema enforces it. Comments can describe an assumption the system does not actually uphold.
Use a consistent set of review lenses
A checklist helps you remember what to inspect; it cannot tell you how a particular feature should work. Use the lenses that fit the change rather than applying the same long questionnaire to every patch.
| Lens | What to inspect | Example review question |
|---|---|---|
| Access control | Identity source, resource ownership, role checks, cross-tenant queries | Can a permitted user act on a resource they do not own? |
| Input and output | Parsing, bounds, query construction, rendering, file paths | Does untrusted data reach an interpreter or sensitive path? |
| Secrets and data | Logs, responses, caches, configuration, retention | Could a credential or private field appear in diagnostics? |
| Failure behavior | Exceptions, retries, defaults, partial writes | Does an unavailable dependency fail closed where required? |
| Dependencies and operations | New packages, runtime permissions, deployment settings | Does the change expand privileges or expose a new service? |
For Java and C++, add language-specific checks where they matter. In Java, inspect deserialization boundaries, reflection, exception handling, and whether mutable objects cross an intended boundary. In C++, inspect ownership, lifetimes, indexing, integer conversions, and error paths around allocation or parsing. These are security concerns: an incorrect length conversion or use-after-free can undermine an otherwise sound access rule.
Combine manual reasoning with tools and tests
Automated checks find repeatable patterns. Dependency scanning can flag known issues in included packages; secret scanning can catch exposed credentials; static analysis can point to suspicious data flow. Linters and compilers catch other mistakes before a person reviews the code. Run the checks appropriate to the repository and investigate their results. A passing scan does not establish that an authorization rule is correct, and a warning does not prove a vulnerability.
Tests make a security claim reviewable. A test named “viewer cannot share document” says more than a broad controller test that checks only for a successful response. For access-sensitive changes, include allowed and denied cases, particularly neighboring roles and resources. For input handling, test malformed and boundary values alongside valid input. For failure handling, test a missing required lookup and an unavailable downstream service.
Prefer assertions about observable behavior to assertions that a private helper was called. A later refactor could preserve a helper-level test while exposing another path. Ask whether the test would fail if the security check were removed or applied to the wrong resource. That tells you more than the number of tests.
Reproduce suspected issues in a local, authorized development environment. Keep live personal data, real credentials, and production tokens out of fixtures and pull-request comments. A small synthetic example usually makes the failure easier to see.
Write findings that lead to a fix
A useful review comment identifies the behavior, its consequence, and the condition that triggers it. “This is insecure” leaves the author guessing. “The query filters by document ID but not by tenant ID; an authenticated tenant member could receive another tenant’s document if its ID is supplied” gives them a rule to verify and a place to investigate. If you are uncertain, distinguish what you have confirmed from what you still need to ask.
Separate blocking security findings from optional design suggestions. Marking every preference urgent makes serious findings harder to spot. At the same time, a potential unauthorized data read should not get lost among style comments. Agree on an escalation path for sensitive findings, especially if a review uncovers an existing issue beyond the proposed change. Discuss live secrets or exploitable production details in an appropriate private channel, not a public repository.
The author’s response should explain the resolution, not just mark the thread complete. A code change, a relevant test, and a brief explanation of how the new behavior satisfies the intended rule give the reviewer something concrete to reassess. If the team accepts a concern as a documented limitation, record who owns that decision and any follow-up work. Silence is not a risk decision.
Make approval and follow-up meaningful
Approval should apply to the version of the code actually reviewed. A significant revision after security approval needs another look, especially if it changes permissions, query construction, or failure handling. Repository protections can require fresh approval when new commits arrive, but reviewers still need to inspect what changed since their last pass.
Some risks appear only at deployment. A configuration default may differ between development and production, a migration may briefly leave data in mixed states, or a new job may receive broader permissions than its code needs. Review the deployment plan for sensitive changes and check that tests reflect realistic settings. After release, investigate an unexpected authorization denial or data-access pattern rather than simply removing the check.
Before approving a new document-sharing endpoint, check one detail in the denied-case test: does it request a document owned by a different account, or only a nonexistent document? A 404 for a missing ID does not show that the endpoint protects an existing resource.
