Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import locale
import logging
import os
import sys
import uuid
from collections.abc import Mapping
Expand All @@ -42,6 +43,8 @@
)
from agent_framework._workflows._state import State

from .._models import _safe_mode_context # type: ignore[reportPrivateUsage]

try:
from powerfx import Engine
except (ImportError, RuntimeError):
Expand Down Expand Up @@ -714,6 +717,13 @@ def _to_powerfx_symbols(self) -> dict[str, Any]:
# Custom namespaces
**state_data.get("Custom", {}),
}
# Expose ``Env`` ONLY when the active workflow factory has explicitly
# opted out of safe mode. Matches the policy enforced by
# ``_try_powerfx_eval`` in ``_models.py`` and the AgentFactory
# ``safe_mode`` flag. Treat the default as safe even when no factory
# is in scope (e.g. in unit tests) so opt-in is required.
if not _safe_mode_context.get():
symbols["Env"] = dict(os.environ)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have we thought about scoping this to only the env vars the YAML references? dict(os.environ) hands every PowerFx expression a full snapshot, so any expression in any action of an opted-in workflow can read AZURE_CLIENT_SECRET, OPENAI_API_KEY, etc. Documented as trust-the-YAML, but a single =Concat(Env.SOME_SECRET, ...) slipping into a logged field or a tool argument now exfiltrates. Could we parse referenced Env.X names up front and expose only those, or wrap in a lazy view that records access for audit?

# Debug log the Local symbols to help diagnose type issues
if local_data:
for key, value in local_data.items():
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
)

from .._loader import AgentFactory
from .._models import _safe_mode_context # type: ignore[reportPrivateUsage]
from ._declarative_builder import DeclarativeWorkflowBuilder
from ._errors import DeclarativeWorkflowError
from ._http_handler import HttpRequestHandler
Expand Down Expand Up @@ -93,6 +94,7 @@ def __init__(
max_iterations: int | None = None,
http_request_handler: HttpRequestHandler | None = None,
mcp_tool_handler: MCPToolHandler | None = None,
safe_mode: bool = True,
) -> None:
"""Initialize the workflow factory.

Expand All @@ -119,6 +121,15 @@ def __init__(
for a default backed by :class:`agent_framework.MCPStreamableHTTPTool`,
or supply your own implementation to enforce SSRF guards, allowlisting,
or auth/connection resolution.
safe_mode: Whether to run in safe mode, default is True.
When safe_mode is True, environment variables are NOT accessible from
PowerFx expressions in the workflow YAML (e.g. ``=Env.SOME_VAR`` will
fail to resolve and the original expression string is preserved).
When safe_mode is False, the full ``os.environ`` snapshot is exposed
via the ``Env`` symbol in every PowerFx evaluation. Set safe_mode to
False ONLY when you fully trust the YAML source. The flag is also
forwarded to the internally-constructed :class:`AgentFactory` so
inline agent definitions follow the same policy.

Examples:
.. code-block:: python
Expand Down Expand Up @@ -152,7 +163,8 @@ def __init__(
env_file=".env",
)
"""
self._agent_factory = agent_factory or AgentFactory(env_file_path=env_file)
self.safe_mode = safe_mode
self._agent_factory = agent_factory or AgentFactory(env_file_path=env_file, safe_mode=safe_mode)
self._agents: dict[str, SupportsAgentRun | AgentExecutor] = dict(agents) if agents else {}
self._bindings: dict[str, Any] = dict(bindings) if bindings else {}
self._tools: dict[str, Any] = {} # Tool registry for InvokeFunctionTool actions
Expand Down Expand Up @@ -338,6 +350,15 @@ def create_workflow_from_definition(
# Validate the workflow definition
self._validate_workflow_def(workflow_def)

# Set safe_mode context before evaluating any PowerFx expressions. The
# contextvar gates ``Env`` exposure inside ``DeclarativeWorkflowState``
# symbols and inside ``_try_powerfx_eval``; both check
# ``_safe_mode_context.get()`` at evaluation time. Because the
# contextvar propagates through asyncio tasks spawned from the current
# context, this value persists into ``workflow.run(...)`` invocations
# made on the same coroutine.
_safe_mode_context.set(self.safe_mode)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we be holding the reset token here? ContextVar.set without .reset(token) mutates the calling context for the rest of its life. If a process creates factory A with safe_mode=False, then factory B with safe_mode=True, then runs workflow A, the PowerFx evaluation reads B's value (True) and Env.* is silently dropped.

The comment promises propagation into workflow.run(...) on the same coroutine, but it also propagates into every subsequent create_workflow_from_definition call, regardless of which factory invokes it. Same shape exists in _loader.py already, so this PR isn't introducing the pattern, but it widens the surface to a new public flag.

Could we instead stash safe_mode on the workflow object and .set it inside workflow.run with a try/finally reset, or run evaluation inside contextvars.copy_context().run(...)?


# Extract workflow metadata
# Support both "name" field and trigger.id for workflow name
name: str = workflow_def.get("name", "")
Expand Down
Loading
Loading