Repository navigation
fix(engine): enforce access checks before protected root execution #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4831142
af5cd36
7fe53b5
eb5a460
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| from __future__ import annotations | ||
|
|
||
| import inspect | ||
| import logging | ||
| from collections.abc import Callable, Mapping, Sequence | ||
| from pathlib import Path | ||
|
|
||
|
|
@@ -17,6 +18,8 @@ | |
| from agent_engine.core.spec import AgentSpec, BaseModelConfig, GraphNode, OrchestratorSpec | ||
| from agent_engine.core.spec import ModelConfig as NodeModelConfig | ||
| from agent_engine.engine.langgraph.checkpointing import CheckpointerHandle | ||
| from agent_engine.engine.langgraph.execution.approval_resume import current_approval_resume | ||
| from agent_engine.engine.langgraph.execution.node_executor import NodeExecutor | ||
| from agent_engine.engine.langgraph.filters import RouteFilter | ||
| from agent_engine.engine.langgraph.graph.traversal import node_id | ||
| from agent_engine.engine.langgraph.nodes.agent_node import AgentNode | ||
|
|
@@ -27,12 +30,15 @@ | |
| from agent_engine.loaders.resolver_loader import ResolverLoader | ||
| from agent_engine.loaders.tool_loader import ToolLoader | ||
| from agent_engine.runtime.hooks import HookManager | ||
| from agent_engine.runtime.hooks.context import current_run_context, public_run_context | ||
| from agent_engine.runtime.state import GraphState | ||
| from agent_engine.runtime.streaming import current_streams | ||
| from agent_engine.tool_usage.context_provider import ToolUsageContextProvider | ||
| from agent_engine.tool_usage.tracker import ToolUsageTracker | ||
|
|
||
| ModelFactory = Callable[..., BaseChatModel] | ||
| _MODEL_FACTORY_OPTIONAL_KWARGS = ("region", "max_tokens", "top_p") | ||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # noinspection PyTypeChecker | ||
| RunGraphBuilder = StateGraph[GraphState, None, GraphState, GraphState] | ||
|
|
@@ -141,13 +147,37 @@ def _wire_node( | |
| parent_path: str | None, | ||
| ) -> None: | ||
| path = node_id(node, parent_path) | ||
| executor: NodeExecutor | ||
| if isinstance(node.node, OrchestratorSpec): | ||
| orchestrator_node = self._build_orchestrator_node(node, parent_path) | ||
| builder.add_node(path, orchestrator_node.execute) | ||
| executor = self._build_orchestrator_node(node, parent_path) | ||
| else: | ||
| assert isinstance(node.node, AgentSpec) | ||
| agent_node = self._build_agent_node(node.node, path) | ||
| builder.add_node(path, agent_node.execute) | ||
| executor = self._build_agent_node(node.node, path) | ||
| filters = tuple(self._filters) | ||
|
|
||
| async def execute_root(state: GraphState) -> GraphState: | ||
| if current_approval_resume.get() is not None: | ||
| ctx = current_run_context.get() | ||
| # Checkpoints retain the reviewed work, not current authority. | ||
| # Replace facts locally for the root and all delegated children. | ||
| state = {**state, "run_context": public_run_context(ctx) if ctx else {}} | ||
| if not node.node.protected: | ||
| return await executor.execute(state) | ||
| # The root has no parent to filter it before delegation. Apply the | ||
| # 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On resume, this reads the |
||
| if not candidates: | ||
| logger.warning("access denied for protected root node '%s'", path) | ||
| answer = "Access denied." | ||
| sink = current_streams.get().answer | ||
| if sink is not None: | ||
| sink(answer) | ||
| return {"visited": [], "answer": answer} | ||
| return await executor.execute(state) | ||
|
|
||
| builder.add_node(path, execute_root) | ||
| builder.add_edge(path, END) | ||
|
|
||
| def _build_orchestrator_node( | ||
|
|
||
There was a problem hiding this comment.
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 withoutcontext, even though it already receives the currentRunContext.I reproduced a protected root allowed by
role=adminpausing for approval; choosing allow in local chat fails withApprovalInvocationMismatchbecause 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.