Skip to content

fix(plugins): restrict bundled plugin dir resolution to trusted package roots#73275

Merged
pgondhi987 merged 20 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-530
Apr 28, 2026
Merged

fix(plugins): restrict bundled plugin dir resolution to trusted package roots#73275
pgondhi987 merged 20 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-530

Conversation

@pgondhi987
Copy link
Copy Markdown
Contributor

Summary

  • Problem: resolveBundledPluginsDir included cwd in the package-root candidate list even in non-source-checkout (production) mode. An attacker-controlled working directory shaped like an OpenClaw source checkout could override the bundled plugin root and supply a malicious extensions/memory-core/runtime-api.js, reachable via doctor.memory.* gateway RPC paths.
  • Why it matters: In supported launch shapes where argv1 doesn't resolve the real package root (single-binary, custom launcher), the fallback path argv1 → cwd → moduleUrl gave attacker-controlled directories priority over the real module root.
  • What changed: cwd is now excluded from the resolution candidate list when preferSourceCheckout is false (production mode). The override-path fallback also adds moduleUrl alongside argv1 as a trusted candidate.
  • What did NOT change: Source-checkout / vitest mode is unaffected; cwd remains trusted there for local dev.

🤖 AI-assisted fix (OpenAI Codex); reviewed and verified by Claude.

Change Type (select all)

  • Bug fix
  • Security hardening

Scope (select all touched areas)

  • Memory / storage
  • API / contracts

Linked Issue/PR

  • This PR fixes a bug or regression

Root Cause (if applicable)

  • Root cause: resolveBundledPluginsDir included { cwd: process.cwd() } unconditionally in the non-preferSourceCheckout path. resolveOpenClawPackageRootSync accepts any directory that contains a package.json with name: "openclaw", so a crafted working directory satisfies the check.
  • Missing detection / guardrail: No test verified that cwd was excluded from the candidate list in production mode.
  • Contributing context (if known): Affects launch shapes where argv1 does not resolve to the real package root.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
  • Target test or file: src/plugins/bundled-dir.test.ts
  • Scenario the test should lock in: When argv1 is a non-package path (/usr/bin/env) and cwd is a crafted OpenClaw-shaped directory, resolveBundledPluginsDir must not return the cwd-rooted extensions tree.
  • Why this is the smallest reliable guardrail: Directly exercises the resolution function with the exact attacker preconditions.
  • Existing test that already covers this (if any): None prior to this PR.
  • If no new test is added, why not: N/A — new test added.

User-visible / Behavior Changes

None in standard installed use. Developers running openclaw directly from a source checkout via a packaged binary without vitest or tsx are unaffected because moduleUrl resolution picks up the same root.

Diagram (if applicable)

Before (production mode):
argv1 -> cwd -> moduleUrl  (cwd could be attacker-controlled)

After (production mode):
argv1 -> moduleUrl          (cwd excluded)

Security Impact (required)

  • New permissions/capabilities? No
  • Secrets/tokens handling changed? No
  • New/changed network calls? No
  • Command/tool execution surface changed? Yes — bundled plugin root is now restricted to trusted package roots only
  • Data access scope changed? No
  • Mitigation: Resolution candidates in production mode are now limited to argv1 and moduleUrl, both of which reflect the actual installed or running package, not the process working directory.

Repro + Verification

Environment

  • OS: Linux
  • Runtime/container: Node 22+
  • Model/provider: N/A
  • Integration/channel: N/A

Steps

  1. Create a fake repo: mkdir /tmp/evil && echo '{"name":"openclaw"}' > /tmp/evil/package.json && mkdir -p /tmp/evil/.git /tmp/evil/src /tmp/evil/extensions/memory-core && echo "throw new Error('pwned')" > /tmp/evil/extensions/memory-core/runtime-api.js
  2. Launch OpenClaw with argv1 pointing to a non-package path (e.g., via a single-binary launcher) and cwd=/tmp/evil.
  3. Trigger any doctor.memory.* RPC path.

Expected

  • Bundled plugin dir resolves to the real installed package's extensions tree; /tmp/evil/extensions is never used.

Actual (before fix)

  • /tmp/evil/extensions was selected as the bundled plugin dir.

Evidence

  • New test does not resolve bundled plugins from cwd when argv1 is not a package root in src/plugins/bundled-dir.test.ts covers the exact attack preconditions and fails on the pre-fix code.

