-
Notifications
You must be signed in to change notification settings - Fork 71
feat(payments): add double-spend validation for escrow funding requests #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import { randomUUID } from "node:crypto"; | ||
| import { Redis } from "ioredis"; | ||
| // @ts-ignore — ioredis-mock is a dev dependency used only in test/mock mode | ||
| import MockRedis from "ioredis-mock"; | ||
| import { createLogger } from "@delego/utils"; | ||
| import type { EscrowFundingLock } from "./validation.js"; | ||
|
|
||
| export type { EscrowFundingLock }; | ||
|
|
||
| const log = createLogger("payments:lock", process.env.LOG_LEVEL ?? "info"); | ||
|
|
||
| const LOCK_KEY_PREFIX = "escrow:funding:lock:"; | ||
| const DEFAULT_LOCK_TTL_MS = 30_000; | ||
|
|
||
| // Atomic check-and-delete: only removes the key when the stored token matches. | ||
| const RELEASE_SCRIPT = ` | ||
| if redis.call("GET", KEYS[1]) == ARGV[1] then | ||
| return redis.call("DEL", KEYS[1]) | ||
| else | ||
| return 0 | ||
| end | ||
| `; | ||
|
|
||
| export class LockServiceError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = "LockServiceError"; | ||
| } | ||
| } | ||
|
|
||
| let _client: Redis | null = null; | ||
|
|
||
| export function getLockRedisClient(): Redis { | ||
| if (!_client) { | ||
| const useMock = | ||
| process.env.NODE_ENV === "test" || process.env.MOCK_REDIS === "true"; | ||
|
|
||
| if (useMock) { | ||
| log.info("Using mock Redis for escrow funding lock"); | ||
| _client = new MockRedis() as unknown as Redis; | ||
| } else { | ||
| const url = process.env.REDIS_URL; | ||
| _client = url | ||
| ? new Redis(url) | ||
| : new Redis({ | ||
| host: process.env.REDIS_HOST ?? "localhost", | ||
| port: Number(process.env.REDIS_PORT ?? 6379), | ||
| }); | ||
|
|
||
| _client.on("error", (err: Error) => { | ||
| log.error("Redis connection error in escrow lock", { error: err.message }); | ||
| }); | ||
| } | ||
| } | ||
| return _client; | ||
| } | ||
|
|
||
| /** | ||
| * Acquires a Redis SET NX PX distributed lock for the given orderId. | ||
| * Returns the lock on success, or null when another request already holds the lock. | ||
| * Throws LockServiceError when Redis is unreachable. | ||
| */ | ||
| export async function acquireLock( | ||
| orderId: string | ||
| ): Promise<EscrowFundingLock | null> { | ||
| const key = `${LOCK_KEY_PREFIX}${orderId}`; | ||
| const lockToken = randomUUID(); | ||
| const ttlMs = Number( | ||
| process.env.ESCROW_FUNDING_LOCK_TTL_MS ?? DEFAULT_LOCK_TTL_MS | ||
| ); | ||
|
|
||
| let result: string | null; | ||
| try { | ||
| result = await getLockRedisClient().set(key, lockToken, "PX", ttlMs, "NX"); | ||
|
Comment on lines
+68
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift A fixed TTL can reopen the critical section before deposit finishes. This lock is acquired once and never renewed. If the protected deposit path runs longer than 🤖 Prompt for AI Agents |
||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "Unknown error"; | ||
| log.error("Failed to acquire escrow funding lock — Redis unavailable", { | ||
| orderId, | ||
| error: message, | ||
| }); | ||
| throw new LockServiceError(`Redis lock service unavailable: ${message}`); | ||
| } | ||
|
|
||
| if (result === null) { | ||
| log.warn("Escrow funding lock already held — duplicate request rejected", { | ||
| orderId, | ||
| }); | ||
| return null; | ||
| } | ||
|
|
||
| log.info("Acquired escrow funding lock", { orderId, ttlMs }); | ||
| return { orderId, lockToken, createdAt: Date.now() }; | ||
| } | ||
|
|
||
| /** | ||
| * Releases the lock using a Lua script so only the owner (matching lockToken) can delete it. | ||
| * Failures are non-fatal: TTL will expire the key automatically. | ||
| */ | ||
| export async function releaseLock( | ||
| orderId: string, | ||
| lockToken: string | ||
| ): Promise<void> { | ||
| const key = `${LOCK_KEY_PREFIX}${orderId}`; | ||
| try { | ||
| const deleted = (await getLockRedisClient().eval( | ||
| RELEASE_SCRIPT, | ||
| 1, | ||
| key, | ||
| lockToken | ||
| )) as number; | ||
| if (deleted === 1) { | ||
| log.info("Released escrow funding lock", { orderId }); | ||
| } else { | ||
| log.debug( | ||
| "Escrow funding lock not released — token mismatch or already expired", | ||
| { orderId } | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : "Unknown error"; | ||
| log.warn( | ||
| "Failed to release escrow funding lock — will expire via TTL", | ||
| { orderId, error: message } | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,12 @@ import { | |
| validateReleaseRequest, | ||
| type ValidationError, | ||
| } from "./validation.js"; | ||
| import { | ||
| acquireLock, | ||
| releaseLock, | ||
| LockServiceError, | ||
| type EscrowFundingLock, | ||
| } from "./lock.js"; | ||
|
|
||
| async function readJsonBody(req: IncomingMessage): Promise<Record<string, unknown>> { | ||
| return new Promise((resolve, reject) => { | ||
|
|
@@ -84,6 +90,7 @@ export function registerRoutes(): Route[] { | |
| }), | ||
|
|
||
| route("POST", "/escrow/deposit", async (req, res) => { | ||
| let lock: EscrowFundingLock | null = null; | ||
| try { | ||
| const idempotency = validateIdempotencyKey(req.headers as Record<string, string | string[] | undefined>, "/escrow/deposit"); | ||
| if (!idempotency.ok) { | ||
|
|
@@ -98,6 +105,22 @@ export function registerRoutes(): Route[] { | |
| } | ||
| if (!(await ensureContractConfig(res))) return; | ||
|
|
||
| // Acquire a distributed lock to prevent double-funding the same order. | ||
| if (validated.value.orderId) { | ||
| lock = await acquireLock(validated.value.orderId); | ||
| if (lock === null) { | ||
| json(res, 409, { | ||
| data: null, | ||
| error: { | ||
| code: "ESCROW_FUNDING_CONFLICT", | ||
| message: | ||
| "A funding request for this order is already in progress", | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
|
Comment on lines
+108
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
ast-grep outline apps/backend/payments/src/validation.ts --view expanded
rg -n -C3 'validateDepositRequest|orderId' apps/backend/payments/src/validation.ts apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego Length of output: 5342 🏁 Script executed: #!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego Length of output: 13642 🏁 Script executed: #!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego Length of output: 13642 🏁 Script executed: #!/bin/bash
sed -n '1,260p' apps/backend/payments/src/validation.ts
printf '\n--- routes ---\n'
sed -n '1,220p' apps/backend/payments/src/routes.tsRepository: DelegoLabs/Delego Length of output: 13642 Require a lock key for deposit requests
🤖 Prompt for AI Agents |
||
| const result = await escrowService.deposit(validated.value); | ||
| json(res, 200, { data: result, error: null }); | ||
| } catch (err) { | ||
|
|
@@ -108,7 +131,22 @@ export function registerRoutes(): Route[] { | |
| }); | ||
| return; | ||
| } | ||
| if (err instanceof LockServiceError) { | ||
| json(res, 503, { | ||
| data: null, | ||
| error: { | ||
| code: "LOCK_SERVICE_UNAVAILABLE", | ||
| message: | ||
| "Funding lock service temporarily unavailable, please retry", | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| sendOperationError(res, "ESCROW_DEPOSIT_FAILED", err); | ||
| } finally { | ||
| if (lock) { | ||
| await releaseLock(lock.orderId, lock.lockToken); | ||
| } | ||
| } | ||
| }), | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| import { describe, it, before, beforeEach } from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import { | ||
| acquireLock, | ||
| releaseLock, | ||
| getLockRedisClient, | ||
| } from "../../../apps/backend/payments/dist/src/lock.js"; | ||
|
|
||
| describe("EscrowFundingLock", () => { | ||
| before(() => { | ||
| process.env.NODE_ENV = "test"; | ||
| process.env.MOCK_REDIS = "true"; | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| const redis = getLockRedisClient(); | ||
| await redis.flushall(); | ||
| }); | ||
|
|
||
| it("acquires lock when orderId has no active funding request", async () => { | ||
| const lock = await acquireLock("order-001"); | ||
|
|
||
| assert.ok(lock !== null, "should acquire lock on first attempt"); | ||
| assert.equal(lock.orderId, "order-001"); | ||
| assert.ok( | ||
| typeof lock.lockToken === "string" && lock.lockToken.length > 0, | ||
| "lockToken must be a non-empty string" | ||
| ); | ||
| assert.ok( | ||
| typeof lock.createdAt === "number" && lock.createdAt > 0, | ||
| "createdAt must be a positive timestamp" | ||
| ); | ||
| }); | ||
|
|
||
| it("blocks a second concurrent lock request on the same orderId", async () => { | ||
| const lock1 = await acquireLock("order-concurrent"); | ||
| assert.ok(lock1 !== null, "first attempt should acquire lock"); | ||
|
|
||
| const lock2 = await acquireLock("order-concurrent"); | ||
| assert.equal(lock2, null, "second concurrent attempt must be blocked"); | ||
| }); | ||
|
|
||
| it("allows different orderIds to acquire locks independently", async () => { | ||
| const lockA = await acquireLock("order-A"); | ||
| const lockB = await acquireLock("order-B"); | ||
|
|
||
| assert.ok(lockA !== null, "order-A should acquire its own lock"); | ||
| assert.ok(lockB !== null, "order-B should acquire its own lock independently"); | ||
| }); | ||
|
|
||
| it("releases lock cleanly so next request can proceed", async () => { | ||
| const lock = await acquireLock("order-release"); | ||
| assert.ok(lock !== null, "initial acquire should succeed"); | ||
|
|
||
| await releaseLock(lock.orderId, lock.lockToken); | ||
|
|
||
| const lockAfter = await acquireLock("order-release"); | ||
| assert.ok(lockAfter !== null, "should re-acquire lock after clean release"); | ||
| }); | ||
|
|
||
| it("does not release a lock owned by a different token", async () => { | ||
| const lock = await acquireLock("order-wrong-token"); | ||
| assert.ok(lock !== null); | ||
|
|
||
| await releaseLock("order-wrong-token", "not-the-real-token"); | ||
|
|
||
| // Lock should still be held because the wrong token was provided | ||
| const attempt = await acquireLock("order-wrong-token"); | ||
| assert.equal( | ||
| attempt, | ||
| null, | ||
| "lock must still be held after wrong-token release attempt" | ||
| ); | ||
| }); | ||
|
|
||
| it("lock expires automatically after TTL allowing reacquisition", async () => { | ||
| const originalTtl = process.env.ESCROW_FUNDING_LOCK_TTL_MS; | ||
| process.env.ESCROW_FUNDING_LOCK_TTL_MS = "100"; // 100 ms for this test | ||
|
|
||
| try { | ||
| const lock = await acquireLock("order-ttl"); | ||
| assert.ok(lock !== null, "initial acquire should succeed"); | ||
|
|
||
| // Confirm lock blocks a second attempt before expiry | ||
| const blocked = await acquireLock("order-ttl"); | ||
| assert.equal(blocked, null, "lock must block while TTL is active"); | ||
|
|
||
| // Wait for TTL to expire | ||
| await new Promise((resolve) => setTimeout(resolve, 200)); | ||
|
|
||
| const lockAfterExpiry = await acquireLock("order-ttl"); | ||
| assert.ok( | ||
| lockAfterExpiry !== null, | ||
| "should re-acquire lock after TTL expiry" | ||
| ); | ||
| } finally { | ||
| if (originalTtl !== undefined) { | ||
| process.env.ESCROW_FUNDING_LOCK_TTL_MS = originalTtl; | ||
| } else { | ||
| delete process.env.ESCROW_FUNDING_LOCK_TTL_MS; | ||
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: DelegoLabs/Delego
Length of output: 18306
Avoid a top-level import of
ioredis-mock.routes.tsloads this module in production, so Node resolves the mock package at startup even whenuseMockis false. Withioredis-mockonly indevDependencies, a pruned production install will fail before serving requests. Lazy-load it inside the mock branch or move it to runtime dependencies.🤖 Prompt for AI Agents