Skip to content

security: require pinned spec + integrity for official npm plugin trust#76455

Closed
fede-kamel wants to merge 3 commits into
openclaw:mainfrom
fede-kamel:security/official-npm-trust-pinning
Closed

security: require pinned spec + integrity for official npm plugin trust#76455
fede-kamel wants to merge 3 commits into
openclaw:mainfrom
fede-kamel:security/official-npm-trust-pinning

Conversation

@fede-kamel

@fede-kamel fede-kamel commented May 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes a trust-bypass gap in isTrustedOfficialNpmPluginInstall (src/plugins/install.ts). The helper that bypasses the install security scan for official @openclaw/* packages previously matched the allowlist by name only, so:

  • openclaw plugin install @openclaw/codex (no version) — npm resolves to latest, scan bypassed
  • openclaw plugin install @openclaw/codex@latest (dist-tag) — scan bypassed
  • openclaw plugin install @openclaw/codex@1.2.3 without an integrity hash — scan bypassed

This contradicted the policy already enforced for trusted catalog installs in src/plugins/provider-install-catalog.ts:67-74 (commit da44d2d572 — "fix(security): require pinned npm specs for trusted installs"), which requires selectorKind === "exact-version" AND a non-empty expectedIntegrity. If the npm @openclaw scope were ever compromised, the unpinned path would have installed a malicious package without scan-level protection.

The current allowlist on main covers four packages (@openclaw/acpx, @openclaw/codex, @openclaw/google-meet, @openclaw/voice-call); the helper iterates the map, so the fix protects all of them.

Changes

  • isTrustedOfficialNpmPluginInstall now requires selectorKind === "exact-version" AND a non-empty expectedIntegrity, mirroring the policy in provider-install-catalog.ts.
  • expectedIntegrity plumbed through PackageInstallCommonParams, pickPackageInstallCommonParams, validatePackagePluginInstallSource, and the installPluginFromNpmSpec → installPluginFromInstalledPackageDir call site so the helper can see the value.

Test plan

  • pnpm test src/plugins/install.test.ts — 95/95 pass, including 5 new integration tests in a new describe("trusted official npm plugin install (isTrustedOfficialNpmPluginInstall)") block:
    • unpinned spec → blocked (SECURITY_SCAN_BLOCKED)
    • dist-tag spec (@openclaw/codex@latest) → blocked
    • pinned spec without expectedIntegrity → blocked
    • pinned spec with expectedIntegrity → allowed, "allowed because it is an official OpenClaw package" warning emitted
    • impostor packageName (e.g. @evil/typosquat masquerading) → blocked even with pinning + integrity
  • pnpm test src/plugins/update.test.ts src/plugins/clawhub.test.ts — 97/97 pass
  • pnpm tsgo:core clean
  • pnpm exec oxfmt --check src/plugins/install.ts src/plugins/install.test.ts clean
  • node scripts/run-oxlint.mjs src/plugins/install.ts src/plugins/install.test.ts — 0 warnings, 0 errors
  • Bug reproduced by reverting just the new pinning/integrity guards in the helper and re-running the new tests: 3 of 5 flip to failing, confirming the unpinned, dist-tag, and pinned-no-integrity scenarios all installed a dangerous spawn("google-chrome", []) payload without scan under the prior code

Related

  • Mirrors policy from da44d2d572 ("fix(security): require pinned npm specs for trusted installs"), which patched the same trust pattern in provider-install-catalog.ts but did not reach this newer parallel helper.

@fede-kamel
fix(security): require pinned spec + integrity for official npm plugi…
b42641d 
…n trust

The isTrustedOfficialNpmPluginInstall helper bypassed the install
security scan whenever the requested npm spec name matched the
@openclaw/codex allowlist entry, regardless of version selector or
expectedIntegrity. This contradicted the policy enforced by
provider-install-catalog.ts (da44d2d), which requires
selectorKind === "exact-version" plus a non-empty expectedIntegrity
for trusted installs.

Tighten the helper to mirror that policy and plumb expectedIntegrity
through PackageInstallCommonParams, validatePackagePluginInstallSource,
and the installPluginFromNpmSpec -> installPluginFromInstalledPackageDir
path so the helper sees the integrity value at the call site.

Adds five integration tests covering unpinned spec, dist-tag spec,
pinned-without-integrity (all blocked), pinned-with-integrity (allowed
with trusted-source warning), and impostor packageName (blocked even
with pinning + integrity).
@openclaw-barnacle openclaw-barnacle Bot added size: S triage: external-plugin-candidate Candidate: plugin/capability may belong on ClawHub. labels May 3, 2026
@clawsweeper

clawsweeper Bot commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

Summary
The PR requires exact-version specs plus non-empty expectedIntegrity before official @openclaw/* npm plugin installs can use the trusted scanner bypass, plumbs that value through install helpers, updates npm-spec tests, and adds a changelog entry.

Reproducibility: yes. Source inspection on current main shows the helper trusts by request/package/plugin name only, and current npm-spec tests exercise bare official specs with dangerous launch calls that expect the scanner bypass.

Next step before merge
No repair lane is needed because the latest PR diff addresses the prior mechanical blockers; the remaining action is maintainer/security review plus CI.

Security
Cleared: The diff narrows an existing plugin install trust bypass and does not add dependencies, workflow permissions, artifact sources, lifecycle hooks, or secret-handling paths.

Review details

Best possible solution:

Land the narrowed trust gate with regression coverage after maintainer/security approval and CI, keeping catalog-backed official install paths responsible for supplying pinned integrity metadata.

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

Yes. Source inspection on current main shows the helper trusts by request/package/plugin name only, and current npm-spec tests exercise bare official specs with dangerous launch calls that expect the scanner bypass.

Is this the best way to solve the issue?

Yes. The latest PR places the exact-version plus integrity requirement at the bypass helper, plumbs the existing integrity value through the npm install path, and updates stale tests and changelog without broadening the security surface.

What I checked:

  • Current main trust gate: isTrustedOfficialNpmPluginInstall on current main checks only npm request kind, parsed package name, package manifest name, and plugin id; it does not require selectorKind === "exact-version" or expectedIntegrity. (src/plugins/install.ts:199, b83b2e3f1cb5)
  • Scanner bypass consequence: Critical install-scan findings are blocked unless dangerouslyForceUnsafeInstall or trustedSourceLinkedOfficialInstall is set, so the official trust helper directly controls whether dangerous package launch code is allowed. (src/plugins/install-security-scan.runtime.ts:571, b83b2e3f1cb5)
  • Current main test expectation: The existing npm-spec tests on current main allow bare official @openclaw/* specs containing spawn or spawnSync payloads and assert the official-package warning. (src/plugins/install.npm-spec.test.ts:345, b83b2e3f1cb5)
  • PR guard: The PR adds exact-version and non-empty expectedIntegrity checks before returning trusted official npm install status, and passes expectedIntegrity into that helper. (src/plugins/install.ts:209, 9409792dbdcd)
  • Prior review addressed: The latest PR patch updates the stale npm-spec tests to use pinned specs with integrity for allowed cases, adds negative unpinned/dist-tag/no-integrity cases, and adds the required Unreleased changelog entry. (src/plugins/install.npm-spec.test.ts:344, 9409792dbdcd)
  • Feature history: Recent local history for the affected install files points to Vincent Koc introducing the official npm launch trust path and adjacent rollback handling. (src/plugins/install.ts, 006bd56dd686)

Likely related people:

  • vincentkoc: Recent main history shows Vincent Koc introduced the reviewed official npm launch-package trust path and then maintained adjacent npm install rollback behavior in the same install area. (role: introduced behavior and recent maintainer; confidence: high; commits: 006bd56dd686, ba3c0fc78e83; files: src/plugins/install.ts, src/plugins/install.test.ts, src/plugins/install.npm-spec.test.ts)
  • Bek: Recent adjacent official external plugin install alias work touched the CLI/catalog path that supplies official npm install metadata before this trust helper is reached. (role: adjacent owner; confidence: medium; commits: 411df5991608; files: src/cli/plugins-install-command.ts, src/cli/plugin-install-plan.ts, src/plugins/official-external-plugin-catalog.ts)

Remaining risk / open question:

  • The targeted test plan is author-reported in the PR body; this read-only review did not execute tests.
  • This hardening intentionally changes manual unpinned official npm installs from trusted-bypass behavior to scanner-enforced behavior, so maintainer security review should confirm that user-visible policy change is intended.

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

fede-kamel added 2 commits May 3, 2026 01:48
@fede-kamel
Merge branch 'main' into security/official-npm-trust-pinning
182050b
@fede-kamel
fix(plugins): address review feedback on trust-pinning
9409792 
- Update install.npm-spec.test.ts: the existing it.each table installed
  bare @openclaw/* specs (with spawn payloads) and asserted ok: true via
  the trusted-source bypass. With the new pinning + integrity gate those
  fixtures must use exact-version specs and pass expectedIntegrity to
  keep proving the legitimate trust path. Add a companion negative
  it.each that asserts the scanner now blocks the bare/dist-tag/
  no-integrity variants.
- Replace the misleading provider-install-catalog.ts citation in the
  install.ts code comment; that policy is not on current main.
- Add an Unreleased Fixes changelog entry under Plugins/install.

Verification:
- pnpm test src/plugins/install.npm-spec.test.ts src/plugins/install.test.ts -> 113/113 pass
- pnpm exec oxfmt --check --threads=1 src/plugins/install.ts src/plugins/install.test.ts src/plugins/install.npm-spec.test.ts CHANGELOG.md -> clean
- node scripts/run-oxlint.mjs src/plugins/install.ts src/plugins/install.test.ts src/plugins/install.npm-spec.test.ts -> 0 warnings 0 errors
- pnpm tsgo:core -> clean
@fede-kamel

fede-kamel commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

/clawsweeper re-review

@vincentkoc

vincentkoc commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

/clawsweeper re-review

@fede-kamel this is for maintainers only. As for your changes given our release process its not straight forward to mint SHA's and link to the releases as we are shipping a ton of stuff at once, I will keep on radar of the security and CI side to have this included. I will try to add some additional enforcements within this PR and should not be merged as-is. This behaviour will also break use-cases where users are upgrading plugins independently of version or switching to beta etc.

@vincentkoc vincentkoc mentioned this pull request May 3, 2026
Merged
25 tasks
@vincentkoc

vincentkoc commented May 3, 2026

Copy link
Copy Markdown
Member

Thanks @fede-kamel. I landed the reworked version in #76501 as 2a22eb6.

I kept the credit in the changelog and changed the hardening shape from npm pin/integrity-gated trust to explicit OpenClaw-owned install provenance: direct npm package names now scan normally, while catalog/onboarding/doctor paths pass the trusted-source bit deliberately.

Closing this PR as superseded by #76501.

@vincentkoc vincentkoc closed this May 3, 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

size: S triage: external-plugin-candidate Candidate: plugin/capability may belong on ClawHub.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fede-kamel @vincentkoc