Human Verification (required)

  • Verified scenarios: Code review confirmed production path removes cwdRoot from candidates when preferSourceCheckout is false; override-fallback path adds moduleUrl as a second trusted candidate.
  • Edge cases checked: preferSourceCheckout=true (vitest/tsx) retains cwd; deduplication filter handles identical roots; isSourceCheckoutRoot guard still excludes source checkouts from the override fallback.
  • What you did not verify: Live E2E with an actual packaged binary launcher.

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.

Compatibility / Migration

  • Backward compatible? Yes
  • Config/env changes? No
  • Migration needed? No

Risks and Mitigations

  • Risk: A user running OpenClaw in a non-standard layout where argv1 and moduleUrl both fail to resolve the package root could lose bundled plugin resolution fallback via cwd.
    • Mitigation: The existing walk-up-from-execPath and walk-up-from-module-file fallbacks (lines 169–199 of bundled-dir.ts) still provide coverage for edge cases.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels Apr 28, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 28, 2026

Greptile Summary

This PR hardens resolveBundledPluginsDir against CWD-based path-hijacking in production mode by removing cwdRoot from the candidate list when preferSourceCheckout is false, and adds moduleUrl as a second trusted fallback in the override path. A targeted regression test covering the exact attack preconditions is included.

Confidence Score: 5/5

Safe to merge — the fix is minimal, logically sound, and directly backed by a regression test.

No P0 or P1 findings. The security hardening is correct: cwdRoot is only resolved when preferSourceCheckout is true (dev/vitest mode), and the override-fallback path now includes moduleUrl alongside argv1. The expectResolvedBundledDirFromRoot helper is correctly updated to default argv1 to a package-rooted path so existing tests remain valid under the new resolution logic. State cleanup in afterEach covers all mutated fields.

No files require special attention.

Reviews (2): Last reviewed commit: "fix: address PR review feedback" | Re-trigger Greptile

Comment thread src/plugins/bundled-dir.test.ts Outdated
@pgondhi987
Copy link
Copy Markdown
Contributor Author

@greptile review

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 28, 2026

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by MEMBER author association and the maintainer label, current main still contains the bundled-plugin CWD/VITEST resolution behavior the PR targets, and the PR remains active security-sensitive loader work.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled plugin directory trust hardening with focused regression coverage after reviewing override/source-checkout compatibility, packaged and non-standard launcher behavior, and the Signal installer fetch/HTTPS handling; otherwise close it manually with a maintainer rationale.

What I checked:

  • Protected maintainer signal: Provided GitHub context reports this PR is open, unmerged, authored by a MEMBER, and labeled maintainer; cleanup policy requires explicit maintainer handling for either signal. git ls-remote also confirms the PR head still exists at d76fe2e9fa6c26103655be67e31051fe14b4b2b8. (d76fe2e9fa6c)
  • Current main still trusts explicit bundled-dir overrides: resolveBundledPluginsDir returns an existing OPENCLAW_BUNDLED_PLUGINS_DIR path directly, before any trusted package-root validation like the PR proposes. (src/plugins/bundled-dir.ts:116, b79e617ad12c)
  • Current main still includes CWD/VITEST path: The resolver derives preferSourceCheckout from env.VITEST, resolves cwdRoot from process.cwd(), and orders non-source candidates as [argvRoot, cwdRoot, moduleRoot], leaving the CWD-root selection path this PR hardens. (src/plugins/bundled-dir.ts:139, b79e617ad12c)
  • CWD-shaped roots can satisfy package-root resolution: resolveOpenClawPackageRootSync walks candidates from buildCandidates; buildCandidates includes opts.cwd, and ancestors are accepted by package.json name openclaw. (src/infra/openclaw-root.ts:102, b79e617ad12c)
  • Resolver feeds executable facade loading: The facade loader calls resolveBundledPluginsDir before resolving bundled facade modules, and memory-core runtime facade loading reaches memory-core/runtime-api.js; gateway doctor.memory.* handlers import that runtime facade. (src/plugin-sdk/facade-loader.ts:75, b79e617ad12c)
  • Current tests have not absorbed the PR guardrail: Current tests still expect source extensions under VITEST and still treat an explicit bundled-dir env override as the stock source root; searches found no current resolveTrustedExistingOverride, requireHttps, or CWD-exclusion regression test from the PR. (src/plugins/bundled-dir.test.ts:205, b79e617ad12c)

Remaining risk / open question:

  • Auto-closing would bypass both the protected maintainer label and the MEMBER-author cleanup policy.
  • Closing as implemented would be wrong because current main still contains the CWD/VITEST bundled-dir resolution path this PR targets.
  • The PR intentionally changes security-sensitive plugin loading, bundled-dir override semantics, source-checkout behavior, non-standard launcher behavior, and Signal installer download handling; those need explicit maintainer/security review before merge or manual closure.

Codex review notes: model gpt-5.5, reasoning high; reviewed against b79e617ad12c.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in a2905f09d933b860aef520a3b95373d583981049.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

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

# Severity Title
1 🟠 High Untrusted env var VITEST enables loading bundled plugins from current working directory
1. 🟠 Untrusted env var VITEST enables loading bundled plugins from current working directory
Property Value
Severity High
CWE CWE-427
Location src/plugins/bundled-dir.ts:186-203

Description

resolveBundledPluginsDir treats env.VITEST as a signal to prefer a source checkout and, when set, adds the package root resolved from process.cwd() to the search order. If an attacker can influence the execution working directory (e.g., running the CLI in an untrusted directory/repo) and can get VITEST set in the process environment (common in test/CI shells), they can cause OpenClaw to resolve bundled plugins from an attacker-controlled extensions/ directory.

Key points:

  • Control input: env.VITEST and process.cwd() are untrusted inputs in many CLI/CI contexts.
  • Spoofable root: resolveOpenClawPackageRootSync({ cwd }) trusts any ancestor directory with package.json name "openclaw" (no signature/ownership check), which an attacker can create.
  • Unsafe selection: when preferSourceCheckout is true, resolveBundledDirFromPackageRoot returns packageRoot/extensions based on mere existence (no manifest validation and no requirement that it is the real installed package).
  • Impact: malicious plugin code placed under extensions/ can be loaded/executed, leading to potential RCE/policy bypass.

Vulnerable code:

const preferSourceCheckout = Boolean(env.VITEST) || runningSourceTypeScriptProcess();
...
const cwdRoot = preferSourceCheckout
  ? resolveOpenClawPackageRootSync({ cwd: process.cwd() })
  : null;
...
preferSourceCheckout ? [cwdRoot, argvRoot, moduleRoot] : [argvRoot, moduleRoot]

and:

if (preferSourceCheckout && fs.existsSync(sourceExtensionsDir)) {
  return sourceExtensionsDir;
}

Recommendation

Do not use a generic env var (VITEST) as a trust signal for plugin resolution in production code.

Safer options (pick one):

  1. Remove env.VITEST from runtime behavior and only prefer source checkout based on hard-to-spoof runtime checks (e.g., runningSourceTypeScriptProcess()), or behind an explicit OpenClaw-owned debug flag.

  2. If keeping a test flag, require that the selected root is the installed OpenClaw package (e.g., moduleRoot) and never cwdRoot, or require strong validation that cwdRoot equals moduleRoot.

  3. Harden selection of extensions/: only allow packageRoot/extensions when isSourceCheckoutRoot(packageRoot) is true and packageRoot is trusted (e.g., equals moduleRoot).

Example tightening:

const moduleRoot = resolveOpenClawPackageRootSync({ moduleUrl: import.meta.url });
const preferSourceCheckout = runningSourceTypeScriptProcess(); // remove env.VITEST

const packageRoots = [argvRoot, moduleRoot].filter(Boolean);// In resolveBundledDirFromPackageRoot:
if (preferSourceCheckout && isSourceCheckoutRoot(packageRoot) && packageRoot === moduleRoot) {
  if (hasUsableBundledPluginTree(sourceExtensionsDir)) return sourceExtensionsDir;
}

This prevents untrusted CWDs from injecting a fake openclaw package root and getting their extensions/ loaded.


Analyzed PR: #73275 at commit 832c44e

Last updated on: 2026-04-28T11:06:06Z

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in a2905f09d933b860aef520a3b95373d583981049.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR has a protected maintainer signal (authorAssociation MEMBER and maintainer label), and current main has not absorbed the resolver hardening. The PR is also a focused security-sensitive plugin-loader change, so it needs explicit maintainer review rather than automated cleanup.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled-dir trust hardening in src/plugins/bundled-dir.ts with the focused regression coverage in src/plugins/bundled-dir.test.ts, after confirming the intended compatibility boundary for packaged installs, source checkouts, and bundled-dir overrides.

