From 9b1bde2561f5b841e8e8699c1d4aae94e0a68c2a Mon Sep 17 00:00:00 2001 From: Fred blum Date: Thu, 30 Apr 2026 04:02:14 +0300 Subject: [PATCH] fix(voice-call): close webhook in-flight limiter fail-open on empty remote address (#74453) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(voice-call): close in-flight limiter fail-open on empty remote address The webhook in-flight limiter (createWebhookInFlightLimiter in src/plugin-sdk/webhook-request-guards.ts) returns true unconditionally when tryAcquire is called with an empty key — that is its by-contract fail-open path used to mean 'caller is opting out of the limiter'. The voice-call webhook handler reached that path silently: it computed 'req.socket.remoteAddress ?? ""' and passed the empty string straight into tryAcquire. Whenever req.socket.remoteAddress was absent (closed socket, edge proxy quirk), the limiter became a no-op and the request proceeded directly to readBody without any concurrency cap. Fix: when remoteAddress is missing, log a warning and fall back to a constant non-empty key ('__voice_call_no_remote__') so all such requests share one in-flight bucket instead of bypassing the limiter entirely. The bucket size stays maxInFlightPerKey (default 8), which is the right defense-in-depth posture against slow-body attacks arriving with stripped IP info. Scoped to voice-call only. Other consumers of the SDK helper (bluebubbles via openclaw/plugin-sdk/webhook-ingress) are not changed to avoid drive-by edits to plugins this PR does not own. The shared SDK contract (empty key = bypass) is left as-is and documented implicitly by the fix's comment block. The existing 8-concurrent test in webhook.test.ts continues to assert the limiter engages on the happy path; no new test added since the private handleRequest path is not unit-test exposed and the change is two-line auditable from the diff alone. * test(voice-call): cover missing webhook remote address limiter * test: align changed package sdk routing --------- Co-authored-by: Peter Steinberger --- CHANGELOG.md | 1 + extensions/voice-call/src/webhook.test.ts | 80 ++++++++++++++++++++++- extensions/voice-call/src/webhook.ts | 12 +++- test/scripts/test-projects.test.ts | 4 +- 4 files changed, 92 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 778ab081919..1029a315489 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Docs: https://docs.openclaw.ai - CLI/status: resolve read-only channel setup runtime fallback from the packaged OpenClaw dist root, so `status --all`, `status --deep`, channel, and doctor paths do not crash when an external channel plugin needs setup metadata. Fixes #74693. Thanks @giangthb. - CLI/update: scope packaged Node compile caches by OpenClaw version and install metadata, so global installs no longer reuse stale compiled chunks after package updates. Thanks @pashpashpash. +- Channels/Voice call: keep pre-auth webhook in-flight limiting active when socket remote address metadata is missing, so slow-body requests from stripped-IP proxy paths still share the fallback bucket. (#74453) Thanks @davidangularme. - Plugin SDK/testing: lazy-load TypeScript from the plugin test-contract runtime and add release checks for critical SDK contract entrypoint imports and bundle size, so published packages fail preflight before shipping ESM-incompatible or oversized contract helpers. Thanks @vincentkoc. - Channels/Microsoft Teams: treat configured `19:...@thread.tacv2` and legacy `19:...@thread.skype` team/channel IDs as already resolved during startup, avoiding false `channels unresolved` warnings while preserving Graph name lookup for display-name entries. Fixes #74683. Thanks @dseravalli. - CLI/browser: preserve parent flags while lazy-loading browser subcommands, so `openclaw browser --json open` and `openclaw browser --json tabs` keep machine-readable output after reparsing. Fixes #74574. Thanks @devintegeritsm. diff --git a/extensions/voice-call/src/webhook.test.ts b/extensions/voice-call/src/webhook.test.ts index 8d5e321a303..14efd6d1f7f 100644 --- a/extensions/voice-call/src/webhook.test.ts +++ b/extensions/voice-call/src/webhook.test.ts @@ -1,4 +1,4 @@ -import { request } from "node:http"; +import { request, type IncomingMessage } from "node:http"; import type { RealtimeTranscriptionProviderPlugin } from "openclaw/plugin-sdk/realtime-transcription"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { VoiceCallConfigSchema, type VoiceCallConfig } from "./config.js"; @@ -941,7 +941,9 @@ describe("VoiceCallWebhookServer pre-auth webhook guards", () => { if (enteredReads === 8) { releaseReads(); } - await unblockReads; + if (enteredReads <= 8) { + await unblockReads; + } return "CallSid=CA123&SpeechResult=hello"; }); @@ -967,6 +969,80 @@ describe("VoiceCallWebhookServer pre-auth webhook guards", () => { await server.stop(); } }); + + it("limits missing remote addresses with a shared fallback bucket", async () => { + const twilioProvider: VoiceCallProvider = { + ...provider, + name: "twilio", + verifyWebhook: () => ({ ok: true, verifiedRequestKey: "twilio:req:test" }), + }; + const { manager } = createManager([]); + const config = createConfig({ provider: "twilio" }); + const server = new VoiceCallWebhookServer(config, manager, twilioProvider); + const runWebhookPipeline = ( + server as unknown as { + runWebhookPipeline: ( + req: IncomingMessage, + webhookPath: string, + ) => Promise<{ statusCode: number; body: string }>; + } + ).runWebhookPipeline.bind(server); + + let enteredReads = 0; + let releaseReads!: () => void; + let unblockReadBodies!: () => void; + const enteredEightReads = new Promise((resolve) => { + releaseReads = resolve; + }); + const unblockReads = new Promise((resolve) => { + unblockReadBodies = resolve; + }); + const readBodySpy = vi.spyOn( + server as unknown as { + readBody: (req: unknown, maxBytes: number, timeoutMs?: number) => Promise; + }, + "readBody", + ); + readBodySpy.mockImplementation(async () => { + enteredReads += 1; + if (enteredReads === 8) { + releaseReads(); + } + await unblockReads; + return "CallSid=CA123&SpeechResult=hello"; + }); + + const makeRequestWithoutRemoteAddress = () => + ({ + method: "POST", + url: "/voice/webhook", + headers: { "x-twilio-signature": "sig" }, + socket: { remoteAddress: undefined }, + }) as unknown as IncomingMessage; + + try { + const inFlightRequests = Array.from({ length: 8 }, () => + runWebhookPipeline(makeRequestWithoutRemoteAddress(), "/voice/webhook"), + ); + await enteredEightReads; + + const rejected = await runWebhookPipeline( + makeRequestWithoutRemoteAddress(), + "/voice/webhook", + ); + expect(rejected.statusCode).toBe(429); + expect(rejected.body).toBe("Too Many Requests"); + expect(readBodySpy).toHaveBeenCalledTimes(8); + + unblockReadBodies(); + + const settled = await Promise.all(inFlightRequests); + expect(settled.every((response) => response.statusCode === 200)).toBe(true); + } finally { + unblockReadBodies(); + readBodySpy.mockRestore(); + } + }); }); describe("VoiceCallWebhookServer response normalization", () => { diff --git a/extensions/voice-call/src/webhook.ts b/extensions/voice-call/src/webhook.ts index 0d04dc75da0..77c9c60cdc5 100644 --- a/extensions/voice-call/src/webhook.ts +++ b/extensions/voice-call/src/webhook.ts @@ -29,6 +29,7 @@ import { startStaleCallReaper } from "./webhook/stale-call-reaper.js"; const MAX_WEBHOOK_BODY_BYTES = WEBHOOK_BODY_READ_DEFAULTS.preAuth.maxBytes; const WEBHOOK_BODY_TIMEOUT_MS = WEBHOOK_BODY_READ_DEFAULTS.preAuth.timeoutMs; +const MISSING_REMOTE_ADDRESS_IN_FLIGHT_KEY = "__voice_call_no_remote__"; const STREAM_DISCONNECT_HANGUP_GRACE_MS = 2000; const TRANSCRIPT_LOG_MAX_CHARS = 200; @@ -616,7 +617,16 @@ export class VoiceCallWebhookServer { return { statusCode: 401, body: "Unauthorized" }; } - const inFlightKey = req.socket.remoteAddress ?? ""; + // createWebhookInFlightLimiter intentionally treats an empty key as fail-open. + // Missing socket metadata must still share one bucket instead of bypassing + // the pre-auth limiter entirely. + const remoteAddress = req.socket.remoteAddress; + if (!remoteAddress) { + console.warn( + `[voice-call] Webhook accepted with no remote address; using shared fallback in-flight key`, + ); + } + const inFlightKey = remoteAddress || MISSING_REMOTE_ADDRESS_IN_FLIGHT_KEY; if (!this.webhookInFlightLimiter.tryAcquire(inFlightKey)) { console.warn(`[voice-call] Webhook rejected before body read: too many in-flight requests`); return { statusCode: 429, body: "Too Many Requests" }; diff --git a/test/scripts/test-projects.test.ts b/test/scripts/test-projects.test.ts index 985db7a79af..a2704e35d52 100644 --- a/test/scripts/test-projects.test.ts +++ b/test/scripts/test-projects.test.ts @@ -489,8 +489,8 @@ describe("scripts/test-projects changed-target routing", () => { expect(plans).toEqual([ { config: "test/vitest/vitest.unit.config.ts", - forwardedArgs: [], - includePatterns: ["packages/sdk/src/**/*.test.ts"], + forwardedArgs: ["packages/sdk/src/index.test.ts"], + includePatterns: null, watchMode: false, }, ]);