Wedge Trade AI
All documentation

Security Review

A focused pass over every security-critical primitive in this codebase, written before public launch: what's correct, what's a real (documented, not silently accepted) gap, and what's out of scope. This complements — not replaces — docs/RBAC.md (roles/permissions), docs/ORGANIZATIONS.md (RLS scoping), and the doc comments on each file reviewed below, which already explain *why* each design choice was made. This document's job is to say plainly what was actually checked and what wasn't.

1. Broker credential encryption — src/lib/security/credential-crypto.ts

Algorithm: AES-256-GCM via Node's built-in crypto module. Correct choice — authenticated encryption, no external dependency, no reliance on a database-side extension (Supabase Vault isn't available on this project).

Verified correct:

  • IV is 96 bits (12 bytes), generated fresh per call via randomBytes()

(cryptographically secure) — the GCM-recommended IV size, never reused, never derived from anything predictable.

  • Auth tag (16 bytes) is captured and verified on decrypt via

setAuthTag() — tampering with the ciphertext or tag causes decipher.final() to throw, not silently return garbage.

  • Key length is validated (must be a 32-byte key, hex-encoded) before

every operation — a misconfigured key fails loudly at first use, not silently with a weaker effective key size.

  • The key lives only in CREDENTIAL_ENCRYPTION_KEY, a server-side env

var, never in the database — a database-only compromise (e.g. a leaked service_role key or SQL injection) does not expose plaintext broker credentials.

  • Blob format (IV || authTag || ciphertext, base64) is self-contained —

no separate metadata table to keep in sync, nothing that can drift out of step with what's actually stored.

  • Every call site that persists broker credentials

(src/app/dashboard/accounts/add-broker-actions.ts) and every call site that reads them back for live connector use (src/lib/broker-connections/live.ts, src/lib/live-copy/account-connector-loader.ts) goes through encryptCredentials()/decryptCredentials() — grepped the whole src/ tree for every caller, confirmed there is no path that writes broker credentials to the database in plaintext. The same functions are also reused for AI conversation content (src/lib/ai/services/supabase-conversation-persistence.ts), which is a legitimate reuse of the same primitive for a second at-rest-encrypted field, not a scope leak.

Real gaps, not fixed this milestone (documented, not silent):

  • No key rotation / versioning. The blob carries no key-version tag.

Rotating CREDENTIAL_ENCRYPTION_KEY today would make every existing encrypted row permanently undecryptable unless every row is read-with-old-key/re-encrypted-with-new-key in one migration *before* the env var changes. There is no tooling for that migration today. For a pre-launch codebase with no production credentials encrypted yet, this is an acceptable gap to launch with — but it should be built before the key is ever rotated in production, not after.

  • No AAD (additional authenticated data) binding. GCM supports

binding ciphertext to a context (e.g. the owning trading_account_id) via setAAD()/associated data, which this implementation doesn't use. Without it, GCM still guarantees the ciphertext itself wasn't tampered with, but it does not cryptographically guarantee that a given encrypted blob is being decrypted in the row it was originally written to — an attacker with direct database write access (already a severe compromise, requiring bypassing RLS/service-role scoping) could in principle copy one user's encrypted blob into another user's row and have it decrypt successfully. This is a real, if narrow, hardening opportunity for a future milestone, not a currently-exploitable gap from the application layer (every read path scopes by the owning user/RLS first).

2. API key hashing — src/lib/security/api-keys.ts

SHA-256 over a 256-bit random token, not a slow password hash. This is correct, not a shortcut — the file's own doc comment gets the reasoning right: the input is already maximum-entropy random data, not a low-entropy human-chosen password, so there's no brute-forceable search space for a slow hash (bcrypt/argon2) to defend against, and a fast hash keeps every Bearer-authenticated request cheap. Verified generateApiKey() never returns or logs the plaintext anywhere except the one-time creation response, and hashApiKey() is what's actually persisted and matched on auth.

3. Two-factor authentication — src/components/settings/mfa-panel.tsx

Delegates entirely to Supabase Auth's own built-in TOTP MFA (supabase.auth.mfa.enroll/challenge/verify) rather than a custom TOTP implementation — verified in the panel's own doc comment and confirmed against the installed @supabase/auth-js source. This is the correct choice: TOTP secret generation, QR rendering, and challenge/verify are a solved, security-critical problem that should not be reimplemented per project. The enrollment QR code is Supabase's own pre-built data:image/svg+xml string rendered as a plain <img src>, never injected as HTML — confirmed not to be an XSS vector.

4. Session management

RevokeSessionButton/revokeUserSession() (admin-side) and the user-facing sessions panel both operate through SECURITY DEFINER Postgres RPCs (admin_revoke_session, admin_list_user_sessions) that independently re-check is_platform_admin()/is_platform_operator() server-side — not just at the calling TypeScript layer. This means even a bug in the TypeScript authorization check (requirePlatformCapability) would not, by itself, let an unauthorized caller revoke another user's session, because the database function re-derives the caller's admin status from their own JWT (auth.uid()), not from anything the client supplies.