What I checked:

  • Protected maintainer signal: Provided GitHub context shows this PR is open and unmerged, authored by a MEMBER, and labeled maintainer. ClawSweeper policy says maintainer-authored or protected-label items must stay open for explicit maintainer judgment. (832c44e79660)
  • Current main still returns existing bundled-dir overrides directly: On current main, resolveBundledPluginsDir resolves OPENCLAW_BUNDLED_PLUGINS_DIR and immediately returns it when it exists, without the trusted-root validation proposed by the PR. (src/plugins/bundled-dir.ts:116, 3eb2a9d37108)
  • Current main still includes cwd in production candidate resolution: The non-source-checkout candidate ordering is still [argvRoot, cwdRoot, moduleRoot], so an OpenClaw-shaped working directory remains in the production candidate list the PR is trying to harden. (src/plugins/bundled-dir.ts:139, 3eb2a9d37108)
  • Package-root resolver accepts cwd-shaped OpenClaw roots: resolveOpenClawPackageRootSync iterates candidates from buildCandidates, and buildCandidates includes opts.cwd; package roots are accepted by reading a package.json whose name is openclaw. That matches the PR root-cause path. (src/infra/openclaw-root.ts:102, 3eb2a9d37108)
  • Resolver feeds executable bundled plugin facade loading: Bundled facade resolution calls resolveBundledPluginsDir, then the facade loader opens and loads the resolved module path through jiti. The memory-core SDK facade loads memory-core/api.js and memory-core/runtime-api.js through that path, and gateway doctor memory handlers import those helpers. (src/plugin-sdk/facade-loader.ts:75, 3eb2a9d37108)
  • Doctor memory surface reaches memory-core runtime facade: Gateway doctor handlers import memory-core runtime helpers and expose doctor.memory.* methods, which supports the PR body's claim that this is an executable bundled-plugin surface, not a cosmetic resolver change. (src/gateway/server-methods/doctor.ts:16, 3eb2a9d37108)

Remaining risk / open question:

  • Auto-closing would bypass the protected maintainer label and MEMBER-author policy.
  • Current main still has the resolver behavior this PR targets, so closing as implemented would be incorrect.
  • The PR changes security-sensitive bundled plugin root and override semantics; maintainers should review compatibility for OPENCLAW_BUNDLED_PLUGINS_DIR, source checkout/VITEST behavior, and non-standard launcher shapes before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3eb2a9d37108.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 9003ebad10fe9ff0bc2b4f6efdaf340ae72e1917.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

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

# Severity Title
1 🟡 Medium Unhandled exception / bundled plugins disablement via OPENCLAW_BUNDLED_PLUGINS_DIR override
1. 🟡 Unhandled exception / bundled plugins disablement via OPENCLAW_BUNDLED_PLUGINS_DIR override
Property Value
Severity Medium
CWE CWE-248
Location src/plugins/bundled-dir.ts:164-185

Description

resolveBundledPluginsDir() now enforces stricter validation of OPENCLAW_BUNDLED_PLUGINS_DIR via resolveTrustedExistingOverride(). This introduces two denial-of-service behaviors when a hostile parent process/service manager can control environment variables:

  • Unexpected process crash: if the override path exists but fs.realpathSync.native() fails (e.g., unreadable directory, permission issues, transient IO errors), resolveTrustedExistingOverride() throws. The exception is not caught in resolveBundledPluginsDir() and many callers invoke it during startup, so the process can terminate.
  • Reliable disabling of bundled plugins: if the override path does not exist or fs.existsSync() returns false due to an error (including common permission failures), resolveBundledPluginsDir() returns undefined (instead of falling back or returning the unresolved path). Callers treat undefined as “no stock root” (e.g., resolvePluginSourceRoots() returns { stock: undefined, ... }), which can silently disable bundled plugins and break expected functionality.

Vulnerable code:

if (fs.existsSync(resolvedOverride)) {
  return resolveTrustedExistingOverride(resolvedOverride);
}
...
return undefined;

and

const realOverride = safeRealpathSync(resolvedOverride);
if (!realOverride) {
  throw new Error("OPENCLAW_BUNDLED_PLUGINS_DIR must resolve to an accessible directory");
}

Recommendation

