fix(channels): preserve external catalog overrides by ngutman · Pull Request #52988 · openclaw/openclaw

ngutman · 2026-03-23T15:55:18Z

Summary

Describe the problem and fix in 2–5 bullets:

Problem: shipped fallback channel metadata from bundled package manifests and dist/channel-catalog.json could take over a channel ID before external catalogs were merged.
Why it matters: catalog-driven installs could ignore a user override like a private whatsapp fork and install the hard-coded shipped npmSpec instead.
What changed: external catalogs now merge at higher precedence than shipped fallback metadata, while discovered plugins still keep the highest precedence they had before; added regressions for both cases.
What did NOT change (scope boundary): discovered plugin precedence and runtime install behavior outside catalog entry selection.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related fix(release): preserve shipped channel surfaces in npm tar #52913
This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write N/A. If the cause is unclear, write Unknown.

Root cause: shipped fallback sources were merged with bundled precedence before external catalogs, and external catalogs only appended missing IDs.
Missing detection / guardrail: there was no unit test covering external-catalog overrides for a channel that also exists in shipped fallback metadata.
Prior context (git blame, prior PR, issue, or refactor if known): this behavior came in during the fallback catalog work in fix(release): preserve shipped channel surfaces in npm tar #52913.
Why this regressed now: fix(release): preserve shipped channel surfaces in npm tar #52913 added more shipped fallback sources but kept external catalog merging as an add-only step.
If unknown, what was ruled out: N/A.

Regression Test Plan (if applicable)

For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write N/A.

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/channels/plugins/plugins-core.test.ts
Scenario the test should lock in: external catalog entries override shipped fallback metadata for the same channel ID, but discovered plugins still outrank the external catalog.
Why this is the smallest reliable guardrail: the bug is entirely in catalog merge precedence and does not require install-time integration to reproduce.
Existing test that already covers this (if any): the file already covered external catalogs and shipped fallback entries separately.
If no new test is added, why not: N/A.

User-visible / Behavior Changes

Catalog-driven channel overrides work again for channels that are also present in shipped fallback metadata.

Security Impact (required)

New permissions/capabilities? (Yes/No): No
Secrets/tokens handling changed? (Yes/No): No
New/changed network calls? (Yes/No): No
Command/tool execution surface changed? (Yes/No): No
Data access scope changed? (Yes/No): No
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: Darwin 25.3.0
Runtime/container: local workspace
Model/provider: N/A
Integration/channel (if any): channel plugin catalog / fallback WhatsApp entry
Relevant config (redacted): temporary OPENCLAW_BUNDLED_PLUGINS_DIR, OPENCLAW_PLUGIN_CATALOG_PATHS, and officialCatalogPaths

Steps

Create shipped fallback metadata for a channel ID such as whatsapp.
Provide an external catalog entry for the same channel ID with a different install.npmSpec.
Call listChannelPluginCatalogEntries() and inspect the selected entry.

Expected

The external catalog entry wins over shipped fallback metadata.
A discovered installed plugin for the same channel ID still wins over the external catalog.

Actual

Before this fix, shipped fallback metadata won and masked the external override.

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios: manual temp-dir repro of the precedence bug before the fix; pnpm check; pnpm test -- src/channels/plugins/plugins-core.test.ts after the fix.
Edge cases checked: external override against bundled metadata, external override against official fallback catalog, and discovered plugin precedence over external catalog.
What you did not verify: end-to-end channel setup flows beyond catalog resolution.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

Backward compatible? (Yes/No): Yes
Config/env changes? (Yes/No): No
Migration needed? (Yes/No): No
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly: revert commit 8dc07b87c5.
Files/config to restore: src/channels/plugins/catalog.ts and src/channels/plugins/plugins-core.test.ts.
Known bad symptoms reviewers should watch for: shipped fallback entries unexpectedly outranking discovered plugins or external catalogs after merge changes.

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

Risk: catalog precedence changes could accidentally let external catalogs outrank discovered plugins.
- Mitigation: the new regression test locks in discovered-plugin precedence separately from shipped fallback precedence.

aisle-research-bot · 2026-03-23T15:55:26Z

🔒 Aisle Security Analysis

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

#	Severity	Title
1	🔵 Low	Untrusted search path for official plugin catalog via process.execPath directory

1. 🔵 Untrusted search path for official plugin catalog via process.execPath directory

Property	Value
Severity	Low
CWE	CWE-426
Location	`src/channels/plugins/catalog.ts:155-159`

Description

resolveOfficialCatalogPaths() adds the directory of the running executable (process.execPath) as a trusted location to load an official channel-catalog.json.

Input/control point: The catalog loader derives execDir = path.dirname(process.execPath).
Trust boundary violation: If the CLI/binary is executed from (or via a wrapper/symlink located in) an attacker-writable directory, an attacker can place channel-catalog.json (or dist/channels/channel-catalog.json) adjacent to the executable.
Sink: loadCatalogEntriesFromPaths() will fs.readFileSync() and JSON.parse() any existing candidate file without authenticity/integrity verification.
Impact: A malicious catalog can redefine channel/plugin metadata (including install npmSpec/localPath), potentially steering users to install attacker-controlled plugins/packages.

