Defense-in-depth security pair-review (security-agent)

Status: Adversarial review complete
Reviewer: security-agent
Date: 2026-07-11 UTC
Reviewed doc: docs/architecture/defense-in-depth-to-production.md (PR #4165)
Worktree: agent-a9d95934451344fa0
Epic: #94

Bottom line up front

The design is NOT launch-ready in its current form. Four items from the residual-risk register require re-classification, two new must-close cards are filed, and the "same transaction" fix for HC-ORD-2 does not close the double-fire path on the LCC two-leg roll. The non-negotiable must-close items before any customer-facing live traffic:

1. HC-AUTH-1 — login challenge consume has a TOCTOU gap independent of the dyno-scaling fix
2. HC-AUTH-3 — the shared admin token unlocks a full account-takeover chain; must NOT be accepted for v1
3. HC-ORD-2 — residual double-fire path exists even with the proposed fix (see §1)
4. HC-NEW-1 — LCC roll submit (POST /lcc/roll/submit) has NO idempotency protection at all

§1 HC-ORD-2 — Order double-fire race: VERDICT INSUFFICIENT

What the design proposes

"Wrap handler side-effects and the idempotency UPDATE in the same DB transaction."

Does it close the double-fire path?

For the simple order path (POST /api/trading/orders, POST /api/trading/orders/ic): Partially. The idempotency decorator's _UPDATE_COMPLETE_SQL runs in a separate get_engine().begin() block AFTER the handler returns. If the handler side effects and the UPDATE were moved into one conn.begin() block, an UPDATE failure would roll back the paper_orders INSERT simultaneously — eliminating the orphan-execution gap. This is achievable for simple orders and closes Scenario B (success + UPDATE failure).

However, it does NOT close Scenario A (crash between handler success and UPDATE). Gunicorn kills the worker process; the DB connection closes; any uncommitted transaction rolls back. If the idempotency UPDATE and the paper_orders INSERT are in the same transaction and the connection dies after the INSERT commits but before the UPDATE commits... that's impossible if they're in the same transaction — they'd both roll back. So for simple orders, the proposed fix does close Scenario A via the transaction boundary. Verdict for simple orders: FIX IS SUFFICIENT IF IMPLEMENTED CORRECTLY.

For the LCC two-leg roll (POST /lcc/roll/submit):

VERDICT: THE PROPOSED FIX DOES NOT APPLY. The route has a structural problem the design doc does not address.

Three concrete issues:

Issue 1: lcc_roll_submit.py has NO idempotency decorator at all.

The @idempotency_key decorator is only applied to trading.py lines 748 and 1110. The route POST /lcc/roll/submit (defined in lcc_roll_submit.py) has only @require_session. No @idempotency_key. The entire double-fire mitigation for the roll path is therefore unimplemented, regardless of the HC-ORD-2 fix.

Issue 2: idempotency_middleware flag defaults to OFF.

Even for the routes that DO carry the decorator (POST /api/trading/orders), the flag idempotency_middleware defaults to false in api/feature_flags.yaml. Until the flag is enabled in production config-vars, no idempotency protection exists on any order endpoint. HC-ORD-2 cannot be marked closed until this flag is active on prod.

Issue 3: MBT engine commits in its own independent transactions.

MbtFillEngine.submit_market_order() uses self._db.begin() — a separate engine connection. Each leg of the LCC roll commits its paper_orders INSERT immediately inside the engine. The outer handler connection that the idempotency decorator wraps cannot reach inside the engine's committed transactions.

Concrete double-fire path on the LCC roll even after the proposed HC-ORD-2 fix:

T1  Idempotency INSERT in_flight  ← committed (decorator, before handler)
T2  Close leg paper_orders INSERT ← committed (MBT engine, self._db.begin())
T3  Close audit write              ← committed (conn.commit() in _write_audit)
    --- CRASH / SIGKILL here ---
T4  Open leg never fires
T5  Idempotency UPDATE never fires → row stays in_flight
    After 30s: stale heal → status=failed → 409
    User retries with NEW idempotency key
T6  SECOND roll fires: close leg fires again on already-closed position

Required additional control: The POST /lcc/roll/submit route needs the @idempotency_key decorator applied (HC-NEW-1 below). Additionally, a pre-execution position-state check must be added: before submitting the close leg, query paper_orders for filled/accepted orders on the same symbol in the past 60 seconds for this user. If found, return 409 with "detail": "recent_order_for_symbol_exists". This gives defense-in-depth against key rotation.

The same check should exist in MbtFillEngine.submit_market_order() — a per-user, per-symbol dedup window of 30 seconds is a reasonable guard against double-fire that survives key rotation.

Residual double-fire path after full fix

Even with HC-ORD-2 and HC-NEW-1 fully implemented, a residual path remains:

• Stale detection marks the key failed and returns 409 with detail: "previous request timed out; key reset to failed".
• The 409 response does NOT currently include "check_order_status": true or any signal that the order may have partially executed.
• A client that receives this 409 and retries with a new key is the mechanism for the double-fire. The 409 body must be updated to include explicit guidance: "verify your current positions before retrying."

This is the signal gap from item 3 of the original HC-ORD-2 remediation. It must be implemented.

§2 HC-AUTH-1 — WebAuthn challenge store TOCTOU: VERDICT INSUFFICIENT

Split finding: registration vs. login

Registration path (in-memory/Redis store via _store_challenge / _pop_challenge): The design doc's description is accurate. Redis GETDEL is atomic. No TOCTOU on Redis. The in-process fallback breaks on multi-dyno because of gunicorn COW fork divergence — correctly identified. Filing issue #4158 to move to Postgres is the right fix.

Login path (login_challenges table): The existing Postgres-backed login challenge store has its OWN TOCTOU gap that the design doc does not mention.

Affected code in webauthn_service.py line 1178-1233:

# SELECT (check consumed_at IS None)
challenge_row = db_connection.execute(
    text("SELECT id, challenge_b64, expires_at, consumed_at FROM login_challenges ..."),
    {"ch": challenge_b64_norm},
).mappings().fetchone()

if challenge_row["consumed_at"] is not None:
    raise WebAuthnLoginError("Challenge already consumed")

# ... expiry check ...

# UPDATE — no WHERE consumed_at IS NULL guard
db_connection.execute(
    text("UPDATE login_challenges SET consumed_at = :ca WHERE id = :id"),
    {"ca": now_utc.isoformat(), "id": challenge_row["id"]},
)
db_connection.commit()

The UPDATE at line 1230 does NOT include AND consumed_at IS NULL. Two concurrent dynos both SELECT the same challenge, both see consumed_at=None, both pass the check, both UPDATE (the second overwrites the first consumed_at timestamp — no uniqueness violation), both proceed to verify_authentication_response() with the same challenge bytes. Both pass.

Exploit scenario: 1. Attacker intercepts or steals a login challenge (e.g., from a MITM or compromised browser) 2. Attacker submits two simultaneous assertion requests on separate dynos using the same challenge 3. Both dynos SELECT consumed_at=None, both pass 4. Both UPDATE (second silently overwrites first) 5. Both call py-webauthn with the same valid challenge bytes 6. Both get back a session token

This is a replay attack window that the "single-use challenge" invariant is supposed to prevent. The existing implementation does not enforce it atomically.

Fix (must close before launch): Change line 1230 to:

UPDATE login_challenges 
   SET consumed_at = :ca 
 WHERE id = :id AND consumed_at IS NULL

Then check rowcount: if 0, raise WebAuthnLoginError("Challenge already consumed — concurrent replay attempt"). Reject the request. This makes the consume atomic via the WHERE guard.

The same pattern should be enforced in the new webauthn_challenges table described in #4158. The table schema in issue #4158 should include the AND consumed_at IS NULL WHERE guard in its consume UPDATE, not just a consumed_at column.

§3 HC-EDGE-1 — CF source-IP fail-open on stale list: VERDICT INSUFFICIENT

Design's proposal

Fail open if the CF IP list refresh fails or is >24h stale.

Attack on fail-open

The fail-open posture means: whenever Cloudflare's IP list endpoint is unavailable (their CDN, not their WAF — different infrastructure), or our scheduled refresh job fails silently, the guard degrades to header-presence-only. This is the Phase 1 posture. Phase 2's entire purpose is to add source-IP validation.

An attacker probing the system can: 1. Determine when IP list staleness is most likely (right after deploy, right after network partition) 2. Set CF-Connecting-IP: 1.2.3.4 on a direct-to-origin request and reach the Heroku dyno, bypassing CF WAF, Bot Fight Mode, and all CF rate limits

If the IP list is stale and we fail open, the Phase 2 control collapses to Phase 1. An attacker who can read our refresh job logs (or probe from outside CF) knows when we're in degraded mode.

Fail-closed argument

Cloudflare's IP list (https://www.cloudflare.com/ips-v4 and -v6) is a stable, slowly-changing document. Historically updated once per several months. The staleness risk is: - Our refresh job fails (network partition, code bug) - CF changes their IPs without our job noticing

The DoS exposure from fail-closed: if the IP list is stale and Cloudflare adds a new ingress IP, requests from that new IP range get 403'd. This is a partial DoS affecting only traffic through the new CF IP. The correct response is an alert + manual unblock, not fail-open.

Recommended posture: 1. Ship a static fallback IP list with the application (a bundled copy of the CF IP ranges, updated at each release). This is the "last known good" baseline. 2. The refresh job updates a cached copy on top of the bundled list. 3. On refresh failure, the cached copy OR the bundled list is used (never fail open). 4. On refresh failure, alert immediately to ops@ — treat as HIGH finding per "nightly scan dark is HIGH" pattern. 5. Stale threshold: 1 hour (not 24h). CF IP changes are rare; a 1h cache is defensive. 6. FLAG_ENFORCE_CF_ORIGIN=0 remains the emergency bypass if a legitimate CF IP is blocked.

This eliminates the fail-open path while keeping the DoS exposure bounded (only new CF IP ranges during the stale window) and recoverable (flag toggle, alert response).

File HC-NEW-2 for this. The current 24h fail-open posture should not ship to production.

§4 HC-AUTH-3 — Shared X-Admin-Service-Token: VERDICT MUST RECLASSIFY

Design's assessment: Accept for v1

This is wrong. Here is the complete blast radius from the token:

Single ADMIN_SERVICE_TOKEN controls ALL of the following simultaneously:

admin_customers.py: - GET /api/internal/customers — full PII read on every user (email, sessions, registration state) - GET /api/internal/customers/<id> — full profile including session tokens - POST /api/internal/customers/<id>/sessions/<session_id>/revoke — terminate any user's active session (forced logout) - DELETE /api/internal/customers/<id>/credentials — delete ALL passkeys for any user - DELETE /api/internal/customers/<id> — GDPR hard delete, destroys ALL user data, irreversible - POST /api/internal/customers/bootstrap-token/register — issue invite tokens - POST /api/internal/customers/<id>/bootstrap-token/revoke - GET /api/internal/customers/<id>/audit — read full audit history for any user

billing_dsr.py: - Initiate, view, and execute DSR billing deletions

billing.py: - Read billing summaries

push_dispatch.py: - POST /api/internal/push/send — send arbitrary push notifications to any user (phishing vector)

internal_merges.py (with X-Merge-Permission-Granted also required — but Console sets this header, so if an attacker controls Console or the token, they control the header): - Account merges, cancellations, reversals

The account-takeover chain:

Step 1: DELETE /api/internal/customers/<victim_id>/credentials — wipes all passkeys for victim
Step 2: POST /api/internal/customers/bootstrap-token/register with victim's email — issues new invite token
Step 3: Complete WebAuthn registration with new passkey — full account takeover on victim

This chain requires ONE leaked secret (ADMIN_SERVICE_TOKEN) and gives the attacker full ownership of any Raxx account, including the ability to place orders under the victim's identity.

The "rotation on disclosure" mitigation assumes disclosure is detected. There is no per-action audit trail of ADMIN_SERVICE_TOKEN usage that surfaces credential-delete followed by bootstrap-token-register as a suspicious pattern. The current audit logs show WHO was acted upon, not attribution of the ADMIN_SERVICE_TOKEN caller vs. a Console operator.

Why "accept for v1" is wrong: - Pre-launch beta users are real accounts. Account takeover on a beta user = real harm. - The token controls GDPR hard deletes — one leaked token can wipe the entire user base. - The push send endpoint gives the attacker a phishing channel if they've already compromised one account. - v1 IS the launch. There is no "we'll fix it after launch" for a credential-delete + bootstrap-token chain.

Reclassification: HC-AUTH-3 → MUST CLOSE BEFORE LAUNCH.

Minimum required before launch (not a full per-route token solution — that is post-launch): 1. DELETE /customers/<id>/credentials must require a second independent authorization signal beyond ADMIN_SERVICE_TOKEN. This can be a time-limited per-operation HMAC computed from (ADMIN_SERVICE_TOKEN + user_id + operation + timestamp), requiring the caller to know both the token AND generate the HMAC correctly. This does not require per-route tokens. 2. DELETE /customers/<id> (hard delete) must require the same second-factor HMAC AND an explicit "confirm_hard_delete": true field in the body (defense against accidental trigger). 3. POST /customers/bootstrap-token/register must be rate-limited to 1 per email per 15 minutes. This limits the bootstrap-token chain's speed even if the token is leaked. 4. Audit log: ADMIN_SERVICE_TOKEN usage must write an audit row with actor_type="admin_service_token", the route called, and the target user_id. This gives the detection signal that "rotation on disclosure" depends on.

File HC-NEW-3 for this.

§5 Residual-risk register re-classifications

New items:

§6 Additional gap: idempotency flag gate in production

This is a pre-launch configuration gap not in the residual-risk register.

api/feature_flags.yaml line 9136: idempotency_middleware: default: false.

The @idempotency_key decorator on POST /api/trading/orders is a transparent passthrough when this flag is off. HC-ORD-2 cannot be considered closed until: - FLAG_IDEMPOTENCY_MIDDLEWARE=1 is set on prod Heroku config-vars - Verified via heroku config:get FLAG_IDEMPOTENCY_MIDDLEWARE --app raxx-api-prod

This must be a pre-launch checklist item tied to HC-ORD-2. Route it to sre-agent.

§7 HC-AUTH-2 — SameSite=Lax: Accept (upheld)

The design's "origin check on state-mutating routes is already present" claim is sufficient for v1. SameSite=Strict would break the discoverable passkey UX (cross-site navigation to raxx.app from the CF Access login page carries the cookie under Lax but not Strict). The origin check is the correct compensating control. Accept upheld.

§8 HC-ORD-4 — FLAG_LIVE_MODE_RBAC_GATE: Accept (upheld, with caveat)

For paper-only v1, the paper-first graduation gate (paper_profitable_gate_met) is the active control. The flag being OFF is only dangerous when live broker credentials are activated. The must-close pre-condition (no live cred activation before flag=1) is correctly identified. The HC-ORD-4 remediation ("set flag=1 before any live broker cred is configured") is accurate and sufficient. Accept for v1 is correct given this pre-condition.

Caveat: this must be in the pre-launch deploy checklist as a blocking item, not just noted in the design doc. Route to sre-agent to add to the launch runbook.

§9 Cards filed this run

New issues filed against raxx-app/TradeMasterAPI by raxx-ops-bot:

• HC-NEW-1: [security] CRITICAL: POST /lcc/roll/submit has no idempotency_key decorator — double-fire unmitigated → feature-developer
• HC-NEW-2: [security] HIGH: CF source-IP guard must fail-closed with bundled IP fallback, not fail-open on stale list → feature-developer + sre-agent
• HC-NEW-3: [security] CRITICAL: ADMIN_SERVICE_TOKEN account-takeover chain via credentials-delete + bootstrap-token — must close before launch → feature-developer
• HC-NEW-4: [security] CRITICAL: login_challenges consume UPDATE missing AND consumed_at IS NULL — TOCTOU on challenge replay → feature-developer

Updates filed on existing cards: - #4158 (HC-AUTH-1): comment noting the login TOCTOU is a separate gap from the dyno-scaling issue; both must be in scope - #4161 (HC-ORD-2): comment noting the MBT engine commits in self._db.begin() — the "same transaction" fix requires the engine to accept an external connection, and the fix cannot close the LCC roll path without HC-NEW-1