Treat override validation failures as non-fatal unless explicitly requested, and differentiate between "disable" and "fallback":

  • Do not throw on realpath errors in normal operation; instead log a warning and fall back to trusted default resolution.
  • If you want a strict mode, gate it behind a separate env flag (e.g., OPENCLAW_STRICT_BUNDLED_DIR_OVERRIDE=1).
  • Consider treating existsSync/permission failures the same as “stale override” and attempt the trusted fallback before returning undefined.

Example mitigation:

if (override) {
  const resolvedOverride = resolveUserPath(override, env);
  try {
    if (fs.existsSync(resolvedOverride)) {
      return resolveTrustedExistingOverride(resolvedOverride);
    }
  } catch (e) {// warn (once) and continue to fallback
  }

  const modulePackageRoot = resolveOpenClawPackageRootSync({ moduleUrl: import.meta.url });
  if (modulePackageRoot && !isSourceCheckoutRoot(modulePackageRoot)) {
    const fallback = resolveBundledDirFromPackageRoot(modulePackageRoot, false);
    if (fallback) return fallback;
  }// if override was set but unusable, either return fallback result or keep default resolution
  return undefined; // only if you explicitly want "no bundled plugins"
}

Analyzed PR: #73275 at commit a2905f0

Last updated on: 2026-04-28T11:25:58Z

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 725e1f917f5e6a3635f7ca04909d966d98feb666.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep this PR open. It has a protected maintainer label and current main still has the bundled-plugin directory resolution behavior the PR is trying to harden, so this is not an automated cleanup close candidate.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled-dir trust hardening with the focused regression coverage after the latest head has green relevant checks and the override/source-checkout compatibility boundary is confirmed.

What I checked:

  • Protected maintainer signal: Live issue metadata shows this PR is open, unmerged, and labeled maintainer; the provided context also reports a maintainer-authored/member signal. ClawSweeper policy says protected-label items need explicit maintainer handling.
  • Current main still includes cwd in production resolution: On current main, resolveBundledPluginsDir computes cwdRoot unconditionally and orders production candidates as [argvRoot, cwdRoot, moduleRoot], leaving the CWD-root selection path the PR targets. (src/plugins/bundled-dir.ts:139, e4ff7c162044)
  • CWD-shaped roots can satisfy the package-root resolver: resolveOpenClawPackageRootSync iterates candidates from buildCandidates, and buildCandidates includes opts.cwd; package roots are accepted by a package.json name of openclaw. That matches the PR root-cause path. (src/infra/openclaw-root.ts:102, e4ff7c162044)
  • Resolver feeds executable bundled plugin facades: The facade loader calls resolveBundledPluginsDir before resolving and loading bundled facade modules, and the memory-core bundled runtime facade loads memory-core/runtime-api.js; gateway doctor memory handlers re-export that runtime facade. (src/plugin-sdk/facade-loader.ts:75, e4ff7c162044)
  • PR head is a focused resolver/test change: The PR head changes only src/plugins/bundled-dir.ts and src/plugins/bundled-dir.test.ts. It removes generic VITEST and cwd trust from bundled-dir resolution, validates existing overrides against module-root-derived bundled plugin roots, and adds targeted regression cases. (src/plugins/bundled-dir.ts:73, 9003ebad10fe)
  • PR discussion still requires maintainer review: Aisle flagged override-handling DoS/disablement concerns, the author replied that later commits addressed them, and live check runs on the latest PR head were still mixed with failures/in-progress jobs during review. This is active security-sensitive loader work, not stale cleanup. (9003ebad10fe)

Remaining risk / open question:

  • Auto-closing would bypass the protected maintainer label policy.
  • Closing as implemented would be incorrect because current main still has the CWD candidate path this PR targets.
  • The PR intentionally changes security-sensitive bundled plugin root and override semantics; maintainers should verify compatibility for packaged installs, source/tsx execution, stale overrides, and non-standard launchers before landing.

Codex review notes: model gpt-5.5, reasoning high; reviewed against e4ff7c162044.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 725e1f917f5e6a3635f7ca04909d966d98feb666.

Quoted comment from @aisle-research-bot:

🤖 We're reviewing this PR with Aisle

We're running a security check on the changes in this PR now. This usually takes a few minutes. ⌛
We'll post the results here as soon as they're ready.

Progress:

  • Analysis
  • Finalization