Vulnerable code:

if (process.execPath) {
  const execDir = path.dirname(process.execPath);
  candidates.push(path.join(execDir, OFFICIAL_CHANNEL_CATALOG_RELATIVE_PATH));
  candidates.push(path.join(execDir, "channel-catalog.json"));
}

Recommendation

Do not treat the executable directory as a trusted source for the official catalog unless you can guarantee it is non-user-writable.

Recommended fixes (choose one):

Remove execDir-based candidates and only load the official catalog from the package installation root.
If you must support this for packaged distributions, gate it behind an explicit opt-in (flag/env var) and/or verify integrity (e.g., embedded hash/signature).

Example (opt-in via env var and path allowlist):

function resolveOfficialCatalogPaths(options: CatalogOptions): string[] {
  // ...packageRoot candidates...

  const allowExecDir = options.env?.OPENCLAW_ALLOW_EXECDIR_CATALOG === "1";
  if (allowExecDir && process.execPath) {
    const execDir = path.dirname(process.execPath);
    // optionally: ensure execDir is within an expected installation prefix
    candidates.push(path.join(execDir, OFFICIAL_CHANNEL_CATALOG_RELATIVE_PATH));
  }

  return dedupe(candidates);
}

If the catalog can influence install behavior, consider signing the catalog and verifying the signature before parsing/using it.

Analyzed PR: #52988 at commit 13ca9a3

_{Last updated on: 2026-03-23T16:28:48Z}

greptile-apps · 2026-03-23T15:57:59Z

Greptile Summary

This PR fixes a catalog merge precedence bug introduced in #52913 where shipped fallback metadata (bundled package.json entries and channel-catalog.json) could win over user-provided external catalog overrides for the same channel ID. The fix promotes both fallback sources to a new FALLBACK_CATALOG_PRIORITY (numeric value 4) and gives external catalogs a priority of EXTERNAL_CATALOG_PRIORITY (3) with proper override-or-skip semantics, so external entries now correctly beat shipped fallbacks while still losing to any discovered installed plugin.