5. Rate limiting — src/lib/security/rate-limit.ts

A Postgres-backed fixed-window counter (check_rate_limit() RPC), deliberately chosen over an in-memory limiter because this app has no persistent process — every request is a fresh serverless invocation, so an in-memory counter would reset every cold start and never be consistent across concurrent instances. Confirmed it fails open (allowed: true) if the RPC itself errors — a documented, deliberate availability-over-strictness trade-off specifically for auth flows (a rate limiter outage should never become an accidental full login outage). This is the right default for this specific set of call sites (login, password reset, MFA challenge) but would be the *wrong* default for a rate limiter protecting something where over-permitting on failure is itself the bigger risk (e.g. a paid-API-credit-consuming endpoint) — worth keeping in mind if this same helper is reused for a different call site in the future.

6. Audit logging — src/lib/security/audit-log.ts

Every admin mutating action reviewed in this pass (revokeUserSession, the new setSubscriptionCancellation, and 16+ other call sites found via grep) writes to audit_logs via the service-role client, because that table's RLS deliberately has no authenticated-role insert policy — the only way a row lands there is through backend code that already went through an admin capability check. logAuditEvent() itself never throws back to the caller on a write failure (logs and swallows) — the right choice, since an audit-log outage should never block the underlying admin action it's trying to record, but it does mean an audit trail gap is possible during a audit_logs-table outage; nothing currently alerts on that specific failure mode.

7. Authentication & JWT handling

src/lib/supabase/middleware.ts (called from src/proxy.ts — Next.js 16 renamed the middleware.ts file convention to proxy.ts; confirmed against node_modules/next/dist/docs/.../proxy.md, not guessed) runs on every request except static assets. Verified it calls supabase.auth.getUser(), not getSession(), to determine the current user — the security-relevant distinction: getUser() revalidates the JWT against the Supabase Auth server on every call, while getSession() only decodes the cookie locally without confirming the token hasn't been revoked. Using the weaker getSession() for authorization decisions is a well-known Supabase footgun; this codebase uses the correct one. The same file also enforces MFA step-up (aal1→aal2 redirect to /mfa-challenge) for any enrolled user before they reach /dashboard/*, using Supabase's own getAuthenticatorAssuranceLevel() rather than a custom TOTP check.

8. Authorization (Row-Level Security)

Every table this app creates gets RLS — verified exhaustively, not sampled: cross-referenced every create table public.* against every alter table public.* enable row level security across all migration files. 47 tables created, 47 with RLS enabled — zero gaps. Spot- checked the highest-sensitivity tables individually (trading_accounts, broker_connections, copy_relationships, subscriptions, api_keys, audit_logs) to confirm the automated cross-reference wasn't missing something a regex could misparse.

9. SQL injection

No raw SQL string construction found anywhere in src/ — grepped for template-literal SELECT/INSERT/UPDATE/DELETE construction and .raw()-style escape hatches. Every database access goes through Supabase's parameterized query builder (.eq(), .select(), RPC calls with typed params) or a SECURITY DEFINER Postgres function taking typed arguments — not string concatenation. The SELECT-suffixed constants seen in several queries.ts files (e.g. COPY_RELATIONSHIP_SELECT) are PostgREST column-selection strings (id, name, related(...)), not SQL, and carry no user input.

10. XSS

Zero uses of dangerouslySetInnerHTML anywhere in src/ — grepped the whole tree. Every place this codebase renders content that originates outside its own source (AI-generated text, user-entered names/comments, Markdown docs) goes through React's default escaping or a dedicated sanitizer (sanitizeUserInput, used throughout the Server Actions already reviewed in earlier passes this session) rather than raw HTML injection.

11. CSRF

Confirmed against the installed Next.js version's own docs (node_modules/next/dist/docs/01-app/02-guides/{data-security, server-actions}.md): Server Actions get a built-in CSRF check — the request's Origin header is compared to Host/X-Forwarded-Host, and mismatches are rejected — with no serverActions.allowedOrigins override configured in next.config.ts, meaning this app relies on that default (strict same-host) behavior rather than a broadened allow-list. Verified the production reverse-proxy config (deploy/nginx.conf) sets proxy_set_header Host $host;, which forwards the real public Host header through unmodified — the default CSRF check works correctly through this proxy setup, not just in local dev. The separate API-key Bearer-token auth path (src/lib/security/api-auth.ts) is inherently immune to CSRF by construction (browsers never attach an Authorization header automatically the way they do cookies).

12. Rate limiting & API abuse

