An actionable security code review comment names the behavior at risk, points to the changed path, asks one answerable question or change, and says what evidence would settle it. Write it against the pull request's revision, not a scanner category in isolation. If you cannot yet tell whether a control exists, ask for that control's location or a focused check; do not call the pull request vulnerable. If the path and missing control are established, request a specific protection and a way to check it. This keeps the review useful to the author while preserving the uncertainty the reviewer still has.
Security review is about the effect of a change on trust boundaries, not merely whether a new line looks suspicious. The OWASP Secure Code Review Cheat Sheet recommends inspecting changed components, the controls they depend on, and the path from input to sensitive operation. The comment is the handoff from that analysis to an engineer who can change or explain the code.
Worked scenario: an export route gains a new lookup
Illustrative repository and pull request: Harbor Orders is a multi-account order system. Pull request 84 adds GET /exports/{exportId}. The changed handler authenticates the caller, fetches a record by exportId, and returns a signed download link. A neighboring list endpoint filters exports by account. The new handler calls a different lookup function. The reviewer has the PR diff and repository source at commit abc123, but no proof about the download service's authorization behavior or whether this route is deployed.
An unhelpful comment would be: “IDOR. Critical. Add auth.” It hides the real question. Authentication is already present; the question is whether the caller may access that export. OWASP's Authorization Cheat Sheet explains why access checks belong on each request and should deny access by default. The reviewer traces the route, the lookup and the link creator, then records what is known:
| Review fact | Evidence at abc123 |
Meaning |
|---|---|---|
| Caller must sign in | Route middleware in the diff | The route is authenticated. |
Caller controls exportId |
Path parameter reaches the lookup | A user can choose which record to request. |
| List route filters by account | Separate list handler | This does not establish a check on direct download. |
| Download route checks account ownership | Not found in handler or lookup read so far | Needs a deeper check before claiming a flaw. |
| Link service checks ownership | Not yet reviewed | Decisive missing fact. |
The reviewer now leaves a comment on the lookup line. GitHub's review guidance describes how to comment on changed lines and collect remarks into a review; the wording below is a method, not a GitHub feature:
Question on this lookup: An ordinary signed-in user can choose
exportId. The list endpoint filters by account, but this direct lookup does not appear to pass the caller's account. Could you point me to the check that confirms this export belongs to the caller before a download link is returned? If the link service does not make that check, please bind the export read to the caller's account and add an authorized-account / other-account check using synthetic records. I have not reviewed the link service yet, so this is a possible cross-account read, not a confirmed exposure.
That comment gives the author two legitimate responses: show the downstream control with a code reference, or change the path and demonstrate the rule. A line citation proves the lookup exists. It does not prove that another account's file can be obtained, and no live exploit is implied.
Continue the thread until the question is answered
The following replies extend the constructed PR. They show what changes the decision rather than what a reviewer should say in every repository.
Author, still at abc123: “The list endpoint already filters by account, so users cannot see other export IDs.”
Reviewer: “That explains the list response. This new route accepts an ID directly and does not call the list handler. Please show the check before createDownloadLink returns the selected export. Difficulty discovering an ID is separate from authorization to download it.”
The author supplies this simplified link-service contract:
createDownloadLink(exportId, trustedCaller):
export = loadExport(exportId)
require currentMembership(trustedCaller, export.account)
return signStoredObject(export.objectKey)
For this exercise, the product policy allows every current account member to download that account's exports. Assume the supplied implementation shows that trustedCaller comes from authenticated server context, failure stops execution, and the object signed is the one just authorized. If export access instead requires a workspace role or resource permission, membership alone would not satisfy that policy. The reviewer also checks that the new handler must call this service and cannot use a raw signing branch. Those facts answer the original question even though the first lookup itself was not account-filtered.
Reviewer, abc123: “The mandatory link service checks current membership against this export's owning account before signing its stored key. I withdraw the request to add a second handler check. This direct-download concern is resolved for the supplied revision and call path. I have not established which release deploys the route.”
Withdrawing the request matters. “Defense in depth” should not be a way to avoid acknowledging that counterevidence defeated the alleged missing control. A separate architectural preference can be discussed with its own benefit and cost; it should not retain the vulnerability label.
Reopen for the changed value, not merely a new timestamp
The author pushes def456. To support legacy exports, the handler now supplies objectKey from a request query parameter when the stored export key is empty. The revised service contract accepts that fallback key as an additional argument. It still authorizes the export record, but the signing call can use the request-supplied key. The previously reviewed property—authorize the object you deliver—no longer holds.
Reviewer, def456: “The membership check protects the selected export, but the fallback key can name a different object. Please keep key selection bound to the authorized export. For legacy records, either resolve the key from trusted stored metadata with the same ownership relationship or deny download until migration. Do not sign an arbitrary key from this request.”
The author chooses denial for unmigrated legacy records because there is no authoritative key mapping yet. At ghi789, both ordinary and legacy branches use the stored key, and the missing-key branch returns a controlled unavailable response before signing. The reviewer checks the call contract and branch termination. The proposed acceptance packet has three cases: an authorized export returns its own object; a foreign export fails before signing; an authorized legacy export without a stored key returns unavailable even if the request supplies a key. Normal download and legacy unavailability are both intentional outcomes.
The final comment can be concise:
At
ghi789, the request key no longer reaches the signer. The mandatory service authorizes the same export whose stored object is delivered; missing trusted metadata denies delivery. Source concern resolved within this path. Before release, provide the agreed synthetic-case evidence and effective route binding. Legacy export availability remains a product/migration task, not a reason to reintroduce arbitrary key selection.
No signed link was created or foreign object accessed for this article. The exchange shows how a reviewer changes from question, to withdrawal, to a narrower change request as the actual evidence changes.
Choose the right tone for the evidence
Use a question when the control is unknown: “Where is the tenant check applied before this response?” Use a change request when the path is clear and the control is absent: “Please include the current account in this lookup and deny a foreign record before signing the link.” Use an observation when the issue is only hardening: “This shared helper could make the policy easier to audit; the current route appears to enforce it already.” These are different outcomes, so do not assign them the same urgency.
Avoid a public PR comment that includes real credentials, customer data, exploit payloads or a private report excerpt. Put sensitive evidence in the team's approved restricted channel and link the authorized record if the review platform's visibility permits it. A reviewer can still describe the behavior and requested fix without publishing the secret. NIST's Secure Software Development Framework treats vulnerability identification, response and evidence as development practices; the local review procedure should define where that evidence is retained.
Leave a useful record when the conversation cannot finish
If the service implementation is inaccessible, the final reply should identify the missing function, its evidence owner and the release decision that remains blocked. “The link service probably checks it” cannot close the question. A credible urgent concern can use the incident or containment route while the technical verdict stays unresolved.
Keep one thread per disputed invariant when possible. The original direct-download check, a later request-key fallback and legacy availability are related but different questions; a resolved badge on the first should not hide the second. At each new push, inspect what changed in the previously decisive path and record the revision your conclusion covers.
Separate a resolved comment from a completed repair
The HarborOrders exchange has several possible endings, and each deserves its own wording. At the original revision, the downstream membership and export checks may justify withdrawing the handler-level request. At the later fallback revision, the same explanation no longer settles the issue because the requested object is chosen differently. At the repair revision, the denial behavior may settle the reviewed control question while an operational problem remains for unmigrated exports.
A concise resolution note could say: “The current candidate denies the unmigrated path rather than accepting an unbound storage key. That answers the reviewed authorization question. Migration of legitimate older exports remains assigned to the service owner.” This preserves the difference between restoring an access invariant and restoring every legitimate workflow. Do not close the migration work merely because security review can proceed.
The reviewer should also identify what would invalidate the resolution. If a follow-up change reintroduces a caller-supplied object key, the current account-bound lookup reasoning must be revisited. If the export policy changes from all account members to a smaller authorized group, the earlier membership check no longer proves the new rule. These are specific reopening conditions, not an instruction to repeat the entire assessment whenever any file changes.
Keep an actionable comment useful across revisions
The developer should not have to infer the current request from a long thread containing rejected hypotheses. Maintain a short current-state summary with the candidate revision, supported concern, decisive counterevidence and remaining action. Preserve the discussion below it so another reviewer can see why the original request changed.
In this exercise, a useful summary has a deliberate correction: the reviewer initially suspected a missing handler check, then learned the service performed that check, then identified a separate fallback that escaped the service's object binding. Leaving only the first accusation at the top would encourage a redundant handler check and conceal the actual defect. Acknowledging the changed conclusion is part of technical accuracy.
Ask the author to show the exact changed caller and the authorized and denied behaviors needed for the current question. Avoid asking for an unrelated full-system proof to resolve one comment. If another path needs review, name it and assign that work separately. The resulting exchange can be both demanding and efficient: the developer understands the invariant to restore, and the reviewer has a bounded basis for accepting or rejecting the supplied evidence.
For the next review, ask for the smallest artifact that distinguishes a safe implementation from an unsafe one. If it supports the author, say so and withdraw the overbroad request. If it supports a needed repair, move the approved work into a useful fix ticket. Use the concise finding-comment guide when the issue is already established.