Review the proposed patch against the original harmful behavior, not only against the diff or a passing check. Read the verified finding and affected revision, trace all relevant paths through the changed control, check that legitimate behavior still works, and record what remains untested. A patch may be safe to merge as an exposure reduction while the vulnerability stays open. Approval to merge means the change meets the team's release bar; it is not evidence that the issue is fixed in a deployed version. If a changed path or security control cannot be understood, pause the security sign-off and request the missing context.
OWASP's Secure Code Review Cheat Sheet recommends a diff review that considers affected components, existing controls, trust boundaries and new attack vectors. For a security patch, write the failed rule in one sentence first. That sentence becomes the review's acceptance target.
An illustrative patch with an incomplete win
Illustrative case, not a customer finding: Ivy Notes lets account members write a note that staff can read in an administration dashboard. A verified finding at revision N4 says a note author's untrusted text reaches the dashboard as HTML rather than plain text. The proposed pull request changes the dashboard template to use the framework's normal text rendering. Its author points to a screenshot showing that a synthetic note containing angle brackets appears as text.
The change improves the dashboard path. During review, a staff engineer traces the same stored note into a customer-facing shared-note view, where a browser-side component still places the value into an HTML sink. The pull request does not change that component. It also changes a shared rendering helper used by ordinary formatting features, so it may alter expected display behavior. OWASP's Cross Site Scripting Prevention Cheat Sheet explains why encoding or safe rendering must fit the output context; a fix at one display point cannot establish safety at another.
The review conclusion is not “the patch is bad.” It is: the dashboard path appears addressed; the shared-note path remains open; the shared helper's intended output needs checking. The team can either extend the pull request to both paths or merge the narrower improvement with an explicit open finding and follow-up owner. Neither a screenshot nor a scanner's silence can settle the unreviewed sink.
Review the actual change in three comments
This browser-DOM sketch makes the patch's scope inspectable. It is illustrative, has not been executed, and uses ordinary elements rather than a framework-specific template:
// Dashboard: note is specified as plain text.
- dashboard.innerHTML = note.text;
+ dashboard.textContent = note.text;
// Shared rendering helper also serves intentional rich-text descriptions.
- target.innerHTML = value;
+ target.textContent = value;
// Shared-note view, unchanged by this proposal.
sharedNote.innerHTML = note.text;
Comment 1 — property satisfied in one place: “Dashboard now treats the note as text in an ordinary element. Inspect the actual call binding and supplied value, then preserve a normal note and a literal-angle-bracket check in the controlled validation record.” OWASP's XSS guidance distinguishes text sinks from HTML parsing; a screenshot alone does not establish every caller.
Also inspect the target element. Setting textContent replaces its children, so a container holding controls or links may lose them. Apply the text sink to the intended text region, and keep this advice scoped to ordinary display elements rather than script or style contexts.
Comment 2 — legitimate behavior changed elsewhere: “The shared helper serves rich-text descriptions. This global change would display their markup literally. Confirm the feature contract or separate the plain-text note renderer from the reviewed rich-text renderer. Do not restore raw HTML for note text to preserve unrelated formatting.”
Comment 3 — finding still has a surviving sink: “The shared-note view still parses note.text as HTML. Either include that plain-text view in this patch or retain the open path under a named follow-up and release decision. The dashboard fix does not cover it.”
The author revises the proposal: applies textContent directly in both plain-text note views, restores the unrelated rich-text helper to its previously reviewed implementation, and documents that helper's callers as unchanged scope. Security checks that the restoration does not reconnect either note view to raw HTML and that the helper's previous safety claim remains applicable. A preexisting helper is not safe merely because it was unchanged.
Record the resulting merge recommendation
Two outcomes can be acceptable under different facts:
- Complete note-view proposal: both plain-text sinks use the checked text path, relevant callers are mapped, and authorized validation still needs to confirm the head artifact. Recommend merge only when the team's required checks complete; keep deployed finding closure pending rollout and reassessment.
- Narrow dashboard proposal: shared-note sink remains open, but the dashboard improvement is independently sound and introduces no helper regression. Recommend it only as partial work under an explicit release decision, with a follow-up owner and any necessary restriction. Do not mark the finding fixed.
For the revised two-view proposal, the review output names what changed and what did not: dashboard and shared-note text contexts corrected; intentional rich-text helper retains its separately reviewed contract; no broad claim about every HTML sink. If the rich-text helper's safety evidence is missing, record that separate uncertainty and assess whether it blocks this release. Do not silently substitute a presumed “approved helper” for evidence.
The review is complete when the author can act on each comment and the reviewer can state the scope of their recommendation. Post-merge verification remains the job described in 031, and selecting additional affected callers is covered by 034.
A review note filled for the revised proposal
| Question | Supplied proposal at N5-revised | Review call |
|---|---|---|
| Dashboard and shared-note plain text | Both use the ordinary-element text sink; source bindings reviewed in the exercise. | Source correction supported for these views. |
| Rich-text description behavior | Global helper change withdrawn; separate reviewed renderer retained. | Preserve formatting; verify its existing contract remains applicable. |
| Controlled behavior evidence | No actual browser checks performed for this article. | Required checks remain descriptive, not reported passes. |
| Deployment | Proposal only, no released artifact. | Finding stays open pending release verification. |
| Merge recommendation | Ready for the team's required controlled checks and final review of N5-revised. | Approve only when those checks support the bounded proposal; no blanket security sign-off. |
Review the revised head, including the parts that were restored
A review comment can be correct while the final patch still differs from what the reviewer intended. In Ivy Notes, the author both narrows the text-sink changes and restores the rich-text helper. Read the final proposed revision after those edits. Do not approve based only on the earlier discussion or the fact that each comment has a resolved badge.
Compare the final patch with three accepted properties: the dashboard treats note text as text; the shared-note view does the same; the unrelated rich-text feature retains its separately reviewed behavior. Then trace the actual values and elements at each call. If one view still reaches the shared helper through an alias, the author's description is incomplete even if the visible diff looks sensible.
A practical review note can have one row per property:
| Property | Evidence at the revised head | Remaining check |
|---|---|---|
| Plain dashboard note | Exact call writes note text into the intended ordinary text region | Confirm the displayed result and retained controls in the authorized build. |
| Plain shared note | Separate view no longer passes note text to an HTML sink | Check the alternate view and its actual bundle. |
| Rich description | Restored renderer and callers match the bounded prior contract | Reassess any changed configuration or caller assumption. |
This is not a request to repeat every earlier review. It is a comparison between the agreed outcome and the artifact actually proposed for merge. Reuse unchanged evidence explicitly, and reread the parts whose behavior or binding changed.
Check useful behavior at the same boundary
A security fix should preserve the legitimate action unless the team deliberately chooses a restriction. For Ivy, ordinary notes should remain readable, line breaks and long text should have acceptable presentation, and controls outside the text region should remain available. The rich-text description feature should keep the formatting its contract permits. These are functional expectations to validate; no browser result is asserted here.
Choose fixtures that expose the actual change. A normal sentence checks the everyday display. Literal angle brackets help distinguish text display from HTML interpretation in that specific sink. A note region containing adjacent action controls checks whether the patch replaces a container too broadly. None of these individual observations establishes every possible browser safety property, but together with source review they can challenge the proposed implementation.
Where the application uses a framework, inspect the framework's actual output semantics and the specific component. Do not mechanically translate a DOM sketch into a framework patch. The teaching example establishes the desired separation between plain text and intentional markup; the real implementation must enforce that separation using its own supported APIs.
Leave a merge recommendation that remains useful after merge
State the reviewed revision, the properties supported and the known open work. A useful recommendation says, “The revised source addresses both plain-text note views and preserves the reviewed rich-text contract; complete the declared controlled checks, then merge under the normal release process. Deployment closure remains pending.” That tells the release owner what the review did and did not establish.
If the team merges only the dashboard change, retain the shared-note path in the current work queue with a named owner and any operational restriction. Do not bury it in a resolved review comment. The merge can still be a sensible incremental improvement, but the finding's overall status must reflect the remaining path.
Finally, check the release artifact against the reviewed revision before claiming the change protects users. A later conflict resolution, backport or rollback can alter the relevant code. The pre-merge review is valuable evidence of the proposed source; the post-merge record must establish which source and configuration actually reached the assessed environment.
What if the deadline is before the complete fix?
If the full patch cannot be ready, decide explicitly whether a narrower change reduces exposure without introducing a worse failure. Record which path it protects and what remains open; give the remaining work an owner and date. A compensating control may be needed, but it has its own scope and verification. Do not mark the finding fixed because a pull request title says “security fix,” and do not state that an illustrative source trace proves an executed exploit.
Make the recommendation precise enough for the merger to use. A reviewer can approve the code-level invariant for the current candidate while requesting release evidence before the vulnerability record closes. If the diff is still missing the required guard on one caller, say that the candidate is not ready and identify that caller. “Looks better” gives neither decision.
Record whether each supporting check was observed, supplied by another reviewer or only proposed. The review may combine source reasoning with validation evidence, but a suggested negative case must not appear in the completed-results column. If a new commit changes the decisive function after approval, compare that change with the recorded invariant and renew the relevant review. Unrelated cosmetic edits may require less work than a changed access predicate, but neither should be assessed solely from the PR title.
For the next proposed patch, write the three concrete review comments and resolve each against the proposed head before approving it. After merge and deployment, use the new-commit fix-verification guide. If the team deliberately ships a narrower change, use the compensating-control record and keep the remaining path visible.