Skip to content

fix(channel): harden local setup trust#89456

Closed
hxy91819 wants to merge 12 commits into
openclaw:mainfrom
hxy91819:fix/channel-load-path-trust-followup
Closed

fix(channel): harden local setup trust#89456
hxy91819 wants to merge 12 commits into
openclaw:mainfrom
hxy91819:fix/channel-load-path-trust-followup

Conversation

@hxy91819

@hxy91819 hxy91819 commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

  • follow up on fix(plugins): block untrusted workspace setup-only channel loads #86953's workspace-only setup trust fix by extending the same trusted-catalog fallback logic to local config / global channel plugins discovered through plugins.load.paths or linked installs
  • prove the regression on current code with the blackbox plugin-trust harness: clean main blocks trusted plugins.load.paths setup because the catalog never sees that channel, while this branch reaches the loader and passes all four trust cases
  • keep the setup trust rule origin-aware: reject untrusted local shadows by { pluginId, origin }, then fall back to a safe bundled/non-local copy instead of suppressing all copies of the same plugin id
  • fix the .dts build break in src/commands/channel-setup/trusted-catalog.ts, which was failing fresh pnpm build:plugin-sdk:dts / Docker image builds even when local incremental caches hid it
  • add main-repo L4 coverage plus a focused Docker/package L6 manual proof script for the local channel plugin trust boundary

Change Type

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs / PR context
  • Security hardening
  • Chore / infra

Scope

  • Integrations
  • API / contracts
  • UI / DX
  • CI / infra
  • Gateway / orchestration
  • Auth / tokens
  • Memory / storage

Linked Issue / PR

Problem Statement

#86953 established the product rule that untrusted local channel plugins should not get imported during setup-only channel flows. That landed for workspace-origin plugins.

The remaining gap was the broader local-origin surface:

  • plugins.load.paths channel plugins appear as origin: "config"
  • linked / managed local installs can appear as origin: "global"

Those are still local code from the operator's point of view. If setup hardening only keys on origin === "workspace", the trust boundary is inconsistent: one class of local plugin is blocked before import, while other local shadows can still win channel catalog resolution or suppress a safe bundled fallback.

This branch also had a separate concrete defect in fresh builds: src/commands/channel-setup/trusted-catalog.ts failed declaration generation, so Docker image builds died in build:plugin-sdk:dts even though local incremental caches could mask the problem.

The fresh-build failure was:

src/commands/channel-setup/trusted-catalog.ts(82,3): error TS2344:
Type '"excludeOrigins" | "excludePluginRefs"' does not satisfy the constraint 'never'.
src/commands/channel-setup/trusted-catalog.ts(97,3): error TS2322:
Type '{ excludeOrigins?: (PluginOrigin | undefined)[] | undefined; ... }'
is not assignable to type 'Pick<CatalogOptions | undefined, "excludeOrigins" | "excludePluginRefs">'.
src/commands/channel-setup/trusted-catalog.ts(113,61): error TS2345:
Argument of type '{ ... excludeOrigins: unknown; excludePluginRefs: unknown; }'
is not assignable to parameter of type 'CatalogOptions'.

Blackbox Proof

Harness:

  • e2e repo: sibling openclaw-e2e-go
  • spec: specs/plugintrust/local_origin_followup_test.go
  • probe: scripts/docker/test-plugintrust-local-origin.sh
  • fixtures: scripts/fixtures/plugintrust/{workspace-shadow,load-paths-shadow}/
  • command: OPENCLAW_E2E_TARGET_REPO=<openclaw checkout> go test ./specs/plugintrust -v

The harness bakes a fresh OpenClaw image from source, then asserts only on marker files written by fixture plugins:

  • PLUGINTRUST_SETUP_IMPORT_MARKER: module top-level import happened
  • PLUGINTRUST_SETUP_REGISTER_MARKER: setup hook actually ran

Clean main (a4196a44454f) behavior:

  • Case 1 workspace + untrusted: PASS, trust gate blocks the untrusted workspace plugin
  • Case 2 workspace + plugins.allow: PASS, trusted workspace plugin loads
  • Case 3 load-paths + untrusted: PASS as probe-only, marker absent because catalog rejects Unknown channel e2e-load-paths before loader
  • Case 4 load-paths + plugins.allow: FAIL, same Unknown channel e2e-load-paths; trusted load-path plugin is still invisible to the catalog on main