catalog.ts: Introduces two named constants (EXTERNAL_CATALOG_PRIORITY, FALLBACK_CATALOG_PRIORITY) to make the intended hierarchy explicit; replaces the old "add only if absent" external-catalog logic with the same priority < existing.priority guard used everywhere else.
catalog.ts – resolveOfficialCatalogPaths: Replaces a try/catch { // ignore } around path.dirname(process.execPath) with a straightforward if (process.execPath) guard; process.execPath is always a non-empty string in Node.js, so this is a safe simplification.
plugins-core.test.ts: Adds two regression tests — one locking in that an external catalog override wins over both bundled-dir and official-catalog fallback metadata for the same channel ID, and another confirming that a locally discovered plugin still outranks any external catalog entry.

Confidence Score: 5/5

Safe to merge — targeted precedence fix with direct regression coverage for both changed scenarios.
The logic change is small and well-understood: two priority constants are introduced and the external-catalog loop gains proper override semantics. Both the regression (external beats fallback) and the preserved invariant (discovered beats external) are covered by new tests. The only open items are minor style concerns — a dead ?? 99 fallback and a priority tie that works correctly by loop ordering — neither of which affects correctness.
No files require special attention.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/channels/plugins/catalog.ts
Line: 55-56

Comment:
**Dead `?? 99` fallback on a typed Record key**

`ORIGIN_PRIORITY` is declared as `Record<PluginOrigin, number>`, and `bundled` is a concrete key in that record, so `ORIGIN_PRIORITY.bundled` is always typed as `number` — never `undefined`. The `?? 99` branch is unreachable dead code and may trigger a TypeScript "unnecessary nullish coalescing" lint warning in strict mode.

```suggestion
const EXTERNAL_CATALOG_PRIORITY = ORIGIN_PRIORITY.bundled;
const FALLBACK_CATALOG_PRIORITY = EXTERNAL_CATALOG_PRIORITY + 1;
```

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/channels/plugins/catalog.ts
Line: 55

Comment:
**Tie between `EXTERNAL_CATALOG_PRIORITY` and `ORIGIN_PRIORITY.bundled` relies on loop ordering**

`EXTERNAL_CATALOG_PRIORITY` (3) equals `ORIGIN_PRIORITY.bundled` (3), so the "discovered bundled beats external catalog" invariant is enforced only because the discovered-plugin loop runs before the external-catalog loop, and the comparison is a strict `<` rather than `<=`. This is correct today, but the intent is obscured and is one loop-reorder away from breaking silently.

A slightly lower number (e.g., `ORIGIN_PRIORITY.bundled + 1`) would make external catalogs numerically weaker than all discovered sources without relying on insertion order, while still beating `FALLBACK_CATALOG_PRIORITY`:

```suggestion
const EXTERNAL_CATALOG_PRIORITY = ORIGIN_PRIORITY.bundled + 1;
const FALLBACK_CATALOG_PRIORITY = EXTERNAL_CATALOG_PRIORITY + 1;
```

The regression tests do guard this invariant, so the current state is safe — this is a readability/maintainability note only.

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

_{Reviews (1): Last reviewed commit: "fix(channels): preserve external catalog..." | Re-trigger Greptile}

src/channels/plugins/catalog.ts

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8dc07b87c5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/channels/plugins/catalog.ts

aisle-research-bot · 2026-03-23T16:04:21Z

🔒 Aisle Security Analysis

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

#	Severity	Title
1	🟠 High	Untrusted external plugin catalogs can override official/bundled channel metadata, enabling supply-chain RCE via arbitrary npm package install

1. 🟠 Untrusted external plugin catalogs can override official/bundled channel metadata, enabling supply-chain RCE via arbitrary npm package install

Property	Value
Severity	High
CWE	CWE-494
Location	`src/channels/plugins/catalog.ts:412-423`

Description

listChannelPluginCatalogEntries() now gives external catalog entries higher priority than shipped fallback metadata (bundled + official), so a local/CI attacker who can influence catalogPaths (or OPENCLAW_PLUGIN_CATALOG_PATHS / default config-dir catalog files) can override a known channel id (e.g., "whatsapp") to point install.npmSpec at an attacker-controlled npm package.

Why this is dangerous:

Input/trust boundary: external catalog file paths come from CatalogOptions.catalogPaths, or environment variables OPENCLAW_PLUGIN_CATALOG_PATHS / OPENCLAW_MPM_CATALOG_PATHS, or default paths under the user's config dir.
No authenticity/integrity check: catalog JSON is read and trusted without signature verification.
Override semantics: external entries take precedence over official/bundled fallback entries for the same entry.id.
Impact: downstream flows (e.g., channel setup) may present/execute an installation of entry.install.npmSpec, leading to execution of attacker-controlled code when the plugin is installed/loaded.

Vulnerable code (priority override):

// External catalogs ... supported override seam for shipped fallback metadata
const priority = EXTERNAL_CATALOG_PRIORITY;
const existing = resolved.get(entry.id);
if (!existing || priority < existing.priority) {
  resolved.set(entry.id, { entry, priority });
}

Note: installPluginFromNpmSpec() restricts specs to registry-style names (no file:, git:, URLs, or semver ranges), which reduces but does not eliminate the risk—an attacker can still redirect installs to any malicious npm package.

Recommendation

Treat external catalogs as untrusted by default and prevent silent substitution of official/bundled channel ids.

Recommended options (pick one or combine):

Require explicit opt-in to allow overrides (e.g., --allow-external-catalog-overrides), otherwise only add entries for unknown ids.
Allowlist permitted npm scopes for externally-sourced channel ids (e.g., only @openclaw/* by default), or require an allowlist mapping of {channelId -> allowedPackageNames}.
Cryptographically verify catalogs (signature + pinned public key), and reject unsigned/untrusted catalogs.

Example: disallow overriding known ids unless explicitly allowed:

const isKnownFallback = existing && existing.priority >= FALLBACK_CATALOG_PRIORITY;
const allowOverride = options.env?.OPENCLAW_ALLOW_CATALOG_OVERRIDE === "1";

if (!existing || (!isKnownFallback) || allowOverride) {
  resolved.set(entry.id, { entry, priority });
}

Additionally, log a warning whenever an external catalog overrides a shipped entry, and consider requiring an exact version pin (name@x.y.z) for external catalogs to reduce supply-chain drift.

Analyzed PR: #52988 at commit 13ca9a3

_{Last updated on: 2026-03-23T16:17:48Z}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 13ca9a362d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-03-23T16:12:36Z

src/channels/plugins/catalog.ts

+    const priority = EXTERNAL_CATALOG_PRIORITY;
+    const existing = resolved.get(entry.id);
+    if (!existing || priority < existing.priority) {
+      resolved.set(entry.id, { entry, priority });


Preserve shipped aliases on external channel overrides

When an external catalog reuses a built-in channel ID just to change install.npmSpec, this now replaces the entire shipped catalog entry instead of overlaying it. resolveCatalogChannelEntry() only matches entry.id and entry.meta.aliases (src/commands/channel-setup/channel-plugin-resolution.ts:49-59), and the built-in catalog already relies on shipped aliases such as Microsoft Teams' teams alias (src/channels/plugins/plugins-core.test.ts:119-123). In the private-fork override scenario this patch targets, any external entry that omits aliases/order/other metadata will silently drop those behaviors, so alias-based channel selection and similar metadata-driven flows stop working unless operators duplicate every shipped field in the override file.

Useful? React with 👍 / 👎.