fix(agent): route explicit channel targets per recipient#73403
fix(agent): route explicit channel targets per recipient#73403vincentkoc wants to merge 1 commit into
Conversation
🔒 Aisle Security AnalysisWe found 2 potential security issue(s) in this PR:
1. 🟡 Unbounded session key derivation from user-controlled --channel/--to can bloat sessions.json (DoS)
Description
Impact:
Vulnerable code (new behavior): const explicitChannelSessionKey =
requestedAgentId && explicitChannel && explicitTo && !requestedSessionId
? buildAgentPeerSessionKey({
channel: explicitChannel,
peerId: explicitTo,
dmScope: "per-channel-peer",
})
: undefined;While this does not appear to be used as a filesystem path, the lack of size bounds allows attacker-controlled inputs to create pathological session keys and oversized session stores. RecommendationEnforce strict validation and size limits on inputs used to derive session keys. Suggested approach:
Example: function assertBoundedToken(name: string, value: string, max = 128) {
const trimmed = value.trim();
if (!trimmed) throw new Error(`${name} must not be empty`);
if (trimmed.length > max) throw new Error(`${name} too long`);
return trimmed;
}
const explicitChannel = opts.channel ? assertBoundedToken("channel", opts.channel, 64) : undefined;
const explicitTo = opts.to ? assertBoundedToken("to", opts.to, 128) : undefined;Optionally, also add an upper bound in 2. 🟡 PII exposure: session keys embed recipient identifiers (e.g., phone numbers) and may be persisted/logged
DescriptionThe session key derivation for explicit
Vulnerable code (new behavior): const explicitChannelSessionKey =
requestedAgentId && explicitChannel && explicitTo && !requestedSessionId
? buildAgentPeerSessionKey({
agentId: storeAgentId,
mainKey,
channel: explicitChannel,
peerKind: "direct",
peerId: explicitTo,
dmScope: "per-channel-peer",
})
: undefined;RecommendationAvoid embedding raw recipient identifiers (phone numbers, handles, emails) directly in session keys. Recommended approaches:
import crypto from "node:crypto";
function peerSessionSuffix(channel: string, peerId: string) {
// Use a keyed HMAC so it cannot be reversed and is stable per deployment.
const secret = process.env.OPENCLAW_SESSION_KEY_SALT!; // provision securely
const h = crypto.createHmac("sha256", secret);
h.update(`${channel}:${peerId}`);
return h.digest("hex").slice(0, 32);
}
const explicitChannelSessionKey =
requestedAgentId && explicitChannel && explicitTo && !requestedSessionId
? `agent:${storeAgentId}:${explicitChannel}:direct:${peerSessionSuffix(explicitChannel, explicitTo)}`
: undefined;
These changes preserve per-peer isolation without leaking PII into request parameters, disk state, or logs. Analyzed PR: #73403 at commit Last updated on: 2026-04-28T08:01:09Z |
vincentkoc
added
clownfish:merge-ready
clawsweeper
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR fixes One minor guard asymmetry: Confidence Score: 4/5Safe to merge — core session-key derivation logic is correct and well-tested; only a minor code-consistency P2 noted. No P0 or P1 issues found. The fix is logically sound: --session-key and --session-id both correctly suppress the new channel-derived key path; the ?? operator ensures the right precedence even with the guard asymmetry. Tests cover the main session, the new channel-recipient path, the --session-id override, and the global-scope non-channel path. Score capped at 4 due to the one P2 style finding. src/agents/command/session.ts — minor guard asymmetry between shouldPreferExplicitChannelSession and explicitChannelSessionKey computation condition. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/command/session.ts
Line: 235-245
Comment:
**Asymmetric guard for `!requestedSessionKey`**
`shouldPreferExplicitChannelSession` (line 214) requires `!requestedSessionKey` to suppress `resolveExplicitAgentSessionKey`, but `explicitChannelSessionKey`'s guard omits that same check. When a caller provides an explicit session key together with agent, channel, and recipient, `buildAgentPeerSessionKey` is called and a channel-derived key is built — only to be silently discarded because `explicitSessionKey` wins via `??`. The behavior is correct, but the extra computation and the divergence between the two guards makes the precedence intent harder to follow. Adding `&& !requestedSessionKey` to the `explicitChannelSessionKey` ternary would align the two guards and avoid the unnecessary call.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agent): route explicit channel targe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const explicitChannelSessionKey = | ||
| requestedAgentId && explicitChannel && explicitTo && !requestedSessionId | ||
| ? buildAgentPeerSessionKey({ | ||
| agentId: storeAgentId, | ||
| mainKey, | ||
| channel: explicitChannel, | ||
| peerKind: "direct", | ||
| peerId: explicitTo, | ||
| dmScope: "per-channel-peer", | ||
| }) | ||
| : undefined; |
There was a problem hiding this comment.
!requestedSessionKey
shouldPreferExplicitChannelSession (line 214) requires !requestedSessionKey to suppress resolveExplicitAgentSessionKey, but explicitChannelSessionKey's guard omits that same check. When a caller provides an explicit session key together with agent, channel, and recipient, buildAgentPeerSessionKey is called and a channel-derived key is built — only to be silently discarded because explicitSessionKey wins via ??. The behavior is correct, but the extra computation and the divergence between the two guards makes the precedence intent harder to follow. Adding && !requestedSessionKey to the explicitChannelSessionKey ternary would align the two guards and avoid the unnecessary call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/command/session.ts
Line: 235-245
Comment:
**Asymmetric guard for `!requestedSessionKey`**
`shouldPreferExplicitChannelSession` (line 214) requires `!requestedSessionKey` to suppress `resolveExplicitAgentSessionKey`, but `explicitChannelSessionKey`'s guard omits that same check. When a caller provides an explicit session key together with agent, channel, and recipient, `buildAgentPeerSessionKey` is called and a channel-derived key is built — only to be silently discarded because `explicitSessionKey` wins via `??`. The behavior is correct, but the extra computation and the divergence between the two guards makes the precedence intent harder to follow. Adding `&& !requestedSessionKey` to the `explicitChannelSessionKey` ternary would align the two guards and avoid the unnecessary call.
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Keep this PR open. It is MEMBER-authored and has the protected Best possible solution: Keep this PR open for maintainer review. The likely path is a focused fix that threads normalized channel information through gateway and embedded agent session resolution, derives per-recipient agent session keys only for explicit deliverable channel targets, preserves What I checked:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 87172dc9fe2c. |
1 participant
Summary
openclaw agent --agent ... --channel ... --to ...instead of falling back toagent:<id>:main.--session-idtakes precedence over the implicit--agentmain-session fallback.Review notes
--session-keyauthoritative over--session-idlookup.Validation
pnpm check:changedProjectClownfish replacement details: