Blog

A change from getProfile(currentUser.id) to getProfile(request.params.id) may look like a harmless refactor. But now the caller chooses the profile ID. If the route does not check whether the signed-in user may view that profile, the change could expose someone else's data. A secure review catches the shift in who controls the ID, not just whether the code compiles.

For a new developer, secure code review is a repeatable way to look for unintended behavior and missing safeguards in a proposed change. It will not find every vulnerability. The aim is to understand what the change accepts, what it can access, and what evidence supports its security assumptions.

Establish the review boundary first

Read the pull request description, then compare it with the diff. Identify the intended behavior, the files changed, and the data affected. A small diff can cross an important boundary: a new route may expose an existing helper to untrusted callers, while a dependency update may alter behavior with little application code changing.

If the expected behavior is unclear, ask the author. You cannot assess authorization against a requirement as vague as “users can see profiles.” Are profiles public, private to their owners, or visible to administrators? That distinction belongs in the review context.

• Entry points: Which HTTP handlers, command-line arguments, files, messages, or background jobs can reach the changed code?
• Assets: Does it handle personal data, credentials, account state, money, files, or privileged operations?
• Callers: Who can trigger it, and what identity or role is available at that point?
• Outputs and effects: What can it return, write, delete, log, or send to another service?

Stay within the repository, test environment, and systems you are authorized to inspect. If production behavior matters, request approved documentation or a safe test account rather than probing live users' data.

Read the change in two passes

On the first pass, trace the feature without judging individual lines. Find the entry point and follow the data to its destination. Note where input is parsed, an authenticated identity is attached, a permission decision is made, and the result becomes visible. You should come away with a short mental model of the request path.

On the second pass, read the diff line by line against that model. Open surrounding functions and relevant callers; the diff is not the whole program. A check inside a helper may look reassuring but never run on the new path. An apparent missing check, on the other hand, may live in a route wrapper. Verify that the wrapper applies to this route.

Write down assumptions you have not verified: “Does this endpoint require login?” or “Can this job receive a filename from a customer?” Check them against code, tests, or a documented contract instead of quietly treating them as facts.

Follow one value end to end

Pick a caller-controlled value, such as a profile ID, search term, uploaded filename, or page size. Follow it through transformations and function calls. Where does it affect a database query, file path, response, or authorization decision? Check whether the safeguard at that point addresses the actual risk.

Converting a route parameter to an integer checks its form; it does not grant permission to view the record. A parameterized query may prevent SQL injection, but it does not establish permission either. Each safeguard answers a different question.

Use security questions tied to the code

A checklist is useful when it sends you back to concrete behavior. Apply these questions to each changed entry point, spending more time where a mistake would have the greatest consequence.

Can the caller choose data they should not control?

Look for request fields, environment values, imported files, or messages that become trusted decisions. A client-supplied isAdmin field should not determine a user's role just because it appears in valid JSON. A client-supplied account ID may identify the target resource, but the server must still decide whether the authenticated caller can act on it.

Check validation where the value is used. Type and size limits matter: an unbounded page size can strain memory or a database even when it has valid syntax. A sort field should accept only expected values, rather than an arbitrary string placed into a query. Name the boundary in your review comment instead of writing only “validate input.”

Does authorization apply to the specific resource and action?

Authentication tells you who the caller is. Authorization determines whether that caller may perform this action on this resource. Check reads and writes, including exports and background tasks. A login requirement will not prevent one user from seeing another user's invoice if the route fetches it by ID without checking ownership or another applicable permission.

Find the check that runs before data is returned or a file is written. Examine failure behavior too: if the permission service is unavailable, does the operation stop or proceed without a decision? For long-running operations where roles or ownership can change, check the project's policy on when authorization must be reevaluated.

Where does untrusted content become executable or visible?

Inspect database calls, HTML rendering, shell commands, templates, and file paths according to how they use the data. Parameterized queries protect values passed as parameters; selectable identifiers such as column names generally need a fixed allowlist. HTML output needs encoding appropriate to its context. Encoding for plain text is not automatically suitable inside a URL or script. Where possible, use a direct API or an argument-array form instead of building shell commands from user strings.