Last updated on: 2026-04-28T12:01:06Z

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in e7d3f02158c71551ec81fe20944e2e88a237d5cc.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep this PR open. It has a protected maintainer label, and current main still contains the bundled-plugin directory resolution behavior the PR is trying to harden, so it is not a conservative cleanup close candidate.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled-dir trust hardening with the focused regression tests after the latest security review and relevant checks are green, and after maintainers confirm the intended compatibility boundary for packaged installs, source/tsx execution, stale overrides, and non-standard launchers.

What I checked:

  • Protected maintainer signal: Live issue metadata shows this PR is open and labeled maintainer; the provided review context also reports authorAssociation MEMBER. Repository policy says protected-label or maintainer-authored items require explicit maintainer handling.
  • Current main still trusts cwd in bundled-dir resolution: On current main, resolveBundledPluginsDir derives preferSourceCheckout from env.VITEST, resolves cwdRoot, and orders non-source candidates as [argvRoot, cwdRoot, moduleRoot], leaving the CWD-root path the PR targets. (src/plugins/bundled-dir.ts:139, e4ff7c162044)
  • CWD-shaped roots can satisfy the package-root resolver: resolveOpenClawPackageRootSync iterates candidates from buildCandidates, and buildCandidates includes opts.cwd; package roots are accepted by package.json name openclaw. That matches the PR's root-cause path. (src/infra/openclaw-root.ts:102, e4ff7c162044)
  • Resolved bundled dir feeds executable facade loading: The SDK facade loader calls resolveBundledPluginsDir before resolving bundled facade modules, and the memory-core bundled runtime facade loads memory-core/runtime-api.js; gateway doctor memory handlers import that runtime facade for doctor.memory.* methods. (src/plugin-sdk/facade-loader.ts:75, e4ff7c162044)
  • PR head is focused but security-sensitive: The latest PR diff at 725e1f917f5e6a3635f7ca04909d966d98feb666 changes only src/plugins/bundled-dir.ts, src/plugins/bundled-dir.test.ts, and src/plugins/source-display.test.ts; it removes VITEST/CWD trust, adds trusted override validation, and updates regression coverage. (src/plugins/bundled-dir.ts:46, 725e1f917f5e)
  • Active review still in progress: PR comments include Greptile approval plus Aisle security findings around VITEST trust and override handling; the author pushed follow-up commits through 725e1f917f5e6a3635f7ca04909d966d98feb666, and latest check-run data showed checks still in progress. This needs maintainer review, not automated closure. (725e1f917f5e)

Remaining risk / open question:

  • Auto-closing would bypass the protected maintainer label policy.
  • Closing as implemented would be incorrect because current main still includes the CWD candidate path this PR targets.
  • The PR intentionally changes bundled plugin root, OPENCLAW_BUNDLED_PLUGINS_DIR, VITEST, and non-standard launcher semantics, so compatibility and security review should finish before landing.

Codex review notes: model gpt-5.5, reasoning high; reviewed against e4ff7c162044.

@openclaw-barnacle openclaw-barnacle Bot added the channel: signal Channel integration: signal label Apr 28, 2026
@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 157abcd012e74bf7e4fa23b68ca9597fbc50c6da.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep this PR open. It is unmerged, maintainer/member-associated, labeled maintainer, and current main has not absorbed the bundled-plugin resolver hardening. The latest PR head also touches security-sensitive plugin resolution plus Signal install download handling, so this needs explicit maintainer review rather than conservative cleanup closure.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled plugin directory trust hardening with focused regression coverage after reviewing the latest head for bundled-dir override/source-checkout compatibility, non-standard launcher behavior, and the additional Signal installer download-safety changes; otherwise close it manually with a maintainer rationale.

