Skip to content

fix(security): block untrusted workspace providers in startup discovery#74302

Closed
vincentkoc wants to merge 1 commit into
mainfrom
codex/fix-untrusted-workspace-provider-execution
Closed

fix(security): block untrusted workspace providers in startup discovery#74302
vincentkoc wants to merge 1 commit into
mainfrom
codex/fix-untrusted-workspace-provider-execution

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Apr 29, 2026

Copy link
Copy Markdown
Member

Motivation

  • Prevent untrusted workspace plugin discovery entries from being imported/executed during startup prewarm when providerDiscoveryEntriesOnly is enabled, closing a path that could run workspace code without explicit trust.

Description

  • When providerDiscoveryEntriesOnly is true, forward includeUntrustedWorkspacePlugins: false into resolveRuntimePluginDiscoveryProviders in resolveImplicitProviders (src/agents/models-config.providers.implicit.ts) to block workspace-origin plugins from being considered.
  • Add/adjust a unit expectation to assert the entries-only startup discovery call includes includeUntrustedWorkspacePlugins: false in src/agents/models-config.providers.implicit.discovery-scope.test.ts.

Testing

  • Ran pnpm test src/agents/models-config.providers.implicit.discovery-scope.test.ts, which passed.
  • The new/updated test verifies the entries-only startup discovery flow sets the untrusted-workspace guard and the assertion passed.

Codex Task

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: XS maintainer Maintainer-authored PR labels Apr 29, 2026
@vincentkoc

vincentkoc commented Apr 29, 2026

Copy link
Copy Markdown
Member Author

/clownfish automerge

@greptile-apps

greptile-apps Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds includeUntrustedWorkspacePlugins: false to the resolveRuntimePluginDiscoveryProviders call when providerDiscoveryEntriesOnly is true, preventing untrusted workspace plugins from being executed during startup prewarm discovery. The corresponding test expectation is updated to assert this guard is present. The change is minimal, consistent with how other call sites (setup.ts, auth-choice.plugin-providers.ts) already handle the flag, and the test passes.

Confidence Score: 5/5

Safe to merge — the change is a targeted, low-risk security hardening with no breaking side effects.

The change is two lines of production code and one line of test assertion. It follows the exact same pattern already used in other callers (setup.ts, auth-choice.plugin-providers.ts), the flag is optional so existing callers without it are unaffected, and the test was correctly updated. No logic errors or regressions detected.

No files require special attention.

Reviews (1): Last reviewed commit: "fix(security): block untrusted workspace..." | Re-trigger Greptile

@clawsweeper

clawsweeper Bot commented Apr 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 4:13 PM ET / 20:13 UTC.

Summary
The PR adds includeUntrustedWorkspacePlugins: false to entries-only implicit provider startup discovery and updates the focused startup-discovery test expectation.

PR surface: Source +2, Tests +1. Total +3 across 2 files.

Reproducibility: no. The original untrusted-workspace execution path is no longer reproducible from source on current main because the shared provider helper filters omitted trust flags by default.

Review metrics: none identified.

Root-cause cluster
Relationship: superseded
Canonical: #81069
Summary: The merged helper-level PR owns the central untrusted workspace provider discovery behavior; this PR remains as an optional explicit startup caller assertion on a protected maintainer item.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🌊 off-meta tidepool
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] GitHub reports the branch as CONFLICTING / DIRTY, so maintainers need a rebase or explicit close decision before any merge.
  • [P1] The central security behavior is already fixed on current main and shipped in v2026.6.11 through the shared provider helper; merging this PR now mainly preserves an explicit startup caller assertion.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the shipped helper-level fail-closed behavior; rebase and merge this PR only if maintainers still want the explicit startup caller contract, otherwise close it through explicit maintainer handling.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • The PR is MEMBER-authored and labeled maintainer, so automated cleanup should keep it open for explicit maintainer choice about rebasing and preserving the caller-level assertion.

Security
Cleared: The diff narrows a pre-trust workspace plugin discovery path and adds no dependency, workflow, secret-handling, download, or broader execution-surface concern.

Review details

Best possible solution:

Keep the shipped helper-level fail-closed behavior; rebase and merge this PR only if maintainers still want the explicit startup caller contract, otherwise close it through explicit maintainer handling.

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

No. The original untrusted-workspace execution path is no longer reproducible from source on current main because the shared provider helper filters omitted trust flags by default.

Is this the best way to solve the issue?

No, not as the standalone security fix anymore. The merged helper-level default is the broader owner-boundary fix; this PR is only an optional explicit caller assertion after rebase.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 5361e5a0b455; fix evidence: release v2026.6.11, commit 0aabaebba172.

Label changes

Label justifications:

  • P2: The PR concerns provider-discovery security hardening, but the central behavior is already implemented and the remaining action is a bounded maintainer decision.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🌊 off-meta tidepool and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Contributor proof gating is not applied because this is a MEMBER-authored, maintainer-labeled PR; the central behavior is separately proven by the merged related PR and shipped tests.
Evidence reviewed

PR surface:

Source +2, Tests +1. Total +3 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 3 1 +2
Tests 1 1 0 +1
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 4 1 +3

What I checked:

Likely related people:

  • mmaps: Authored the merged helper-level PR that changed the shared provider discovery default and added setup-trust marker coverage for the central behavior. (role: helper-level fix author; confidence: high; commits: 926cf98da6e0, 0aabaebba172; files: src/plugins/providers.ts, src/plugins/providers.setup-trust.test.ts, src/plugins/providers.test.ts)
  • eleqtrizit: Merged the related helper-level fix and commented on the setup compatibility risk before merge. (role: merger and compatibility reviewer; confidence: medium; commits: 28e0b880fcdc, 0aabaebba172; files: src/plugins/providers.ts, src/plugins/providers.setup-trust.test.ts)
  • vincentkoc: Authored this startup assertion branch and is connected to adjacent provider/auth discovery work in the OpenClaw history visible from related PR/search context. (role: current branch owner and adjacent contributor; confidence: medium; commits: 7c07ee5d68ff; files: src/agents/models-config.providers.implicit.ts, src/agents/models-config.providers.implicit.discovery-scope.test.ts)
  • pgondhi987: Related search for includeUntrustedWorkspacePlugins shows earlier merged provider trust hardening in the same security boundary, including provider auth-choice/runtime paths. (role: adjacent workspace-provider trust contributor; confidence: low; files: src/plugins/provider-auth-choices.ts, src/plugins/providers.runtime.ts, src/plugins/providers.ts)
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.

@martingarramon

martingarramon commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

LGTM. The 4-line change at models-config.providers.implicit.ts:484-489 consistently applies the includeUntrustedWorkspacePlugins: false guard alongside discoveryEntriesOnly: true — matches the pattern already in setup.ts and auth-choice.plugin-providers.ts per Greptile's note. Test expectation update at discovery-scope.test.ts:111-114 is the right boundary.

@clawsweeper clawsweeper Bot mentioned this pull request May 12, 2026
Merged
8 tasks
@jesse-merhi jesse-merhi added security Security documentation and removed security Security documentation labels May 25, 2026
@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. labels May 25, 2026
@clawsweeper clawsweeper Bot added the P2 Normal backlog priority with limited blast radius. label Jun 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. 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: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 22, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 3, 2026
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded by #81069 / 0aabaebba17280f67f2cec88acc1d6f5c615c39a.

The original security concern is valid, but current main already owns the fix at the shared provider-discovery boundary:

  • src/plugins/providers.ts:300-359 treats omitted includeUntrustedWorkspacePlugins as fail-closed (!== true) and requires activation/trust for workspace owners.
  • src/plugins/providers.setup-trust.test.ts:90-129 creates an executable workspace provider and proves default setup discovery neither imports nor registers it; :132-175 proves the explicit-trust control still loads.
  • src/agents/models-config.providers.implicit.ts:553-570 still reaches that shared boundary, so adding the same policy again at this one caller would be redundant rather than a second security layer.

I also checked the current Codex contract directly at openai/codex@db887d03e1f907467e33271572dffb73bceecd6b: codex-rs/config/src/config_toml.rs:895-951 validates/deserializes the explicit model_providers map, and codex-rs/core/src/config/mod.rs:3394-3416 merges and selects that map. Codex does not consume this OpenClaw workspace-plugin discovery path.

Supported path: keep the shipped shared fail-closed policy and explicit plugin trust/allowlist control. A concrete bypass of resolveDiscoveredProviderPluginIds, or a marker repro showing default startup imports untrusted workspace code on current main, would justify reopening with that evidence. Thanks @vincentkoc for identifying the original boundary.

@steipete steipete closed this Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

aardvark agents Agent runtime and tooling codex maintainer Maintainer-authored PR P2 Normal backlog priority with limited blast radius. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. size: XS status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vincentkoc @martingarramon @steipete @jesse-merhi