Security code review interview questions

Questions about finding security problems in code and explaining a workable fix. Topics include authorization, unsafe input handling, secrets and the boundaries between services. Code snippets are included where the interview provides them.

This topic has 16 opening questions and 61 follow-ups in the interview. Choose this domain in setup to practise it by voice or text.

You are asked to security-review a pull request that adds a new HTTP endpoint. You have ten minutes. What are you actually looking at?
beginner3 follow-ups
A pull request wraps a permission check in a try/catch. On an exception it logs the error and lets the request continue. What do you say in the review?
beginner3 follow-ups

A pull request adds a forgot-password flow. Walk me through your review.

@app.post("/forgot")
def forgot(email):
    user = db.find_user(email)
    if user is None:
        return "No account with that email", 404
    code = str(random.randint(0, 999999)).zfill(6)
    db.set_reset_code(user.id, code)
    mail.send(email, f"Your reset code is {code}")
    return "Code sent", 200

@app.post("/reset")
def reset(email, code, new_password):
    user = db.find_user(email)
    if user and db.get_reset_code(user.id) == code:
        db.set_password(user.id, new_password)
        return "Password updated", 200
    return "Invalid code", 400

beginner5 follow-ups

The author says, "Switched to parameterized queries, so this is safe from injection." Filter values use bound parameters, but the raw sort parameter is concatenated into ORDER BY without validation or identifier escaping. What do you say?
beginner4 follow-ups
A pull request adds a shared application cache to an endpoint that returns the signed-in user’s private account details. The key is the request path and query string. A cache hit returns the stored response without rebuilding it for the caller. Review it.
beginner4 follow-ups

This came up in review on a reporting service. Walk me through what you would say on this pull request.

@app.route("/api/export/<report_id>")
def export_report(report_id):
    report = db.get_report(report_id)
    if report is None:
        abort(404)

    if not user_can_read(g.user, report):
        app.logger.warning(
            "denied export user=%s report=%s token=%s",
            g.user.id, report_id, g.user.session_token,
        )
        abort(403)

    # Re-read with the finance columns the exporter needs.
    full = db.get_report(report_id, include_financials=True)
    return send_file(render_pdf(full), download_name=f"{report_id}.pdf")

beginner4 follow-ups

The pull request title is "DRY profile update — loop over fields instead of repeating setattr". Review the change.

@@ users.py @@
 @app.patch("/api/users/me")
 def update_profile():
     data = request.get_json(force=True)
-    if "display_name" in data:
-        g.user.display_name = data["display_name"]
-    if "avatar_url" in data:
-        g.user.avatar_url = data["avatar_url"]
-    db.session.commit()
-    return jsonify(g.user.to_dict())
+    for key, value in data.items():
+        setattr(g.user, key, value)
+    db.session.commit()
+    return jsonify(g.user.to_dict())

@@ test_users.py @@
 def test_update_profile():
     resp = client.patch("/api/users/me", json={"display_name": "Ada"})
     assert resp.status_code == 200
     assert resp.json()["display_name"] == "Ada"

beginner4 follow-ups

The author says this closes the open redirect from the pentest report. Does it?

// Fix for PENTEST-31: open redirect on /login?next=
app.get("/login", (req, res) => {
  const next = req.query.next || "/";
  if (!next.startsWith("/") || next.startsWith("//")) {
    return res.redirect("/");
  }
  res.render("login", { next });
});

app.post("/login", (req, res) => {
  if (authenticate(req.body)) {
    return res.redirect(req.body.next);
  }
  res.render("login", { error: "Bad credentials", next: req.body.next });
});

beginner5 follow-ups

The app has an authorization middleware that all routes are supposed to use. This PR adds an internal endpoint under /api/internal/replay to restage events, "only called from our workers". The middleware is not on it. Review the change.
advanced3 follow-ups
To debug a customer issue, a pull request adds logging of full request and response bodies on one endpoint, behind a config flag that defaults to off. Review it.
advanced3 follow-ups

A colleague opens this with the description "fix flaky webhook test". What is your review?

@@ webhooks.py @@
 def verify_webhook(request):
-    signature = request.headers.get("X-Signature", "")
-    expected = hmac.new(SECRET, request.body, hashlib.sha256).hexdigest()
-    if not hmac.compare_digest(signature, expected):
-        raise Unauthorized()
-    return json.loads(request.body)
+    signature = request.headers.get("X-Signature", "")
+    body = request.body
+    try:
+        expected = hmac.new(SECRET, body, hashlib.sha256).hexdigest()
+        if not hmac.compare_digest(signature, expected):
+            raise Unauthorized()
+    except Exception as exc:
+        logger.info("signature check skipped: %s", exc)
+    return json.loads(body)

@@ test_webhooks.py @@
 def test_verify_webhook_bad_signature():
-    with pytest.raises(Unauthorized):
-        verify_webhook(make_request(sig="wrong"))
+    # Flaky in CI when SECRET is unset; assert it returns instead.
+    assert verify_webhook(make_request(sig="wrong")) is not None

advanced4 follow-ups

A pull request adds "import document from URL" for enterprise customers. The diff removes a localhost block "because customers host on private networks". Review it.

@@ integrations.py @@
 def import_from_url(user, url):
     parsed = urlparse(url)
     if parsed.scheme not in ("http", "https"):
         raise BadRequest("http or https only")
-    if parsed.hostname in ("localhost", "127.0.0.1"):
-        raise BadRequest("invalid host")
     resp = requests.get(url, timeout=5, allow_redirects=True)
     if resp.status_code != 200:
         raise BadRequest("fetch failed")
     return save_document(user, resp.content)

@@ integrations.py @@
 def save_document(user, content):
     doc = Document(owner_id=user.id, body=content[:500_000])
     db.session.add(doc)
     db.session.commit()
     return doc.id

advanced4 follow-ups

A pull request updates a GitHub Actions workflow, bumps three dependencies, and adds a preinstall script to a package the app already used. The feature diff is a small UI change. Where do you spend your time?
advanced3 follow-ups
You keep finding the same class of bug in reviews across different teams. Commenting on each pull request is clearly not working. What do you do?
advanced3 follow-ups

This is the rate limiter in front of every public endpoint on a service running about forty containers. Review it.

// Rate limiter used by every public API route.
const WINDOW_MS = 60_000;
const buckets = new Map(); // key -> { count, resetAt }

export function allow(req) {
  const key = req.headers["x-forwarded-for"] || req.ip;
  const now = Date.now();
  let b = buckets.get(key);

  if (!b || b.resetAt < now) {
    b = { count: 0, resetAt: now + WINDOW_MS };
    buckets.set(key, b);
  }

  b.count += 1;
  return b.count <= limitFor(req.path);
}

advanced4 follow-ups

A senior engineer opens a pull request introducing a new internal RPC framework that every service will adopt over the next year. You have two days to review it. What do you spend them on?
advanced5 follow-ups

Set up an interview

Each question is beginner or advanced. The tier describes the starting question; it is a practice label, not a certification. Questions by Pratik Amin.