diff --git a/CHANGELOG.md b/CHANGELOG.md index ca22584..dc627ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **Redis TOCTOU safety** (#71): step cache and placeholder Redis ops use call-time client resolution with try/catch helpers (`redisHGetAll` / `redisHSet` / `redisExpire`), so a disconnect or `resetRedis()` between null-check and use no longer crashes step execution. `resetRedis()` now clears the module reference before disconnecting. + ### Added - **OpenCode Zen gateway support**: set `gateway: "opencodezen"` in `configure()` and provide `OPENCODEZEN_API_KEY` to route all model requests through [OpenCode Zen](https://opencode.ai/docs/ko/zen/) (`https://opencode.ai/zen/v1`), an OpenAI-compatible gateway with 30+ curated models including Claude, Gemini, GPT, Qwen, and more. diff --git a/src/__tests__/data-cache.test.ts b/src/__tests__/data-cache.test.ts index 78e1db5..41f0238 100644 --- a/src/__tests__/data-cache.test.ts +++ b/src/__tests__/data-cache.test.ts @@ -1,7 +1,15 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; vi.mock("../redis", () => ({ - getRedis: () => ({ hgetall: vi.fn(), hset: vi.fn(), expire: vi.fn() }), + getRedis: () => ({ + hgetall: vi.fn().mockResolvedValue({}), + hset: vi.fn().mockResolvedValue("OK"), + expire: vi.fn().mockResolvedValue(1), + }), + resetRedis: vi.fn(), + redisHGetAll: vi.fn().mockResolvedValue({}), + redisHSet: vi.fn().mockResolvedValue(true), + redisExpire: vi.fn().mockResolvedValue(true), })); vi.mock("../email", () => ({ diff --git a/src/__tests__/integration/run-steps.test.ts b/src/__tests__/integration/run-steps.test.ts index 5dd590b..5df8a14 100644 --- a/src/__tests__/integration/run-steps.test.ts +++ b/src/__tests__/integration/run-steps.test.ts @@ -6,7 +6,8 @@ vi.mock("../../instrumentation", () => ({ initTelemetry: vi.fn(), })); -// Mock Redis +// Mock Redis client + safe helpers (helpers must be mocked: they close over getRedis +// inside the redis module, so overriding only the getRedis export is not enough). const mockRedis = { hgetall: vi.fn().mockResolvedValue({}), hset: vi.fn().mockResolvedValue("OK"), @@ -14,6 +15,30 @@ const mockRedis = { }; vi.mock("../../redis", () => ({ getRedis: () => mockRedis, + resetRedis: vi.fn(), + redisHGetAll: async (key: string) => { + try { + return (await mockRedis.hgetall(key)) ?? {}; + } catch { + return {}; + } + }, + redisHSet: async (key: string, data: Record) => { + try { + await mockRedis.hset(key, data); + return true; + } catch { + return false; + } + }, + redisExpire: async (key: string, seconds: number) => { + try { + await mockRedis.expire(key, seconds); + return true; + } catch { + return false; + } + }, })); // Mock AI SDK diff --git a/src/__tests__/redis.test.ts b/src/__tests__/redis.test.ts new file mode 100644 index 0000000..3220e92 --- /dev/null +++ b/src/__tests__/redis.test.ts @@ -0,0 +1,110 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +const hgetall = vi.fn(); +const hset = vi.fn(); +const expire = vi.fn(); +const disconnect = vi.fn(); + +vi.mock("ioredis", () => { + class MockRedis { + hgetall = hgetall; + hset = hset; + expire = expire; + disconnect = disconnect; + } + return { default: MockRedis }; +}); + +vi.mock("../config", () => ({ + getConfig: vi.fn(() => ({ redis: { url: "redis://localhost:6379" } })), +})); + +vi.mock("../logger", () => ({ + logger: { + warn: vi.fn(), + debug: vi.fn(), + error: vi.fn(), + info: vi.fn(), + }, +})); + +import { + getRedis, + resetRedis, + redisHGetAll, + redisHSet, + redisExpire, +} from "../redis"; +import { getConfig } from "../config"; +import { logger } from "../logger"; + +describe("redis safe helpers (TOCTOU / error resilience)", () => { + beforeEach(() => { + resetRedis(); + hgetall.mockReset(); + hset.mockReset(); + expire.mockReset(); + disconnect.mockReset(); + vi.mocked(logger.warn).mockClear(); + vi.mocked(getConfig).mockReturnValue({ + redis: { url: "redis://localhost:6379" }, + } as ReturnType); + }); + + afterEach(() => { + resetRedis(); + }); + + it("redisHGetAll returns hash data on success", async () => { + hgetall.mockResolvedValue({ locator: "#btn", action: "click" }); + const result = await redisHGetAll("step:flow:click"); + expect(result).toEqual({ locator: "#btn", action: "click" }); + expect(hgetall).toHaveBeenCalledWith("step:flow:click"); + }); + + it("redisHGetAll returns {} and does not throw when hgetall fails", async () => { + hgetall.mockRejectedValue(new Error("Connection is closed")); + await expect(redisHGetAll("step:flow:x")).resolves.toEqual({}); + expect(logger.warn).toHaveBeenCalled(); + }); + + it("redisHGetAll returns {} when Redis is not configured", async () => { + vi.mocked(getConfig).mockReturnValue({} as ReturnType); + resetRedis(); + await expect(redisHGetAll("k")).resolves.toEqual({}); + expect(hgetall).not.toHaveBeenCalled(); + }); + + it("redisHSet returns true on success and false on failure without throwing", async () => { + hset.mockResolvedValue(1); + await expect(redisHSet("k", { a: "1" })).resolves.toBe(true); + + hset.mockRejectedValue(new Error("READONLY")); + await expect(redisHSet("k", { a: "1" })).resolves.toBe(false); + expect(logger.warn).toHaveBeenCalled(); + }); + + it("redisExpire returns false on failure without throwing", async () => { + expire.mockRejectedValue(new Error("timeout")); + await expect(redisExpire("k", 60)).resolves.toBe(false); + }); + + it("resetRedis nulls the client before disconnect (no dangling module ref)", async () => { + const client = getRedis(); + expect(client).toBeTruthy(); + resetRedis(); + expect(disconnect).toHaveBeenCalled(); + hgetall.mockResolvedValue({}); + await expect(redisHGetAll("after-reset")).resolves.toEqual({}); + }); + + it("safe helpers re-resolve client after resetRedis mid-flight (TOCTOU)", async () => { + hgetall.mockImplementation(async () => { + resetRedis(); + return { ok: "1" }; + }); + await expect(redisHGetAll("k")).resolves.toEqual({ ok: "1" }); + hgetall.mockResolvedValue({}); + await expect(redisHGetAll("k2")).resolves.toEqual({}); + }); +}); \ No newline at end of file diff --git a/src/data-cache.ts b/src/data-cache.ts index dddce69..59c36c7 100644 --- a/src/data-cache.ts +++ b/src/data-cache.ts @@ -4,7 +4,7 @@ import { getConfig } from "./config"; import { extractEmailContent } from "./email"; import { GLOBAL_VALUES_TTL_SECONDS } from "./constants"; import { logger } from "./logger"; -import { getRedis } from "./redis"; +import { redisHGetAll, redisHSet, redisExpire } from "./redis"; import { Step } from "./types"; import { generatePhoneNumber } from "./utils"; @@ -125,10 +125,8 @@ function getRedisKey(executionId: string): string { export async function getGlobalValues( executionId: string, ): Promise | null> { - const redis = getRedis(); - if (!redis) return null; const key = getRedisKey(executionId); - const values = await redis.hgetall(key); + const values = await redisHGetAll(key); if (!values || Object.keys(values).length === 0) { return null; @@ -145,16 +143,14 @@ export async function saveGlobalValues( executionId: string, values: GlobalPlaceholders, ): Promise { - const redis = getRedis(); - if (!redis) return; - const key = getRedisKey(executionId); - // Save all values as a hash - await redis.hset(key, values); + // Save all values as a hash (safe helper: no TOCTOU / never throws) + const saved = await redisHSet(key, values); + if (!saved) return; // Set TTL - await redis.expire(key, GLOBAL_VALUES_TTL_SECONDS); + await redisExpire(key, GLOBAL_VALUES_TTL_SECONDS); logger.debug(`Saved global values to Redis for execution: ${executionId}`); } @@ -175,10 +171,8 @@ function getProjectDataRedisKey(projectId: string): string { * Returns an empty object if no data exists. */ export async function getProjectData(projectId: string): Promise { - const redis = getRedis(); - if (!redis) return {}; const key = getProjectDataRedisKey(projectId); - const values = await redis.hgetall(key); + const values = await redisHGetAll(key); if (!values || Object.keys(values).length === 0) { return {}; diff --git a/src/index.ts b/src/index.ts index 75ee9d4..746584f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -21,7 +21,7 @@ async function maybeWithSpan( } import { z } from "zod"; import { buildRunStepsPrompt, buildRunUserFlowPrompt } from "./prompts"; -import { getRedis } from "./redis"; +import { getRedis, redisHGetAll, redisHSet } from "./redis"; import { getAItools } from "./tools"; import { RunStepsOptions, UserFlowOptions } from "./types"; import { @@ -352,8 +352,8 @@ export const runSteps = async ({ continue; } - // First check if the step is cached on redis - const cachedStep = redis ? await redis.hgetall(`step:${userFlow}:${step.description}`) : {}; + // First check if the step is cached on redis (safe helper: no TOCTOU / never throws) + const cachedStep = await redisHGetAll(`step:${userFlow}:${step.description}`); if ( !bypassCache && @@ -532,11 +532,13 @@ export const runSteps = async ({ .flatMap((s) => s.toolCalls) .filter((tool) => ["browser_snapshot", "browser_stop"].indexOf(tool.toolName) === -1); - if (allToolCalls.length === 1 && redis) { + if (allToolCalls.length === 1) { const cacheData = getPendingCacheData(); if (cacheData) { - await redis.hset(`step:${userFlow}:${step.description}`, cacheData); - logger.debug(`Cached step action: ${step.description}`); + const cached = await redisHSet(`step:${userFlow}:${step.description}`, cacheData); + if (cached) { + logger.debug(`Cached step action: ${step.description}`); + } } } diff --git a/src/redis.ts b/src/redis.ts index a004b6b..c33a86d 100644 --- a/src/redis.ts +++ b/src/redis.ts @@ -12,6 +12,11 @@ let initialized = false; * * Lazy: the connection is opened on first call so users can call `configure()` * before any Redis-dependent code path runs. + * + * Do not hold the returned client across `await` boundaries for long-lived work. + * Prefer {@link redisHGetAll}, {@link redisHSet}, and {@link redisExpire}, which + * re-resolve the client at call time and degrade gracefully on errors (avoids + * TOCTOU races with `resetRedis()` / disconnect mid-execution). */ export function getRedis(): Redis | null { if (initialized) return client; @@ -32,7 +37,61 @@ export function getRedis(): Redis | null { /** @internal Reset the memoized client. Used for testing only. */ export function resetRedis() { - client?.disconnect(); + // Null the module ref before disconnect so concurrent getRedis()/safe helpers + // cannot observe a half-dead client mid-teardown (TOCTOU with in-flight awaits). + const prev = client; client = null; initialized = false; + prev?.disconnect(); +} + +/** + * Safe HGETALL: resolves the client at call time and never throws. + * Returns {} when Redis is unavailable, reset mid-flight, or the command fails. + */ +export async function redisHGetAll(key: string): Promise> { + const redis = getRedis(); + if (!redis) return {}; + try { + const values = await redis.hgetall(key); + return values ?? {}; + } catch (err) { + logger.warn({ err, key }, "Redis hgetall failed; treating cache as empty"); + return {}; + } } + +/** + * Safe HSET: resolves the client at call time and never throws. + * Returns true on success, false when Redis is unavailable or the command fails. + */ +export async function redisHSet( + key: string, + data: Record, +): Promise { + const redis = getRedis(); + if (!redis) return false; + try { + await redis.hset(key, data); + return true; + } catch (err) { + logger.warn({ err, key }, "Redis hset failed; cache write skipped"); + return false; + } +} + +/** + * Safe EXPIRE: resolves the client at call time and never throws. + * Returns true on success, false when Redis is unavailable or the command fails. + */ +export async function redisExpire(key: string, seconds: number): Promise { + const redis = getRedis(); + if (!redis) return false; + try { + await redis.expire(key, seconds); + return true; + } catch (err) { + logger.warn({ err, key }, "Redis expire failed; TTL not applied"); + return false; + } +} \ No newline at end of file