Skip to content

fix(permission-management): harden permission lifecycle races - #555

Merged
ewen-poch merged 2 commits into
mainfrom
fix/permission-management-hardening
Jul 31, 2026
Merged

fix(permission-management): harden permission lifecycle races#555
ewen-poch merged 2 commits into
mainfrom
fix/permission-management-hardening

Conversation

@ewen-poch

Copy link
Copy Markdown
Member

Problem

The merged permission-management work in #533 left several time-of-check/time-of-use gaps:

  • a Connector call could still dispatch after its policy changed from Ask to Block while approval was pending, or while the in-memory settings projection lagged the durable Block;
  • a grant revoked during an asynchronous scope-liveness check could still authorize the stale request;
  • deleting a Compute provider could race the persistence tail of an already-approved grant, and a stale Once request could arrive after deletion completed;
  • startup reconciliation truncated valid SSH aliases containing / and pruned their grants;
  • the Undo snackbar paused its UI timer during interaction without extending the authoritative 8-second receipt.

Proposed change

  • Re-read durable Connector settings at every authorization/custom lookup boundary before discovery or dispatch.
  • Revalidate the exact cached grant revision after scope-liveness checks.
  • Drain tracked Compute approval operations while provider deletion is in progress, revalidate every non-deny decision against the immutable owner, then prune grants before releasing the deletion barrier.
  • Parse the final Compute capability segment as the operation so the full provider id is retained.
  • Preserve hover/focus pause by periodically extending the authoritative in-memory Undo receipt; remove the action if renewal fails or the receipt has expired.

Scope and non-goals

  • No database schema, data model, or permission-scope changes.
  • One narrow permissions:extend-undo IPC method renews only the existing in-memory receipt; no new architectural component or persistent state is introduced.
  • No redesign of the permission UI. Hover/focus continues to pause auto-dismiss as specified in feat(permission-management): add scoped permission management #533, now with matching backend validity. Non-restorable explanations pause locally without attempting receipt renewal.

Acceptance criteria and validation

All commands below ran after the final material edit.

  • Connector policy/availability uses durable state, grant revoke cannot release stale authority, Compute deletion drains and rejects stale owners, slash-containing provider ids survive reconciliation, and interactive Undo receipts remain authoritative -> npm test -- --run src/main/connectors/service.test.ts src/main/permission-grants/registry.test.ts src/main/permission-grants/reconciliation.test.ts src/main/permission-grants/ipc.test.ts src/main/compute/compute-approval-broker.test.ts src/main/compute/ipc.test.ts src/renderer/src/stores/permission-grants-store.test.ts src/renderer/src/components/PermissionUndoSnackbar.test.tsx src/main/acp/permission-broker-registry.test.ts -> 9 files passed, 161 tests passed.
  • Node and renderer type safety -> npm run typecheck -> passed.
  • Repository lint gate -> npm run lint -> passed with 0 errors (23 pre-existing warnings outside this change).
  • Web API projection remains current -> npm run check:web-api-map -> passed.
  • Full regression suite -> npm test -> 591 files passed, 11 skipped; 8,891 tests passed, 140 skipped.
  • Patch hygiene -> git diff --check -> passed.

Uncovered risk: the policy-race coverage uses deterministic unit seams rather than a packaged Electron end-to-end interaction. The same production settings read and dispatch boundary are exercised directly.

An independent review of the final diff confirmed this behavior-to-check mapping.

Review focus

  • Connector policy precedence across every asynchronous boundary before external discovery/dispatch.
  • Compute provider invalidation ordering and failure cleanup.
  • The deliberate alignment of Undo visibility with the authoritative receipt expiry.

Refs #533.

@github-actions github-actions Bot added the bug Something isn't working label Jul 31, 2026
Comment thread src/main/connectors/service.ts Fixed
@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Prevent invalidation from missing a request created after the pending sweep

src/main/compute/compute-approval-broker.ts:165

Impact: If provider invalidation starts while the durable grant lookup is still awaiting, this operation can create a new pending approval after invalidateProvider has already swept pending requests. invalidateProvider then waits forever on this in-flight promise, so host deletion hangs before repository.delete and the invalidation completion cleanup.

Recommendation: Re-check the provider generation/invalidatingProviders immediately before calling request(), or make invalidation cancel/drain requests created after the initial sweep so every operation settles during deletion.

Summary: Static inspection found one merge-blocking compute deletion race. Branch and pull request title checks are valid.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed the compute invalidation race in 36368f0. The broker now re-checks both the invalidation barrier and the provider generation immediately before creating a pending approval. If invalidation starts during an asynchronous grant lookup, the operation fails closed and settles, so invalidateProvider can finish draining. A deterministic regression test covers that exact ordering. Validation: 58 targeted tests passed, typecheck passed, lint completed with 0 errors, and the full suite passed with 8,892 tests.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the pull request changes.

@github-actions github-actions Bot added the ready-to-merge All completed AI reviewers found this pull request mergeable. label Jul 31, 2026
@ewen-poch
ewen-poch merged commit f6a0e70 into main Jul 31, 2026
18 checks passed
@ewen-poch
ewen-poch deleted the fix/permission-management-hardening branch July 31, 2026 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready-to-merge All completed AI reviewers found this pull request mergeable.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant