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: found issues before merge. Reviewed July 3, 2026, 6:01 AM ET / 10:01 UTC. Summary PR surface: Source +26, Tests +87, Docs +1. Total +114 across 7 files. Reproducibility: yes. Source-level reproduction is high confidence: current Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Refresh or replace the branch on current Do we have a high-confidence way to reproduce the issue? Yes. Source-level reproduction is high confidence: current Is this the best way to solve the issue? No, not in the submitted form. The shared resolver plus Gateway/embedded caller boundary is the right fix direction, but the branch must first resolve conflicts and settle upgrade plus recipient-key privacy/resource behavior. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 36dd9ee3c3c1. Label changesLabel justifications:
Evidence reviewedPR surface: Source +26, Tests +87, Docs +1. Total +114 across 7 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
clawsweeper
Bot
added
rating: 🦐 gold shrimp
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Landed the reviewed and hardened replay in #101507 as a2a68d1. The replacement keeps this PR's core fix, adds provider-owned identity classification, binding/main-key handling, Gateway cancellation/idempotency safety, targeted SecretRef resolution, docs, and broad regression coverage. The original branch could not be updated because maintainer edits are disabled, so I am closing this PR in favor of the landed replacement. Thank you @vincentkoc for the implementation and @pingfanfan for the original focused approach. For future PRs, enabling Allow edits by maintainers lets us apply review fixups without replaying the branch. |
2 participants
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: