Inverted Code Review: Clearing the One Handler That Is Actually Safe

Most code review exercises ask you to find the bug, which rewards pattern matching: scan for a dangerous function, name it, move on. Real review is the opposite shape. You have to account for every path before you can sign anything off, and a handler is only clear once you have explained why each of its inputs cannot hurt you.

These four handlers are deliberately uniform. They come from one file, share a style, all carry login_required, and all read attacker-influenced input. The decorator is the trap: it proves the caller is signed in and nothing else. It does not parameterise a query, escape a response, or validate a destination.

get_order interpolates order_id into SQL with an f-string. The route captures it as a string, so a payload reaches the statement intact and can terminate the condition, which breaks both the query and the user_id ownership check sitting in the same string. render_receipt concatenates the note parameter into markup returned as text/html, which is reflected XSS. fetch_logo requires the URL to start with https:// and then requests it from the server, which allows link-local metadata addresses and internal service names, the classic SSRF bypass.

update_contact is the one that survives, and it survives on three specific properties: input validated with a full-match regular expression and a length bound before use, a parameterised statement so the value never becomes statement text, and a WHERE clause keyed on session['user_id'] so the row being modified is chosen by the server, not the caller.

• Answering get_order because it checks user_id. That check lives inside the same injectable f-string, so a payload can neutralise it. An ownership clause that an attacker can edit is not an ownership clause.
• Answering fetch_logo because it validates the URL. Checking the scheme is not validating the destination. https://169.254.169.254/ passes that test.
• Answering render_receipt because order_id uses the int converter. The identifier is fine; the note parameter is what reaches the HTML response unescaped.
• Treating login_required as the deciding factor. All four handlers have it, so it cannot distinguish them.
• Stopping at the first vulnerability found. The question asks which handler is clear, so all four have to be reviewed.

Each of the three flawed handlers has a well-established fix, and update_contact already demonstrates the pattern the others should follow.

• Injection. Always use bound parameters. Never build SQL with f-strings or concatenation, and constrain route captures with the correct converter, for example <int:order_id>.
• Cross-site scripting. Render through a template engine with contextual auto-escaping instead of concatenating strings, and add a Content-Security-Policy so a missed escape is not immediately exploitable.
• SSRF. Resolve the hostname and reject private, loopback and link-local ranges, re-check after redirects, use an allowlist of permitted hosts, and route outbound fetches through an egress proxy.
• Authorisation. Scope every query by the session principal, as update_contact does, so ownership is enforced by the server rather than asserted by the request.
• Process. Add linting for string-built SQL and unescaped response construction so these patterns fail CI rather than review.