🔒 Aisle Security Analysis

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

1. 🟡 DoS risk: status requests spawn external qmd --version process on every call (no caching/concurrency control)

Description

getMemorySearchManager() now probes QMD availability by executing qmd --version via checkQmdBinaryAvailable(...) even for purpose: "status" calls, and status managers are explicitly not cached.

This creates a resource-exhaustion vector:

• Any caller that can trigger a status check (e.g. the gateway method doctor.memory.status, which is in operator.read scope) can cause a new OS process spawn per request.
• There is no memoization, TTL, or in-flight de-duplication of the probe; concurrent/polled status calls can accumulate child processes.
• The probe uses a 5s timeout, so a slow/hung qmd binary (or misconfiguration) can tie up resources longer per request.

Vulnerable code:

const qmdCheck = await checkQmdBinaryAvailable(resolved.qmd.command, 5000, workspaceDir);

Recommendation

Avoid spawning a process per status request.

Mitigations (one or combine):

1. Cache probe results per (agentId, resolved.qmd.command) with a TTL (e.g. 30–300s), and reuse the last result for purpose: "status".
2. De-duplicate in-flight probes so concurrent calls await the same Promise.
3. Consider a cheaper probe (e.g. which/where lookup or fs.access for explicit paths) and only run --version on first use.

Example (sketch):

const QMD_PROBE_CACHE = new Map<string, { ts: number; result: QmdProbeResult; inFlight?: Promise<QmdProbeResult> }>();

async function getQmdProbe(key: string, run: () => Promise<QmdProbeResult>) {
  const now = Date.now();
  const entry = QMD_PROBE_CACHE.get(key);
  if (entry?.result && now - entry.ts < 60_000) return entry.result;
  if (entry?.inFlight) return await entry.inFlight;
  const inFlight = run().finally(() => {
    const cur = QMD_PROBE_CACHE.get(key);
    if (cur) delete cur.inFlight;
  });
  QMD_PROBE_CACHE.set(key, { ts: now, result: entry?.result ?? { available: false, error: "" } as any, inFlight });
  const result = await inFlight;
  QMD_PROBE_CACHE.set(key, { ts: Date.now(), result });
  return result;
}

2. 🟡 Untrusted search path leads to unintended binary execution in checkQmdBinaryAvailable (CWE-426)

Description

The new checkQmdBinaryAvailable() helper executes a user-supplied/bare command name via execFile() to probe availability.

On Windows (and in some PATH configurations), if the requested command is not found on PATH, resolveCliSpawnInvocation() can leave spawnInvocation.command as a bare name (e.g. "qmd"). execFile()/spawn() then falls back to OS search rules which include the current working directory. This can lead to execution of an attacker-planted qmd.exe/qmd.cmd in the process CWD when the user runs openclaw from an untrusted directory.

Key points:

• Input: command ultimately comes from config (memory.qmd.command) or callers.
• The function intends to check “on PATH”, but it may execute a same-named binary from the working directory when PATH resolution fails.
• The provided cwd is ignored for bare command names (effectiveCwd becomes undefined), which means Node uses process.cwd().

Vulnerable code:

const effectiveCwd = hasPath ? cwd : undefined;

await execFileAsync(spawnInvocation.command, spawnInvocation.argv, {
  cwd: effectiveCwd,
  shell: spawnInvocation.shell,
});

Attack scenario (Windows):

• User configures memory.backend=qmd but does not have QMD installed / not on PATH.
• User runs openclaw from an untrusted folder containing a malicious qmd.cmd/qmd.exe.
• checkQmdBinaryAvailable("qmd") executes the planted binary during “doctor/status” flows, causing local code execution with the privileges of the OpenClaw process.

Recommendation

Harden the probe so it does not execute a bare command that failed PATH resolution.

Recommended changes (Windows-focused):

1. Avoid executing when PATH resolution did not yield a path (prevents CWD hijacking):

const spawnInvocation = resolveCliSpawnInvocation({ /* ... */ });

​// If the user provided a bare name, require resolution to an explicit path.
if (!hasPath && process.platform === "win32" && !/[\\/]/.test(spawnInvocation.command)) {
  return {
    available: false,
    error: `QMD binary "${command}" not found on PATH. Please install QMD or check your configuration.`,
  };
}

2. Consider setting allowShellFallback: false for the probe (or exposing it via resolveCliSpawnInvocation) so the availability check does not rely on shell execution.

3. Optionally, resolve executables without running them by iterating PATH entries and checking fs.access() (and PATHEXT on Windows) instead of spawning.

3. 🔵 Unbounded limit in ACP listSessions can trigger large session enumeration (DoS)

Description

The ACP listSessions method forwards a client-controlled limit from params._meta to the gateway without any upper bound.

• Input (user-controlled): params._meta.limit in the ACP request
• Weak validation: readNumber only checks typeof value === "number" && Number.isFinite(value) (no min/max/integer enforcement)
• Sink: forwarded to this.gateway.request("sessions.list", { limit })
• Downstream: gateway sessions.list schema enforces only minimum: 1 for limit (no maximum), so extremely large values are accepted

Impact:

• A malicious/buggy ACP client can request an excessively large limit, causing the gateway to return all sessions from disk-backed store.
• This can create a very large JSON response and high CPU/memory usage in both gateway and ACP process, potentially leading to degraded performance or denial of service (CWE-400).

Vulnerable code:

async listSessions(params: ListSessionsRequest): Promise<ListSessionsResponse> {
  const limit = readNumber(params._meta, ["limit"]) ?? 100;
  const result = await this.gateway.request<SessionsListResult>("sessions.list", { limit });
  ​// ...
}

Recommendation

Enforce a reasonable upper bound for limit (and coerce to an integer) either at the ACP layer, the gateway layer, or both.

Example fix in src/acp/translator.ts:

const DEFAULT_LIMIT = 100;
const MAX_LIMIT = 200; // choose based on UX/perf

const raw = readNumber(params._meta, ["limit"]);
const limit = Math.min(
  MAX_LIMIT,
  Math.max(1, Math.floor(raw ?? DEFAULT_LIMIT)),
);

Additionally, update the gateway schema to reject overly large limits:

​// src/gateway/protocol/schema/sessions.ts
limit: Type.Optional(Type.Integer({ minimum: 1, maximum: 200 })),

This prevents resource exhaustion even if another caller bypasses ACP and calls sessions.list directly.

4. 🔵 Sensitive recipient target leaked via Telegram bot-not-member error message (logged/persisted)

Description

The new wrapTelegramBotNotMemberError wraps Telegram API errors by creating a new Error whose message includes the unredacted input parameter:

• params.input is ultimately the caller-provided to target (chat id / @​username / t.me link / internal prefixes), passed in createRequestWithChatNotFound({ input: to }).
• The wrapped error message is then:
  • logged through withTelegramApiErrorLogging() (which logs formatErrorMessage(err)), and
  • persisted into the outbound delivery queue as lastError (via failDelivery(..., err.message, ...)).

This can cause information disclosure of recipient identifiers (chat IDs/usernames/links) into logs and on-disk queue state, and can also enable log amplification (very long to strings) because there is no truncation/size bound.

Vulnerable code:

`Input was: ${JSON.stringify(params.input)}.`

While JSON.stringify helps avoid newline/ANSI escape log-forging (it escapes control chars), it does not address privacy (PII) or log-size concerns.

Recommendation

Avoid embedding raw request/target input directly in error messages that may be logged/persisted.

Recommended changes:

• Redact + truncate the input before adding it to the message, or omit it by default.
• Prefer structured error metadata (fields) over concatenating into the human message.

Example (redact + truncate):

function safePreview(value: string, max = 128): string {
  const truncated = value.length > max ? value.slice(0, max) + "…" : value;
  return redactSensitiveText(truncated);
}

​// ...
`Input was: ${JSON.stringify(safePreview(params.input))}.`

Or: remove Input was: from the thrown error message and attach it as non-logged diagnostic data when verbose/diagnostic flags are enabled.

Analyzed PR: #48296 at commit 311ad68

