Security Code Review Interview: Prioritize Findings and Explain Exploitability

A security code review interview is a test of evidence and judgment. Finding a suspicious function name is only the start. You need to show how an attacker-controlled value reaches a sensitive operation, which protection is missing, what conditions make the path reachable, and why the resulting impact deserves attention before other findings.

Then write one review comment that includes a concrete trigger, a violated security rule, a focused repair, and a regression test. That is more useful than producing the longest possible list of concerns.

Establish the application boundary before reading line by line

Start with the entry point, the intended users, and the assets the change touches. An internal maintenance script and a public endpoint may contain similar code but expose very different paths. Ask how the handler is registered, which authentication middleware runs, and whether the change introduces a new trust boundary.

Official guidance: OWASP's secure code review guidance distinguishes baseline reviews from reviews of changes. It emphasizes understanding architecture, security requirements, data flow, and the effect of modifications on existing controls. OWASP Secure Code Review

For an original exercise, review a fictional invoice-export endpoint. A signed-in employee submits an invoice ID. The service loads the invoice, creates a CSV export, stores it, and returns a download reference. The intended rule is that employees may export only invoices their tenant and role permit.

Editorial inference: the export handler and finding ledger below are original exercises using synthetic data, not reports of a vulnerability in any real service.

Before making a finding, determine which parts of the implementation you can inspect. If the policy helper or storage adapter is missing from the exercise, state the uncertainty instead of inventing its behavior.

Follow one input all the way to its effect

A useful review trace has a source, transformations, a decision point, and a sensitive effect. In the export example, the invoice ID enters through the request, is parsed, reaches a repository lookup, and determines which rows are written into the export.

Parsing the identifier as an integer establishes syntax. Parameterizing the database query protects query structure. Neither establishes that the caller may export the selected invoice. A review that stops at a safe SQL call may miss the missing permission decision immediately around it.

Here is the deliberately incomplete original sketch:

principal = require_session(request)
invoice = repository.find_invoice(request.invoice_id)
csv = render_export(invoice)
download_ref = storage.save(csv)
return download_ref

The concern is not automatically confirmed just because the sketch lacks an obvious authorize call. Inspect whether find_invoice is already scoped by a trusted principal or whether the route uses an enforcement layer. If it loads globally and no later policy runs, you can describe the missing boundary precisely.

Official classification: CWE-862 describes missing authorization when software fails to perform an authorization check for an actor attempting to access a resource or perform an action. The category supports the label; the actual code path supplies the evidence. CWE-862

Explain exploitability without overstating what you know

For the export finding, identify the preconditions: the caller has an ordinary valid account, can reach the route, and supplies a known synthetic invoice ID outside their permitted scope. Then show that the global lookup feeds an export without the required permission check.

The impact is disclosure of the selected invoice data. Do not inflate that into arbitrary database access unless another reachable mechanism supports the claim. Similarly, a successful export of one controlled cross-tenant fixture proves that path, not the exposure of every data type in the service.

Use confidence language that matches the evidence. “Confirmed in the provided implementation” is appropriate when the relevant path and test are available. “Potential issue pending the repository helper's scope” is appropriate when an important enforcement layer is missing from view.

A useful interview sentence is: “If this helper is an unrestricted lookup, an ordinary user can export another tenant's invoice. I would inspect its implementation next; the route itself does not show an object policy.” That is a focused uncertainty, not a vague disclaimer.

When testing is permitted, use two known fixtures and the minimum action needed to demonstrate the boundary. The review does not need large-scale enumeration, unrelated customer data, or a destructive operation to be persuasive.

Prioritize the findings by impact and reachable conditions

Now imagine the diff contains four concerns: the missing export permission, credentials written into an exception log, an unbounded export size, and a misleading variable name. Do not give them equal prominence merely because they were noticed in the same file.

Observation	Evidence to establish	Initial review treatment
Export lacks object authorization	Reachable route and unrestricted object lookup	High-priority boundary violation once confirmed
Exception logs a credential	Actual sensitive value reaches retained logs	Assess credential privilege and log audience
Export has no size bound	Caller can request enough work to exhaust a resource	Assess reachable limits and operational impact
Variable name is unclear	Readability problem only	Lower-priority maintainability comment

This is an original prioritization exercise, not a universal severity scale. A credential with broad production privileges could outrank the invoice disclosure. An unbounded operation reachable anonymously at low cost could also become the most urgent issue. Explain which contextual fact changes the order.

Keep severity and confidence separate. A potentially severe issue with an unverified precondition is not the same as a demonstrated low-impact bug. Report both dimensions instead of hiding uncertainty inside a single dramatic label.

Write a comment an engineer can act on

A strong comment starts with the concrete behavior. For example: “This lookup is not scoped to the authenticated tenant. An ordinary Tenant A user can export the controlled Tenant B invoice, because the loaded object goes directly into the CSV. Enforce the export policy before rendering or storing the artifact, and add a cross-tenant denial test that also checks no export was created.”

That comment contains the trigger, mechanism, impact, correction, and verification. It is short enough to attach to the relevant lookup or policy boundary. It does not require the reader to infer the threat from a CWE label alone.

Avoid personal language about the author. Also avoid prescribing a broad rewrite when a focused policy integration would repair the issue. If the codebase already has a suitable authorization helper, identify that established path rather than introducing a parallel policy system.

A weaker comment says, “Security issue: validate input.” It confuses input shape with access control and gives no testable requirement. Improve it by naming which input relationship matters and which operation must be denied.

Distinguish a suspicious pattern from a confirmed flaw

A static analyzer may flag string construction near a database call. Follow the data into the actual execution API. A string used only as a log message is different from a string interpreted as SQL. A parameterized value is different from a dynamically chosen table name that cannot be bound in the same way.

Official classification: CWE-89 concerns untrusted input changing the intended structure of an SQL command. The question is how the database interprets the constructed command, not whether the code happens to contain concatenation. CWE-89

Likewise, a method named sanitize is not proof of safety. Inspect its contract and the context in which the output is used. An HTML-safe string may be inappropriate in a different interpreter, while a business-rule validator may intentionally do no output encoding at all.

Official guidance: OWASP notes both the usefulness and limitations of source-analysis tools. Automated findings need contextual assessment, and an absence of alerts does not prove the application has no security flaws. OWASP Source Code Analysis Tools

In an interview, show how you reduce uncertainty. Read one relevant helper, inspect one route binding, or propose one focused test. Repeating an analyzer's warning without examining the path adds little evidence.

Review changes to existing controls, not just new code

A small diff can remove a security property without adding an obviously dangerous call. Replacing a scoped repository method with a faster global cache lookup might bypass object checks. Moving authorization after artifact creation may leave forbidden data in storage even when the final response is denied.

For the export exercise, compare the old and new ordering. If the old code authorizes before rendering and the new code renders first for caching, ask what side effects now occur before the decision. The relevant regression may be data written to an object store rather than a successful HTTP response.

Also inspect alternate callers. A helper that used to be reachable only through an authenticated route may now be invoked by a queue worker or administrative API. Its security assumptions may no longer hold at every entry point.

Keep the review scope explicit. You do not need to audit the entire repository to inspect a changed trust assumption, but you should read enough surrounding code to establish whether the control still runs where required.

Verify the repair with both allowed and denied cases

For the missing export check, create a permitted invoice and a forbidden invoice under synthetic tenants. Verify the permitted export still contains the expected data. Verify the forbidden request produces no accessible artifact and no protected response content.

If the implementation queues export work, inspect the job and retrieval stages too. A denial at submission is not sufficient if another entry point can enqueue the same work. Conversely, a worker that rejects everything would pass a negative-only test while breaking the intended feature.

The regression test should fail against the faulty implementation and pass after the correction. If it passes both versions, investigate whether the fixture actually crosses a permission boundary or whether the assertion observes the relevant effect.

For the logging issue, test redaction through the actual exception path and retained sink. For the resource-limit issue, test the defined bound and failure behavior using controlled load. Match each test to its own mechanism instead of reusing one generic status assertion.

Handle disagreement by identifying the missing fact

Suppose the interviewer says the endpoint is internal. Ask what internal access means: a private network, authenticated employee access, or a service identity with a narrow policy. Internal reachability may change exposure, but it does not automatically make cross-tenant export intended behavior.

If the interviewer reveals a verified authorization wrapper, update the finding. Withdraw a concern that the new evidence resolves, and explain why. Maintaining an incorrect finding to appear decisive is weaker than showing a clear evidence threshold.

If time is short, present the highest-confidence important finding first, then identify one unresolved concern and the next inspection that would settle it. Do not bury a real boundary violation under a dozen speculative hardening suggestions.

Your final review can distinguish blockers, follow-up improvements, and questions. Each category should reflect an observed or explicitly conditional fact, with enough context for another engineer to reproduce your reasoning.

Sources and Further Reading

• OWASP Secure Code Review Cheat Sheet
• OWASP Source Code Analysis Tools
• CWE-862: Missing Authorization
• CWE-89: SQL Injection