fix: Phase 1 — P1 security & execution (review remediation)#38
Open
yhaspel wants to merge 11 commits into
Open
fix: Phase 1 — P1 security & execution (review remediation)#38yhaspel wants to merge 11 commits into
yhaspel wants to merge 11 commits into
Conversation
The login second factor was rate-limited per IP only. An attacker with the password could mint fresh mfa_tokens at 5/min and brute-force TOTP from a distributed IP pool, and a failed mfa_token stayed reusable for its full TTL. - Per-user failure counter (survives token re-minting) locks the challenge after MFA_LOGIN_MAX_FAILURES within the window (MFA_LOCKED, 429). - Per-token (jti) counter burns the specific mfa_token after the cap, so it cannot be reused — forces a fresh password login. - Correct code before the cap resets both counters. Per-IP limiter retained. - decode_mfa_token_claims exposes the jti; decode_mfa_token delegates to it. Tests: locks after N per-user (even with fresh tokens each try); burns the token after N on it; valid code within the limit still succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
compute_size ran with multiplier=1 and a per-share premium, so raw_qty, max_qty_by_pos, and the leverage-cap clamp were all ~100x too loose for equity options (and per-root wrong for futures) — the ceilings never bound and the alert qty passed through effectively unclamped. - integration.apply_sizing now sets SizingInputs.contract_multiplier from the order's trusted asset class (100 for options; per-root FUTURE_MULTIPLIERS map with a conservative default + TODO for futures). - max_qty_by_pos now divides by price*multiplier (was price only); raw_qty and the leverage clamp already used the multiplier once it's populated. Multiplier recorded in the SizingDecision meta for auditability. - webhooks.tasks refreshes the order after persisting resolved intent so apply_sizing sees the real asset_class/future_root (the in-memory order held the creation-time default), which also fixes permitted-asset-class enforcement for real option/future orders. Tests: option notional == qty x price x 100; leverage cap binds 100x tighter for options; equity sizing unchanged (mult=1); option order sized end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1-3) The completed export emailed/returned a /media/exports/... URL that 404s — MEDIA_URL/ROOT are defined nowhere and nothing serves /media/. Where FileSystemStorage is used the "24h signed URL" was a silent no-op (unsigned, permanent). The GDPR export feature was non-functional in the default deploy. - New DataExportDownloadView streams the ZIP via FileResponse only to the owner, only while READY and unexpired (404 non-owner / not-ready, 410 expired), Content-Disposition: attachment. - signed_export_url(job) now returns that view's URL for the filesystem backend (absolutized via FRONTEND_BASE_URL) and keeps the real presigned URL for S3/R2. Tests: owner 200 streams bytes + attachment header, non-owner 404, expired 410, and the URL points at the served route (not /media/) for the filesystem backend. Prod-import smoke passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (P1-4) The SPA persisted the 30-day refresh token in localStorage, which any XSS can read — a single injection yields indefinite access-token minting (persistent takeover of an account that places live trades). Backend: - New cookies helper sets the refresh as HttpOnly; Secure (prod); SameSite=Strict cookie scoped to /api/v1/auth/, and strips it from every token-pair response body (login, MFA verify, refresh, OAuth exchange, verify-email, reset-confirm). - /auth/refresh/ and /auth/logout/ read the token from the cookie; body accepted as a fallback for non-browser clients (browser never sends one). Invalid refresh clears the cookie. SameSite=Strict is the CSRF control (ADR-107). Frontend: - Store no longer holds/persists a refresh token and purges the legacy localStorage key. Facade attempts refresh unconditionally (cookie is opaque to JS); bootstrap + 401 interceptor rely on the server's verdict. Auth API calls that set/send the cookie use withCredentials. Guards judge auth by in-memory state only. Tests: login/mfa/oauth/verify-email set the cookie (not body); refresh via cookie rotates; missing token 401s; logout clears the cookie; store purges legacy key; interceptor attempts refresh on 401. ADR-107 records the decision. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (P1-5) If place_order raised after a submit timeout and the duplicate probe also failed, the order was set REJECTED locally while possibly live/resting at the broker. The reconnect sweep never re-examines a REJECTED order, so a working order became invisible to the platform and to daily-loss accounting and could fill later untracked. - New non-terminal Order.Status.NEEDS_RECONCILE (migration 0005). - process_alert: a retryable BrokerError (timeout/5xx/rate-limited) or an unexpected exception -> NEEDS_RECONCILE; a definitive broker rejection (retryable=False) still -> REJECTED. - resolve_needs_reconcile() probes the broker by client_order_id: found -> take the broker's status + broker_order_id; never seen -> REJECTED (RECONCILE_NOT_FOUND). Wired into catch_up_account (reconnect) and reconcile_account (periodic). AlpacaAdapter.resolve_by_client_id added. - Frontend: NEEDS_RECONCILE added to the order-status filter + i18n label. Tests: ambiguous/unexpected submit -> NEEDS_RECONCILE + alert accepted; definitive rejection still rejects; reconcile resolves found orders and rejects never-seen ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1-6) Terminal order state was computed from a local running sum of applied fills. If any fill was dropped (see P0-2) the sum stayed short and the order never reached FILLED — reconcile heals position qty but not order state. The event already carries the broker's authoritative cumulative filled_qty, which was discarded. ingest_fill_event now sets order.filled_qty = max(local, event.filled_qty) when the event carries a cumulative, and resolves FILLED vs PARTIAL against order.qty using it; the local sum remains the fallback when no cumulative is present, and max() prevents a late/duplicate event from reducing the count. Tests: filled_qty uses the broker cumulative; a dropped intermediate fill still reaches FILLED on the final event; partial-then-full sequence resolves correctly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When KILL_SWITCHES_ENABLED was false, is_blocked honored only an L0 strategy toggle and returned None for L1 (user-global), L2 (auto daily-loss) and L3 (platform) halts. A single ops flag meant to disable the engine also silently voided the platform-wide emergency stop and the daily-loss breaker. is_blocked now always enforces active TradingHalt rows (platform → user → strategy). The flag gates only the automatic tripping of new L2 halts in daily_loss_watcher (already guarded there), never the reading/enforcement of existing halts. Removed the now-dead _enabled() helper + unused settings import. Tests: platform/user/existing-L2 halts enforced with the engine disabled; the flag still skips the watcher's auto-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hard_stop_pct was collected and validated (soft < hard) but never read on the execution path — only soft_stop_pct was consumed (halves size). Users configured a hard stop expecting trading to STOP at that drawdown; it was a silent no-op. - compute_size: when intraday_dd_pct >= hard_stop_pct, reject the order (HARD_STOP), checked before the soft stop. - apply_sizing: a HARD_STOP reject also trips the daily L2 halt (auto, flatten), mirroring the daily-loss trip, so trading actually stops for the day rather than re-attempting into the same drawdown on the next alert. - Surface HARD_STOP in the frontend reason allowlist. Tests: hard stop rejects at/above threshold (inclusive boundary); soft stop still only halves below hard; end-to-end reject + L2 halt created. Existing soft-stop test's hard_stop raised to 20% so its 10% DD stays soft. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The audit scrubber used exact key matching, so substring variants (api_secret,
current_password, secret_encrypted, *_enc, refresh_token) passed through into
the append-only, immutable log — combined with StrategyDetailView.patch copying
every client body key into the diff, a body like {"current_password":"…"} could
persist unredacted forever.
- scrub() now redacts a key if it exactly matches the audit key set OR contains
a sensitive substring. Shared SENSITIVE_FIELD_PARTS is the single source of
truth, reused by the GDPR export redactor. Short/broad exact keys (sig, key,
code) stay exact-only so "design"/"monkey"/"zip_code" aren't over-redacted.
- StrategyDetailView.patch audits only the validated (allowlisted) fields
(name, is_enabled), never arbitrary request keys.
Tests: substring redaction (+ no over-match), nested redaction, shared denylist
across scrub + gdpr, PATCH audit limited to allowlisted fields.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Flipping the toggle on armed automated live order execution immediately — a
user-uploaded strategy shows an "untested" banner yet went live on one tap with
no confirmation, while the far-less-dangerous delete DID confirm (inverted risk).
- Enabling now opens the shared focus-trapping modal with consequence copy
("Enabling this strategy will execute live orders from its webhook alerts");
toggleEnabled fires only on confirm. Disabling stays one-click.
- Toggle exposes role="switch" + aria-checked (a11y).
- Delete confirm copy rewritten for end users (no is_enabled/admin jargon) and
moved to an i18n key (no hardcoded English).
Karma: enable opens the modal without toggling; confirm toggles once; cancel
does nothing; disable is immediate; switch exposes aria-checked.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
yhaspel
force-pushed
the
fix/review-p1-security-and-execution
branch
from
July 19, 2026 06:07
2fb38c9 to
37a9438
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Phase 1 (P1 High) of
development-plans/2026-07-17-review-remediation-plan.md. Stacked on the P0 PR (#37) — base will retarget tomainwhen P0 merges.Items
MFA_LOCKED) + per-jti token burn.NEEDS_RECONCILE(migration 0005), resolved by client_order_id.filled_qty/status trust the broker cumulative (dropped fill still reaches FILLED).KILL_SWITCHES_ENABLEDgates only auto-tripping.hard_stop_pctenforced: rejectHARD_STOP+ trip L2 halt.role="switch"/aria-checked.Verification (local gauntlet)
ruff✓,bandit(medium) ✓,pytest690 passed / 8 skipped,pytest -m pg8 passed,makemigrations --checkclean, prod-import smoke ✓.ngc --noEmit✓, karma 131 passed,pnpm build✓.Order.Status.NEEDS_RECONCILE) is reversible.Note: OpenAPI
TokenPairDatastill documentsrefresh(the generatedschema.tsdoesn't regenerate cleanly in this env); the field is retained in the frontend type for contract equivalence but never read — see ADR-107.🤖 Generated with Claude Code