Harden scanner trust boundaries: non-suppressible permit-all banner + reject in-node_modules PnP runtime#188
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a guard so the PnP loader refuses runtimes resolved from within ChangesPnP Trust Boundary Enforcement
CLI Notice Output Tests
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit a2b66d4
☁️ Nx Cloud last updated this comment at |
commit: |
Summary
Two low-risk, self-contained hardening changes to the discovery/notice surfaces. No behavior change for real projects today; both close latent gaps before later trust-lockfile work builds on them.
1. Refuse a Yarn PnP runtime resolved from within node_modules
The scanner reads package data as files and never executes discovered package code — with one sanctioned exception: loading the project's own Yarn PnP runtime (
.pnp.cjs) to map package identities to readable locations. That is safe only for the project's root-or-ancestor manifest.Today
findPnpFilewalks upward-only, so a.pnp.cjsshipped inside a dependency is unreachable — there is no live bug. But nothing asserts that: a future change that started the search from a package directory couldrequire()a package-supplied.pnp.cjsand execute untrusted code in-process.This adds an explicit, fail-closed guard: if the resolved PnP runtime lives within
node_modules,loadPnpApithrows a clear diagnostic instead of loading it. Exposed as a pureisPnpRuntimeWithinNodeModulespredicate for direct testing.2. Regression test for the non-suppressible permit-all banner
The
intent.skills: ["*"]permit-all risk banner must stay visible even under--no-notices/INTENT_NO_NOTICES(a risk acknowledgment a CI run can mute is not a safeguard). The fix already shipped; this locks it with a test asserting the banner prints through the notices channel under suppression, other notices stay suppressed, and it never leaks into the warnings channel.Impact / compatibility
.pnp.cjslives at the project/workspace root, not innode_modules).node_modules— precisely the untrusted case, and one the current upward-only search doesn't produce.Summary by CodeRabbit
Bug Fixes
node_modules.Tests