For file operations, check how the base directory is chosen and how the final path is resolved. Rejecting the literal text ../ is not enough: normalization and platform rules can change the effective destination. For uploads, prefer a server-chosen directory and generated storage name. Check file type and size requirements separately.

Can failures disclose secrets or weaken controls?

Trace error paths as carefully as successful ones. Logs and responses can expose tokens, session values, private records, or internal details. A response can say an operation failed without returning a stack trace. Logs can keep a request identifier and safe error category without recording a full credential or sensitive payload.

Check timeouts, malformed input, and partial failure. If a payment update succeeds but an associated permission update fails, the system may be left in an inconsistent state. If an exception skips cleanup, a temporary file may remain readable. The remedy depends on the application; first identify the state left behind.

Review the evidence, not only the implementation

Tests reveal which behavior the team expects to preserve. Compare that behavior with the security claim you are assessing. A test showing that an owner can read a record says nothing about whether another signed-in user can read it. For an authorization change, useful cases often include an allowed caller, an unauthenticated caller, and an authenticated caller without access. For input handling, test a normal value and a relevant boundary or malformed value.

Run the project's documented tests locally when practical, but do not treat a green suite as proof of security. It may miss the new route, a denial case, or a production-only configuration. Treat a static-analysis warning as a lead, not a verdict: check whether untrusted data can reach the flagged operation and whether a safeguard applies.

Give dependency and configuration changes the same attention. A new library may introduce a data flow or permission requirement; a configuration default may expose a debug endpoint without changing its handler. Check that code, fixtures, and sample configuration contain no committed secrets. If you find one in the proposed change, do not repeat it in a review comment. Follow the team's process for removal and, when necessary, rotation.

Prioritize findings by plausible impact

Time is limited. Separate a demonstrated issue from a possibility that needs more investigation. Can an untrusted caller reach the path? What can they influence, and what data or operation is affected? A missing ownership check on a private-record endpoint usually deserves attention sooner than a theoretical edge case in a development-only script. An unusual line is not automatically a vulnerability.

A short review note can separate observation, consequence, and next step:

• Observation: The route reads invoiceId from the request and fetches that invoice, but this path does not compare its account with the authenticated caller's account.
• Consequence: A signed-in user may be able to retrieve an invoice belonging to another account, subject to any controls elsewhere on this path.
• Next step: Confirm the intended sharing rule, add the resource-level authorization check, and test access with an unrelated account.

That gives the author more to work with than “insecure endpoint,” while leaving room for a control you may have missed. If your concern depends on an assumption, say so: “If this job accepts filenames from external requests, the resolved path needs review.”

Choose a small, safe verification

When reading the code does not settle a question, use the least disruptive check that will. Inspect a call site, add a unit test, or use synthetic records in an authorized development environment. Do not copy production personal data into a local test, scan services without permission, or try to access someone else's account to confirm a suspicion.

For the profile-ID change, a focused regression test could create two test users and a private profile owned by the first. Call the route as the second user with that profile's ID. Assert that the response contains no profile data and follows the application's expected denial behavior; then confirm the owner can still access it. Test the observable result, not merely whether a particular helper was called.

Sometimes you need an answer, not a code change. If the requirements do not say whether administrators can access deleted records, do not invent a permission rule in a review comment. Ask for the rule to be documented, then compare the implementation and tests with it.

Leave a review another developer can use

Group comments by issue, cite the relevant path, and distinguish blocking security concerns from optional improvements. Explain a shared flaw once rather than repeating the warning on every affected line. If several routes use one helper, identify the helper and name the routes that need coverage. Acknowledge controls you verified, especially when they are easy to miss in the diff.

Before approving, revisit code changed in response to your comments. A new authorization check may protect the original route but miss a second caller; a new test may cover only the allowed case. Resolve comments against the final code, not a promise to fix it later.

For the profile route, find the exact call that fetches the requested ID and identify the permission decision that runs before its data is returned. Keep the second user's denial test beside the owner's success test. The boundary will be easier to see the next time that route changes.