What I checked:

  • Protected maintainer signal: The provided GitHub context shows this PR is open, unmerged, authored by a MEMBER, and labeled maintainer. ClawSweeper policy says maintainer-authored items and protected-label items require explicit maintainer handling. (e7d3f02158c7)
  • Current main still includes cwd in bundled-dir resolution: resolveBundledPluginsDir derives preferSourceCheckout from env.VITEST or source TypeScript execution, resolves cwdRoot from process.cwd(), and uses [argvRoot, cwdRoot, moduleRoot] for non-source-checkout ordering. That is the trust path this PR is trying to harden. (src/plugins/bundled-dir.ts:139, 12aaef9035ea)
  • Cwd-shaped roots can satisfy package-root resolution: resolveOpenClawPackageRootSync iterates buildCandidates, and buildCandidates includes opts.cwd; findPackageRootSync accepts ancestors whose package.json name is openclaw. This matches the PR's stated shaped-working-directory root cause. (src/infra/openclaw-root.ts:102, 12aaef9035ea)
  • Resolver feeds executable bundled facade loading: The facade loader calls resolveBundledPluginsDir, resolves bundled facade module locations, opens them inside the selected boundary, and loads them through jiti. The resolver behavior is therefore security-sensitive, not cosmetic. (src/plugin-sdk/facade-loader.ts:75, 12aaef9035ea)
  • Memory doctor surface reaches memory-core runtime facade: Gateway doctor memory helpers re-export from memory-core-bundled-runtime, which loads memory-core/runtime-api.js through the bundled plugin facade loader. That supports the PR body's affected doctor.memory.* surface claim. (src/plugin-sdk/memory-core-bundled-runtime.ts:102, 12aaef9035ea)
  • PR head includes active security-sensitive changes: The provided PR context shows latest head e7d3f02158c71551ec81fe20944e2e88a237d5cc changes src/plugins/bundled-dir.ts, bundled-dir/source-display tests, and extensions/signal/src/install-signal-cli.ts; comments show Greptile approval, Aisle security review cycles, and author follow-up commits. That is active review work, not obsolete cleanup. (extensions/signal/src/install-signal-cli.ts:97, e7d3f02158c7)

Remaining risk / open question:

  • Closing would bypass the protected maintainer label and MEMBER-author policy.
  • Closing as implemented would be incorrect because current main still contains the CWD candidate path this PR targets.
  • The PR changes bundled plugin root trust, override handling, VITEST/source-checkout behavior, and Signal installer download code; those security-sensitive paths should receive explicit maintainer review before merge or closure.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 12aaef9035ea.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 0760db5a1468f88195869f1d32048b6cf19db5bc.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by maintainer/member signals, current main still has the bundled-plugin CWD/VITEST trust path the PR targets, and the latest PR head includes security-sensitive loader and Signal installer download changes that need explicit maintainer review.

Best possible solution:

Keep this PR open for explicit maintainer review. If accepted, land the bundled plugin directory trust hardening with focused regression coverage after reviewing override/source-checkout compatibility, non-standard launcher behavior, and the Signal installer fetch/size-limit change; otherwise close it manually with a maintainer rationale.

What I checked:

  • Protected maintainer signal: The provided GitHub context marks this PR open, unmerged, authorAssociation MEMBER, and labeled maintainer; the public PR page also shows the PR is Open and labels include maintainer and channel: signal. ()
  • Current main still includes existing override trust: resolveBundledPluginsDir returns an existing OPENCLAW_BUNDLED_PLUGINS_DIR override directly before package-root fallback, so the trusted-root validation proposed by the PR is not implemented on current main. (src/plugins/bundled-dir.ts:116, f256eeba431b)
  • Current main still includes CWD/VITEST package-root resolution: resolveBundledPluginsDir derives preferSourceCheckout from env.VITEST, resolves cwdRoot from process.cwd(), and orders production candidates as [argvRoot, cwdRoot, moduleRoot], leaving the behavior this PR hardens. (src/plugins/bundled-dir.ts:139, f256eeba431b)
  • CWD-shaped roots can satisfy package-root resolution: resolveOpenClawPackageRootSync walks candidates from buildCandidates; buildCandidates includes opts.cwd, and findPackageRootSync accepts ancestors whose package.json name is openclaw. (src/infra/openclaw-root.ts:102, f256eeba431b)
  • Resolved bundled dir feeds executable facade loading: The SDK facade loader calls resolveBundledPluginsDir, resolves bundled facade module locations, opens the selected boundary, and loads the module through jiti; this makes the resolver security-sensitive rather than cosmetic. (src/plugin-sdk/facade-loader.ts:75, f256eeba431b)
  • Doctor memory surface reaches memory-core runtime facade: Gateway doctor.memory.* handlers import memory-core runtime helpers, and memory-core-bundled-runtime loads memory-core/runtime-api.js through the bundled facade loader, matching the PR's affected surface claim. (src/gateway/server-methods/doctor.ts:802, f256eeba431b)

