mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 07:20:45 +00:00
fix(gateway): prevent internal route leakage in chat.send
Synthesis of routing fixes from #35321, #34635, and #35356 for internal-client reply safety. - Require explicit `deliver: true` before inheriting any external delivery route. - Keep webchat/TUI/UI-origin traffic on internal routing by default. - Allow configured-main session inheritance only for non-Webchat/UI clients, and honor `session.mainKey`. - Add regression tests for UI no-inherit, configured-main CLI inherit, and deliver-flag behavior. Co-authored-by: alexyyyander <alexyyyander@users.noreply.github.com> Co-authored-by: Octane0411 <88922959+Octane0411@users.noreply.github.com> Co-authored-by: Linux2010 <35169750+Linux2010@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
This commit is contained in:
@@ -56,6 +56,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Routing/session duplicate suppression synthesis: align shared session delivery-context inheritance, channel-paired route-field merges, and reply-surface target matching so dmScope=main turns avoid cross-surface duplicate replies while thread-aware forwarding keeps intended routing semantics. (from #33629, #26889, #17337, #33250) Thanks @Yuandiaodiaodiao, @kevinwildenradt, @Glucksberg, and @bmendonca3.
|
||||
- Routing/legacy session route inheritance: preserve external route metadata inheritance for legacy channel session keys (`agent:<agent>:<channel>:<peer>` and `...:thread:<id>`) so `chat.send` does not incorrectly fall back to webchat when valid delivery context exists. Follow-up to #33786.
|
||||
- Routing/legacy route guard tightening: require legacy session-key channel hints to match the saved delivery channel before inheriting external routing metadata, preventing custom namespaced keys like `agent:<agent>:work:<ticket>` from inheriting stale non-webchat routes.
|
||||
- Gateway/internal client routing continuity: prevent webchat/TUI/UI turns from inheriting stale external reply routes by requiring explicit `deliver: true` for external delivery, keeping main-session external inheritance scoped to non-Webchat/UI clients, and honoring configured `session.mainKey` when identifying main-session continuity. (from #35321, #34635, #35356) Thanks @alexyyyander and @Octane0411.
|
||||
- Security/auth labels: remove token and API-key snippets from user-facing auth status labels so `/status` and `/models` do not expose credential fragments. (#33262) thanks @cu1ch3n.
|
||||
- Auth/credential semantics: align profile eligibility + probe diagnostics with SecretRef/expiry rules and harden browser download atomic writes. (#33733) thanks @joshavant.
|
||||
- Security/audit denyCommands guidance: suggest likely exact node command IDs for unknown `gateway.nodes.denyCommands` entries so ineffective denylist entries are easier to correct. (#29713) thanks @liquidhorizon88-bot.
|
||||
|
||||
@@ -4,12 +4,13 @@ import path from "node:path";
|
||||
import { CURRENT_SESSION_VERSION } from "@mariozechner/pi-coding-agent";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import type { MsgContext } from "../../auto-reply/templating.js";
|
||||
import { GATEWAY_CLIENT_CAPS } from "../protocol/client-info.js";
|
||||
import { GATEWAY_CLIENT_CAPS, GATEWAY_CLIENT_MODES } from "../protocol/client-info.js";
|
||||
import type { GatewayRequestContext } from "./types.js";
|
||||
|
||||
const mockState = vi.hoisted(() => ({
|
||||
transcriptPath: "",
|
||||
sessionId: "sess-1",
|
||||
mainSessionKey: "main",
|
||||
finalText: "[[reply_to_current]]",
|
||||
triggerAgentRunStart: false,
|
||||
agentRunId: "run-agent-1",
|
||||
@@ -31,7 +32,11 @@ vi.mock("../session-utils.js", async (importOriginal) => {
|
||||
return {
|
||||
...original,
|
||||
loadSessionEntry: (rawKey: string) => ({
|
||||
cfg: {},
|
||||
cfg: {
|
||||
session: {
|
||||
mainKey: mockState.mainSessionKey,
|
||||
},
|
||||
},
|
||||
storePath: path.join(path.dirname(mockState.transcriptPath), "sessions.json"),
|
||||
entry: {
|
||||
sessionId: mockState.sessionId,
|
||||
@@ -148,15 +153,25 @@ async function runNonStreamingChatSend(params: {
|
||||
idempotencyKey: string;
|
||||
message?: string;
|
||||
sessionKey?: string;
|
||||
deliver?: boolean;
|
||||
client?: unknown;
|
||||
expectBroadcast?: boolean;
|
||||
}) {
|
||||
const sendParams: {
|
||||
sessionKey: string;
|
||||
message: string;
|
||||
idempotencyKey: string;
|
||||
deliver?: boolean;
|
||||
} = {
|
||||
sessionKey: params.sessionKey ?? "main",
|
||||
message: params.message ?? "hello",
|
||||
idempotencyKey: params.idempotencyKey,
|
||||
};
|
||||
if (typeof params.deliver === "boolean") {
|
||||
sendParams.deliver = params.deliver;
|
||||
}
|
||||
await chatHandlers["chat.send"]({
|
||||
params: {
|
||||
sessionKey: params.sessionKey ?? "main",
|
||||
message: params.message ?? "hello",
|
||||
idempotencyKey: params.idempotencyKey,
|
||||
},
|
||||
params: sendParams,
|
||||
respond: params.respond as unknown as Parameters<
|
||||
(typeof chatHandlers)["chat.send"]
|
||||
>[0]["respond"],
|
||||
@@ -190,6 +205,7 @@ async function runNonStreamingChatSend(params: {
|
||||
describe("chat directive tag stripping for non-streaming final payloads", () => {
|
||||
afterEach(() => {
|
||||
mockState.finalText = "[[reply_to_current]]";
|
||||
mockState.mainSessionKey = "main";
|
||||
mockState.triggerAgentRunStart = false;
|
||||
mockState.agentRunId = "run-agent-1";
|
||||
mockState.sessionEntry = {};
|
||||
@@ -369,6 +385,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
respond,
|
||||
idempotencyKey: "idem-origin-routing",
|
||||
sessionKey: "agent:main:telegram:direct:6812765697",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
@@ -403,6 +420,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
respond,
|
||||
idempotencyKey: "idem-feishu-origin-routing",
|
||||
sessionKey: "agent:main:feishu:direct:ou_feishu_direct_123",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
@@ -436,6 +454,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
respond,
|
||||
idempotencyKey: "idem-per-account-channel-peer-routing",
|
||||
sessionKey: "agent:main:telegram:account-a:direct:6812765697",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
@@ -469,6 +488,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
respond,
|
||||
idempotencyKey: "idem-legacy-channel-peer-routing",
|
||||
sessionKey: "agent:main:telegram:6812765697",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
@@ -504,6 +524,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
respond,
|
||||
idempotencyKey: "idem-legacy-thread-channel-peer-routing",
|
||||
sessionKey: "agent:main:telegram:6812765697:thread:42",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
@@ -550,6 +571,90 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
);
|
||||
});
|
||||
|
||||
it("chat.send does not inherit external delivery context for UI clients on main sessions", async () => {
|
||||
createTranscriptFixture("openclaw-chat-send-main-ui-routes-");
|
||||
mockState.finalText = "ok";
|
||||
mockState.sessionEntry = {
|
||||
deliveryContext: {
|
||||
channel: "whatsapp",
|
||||
to: "whatsapp:+8613800138000",
|
||||
accountId: "default",
|
||||
},
|
||||
lastChannel: "whatsapp",
|
||||
lastTo: "whatsapp:+8613800138000",
|
||||
lastAccountId: "default",
|
||||
};
|
||||
const respond = vi.fn();
|
||||
const context = createChatContext();
|
||||
|
||||
await runNonStreamingChatSend({
|
||||
context,
|
||||
respond,
|
||||
idempotencyKey: "idem-main-ui-routes",
|
||||
client: {
|
||||
connect: {
|
||||
client: {
|
||||
mode: GATEWAY_CLIENT_MODES.UI,
|
||||
id: "openclaw-tui",
|
||||
},
|
||||
},
|
||||
} as unknown,
|
||||
sessionKey: "agent:main:main",
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
expect(mockState.lastDispatchCtx).toEqual(
|
||||
expect.objectContaining({
|
||||
OriginatingChannel: "webchat",
|
||||
OriginatingTo: undefined,
|
||||
AccountId: undefined,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("chat.send inherits external delivery context for CLI clients on configured main sessions", async () => {
|
||||
createTranscriptFixture("openclaw-chat-send-config-main-cli-routes-");
|
||||
mockState.mainSessionKey = "work";
|
||||
mockState.finalText = "ok";
|
||||
mockState.sessionEntry = {
|
||||
deliveryContext: {
|
||||
channel: "whatsapp",
|
||||
to: "whatsapp:+8613800138000",
|
||||
accountId: "default",
|
||||
},
|
||||
lastChannel: "whatsapp",
|
||||
lastTo: "whatsapp:+8613800138000",
|
||||
lastAccountId: "default",
|
||||
};
|
||||
const respond = vi.fn();
|
||||
const context = createChatContext();
|
||||
|
||||
await runNonStreamingChatSend({
|
||||
context,
|
||||
respond,
|
||||
idempotencyKey: "idem-config-main-cli-routes",
|
||||
client: {
|
||||
connect: {
|
||||
client: {
|
||||
mode: GATEWAY_CLIENT_MODES.CLI,
|
||||
id: "cli",
|
||||
},
|
||||
},
|
||||
} as unknown,
|
||||
sessionKey: "agent:main:work",
|
||||
deliver: true,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
expect(mockState.lastDispatchCtx).toEqual(
|
||||
expect.objectContaining({
|
||||
OriginatingChannel: "whatsapp",
|
||||
OriginatingTo: "whatsapp:+8613800138000",
|
||||
AccountId: "default",
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("chat.send does not inherit external delivery context for non-channel custom sessions", async () => {
|
||||
createTranscriptFixture("openclaw-chat-send-custom-no-cross-route-");
|
||||
mockState.finalText = "ok";
|
||||
@@ -584,4 +689,38 @@ describe("chat directive tag stripping for non-streaming final payloads", () =>
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("chat.send keeps replies on the internal surface when deliver is not enabled", async () => {
|
||||
createTranscriptFixture("openclaw-chat-send-no-deliver-internal-surface-");
|
||||
mockState.finalText = "ok";
|
||||
mockState.sessionEntry = {
|
||||
deliveryContext: {
|
||||
channel: "discord",
|
||||
to: "user:1234567890",
|
||||
accountId: "default",
|
||||
},
|
||||
lastChannel: "discord",
|
||||
lastTo: "user:1234567890",
|
||||
lastAccountId: "default",
|
||||
};
|
||||
const respond = vi.fn();
|
||||
const context = createChatContext();
|
||||
|
||||
await runNonStreamingChatSend({
|
||||
context,
|
||||
respond,
|
||||
idempotencyKey: "idem-no-deliver-internal-surface",
|
||||
sessionKey: "agent:main:discord:direct:1234567890",
|
||||
deliver: false,
|
||||
expectBroadcast: false,
|
||||
});
|
||||
|
||||
expect(mockState.lastDispatchCtx).toEqual(
|
||||
expect.objectContaining({
|
||||
OriginatingChannel: "webchat",
|
||||
OriginatingTo: undefined,
|
||||
AccountId: undefined,
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -17,7 +17,11 @@ import {
|
||||
stripInlineDirectiveTagsForDisplay,
|
||||
stripInlineDirectiveTagsFromMessageForDisplay,
|
||||
} from "../../utils/directive-tags.js";
|
||||
import { INTERNAL_MESSAGE_CHANNEL, normalizeMessageChannel } from "../../utils/message-channel.js";
|
||||
import {
|
||||
INTERNAL_MESSAGE_CHANNEL,
|
||||
isWebchatClient,
|
||||
normalizeMessageChannel,
|
||||
} from "../../utils/message-channel.js";
|
||||
import {
|
||||
abortChatRunById,
|
||||
abortChatRunsForSessionKey,
|
||||
@@ -28,7 +32,11 @@ import {
|
||||
} from "../chat-abort.js";
|
||||
import { type ChatImageContent, parseMessageWithAttachments } from "../chat-attachments.js";
|
||||
import { stripEnvelopeFromMessage, stripEnvelopeFromMessages } from "../chat-sanitize.js";
|
||||
import { GATEWAY_CLIENT_CAPS, hasGatewayClientCap } from "../protocol/client-info.js";
|
||||
import {
|
||||
GATEWAY_CLIENT_CAPS,
|
||||
GATEWAY_CLIENT_MODES,
|
||||
hasGatewayClientCap,
|
||||
} from "../protocol/client-info.js";
|
||||
import {
|
||||
ErrorCodes,
|
||||
errorShape,
|
||||
@@ -856,6 +864,7 @@ export const chatHandlers: GatewayRequestHandlers = {
|
||||
);
|
||||
const commandBody = injectThinking ? `/think ${p.thinking} ${parsedMessage}` : parsedMessage;
|
||||
const clientInfo = client?.connect?.client;
|
||||
const shouldDeliverExternally = p.deliver === true;
|
||||
const routeChannelCandidate = normalizeMessageChannel(
|
||||
entry?.deliveryContext?.channel ?? entry?.lastChannel,
|
||||
);
|
||||
@@ -867,11 +876,12 @@ export const chatHandlers: GatewayRequestHandlers = {
|
||||
const sessionScopeParts = (parsedSessionKey?.rest ?? sessionKey).split(":").filter(Boolean);
|
||||
const sessionScopeHead = sessionScopeParts[0];
|
||||
const sessionChannelHint = normalizeMessageChannel(sessionScopeHead);
|
||||
const normalizedSessionScopeHead = (sessionScopeHead ?? "").trim().toLowerCase();
|
||||
const sessionPeerShapeCandidates = [sessionScopeParts[1], sessionScopeParts[2]]
|
||||
.map((part) => (part ?? "").trim().toLowerCase())
|
||||
.filter(Boolean);
|
||||
const isChannelAgnosticSessionScope = CHANNEL_AGNOSTIC_SESSION_SCOPES.has(
|
||||
(sessionScopeHead ?? "").trim().toLowerCase(),
|
||||
normalizedSessionScopeHead,
|
||||
);
|
||||
const isChannelScopedSession = sessionPeerShapeCandidates.some((part) =>
|
||||
CHANNEL_SCOPED_SESSION_SHAPES.has(part),
|
||||
@@ -880,16 +890,24 @@ export const chatHandlers: GatewayRequestHandlers = {
|
||||
!isChannelScopedSession &&
|
||||
typeof sessionScopeParts[1] === "string" &&
|
||||
sessionChannelHint === routeChannelCandidate;
|
||||
// Only inherit prior external route metadata for channel-scoped sessions.
|
||||
// Channel-agnostic sessions (main, direct:<peer>, etc.) can otherwise
|
||||
// leak stale routes across surfaces.
|
||||
const clientMode = client?.connect?.client?.mode;
|
||||
const isFromWebchatClient =
|
||||
isWebchatClient(client?.connect?.client) || clientMode === GATEWAY_CLIENT_MODES.UI;
|
||||
const configuredMainKey = (cfg.session?.mainKey ?? "main").trim().toLowerCase();
|
||||
const isConfiguredMainSessionScope =
|
||||
normalizedSessionScopeHead.length > 0 && normalizedSessionScopeHead === configuredMainKey;
|
||||
// Channel-agnostic session scopes (main, direct:<peer>, etc.) can leak
|
||||
// stale routes across surfaces. Allow configured main sessions from
|
||||
// non-Webchat/UI clients (e.g., CLI, backend) to keep the last external route.
|
||||
const canInheritDeliverableRoute = Boolean(
|
||||
sessionChannelHint &&
|
||||
sessionChannelHint !== INTERNAL_MESSAGE_CHANNEL &&
|
||||
!isChannelAgnosticSessionScope &&
|
||||
(isChannelScopedSession || hasLegacyChannelPeerShape),
|
||||
((!isChannelAgnosticSessionScope &&
|
||||
(isChannelScopedSession || hasLegacyChannelPeerShape)) ||
|
||||
(isConfiguredMainSessionScope && client?.connect !== undefined && !isFromWebchatClient)),
|
||||
);
|
||||
const hasDeliverableRoute =
|
||||
shouldDeliverExternally &&
|
||||
canInheritDeliverableRoute &&
|
||||
routeChannelCandidate &&
|
||||
routeChannelCandidate !== INTERNAL_MESSAGE_CHANNEL &&
|
||||
|
||||
Reference in New Issue
Block a user