Skip to content

Refuse path-trust migration from corrupt stores - #995

Open
TheGreatAxios wants to merge 3 commits into
mainfrom
cl-6722-do-not-auto-migrate-path-trust-grants-from-corrupt-stores
Open

Refuse path-trust migration from corrupt stores#995
TheGreatAxios wants to merge 3 commits into
mainfrom
cl-6722-do-not-auto-migrate-path-trust-grants-from-corrupt-stores

Conversation

@TheGreatAxios

Copy link
Copy Markdown
Collaborator

Summary

  • Path-trust migration seeds grants only when the store file was never created; a corrupt store now refuses to seed and returns no grants
  • Corrupt stores log a repair warning pointing at delete-to-reseed or explicit re-consent, and path plugins load metadata-only until repaired

Verification

  • bun run check passes (lint, typecheck, build, projects-dir-guard, 7298 tests, 0 fail)
  • New revoke-corrupt-migrate regression test fails before the fix and passes after

Fixes CL-6722

@linear-code

linear-code Bot commented Sep 13, 2026

Copy link
Copy Markdown

CL-6722

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Critic · Comment (no blockers)

Refuses path-trust auto-seeding from corrupt stores and leaves the file untouched so path plugins stay metadata-only until repair or re-consent.

Findings

  • src/trust/path-trust.ts:210 — early return on invalid before any resolveMembers/savePathTrust/trustPathPlugins call is correct and complete: no grant, no overwrite, file stays invalid. Revoke-then-corrupt cannot re-grant. Genuinely fine.
  • src/session/assemble-runtime.ts:178 — sole caller passes onMigrated: reportPathTrustMigration through and needs no change; the early return structurally skips it. Verified, no diff needed.
  • tests/unit/path-trust.test.ts — zero-byte, malformed, and revoke-then-corrupt refusal tests pin isTrusted == false, empty grants, resolveCalls == 0, and the store file still invalid (no save). Red is real by inspection: pre-fix the invalid state falls through to resolve-and-grant, so each new assertion fails without the fix. Ran bun test tests/unit/path-trust.test.ts on-branch: 16 pass, 0 fail.
  • tests/unit/path-trust.test.ts — should-fix, not blocking: none of the three refusal tests pass opts.onMigrated, so a regression that fires the one-time seeding notice on refusal would go uncaught. Add an onMigrated spy asserting zero calls to each refusal test.

Notes

  • src/trust/path-trust.ts:211 — nit: the refusal warning also fires when pluginPaths is empty (nothing would seed anyway). Harmless; consider checking length first if the noise matters.
  • src/trust/path-trust.ts:178 — nit: the doc lead "when no valid store file exists yet" now covers two behaviors (missing seeds, corrupt refuses). The new paragraph below it is accurate, so this is just the lead sentence going slightly stale.
  • Hygiene: no dead code, no signature drift (migratePathTrustFromPluginPaths keeps its sync-shape/params/return), scope stays inside path-trust plus its tests. Do not merge per brief.

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

[warden trust review] Trust verdict: APPROVE (fail-closed). The corrupt-store refusal is airtight on the reviewed path.

