← Back to System Internals

Reviewing a Security-Critical Change

The interviewer hands you a diff for a service that moves money and asks “what's missing?” The senior answer is a checklist you run, not a vibe you have.

This is the depth question in the code-review track. You're handed a pull request for a microservice that handles secure transactions — a payments path, a consent write, a credential exchange — and asked what a senior reviewer would flag. The trap is to comment on style and a missing null check and stop. The signal is that you run the change through a repeatable security-and-correctness checklist, name the classes of risk that would cause a SEV or a breach, and then describe how you'd deliver that feedback so it lands and the diff still ships. What follows is the checklist a senior reviewer runs, plus a wrong-versus-right snippet, plus how to give the feedback.

What this question is really testing

Whether you have an internalized threat model for code that touches money, secrets, or user data — and the discipline to apply it the same way every time rather than relying on catching something clever in the moment. The interviewer wants to hear that you review for the classes of failure that hurt (a breach, a double-charge, a leaked secret, a data-corruption SEV), that you weight the failure and error paths as heavily as the happy path, and that you can prioritize: what actually blocks the land versus what's a follow-up. They're also listening for whether you'd deliver a security block without shaming the author, because a reviewer who's feared gets routed around.

How to answer

• Start with the threat model, not the code. Say out loud what this service protects — money movement, credentials, PII — and who the adversary is. That framing is what makes the rest of the checklist purposeful rather than a laundry list.
• Run the checklist and name the classes. Missing tests on the failure paths; unhandled exceptions and error paths; races and concurrency on shared state; secrets handling; authn/authz; input validation and injection; idempotency; encryption; PII in logs and an audit trail; rate limiting and abuse; dependency and supply-chain risk; and a safe rollout. Group them — don't just recite.
• Weight the failure paths. Most bugs in a transaction service hide in the error branch: the retry that double-charges, the exception that leaves a half-written record, the timeout with no compensation. Say explicitly that you review the unhappy paths hardest.
• Separate blocking from follow-up. A hardcoded secret or a missing authz check blocks the land; a metric you'd like on the fallback branch is a fast-follow. Be explicit, so the author knows exactly what stands between the diff and green.
• Show how you'd give the feedback. Lead with the risk and the reasoning, link the standard or a past SEV, and offer the fix — “this reads the key from an env var; route it through the secrets manager, here's the pattern” — so the block is a path forward, not a wall.

What the interviewer is looking for

• A threat model stated first, so the review is purposeful, not a checklist recital.
• The high-severity classes named — secrets, authz, injection, idempotency — not just tests and null checks.
• The failure and error paths reviewed as hard as the happy path.
• Clear blocking-versus-follow-up prioritization.
• Feedback framed to teach and unblock, not to shame — a security reviewer people want, not avoid.

The checklist a senior reviewer runs

Group your findings the way you'd group them in the review so the author can act on them. Everything above the fold blocks a land; the operational hardening is where you separate must-fix from fast-follow.

Area	What you look for	Why it’s here
Tests & failure paths	Unit and integration tests that exercise the error branches, retries, and partial failures — not just the happy path	The bugs that hurt live in the paths no one tested
Error handling	No unhandled exceptions; no bare except; errors surfaced, not swallowed; resources released on the failure path	A swallowed error in a money path is a silent data bug
Concurrency	Races on shared mutable state; check-then-act on balances; correct locking or an atomic compare-and-set	Two concurrent requests must not both spend the same funds
Secrets & credentials	No hardcoded secrets or keys; pull from a secrets manager; least-privilege scopes; rotation possible	A committed secret is a breach waiting in git history
AuthN / AuthZ	Caller is authenticated and authorized for this resource; no missing object-level checks (IDOR)	Authn without per-object authz is the classic escalation
Input validation & injection	Validate and bound all external input; parameterized queries; no shell string-building; no unsafe deserialization	SQL, command, and deserialization injection all start here
Idempotency	Money movement carries an idempotency key; a retry or duplicate delivery cannot double-apply	At-least-once delivery means retries will happen
Encryption	TLS in transit; sensitive fields encrypted at rest; correct algorithms, no home-rolled crypto	Transactions and PII must be protected on the wire and on disk
PII in logs & audit	No card numbers, tokens, or PII in logs or error messages; a tamper-evident audit trail of who did what	Logs leak, and money movement needs a forensic record
Rate limiting & abuse	Per-caller limits, bounded retries with backoff, and abuse/fraud guards on the endpoint	An unbounded money endpoint is a fraud and DoS surface
Dependencies	New libraries vetted, pinned, and scanned; no unmaintained or typo-squatted packages	Supply-chain risk enters through a one-line import
Safe rollout	Behind a gate/flag; canary or staged ramp; a fast, tested rollback	You want to turn a bad change off in minutes, not redeploy

Wrong versus right

The two anti-patterns you flag most often on a transaction path are a hardcoded credential and an unchecked, non-idempotent charge. Here's what you comment on, and the fix you offer alongside the block.

What you'd block

# WRONG: hardcoded secret, unchecked exception, no idempotency
API_KEY = "sk_live_9f3a2b7c1d4e"  # secret in source → leaks via git history

def charge(user_id, amount):
    conn = payments.connect(API_KEY)
    # SQL built by string interpolation → injection
    row = conn.query("SELECT balance FROM acct WHERE id = " + user_id)
    # check-then-act race + no retry guard → double-charge
    if row["balance"] >= amount:
        conn.debit(user_id, amount)   # raises on timeout; nobody catches it
    logging.info("charged %s for %s card=%s", user_id, amount, row["card"])
    # ^ PII (card) in logs; no audit record; no authz check on user_id

What you'd approve

# RIGHT: secrets manager, handled errors, idempotent, authorized, no PII
def charge(caller, user_id, amount, idempotency_key):
    if not authz.can_debit(caller, user_id):        # per-object authz
        raise PermissionError("not authorized for account")

    api_key = secrets_manager.get("payments/api_key")  # least-privilege, rotatable
    try:
        with payments.connect(api_key) as conn:
            # parameterized query + atomic conditional debit → no race
            result = conn.debit_if_sufficient(
                account_id=user_id, amount=amount,
                idempotency_key=idempotency_key,      # retry-safe
            )
    except PaymentTimeout as err:
        audit.record("charge_timeout", user_id, amount)   # forensic trail
        raise ChargeFailed("payment provider timeout") from err  # handled + chained

    audit.record("charge_ok", user_id, amount)
    logging.info("charged account", extra={"user_id": user_id, "amount": amount})
    # ^ no card / PII in logs; structured context only
    return result

Common follow-ups

Where to get your data (Meta)

• Phabricator — the review thread where you blocked on a secret, an authz gap, or a non-idempotent charge, and the fix you proposed.
• SEV / postmortems — the incident where a missed check (double-charge, leaked secret, injection) validated a class you now always review for.
• Security review tooling — the static-analysis, secret-scanning, or dependency-scan finding that backed your comment.
• Launch / GK gating — the flag, canary, or gated rollout that let you ship the change reversibly.
• Scuba / Unidash — the metric (error rate, duplicate rate, auth-failure rate) that turned a security opinion into evidence.
• The internal wiki — the security review checklist or transaction-code standard you followed or set.
• GSD — the project or milestone the change belonged to, to show the stakes and the outcome.

---