diff --git a/backend/package-lock.json b/backend/package-lock.json index 5154236a6..87a79279c 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -29,6 +29,7 @@ "express-async-errors": "^3.1.1", "express-rate-limit": "^7.0.0", "fast-check": "^4.8.0", + "helmet": "^8.3.0", "ioredis": "^5.10.1", "node-cache": "^5.1.2", "pg": "^8.22.0", @@ -66,6 +67,9 @@ "ts-jest": "^29.1.0", "tsx": "^4.0.0", "typescript": "^5.1.0" + }, + "engines": { + "node": ">=22" } }, "../packages/api-schemas": { @@ -78,6 +82,9 @@ "devDependencies": { "typescript": "~5.9.3", "vitest": "^4.1.5" + }, + "engines": { + "node": ">=22" } }, "node_modules/@apidevtools/json-schema-ref-parser": { @@ -6999,6 +7006,18 @@ "node": ">= 0.4" } }, + "node_modules/helmet": { + "version": "8.3.0", + "resolved": "https://registry.npmjs.org/helmet/-/helmet-8.3.0.tgz", + "integrity": "sha512-Qgpiaws3Sm30Av8Eah6sjMCZZwjlBu+E68rhpCWBshY1lb09HtLwj5GviX0OyQIn+ulUS0iX0AxN5n3tLZzz1w==", + "license": "MIT", + "engines": { + "node": ">=18.0.0" + }, + "funding": { + "url": "https://github.com/sponsors/EvanHahn" + } + }, "node_modules/html-escaper": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/html-escaper/-/html-escaper-2.0.2.tgz", diff --git a/backend/package.json b/backend/package.json index 98aaeddf1..46eec1043 100644 --- a/backend/package.json +++ b/backend/package.json @@ -72,6 +72,7 @@ "express-async-errors": "^3.1.1", "express-rate-limit": "^7.0.0", "fast-check": "^4.8.0", + "helmet": "^8.3.0", "ioredis": "^5.10.1", "node-cache": "^5.1.2", "pg": "^8.22.0", diff --git a/backend/src/__tests__/api.test.ts b/backend/src/__tests__/api.test.ts index b5236447b..77586fce3 100644 --- a/backend/src/__tests__/api.test.ts +++ b/backend/src/__tests__/api.test.ts @@ -44,6 +44,29 @@ describe('Backend API', () => { }); }); + // ─── Security Headers (Issue #1676) ────────────────────────────────────── + + describe('security headers', () => { + it('sets security headers on /health', async () => { + const response = await request(app).get('/health'); + + expect(response.headers['x-content-type-options']).toBe('nosniff'); + expect(response.headers['x-frame-options']).toBe('DENY'); + expect(response.headers['content-security-policy']).toBeDefined(); + + // HSTS is production-only; it must not be set in the test environment. + expect(response.headers['strict-transport-security']).toBeUndefined(); + }); + + it('CSP still allows the Swagger UI docs page to load', async () => { + const response = await request(app).get('/docs'); + + expect(response.status).toBe(200); + expect(response.headers['content-security-policy']).toBeDefined(); + expect(response.text).toContain('swagger'); + }); + }); + // ─── Readiness Endpoint Tests ──────────────────────────────────────────── describe('GET /ready', () => { diff --git a/backend/src/__tests__/gracefulShutdown.test.ts b/backend/src/__tests__/gracefulShutdown.test.ts index 713024481..6a0f55395 100644 --- a/backend/src/__tests__/gracefulShutdown.test.ts +++ b/backend/src/__tests__/gracefulShutdown.test.ts @@ -100,4 +100,49 @@ describe('GracefulShutdownHandler', () => { expect(handler.isShuttingDown()).toBe(true); expect(exitSpy).toHaveBeenCalledWith(0); }); + + // Issue #1675: one shutdown path — the handler registered in index.ts runs + // outbox, then DB cleanup, in a documented order after the HTTP server has + // drained in-flight requests. + it('runs onShutdown tasks in documented order after the server drains', async () => { + const server = http.createServer((_req, res) => { + // Hold the socket open briefly so the drain step is exercised. + setTimeout(() => { + res.writeHead(200); + res.end('ok'); + }, 50); + }); + await listen(server); + + const order: string[] = []; + const handler = new GracefulShutdownHandler(2000, 50); + handler.register(server); + + // Documented order (matches index.ts): background jobs/outbox first, DB last. + handler.onShutdown(async () => { + order.push('outbox-stop'); + }); + handler.onShutdown(async () => { + order.push('db-disconnect'); + }); + + let serverClosed = false; + await new Promise((resolve) => { + server.close(() => { + serverClosed = true; + resolve(); + }); + }); + + process.emit('SIGTERM'); + + await new Promise((r) => setTimeout(r, 200)); + + expect(handler.isShuttingDown()).toBe(true); + // Server closes before cleanup tasks run. + expect(serverClosed).toBe(true); + // Cleanup tasks run in registration order: outbox before DB. + expect(order).toEqual(['outbox-stop', 'db-disconnect']); + expect(exitSpy).toHaveBeenCalledWith(0); + }); }); diff --git a/backend/src/index.ts b/backend/src/index.ts index 86d83ee9d..cdad1bb1b 100644 --- a/backend/src/index.ts +++ b/backend/src/index.ts @@ -11,6 +11,7 @@ initTracing(); import 'express-async-errors'; import express, { Express, Request, Response, NextFunction, ErrorRequestHandler } from 'express'; +import helmet from 'helmet'; import { loginHandler, nonceHandler, refreshHandler, requireAuth, revokeAccessToken, revokeAllAccessTokens, revokeCurrentSession, revokeAllSessions } from './auth'; import vaultListRouter from './routes/vaults'; import { @@ -642,6 +643,29 @@ async function handleTransactionExport(req: Request, res: Response): Promise { eventOutboxService.stop(); }); - const shutdownController = new AbortController(); - const shutdownSignal = shutdownController.signal; - eventOutboxService.start({ signal: shutdownSignal }); - - // Register graceful shutdown for the outbox processor: abort in-flight work - // and wait for the poller to drain before the process tears down resources. - const stopOutbox = () => { - shutdownController.abort(); - void eventOutboxService.stop(); - }; - process.on('SIGTERM', stopOutbox); - process.on('SIGINT', stopOutbox); + eventOutboxService.start(); } diff --git a/contracts/vault/src/errors.rs b/contracts/vault/src/errors.rs index 24a4a7c41..ddd09fbe1 100644 --- a/contracts/vault/src/errors.rs +++ b/contracts/vault/src/errors.rs @@ -41,7 +41,10 @@ pub enum VaultError { ExceedsStrategyCap = 10, /// Strategy allocation exceeds configured risk threshold. ExceedsRiskThreshold = 11, - /// Withdrawal blocked due to active deposit cooldown. + /// Withdrawal blocked due to an active deposit cooldown, or because the + /// user already has a pending large withdrawal that must be executed or + /// cancelled with `cancel_withdrawal` before another can be queued + /// (Issue #1680). WithdrawalCooldownActive = 12, /// Requested storage migration target is older than the current stored version. /// diff --git a/contracts/vault/src/lib.rs b/contracts/vault/src/lib.rs index 22e6d6983..599471847 100644 --- a/contracts/vault/src/lib.rs +++ b/contracts/vault/src/lib.rs @@ -858,11 +858,6 @@ impl YieldVault { .get(&DataKey::LastStrategySwitchTime) .unwrap_or(0); let earliest_switch = last_switch.saturating_add(cooldown); - if env.ledger().timestamp() < earliest_switch { - let earliest_switch = last_switch.checked_add(cooldown).expect("overflow"); - if now < earliest_switch { - return Err(VaultError::AdminParamChangeTooSoon); - } } let current_strategy = Self::strategy(env.clone()); @@ -1476,43 +1471,18 @@ impl YieldVault { let oracle_client = OracleClient::new(&env, &oracle_addr); let price_data = oracle_client.get_price(&token, &token); let max_age = Self::oracle_heartbeat(env.clone()); - let last: Option = env - .storage() - .instance() - .get(&DataKeyExt::Risk(RiskExtKey::LastPx)); - let _last_price = Self::last_oracle_price(&env); let last_price = Self::last_oracle_price(&env); oracle::OracleValidator::validate_price_data( &env, &price_data, max_age, Some(oracle::MAX_PRICE_DEVIATION_BPS), - last.as_ref(), last_price.as_ref(), ) .map_err(|_| VaultError::OracleValidationFailed)?; Self::set_last_oracle_price(&env, &price_data); } - let token = Self::token(env.clone()); - if Self::is_oracle_enabled(env.clone()) { - if let Some(oracle_addr) = Self::price_oracle(env.clone()) { - let oracle_client = OracleClient::new(&env, &oracle_addr); - let price_data = oracle_client.get_price(&token, &token); - let max_age = Self::oracle_heartbeat(env.clone()); - let last_price = Self::last_oracle_price(&env); - oracle::OracleValidator::validate_price_data( - &env, - &price_data, - max_age, - Some(oracle::MAX_PRICE_DEVIATION_BPS), - last_price.as_ref(), - ) - .map_err(|_| VaultError::OracleValidationFailed)?; - Self::set_last_oracle_price(&env, &price_data); } - } - - let strategy_assets = if let Some(strategy_addr) = Self::strategy(env.clone()) { Self::require_valid_strategy_response(&env, &strategy_addr, &token) } else { 0 @@ -2850,6 +2820,10 @@ impl YieldVault { /// is created with a 24-hour timelock. Call `execute_withdrawal` after the /// timelock expires to complete the transfer. /// + /// A user with a pending large withdrawal cannot queue another one: the + /// request is rejected until the pending withdrawal is executed or + /// cancelled with [`Self::cancel_withdrawal`] (#1680). + /// /// ### Parameters /// * `user` - The share holder (requires auth). /// * `shares` - The number of shares to burn. @@ -2907,6 +2881,14 @@ impl YieldVault { .map_err(|_| VaultError::MathOverflow)?; if assets_to_return > threshold { + // A user may not queue a second large withdrawal while one is + // already pending (#1680): the pending request must be executed + // or cancelled first, so we never silently overwrite it and + // restart its 24-hour timelock. + if env.storage().instance().has(&DataKey::PendingWithdrawal(user.clone())) { + return Err(VaultError::WithdrawalCooldownActive); + } + // Create a pending withdrawal with a 24-hour timelock let unlock_ts = env .ledger() @@ -2971,6 +2953,39 @@ impl YieldVault { Self::do_withdraw(&env, &mut state, user, pending.shares, assets_to_return) } + /// Returns the pending large withdrawal for `user`, if one exists. + pub fn pending_withdrawal(env: Env, user: Address) -> Option { + env.storage().instance().get(&DataKey::PendingWithdrawal(user)) + } + + /// Cancel a pending large withdrawal before its timelock expires. + /// + /// The caller must own the pending withdrawal (`user.require_auth()`). + /// Cancelling removes the pending entry so the user can submit a new + /// withdrawal instead of waiting out the 24-hour timelock (#1681). + /// + /// ### Errors + /// * [`VaultError::NoPendingWithdrawal`] - no pending withdrawal exists + /// for `user`. + pub fn cancel_withdrawal(env: Env, user: Address) -> Result<(), VaultError> { + user.require_auth(); + + let pending: PendingWithdrawal = env + .storage() + .instance() + .get(&DataKey::PendingWithdrawal(user.clone())) + .ok_or(VaultError::NoPendingWithdrawal)?; + + env.storage() + .instance() + .remove(&DataKey::PendingWithdrawal(user.clone())); + + env.events() + .publish((symbol_short!("pndwcn"), user), (pending.shares, pending.unlock_timestamp)); + + Ok(()) + } + /// Internal: burns shares, transfers assets, updates state. fn do_withdraw( env: &Env, diff --git a/contracts/vault/src/permissions.rs b/contracts/vault/src/permissions.rs index fd5f0013c..70c992446 100644 --- a/contracts/vault/src/permissions.rs +++ b/contracts/vault/src/permissions.rs @@ -13,10 +13,8 @@ //! - Migration-safe updates: old and new signer sets coexist during transition //! - Operations require threshold signatures from the current active set -use soroban_sdk::{Address, Vec}; -use crate::VaultError; -use crate::VaultError; use soroban_sdk::{Address, Env, Map, Vec}; +use crate::VaultError; /// Verifies that the caller is the admin /// @@ -49,15 +47,11 @@ pub fn require_pauser_or_admin_auth( caller.require_auth(); let is_admin = caller == admin; let is_pauser = pauser.map_or(false, |p| caller == p); - if !(is_admin || is_pauser) { - return Err(VaultError::RescueUnauthorized); + if is_admin || is_pauser { + Ok(()) + } else { + Err(VaultError::RescueUnauthorized) } - Ok(()) - let is_pauser = pauser == Some(caller); - assert!( - is_admin || is_pauser, - "unauthorized: caller must be admin or pauser" - ); } /// Multi-signer threshold validator for governance operations. diff --git a/contracts/vault/src/test.rs b/contracts/vault/src/test.rs index 8ca2af415..88e5e89e9 100644 --- a/contracts/vault/src/test.rs +++ b/contracts/vault/src/test.rs @@ -32,7 +32,7 @@ use crate::strategy_registration::{STATE_ACTIVE, STATE_PENDING, STATE_RETIRED}; use soroban_sdk::testutils::{Address as _, Events as _, Ledger as _}; use soroban_sdk::xdr::{ContractEventBody, ScSymbol, ScVal}; use soroban_sdk::{ - contract, contractimpl, contracttype, token, Address, Env, IntoVal, TryFromVal, Vec, + contract, contractimpl, contracttype, token, Address, Env, IntoVal, Symbol, TryFromVal, Vec, }; #[contract] @@ -1756,6 +1756,122 @@ fn test_withdrawal_cooldown_then_timelock_then_execute() { assert_eq!(executed, 50_000); } +// ─── 12. Large withdrawal pending-state guards (Issues #1680, #1681) ───────── + +#[test] +fn test_second_large_withdrawal_rejected_while_pending() { + let env = Env::default(); + env.mock_all_auths(); + + let (vault, _, usdc_sa, _) = setup_vault(&env); + let user = Address::generate(&env); + usdc_sa.mint(&user, &100_000); + + vault.set_large_withdrawal_threshold(&1000); + vault.deposit(&user, &100_000); + + // First large withdrawal queues a pending timelocked withdrawal. + assert_eq!(vault.withdraw(&user, &50_000), 0); + assert_eq!( + vault.pending_withdrawal(&user), + Some(PendingWithdrawal { + shares: 50_000, + unlock_timestamp: 100 + 86_400, + }) + ); + + // A second large withdrawal while one is pending is rejected. + let result = vault.try_withdraw(&user, &25_000); + assert_eq!(result, Err(Ok(VaultError::WithdrawalCooldownActive))); + + // The original pending entry is untouched: no silent overwrite, no new timelock. + let pending = vault.pending_withdrawal(&user).unwrap(); + assert_eq!(pending.shares, 50_000); + assert_eq!(pending.unlock_timestamp, 100 + 86_400); +} + +#[test] +fn test_cancel_withdrawal_requires_user_auth() { + let env = Env::default(); + env.mock_all_auths(); + + let (vault, _, usdc_sa, _) = setup_vault(&env); + let user = Address::generate(&env); + usdc_sa.mint(&user, &100_000); + + vault.set_large_withdrawal_threshold(&1000); + + vault.deposit(&user, &100_000); + assert_eq!(vault.withdraw(&user, &50_000), 0); + assert!(vault.pending_withdrawal(&user).is_some()); + + // The owner can cancel (auth satisfied via mock_all_auths). + vault.cancel_withdrawal(&user); + assert_eq!(vault.pending_withdrawal(&user), None); +} + +#[test] +fn test_cancel_withdrawal_without_pending_returns_error() { + let env = Env::default(); + env.mock_all_auths(); + + let (vault, _, _, _) = setup_vault(&env); + let user = Address::generate(&env); + + let result = vault.try_cancel_withdrawal(&user); + assert_eq!(result, Err(Ok(VaultError::NoPendingWithdrawal))); +} + +#[test] +fn test_cancel_withdrawal_then_new_withdrawal() { + let env = Env::default(); + env.mock_all_auths(); + + let (vault, _, usdc_sa, _) = setup_vault(&env); + let user = Address::generate(&env); + usdc_sa.mint(&user, &100_000); + + vault.set_large_withdrawal_threshold(&1000); + vault.deposit(&user, &100_000); + + vault.withdraw(&user, &50_000); + assert!(vault.pending_withdrawal(&user).is_some()); + + vault.cancel_withdrawal(&user); + assert_eq!(vault.pending_withdrawal(&user), None); + + // Cancel clears the way for a new large withdrawal with a fresh timelock. + assert_eq!(vault.withdraw(&user, &50_000), 0); + assert_eq!(vault.pending_withdrawal(&user).unwrap().shares, 50_000); +} + +#[test] +fn test_cancel_withdrawal_emits_pndwcn_event() { + let env = Env::default(); + env.mock_all_auths(); + + let (vault, _, usdc_sa, _) = setup_vault(&env); + let user = Address::generate(&env); + usdc_sa.mint(&user, &100_000); + + vault.set_large_withdrawal_threshold(&1000); + vault.deposit(&user, &100_000); + vault.withdraw(&user, &50_000); + vault.cancel_withdrawal(&user); + + let mut found = false; + for event in env.events().all().iter() { + if !event.1.is_empty() { + if let Ok(topic) = Symbol::try_from_val(&env, &event.1.get(0).unwrap()) { + if topic == symbol_short!("pndwcn") { + found = true; + } + } + } + } + assert!(found, "expected `pndwcn` cancel event"); +} + // ─── 11. batch_deposit ──────────────────────────────────────────────────────── /// Helper: set up a vault with a registered relayer and mint USDC to `users`. diff --git a/docs/api/ERROR_CODE_CATALOG.md b/docs/api/ERROR_CODE_CATALOG.md index 1679cf257..e77ff7d22 100644 --- a/docs/api/ERROR_CODE_CATALOG.md +++ b/docs/api/ERROR_CODE_CATALOG.md @@ -173,7 +173,7 @@ source of truth for contract failure behavior. | 9 | `LiquidityBufferNotMet` | `invest` would breach minimum idle buffer | Reduce invest amount or wait for deposits | | 10 | `ExceedsStrategyCap` | Strategy allocation exceeds cap | Lower allocation amount | | 11 | `ExceedsRiskThreshold` | Allocation exceeds risk threshold | Reduce allocation or adjust threshold | -| 12 | `WithdrawalCooldownActive` | Withdrawal during deposit cooldown | Wait for cooldown window | +| 12 | `WithdrawalCooldownActive` | Withdrawal during deposit cooldown; also raised by `withdraw` when a large withdrawal is already pending for the user (Issue #1680) | Wait for cooldown window; cancel or execute the pending withdrawal first | | 13 | `InvalidMigrationTarget` | Storage migration target invalid; also reused by `update_shipment_status` for an invalid RWA shipment lifecycle transition | Use current or next storage version; use a valid shipment status transition | | 14 | `MathOverflow` | Arithmetic overflow guard | Lower amounts; report if unexpected | | 15 | `SlippageExceeded` | Strategy slippage limit exceeded | Retry with adjusted bounds |