This branch behavior from the previously built fresh image:

  • Case 1: PASS, untrusted workspace shadow not loaded
  • Case 2: PASS, trusted workspace shadow loaded
  • Case 3: PASS, load-path probe reached and recorded
  • Case 4: PASS, trusted load-path plugin loaded

That is the direct before/after proof for the regression this PR fixes: on main, trusted plugins.load.paths channels never reach setup because the catalog cannot resolve them; on this branch, the same scenario resolves and loads correctly while the untrusted cases still stay blocked.

What Changed

  1. Thread extraPaths through channel catalog resolution so trusted channel setup/add flows can actually see channel plugins discovered from plugins.load.paths.
  2. Make fallback exclusion origin-aware with excludePluginRefs: [{ pluginId, origin }], so rejecting an untrusted local shadow only skips that exact local candidate and still allows a bundled copy of the same channel/plugin id to win.
  3. Keep setup discovery lenient for display-only cases: if an untrusted local entry has no trusted fallback, it stays visible in the setup discovery bucket rather than disappearing from the UI.
  4. Fix the TypeScript declaration-build typing in trusted-catalog.ts by replacing the unstable Pick<Parameters<...>[1], ...> shape with an explicit local exclusions type and explicit local-origin narrowing.
  5. Add src/commands/channels.add.trust.e2e.test.ts as a main-repo L4 E2E regression for the real channels add CLI path.
  6. Add bash scripts/e2e/channel-plugin-trust-docker.sh as a focused Docker/package L6 manual proof script for the plugins.load.paths trust boundary.

Why This Is The Right Follow-up To #86953

#86953 already proved the core trust model: local untrusted channel plugin code must not execute in setup-only flows. This PR does not redefine that model; it applies the same invariant consistently to the remaining local origins the earlier PR intentionally left as a follow-up hardening item.

The pr-86953-autoreview-follow-up.zh.md note called out exactly this direction: stop hardcoding one origin name and move the loader/catalog gate toward a unified local trust decision. This PR stays within that scope and keeps the current product behavior for trusted plugins:

  • explicitly trusted / allowlisted local plugins still resolve
  • bundled entries still act as the safe fallback when a local shadow is not trusted
  • discovery UI can still show installable local-only candidates without treating them as executable

Verification

  • pnpm build:plugin-sdk:dts
  • pnpm test src/commands/channel-setup/trusted-catalog.test.ts -- --reporter=verbose
  • pnpm build
  • pnpm test src/commands/channels.add.trust.e2e.test.ts -- --reporter=verbose
  • bash scripts/e2e/channel-plugin-trust-docker.sh
  • git diff --check
  • env npm_config_registry=https://registry.npmjs.org/ node scripts/generate-npm-shrinkwrap.mjs --check
  • Blackbox harness context above from sibling openclaw-e2e-go

Evidence

  • Fresh pnpm build:plugin-sdk:dts now passes after the trusted-catalog.ts type fix; this was the blocker for Docker image rebuilds.
  • Focused Vitest passed: 10/10 in src/commands/channel-setup/trusted-catalog.test.ts.
  • pnpm build passes after the declaration-build fix.
  • Main-repo L4 E2E passed: src/commands/channels.add.trust.e2e.test.ts covers workspace/load-paths x trusted/untrusted, 4/4 passing through the real openclaw channels add CLI path.
  • New L6 Docker/package manual proof passed locally: bash scripts/e2e/channel-plugin-trust-docker.sh built the package artifacts, packed/checked the OpenClaw tarball, installed it into the functional Docker image, then verified plugins.load.paths untrusted blocks setup entry execution and plugins.allow permits setup entry execution.
  • Root npm-shrinkwrap.json was regenerated/check-verified with https://registry.npmjs.org/; no local mirror URLs are present.
  • pnpm check:changed could not complete locally because the installed Crabbox binary is 0.15.0 while current Testbox delegation requires Crabbox >=0.22.0. Running the child command directly reached existing shrinkwrap guard behavior for changed plugin packages; no test/lint/type failure was observed before that guard.

@openclaw-barnacle openclaw-barnacle Bot added commands Command implementations size: XL maintainer Maintainer-authored PR labels Jun 2, 2026
@clawsweeper

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 3:32 AM ET / 07:32 UTC.

Summary
Review failed before ClawSweeper could summarize the requested change.

PR surface: Source +160, Tests +788, Other +324. Total +1272 across 13 files.

Reproducibility: unclear. The review failed before ClawSweeper could establish a reproduction path.

Review metrics: none identified.

Merge readiness
Overall: 🌊 off-meta tidepool
Proof: 🌊 off-meta tidepool
Patch quality: 🌊 off-meta tidepool
Result: rating does not apply to this item.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] No close action taken because the review did not complete.

Maintainer options:

  1. Decide the mitigation before merge
    Retry the Codex review after fixing the execution failure.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • Review did not complete, so no work-lane recommendation was made.
Review details

Best possible solution:

Retry the Codex review after fixing the execution failure.

Do we have a high-confidence way to reproduce the issue?

Unclear. The review failed before ClawSweeper could establish a reproduction path.

Is this the best way to solve the issue?

Unclear. Retry the review first so ClawSweeper can evaluate the actual issue and fix direction.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against 418d7e1e83c5.

Label changes

Label changes:

  • remove P2: Current review triage priority is none.
  • remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
  • remove merge-risk: 🚨 security-boundary: Current PR review selected no merge-risk labels.

Label justifications:

  • rating: 🌊 off-meta tidepool: Overall readiness is 🌊 off-meta tidepool; proof is 🌊 off-meta tidepool and patch quality is 🌊 off-meta tidepool.
Evidence reviewed

PR surface:

Source +160, Tests +788, Other +324. Total +1272 across 13 files.

View PR surface stats
Area Files Added Removed Net
Source 4 184 24 +160
Tests 8 953 165 +788
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 1 324 0 +324
Total 13 1461 189 +1272

What I checked:

  • failure reason: codex execution failed.
  • codex failure detail: Codex review failed for this PR with exit 1.
  • codex stdout: Per-item Codex failure; continuing with the rest of the shard.

Likely related people:

  • unknown: Codex failed before it could trace repository history. (role: review did not complete; confidence: low)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 2, 2026
@byungskers

Copy link
Copy Markdown

I like the origin-aware fallback here — excluding { pluginId, origin } instead of suppressing every copy of the same id feels like the right shape. One thing I wondered about: resolveTrustedCatalogEntry() is now recursive across rejected candidates. In the normal catalog flow that should terminate quickly, but would it be worth adding a tiny guard against repeatedly resurfacing the same local candidate set (for example a malformed discovery result that keeps producing an entry without a stable pluginId)? Probably not a blocker, just checking whether you thought about a recursion cap / seen-set here.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 2, 2026
@hxy91819 hxy91819 force-pushed the fix/channel-load-path-trust-followup branch from 182404f to 8622783 Compare June 3, 2026 02:16
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 3, 2026
@hxy91819 hxy91819 requested a review from a team as a code owner June 4, 2026 06:36
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts docker Docker and sandbox tooling labels Jun 4, 2026
@github-actions github-actions Bot added the dependencies-changed PR changes dependency-related files label Jun 4, 2026
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Dependency graph guard cleared

This PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh /allow-dependencies-change comment after the guard blocks that new head SHA.

  • Current SHA: c99d3b1f4271dba6effed8a484eecc988dca1ada

@clawsweeper clawsweeper Bot removed the rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. label Jun 4, 2026
@hxy91819 hxy91819 force-pushed the fix/channel-load-path-trust-followup branch from d38ade1 to e126b79 Compare June 8, 2026 14:05
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jun 8, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 11, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 11, 2026
@hxy91819

Copy link
Copy Markdown
Member Author

@clawsweeper automerge

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

🦞🔧
ClawSweeper automerge is enabled.

Draft PRs stay fix-only until GitHub marks them ready for review. Pause with /clawsweeper stop.

Automerge progress:

  • 2026-06-11 12:34:09 UTC review queued c99d3b1f4271 (queued)

@clawsweeper clawsweeper Bot added the clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge label Jun 11, 2026
@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper 🐠 reef update

Thanks for the work on this. ClawSweeper opened a replacement PR only because the source branch was not writable from the available bot permissions. branch tides, not contributor blame.

Why replacement: ClawSweeper could not update the source PR branch directly; GitHub did not grant sufficient push rights to the bot for that branch.
Replacement PR: #92175
Why close: this run explicitly closes the superseded source PR after the credited replacement PR is open, so review continues in one place.
Closing this source PR only because source-PR closing was explicitly enabled for this run.
Credit follows the fix over to the replacement PR. no sneaky treasure grab.
Co-author credit kept:

fish notes: reasoning high; reviewed against bf86d9f.

@clawsweeper clawsweeper Bot closed this Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge commands Command implementations docker Docker and sandbox tooling maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. scripts Repository scripts size: XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants