From 90495894543a528b1b90fed2849d468f5c5d4c88 Mon Sep 17 00:00:00 2001 From: Marvelous Oni Date: Tue, 25 Aug 2026 16:13:37 +0000 Subject: [PATCH 1/2] fix: secure POST /transactions/submit against third-party XDR (#117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bind submissions to the authenticated wallet (source account, fee-bump inner source, or Soroban invocation auth), enforce a per-type operation allowlist, make submission idempotent per transaction hash via a unique-index migration, rate limit per wallet and IP, and persist records before Horizon submission so persistence failures surface instead of being silently dropped. Generated with Codebuff 🤖 Co-Authored-By: Codebuff --- context/progress-tracker.md | 33 ++ .../dto/submit-transaction-request.dto.ts | 16 + .../dto/submit-transaction-response.dto.ts | 19 +- .../transactions/transactions.controller.ts | 21 +- .../transactions/transactions.service.ts | 282 +++++++++++++- .../transactions/wallet-throttler.guard.ts | 16 + ...0825000001_add_unique_transaction_hash.sql | 31 ++ .../transactions.controller.spec.ts | 5 + .../transactions/transactions.service.spec.ts | 361 +++++++++++++++++- .../wallet-throttler.guard.spec.ts | 40 ++ 10 files changed, 797 insertions(+), 27 deletions(-) create mode 100644 src/modules/transactions/wallet-throttler.guard.ts create mode 100644 supabase/migrations/20260825000001_add_unique_transaction_hash.sql create mode 100644 test/unit/modules/transactions/wallet-throttler.guard.spec.ts diff --git a/context/progress-tracker.md b/context/progress-tracker.md index eac5d06..2462f1e 100644 --- a/context/progress-tracker.md +++ b/context/progress-tracker.md @@ -6,6 +6,39 @@ pure chore/docs commits). Direct pushes to main must also be logged here. --- +## 2026-08-25 + +- Secured `POST /transactions/submit` (#117): + - **Source binding** — the authenticated wallet must be the transaction + source account (or the inner source for fee-bump transactions), or must + appear as an authorized address in the Soroban invocation auth. Third-party + XDR where the wallet is neither source nor authorizer is rejected with + `TRANSACTION_SOURCE_MISMATCH`. (Deposit/withdraw/repay/vendor XDRs built + by this API use a random source account and authorize via Soroban auth, so + the auth check keeps those flows working.) + - **Operation allowlist per type** — every operation must be a Soroban + `invokeHostFunction` whose function name matches the declared type + (`create_loan`, `repay_loan`/`repay_installment`, `deposit`, `withdraw`, + `approve_vendor`, `suspend_vendor`) and, when configured, must target the + contract owned by that flow. Rejections use `TRANSACTION_TYPE_MISMATCH` / + `TRANSACTION_OPERATION_NOT_ALLOWED`. + - **Idempotency** — migration + `20260825000001_add_unique_transaction_hash.sql` adds partial unique + indexes on `transaction_hash` and `hash` (with pre-existing row dedupe); + the service checks for an existing record before submitting and returns it + (`duplicate: true`) instead of re-submitting, with the unique-constraint + violation as the concurrency backstop. + - **Rate limits** — `WalletThrottlerGuard` keys `@nestjs/throttler` on the + authenticated wallet; the submit route is limited to 10 req / 60 s per + wallet AND per IP (global guard), matching the auth-endpoint pattern. + - **Persistence-first** — the local record is written (await) before the + Horizon submission, so persistence failures surface as + `TRANSACTION_PERSISTENCE_FAILED` instead of being silently dropped, and + the transaction hash is always known to the status checker / indexer. + - Updated `SubmitTransactionResponseDto` (`status` may reflect the recorded + status, plus `duplicate` flag), controller Swagger, and unit tests covering + every rejection branch plus the happy path. + ## 2026-07-23 - Added GitHub Actions health check workflow (`health-check.yml`) to ping the Render API every 6 hours to prevent the free tier instance from sleeping. Auto-creates or comments on issues with the `incident` label if the ping fails, preventing silent outages. diff --git a/src/modules/transactions/dto/submit-transaction-request.dto.ts b/src/modules/transactions/dto/submit-transaction-request.dto.ts index 2a89e71..453b1e9 100644 --- a/src/modules/transactions/dto/submit-transaction-request.dto.ts +++ b/src/modules/transactions/dto/submit-transaction-request.dto.ts @@ -13,6 +13,22 @@ export enum TransactionType { /** * DTO for submitting a signed Stellar XDR transaction to the network. + * + * POST /transactions/submit enforces the following guarantees: + * - The transaction source account (or, for fee-bump transactions, the inner + * source account) must equal the authenticated wallet, or the wallet must + * appear as an authorized address in the Soroban invocation auth. XDR in + * which the wallet is neither source nor authorizer is rejected with + * `TRANSACTION_SOURCE_MISMATCH`. + * - The declared `type` must match the operations contained in the XDR + * (e.g. `deposit` must be a Soroban `deposit` invocation on the liquidity + * pool contract). Mismatches are rejected with `TRANSACTION_TYPE_MISMATCH` + * or `TRANSACTION_OPERATION_NOT_ALLOWED`. + * - Submission is idempotent per transaction hash: re-submitting an already + * recorded hash returns the original record without a second Horizon + * submission (`duplicate: true` on the response). + * - The local record is persisted before the Horizon submission, so + * persistence failures surface as errors rather than being silently dropped. */ export class SubmitTransactionRequestDto { @ApiProperty({ diff --git a/src/modules/transactions/dto/submit-transaction-response.dto.ts b/src/modules/transactions/dto/submit-transaction-response.dto.ts index 5fe4d82..9c35b2d 100644 --- a/src/modules/transactions/dto/submit-transaction-response.dto.ts +++ b/src/modules/transactions/dto/submit-transaction-response.dto.ts @@ -1,7 +1,11 @@ -import { ApiProperty } from '@nestjs/swagger'; +import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; /** * DTO returned after successfully submitting a transaction to the Stellar network. + * + * Submission is idempotent per transaction hash: when the same hash was already + * recorded locally, the original record is returned (with `duplicate: true`) + * instead of submitting to Horizon a second time. */ export class SubmitTransactionResponseDto { @ApiProperty({ @@ -11,8 +15,17 @@ export class SubmitTransactionResponseDto { transactionHash: string; @ApiProperty({ - description: 'Transaction status immediately after submission', + description: + 'Transaction status. Fresh submissions return `pending`; duplicate submissions return the recorded status of the original record.', + enum: ['pending', 'success', 'failed'], example: 'pending', }) - status: 'pending'; + status: 'pending' | 'success' | 'failed'; + + @ApiPropertyOptional({ + description: + 'True when the hash was already recorded locally and the original record is returned without re-submitting to Horizon.', + example: false, + }) + duplicate?: boolean; } diff --git a/src/modules/transactions/transactions.controller.ts b/src/modules/transactions/transactions.controller.ts index d13c45e..59b10cc 100644 --- a/src/modules/transactions/transactions.controller.ts +++ b/src/modules/transactions/transactions.controller.ts @@ -21,6 +21,11 @@ import { SubmitTransactionResponseDto } from './dto/submit-transaction-response. import { TransactionStatusResponseDto } from './dto/transaction-status-response.dto'; import { JwtAuthGuard } from '../../common/guards/jwt-auth.guard'; import { CurrentUser } from '../../common/decorators/current-user.decorator'; +import { Throttle } from '@nestjs/throttler'; +import { WalletThrottlerGuard } from './wallet-throttler.guard'; + +const SUBMIT_RATE_LIMIT = 10; +const SUBMIT_RATE_TTL_MS = 60000; @ApiTags('transactions') @Controller('transactions') @@ -31,20 +36,28 @@ export class TransactionsController { @Post('submit') @HttpCode(HttpStatus.OK) - @UseGuards(JwtAuthGuard) + @Throttle({ default: { limit: SUBMIT_RATE_LIMIT, ttl: SUBMIT_RATE_TTL_MS } }) + @UseGuards(JwtAuthGuard, WalletThrottlerGuard) @ApiBearerAuth() @ApiOperation({ summary: 'Submit a signed XDR transaction to the Stellar network', description: - 'Validates the XDR format, submits the signed transaction to the Stellar network via Horizon API, stores the transaction hash with pending status in the database, and returns the hash immediately without waiting for confirmation.', + 'Validates that the XDR source account (or Soroban authorization) matches the authenticated wallet and that the operations match the declared type, then persists the transaction record and submits the signed transaction to the Stellar network via Horizon. Returns the hash immediately without waiting for confirmation. Submission is idempotent per transaction hash: re-submitting an already recorded hash returns the original record. Rate limited per wallet and per IP.', }) @ApiResponse({ status: 200, - description: 'Transaction submitted successfully — hash returned with pending status', + description: + 'Transaction submitted successfully — hash returned with pending status (or the original record when the hash was already submitted)', type: SubmitTransactionResponseDto, }) - @ApiResponse({ status: 400, description: 'Malformed XDR, invalid signature, or Stellar rejection' }) + @ApiResponse({ + status: 400, + description: + 'Malformed XDR, source account mismatch (TRANSACTION_SOURCE_MISMATCH), operation/type mismatch (TRANSACTION_TYPE_MISMATCH, TRANSACTION_OPERATION_NOT_ALLOWED), or Stellar rejection', + }) @ApiResponse({ status: 401, description: 'Unauthorized - missing or invalid JWT' }) + @ApiResponse({ status: 429, description: 'Too many requests - rate limit exceeded (per wallet or per IP)' }) + @ApiResponse({ status: 500, description: 'Failed to persist the transaction record locally (TRANSACTION_PERSISTENCE_FAILED)' }) @ApiResponse({ status: 503, description: 'Stellar network temporarily unavailable' }) async submitTransaction( @CurrentUser() user: { wallet: string }, diff --git a/src/modules/transactions/transactions.service.ts b/src/modules/transactions/transactions.service.ts index c5baa08..d1aaace 100644 --- a/src/modules/transactions/transactions.service.ts +++ b/src/modules/transactions/transactions.service.ts @@ -13,6 +13,9 @@ import { Cache } from 'cache-manager'; import * as StellarSdk from 'stellar-sdk'; import { SupabaseService } from '../../database/supabase.client'; import { SubmitTransactionRequestDto, TransactionType } from './dto/submit-transaction-request.dto'; +import { CREDIT_LINE_CONTRACT_ID_KEY } from '../../stellar/contracts/interfaces/creditline.interface'; +import { LIQUIDITY_POOL_CONTRACT_ID_KEY } from '../../stellar/contracts/interfaces/liquidity-pool.interface'; +import { VENDOR_REGISTRY_CONTRACT_ID_KEY } from '../../stellar/contracts/interfaces/vendor-registry.interface'; import { SubmitTransactionResponseDto } from './dto/submit-transaction-response.dto'; import { TransactionErrorDetailsDto, @@ -61,6 +64,53 @@ type TransactionRecord = { const FINALIZED_TRANSACTION_CACHE_TTL = 0; +/** + * Per-type operation allowlist for POST /transactions/submit. + * + * Every operation in a submitted transaction must be a Soroban contract + * invocation whose function name matches the declared transaction type, and + * (when the expected contract ID is configured) must target the contract + * owned by that flow. Anything else — payments, account merges, trustlines, + * or invocations of StepFi's contracts under the wrong declared type — is + * rejected before reaching Horizon. + */ +interface TransactionTypeAllowlist { + functionNames: readonly string[]; + contractIdKey: string; +} + +const TRANSACTION_TYPE_ALLOWLIST: Record = { + [TransactionType.LOAN_CREATE]: { + functionNames: ['create_loan'], + contractIdKey: CREDIT_LINE_CONTRACT_ID_KEY, + }, + [TransactionType.LOAN_REPAY]: { + // `repay_loan` is the canonical contract entry point; `repay_installment` + // is accepted while the repayment flow is migrating between them. + functionNames: ['repay_loan', 'repay_installment'], + contractIdKey: CREDIT_LINE_CONTRACT_ID_KEY, + }, + [TransactionType.DEPOSIT]: { + functionNames: ['deposit'], + contractIdKey: LIQUIDITY_POOL_CONTRACT_ID_KEY, + }, + [TransactionType.WITHDRAW]: { + functionNames: ['withdraw'], + contractIdKey: LIQUIDITY_POOL_CONTRACT_ID_KEY, + }, + [TransactionType.VENDOR_APPROVE]: { + functionNames: ['approve_vendor'], + contractIdKey: VENDOR_REGISTRY_CONTRACT_ID_KEY, + }, + [TransactionType.VENDOR_SUSPEND]: { + functionNames: ['suspend_vendor'], + contractIdKey: VENDOR_REGISTRY_CONTRACT_ID_KEY, + }, +}; + +type StellarTransaction = StellarSdk.Transaction | StellarSdk.FeeBumpTransaction; +type StellarOperation = StellarSdk.Transaction['operations'][number]; + @Injectable() export class TransactionsService { private readonly logger = new Logger(TransactionsService.name); @@ -90,19 +140,58 @@ export class TransactionsService { ): Promise { const transaction = this.parseXdr(dto.xdr); - let transactionHash: string; + // 1. The declared type must match the operations actually contained in the XDR. + this.assertOperationAllowlist(transaction, dto.type); + + // 2. The authenticated wallet must be the source account or an authorizer. + this.assertWalletAuthorizes(transaction, wallet); + + const transactionHash = transaction.hash().toString('hex'); + + // 3. Idempotency: an already-recorded hash is returned as-is, never + // re-submitted to Horizon. + const existing = await this.findTransactionRecord(transactionHash); + if (existing) { + this.logger.log( + `Duplicate submission — returning existing record for hash ${transactionHash} (status: ${existing.status ?? 'pending'})`, + ); + return { + transactionHash, + status: existing.status ?? 'pending', + duplicate: true, + }; + } + + // 4. Persist first so persistence failures surface instead of being + // silently dropped. The unique hash indexes backstop the check above + // against concurrent duplicate submissions. try { - const horizonResult = await this.horizonServer.submitTransaction(transaction); - transactionHash = horizonResult.hash; + await this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr); } catch (error) { - this.handleHorizonError(error); + if (this.isUniqueViolationError(error)) { + const existingAfterRace = await this.findTransactionRecord(transactionHash); + if (existingAfterRace) { + return { + transactionHash, + status: existingAfterRace.status ?? 'pending', + duplicate: true, + }; + } + } + + throw new InternalServerErrorException({ + code: 'TRANSACTION_PERSISTENCE_FAILED', + message: 'Failed to record the transaction locally. No transaction was submitted to the Stellar network — please try again.', + }); } - this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr).catch((err) => { - this.logger.error( - `Failed to persist transaction record for hash ${transactionHash}: ${err.message}`, - ); - }); + // 5. Submit to Horizon only after the local record exists, so the + // transaction hash is always known to the status checker / indexer. + try { + await this.horizonServer.submitTransaction(transaction); + } catch (error) { + this.handleHorizonError(error); + } this.logger.log( `Transaction submitted — hash: ${transactionHash}, type: ${dto.type}, wallet: ${wallet.slice(0, 8)}...`, @@ -162,6 +251,181 @@ export class TransactionsService { } } + /** + * Rejects transactions whose operations do not match the declared type. + * Every operation must be a Soroban contract invocation whose function name + * is allowlisted for the type, targeting the contract configured for that + * flow when a contract ID is configured. + */ + private assertOperationAllowlist(transaction: StellarTransaction, type: TransactionType): void { + const allowlist = TRANSACTION_TYPE_ALLOWLIST[type]; + const operations = this.getInnerTransaction(transaction).operations; + + if (!operations || operations.length === 0) { + throw new BadRequestException({ + code: 'TRANSACTION_TYPE_MISMATCH', + message: `Transaction for type '${type}' must contain at least one operation.`, + }); + } + + for (const operation of operations) { + if (operation.type !== 'invokeHostFunction') { + throw new BadRequestException({ + code: 'TRANSACTION_OPERATION_NOT_ALLOWED', + message: `Operation '${operation.type}' is not allowed for transaction type '${type}'. Only Soroban contract invocations are accepted.`, + }); + } + + const invocation = this.extractInvocationAttributes(operation); + if (!invocation.functionName) { + throw new BadRequestException({ + code: 'TRANSACTION_TYPE_MISMATCH', + message: `Could not determine the invoked contract function for transaction type '${type}'.`, + }); + } + + if (!allowlist.functionNames.includes(invocation.functionName)) { + throw new BadRequestException({ + code: 'TRANSACTION_TYPE_MISMATCH', + message: `Transaction type '${type}' does not allow contract function '${invocation.functionName}'. Allowed: ${allowlist.functionNames.join(', ')}.`, + }); + } + + const expectedContractId = this.configService.get(allowlist.contractIdKey); + if (expectedContractId && invocation.contractId && invocation.contractId !== expectedContractId.trim()) { + throw new BadRequestException({ + code: 'TRANSACTION_TYPE_MISMATCH', + message: `Transaction type '${type}' must invoke contract ${expectedContractId}, but the XDR targets ${invocation.contractId}.`, + }); + } + } + } + + /** + * Rejects third-party-sourced XDR: the authenticated wallet must be the + * transaction source (the inner source for fee-bump transactions), or must + * appear as an authorized address in the Soroban invocation auth. This + * prevents transactions signed by entirely different accounts from being + * recorded as the authenticated user's activity. + */ + private assertWalletAuthorizes(transaction: StellarTransaction, wallet: string): void { + const effectiveSource = + transaction instanceof StellarSdk.FeeBumpTransaction + ? transaction.innerTransaction.source + : transaction.source; + + if (effectiveSource === wallet) { + return; + } + + if (this.collectAuthorizedAddresses(transaction).includes(wallet)) { + return; + } + + throw new BadRequestException({ + code: 'TRANSACTION_SOURCE_MISMATCH', + message: + 'The transaction source account does not match the authenticated wallet, and the wallet is not an authorizer of this transaction. Only transactions signed by your wallet can be submitted.', + }); + } + + private getInnerTransaction(transaction: StellarTransaction): StellarSdk.Transaction { + return transaction instanceof StellarSdk.FeeBumpTransaction + ? transaction.innerTransaction + : transaction; + } + + /** + * Extracts the invoked function name, target contract ID, and authorization + * entries from a Soroban invokeHostFunction operation. Reaches into the XDR + * object's internal structure (no public accessor), mirroring the existing + * pattern in the transaction status checker. + */ + private extractInvocationAttributes(operation: StellarOperation): { + functionName?: string; + contractId?: string; + auth: unknown[]; + } { + const func = (operation as { func?: unknown }).func; + const attributes = (func as { _value?: { _attributes?: unknown } })?._value?._attributes as + | { + functionName?: { toString?: () => string }; + contractAddress?: unknown; + auth?: unknown[]; + } + | undefined; + + const functionName = attributes?.functionName?.toString?.(); + + let contractId: string | undefined; + const rawContractAddress = attributes?.contractAddress; + if (rawContractAddress) { + const buffer = Buffer.isBuffer(rawContractAddress) + ? rawContractAddress + : typeof (rawContractAddress as { value?: () => unknown }).value === 'function' + ? (rawContractAddress as { value: () => unknown }).value() + : undefined; + + if (Buffer.isBuffer(buffer) && buffer.length === 32) { + try { + contractId = StellarSdk.StrKey.encodeContract(buffer); + } catch { + contractId = undefined; + } + } + } + + const auth = Array.isArray(attributes?.auth) ? attributes.auth : []; + + return { functionName, contractId, auth }; + } + + /** + * Collects the wallet addresses authorized by a transaction's Soroban auth + * entries (both address- and account-credential forms). Malformed entries + * are skipped — the source-account check still applies. + */ + private collectAuthorizedAddresses(transaction: StellarTransaction): string[] { + const addresses: string[] = []; + const operations = this.getInnerTransaction(transaction).operations; + + for (const operation of operations) { + if (operation.type !== 'invokeHostFunction') { + continue; + } + + for (const entry of this.extractInvocationAttributes(operation).auth) { + try { + const credentials = (entry as { credentials?: () => unknown }).credentials?.(); + const switchName = (credentials as { switch?: () => { name?: string } })?.switch?.()?.name; + const value = (credentials as { value?: () => unknown })?.value?.(); + const addressHolder = + switchName === 'sorobanCredentialsAccount' + ? (value as { account?: () => unknown })?.account?.() + : (value as { address?: () => unknown })?.address?.(); + const address = (addressHolder as { + address?: () => { toString?: () => string }; + })?.address?.()?.toString?.(); + + if (address && !addresses.includes(address)) { + addresses.push(address); + } + } catch { + // Skip malformed auth entries; the source-account check still applies. + } + } + } + + return addresses; + } + + private isUniqueViolationError(error: unknown): boolean { + const err = error as { code?: string; message?: string }; + const code = err?.code; + const message = err?.message?.toLowerCase() ?? ''; + return code === '23505' || message.includes('duplicate key value violates unique constraint'); + } + private handleHorizonError(error: unknown): never { const err = error as { response?: { data?: { extras?: { result_codes?: { transaction?: string; operations?: string[] } } } }; diff --git a/src/modules/transactions/wallet-throttler.guard.ts b/src/modules/transactions/wallet-throttler.guard.ts new file mode 100644 index 0000000..8fdc2bd --- /dev/null +++ b/src/modules/transactions/wallet-throttler.guard.ts @@ -0,0 +1,16 @@ +import { Injectable } from '@nestjs/common'; +import { ThrottlerGuard } from '@nestjs/throttler'; + +/** + * ThrottlerGuard variant that keys rate limits on the authenticated wallet + * (from the JWT payload) instead of the client IP. Used alongside the global + * IP-based guard so POST /transactions/submit is bounded per wallet AND per + * IP, preventing a single wallet from being used as an open relay to Horizon. + */ +@Injectable() +export class WalletThrottlerGuard extends ThrottlerGuard { + protected async getTracker(req: { user?: { wallet?: string } }): Promise { + const wallet = req.user?.wallet; + return wallet ? `wallet:${wallet}` : super.getTracker(req); + } +} diff --git a/supabase/migrations/20260825000001_add_unique_transaction_hash.sql b/supabase/migrations/20260825000001_add_unique_transaction_hash.sql new file mode 100644 index 0000000..3ac5fcc --- /dev/null +++ b/supabase/migrations/20260825000001_add_unique_transaction_hash.sql @@ -0,0 +1,31 @@ +-- Idempotency backstop for POST /transactions/submit (issue #117). +-- +-- A Stellar transaction hash may only be recorded once: duplicate submissions +-- return the original record instead of re-submitting to Horizon. The unique +-- indexes below make that guarantee hold even under concurrent requests. +-- Partial indexes are used because both columns are nullable (legacy rows may +-- only populate one). + +-- Deduplicate pre-existing rows (keep the earliest record per hash) so the +-- unique indexes below can be created. +DELETE FROM public.transactions AS dup +USING public.transactions AS kept +WHERE dup.transaction_hash IS NOT NULL + AND dup.transaction_hash = kept.transaction_hash + AND dup.id <> kept.id + AND dup.submitted_at > kept.submitted_at; + +DELETE FROM public.transactions AS dup +USING public.transactions AS kept +WHERE dup.hash IS NOT NULL + AND dup.hash = kept.hash + AND dup.id <> kept.id + AND dup.submitted_at > kept.submitted_at; + +CREATE UNIQUE INDEX transactions_transaction_hash_unique + ON public.transactions (transaction_hash) + WHERE transaction_hash IS NOT NULL; + +CREATE UNIQUE INDEX transactions_hash_unique + ON public.transactions (hash) + WHERE hash IS NOT NULL; diff --git a/test/unit/modules/transactions/transactions.controller.spec.ts b/test/unit/modules/transactions/transactions.controller.spec.ts index bd0dcb0..5882813 100644 --- a/test/unit/modules/transactions/transactions.controller.spec.ts +++ b/test/unit/modules/transactions/transactions.controller.spec.ts @@ -1,5 +1,6 @@ import { BadRequestException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; +import { ThrottlerModule } from '@nestjs/throttler'; import { TransactionsController } from '../../../../src/modules/transactions/transactions.controller'; import { TransactionType } from '../../../../src/modules/transactions/dto/submit-transaction-request.dto'; import { TransactionsService } from '../../../../src/modules/transactions/transactions.service'; @@ -18,7 +19,11 @@ describe('TransactionsController', () => { beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ + imports: [ThrottlerModule.forRoot([{ ttl: 60000, limit: 1000 }])], controllers: [TransactionsController], + // The WalletThrottlerGuard on the submit route needs the throttler + // options/storage provided by ThrottlerModule above; the JWT guard is + // resolved by passport and needs no providers here. providers: [{ provide: TransactionsService, useValue: mockTransactionsService }], }).compile(); diff --git a/test/unit/modules/transactions/transactions.service.spec.ts b/test/unit/modules/transactions/transactions.service.spec.ts index 06911a7..8127382 100644 --- a/test/unit/modules/transactions/transactions.service.spec.ts +++ b/test/unit/modules/transactions/transactions.service.spec.ts @@ -60,10 +60,24 @@ describe('TransactionsService', () => { getServiceRoleClient: jest.fn().mockReturnValue(mockSupabaseClient), }; + const LIQUIDITY_CONTRACT_ID = 'CCBK3YMI3RVGWFUREH5PZMG3HIU3L2XF6YXB2DPFQ4V42Q4JWXPGFSMB'; + const CREDIT_LINE_CONTRACT_ID = StellarSdk.StrKey.encodeContract( + Buffer.from('credit-line-test-contract-id-0000000000000000'.slice(0, 32)), + ); + const VENDOR_REGISTRY_CONTRACT_ID = StellarSdk.StrKey.encodeContract( + Buffer.from('vendor-registry-test-contract-id-0000000000000'.slice(0, 32)), + ); + const OTHER_CONTRACT_ID = StellarSdk.StrKey.encodeContract( + Buffer.from('some-unrelated-attacker-contract-id-00000000'.slice(0, 32)), + ); + const mockConfigService = { get: jest.fn((key: string) => { if (key === 'STELLAR_HORIZON_URL') return 'https://horizon-testnet.stellar.org'; if (key === 'STELLAR_NETWORK_PASSPHRASE') return StellarSdk.Networks.TESTNET; + if (key === 'LIQUIDITY_POOL_CONTRACT_ID') return LIQUIDITY_CONTRACT_ID; + if (key === 'CREDIT_LINE_CONTRACT_ID') return CREDIT_LINE_CONTRACT_ID; + if (key === 'VENDOR_REGISTRY_CONTRACT_ID') return VENDOR_REGISTRY_CONTRACT_ID; return undefined; }), }; @@ -316,6 +330,110 @@ describe('TransactionsService', () => { return tx.toXDR(); } + /** + * Builds a signed Soroban invokeHostFunction transaction with the given + * source account, contract ID, and function name (the shape the StepFi XDR + * builders produce). + */ + function buildSorobanTx( + sourceKeypair: StellarSdk.Keypair, + functionName: string, + contractId: string, + ): StellarSdk.Transaction { + const source = sourceKeypair.publicKey(); + const account = new StellarSdk.Account(source, '0'); + const tx = new StellarSdk.TransactionBuilder(account, { + fee: '100', + networkPassphrase: StellarSdk.Networks.TESTNET, + }) + .addOperation( + new StellarSdk.Contract(contractId).call( + functionName, + StellarSdk.nativeToScVal(source, { type: 'string' }), + StellarSdk.nativeToScVal(100, { type: 'i128' }), + ), + ) + .setTimeout(30) + .build(); + tx.sign(sourceKeypair); + return tx; + } + + function buildSorobanXdr( + sourceKeypair: StellarSdk.Keypair, + functionName: string, + contractId: string, + ): string { + return buildSorobanTx(sourceKeypair, functionName, contractId).toXDR(); + } + + function buildFeeBumpSorobanXdr( + innerSourceKeypair: StellarSdk.Keypair, + functionName: string, + contractId: string, + ): string { + const feeKeypair = StellarSdk.Keypair.random(); + const inner = buildSorobanTx(innerSourceKeypair, functionName, contractId); + const feeBump = StellarSdk.TransactionBuilder.buildFeeBumpTransaction( + feeKeypair.publicKey(), + StellarSdk.BASE_FEE, + inner, + StellarSdk.Networks.TESTNET, + ); + feeBump.sign(feeKeypair); + return feeBump.toXDR(); + } + + /** + * Computes the transaction hash (hex) exactly as the service does. + */ + function hashOfXdr(xdr: string): string { + return StellarSdk.TransactionBuilder.fromXDR(xdr, StellarSdk.Networks.TESTNET) + .hash() + .toString('hex'); + } + + /** + * Builds a fake parsed transaction whose invokeHostFunction operation carries + * the given Soroban auth addresses — used to exercise the wallet-authorizes + * path where the source account differs from the authenticated wallet. + */ + function buildFakeSorobanTransaction(opts: { + source: string; + functionName: string; + contractId: string; + authAddresses?: string[]; + hash?: string; + }) { + const auth = (opts.authAddresses ?? []).map((address) => ({ + credentials: () => ({ + switch: () => ({ name: 'sorobanCredentialsAddress' }), + value: () => ({ + address: () => ({ + address: () => ({ toString: () => address }), + }), + }), + }), + })); + const operation = { + type: 'invokeHostFunction', + func: { + _value: { + _attributes: { + functionName: { toString: () => opts.functionName }, + contractAddress: Buffer.from(StellarSdk.StrKey.decodeContract(opts.contractId)), + auth, + }, + }, + }, + }; + return { + source: opts.source, + operations: [operation], + hash: () => Buffer.from(opts.hash ?? 'b'.repeat(64), 'hex'), + }; + } + function buildHorizonResultCodesError(transaction: string, operations: string[] = []): unknown { return { response: { data: { extras: { result_codes: { transaction, operations } } } }, @@ -328,66 +446,287 @@ describe('TransactionsService', () => { // ══════════════════════════════════════════════════════════════════════════ describe('submitTransaction', () => { - it('returns pending status and the transaction hash on a successful Horizon submission', async () => { - mockSubmitTransaction.mockResolvedValue({ hash: validHash }); + let walletKeypair: StellarSdk.Keypair; + let wallet: string; - const result = await service.submitTransaction(validWallet, { - xdr: buildValidXdr(), + beforeEach(() => { + walletKeypair = StellarSdk.Keypair.random(); + wallet = walletKeypair.publicKey(); + // The pre-insert idempotency lookup misses by default. + mockDbLookup(null); + }); + + it('submits a valid Soroban deposit from the wallet source account', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + const expectedHash = hashOfXdr(xdr); + mockSubmitTransaction.mockResolvedValue({ hash: expectedHash }); + + const result = await service.submitTransaction(wallet, { + xdr, type: 'deposit' as TransactionType, }); - expect(result).toEqual({ transactionHash: validHash, status: 'pending' }); + expect(result).toEqual({ transactionHash: expectedHash, status: 'pending' }); expect(mockSubmitTransaction).toHaveBeenCalledTimes(1); + expect(mockSupabaseTable.insert).toHaveBeenCalledTimes(1); }); it('throws BadRequestException with TRANSACTION_INVALID_XDR when XDR is malformed', async () => { await expect( - service.submitTransaction(validWallet, { xdr: 'not-valid-xdr', type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr: 'not-valid-xdr', type: 'deposit' as TransactionType }), ).rejects.toThrow(BadRequestException); await expect( - service.submitTransaction(validWallet, { xdr: 'not-valid-xdr', type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr: 'not-valid-xdr', type: 'deposit' as TransactionType }), ).rejects.toMatchObject({ response: { code: 'TRANSACTION_INVALID_XDR' } }); }); + it('rejects a classic (non-Soroban) transaction with TRANSACTION_OPERATION_NOT_ALLOWED', async () => { + await expect( + service.submitTransaction(wallet, { + xdr: buildValidXdr(), + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_OPERATION_NOT_ALLOWED' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('rejects a transaction whose invoked function does not match the declared type', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'withdraw', LIQUIDITY_CONTRACT_ID); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_TYPE_MISMATCH' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('rejects a transaction targeting a contract other than the configured one for the type', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', OTHER_CONTRACT_ID); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_TYPE_MISMATCH' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('rejects third-party XDR where the wallet is neither source nor authorizer', async () => { + const attackerKeypair = StellarSdk.Keypair.random(); + const xdr = buildSorobanXdr(attackerKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_SOURCE_MISMATCH' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('accepts XDR whose source differs from the wallet when the wallet authorizes via Soroban auth', async () => { + const fakeTx = buildFakeSorobanTransaction({ + source: StellarSdk.Keypair.random().publicKey(), + functionName: 'deposit', + contractId: LIQUIDITY_CONTRACT_ID, + authAddresses: [wallet], + }); + const fromXdrSpy = jest + .spyOn(StellarSdk.TransactionBuilder, 'fromXDR') + .mockReturnValue(fakeTx as any); + mockSubmitTransaction.mockResolvedValue({ hash: 'b'.repeat(64) }); + + try { + const result = await service.submitTransaction(wallet, { + xdr: 'AAAA', + type: 'deposit' as TransactionType, + }); + expect(result.status).toBe('pending'); + expect(mockSubmitTransaction).toHaveBeenCalledTimes(1); + } finally { + fromXdrSpy.mockRestore(); + } + }); + + it('rejects XDR where the wallet is not in the Soroban auth either', async () => { + const fakeTx = buildFakeSorobanTransaction({ + source: StellarSdk.Keypair.random().publicKey(), + functionName: 'deposit', + contractId: LIQUIDITY_CONTRACT_ID, + authAddresses: [StellarSdk.Keypair.random().publicKey()], + }); + const fromXdrSpy = jest + .spyOn(StellarSdk.TransactionBuilder, 'fromXDR') + .mockReturnValue(fakeTx as any); + + try { + await expect( + service.submitTransaction(wallet, { + xdr: 'AAAA', + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_SOURCE_MISMATCH' } }); + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + } finally { + fromXdrSpy.mockRestore(); + } + }); + + it('accepts a fee-bump transaction whose inner source is the wallet', async () => { + const xdr = buildFeeBumpSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + const expectedHash = hashOfXdr(xdr); + mockSubmitTransaction.mockResolvedValue({ hash: expectedHash }); + + const result = await service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }); + + expect(result).toEqual({ transactionHash: expectedHash, status: 'pending' }); + expect(mockSubmitTransaction).toHaveBeenCalledTimes(1); + }); + + it('rejects a fee-bump transaction whose inner source is not the wallet', async () => { + const attackerKeypair = StellarSdk.Keypair.random(); + const xdr = buildFeeBumpSorobanXdr(attackerKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_SOURCE_MISMATCH' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('returns the existing record without re-submitting when the hash was already recorded', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + const expectedHash = hashOfXdr(xdr); + mockDbLookup({ + hash: expectedHash, + type: 'deposit' as TransactionType, + status: 'success', + submitted_at: '2026-03-23T05:15:00.000Z', + }); + + const result = await service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }); + + expect(result).toEqual({ + transactionHash: expectedHash, + status: 'success', + duplicate: true, + }); + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + expect(mockSupabaseTable.insert).not.toHaveBeenCalled(); + }); + + it('returns the existing record when a concurrent duplicate hits the unique constraint', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + const expectedHash = hashOfXdr(xdr); + + mockSupabaseTable.select.mockReturnThis(); + mockSupabaseTable.eq.mockReturnThis(); + // Pre-check queries both hash columns and misses; the post-race lookup + // (after the unique-violation insert error) finds the existing record. + mockSupabaseTable.maybeSingle + .mockResolvedValueOnce({ data: null, error: null }) + .mockResolvedValueOnce({ data: null, error: null }) + .mockResolvedValue({ + data: { + hash: expectedHash, + type: 'deposit' as TransactionType, + status: 'pending', + submitted_at: '2026-03-23T05:15:00.000Z', + }, + error: null, + }); + mockSupabaseTable.insert.mockRejectedValueOnce({ + code: '23505', + message: 'duplicate key value violates unique constraint "transactions_transaction_hash_unique"', + }); + + const result = await service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }); + + expect(result).toEqual({ + transactionHash: expectedHash, + status: 'pending', + duplicate: true, + }); + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); + + it('surfaces persistence failures instead of silently dropping them', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + mockSupabaseTable.insert.mockRejectedValueOnce({ + message: 'connection refused', + }); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_PERSISTENCE_FAILED' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + }); it('throws BadRequestException mapped from a known tx-level result code (tx_bad_auth)', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); mockSubmitTransaction.mockRejectedValue( buildHorizonResultCodesError('tx_bad_auth'), ); await expect( - service.submitTransaction(validWallet, { xdr: buildValidXdr(), type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), ).rejects.toMatchObject({ response: { code: 'STELLAR_TX_BAD_AUTH' }, }); }); it('throws BadRequestException with STELLAR_TRANSACTION_FAILED for an unmapped result code', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); mockSubmitTransaction.mockRejectedValue( buildHorizonResultCodesError('tx_some_unknown_code'), ); await expect( - service.submitTransaction(validWallet, { xdr: buildValidXdr(), type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), ).rejects.toMatchObject({ response: { code: 'STELLAR_TRANSACTION_FAILED' }, }); }); it('throws ServiceUnavailableException when Horizon submission times out', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); mockSubmitTransaction.mockRejectedValue(new Error('network timeout')); await expect( - service.submitTransaction(validWallet, { xdr: buildValidXdr(), type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), ).rejects.toThrow(ServiceUnavailableException); }); it('throws InternalServerErrorException for an unexpected Horizon submission error', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); mockSubmitTransaction.mockRejectedValue(new Error('something unexpected')); await expect( - service.submitTransaction(validWallet, { xdr: buildValidXdr(), type: 'deposit' as TransactionType }), + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), ).rejects.toThrow(InternalServerErrorException); }); }); diff --git a/test/unit/modules/transactions/wallet-throttler.guard.spec.ts b/test/unit/modules/transactions/wallet-throttler.guard.spec.ts new file mode 100644 index 0000000..6fccfed --- /dev/null +++ b/test/unit/modules/transactions/wallet-throttler.guard.spec.ts @@ -0,0 +1,40 @@ +import { WalletThrottlerGuard } from '../../../../src/modules/transactions/wallet-throttler.guard'; + +describe('WalletThrottlerGuard', () => { + function createGuard(): WalletThrottlerGuard { + const storageService = { + increment: jest.fn(), + getRecord: jest.fn(), + }; + const options = [{ ttl: 60000, limit: 10 }]; + const reflector = {}; + return new (WalletThrottlerGuard as any)(options, storageService, reflector); + } + + function getTrackerOf(guard: WalletThrottlerGuard, req: unknown): Promise { + return (guard as unknown as { + getTracker: (request: unknown) => Promise; + }).getTracker(req); + } + + it('keys the rate limit on the authenticated wallet when present', async () => { + const guard = createGuard(); + const wallet = 'GABCDEFGHIJKLMNOPQRSTUVWXYZ234567ABCDEFGHIJKLMNOPQRSTUVW'; + + await expect(getTrackerOf(guard, { user: { wallet } })).resolves.toBe(`wallet:${wallet}`); + }); + + it('falls back to the IP-based tracker when no user is present', async () => { + const guard = createGuard(); + + await expect(getTrackerOf(guard, { ip: '203.0.113.7' })).resolves.toBe('203.0.113.7'); + }); + + it('falls back to the IP-based tracker when the user object has no wallet', async () => { + const guard = createGuard(); + + await expect(getTrackerOf(guard, { ip: '198.51.100.9', user: {} })).resolves.toBe( + '198.51.100.9', + ); + }); +}); From e86fd409569421bda4ff17abb1f5ea85347f4846 Mon Sep 17 00:00:00 2001 From: Marvelous Oni Date: Thu, 27 Aug 2026 03:02:12 +0000 Subject: [PATCH 2/2] fix: fail closed on contract allowlist and mark records failed on Horizon rejection (#117) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #117 Addresses the audit gaps on the secured submit endpoint: the per-type contract check no longer degrades to function-name-only matching when the contract ID is unset or unextractable, and persisted records are marked failed when Horizon rejects the transaction instead of lingering as stale pending rows. Also resolves the committed merge-conflict markers in the progress tracker. 🤖 Generated with Codebuff Co-Authored-By: Codebuff --- context/progress-tracker.md | 67 ++++++++- .../dto/submit-transaction-request.dto.ts | 11 +- .../transactions/transactions.controller.ts | 4 +- .../transactions/transactions.service.ts | 127 ++++++++++++++++-- .../transactions/transactions.service.spec.ts | 105 ++++++++++++++- 5 files changed, 286 insertions(+), 28 deletions(-) diff --git a/context/progress-tracker.md b/context/progress-tracker.md index e7e6363..bb527c7 100644 --- a/context/progress-tracker.md +++ b/context/progress-tracker.md @@ -6,6 +6,66 @@ pure chore/docs commits). Direct pushes to main must also be logged here. --- +## 2026-08-27 + +- Closed the audit gaps on `POST /transactions/submit` (#117): + - **Fail-closed contract allowlist** — the contract ID for the declared + type must be configured and the XDR must target it. Function-name-only + matching was removed: an unset contract ID now rejects with + `TRANSACTION_CONTRACT_NOT_CONFIGURED`, and an invocation whose target + contract cannot be determined from the XDR rejects with + `TRANSACTION_TYPE_MISMATCH` instead of silently skipping the check. + - **No stale pending rows on submission failure** — when Horizon rejects + the transaction (or submission fails unexpectedly), the persisted record + is marked `failed` with the mapped error message and `completed_at`, so + the row no longer lingers as `pending` attributable to the submitting + wallet. Transient network unavailability (503) leaves the row `pending` + for the status checker to reconcile, since the transaction may still be + in flight. + - Resolved the committed merge-conflict markers in this file (stale + StepFi-Contracts content from the wrong repo removed; StepFi-API history + retained). + +## 2026-08-26 + +- Fixed registration race conditions in `AuthService.register()` by eliminating application-side pre-checks (`findByWallet`, `checkUsernameExists`) and relying directly on DB-level UNIQUE constraints (`users.wallet_address`, `users.username`). +- Added idempotent migration `20260826130000_ensure_users_unique_constraints.sql` to ensure unique indexes exist on `users.wallet_address` and `users.username`. +- Updated `UsersRepository.createProfile()` to catch PostgreSQL unique constraint violation error `23505` and map to structured 409 `ConflictException` (`AUTH_WALLET_EXISTS`, `AUTH_USERNAME_TAKEN`). +- Added cleanup handlers (`deleteAvatar`, `deleteUserById`) in `AuthService.register()` and `UsersRepository` to ensure failed registrations do not leave orphaned avatar files or partial user records. + +## 2026-08-25 + +- Secured `POST /transactions/submit` (#117): + - **Source binding** — the authenticated wallet must be the transaction + source account (or the inner source for fee-bump transactions), or must + appear as an authorized address in the Soroban invocation auth. Third-party + XDR where the wallet is neither source nor authorizer is rejected with + `TRANSACTION_SOURCE_MISMATCH`. (Deposit/withdraw/repay/vendor XDRs built + by this API use a random source account and authorize via Soroban auth, so + the auth check keeps those flows working.) + - **Operation allowlist per type** — every operation must be a Soroban + `invokeHostFunction` whose function name matches the declared type + (`create_loan`, `repay_loan`/`repay_installment`, `deposit`, `withdraw`, + `approve_vendor`, `suspend_vendor`) and must target the contract owned by + that flow. Rejections use `TRANSACTION_TYPE_MISMATCH` / + `TRANSACTION_OPERATION_NOT_ALLOWED`. + - **Idempotency** — migration + `20260825000001_add_unique_transaction_hash.sql` adds partial unique + indexes on `transaction_hash` and `hash` (with pre-existing row dedupe); + the service checks for an existing record before submitting and returns it + (`duplicate: true`) instead of re-submitting, with the unique-constraint + violation as the concurrency backstop. + - **Rate limits** — `WalletThrottlerGuard` keys `@nestjs/throttler` on the + authenticated wallet; the submit route is limited to 10 req / 60 s per + wallet AND per IP (global guard), matching the auth-endpoint pattern. + - **Persistence-first** — the local record is written (await) before the + Horizon submission, so persistence failures surface as + `TRANSACTION_PERSISTENCE_FAILED` instead of being silently dropped, and + the transaction hash is always known to the status checker / indexer. + - Updated `SubmitTransactionResponseDto` (`status` may reflect the recorded + status, plus `duplicate` flag), controller Swagger, and unit tests covering + every rejection branch plus the happy path. + ## 2026-08-24 - **Session families + refresh-token replay detection** (`sessions.family_id` @@ -27,13 +87,6 @@ pure chore/docs commits). Direct pushes to main must also be logged here. event, blocked-user denial within TTL bound, cache expiry re-query, cleanup job deletes-only-expired. -## 2026-08-26 - -- Fixed registration race conditions in `AuthService.register()` by eliminating application-side pre-checks (`findByWallet`, `checkUsernameExists`) and relying directly on DB-level UNIQUE constraints (`users.wallet_address`, `users.username`). -- Added idempotent migration `20260826130000_ensure_users_unique_constraints.sql` to ensure unique indexes exist on `users.wallet_address` and `users.username`. -- Updated `UsersRepository.createProfile()` to catch PostgreSQL unique constraint violation error `23505` and map to structured 409 `ConflictException` (`AUTH_WALLET_EXISTS`, `AUTH_USERNAME_TAKEN`). -- Added cleanup handlers (`deleteAvatar`, `deleteUserById`) in `AuthService.register()` and `UsersRepository` to ensure failed registrations do not leave orphaned avatar files or partial user records. - ## 2026-07-23 - Added GitHub Actions health check workflow (`health-check.yml`) to ping the Render API every 6 hours to prevent the free tier instance from sleeping. Auto-creates or comments on issues with the `incident` label if the ping fails, preventing silent outages. diff --git a/src/modules/transactions/dto/submit-transaction-request.dto.ts b/src/modules/transactions/dto/submit-transaction-request.dto.ts index 453b1e9..a05c3cd 100644 --- a/src/modules/transactions/dto/submit-transaction-request.dto.ts +++ b/src/modules/transactions/dto/submit-transaction-request.dto.ts @@ -23,12 +23,21 @@ export enum TransactionType { * - The declared `type` must match the operations contained in the XDR * (e.g. `deposit` must be a Soroban `deposit` invocation on the liquidity * pool contract). Mismatches are rejected with `TRANSACTION_TYPE_MISMATCH` - * or `TRANSACTION_OPERATION_NOT_ALLOWED`. + * or `TRANSACTION_OPERATION_NOT_ALLOWED`. The allowlist is fail-closed on + * the contract ID: when the contract for the flow is not configured on the + * server the submission is rejected with `TRANSACTION_CONTRACT_NOT_CONFIGURED` + * (never falling back to function-name-only matching), and an invocation + * whose target contract cannot be determined from the XDR is rejected with + * `TRANSACTION_TYPE_MISMATCH`. * - Submission is idempotent per transaction hash: re-submitting an already * recorded hash returns the original record without a second Horizon * submission (`duplicate: true` on the response). * - The local record is persisted before the Horizon submission, so * persistence failures surface as errors rather than being silently dropped. + * When Horizon rejects the transaction (or submission fails unexpectedly), + * the persisted record is marked `failed` with the mapped error message; + * transient network unavailability leaves it `pending` for the status + * checker to reconcile. */ export class SubmitTransactionRequestDto { @ApiProperty({ diff --git a/src/modules/transactions/transactions.controller.ts b/src/modules/transactions/transactions.controller.ts index 59b10cc..eb5484f 100644 --- a/src/modules/transactions/transactions.controller.ts +++ b/src/modules/transactions/transactions.controller.ts @@ -57,8 +57,8 @@ export class TransactionsController { }) @ApiResponse({ status: 401, description: 'Unauthorized - missing or invalid JWT' }) @ApiResponse({ status: 429, description: 'Too many requests - rate limit exceeded (per wallet or per IP)' }) - @ApiResponse({ status: 500, description: 'Failed to persist the transaction record locally (TRANSACTION_PERSISTENCE_FAILED)' }) - @ApiResponse({ status: 503, description: 'Stellar network temporarily unavailable' }) + @ApiResponse({ status: 500, description: 'Failed to persist the transaction record locally (TRANSACTION_PERSISTENCE_FAILED) or an unexpected Stellar submission failure (STELLAR_SUBMISSION_FAILED)' }) + @ApiResponse({ status: 503, description: 'Stellar network temporarily unavailable, or the contract for the declared type is not configured on the server (TRANSACTION_CONTRACT_NOT_CONFIGURED)' }) async submitTransaction( @CurrentUser() user: { wallet: string }, @Body() dto: SubmitTransactionRequestDto, diff --git a/src/modules/transactions/transactions.service.ts b/src/modules/transactions/transactions.service.ts index d1aaace..986b57f 100644 --- a/src/modules/transactions/transactions.service.ts +++ b/src/modules/transactions/transactions.service.ts @@ -165,8 +165,9 @@ export class TransactionsService { // 4. Persist first so persistence failures surface instead of being // silently dropped. The unique hash indexes backstop the check above // against concurrent duplicate submissions. + let lookupColumn: TransactionLookupColumn | null = null; try { - await this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr); + lookupColumn = await this.persistTransactionRecord(wallet, transactionHash, dto.type, dto.xdr); } catch (error) { if (this.isUniqueViolationError(error)) { const existingAfterRace = await this.findTransactionRecord(transactionHash); @@ -187,9 +188,14 @@ export class TransactionsService { // 5. Submit to Horizon only after the local record exists, so the // transaction hash is always known to the status checker / indexer. + // When Horizon rejects the transaction (or submission fails + // unexpectedly), the persisted record is marked failed so it does not + // linger as a stale `pending` row attributable to the submitting + // wallet. try { await this.horizonServer.submitTransaction(transaction); } catch (error) { + await this.markTransactionFailed(lookupColumn, transactionHash, error); this.handleHorizonError(error); } @@ -291,8 +297,29 @@ export class TransactionsService { }); } + // Fail closed on the contract ID: the allowlist never degrades to + // function-name-only matching. If the contract for this flow is not + // configured, submissions of this type are disabled rather than + // accepting invocations of attacker-deployed contracts whose functions + // share StepFi names. const expectedContractId = this.configService.get(allowlist.contractIdKey); - if (expectedContractId && invocation.contractId && invocation.contractId !== expectedContractId.trim()) { + if (!expectedContractId) { + throw new ServiceUnavailableException({ + code: 'TRANSACTION_CONTRACT_NOT_CONFIGURED', + message: `The contract for transaction type '${type}' is not configured on the server. Submission is disabled until the contract ID is set.`, + }); + } + + // The target contract must be determinable from the XDR — a matching + // function name is not enough. + if (!invocation.contractId) { + throw new BadRequestException({ + code: 'TRANSACTION_TYPE_MISMATCH', + message: `Could not determine the target contract address for transaction type '${type}'.`, + }); + } + + if (invocation.contractId !== expectedContractId.trim()) { throw new BadRequestException({ code: 'TRANSACTION_TYPE_MISMATCH', message: `Transaction type '${type}' must invoke contract ${expectedContractId}, but the XDR targets ${invocation.contractId}.`, @@ -426,7 +453,17 @@ export class TransactionsService { return code === '23505' || message.includes('duplicate key value violates unique constraint'); } - private handleHorizonError(error: unknown): never { + /** + * Maps a Horizon submission error to the HTTP status, typed code, and + * user-facing message that should be returned to the client. Shared by + * `handleHorizonError` (which throws) and `markTransactionFailed` (which + * records the same message on the local row). + */ + private describeHorizonError(error: unknown): { + httpStatus: number; + code: string; + message: string; + } { const err = error as { response?: { data?: { extras?: { result_codes?: { transaction?: string; operations?: string[] } } } }; message?: string; @@ -441,32 +478,94 @@ export class TransactionsService { for (const code of allCodes) { if (code && HORIZON_ERROR_MAP[code]) { - throw new BadRequestException({ + return { + httpStatus: 400, code: `STELLAR_${code.toUpperCase()}`, message: HORIZON_ERROR_MAP[code], - }); + }; } } - throw new BadRequestException({ + return { + httpStatus: 400, code: 'STELLAR_TRANSACTION_FAILED', message: `Transaction rejected by the Stellar network: ${allCodes.join(', ')}`, - }); + }; } const message = err?.message ?? 'Unknown error'; if (message.toLowerCase().includes('timeout') || message.toLowerCase().includes('network')) { - throw new ServiceUnavailableException({ + return { + httpStatus: 503, code: 'STELLAR_NETWORK_UNAVAILABLE', message: 'Stellar network is temporarily unavailable. Please try again later.', - }); + }; } this.logger.error(`Horizon submission error: ${message}`); - throw new InternalServerErrorException({ + return { + httpStatus: 500, code: 'STELLAR_SUBMISSION_FAILED', message: 'Failed to submit transaction to the Stellar network. Please try again.', - }); + }; + } + + private handleHorizonError(error: unknown): never { + const details = this.describeHorizonError(error); + + if (details.httpStatus === 503) { + throw new ServiceUnavailableException({ code: details.code, message: details.message }); + } + if (details.httpStatus === 500) { + throw new InternalServerErrorException({ code: details.code, message: details.message }); + } + throw new BadRequestException({ code: details.code, message: details.message }); + } + + /** + * Best-effort update of the persisted record when Horizon rejected the + * transaction or submission failed unexpectedly, so the row does not linger + * as a stale `pending` record attributable to the submitting wallet. On + * transient network unavailability (503) the outcome is unknown — the + * transaction may still be in flight — so the row is left `pending` for the + * status checker to reconcile. Never throws: the original Horizon error is + * what surfaces to the client. + */ + private async markTransactionFailed( + lookupColumn: TransactionLookupColumn | null, + hash: string, + error: unknown, + ): Promise { + if (!lookupColumn) { + return; + } + + const details = this.describeHorizonError(error); + if (details.httpStatus === 503) { + return; + } + + try { + const failedAt = new Date().toISOString(); + const client = this.supabaseService.getServiceRoleClient(); + const { error: updateError } = await client + .from('transactions') + .update({ + status: 'failed', + error: details.message, + completed_at: failedAt, + updated_at: failedAt, + }) + .eq(lookupColumn, hash); + + if (updateError) { + this.logger.warn(`Failed to mark transaction ${hash} as failed: ${updateError.message}`); + } + } catch (persistError) { + this.logger.warn( + `Failed to mark transaction ${hash} as failed: ${(persistError as Error).message}`, + ); + } } private async persistTransactionRecord( @@ -474,7 +573,7 @@ export class TransactionsService { hash: string, type: TransactionType, xdr: string, - ): Promise { + ): Promise { const client = this.supabaseService.getServiceRoleClient(); const submittedAt = new Date().toISOString(); const transactionHashPayload: Record = { @@ -491,7 +590,7 @@ export class TransactionsService { .insert(transactionHashPayload); if (!transactionHashError) { - return; + return 'transaction_hash'; } if (!this.isUnknownColumnError(transactionHashError)) { @@ -512,6 +611,8 @@ export class TransactionsService { if (legacyHashError) { throw new Error(legacyHashError.message ?? 'Supabase insert failed'); } + + return 'hash'; } private async findTransactionRecord(hash: string): Promise { diff --git a/test/unit/modules/transactions/transactions.service.spec.ts b/test/unit/modules/transactions/transactions.service.spec.ts index 8127382..3f48494 100644 --- a/test/unit/modules/transactions/transactions.service.spec.ts +++ b/test/unit/modules/transactions/transactions.service.spec.ts @@ -404,6 +404,7 @@ describe('TransactionsService', () => { contractId: string; authAddresses?: string[]; hash?: string; + omitContractId?: boolean; }) { const auth = (opts.authAddresses ?? []).map((address) => ({ credentials: () => ({ @@ -415,15 +416,20 @@ describe('TransactionsService', () => { }), }), })); + const attributes: Record = { + functionName: { toString: () => opts.functionName }, + auth, + }; + if (!opts.omitContractId) { + attributes.contractAddress = Buffer.from( + StellarSdk.StrKey.decodeContract(opts.contractId), + ); + } const operation = { type: 'invokeHostFunction', func: { _value: { - _attributes: { - functionName: { toString: () => opts.functionName }, - contractAddress: Buffer.from(StellarSdk.StrKey.decodeContract(opts.contractId)), - auth, - }, + _attributes: attributes, }, }, }; @@ -518,6 +524,47 @@ describe('TransactionsService', () => { expect(mockSubmitTransaction).not.toHaveBeenCalled(); }); + it('fails closed when the contract ID for the declared type is not configured', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + // The function name and source are valid, but the allowlist must not + // degrade to function-name-only matching when the contract ID is unset. + mockConfigService.get.mockImplementationOnce(() => undefined); + + await expect( + service.submitTransaction(wallet, { + xdr, + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_CONTRACT_NOT_CONFIGURED' } }); + + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + expect(mockSupabaseTable.insert).not.toHaveBeenCalled(); + }); + + it('fails closed when the target contract address cannot be determined from the XDR', async () => { + const fakeTx = buildFakeSorobanTransaction({ + source: wallet, + functionName: 'deposit', + contractId: LIQUIDITY_CONTRACT_ID, + omitContractId: true, + }); + const fromXdrSpy = jest + .spyOn(StellarSdk.TransactionBuilder, 'fromXDR') + .mockReturnValue(fakeTx as any); + + try { + await expect( + service.submitTransaction(wallet, { + xdr: 'AAAA', + type: 'deposit' as TransactionType, + }), + ).rejects.toMatchObject({ response: { code: 'TRANSACTION_TYPE_MISMATCH' } }); + expect(mockSubmitTransaction).not.toHaveBeenCalled(); + } finally { + fromXdrSpy.mockRestore(); + } + }); + it('rejects third-party XDR where the wallet is neither source nor authorizer', async () => { const attackerKeypair = StellarSdk.Keypair.random(); const xdr = buildSorobanXdr(attackerKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); @@ -699,6 +746,54 @@ describe('TransactionsService', () => { }); }); + it('marks the persisted record as failed when Horizon rejects the transaction', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + mockSubmitTransaction.mockRejectedValue( + buildHorizonResultCodesError('tx_bad_auth'), + ); + + await expect( + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), + ).rejects.toMatchObject({ response: { code: 'STELLAR_TX_BAD_AUTH' } }); + + // The locally persisted pending row is updated to failed with the same + // message the client receives, so it does not linger as stale pending. + expect(mockSupabaseTable.update).toHaveBeenCalledWith( + expect.objectContaining({ + status: 'failed', + error: 'Invalid transaction signature. Please re-sign and try again.', + completed_at: now, + }), + ); + expect(mockSupabaseTable.update).toHaveBeenCalledTimes(1); + }); + + it('leaves the record pending when Horizon is temporarily unavailable', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + mockSubmitTransaction.mockRejectedValue(new Error('network timeout')); + + await expect( + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), + ).rejects.toThrow(ServiceUnavailableException); + + // The transaction may still be in flight — the row stays pending so the + // status checker can reconcile the truth. + expect(mockSupabaseTable.update).not.toHaveBeenCalled(); + }); + + it('marks the persisted record as failed on an unexpected Horizon submission error', async () => { + const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); + mockSubmitTransaction.mockRejectedValue(new Error('something unexpected')); + + await expect( + service.submitTransaction(wallet, { xdr, type: 'deposit' as TransactionType }), + ).rejects.toThrow(InternalServerErrorException); + + expect(mockSupabaseTable.update).toHaveBeenCalledWith( + expect.objectContaining({ status: 'failed' }), + ); + }); + it('throws BadRequestException with STELLAR_TRANSACTION_FAILED for an unmapped result code', async () => { const xdr = buildSorobanXdr(walletKeypair, 'deposit', LIQUIDITY_CONTRACT_ID); mockSubmitTransaction.mockRejectedValue(