Remaining risk / open question:

  • Auto-closing would bypass the protected maintainer label and MEMBER-author policy.
  • Closing as implemented would be incorrect because current main still contains the CWD/VITEST bundled-dir resolution behavior this PR targets.
  • The PR changes bundled plugin root trust, override fallback semantics, source/tsx behavior, non-standard launcher behavior, and Signal installer download handling; those are security-sensitive compatibility surfaces.

Codex review notes: model gpt-5.5, reasoning high; reviewed against f256eeba431b.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 0760db5a1468f88195869f1d32048b6cf19db5bc.

Quoted comment from @aisle-research-bot:

🤖 We're reviewing this PR with Aisle

We're running a security check on the changes in this PR now. This usually takes a few minutes. ⌛
We'll post the results here as soon as they're ready.

Progress:

  • Analysis
  • Finalization

Last updated on: 2026-04-28T15:19:19Z

@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in 6a27a91c3514b467c4ec95c27e52be6dfab40c6a.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

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

# Severity Title
1 🟡 Medium Signal CLI installer now allows plaintext HTTP (and HTTPS→HTTP redirects) for release archive download
1. 🟡 Signal CLI installer now allows plaintext HTTP (and HTTPS→HTTP redirects) for release archive download
Property Value
Severity Medium
CWE CWE-319
Location extensions/signal/src/install-signal-cli.ts:115-121

Description

downloadToFile() in the Signal extension was rewritten to use fetchWithSsrFGuard(). Unlike the previous implementation (node:https.request), fetchWithSsrFGuard permits both http: and https: URLs.

Impact:

  • If the GitHub release JSON (or network path/DNS) is tampered with to point to an http: asset URL, OpenClaw will download the installer archive over plaintext HTTP.
  • fetchWithSsrFGuard also follows redirects and does not prevent cross-protocol downgrade (e.g., https://...http://...), enabling MITM modification of the downloaded archive.
  • The downloaded archive is subsequently extracted and used as an executable (signal-cli), so a successful MITM could lead to arbitrary code execution in the operator context.

Vulnerable code:

const { response, release } = await fetchWithSsrFGuard({
  url,
  maxRedirects,
  capture: false,
  auditContext: "signal-cli-install-archive",
});

(no check that url / final URL is https: only)

Recommendation

Enforce HTTPS-only for release asset downloads (and for every redirect hop).

Example fix:

async function downloadToFile(url: string, dest: string, maxRedirects = 5): Promise<void> {
  const parsed = new URL(url);
  if (parsed.protocol !== "https:") {
    throw new Error("signal-cli download URL must be https");
  }

  const { response, finalUrl, release } = await fetchWithSsrFGuard({
    url,
    maxRedirects,
    capture: false,
    auditContext: "signal-cli-install-archive",
  });

  try {
    const finalParsed = new URL(finalUrl);
    if (finalParsed.protocol !== "https:") {
      throw new Error("signal-cli download redirect must remain https");
    }// ... stream to file as before
  } finally {
    await release();
  }
}

Optionally, also restrict hostnames to GitHub’s expected domains (e.g., github.com, objects.githubusercontent.com, github-releases.githubusercontent.com) if the project’s policy allows it.


Analyzed PR: #73275 at commit 0760db5

Last updated on: 2026-04-28T15:25:00Z

@openclaw-barnacle openclaw-barnacle Bot added the scripts Repository scripts label Apr 28, 2026
@pgondhi987
Copy link
Copy Markdown
Contributor Author

Addressed in d76fe2e9fa6c26103655be67e31051fe14b4b2b8.

Quoted comment from @aisle-research-bot:

🤖 We're reviewing this PR with Aisle

We're running a security check on the changes in this PR now. This usually takes a few minutes. ⌛
We'll post the results here as soon as they're ready.

Progress:

  • Analysis
  • Finalization

Last updated on: 2026-04-28T15:51:35Z

@pgondhi987 pgondhi987 merged commit bdfb408 into openclaw:main Apr 28, 2026
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026
…ge roots (openclaw#73275)

* fix: address issue

* fix: address review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address codex review feedback

* fix: address codex review feedback

* fix: address codex review feedback

* fix: address PR review feedback

* fix: address review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address review feedback

* fix: address PR review feedback

* fix: address PR review feedback

* fix: address review feedback

* docs: add changelog entry for PR merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: signal Channel integration: signal maintainer Maintainer-authored PR scripts Repository scripts size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant