🔒 Aisle Security Analysis

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

1. 🟡 Information disclosure via /plugins list and /plugins show on internal (webchat) channel

Description

The new /plugins command exposes internal plugin and install metadata to non-owner senders on the internal webchat channel.

Why this is a problem

• allowInternalReadOnly explicitly bypasses the owner gate for /plugins list and /plugins show when params.command.channel is internal (webchat).
• renderJsonBlock() uses JSON.stringify() on the full plugin record and config.plugins.installs[plugin.id] (the install record) with no redaction.
• The exposed objects include multiple fields that can disclose sensitive environment details:
  • PluginRecord: source, rootDir, workspaceDir, and error (often contains file paths / stack traces / environment details).
  • PluginInstallRecord: sourcePath, installPath (local filesystem paths), and spec / resolvedSpec (may contain private registry URLs or credential-bearing URLs), plus integrity/shasum metadata.

Exposure to non-owners

• For internal gateway chat requests, the server sets CommandAuthorized: true (see gateway/server-methods/chat.ts), meaning any internal client may be treated as command-authorized.
• Because /plugins list|show bypasses rejectNonOwnerCommand() on internal channels, a non-owner webchat user can retrieve these details.

Vulnerable code:

const allowInternalReadOnly =
  (pluginsCommand.action === "list" || pluginsCommand.action === "show") &&
  isInternalMessageChannel(params.command.channel);
const nonOwner = allowInternalReadOnly ? null : rejectNonOwnerCommand(params, "/plugins");

...
text: renderJsonBlock(`🔌 Plugin "${plugin.id}"`, {
  plugin,
  install,
}),

Recommendation

Tighten authorization and/or redact sensitive fields before returning plugin/install details.

Option A (simplest/safer): require owner (or operator.admin) even for internal read-only

const allowInternalReadOnly = false; // remove bypass
​// OR require admin scope on internal channel for read-only too
const missingScope = requireGatewayClientScopeForInternalChannel(params, {
  label: "/plugins read",
  allowedScopes: ["operator.admin"],
  missingText: "❌ /plugins list|show requires operator.admin on internal channel.",
});
if (missingScope) return missingScope;

Option B: keep internal read-only but sanitize output

• For /plugins list, output only id, name, version, enabled, status.
• For /plugins show, do not include source, rootDir, workspaceDir, error, installPath, sourcePath, spec, resolvedSpec, integrity, shasum.

Example:

type SafePluginView = Pick<PluginRecord, "id"|"name"|"version"|"enabled"|"status"|"format"|"bundleFormat">;
const safe: SafePluginView = {
  id: plugin.id,
  name: plugin.name,
  version: plugin.version,
  enabled: plugin.enabled,
  status: plugin.status,
  format: plugin.format,
  bundleFormat: plugin.bundleFormat,
};
return { text: renderJsonBlock(`🔌 Plugin "${plugin.id}"`, safe) };

2. 🔵 Plugin command can hijack new built-in /plugins (/plugin) management command due to handler ordering and missing reservation

Description

The new built-in /plugins management command can be intercepted by a plugin-registered command with the same name, because plugin commands are dispatched before built-in handlers and plugins/plugin are not reserved command names.

Impact:

• A plugin can register a command named plugins or plugin, causing handlePluginCommand to execute instead of the built-in handlePluginsCommand.
• Since plugin commands can set requireAuth: false, a malicious/buggy plugin could make /plugins ... invocations run without the owner/admin checks implemented in handlePluginsCommand.
• Even when requireAuth is left at the default (true), this collision can still undermine expected authorization semantics by bypassing built-in gating (e.g., commands.plugins flag, owner-only write paths).

Vulnerable dispatch ordering (plugin commands first):

HANDLERS = [
  ​// Plugin commands are processed first, before built-in commands
  handlePluginCommand,
  ​// ...
  handleMcpCommand,
  handlePluginsCommand,
  ​// ...
];

This risk was introduced/expanded by adding a built-in /plugins command (with "/plugin" as an alias) without preventing plugins from registering those names.

Recommendation

Prevent plugins from overriding built-in management commands.

Option A (recommended): add plugin and plugins to the reserved command list used by plugin command registration.

​// src/plugins/commands.ts
const RESERVED_COMMANDS = new Set([
  ​// ...existing...
  "plugin",
  "plugins",
]);

Option B: ensure built-in command handlers run before plugin command dispatch for reserved names (e.g., special-case /plugins and /plugin in handlePluginCommand to return null so built-ins can handle them).

Also consider adding a regression test that a plugin cannot register plugins/plugin as a command name.

3. 🔵 Prototype pollution risk when persisting plugin enable/disable state using unvalidated plugin IDs

Description

The /plugins enable|disable command writes plugin state into config.plugins.entries using plugin.id as an object key without blocking prototype-pollution keys.

• plugin.id ultimately comes from untrusted plugin manifests on disk (openclaw.plugin.json) and is only validated as a non-empty string.
• The handler passes this ID directly into setPluginEnabledInConfig(...), which uses computed object keys ([resolvedId]) to build the next config.
• If a plugin declares an ID like __proto__, constructor, or prototype, this can lead to prototype-pollution style object corruption in downstream code that treats plugins.entries as a dictionary (e.g., code that assigns into a plain {} using obj[pluginId] = ...).

Vulnerable entry point (new command handler):

const next = setPluginEnabledInConfig(
  structuredClone(loaded.config),
  plugin.id,
  pluginsCommand.action === "enable",
);