Extends section 5 above: confirmed every auth-adjacent Server Action that could be abused for enumeration/spam has its own rate limit — login (10/15min by email-or-IP), signup (10/hour by IP — initially missed during this pass because it's exported from login/actions.ts rather than a signup/actions.ts file that doesn't exist; re-checked directly against the source before concluding this, not assumed), forgot-password, and mfa-challenge all call checkRateLimit(). The AI chat route and billing usage-limits are separately rate- limited/quota-enforced for cost-abuse reasons (already covered in earlier milestones). GET /api/health* accepts unauthenticated requests by design (the standard shape for an uptime-monitor target) and isn't additionally throttled — reviewed deliberately, not overlooked: each check is a handful of lightweight read queries, the same "reasonable to leave a health check cheap and public" trade-off nearly every production app makes, and adding throttling here risks breaking legitimate uptime monitoring for negligible benefit.

13. Secrets & environment variables

.gitignore excludes .env* except .env*.example — confirmed via git log --all --full-history -- .env.local that no real .env.local has ever been committed, only the tracked .env.local.example template. Every NEXT_PUBLIC_* variable (client-bundle-exposed, confirmed by listing all four: NEXT_PUBLIC_SUPABASE_URL, NEXT_PUBLIC_SUPABASE_ANON_KEY, NEXT_PUBLIC_SITE_URL, NEXT_PUBLIC_SALES_EMAIL) is safe to expose by design — Supabase's anon key is meant to be public (RLS is the real protection boundary, not secrecy of that key), and the other three are non-sensitive by nature. No server-only secret (SUPABASE_SERVICE_ROLE_KEY, STRIPE_SECRET_KEY, CREDENTIAL_ENCRYPTION_KEY, TWILIO_AUTH_TOKEN, etc.) carries the NEXT_PUBLIC_ prefix.

14. Dependency / supply-chain audit — the one real, unresolved finding

npm audit --production (run fresh for this pass — previously "not run" in this document): 13 vulnerabilities (2 critical, 4 high, 3 moderate, 4 low), every one of them a transitive dependency of metaapi.cloud-sdk (MT5 connectivity) — axios (high, a long list of CVEs including SSRF and prototype pollution), crypto-js (critical — PBKDF2 implemented ~1.3 million times weaker than the current standard), lodash (high — code injection via _.template, prototype pollution), plus elliptic/socket.io-client/engine.io-client/ parseuri/browserify-sign/create-ecdh/crypto-browserify (low-to- moderate).

Not fixed automatically, and here's exactly why: npm audit fix (no --force) made zero changes — nothing in this tree is fixable without touching the parent. npm audit fix --force's own suggested fix is to install metaapi.cloud-sdk@23.1.1 — which is not an upgrade, it's a downgrade from the currently-pinned and installed 29.3.3 (the latest published version) by six major versions. Blindly running that would trade a dependency-vulnerability problem for a far bigger, unknown-behavior problem in the one connector this codebase has the least ability to verify — there is no live MT5 account in this environment to confirm a six-major-version SDK downgrade doesn't break authentication, account discovery, or order placement. This is not something to fix silently as part of an automated audit pass.

None of these vulnerable packages are used directly by this codebase's own code — credential-crypto.ts uses Node's built-in crypto module, not crypto-js; nothing in src/ imports axios/lodash/elliptic directly (confirmed: they're metaapi.cloud-sdk's own internal dependencies, invisible to this codebase's own import graph). The exposure is real but bounded to whatever metaapi.cloud-sdk itself does with them internally (HTTP calls to MetaApi's cloud service, its own internal encryption/signing) — not a direct hole in this app's own logic.

Recommended remediation path (deliberately left as a decision for whoever owns this in production, not executed here): 1. Check for a newer metaapi.cloud-sdk release that has updated its own dependencies — the cleanest fix, whenever it ships. 2. If urgency requires acting sooner, evaluate package.json overrides for just the worst offenders (crypto-js, axios, lodash) pinned to patched versions without changing metaapi.cloud-sdk itself — lower blast radius than a full SDK version change, but still requires a full tsc/lint/test/build pass AND a live MT5 demo-account retest (docs/BROKER_INTEGRATION_REPORT.md's Validation checklist) before trusting it, since an override changes what code actually runs inside the SDK's own internals. 3. Track this as a named item on the Beta Launch checklist, not a silent gap — see docs/BETA_LAUNCH_CHECKLIST.md.

What this review did NOT cover

  • Penetration testing — this is a code-level review of the primitives

themselves (algorithm choice, key handling, auth-check placement), not a live attempt to break the deployed app.

  • Infrastructure-level security (Supabase project settings, network

policies, secret storage in the hosting provider) — outside this codebase's scope to review from source alone.

  • A live MT5 SDK dependency override/downgrade retest — see section

14 above; deliberately not performed in this environment.