security(pi): require Pi 0.79+ and document project trust - #1291
Conversation
backnotprop
left a comment
There was a problem hiding this comment.
Security review: Pi 0.79.1 floor + project-trust gating
TLDR: The change is correct, minimal, and does what it claims. The single read path of <cwd>/.pi/plannotator.json is gated on Pi's resolved trust decision, the 0.79.1 floor claim is exact (verified against Pi source history), the documented trust matrix matches Pi 0.79.1's implementation, and the four Dependabot alerts genuinely correspond and should close. One SHOULD FIX: there is no runtime guard for hosts older than 0.79.1, so a Pi 0.74-0.79.0 user who updates the extension gets a cryptic ctx.isProjectTrusted is not a function extension error and a silently degraded session instead of a clear "update Pi" message. That, plus the intentional noninteractive behavior change, needs a release-notes callout. The one red CI job is an unrelated infra flake. Merge-ready once the old-host experience is addressed or explicitly accepted and documented.
SHOULD FIX
1. No runtime capability check for pre-0.79.1 hosts; the failure mode is confusing and over-broad.
apps/pi-extension/index.ts:1604-1606 calls ctx.isProjectTrusted() unconditionally. The peer range cannot prevent this situation: Pi installs npm extensions with a plain npm install -g (Pi core/package-manager.ts, installNpmBatch) and never compares an extension's peer range against the running host version, and npm peer ranges warn rather than block anyway. On Pi 0.74.0-0.79.0 the call throws TypeError, Pi's runner catches it per handler (runner.ts catch around the handler loop, present since 0.74.0) and surfaces a generic extension error. The security outcome is fail-closed (project config is never read), which is good. But the blast radius is wider than the project file: plannotatorConfig stays {} (index.ts:285), so the bundled internal config and the user's global ~/.pi/agent/plannotator.json also never load, the --plan flag is ignored (index.ts:1613), and resyncPhaseFromSession never runs, so session resume loses phase state. The user sees "is not a function", not "your Pi is too old and carries known CVEs".
Recommend a narrow guard at the call site:
const trustFn = ctx.isProjectTrusted;
const projectTrusted = typeof trustFn === "function" ? trustFn() : false;
if (typeof trustFn !== "function") {
ctx.ui.notify("Plannotator requires Pi 0.79.1 or newer. Update Pi; project-local config is disabled on this host.", "warning");
}This stays fail-closed for the repo-controlled file, keeps bundled and global config working, and turns the breakage into an actionable message. If the maintainer instead wants old hosts to break hard (defensible, since those hosts carry the CVEs), keep the hard failure but throw an explicit Error("Plannotator requires Pi >= 0.79.1 ...") so the surfaced extension error is self-explanatory. Either way this belongs in the release notes (see below).
CONSIDER
2. Mark the manifest floor pins as deliberate.
apps/pi-extension/startup.test.ts:69-87 pins >=0.79.1 with toBe. Per the repo Testing Rules, deliberate pins should say so in a comment. The PR body's rollback instruction ("never below 0.79.1; the trust guard depends on the 0.79.1 extension API") is exactly the rationale that should live next to the assertion, so a future range edit trips the test and immediately explains itself.
VERIFIED-OK
- Every read is gated.
loadPlannotatorConfigis the only reader of.pi/plannotator.json(apps/pi-extension/config.ts:261-264in the PR) andindex.ts:1604is its only caller. There are no mid-session re-reads to miss: Pi 0.79.1 resolves trust once at startup and requires a restart for changes (interactive warning "Use /trust ... then restart pi",/trustsave message "Restart pi for this to take effect"), andsession_startrefires on/newand session replacement, so a session_start-only read matches host semantics exactly. - Fail-closed API shape. The new
options: LoadPlannotatorConfigOptionsparameter is required, not optional-defaulting-to-true (config.ts:240-243), so any future call site is forced to supply a trust decision. Untrusted resolution returns{ config: {}, warnings: [] }and still merges bundled + global config; only the repo-controlled layer is dropped. Correct trust boundary. - The 0.79.1 claim is exact. In Pi's history, internal trust gating lands in 0.79.0 (
89a92207f), butctx.isProjectTrusted()for extensions lands indb3f9953e("expose project trust to extensions", closes #5523), whose first containing tag is v0.79.1. Requiring 0.79.1 rather than 0.79.0 is right. - The documented trust matrix matches Pi 0.79.1 source (
core/project-trust.ts,cli/args.ts:178-183):--approve/-aand--no-approve/-naoverride saved decisions; saved store decision next;defaultProjectTrustalways/never/ask; ask without UI resolves false (noninteractive default deny); ask with UI shows the startup trust prompt with save. README wording is accurate. - No bypass via "no trust inputs". Pi short-circuits to trusted when a project has no trust inputs, but
hasProjectTrustInputsreturns true whenever a.pidirectory exists (trust-manager.ts,hasProjectConfigDir). A repo carrying.pi/plannotator.jsonnecessarily has a.pidir, so it can never reach the implicit-trust path. The gate cannot be sidestepped by a minimal hostile repo. - Silent skip is consistent with the host. Pi itself renders "This project is not trusted. Project .pi resources and packages are ignored" in interactive sessions, so Plannotator adding its own untrusted-skip notice would be redundant.
- Tests guard behavior, not prose. The new tests assert observable outcomes with sentinels: untrusted project config must not leak into the phase framing message (
phase-prompts.test.ts:374-385, asserts absence ofuntrusted-project-instructions) and executionMode must fall back to the global layer (config.test.ts:65-81). Env hygiene is clean:HOMEis mutated inside tests and restored inafterEach(config.test.ts:8, 16-26); no module-scope mutation was added. The harness mocks (external-execution,phase-tools-runtime,todo-provider-sync) addisProjectTrusted: () => truecoherently, andstale-ctx.test.ts:88-91correctly wraps it inassertActive()to extend the existing stale-context contract. - Lockfile delta is metadata-only.
bun.lockchanges only the declared workspace range strings; resolved Pi artifacts remain 0.79.1 (bun.lock:528-534), matching the PR's "no resolution changes" claim. - Scope is tight. 11 files plus lockfile, all in
apps/pi-extension, no dependency churn, docs match code. Nothing beyond the stated goal.
Security
What a hostile .pi/plannotator.json could previously do (severity assessment for the record): the schema carries no filesystem paths, commands, or URLs (config.ts normalizers), so this was never direct code execution. But it is more than cosmetic:
phases.*.instructions/defaults.instructionsare delivered verbatim as phase framing conversation messages: a repo-controlled prompt-injection channel into the agent.activeToolsreplaces the built-in planning tool list (index.ts:468-489), so an untrusted repo could re-enable editing or shell tools during plan mode, undermining the phase restriction that is Plannotator's core promise.model/thinkingcan redirect phases to any model in the user's configured registry (cost or capability manipulation, no new endpoints reachable).executionMode: "external"silently changes the execution workflow.
Moderate severity, real, and previously outside Pi's own project-resource trust gate because Plannotator reads the file manually. Gating it on ctx.isProjectTrusted() closes that bypass and aligns exactly with the upstream trust boundary. Untrusted resolution is fail-closed by construction.
Dependabot alerts #9-#12: verified via the API. All four target @earendil-works/pi-coding-agent in apps/pi-extension/package.json (development scope) and match the PR body's GHSAs exactly: #9 GHSA-7v5m-pr3q-6453, #10 GHSA-r95r-rj6r-c39x, #11 GHSA-jfgx-wxx8-mp94 (all patched 0.78.1), #12 GHSA-mqxh-6gq7-558m (patched 0.79.0). The lockfile already resolves 0.79.1, so the open alerts key on the declared >=0.74.0 floor admitting vulnerable versions; raising it to >=0.79.1 should close all four. To be precise about what is and is not fixed: this repairs our manifest's advertised compatibility and what fresh installs resolve; the actual runtime exposure lives in the user's separately installed Pi host, which only a Pi update repairs. The PR body states this honestly.
Behavior change for legitimate users (intentional, needs visibility): noninteractive sessions (CI, agents, RPC) with no saved trust decision previously had .pi/plannotator.json honored, because Plannotator read it outside Pi's loader. After this PR it is ignored by default; such users must start Pi with --approve or save a trust decision. Correct security posture, but repo-local config silently stops applying.
Release-notes callout (recommended)
The next release's notes should carry an explicit Pi section, like the v0.27.0 command rename did:
- Plannotator now requires Pi 0.79.1+; Pi 0.74-0.79.0 hosts will show an extension error at session start and lose phase configuration until Pi is updated (message improves if SHOULD FIX 1 lands).
.pi/plannotator.jsonnow follows Pi's project trust: noninteractive sessions ignore it unless trust was saved or Pi runs with--approve;--no-approvedisables it for a run.- Why: Pi hosts below 0.79.1 carry GHSA-7v5m-pr3q-6453, GHSA-r95r-rj6r-c39x, GHSA-jfgx-wxx8-mp94, GHSA-mqxh-6gq7-558m; update Pi itself, not just the extension.
CI
All jobs green except smoke-binaries (windows-latest), which failed in the agent-terminal runtime install step with a download timeout ("Skipping agent terminal runtime install (timed out)" then FAIL). That path is untouched by this PR (which changes only apps/pi-extension); the review and annotate server smokes in the same job passed. Infra flake; re-run before merge.
AI-assisted review (Claude, Fable 5) under maintainer direction.
|
Addressed the SHOULD FIX in 5481d51.
Validation: focused review tests 26/26, full Pi extension suite 218/218, typecheck passed, and |
Summary
Raise the published Pi host/support floor from
>=0.74.0to>=0.79.1, keep the already-resolved 0.79.1 artifacts unchanged, and make Plannotator honor Pi's resolved project-trust decision before reading<cwd>/.pi/plannotator.json. A runtime capability guard gives older hosts an actionable update warning while keeping project configuration fail-closed.The lockfile was already on the Pi 0.79.1 family, so Plannotator's development and release checks were using patched code. The published peer range was still security-relevant, however: it advertised compatibility with vulnerable Pi hosts installed on user machines. Updating only
@plannotator/pi-extensioncannot repair an older host.Threat model and advisory coverage
This sets the support floor above the fixes for all four open Pi Dependabot alerts:
auth.jsonwrites that could expose credentials, patched in 0.78.1.Pi 0.79.0 introduced the project-trust lifecycle. This PR requires 0.79.1 specifically because that is the first release exposing
ctx.isProjectTrusted()to extensions. Plannotator is commonly loaded globally or through the CLI and manually reads<cwd>/.pi/plannotator.json; without the new guard, that project file was outside Pi's own project-resource loader and could still influence Plannotator after trust was denied.User-visible trust behavior
--approve/-a.--no-approve/-nadisables project inputs for that run, including when an allow decision was previously saved..pi/plannotator.json; global and bundled Plannotator configuration remain available.Changes
@earendil-works/pi-coding-agentpeer floor and the four coherent Pi development floors to>=0.79.1.ctx.isProjectTrusted()and fail closed with a clear update warning when an older host lacks that API.This intentionally does not incorporate the broad dependency churn in #1281.
Supply-chain audit
The dependency-update audit was completed for
pi-coding-agent,pi-agent-core,pi-ai, andpi-tuibefore changing their declared ranges:pi-tui's macOS/Windows native helper hashes match tagged source and the helpers have narrow documented behavior.That follow-up should also assess later advisories reported by
npm auditin the 0.79.1 Pi host graph (undici,brace-expansion,protobufjs, andws); the 0.84.1 host graph audited clean. These are not concealed or folded into this four-alert/floor-only PR, and the open-ended peer range already permits current Pi.Validation
bun install --frozen-lockfile— passed; no resolution changes.bun test apps/pi-extensionwith isolated Plannotator data — 218 passed, 0 failed.bun testin an isolated home — 3,375 passed, 496 skipped, 0 failed.bun run typecheck— passed, including Pi vendoring and the Pi extension TypeScript project.bun run build:pi— passed.bun run check:release-version— passed for 0.27.1.npm pack/tarball inspection — passed; the packed package declares peer@earendil-works/pi-coding-agent >=0.79.1and contains the expected runtime files.--approve=true,--no-approve=false, saved allow=true, saved deny=false, and command-line flags override saved decisions. The interactive prompt/save path was substantiated against upstream release/source behavior; CI/manual terminal testing may exercise the visible prompt.Release and rollback
Merging this PR does not publish the remediation. After CI and review, maintainers should issue a patch release containing the updated
@plannotator/pi-extension, verify the registry metadata/tarball, and confirm Dependabot alerts #9-#12 close against the published compatibility floor.Do not roll the peer/support floor back below 0.79.1 (and therefore never below 0.79.0). The trust guard depends on the 0.79.1 extension API; a package rollback must preserve that host floor even if other changes are reverted.