fix(plugins): secure setup-mode provider discovery against untrusted workspace plugins [AI-assisted]#81069
Conversation
|
Codex review: needs maintainer review before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. Current main forwards an omitted setup trust flag into helpers that only filter on explicit PR rating Rank-up moves:
What the crustacean ranks mean
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. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land the fail-closed setup-discovery fix after maintainer acceptance of the Do we have a high-confidence way to reproduce the issue? Yes. Current main forwards an omitted setup trust flag into helpers that only filter on explicit Is this the best way to solve the issue? Yes. The Label changes:
Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 777a113973e9. |
|
Not applicable to this automation stage; changelog/release-note and external real behavior proof requirements are handled outside auto-pr stages. Quoted comment from @clawsweeper:
|
|
Real behavior proof for PR head This is a one-off runtime proof against the production Command: GITHUB_SHA=926cf98da6e026d48922213277c63cdc8e263849 node --import tsx /tmp/openclaw-pr-81069-real-proof.tsOutput: Observed result: with default setup discovery and no explicit trust, OpenClaw returns no provider and neither the top-level marker nor the register marker exists. With the same plugin explicitly allowlisted, the provider is discovered and both markers are written, confirming the proof fixture would show execution if setup discovery loaded the workspace plugin. I also ran the focused regression proof with: node scripts/run-vitest.mjs run src/plugins/providers.setup-trust.test.ts --reporter=verboseThat command exited @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Mossy Clawlet Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
There is a small compatibility risk but it only involves setup. Let's rebase so we can update the changelog. |
Summary
resolveDiscoveredProviderPluginIds/resolveDiscoverableProviderOwnerPluginIdswithincludeUntrustedWorkspacePluginsunset (undefined), which evaluatedundefined === falseasfalse— leavingshouldFilterUntrustedWorkspacePlugins = falseand allowing untrusted workspace plugin code to be imported and executed before any explicit user trust..openclaw/extensions/could run arbitrary code at setup-discovery time, before the user has ever enabled or approved that plugin.providers.tschanged from=== falseto!== true, making the default (undefined) secure — untrusted workspace plugins are filtered unless the caller explicitly passesincludeUntrustedWorkspacePlugins: true.includeUntrustedWorkspacePluginsis explicitlyfalseortrue, and theprovider-install-catalog/provider-auth-choicespaths (which use a different, intentional semantic for that flag).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Real behavior proof (required for external PRs)
register()) executed during default setup discovery with no explicit trust granted.pnpm test src/plugins/providers.setup-trust.test.tsandpnpm test src/plugins/providers.test.tsproviders.setup-trust.test.tsuses filesystem markers written by the plugin's top-level code andregister()to prove no plugin code runs for the untrusted case, and that it does run for the explicitly-trusted (allowlisted) case.resolvePluginProviders({ mode: "setup" })without explicit trust returns[]and produces zero filesystem side effects from the workspace plugin.register()called withregistrationMode: "discovery"andtop-levelmarker written during default setup-mode call with no explicit trust.Root Cause
=== falsestrict-equality check treatedundefined(parameter omitted) identically totrue— both leftshouldFilterUntrustedWorkspacePlugins = false, disabling the workspace-plugin filter by default.falsepath was tested.falsecall sites in auth helpers, masking that the default left the gate open in the setup discovery path.Regression Test Plan
src/plugins/providers.setup-trust.test.ts(new),src/plugins/providers.test.ts(updated)falsepath.User-visible / Behavior Changes
Workspace-origin provider plugins are no longer loaded during setup-mode discovery unless they appear in
config.plugins.allow. Users relying on implicit setup-mode loading of workspace plugins must add those plugin IDs to theirallowlist.Diagram
Security Impact
register()and module-level code can no longer run during setup-mode provider discovery.Repro + Verification
Environment
pnpmconfig.plugins.enabled: true, noallowlistSteps
.openclaw/extensions/<id>/with aregister()that writes a filesystem marker.resolvePluginProviders({ mode: "setup", config: { plugins: { enabled: true } } })without settingincludeUntrustedWorkspacePlugins.Expected
Actual (after fix)
Evidence
providers.setup-trust.test.tsintegration markers + updatedproviders.test.tsassertions.Human Verification (required)
trueincludes it; explicitfalsestill filters; bundled plugins unaffected.allowlist is still discoverable in setup mode (second integration test).includeUntrustedWorkspacePluginsexplicitly (all confirmed to passfalse, which remains filtering behavior).Review Conversations
Compatibility / Migration
config.plugins.allow.plugins.allowarray in your OpenClaw config.Risks and Mitigations
config.plugins.allow. Error messaging in setup flows should guide users to add the plugin to their allow list.