_{Last updated on: 2026-03-16T16:08:22Z}

Greptile Summary

This PR adds actionable error handling for Telegram 403 "bot is not a member" errors, following the same pattern as the existing wrapTelegramChatNotFoundError. The regex (BOT_NOT_MEMBER_RE) correctly covers all chat types without anchoring to a specific type suffix. The change is accompanied by several unrelated but clean improvements: a QMD binary availability probe before manager creation, a listSessions API stabilization for ACP SDK v0.16.x, a TDZ fix for model alias initialization, and a form coercion fix for Discord-style numeric IDs.

• src/telegram/send.ts — New BOT_NOT_MEMBER_RE and wrapTelegramBotNotMemberError() are correctly chained in createRequestWithChatNotFound; regex covers channels, supergroups, and groups.
• src/telegram/send.test.ts — The "message send to group" test uses the same "channel chat" error string as the channel test instead of a group-specific variant (e.g. "bot was kicked from the group chat"), leaving the group-specific regex path untested.
• src/memory/backend-config.ts — checkQmdBinaryAvailable() is solid; the hasPath guard intentionally prevents workspace cwd from being used for bare command names, which is a good security boundary.
• src/memory/search-manager.ts — Switches from the manager-runtime.ts re-export shim to ./manager.js directly; functionally identical since manager-runtime.ts only re-exports manager.ts.
• ui/src/ui/controllers/config/form-coerce.ts — The hasStringVariant short-circuit intentionally skips number coercion for string | number unions to prevent numeric IDs from being coerced; behavior is intentional and well-documented.

Confidence Score: 4/5

• Safe to merge; the Telegram error improvement is correct and the unrelated refactors are clean, with only a minor test coverage gap in the group scenario.
• The core Telegram change is implemented correctly with a well-scoped regex and proper chaining. The only substantive concern is that the "message send to group" test uses the wrong error string (reuses the "channel chat" error), meaning the regex is never exercised against group-specific error variants in the test suite. All other changes are low-risk refactors or additions.
• src/telegram/send.test.ts — the group test case should use a group-specific error string to actually cover that regex branch.

This is a comment left during a code review.
Path: src/telegram/send.test.ts
Line: 810-824

Comment:
**"Group" test case uses wrong error string**

The second test case is named `"message send to group"` but it throws the same error string as the channel test (`"bot is not a member of the channel chat"`), not a group-specific variant like `"bot is not a member of the group chat"` or `"bot was kicked from the group chat"`.

The `BOT_NOT_MEMBER_RE` regex is `/403:\s*Forbidden:\s*bot (?:is not a member|was kicked from the)/i` — both group-specific variants would match, but this test doesn't actually exercise them. If the regex were accidentally narrowed in a future refactor (e.g., someone adds ` of the channel` to it), this test would not catch the regression.

Consider using a group-appropriate error message to make the test actually verify coverage for the group scenario:

```suggestion
        {
          name: "message send to group",
          run: async () => {
            const chatId = "-456";
            const err = new Error("403: Forbidden: bot was kicked from the group chat");
            const sendMessage = vi.fn().mockRejectedValue(err);
            const api = { sendMessage } as unknown as {
              sendMessage: typeof sendMessage;
            };
            await expectBotNotMemberWithChatId(
              sendMessageTelegram(chatId, "hi", { token: "tok", api }),
              chatId,
            );
          },
        },
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 311ad68}

@greptile-apps 感谢审查评论！我已修复了正则表达式。

修复内容：

• 将 BOT_NOT_MEMBER_RE 从只匹配 "channel chat" 更新为匹配所有聊天类型
• 新的正则表达式：/403:\s*Forbidden:\s*bot (?:is not a member|was kicked from the)/i

现在匹配的错误消息：

• Channels: "bot is not a member of the channel chat"
• Supergroups: "bot is not a member of the supergroup chat"
• Groups: "bot is not a member of the group chat" 或 "bot was kicked from the group chat"

改进效果：

• 确保所有聊天类型的一致性错误处理
• 用户在所有场景下都能收到可操作的错误消息

请查看最新的提交 311ad686a。