🔒 Aisle Security Analysis

We found 2 potential security issue(s) in this PR:

1. 🟡 Cron outbound delivery can leak outputs by inheriting mutable main-session last route (fail-open recipient selection)

Description

resolveDeliveryTarget() for cron jobs can implicitly select the outbound recipient from the agent main session last* routing metadata, and it explicitly does not enable the new failClosedMainSessionLastRoute protection.

In multi-user/shared deployments (where multiple users can update the shared main session’s lastTo/lastChannel), this creates a misdelivery / information-disclosure risk:

• Input (attacker influence): an arbitrary user interacts with the agent and updates the shared main session’s lastTo/lastChannel.
• Routing decision: cron delivery resolution reads store[mainSessionKey] and allows inheriting last* fields.
• Sink: dispatchCronDelivery() uses the resolved {channel,to} to send externally (announce/direct), potentially sending cron results to the attacker’s recipient.

This is especially risky for cron jobs that omit an explicit delivery.to and rely on delivery.channel=last (the default), because the recipient becomes whichever user last touched the shared main session.

Vulnerable code:

​// defaults to main session entry and does not enable fail-closed last-route blocking
const main = threadEntry ?? store[mainSessionKey];
​// Intentionally do not set `failClosedMainSessionLastRoute` here...
const preliminary = resolveSessionDeliveryTarget({
  entry: main,
  requestedChannel,
  explicitTo,
  allowMismatchedLastTo,
  sessionKey: sessionRouteKey,
  mainKey: sessionMainKey,
});

Recommendation

Avoid inheriting last* route metadata from shared main sessions for cron external delivery.

Options (prefer combining 1 + 2):

1. Require explicit recipient for cron external delivery (recommended for external_required): if delivery.to is missing, fail the run (or skip for best-effort).

2. Enable fail-closed last-route blocking when resolving cron delivery targets, using the newly added controls:

​// compute intent before resolving target
const deliveryBestEffort = resolveCronDeliveryBestEffort(job);
const deliveryIntent: DeliveryIntent = !deliveryRequested || deliveryPlan.mode === "none"
  ? "internal_only"
  : deliveryBestEffort ? "external_preferred" : "external_required";

const failClosed = deliveryIntent !== "internal_only";

const preliminary = resolveSessionDeliveryTarget({
  entry: main,
  requestedChannel,
  explicitTo,
  allowMismatchedLastTo,
  sessionKey: sessionRouteKey,
  mainKey: sessionMainKey,
  failClosedMainSessionLastRoute: failClosed,
});

3. For best-effort delivery, consider using resolveSessionRouteDecision({ intent, ..., failClosedMainSessionLastRoute: true }) and only deliver when status === "resolved"; otherwise skip with a clear warning.

These changes prevent an untrusted user from steering cron deliveries by racing/updating the shared main session’s lastTo.

2. 🔵 Unbounded turnSource* routing metadata allows resource exhaustion and session/prompt poisoning

Description

The new turnSource* override fields (turnSourceChannel, turnSourceTo, turnSourceAccountId, turnSourceThreadId) are accepted with minimal validation and no size/character constraints.

Even though overrides are gated to node role or operator.admin scope, the gateway:

• Trims but does not enforce max length for turnSourceTo/turnSourceAccountId, allowing very large strings.
• Accepts turnSourceThreadId as string | number with no finiteness/integer bound checks, so extremely large numeric values (e.g. 1e309 → Infinity in JS) or huge strings can flow through.
• Uses the resolved values as authoritative routing metadata in MsgContext (OriginatingTo, AccountId, MessageThreadId), which is subsequently used by session state and prompt building (e.g., included in the “trusted metadata” JSON block via buildInboundMetaSystemPrompt). This can bloat session storage and model prompts and can destabilize downstream delivery logic.

Vulnerable code (no length/bounds validation before using attacker-provided values):

const turnSourceTo =
  allowTurnSourceOverride && typeof p.turnSourceTo === "string" && p.turnSourceTo.trim()
    ? p.turnSourceTo.trim()
    : undefined;
...
const turnSourceThreadId =
  allowTurnSourceOverride && p.turnSourceThreadId != null && p.turnSourceThreadId !== ""
    ? p.turnSourceThreadId
    : undefined;

Recommendation

Harden validation at the schema and handler layer:

1. Add max-length and character constraints in ChatSendParamsSchema for all turnSource* fields.
2. Normalize and bound turnSourceThreadId to a safe integer/string range.
3. Optionally reject control characters (CR/LF, NUL, etc.) in these metadata fields to avoid downstream log/prompt issues.

Example (TypeBox schema hardening):

const TurnSourceChannel = Type.Optional(Type.String({ maxLength: 32 }));
const TurnSourceTo = Type.Optional(Type.String({ maxLength: 256 }));
const TurnSourceAccountId = Type.Optional(Type.String({ maxLength: 128 }));
const TurnSourceThreadId = Type.Optional(
  Type.Union([
    Type.String({ maxLength: 128 }),
    Type.Integer({ minimum: 0, maximum: 9_007_199_254_740_991 }), // JS safe int
  ]),
);

Example (runtime validation in chat.send):

function assertMaxLen(name: string, v: string, max: number) {
  if (v.length > max) throw new Error(`${name} too long`);
  return v;
}

const turnSourceTo = allowTurnSourceOverride && typeof p.turnSourceTo === "string"
  ? assertMaxLen("turnSourceTo", p.turnSourceTo.trim(), 256)
  : undefined;

const turnSourceThreadId = (() => {
  if (!allowTurnSourceOverride || p.turnSourceThreadId == null || p.turnSourceThreadId === "") return undefined;
  if (typeof p.turnSourceThreadId === "number") {
    if (!Number.isFinite(p.turnSourceThreadId)) throw new Error("turnSourceThreadId must be finite");
    return Math.trunc(p.turnSourceThreadId);
  }
  return assertMaxLen("turnSourceThreadId", p.turnSourceThreadId.trim(), 128);
})();

Analyzed PR: #35451 at commit d3f0c3c

_{Last updated on: 2026-03-05T07:14:19Z}

Greptile Summary

This PR canonicalizes route derivation across chat.send and cron delivery paths by introducing shared helpers (resolveSessionDeliveryTarget extensions and the new resolveSessionRouteDecision) in src/infra/outbound/targets.ts. It replaces duplicated, divergent routing logic that caused stale and cross-channel routing behavior with a single explicit DeliveryIntent contract (internal_only / external_preferred / external_required) and a fail-closed main-alias guard.

Key changes:

• chat.send now accepts explicit turnSource* routing fields and maps the deliver boolean to a typed DeliveryIntent; deliver=true with an unresolvable external route now fails explicitly with INVALID_REQUEST rather than silently degrading.
• resolveSessionDeliveryTarget gains sessionKey, mainKey, and failClosedMainSessionLastRoute parameters; when enabled, main/shared-session aliases no longer inherit mutable last* route fields, and non-channel-scoped sessions (e.g. cron, run) are equally blocked from inheriting external routes.
• Cron announce session key resolution is refactored into a typed result union (CronAnnounceSessionRouteResolution) that eliminates the silent main-session fallback; strict mode now fails explicitly, best-effort mode degrades safely without delivery.
• AGENTS.md codifies the canonicalization contract and required parity-test coverage for future route-path changes.

One subtle ordering detail worth being aware of: when both turnSourceChannel (without turnSourceTo) and a main-alias session key with failClosedMainSessionLastRoute apply simultaneously, resolveSessionRouteDecision returns missing_turn_source_to (status "missing") rather than main_alias_blocked (status "blocked"). This is the correct and more actionable result, but is currently undocumented in the function body.

Additionally, delivery-target.ts intentionally omits failClosedMainSessionLastRoute from the cron delivery target resolveSessionDeliveryTarget calls — the fail-closed behaviour for cron is enforced downstream in delivery-dispatch.ts. A short inline comment clarifying this design decision would reduce the risk of a future contributor inadvertently adding it here and breaking cron jobs that legitimately rely on main-session route inheritance.

Confidence Score: 4/5

• Safe to merge with low risk; the refactor is well-scoped, tested, and backward-compatible, with one minor documentation gap.
• The core routing logic in resolveSessionRouteDecision and resolveSessionDeliveryTarget is correct and covered by dedicated tests for all four key scenarios (main alias fail-closed, turn-source precedence, channel-scoped inheritance, internal-only blocking). The intent contract is applied consistently across chat.send and cron paths. No security or data-access surface changes. One point deducted for the undocumented asymmetry between chat.send (fail-closed enabled) and delivery-target.ts (fail-closed not enabled), which increases the risk of future drift — the very problem this PR is trying to solve.
• src/cron/isolated-agent/delivery-target.ts — the intentional absence of failClosedMainSessionLastRoute is undocumented and could be misread as an omission in a future audit.

_{Last reviewed commit: 212ebe2}

@greptile review
@codex review

@greptile review
@codex review

Codex Review: Didn't find any major issues. Another round soon, please!

Addressed Aisle findings in commit 72af51d.

1. High: chat.send turnSource confused-deputy risk

• Added authorization gate in src/gateway/server-methods/chat.ts.
• turnSource overrides are now rejected unless caller is trusted (node role or operator.admin).
• Untrusted override attempts now return INVALID_REQUEST.
• Added regression coverage:
  • src/gateway/server-methods/chat.directive-tags.test.ts
  • src/gateway/server.chat.gateway-server-chat.test.ts

2. Low: cron announce route error leakage

• Sanitized announce route-resolution error propagation in src/cron/isolated-agent/delivery-dispatch.ts.
• Removed recipient-id/raw exception leakage from user-facing/run error paths.
• Strict mode now returns generic route-resolution failure text; best-effort logs generic message.
• Updated tests:
  • src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts

Validation:

• OPENCLAW_TEST_PROFILE=low OPENCLAW_TEST_SERIAL_GATEWAY=1 pnpm test src/gateway/server-methods/chat.directive-tags.test.ts src/cron/isolated-agent.skips-delivery-without-whatsapp-recipient-besteffortdeliver-true.test.ts src/infra/outbound/targets.test.ts
• Result: 3 files passed, 90 tests passed.

@greptile codex

I need more context about your question. Are you asking about:

• A specific file or module named "codex" in this PR?
• A coding standard or documentation guideline?
• Something else related to this route canonicalization refactor?

Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit.