fix(templating): (T1) close two plugin-registry trust-cache gaps - #10326
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
✅ Circular References ReportGenerated at: 2026-08-04T17:23:15.633Z Summary
Click to view all circular references in PR (9)Click to view all circular references in base branch (9)Analysis✅ No Change: This PR does not introduce or remove any circular references. This report was generated automatically by comparing against the |
- pluginConfig.elevated is keyed by declared plugin name, not by folder; a same-named folder placed alongside an already-elevated plugin inherited its trust grant and ran in-process before any collision was even noticed. traversePluginPath now pre-scans for duplicate names (order-independent) and refuses to load any colliding folder. - applyRequestHooks/applyResponseHooks tagged a caught error with `error.plugin = plugin`, a plain assignment that a plugin-thrown Error could intercept via its own `plugin` property setter, handing the hook a live, mutable reference to its own cached registry entry and letting it flip `directory`/`config.elevated` to defeat later sandboxing. Switched to Object.defineProperty, which bypasses any such setter.
A bare .startsWith(base) on a resolved path accepts a sibling directory whose name happens to prefix-match the base (e.g. /plugins-evil vs /plugins). Added a shared isContainedIn helper (path.relative, rejects .. or an absolute result) and applied it to both the existing plugin-path containment check and the new duplicate-name pre-pass.
5c5b467 to
7d0dd73
Compare
|
7d0dd73 — replaces a |
jackkav
left a comment
jackkav
left a comment
There was a problem hiding this comment.
Reviewed — approving. Both fixes close real gaps in the T1 trust logic:
-
Name-collision trust inheritance.
pluginConfig.elevatedis keyed by plugin name, and load was order-dependent (last-writer-wins inpluginMap), so a colliding folder could shadow an elevated name and run in-process. The order-independentfindDuplicatePluginNamespre-pass + fail-closed refusal is the right fix; the pre-pass walk mirrorstraversePluginPath's (sameisContainedIn, same'insomnia' in pluginJsongate) so it can't disagree with the real pass. Reading package.json in the pre-pass runs no plugin code. Also nice catch upgrading thestartsWith(base)containment check to apath.relativeone (/plugins-evilno longer prefix-matches/plugins). -
Cached-object leak via a
pluginsetter trap.(error as any).plugin = pluginwould invoke a plugin-defined setter, handing plugin code a live reference to its own cached registry entry to flipdirectory/elevated.Object.defineProperty(data descriptor) bypasses the setter; the try/catch keeps it best-effort. Correct.
Tests are properly adversarial (prefix-sibling + .. escape for containment, an unrelated folder failing to inherit an elevated name, and the setter-trap not firing with the cached object left unmutated). CI green across all shards.
One deliberate behavior change worth noting for the changelog: a name claimed by two discoverable folders is now refused entirely (with a warning) rather than resolved last-wins. That's the correct fail-closed posture for an elevated-trust key, just a change from the previous silent dedup.
🤖 Reviewed by Claude Code
d0d7d3d
into
claude/sandbox-pr12-trust-flip
2 participants
What this PR does
pluginConfig.elevatedis keyed by plugin name, not folder. A same-named folder placed alongside an already-elevated plugin inherited its trust grant and ran in-process before any collision was noticed. Now detected via an order-independent pre-pass; colliding folders are refused.applyRequestHooks/applyResponseHookstagged caught errors witherror.plugin = plugin, a plain assignment a plugin-thrown Error could intercept via its own setter, handing the hook a live reference to its own cached registry entry and letting it flip directory/elevated to defeat later sandboxing. Switched to Object.defineProperty, which bypasses any such setter.