Skip to content

fix(engine): enforce access checks before protected root execution - #154

Open
Asaf-prog wants to merge 4 commits into
mainfrom
codex/fix-151-protected-root
Open

Asaf-prog wants to merge 4 commits into
mainfrom
codex/fix-151-protected-root

Conversation

@Asaf-prog

@Asaf-prog Asaf-prog commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Protected root agents and orchestrators could execute even when the access resolver denied access, because filtering only covered child delegation. Apply the existing policy before entering a protected root executor, preventing prompt resolution, model calls and tool execution on denial. Missing access plugins also fail at startup for direct Python callers.

Denied new runs return Access denied. with an empty route and emit the refusal through the existing streaming answer sink.

Approval replay now uses the current verified decision request's authorization facts. The manager supplies a fresh RunContext on both approval paths; root entry replaces the execution-local context for root and child checks instead of reading saved grants from the checkpoint. Revoked roles/scopes/organization/metadata and omitted fresh permission facts cannot inherit the original grant. Caller/run/session conflicts are rejected before claiming the approval. The reviewed tool request and completed replay steps remain bound to the checkpoint.

Local chat also forwards its host-supplied RunContext when approving a tool in both streaming and non-streaming console modes. Current role/scope/metadata facts reach replay; changing a permission at the approval prompt still prevents the tool effect.

Compatibility:

  • Python resume / resume_stream gain an optional context: RunContext | None keyword. External implementations of the optional approval ports must accept it; hosts must supply trusted current facts. ADR 0002 records the contract.
  • YAML, access-plugin signatures, HTTP bodies and SSE event types are unchanged. Credentials stay in request-local context and are excluded from graph projections.
  • A denied resumed root follows the existing approval mismatch error path. A filtered child may trigger the existing checkpoint mismatch error path (sanitized HTTP 500 / SSE error); both prevent the pending tool effect and fail the run.
  • Current main is merged and the task-index conflict is resolved.

Validation (2026-10-24):

  • make check: 1,546 tests passed; formatting (363 files), lint, mypy (359 files) and generated-stub checks passed, with 27 dependency warnings.
  • make validate and agentctl validate examples/whatsapp-pilot/agents.yml: all three examples passed.
  • git diff --check: passed.
  • The two protected-access files contain 79 regression cases, including 40 added for the review follow-up: real signed manager HTTP requests, root agents/orchestrators and protected children, revoked/current roles, missing fresh context, scope/metadata/organization replacement, owner binding and concurrent resume isolation.
  • Before correction, six HTTP revoked-role cases executed the local tool and two omitted-context engine cases reused saved roles. Still-authorized cases continue to execute the reviewed tool once.
  • Eight additional real-engine local-chat cases cover both root kinds, both console modes, permission revocation at the approval prompt, exactly-once allowed execution and credential exclusion. All eight failed before the local-context forwarding correction and pass afterwards.
  • Post-implementation code review completed. Tests use fake models/providers and temporary local storage; no live provider or identity deployment was exercised.

Fixes #151

Apply the existing access policy before protected root agents or
orchestrators resolve prompts, call models, or execute tools. Fail closed
when the access plugin is missing, emit refusals through the stream sink,
and recheck access during tool-approval replay.

Add HTTP and engine regressions plus access-policy documentation.

Fixes #151
@Asaf-prog
Asaf-prog requested a review from AmitAvital1 October 22, 2026 16:54
@Asaf-prog Asaf-prog self-assigned this Oct 22, 2026
# same policy before entering either kind of protected executor.
candidates = [node.node]
for route_filter in filters:
candidates = route_filter.filter(state.get("run_context", {}), candidates)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On resume, this reads the run_context saved in the original checkpoint instead of the approver’s current verified principal.
I reproduced a user starting with role=admin, losing that role before approval, and the tool still running because the old role remains in state.
This means role/scope revocation is not enforced; the resumed check needs fresh, verified authorization context.

Resolve the task-index conflict while retaining protected-root and upstream
task entries. Verified the combined tree with make check (1,498 tests).
Carry the current verified principal through both approval ports and use
its request-local permission facts for protected root and child checks.
Do not inherit checkpointed roles, scopes, organization or metadata.
Validate caller/run/session consistency before claiming the approval and
keep credentials out of graph projections.

Add role revocation, missing context, permission facts, ownership and
concurrent resume regressions. Document the additive Python port keyword.

Fixes #151
caller_user_id: str | None = None,
caller_session_id: str | None = None,
access_token: str | None = None,
context: RunContext | None = None,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agentctl.chat._resolve_local_approvals() still calls this port without context, even though it already receives the current RunContext.
I reproduced a protected root allowed by role=admin pausing for approval; choosing allow in local chat fails with ApprovalInvocationMismatch because resume sees no roles.
Please forward the context there and cover both local stream and non-stream paths, otherwise this regresses local HITL for context-based policies.

Pass the local chat RunContext through the approval port so protected
roots retain current role, scope and metadata facts in both console modes.
Add real-engine regressions for allowed and revoked permissions without
exposing credentials or weakening fail-closed replay checks.

This branch has not been deployed

No deployments
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.

[Security][P1] : Enforce Caller Identity and Authorization for Protected Agent Nodes

2 participants