mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 12:30:44 +00:00
fix(voice-call): close webhook in-flight limiter fail-open on empty remote address (#74453)
* 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 <steipete@gmail.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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<void>((resolve) => {
|
||||
releaseReads = resolve;
|
||||
});
|
||||
const unblockReads = new Promise<void>((resolve) => {
|
||||
unblockReadBodies = resolve;
|
||||
});
|
||||
const readBodySpy = vi.spyOn(
|
||||
server as unknown as {
|
||||
readBody: (req: unknown, maxBytes: number, timeoutMs?: number) => Promise<string>;
|
||||
},
|
||||
"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", () => {
|
||||
|
||||
@@ -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" };
|
||||
|
||||
@@ -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,
|
||||
},
|
||||
]);
|
||||
|
||||
Reference in New Issue
Block a user