Impact depends on runtime/consumers, but prototype-key IDs can:

• Corrupt in-memory config-derived dictionaries (logic bypass/DoS)
• In some build/transpile environments (e.g., object spread lowered to Object.assign), potentially become classic prototype pollution.

Because this is triggered by a config write path, the unsafe key can also be persisted to disk on initial config creation scenarios.

Recommendation

Block prototype-pollution keys for plugin IDs at all ingestion points, and defensively avoid using plain objects as maps.

1) Validate plugin IDs (manifest load + config keys):

import { isBlockedObjectKey } from "../config/prototype-keys.js";

function assertSafePluginId(id: string): void {
  if (isBlockedObjectKey(id)) {
    throw new Error(`Invalid plugin id: ${id}`);
  }
  ​// optionally enforce a conservative charset
  if (!/^[a-z0-9][a-z0-9._/-]*$/i.test(id)) {
    throw new Error(`Invalid plugin id format: ${id}`);
  }
}

2) Enforce in /plugins write path (belt-and-suspenders):

assertSafePluginId(plugin.id);

3) Use null-prototype maps for dictionaries where arbitrary external keys are stored:

const entries = Object.create(null) as Record<string, PluginEntry>;

This prevents __proto__/constructor from having special behavior even if they slip through.

Analyzed PR: #48765 at commit aa85982

_{Last updated on: 2026-03-17T07:32:57Z}

Greptile Summary

This PR adds /plugins and /plugin chat commands for plugin inspection and enablement toggling, filling a gap between the existing /mcp management surface and the CLI-only plugin management path. The implementation closely mirrors the existing /mcp command in structure — using the same auth-gate ordering (rejectUnauthorizedCommand → owner/internal-bypass → feature-flag), reading fresh state from disk via readConfigFileSnapshot(), and validating config before writes.

Key changes:

• New commands-plugins.ts handler and plugins-commands.ts parser for list, show/get, enable, disable
• commands.plugins config flag (default false) gates the feature
• /plugin (singular) registered as a text alias alongside /plugins
• operator.admin scope required for write operations on internal gateway channels
• Tests cover list/show, enable/disable lifecycle, and the admin-scope rejection

Two items worth confirming:

• The /plugin singular alias is documented as "for read flows" but the parser and registry allow it for enable/disable writes as well — if this should be read-only, enforcement needs to be added to the parser; if writes are intentional, the docs should reflect that.
• When a malformed command (e.g. /plugins enable with no name) is sent on an internal channel by a non-owner, the handler returns "owner required" rather than the parse-error hint, since allowInternalReadOnly excludes the "error" action. This matches the /mcp pattern so is likely intentional.

Confidence Score: 4/5

• Safe to merge with minor follow-up on the /plugin alias write-access question.
• The implementation is well-structured, correctly gated behind commands.plugins: false by default, and the write path (enable/disable) enforces owner + operator.admin scope. The logic mirrors the established /mcp pattern and has meaningful test coverage. The main open question is whether /plugin (singular) should truly be restricted to reads as documented — if yes, a one-line parser guard is needed; if no, the docs need updating. No security regressions or data-loss risks are evident.
• src/auto-reply/reply/plugins-commands.ts — confirm intent for the /plugin alias allowing write operations.

This is a comment left during a code review.
Path: src/auto-reply/reply/plugins-commands.ts
Line: 9-11

Comment:
**`/plugin` alias permits write operations**

The PR description and docs both state that `/plugin` (singular) is an alias "for read flows only", but the regex `\/plugins?` and the registry `textAliases: ["/plugins", "/plugin"]` allow it to execute any action including `enable` and `disable`. For example, `/plugin enable foo` will work identically to `/plugins enable foo`.

If the intent is truly to restrict `/plugin` to read operations, the parser should reject write actions when the singular form is used:

```suggestion
export function parsePluginsCommand(raw: string): PluginsCommand | null {
  const match = raw.match(/^(\/plugins?)((?:\s+.*)?)$/i);
  if (!match) {
    return null;
  }
  const isAlias = match[1].toLowerCase() === "/plugin"; // singular alias
```

Then reject `enable`/`disable` when `isAlias` is `true`. If writes via `/plugin` are intentionally allowed, the docs and PR description should say so to avoid confusion.

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

---

This is a comment left during a code review.
Path: src/auto-reply/reply/commands-plugins.ts
Line: 79-87

Comment:
**Owner check skipped for malformed commands on internal channels**

When `parsePluginsCommand` returns `{ action: "error" }` (e.g., `/plugins enable` with no plugin name), the `allowInternalReadOnly` condition evaluates to `false` (since `"error"` isn't `"list"` or `"show"`), so `rejectNonOwnerCommand` is called unconditionally. A non-owner on an internal channel sending a malformed `/plugins` command will receive "owner required" rather than the parse-error usage hint.

This mirrors the MCP command's behaviour (which has the same pattern), but it's worth noting: if surfacing parse errors to internal-channel non-owners is desired, the `allowInternalReadOnly` guard could include `"error"`:

```suggestion
  const allowInternalReadOnly =
    (pluginsCommand.action === "list" ||
      pluginsCommand.action === "show" ||
      pluginsCommand.action === "error") &&
    isInternalMessageChannel(params.command.channel);
```

Otherwise the current behaviour is acceptable from a security standpoint (don't reveal command availability to non-owners).

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

_{Last reviewed commit: aa85982}