Findings (all checked against src/trust/path-trust.ts in worktree cl-6722):

  • path-trust.ts:210-213 — revoke->corrupt->migrate truly does not re-grant. The invalid branch returns emptyStore() before resolveMembers, before savePathTrust, before onMigrated. No grant, no save, no callback, no resolver side effects (test asserts resolveCalls === 0, store stays invalid).
  • path-trust.ts:211 — refusal leaves the file untouched, so a second launch re-reads invalid and refuses again. No latch into a granted state; corrupt stays corrupt until the user acts. Fail-closed, stable across restarts.
  • Repair path is safe: delete-to-reseed requires filesystem write to the trust dir, which is the same privilege as writing grants directly — no escalation. Explicit re-consent (trustPathPlugin(s) via tui/plugins-admin-backend.ts:170,319) heals the file through the same module, which is the intended recovery. Delete-vs-consent distinction is stated in the doc comment (path-trust.ts:193-198) and the warn text.
  • No grant/token leakage: the warn logs only the store path, never grant contents (path-trust.ts:211).
  • No alternate writer bypass: the only writers of path-plugins.json are savePathTrust, reached via trustPathPlugins, revokePathPlugin, and the missing-store migrate leg. grep shows no other module writes that path; uninstall revocation funnels through the injected revokePathPlugin (plugins/uninstall.ts:272).
  • Empty-store => metadata-only confirmed: assembleSessionTrust (session/assemble-runtime.ts:186-187,196) builds isRegisteredPathTrusted from the returned (empty) store, and loadPluginsFromPaths (plugins/loader.ts:773) loads untrusted path origins metadata-only (never import()s). EACCES-unreadable stores now return empty without a throwing save attempt — strictly more fail-closed than before.
  • Genuinely fine: arktype boundary intact (PathTrustStoreSchema validated on every load, requireAbsolute on grant, relative pluginPaths dropped at migrate path-trust.ts:222-228).

File-for-later (pre-existing, NOT introduced here, not blocking): cross-process TOCTOU on the missing-store leg — migrate reads missing, awaits slow resolveMembers, then trustPathPlugins re-reads and writes, so a revoke landing in that window could be re-granted. The new invalid branch has no such window (single read, synchronous return). Suggest a kept test for the missing-leg race if the team ever serializes it cross-process.

Not covered (so the parent does not assume closed): provider-auth and permission-gate code are untouched by this diff (touches only src/trust/path-trust.ts + its unit test); loader metadata-only enforcement was spot-checked, not exhaustively reviewed.

Tests: bun test tests/unit/path-trust.test.ts — 16 pass, 0 fail. Kept scenarios worth keeping: revoke-then-corrupt refusal with resolver-uncalled assertion, zero-byte refusal, corrupt refusal with file-left-invalid assertion.

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Critic follow-ups addressed in 6fe3571 (pushed, not merging):

  • onMigrated zero-call spies added to all three refusal tests (zero-byte, corrupt, revoke-then-corrupt) — each now asserts the one-time seeding notice never fires on refusal.
  • Refusal warning gated on pluginPaths being non-empty, so corrupt store + empty settings stays silent (refusal semantics unchanged: still returns empty, file untouched).
  • Doc lead tightened to missing-only ('when the store file is missing (first launch)'); the invalid-refusal paragraph below now owns the corrupt case.

Verify: bun test tests/unit/path-trust.test.ts 16 pass, 0 fail; bun run check exit 0.

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Warden follow-up noted in 6fe3571 (pushed, not merging): the cross-process TOCTOU on the missing-store leg is pre-existing and intentionally left as-is per the file-for-later note — out of scope for this refusal fix. The new invalid branch is unaffected (single read, synchronous return, no resolver/save/callback window). Verify: bun test tests/unit/path-trust.test.ts 16 pass, 0 fail; bun run check exit 0.

@TheGreatAxios

Copy link
Copy Markdown
Collaborator Author

Critic re-check · Approve

6fe3571 resolves all three follow-ups with refusal semantics intact.

Findings

  • tests/unit/path-trust.test.ts:251,280,312 — onMigrated zero-call spies in all three refusal tests pass; the one-time notice structurally cannot fire (return precedes trustPathPlugins/onMigrated).
  • src/trust/path-trust.ts:210-214 — warn gated on pluginPaths.length > 0; the return emptyStore() sits outside the gate so refusal is unchanged. Temp test confirmed corrupt + empty paths refuses silently (empty grants, no resolve, no callback, file stays invalid).
  • src/trust/path-trust.ts:177 — doc lead now missing-only; corrupt case owned by the invalid paragraph below.

No findings. bun test tests/unit/path-trust.test.ts 16 pass, 0 fail. No commits, no merge per brief.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant