Fix/issues 245 246 247 248

contracts/THREAT_MODEL.md

@@ -52,7 +52,17 @@ This document outlines the threat modeling analysis for AirFlex Soroban smart co
 
 ---
 
-### 6. Critical Contract Bug or Settlement Deadlock (Trapped Funds Escape Hatch)
+---
+
+### 7. Secret Key Transmitted in Trade Request Body (Resolved — Issue #245)
+* **Threat Vector**: The original `POST /api/v1/trades/:id/buy` endpoint accepted a `buyerSecretKey` field in the JSON body and forwarded it to `depositToEscrow`. The key appeared in access logs, was visible to any process with HTTP-body access, and gave the server full, unaudited control over the buyer's funds.
+* **Impact**: Critical — Full compromise of every buyer's Stellar wallet from a single log-file leak. Any platform operator with log access could drain all buyer accounts.
+* **Mitigation (implemented)**:
+  * `buyerSecretKey` is removed from the request schema entirely. The Zod schema (`buyTradeSchema`) now accepts only `{ signedXdr: string }` — the server never receives secret key material in a trade request.
+  * Transaction signing is moved client-side. The `/buy/prepare` endpoint returns an unsigned XDR envelope; the browser signs it in memory (using the short-lived key from `/wallet/unlock`); only the signed envelope is submitted.
+  * The server calls `submitSignedTransaction({ signedXdr, expectedSourceAccount })`, which verifies the envelope's source account before submission — it cannot be replayed for a different buyer.
+  * See [docs/security.md](../../docs/security.md#stellar-key-handling--client-side-signing-model) for the full model description.
+
 * **Threat Vector**: A zero-day defect or irrecoverable logic deadlock permanently breaks settlement workflows (`release_payment`, `cancel_and_refund`, `resolve_dispute`), locking protocol assets permanently inside contract balances with no standard path for user withdrawal.
 * **Impact**: Critical — Permanent loss of trapped protocol liquidity and user deposits.
 * **Mitigation**:

docs/security.md

@@ -48,9 +48,50 @@ acknowledgement within 48 hours, please follow up at security@airflex.io.
 
 ---
 
-## Scope
+## Stellar Key Handling & Client-Side Signing Model
+
+### How transaction signing works
+
+AirFlex uses a **custodial wallet model** during its current phase: when a user
+completes their first OTP verification, the server generates a Stellar keypair,
+encrypts the secret key with `ENCRYPTION_KEY` (AES-256-GCM), and stores only
+the ciphertext in `wallets.stellar_secret_key`.
+
+When a buyer wants to lock a trade, signing happens **client-side**:
+
+1. `POST /api/v1/trades/:id/buy/prepare` — the server builds the unsigned XDR
+   envelope for the `deposit_to_escrow` Soroban invocation and returns it to
+   the browser. No secret material leaves the server.
+2. `POST /api/v1/wallet/unlock` — the server decrypts the caller's own secret
+   key and returns it **only to the key's owner** over the authenticated
+   session. The response carries `Cache-Control: no-store`. Rate-limited to 10
+   requests per 60 seconds per user.
+3. The browser signs the XDR envelope in memory using the decrypted key and
+   passes only the **signed XDR** to `POST /api/v1/trades/:id/buy`.
+4. The server submits the signed envelope to Horizon and records the resulting
+   transaction hash. The raw secret key is never present in any trade request
+   body, access log line, or database column.
+
+### What the server never does
+
+- **The server never accepts a `buyerSecretKey` field** in any request body.
+  The `buyTradeSchema` (Zod) accepts only `{ signedXdr: string }` — any
+  payload containing a secret key key field is rejected at schema validation.
+- The server never logs decrypted key material. The `unlock` endpoint logs only
+  the `user_id`, never the key value.
+- The decrypted key is never written to any database column, cache, or response
+  body other than the direct `/wallet/unlock` response.
+
+### Roadmap
+
+The `/wallet/unlock` endpoint is a **transitional mechanism** on the path to
+non-custodial wallets. Once users can import or generate their own keypair in a
+browser extension or hardware wallet, the endpoint will be removed and the
+server will never possess the secret key at all.
+
+---
+
 
-### In Scope
 
 | Target | Examples |
 |--------|---------|

server/src/__tests__/validateEnv.test.ts

@@ -0,0 +1,70 @@
+/**
+ * validateEnv.test.ts
+ *
+ * Verifies that assertEnvValid() throws (and would cause a process.exit(1) in
+ * production) when JWT_SECRET is absent or too short (issue #247).
+ */
+
+import { validateEnv, assertEnvValid } from "../config/validateEnv";
+
+/** Minimal valid env that satisfies every required variable */
+const VALID_ENV: Record<string, string> = {
+  JWT_SECRET: "a".repeat(64), // 64 chars — well above the 32-char minimum
+  DATABASE_URL: "postgresql://test:test@localhost/test",
+  ENCRYPTION_KEY: "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f",
+  STELLAR_SERVER_SECRET: "SAIXYZSSAQUEJO3Z3LUQ4PM3VVDK3DIO55V76ORFO6HH2VULC43AFHZX",
+  PLATFORM_TREASURY_USER_ID: "00000000-0000-0000-0000-000000000001",
+  PAYSTACK_SECRET_KEY: "sk_test_xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
+  TERMII_API_KEY: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
+};
+
+describe("validateEnv — JWT_SECRET (issue #247)", () => {
+  it("passes with a sufficiently long JWT_SECRET", () => {
+    const result = validateEnv(VALID_ENV);
+    expect(result.valid).toBe(true);
+    expect(result.errors).toHaveLength(0);
+  });
+
+  it("fails when JWT_SECRET is absent", () => {
+    const env = { ...VALID_ENV };
+    delete env["JWT_SECRET"];
+
+    const result = validateEnv(env);
+    expect(result.valid).toBe(false);
+    expect(result.missingRequired).toContain("JWT_SECRET");
+  });
+
+  it("fails when JWT_SECRET is shorter than 32 characters", () => {
+    const result = validateEnv({ ...VALID_ENV, JWT_SECRET: "tooshort" });
+    expect(result.valid).toBe(false);
+    const hasLengthError = result.errors.some((e) =>
+      e.includes("JWT_SECRET is too short")
+    );
+    expect(hasLengthError).toBe(true);
+  });
+
+  it("passes when JWT_SECRET is exactly 32 characters", () => {
+    const result = validateEnv({ ...VALID_ENV, JWT_SECRET: "a".repeat(32) });
+    expect(result.valid).toBe(true);
+  });
+
+  it("assertEnvValid throws when JWT_SECRET is absent", () => {
+    const env = { ...VALID_ENV };
+    delete env["JWT_SECRET"];
+    expect(() => assertEnvValid(env)).toThrow(/JWT_SECRET/);
+  });
+
+  it("assertEnvValid throws when JWT_SECRET is too short", () => {
+    expect(() =>
+      assertEnvValid({ ...VALID_ENV, JWT_SECRET: "weak" })
+    ).toThrow(/JWT_SECRET is too short/);
+  });
+});
+
+describe("validateEnv — ENCRYPTION_KEY format", () => {
+  it("fails when ENCRYPTION_KEY is not 64-char hex", () => {
+    const result = validateEnv({ ...VALID_ENV, ENCRYPTION_KEY: "not-hex" });
+    expect(result.valid).toBe(false);
+    expect(result.errors.some((e) => e.includes("ENCRYPTION_KEY"))).toBe(true);
+  });
+});

server/src/config/validateEnv.ts

@@ -7,6 +7,12 @@
  */
 import logger from "../utils/logger";
 
+// JWT_SECRET must be at least 32 characters so it provides ≥256-bit effective
+// entropy when used as an HMAC-SHA256 secret.  A shorter secret allows
+// offline brute-forcing of tokens — this is an independent check on top of
+// the "present" check so the error message is actionable (issue #247).
+const JWT_SECRET_MIN_LEN = 32;
+
 /**
  * Variables that MUST be provided for the server to operate safely.
  * The server will refuse to start if any of these are missing.
@@ -34,6 +40,11 @@ export const OPTIONAL_ENV_VARS = [
   "MAX_TRADES_PER_HOUR",
   "MAX_DEPOSITS_PER_DAY",
   "MAX_WITHDRAWALS_PER_DAY",
+  // Storage — required in production when STORAGE_PROVIDER=s3 (default)
+  "STORAGE_PROVIDER",
+  "S3_BUCKET",
+  "S3_REGION",
+  "S3_ENDPOINT",
 ] as const;
 
 export interface EnvValidationResult {
@@ -63,6 +74,18 @@ export function validateEnv(
     );
   }
 
+  // JWT_SECRET quality check — must be long enough to resist offline brute-force
+  // (issue #247).  Only run when the secret is present (missing case is already
+  // covered by the missingRequired check above).
+  const jwtSecret = env["JWT_SECRET"];
+  if (jwtSecret && jwtSecret.length < JWT_SECRET_MIN_LEN) {
+    errors.push(
+      `[startup] JWT_SECRET is too short (${jwtSecret.length} chars). ` +
+        `It must be at least ${JWT_SECRET_MIN_LEN} characters to provide adequate security. ` +
+        `Generate one with: node -e "console.log(require('crypto').randomBytes(32).toString('hex'))"`
+    );
+  }
+
   const encryptionKey = env["ENCRYPTION_KEY"];
   if (encryptionKey && !/^[0-9a-fA-F]{64}$/.test(encryptionKey)) {
     errors.push("[startup] ENCRYPTION_KEY must be a 64-character hex string (32 bytes)");
@@ -76,6 +99,20 @@ export function validateEnv(
     }
   }
 
+  // Storage validation — when STORAGE_PROVIDER is s3 (or unset, since s3 is the
+  // default) the S3_BUCKET and S3_REGION variables are mandatory (issue #248).
+  const storageProvider = (env["STORAGE_PROVIDER"] ?? "s3").toLowerCase();
+  if (storageProvider === "s3") {
+    const missingS3 = ["S3_BUCKET", "S3_REGION"].filter((k) => !env[k]);
+    if (missingS3.length > 0) {
+      errors.push(
+        `[startup] S3 storage is enabled (STORAGE_PROVIDER=${storageProvider}) but the following ` +
+          `required variables are missing: ${missingS3.join(", ")}. ` +
+          `Set STORAGE_PROVIDER=local to use the local filesystem for development.`
+      );
+    }
+  }
+
   return {
     valid: errors.length === 0,
     missingRequired,

server/src/db/migrations/010_withdrawals.sql

@@ -0,0 +1,80 @@
+-- Migration 010: withdrawals table and supporting indexes (issue #246).
+--
+-- The withdrawal route previously called the Paystack Transfer API inline and
+-- returned 200 on the HTTP response. This migration introduces a proper
+-- `withdrawals` table that decouples the HTTP request from the Paystack API
+-- call. The route now:
+--   1. Validates the request and checks the fiat balance.
+--   2. Inserts a withdrawal record in `pending` state.
+--   3. Deducts the amount from `wallets.fiat_balance` atomically.
+--   4. Enqueues a `process-withdrawal` background job.
+--   5. Returns 202 Accepted with the withdrawal id.
+--
+-- The `process-withdrawal` job processor calls Paystack and moves the record
+-- to `processing`. The Paystack `transfer.success` / `transfer.failed` webhook
+-- then sets the final state to `completed` or `failed`, optionally crediting
+-- the fiat_balance back on failure.
+
+-- ---------------------------------------------------------------------------
+-- withdrawals table
+-- ---------------------------------------------------------------------------
+
+CREATE TABLE IF NOT EXISTS withdrawals (
+    id                  UUID          PRIMARY KEY DEFAULT gen_random_uuid(),
+    user_id             UUID          NOT NULL REFERENCES users(id) ON DELETE CASCADE,
+
+    -- Naira amount requested by the user
+    amount              NUMERIC(18,2) NOT NULL CHECK (amount > 0),
+
+    -- Destination bank details (snapshot at request time)
+    bank_code           VARCHAR(10)   NOT NULL,
+    account_number      VARCHAR(20)   NOT NULL,
+    account_name        VARCHAR(255)  NOT NULL,
+
+    -- Paystack identifiers populated by the background processor
+    paystack_recipient_code TEXT,
+    paystack_transfer_code  TEXT,
+
+    -- Unique idempotency reference we generate; stored on the Paystack transfer
+    reference           TEXT          NOT NULL UNIQUE
+                            DEFAULT ('airflex-wd-' || gen_random_uuid()::text),
+
+    -- State machine:
+    --   pending    → job has been enqueued, no Paystack call yet
+    --   processing → Paystack transfer initiated (transfer_code obtained)
+    --   completed  → transfer.success webhook received
+    --   failed     → transfer.failed / permanent processor failure
+    status              VARCHAR(16)   NOT NULL DEFAULT 'pending'
+                            CHECK (status IN ('pending', 'processing', 'completed', 'failed')),
+
+    failure_reason      TEXT,
+
+    created_at          TIMESTAMPTZ   NOT NULL DEFAULT NOW(),
+    updated_at          TIMESTAMPTZ   NOT NULL DEFAULT NOW()
+);
+
+CREATE INDEX IF NOT EXISTS idx_withdrawals_user
+    ON withdrawals (user_id, created_at DESC);
+
+CREATE INDEX IF NOT EXISTS idx_withdrawals_status
+    ON withdrawals (status, created_at DESC);
+
+CREATE INDEX IF NOT EXISTS idx_withdrawals_reference
+    ON withdrawals (reference);
+
+-- ---------------------------------------------------------------------------
+-- Trigger: keep withdrawals.updated_at current on every UPDATE
+-- ---------------------------------------------------------------------------
+
+CREATE OR REPLACE FUNCTION set_withdrawal_updated_at()
+RETURNS TRIGGER AS $$
+BEGIN
+    NEW.updated_at = NOW();
+    RETURN NEW;
+END;
+$$ LANGUAGE plpgsql;
+
+DROP TRIGGER IF EXISTS trg_withdrawals_updated_at ON withdrawals;
+CREATE TRIGGER trg_withdrawals_updated_at
+    BEFORE UPDATE ON withdrawals
+    FOR EACH ROW EXECUTE FUNCTION set_withdrawal_updated_at();

server/src/jobs/index.ts

@@ -12,26 +12,29 @@ import { sendNotificationProcessor }      from "./processors/send-notification";
 import { verifyTradeDeliveryProcessor }   from "./processors/verify-trade-delivery";
 import { processPaystackWebhookProcessor } from "./processors/process-paystack-webhook";
 import { refundCancelledTradeProcessor }  from "./processors/refund-cancelled-trade";
+import { processWithdrawalProcessor }     from "./processors/process-withdrawal";
 
 /** Initialise the queue service and register all processors. */
 export function initJobQueue(): void {
   // Connect to Redis (falls back to in-process queue if REDIS_URL is absent)
   QueueService.init();
 
   // Register typed processors
-  QueueService.register("fund-stellar-account",   fundStellarAccountProcessor);
+  QueueService.register("fund-stellar-account",    fundStellarAccountProcessor);
   QueueService.register("create-virtual-account",  createVirtualAccountProcessor);
   QueueService.register("send-notification",       sendNotificationProcessor);
   QueueService.register("verify-trade-delivery",   verifyTradeDeliveryProcessor);
   QueueService.register("process-paystack-webhook", processPaystackWebhookProcessor);
-  QueueService.register("refund-cancelled-trade",   refundCancelledTradeProcessor);
+  QueueService.register("refund-cancelled-trade",  refundCancelledTradeProcessor);
+  QueueService.register("process-withdrawal",      processWithdrawalProcessor);
 }
 
 // Re-export queue primitives so route/service files only import from here
 export { QueueService } from "./queue";
 export type { QueueName, Job } from "./queue";
-export type { FundStellarAccountData }   from "./processors/fund-stellar-account";
-export type { CreateVirtualAccountData } from "./processors/create-virtual-account";
-export type { SendNotificationData }     from "./processors/send-notification";
-export type { VerifyTradeDeliveryData }  from "./processors/verify-trade-delivery";
-export type { RefundCancelledTradeData } from "./processors/refund-cancelled-trade";
+export type { FundStellarAccountData }    from "./processors/fund-stellar-account";
+export type { CreateVirtualAccountData }  from "./processors/create-virtual-account";
+export type { SendNotificationData }      from "./processors/send-notification";
+export type { VerifyTradeDeliveryData }   from "./processors/verify-trade-delivery";
+export type { RefundCancelledTradeData }  from "./processors/refund-cancelled-trade";
+export type { ProcessWithdrawalData }     from "./processors/process-withdrawal";

server/src/jobs/processors/process-paystack-webhook.ts

@@ -6,6 +6,7 @@ import type { Job } from "../queue";
 import {
   applyChargeSuccess,
   applyDVAAssigned,
+  applyTransferEvent,
   type PaystackWebhookEvent,
 } from "../../services/paystackWebhookHandlers";
 
@@ -55,6 +56,12 @@ export async function processPaystackWebhookProcessor(
       await applyChargeSuccess(event);
     } else if (event.event === "dedicatedaccount.assign.success") {
       await applyDVAAssigned(event);
+    } else if (
+      event.event === "transfer.success" ||
+      event.event === "transfer.failed" ||
+      event.event === "transfer.reversed"
+    ) {
+      await applyTransferEvent(event);
     }
 
     await markWebhookStatus(webhookEventId, "processed");