From 7823b1732ea388cbe50b8c296167eb56016c49e4 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 11:18:52 +0000 Subject: [PATCH 01/14] add design document --- DESIGN.md | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 DESIGN.md diff --git a/DESIGN.md b/DESIGN.md new file mode 100644 index 0000000..14ff509 --- /dev/null +++ b/DESIGN.md @@ -0,0 +1,143 @@ +# Design: Jinja2 Sandboxing for Config Template Rendering + +## Summary + +Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`, and restrict the template context to a whitelisted subset of environment variables instead of exposing all of `os.environ`. + +## Goals + +- Prevent Jinja2 template injection from escalating to arbitrary code execution on the server. +- Limit the attack surface in the event an attacker gains write access to a config file or environment variable. +- Maintain all existing functionality for legitimate template usage (e.g., referencing `$HOME`, `$RAVNAR_*` variables in config values). +- Keep the change small and contained to `utils.py`. + +## Non-Goals + +- Removing the template rendering feature entirely — it is useful for dynamic configuration. +- Adding per-value access control (different env vars for different config values). +- Replacing Jinja2 with a different templating engine. +- Auditing each config value to determine whether it is safe to render (the concern is the renderer, not the value). + +## Background / Motivation + +Ravnar's configuration system applies Jinja2 template rendering to all config values via `render_template()` in `utils.py`: + +```python +def render_template(s: Any) -> Any: + if isinstance(s, str): + return jinja2.Environment().from_string(s).render(**os.environ) +``` + +This function is called as part of `RenderableMixin._render_templates`, which processes every value in the YAML config tree and every `RAVNAR_*` environment variable. The rendered values are then used to construct the final `Config` object. + +The current code has two problems: + +1. **No sandbox:** `jinja2.Environment()` is the standard environment, which allows access to Python builtins through the template sandbox escape technique (`{{ config.__class__.__init__.__globals__ }}`). While this requires an attacker to control a config value or env var, the lack of sandboxing means a single bypass of config-file protection leads to code execution. + +2. **Full environment exposure:** `os.environ` is passed as the template context. This includes all environment variables, not just ravnar-specific ones. An attacker who can influence any env var can reference it in a template. More importantly, the full environment is more surface area for potential escape vectors through Jinja2's interaction with dict-like objects. + +The fix uses `SandboxedEnvironment` and a restricted context, making template-based code execution practically impossible without a Jinja2 sandbox escape vulnerability being discovered and unpatched. + +## Design + +### 1. Sandboxed Environment + +Replace `jinja2.Environment()` with `jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined)`: + +```python +import jinja2 +import jinja2.sandbox + +def render_template(s: Any) -> Any: + if isinstance(s, str): + env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) + return env.from_string(s).render(**context) +``` + +`SandboxedEnvironment` blocks access to: +- Dunder attributes (`__class__`, `__bases__`, `__subclasses__`, `__globals__`, etc.) +- Built-in functions (`eval`, `exec`, `open`, `import`, etc.) +- Methods flagged as unsafe (`__call__` on callables that are not explicitly safe) + +`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This means non-whitelisted environment variables will cause a hard failure at startup, rather than silently rendering as empty strings. + +This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined with `StrictUndefined`, it provides defense in depth: the sandbox blocks code execution, and the strict undefined handling prevents silent misconfigurations caused by the restricted context. + +### 2. Restricted Context + +Instead of passing all of `os.environ`, construct a dict with a whitelist of environment variables: + +```python +_ALLOWED_ENV_VARS = frozenset({ + "HOME", "USER", "USERNAME", + "HOSTNAME", "HOST", + "PATH", + # All RAVNAR_* variables +}) + +def _build_template_context() -> dict[str, str]: + return { + k: v for k, v in os.environ.items() + if k in _ALLOWED_ENV_VARS or k.startswith("RAVNAR_") + } +``` + +This ensures: +- All `RAVNAR_*` variables are available (these are the intended variables for configuring ravnar). +- Common system variables like `HOME`, `USER`, `HOSTNAME` are available (they are commonly referenced in config paths). +- All other environment variables (e.g., `AWS_*`, `DB_*`, `SECRET_*`, `PATH` — wait, `PATH` is explicitly allowed above) are not exposed. + +The whitelist should be documented in the hardening guide. Users who need additional env vars in templates can request additions through a code change rather than via configuration (since the whitelist is a code constant, not a config item, to prevent self-defeat). + +**Note:** Because `StrictUndefined` is used, any template referencing a non-whitelisted env var will raise `UndefinedError` at startup. Users who previously relied on non-whitelisted env vars must either prefix them with `RAVNAR_` or add them to the whitelist. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. + +### 3. Dict Key Rendering + +The function `ImportStringWithParams._render_param_items` currently calls `render_template()` on both keys and values of the params dict: + +```python +return {render_template(k): render_template(v) for k, v in params.items()} +``` + +This means env-var-derived strings can also be parameter names. With the sandboxing change this is less dangerous, but it is still surprising behavior. The fix here is narrower: only render keys if they are strings containing template syntax (`{{` or `{%`). A simpler approach is to apply `render_template` to all string keys as before — the sandboxing is the real defense, and changing key-rendering behavior could break someone who relies on it (unlikely but possible). **Decision: leave key rendering as-is, since sandboxing covers the risk.** + +### 4. SecurityError Handling + +`SandboxedEnvironment` raises `jinja2.exceptions.SecurityError` when a template attempts dunder access, dangerous builtins, or other sandboxed operations. The handling depends on when the template is rendered: + +**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during model validation): `SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason for a config template to trigger a `SecurityError`. + +**Runtime agent registration** (`RegisterAgentData.agent` via API): `SecurityError` must be **caught and converted to an HTTPException** (e.g., `400 Bad Request` or `422 Unprocessable Entity`) so the API client receives a proper error response instead of an unhandled `500 Internal Server Error`. The error message should indicate that the template contains disallowed content. + +### 5. Startup Logging + +No explicit warning is added for the sandboxing change — it is a silent fix. No logging changes are required. + +## Tradeoffs & Risks + +- **Template features that are sandboxed:** `SandboxedEnvironment` blocks dunder access, dangerous builtins, and arbitrary calls. It does **not** block `range()`, `cycler()`, `joiner()`, `namespace()`, or `{% extends %}` / `{% include %}` — but since no template loader is configured, file-based inheritance is already non-functional. None of these features are used in ravnar's config templates (they are short, single-line expressions like `{{ HOME }}/data`). +- **StrictUndefined breakage:** `StrictUndefined` raises on any access to a missing variable. This breaks the common Jinja2 idiom `{{ VAR if VAR else "default" }}` because the boolean check counts as an access. The supported alternatives are `{{ VAR | default("default") }}` and `{% if VAR is defined %}...{% endif %}`. This is a deliberate breaking change for security: silent empty strings are unacceptable for config values. +- **Whitelist maintenance:** If a legitimate use case requires an env var not in the whitelist, a code change is needed. This is intentional — environment variables are a broad attack surface and should not be blindly exposed. +- **Jinja2 sandbox security:** The sandbox is not perfect — sandbox escapes have been found in Jinja2 before. However, it raises the bar significantly. The restricted context further limits what an attacker could reach even with a sandbox escape. +- **Performance:** `SandboxedEnvironment` has a small overhead compared to `Environment`. Config rendering happens once at startup, so this is negligible. + +## Testing Strategy + +- **Unit tests for sandbox:** + - Render `{{ 7 * 7 }}` → `49` (basic math still works). + - Render `{{ HOME }}` → value of `$HOME` (env var access works). + - Render `{{ RAVNAR_SOME_VAR }}` → value of that env var. + - Render `{{ config.__class__ }}` → raises `SecurityError` (fail-closed at startup, converted to HTTPException at runtime). + - Render `{{ self.__class__.__mro__ }}` → raises `SecurityError`. + - Render `{{ ''.__class__.__mro__ }}` → raises `SecurityError`. +- **Unit tests for restricted context:** + - Env vars in the whitelist are accessible. + - Env vars not in the whitelist raise `UndefinedError` (fail-closed). + - `RAVNAR_*` vars are all accessible regardless of whitelist membership. + - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. +- **Integration tests:** Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. Start ravnar with a config that uses a non-whitelisted env var (e.g., `{{ MY_SECRET }}`) — verify that startup fails with `UndefinedError`. +- **No e2e tests needed.** + +## Open Questions + +*(none — all design decisions are resolved)* From e4c408005914030b72f2eefda3c36479188e852f Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 11:30:33 +0000 Subject: [PATCH 02/14] update design: split context between startup and runtime --- DESIGN.md | 52 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 14ff509..937725e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -2,7 +2,7 @@ ## Summary -Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`, and restrict the template context to a whitelisted subset of environment variables instead of exposing all of `os.environ`. +Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with `StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a restricted whitelist of environment variables to prevent a privilege-escalation exfiltration attack where a user with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. ## Goals @@ -34,9 +34,9 @@ The current code has two problems: 1. **No sandbox:** `jinja2.Environment()` is the standard environment, which allows access to Python builtins through the template sandbox escape technique (`{{ config.__class__.__init__.__globals__ }}`). While this requires an attacker to control a config value or env var, the lack of sandboxing means a single bypass of config-file protection leads to code execution. -2. **Full environment exposure:** `os.environ` is passed as the template context. This includes all environment variables, not just ravnar-specific ones. An attacker who can influence any env var can reference it in a template. More importantly, the full environment is more surface area for potential escape vectors through Jinja2's interaction with dict-like objects. +2. **Runtime exfiltration via dynamic agents:** A user with `agents:write` permission can register an agent whose parameters contain template expressions like `{{ AWS_SECRET_ACCESS_KEY }}`. The template renders during `ImportStringWithParams` validation, substituting the secret into the agent's config. The agent's `getCapabilities()` returns this value, and the user can read it via `GET /api/agents` (requires only `agents:read`, a standard permission). This is a privilege escalation: a write-scoped attacker exfiltrates read-scoped secrets from the environment. -The fix uses `SandboxedEnvironment` and a restricted context, making template-based code execution practically impossible without a Jinja2 sandbox escape vulnerability being discovered and unpatched. +The fix uses `SandboxedEnvironment` for all rendering, with a restricted context only for runtime agent registration. This blocks code execution in all cases and prevents the exfiltration attack while preserving the UX of full env-var access for legitimate config file authoring. ## Design @@ -48,10 +48,16 @@ Replace `jinja2.Environment()` with `jinja2.sandbox.SandboxedEnvironment(undefin import jinja2 import jinja2.sandbox -def render_template(s: Any) -> Any: +def render_template(s: Any, *, context: dict[str, str] | None = None) -> Any: if isinstance(s, str): env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) - return env.from_string(s).render(**context) + ctx = context if context is not None else dict(os.environ) + return env.from_string(s).render(**ctx) + if isinstance(s, dict): + return {render_template(k, context=context): render_template(v, context=context) for k, v in s.items()} + if isinstance(s, list): + return [render_template(v, context=context) for v in s] + return s ``` `SandboxedEnvironment` blocks access to: @@ -59,13 +65,15 @@ def render_template(s: Any) -> Any: - Built-in functions (`eval`, `exec`, `open`, `import`, etc.) - Methods flagged as unsafe (`__call__` on callables that are not explicitly safe) -`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This means non-whitelisted environment variables will cause a hard failure at startup, rather than silently rendering as empty strings. +`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This prevents silent misconfigurations caused by typos or missing env vars. + +The `context` parameter controls which environment variables are exposed to the template. When `None` (default), the full `os.environ` is used. This is the behavior for startup config rendering. -This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined with `StrictUndefined`, it provides defense in depth: the sandbox blocks code execution, and the strict undefined handling prevents silent misconfigurations caused by the restricted context. +This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined with `StrictUndefined`, it blocks code execution and prevents silent misconfigurations. -### 2. Restricted Context +### 2. Restricted Context (Runtime Only) -Instead of passing all of `os.environ`, construct a dict with a whitelist of environment variables: +For runtime agent registration, construct a restricted context dict with a whitelist of environment variables: ```python _ALLOWED_ENV_VARS = frozenset({ @@ -75,37 +83,41 @@ _ALLOWED_ENV_VARS = frozenset({ # All RAVNAR_* variables }) -def _build_template_context() -> dict[str, str]: +def _build_restricted_template_context() -> dict[str, str]: return { k: v for k, v in os.environ.items() if k in _ALLOWED_ENV_VARS or k.startswith("RAVNAR_") } ``` +This restricted context is **only** passed to `render_template` during `ImportStringWithParams` validation for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used for startup config rendering. + This ensures: - All `RAVNAR_*` variables are available (these are the intended variables for configuring ravnar). - Common system variables like `HOME`, `USER`, `HOSTNAME` are available (they are commonly referenced in config paths). - All other environment variables (e.g., `AWS_*`, `DB_*`, `SECRET_*`, `PATH` — wait, `PATH` is explicitly allowed above) are not exposed. -The whitelist should be documented in the hardening guide. Users who need additional env vars in templates can request additions through a code change rather than via configuration (since the whitelist is a code constant, not a config item, to prevent self-defeat). +The whitelist should be documented in the hardening guide. Users who need additional env vars in dynamic agent templates can request additions through a code change rather than via configuration (since the whitelist is a code constant, not a config item, to prevent self-defeat). -**Note:** Because `StrictUndefined` is used, any template referencing a non-whitelisted env var will raise `UndefinedError` at startup. Users who previously relied on non-whitelisted env vars must either prefix them with `RAVNAR_` or add them to the whitelist. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. +**Note:** Because `StrictUndefined` is used, any template referencing a non-whitelisted env var will raise `UndefinedError` at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. ### 3. Dict Key Rendering The function `ImportStringWithParams._render_param_items` currently calls `render_template()` on both keys and values of the params dict: ```python -return {render_template(k): render_template(v) for k, v in params.items()} +return {render_template(k, context=context): render_template(v, context=context) for k, v in params.items()} ``` This means env-var-derived strings can also be parameter names. With the sandboxing change this is less dangerous, but it is still surprising behavior. The fix here is narrower: only render keys if they are strings containing template syntax (`{{` or `{%`). A simpler approach is to apply `render_template` to all string keys as before — the sandboxing is the real defense, and changing key-rendering behavior could break someone who relies on it (unlikely but possible). **Decision: leave key rendering as-is, since sandboxing covers the risk.** +`ImportStringWithParams._render_field_templates` and `_render_param_items` must pass the restricted context when called during runtime agent registration. For startup config rendering, `RenderableMixin._render_templates` already pre-renders values with the full `os.environ` context, so the `ImportStringWithParams` validators will receive plain strings (mostly no-op). + ### 4. SecurityError Handling `SandboxedEnvironment` raises `jinja2.exceptions.SecurityError` when a template attempts dunder access, dangerous builtins, or other sandboxed operations. The handling depends on when the template is rendered: -**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during model validation): `SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason for a config template to trigger a `SecurityError`. +**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during startup model validation): `SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason for a config template to trigger a `SecurityError`. **Runtime agent registration** (`RegisterAgentData.agent` via API): `SecurityError` must be **caught and converted to an HTTPException** (e.g., `400 Bad Request` or `422 Unprocessable Entity`) so the API client receives a proper error response instead of an unhandled `500 Internal Server Error`. The error message should indicate that the template contains disallowed content. @@ -130,12 +142,16 @@ No explicit warning is added for the sandboxing change — it is a silent fix. N - Render `{{ config.__class__ }}` → raises `SecurityError` (fail-closed at startup, converted to HTTPException at runtime). - Render `{{ self.__class__.__mro__ }}` → raises `SecurityError`. - Render `{{ ''.__class__.__mro__ }}` → raises `SecurityError`. -- **Unit tests for restricted context:** - - Env vars in the whitelist are accessible. - - Env vars not in the whitelist raise `UndefinedError` (fail-closed). +- **Unit tests for restricted context (runtime only):** + - Env vars in the whitelist are accessible during runtime agent registration. + - Env vars not in the whitelist raise `UndefinedError` during runtime agent registration. - `RAVNAR_*` vars are all accessible regardless of whitelist membership. - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. -- **Integration tests:** Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. Start ravnar with a config that uses a non-whitelisted env var (e.g., `{{ MY_SECRET }}`) — verify that startup fails with `UndefinedError`. +- **Integration tests:** + - Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. + - Start ravnar with a config that uses an arbitrary env var (e.g., `{{ MY_SECRET }}`) — verify the path resolves correctly (full context is available for config). + - Register a dynamic agent via API with a whitelisted env var in params — verify it succeeds. + - Register a dynamic agent via API with a non-whitelisted env var in params — verify it fails with `UndefinedError` (or `SecurityError` if dunder access is attempted). - **No e2e tests needed.** ## Open Questions From f58c3727d652c5bb8906d261ae814b6ff25a40c6 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 11:34:41 +0000 Subject: [PATCH 03/14] update design: configurable env var allowlist for dynamic agents --- DESIGN.md | 73 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 49 insertions(+), 24 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 937725e..a561a74 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -2,7 +2,7 @@ ## Summary -Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with `StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a restricted whitelist of environment variables to prevent a privilege-escalation exfiltration attack where a user with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. +Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with `StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a configurable allowlist of environment variables (default empty) to prevent a privilege-escalation exfiltration attack where a user with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. The admin explicitly controls which env vars are exposed to dynamic agents via `agents.dynamic.allowed_env_vars` in the config. ## Goals @@ -71,35 +71,59 @@ The `context` parameter controls which environment variables are exposed to the This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined with `StrictUndefined`, it blocks code execution and prevents silent misconfigurations. -### 2. Restricted Context (Runtime Only) +### 2. Configurable Allowlist for Runtime Agent Registration -For runtime agent registration, construct a restricted context dict with a whitelist of environment variables: +Add `allowed_env_vars` to `DynamicAgentConfig`: ```python -_ALLOWED_ENV_VARS = frozenset({ - "HOME", "USER", "USERNAME", - "HOSTNAME", "HOST", - "PATH", - # All RAVNAR_* variables -}) - -def _build_restricted_template_context() -> dict[str, str]: +class DynamicAgentConfig(BaseModel, RenderableMixin): + enabled: bool = False + allowed_env_vars: list[str] = Field(default_factory=list) +``` + +The default is an empty list: **no environment variables are exposed to dynamic agents by default**. The admin must explicitly list each variable name that should be available in templates during runtime agent registration. + +For runtime agent registration, construct a restricted context dict from the configured allowlist: + +```python + +def _build_restricted_template_context(allowed_vars: list[str]) -> dict[str, str]: return { k: v for k, v in os.environ.items() - if k in _ALLOWED_ENV_VARS or k.startswith("RAVNAR_") + if k in allowed_vars } ``` This restricted context is **only** passed to `render_template` during `ImportStringWithParams` validation for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used for startup config rendering. -This ensures: -- All `RAVNAR_*` variables are available (these are the intended variables for configuring ravnar). -- Common system variables like `HOME`, `USER`, `HOSTNAME` are available (they are commonly referenced in config paths). -- All other environment variables (e.g., `AWS_*`, `DB_*`, `SECRET_*`, `PATH` — wait, `PATH` is explicitly allowed above) are not exposed. +**Passing the context to runtime rendering:** `render_template` is called from Pydantic validators (`ImportStringWithParams._render_field_templates` and `_render_param_items`), which are class methods and do not receive request state directly. Use a `contextvars.ContextVar` to pass the restricted context implicitly during request parsing. A FastAPI `yield` dependency on the dynamic agents routes sets the context variable before request body parsing and clears it afterward: + +```python +import contextvars + +_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar("_template_context", default=None) + +def render_template(s: Any, *, context: dict[str, str] | None = None) -> Any: + if isinstance(s, str): + env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) + ctx = context if context is not None else (_template_context.get() or dict(os.environ)) + return env.from_string(s).render(**ctx) + ... + +async def _set_restricted_template_context(config: Config = Depends(get_config)): + _template_context.set(_build_restricted_template_context(config.agents.dynamic.allowed_env_vars)) + yield + _template_context.set(None) + +# Applied to dynamic agents routes: +@router.post("", dependencies=[Depends(_set_restricted_template_context)]) +``` + +For startup config, `_template_context` is not set, so `render_template` falls back to the full `os.environ`. -The whitelist should be documented in the hardening guide. Users who need additional env vars in dynamic agent templates can request additions through a code change rather than via configuration (since the whitelist is a code constant, not a config item, to prevent self-defeat). +**Why default-deny:** The attack model is a user with `agents:write` registering an agent that exfiltrates arbitrary env vars via `getCapabilities()`. The admin controls the ravnar config file (or deployment configuration), so they can explicitly decide which env vars are safe to expose to dynamic agents. A hardcoded whitelist would either be too permissive (blocking legitimate use cases) or require code changes for every deployment. A config value lets the admin make the security decision at deployment time. -**Note:** Because `StrictUndefined` is used, any template referencing a non-whitelisted env var will raise `UndefinedError` at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. +**Note:** Because `StrictUndefined` is used, any template referencing a non-allowed env var will raise `UndefinedError` at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. ### 3. Dict Key Rendering @@ -129,7 +153,7 @@ No explicit warning is added for the sandboxing change — it is a silent fix. N - **Template features that are sandboxed:** `SandboxedEnvironment` blocks dunder access, dangerous builtins, and arbitrary calls. It does **not** block `range()`, `cycler()`, `joiner()`, `namespace()`, or `{% extends %}` / `{% include %}` — but since no template loader is configured, file-based inheritance is already non-functional. None of these features are used in ravnar's config templates (they are short, single-line expressions like `{{ HOME }}/data`). - **StrictUndefined breakage:** `StrictUndefined` raises on any access to a missing variable. This breaks the common Jinja2 idiom `{{ VAR if VAR else "default" }}` because the boolean check counts as an access. The supported alternatives are `{{ VAR | default("default") }}` and `{% if VAR is defined %}...{% endif %}`. This is a deliberate breaking change for security: silent empty strings are unacceptable for config values. -- **Whitelist maintenance:** If a legitimate use case requires an env var not in the whitelist, a code change is needed. This is intentional — environment variables are a broad attack surface and should not be blindly exposed. +- **Allowlist configuration:** The admin must explicitly configure `agents.dynamic.allowed_env_vars` for each env var needed by dynamic agents. This is a one-time deployment decision per variable. The default-deny model is secure but requires admin awareness. If a user tries to register a dynamic agent that references an unallowed env var, the registration fails with `UndefinedError`. The error message should be clear enough to guide the admin to add the variable to the allowlist. - **Jinja2 sandbox security:** The sandbox is not perfect — sandbox escapes have been found in Jinja2 before. However, it raises the bar significantly. The restricted context further limits what an attacker could reach even with a sandbox escape. - **Performance:** `SandboxedEnvironment` has a small overhead compared to `Environment`. Config rendering happens once at startup, so this is negligible. @@ -143,15 +167,16 @@ No explicit warning is added for the sandboxing change — it is a silent fix. N - Render `{{ self.__class__.__mro__ }}` → raises `SecurityError`. - Render `{{ ''.__class__.__mro__ }}` → raises `SecurityError`. - **Unit tests for restricted context (runtime only):** - - Env vars in the whitelist are accessible during runtime agent registration. - - Env vars not in the whitelist raise `UndefinedError` during runtime agent registration. - - `RAVNAR_*` vars are all accessible regardless of whitelist membership. + - Env vars in `allowed_env_vars` are accessible during runtime agent registration. + - Env vars not in `allowed_env_vars` raise `UndefinedError` during runtime agent registration. + - Default empty allowlist: any env var reference raises `UndefinedError`. - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. - **Integration tests:** - Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. - Start ravnar with a config that uses an arbitrary env var (e.g., `{{ MY_SECRET }}`) — verify the path resolves correctly (full context is available for config). - - Register a dynamic agent via API with a whitelisted env var in params — verify it succeeds. - - Register a dynamic agent via API with a non-whitelisted env var in params — verify it fails with `UndefinedError` (or `SecurityError` if dunder access is attempted). + - Register a dynamic agent via API with `allowed_env_vars` empty — verify any env var reference fails with `UndefinedError`. + - Register a dynamic agent via API with `allowed_env_vars` containing `AWS_SECRET_ACCESS_KEY` — verify the template renders correctly. + - Register a dynamic agent via API with `allowed_env_vars` configured but a non-allowed env var referenced — verify it fails with `UndefinedError`. - **No e2e tests needed.** ## Open Questions From 13ff718653a7ae23365a2efdaba528f87c0a0833 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 11:46:03 +0000 Subject: [PATCH 04/14] update design: avoid contextvars, explicit dependency, generic error messages --- DESIGN.md | 237 +++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 173 insertions(+), 64 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index a561a74..022bfed 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -2,13 +2,19 @@ ## Summary -Replace the unrestricted `jinja2.Environment` used in config template rendering with `jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with `StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a configurable allowlist of environment variables (default empty) to prevent a privilege-escalation exfiltration attack where a user with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. The admin explicitly controls which env vars are exposed to dynamic agents via `agents.dynamic.allowed_env_vars` in the config. +Replace the unrestricted `jinja2.Environment` used in config template rendering with +`jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with +`StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a configurable +allowlist of environment variables (default empty) to prevent a privilege-escalation exfiltration attack where a user +with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. The admin explicitly controls which env +vars are exposed to dynamic agents via `agents.dynamic.allowed_env_vars` in the config. ## Goals - Prevent Jinja2 template injection from escalating to arbitrary code execution on the server. - Limit the attack surface in the event an attacker gains write access to a config file or environment variable. -- Maintain all existing functionality for legitimate template usage (e.g., referencing `$HOME`, `$RAVNAR_*` variables in config values). +- Maintain all existing functionality for legitimate template usage (e.g., referencing `$HOME`, `$RAVNAR_*` variables in + config values). - Keep the change small and contained to `utils.py`. ## Non-Goals @@ -20,7 +26,8 @@ Replace the unrestricted `jinja2.Environment` used in config template rendering ## Background / Motivation -Ravnar's configuration system applies Jinja2 template rendering to all config values via `render_template()` in `utils.py`: +Ravnar's configuration system applies Jinja2 template rendering to all config values via `render_template()` in +`utils.py`: ```python def render_template(s: Any) -> Any: @@ -28,15 +35,27 @@ def render_template(s: Any) -> Any: return jinja2.Environment().from_string(s).render(**os.environ) ``` -This function is called as part of `RenderableMixin._render_templates`, which processes every value in the YAML config tree and every `RAVNAR_*` environment variable. The rendered values are then used to construct the final `Config` object. +This function is called as part of `RenderableMixin._render_templates`, which processes every value in the YAML config +tree and every `RAVNAR_*` environment variable. The rendered values are then used to construct the final `Config` +object. The current code has two problems: -1. **No sandbox:** `jinja2.Environment()` is the standard environment, which allows access to Python builtins through the template sandbox escape technique (`{{ config.__class__.__init__.__globals__ }}`). While this requires an attacker to control a config value or env var, the lack of sandboxing means a single bypass of config-file protection leads to code execution. +1. **No sandbox:** `jinja2.Environment()` is the standard environment, which allows access to Python builtins through + the template sandbox escape technique (`{{ config.__class__.__init__.__globals__ }}`). While this requires an + attacker to control a config value or env var, the lack of sandboxing means a single bypass of config-file protection + leads to code execution. -2. **Runtime exfiltration via dynamic agents:** A user with `agents:write` permission can register an agent whose parameters contain template expressions like `{{ AWS_SECRET_ACCESS_KEY }}`. The template renders during `ImportStringWithParams` validation, substituting the secret into the agent's config. The agent's `getCapabilities()` returns this value, and the user can read it via `GET /api/agents` (requires only `agents:read`, a standard permission). This is a privilege escalation: a write-scoped attacker exfiltrates read-scoped secrets from the environment. +2. **Runtime exfiltration via dynamic agents:** A user with `agents:write` permission can register an agent whose + parameters contain template expressions like `{{ AWS_SECRET_ACCESS_KEY }}`. The template renders during + `ImportStringWithParams` validation, substituting the secret into the agent's config. The agent's `getCapabilities()` + returns this value, and the user can read it via `GET /api/agents` (requires only `agents:read`, a standard + permission). This is a privilege escalation: a write-scoped attacker exfiltrates read-scoped secrets from the + environment. -The fix uses `SandboxedEnvironment` for all rendering, with a restricted context only for runtime agent registration. This blocks code execution in all cases and prevents the exfiltration attack while preserving the UX of full env-var access for legitimate config file authoring. +The fix uses `SandboxedEnvironment` for all rendering, with a restricted context only for runtime agent registration. +This blocks code execution in all cases and prevents the exfiltration attack while preserving the UX of full env-var +access for legitimate config file authoring. ## Design @@ -45,31 +64,42 @@ The fix uses `SandboxedEnvironment` for all rendering, with a restricted context Replace `jinja2.Environment()` with `jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined)`: ```python +import contextvars import jinja2 import jinja2.sandbox -def render_template(s: Any, *, context: dict[str, str] | None = None) -> Any: +render_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar( + "render_template_context", default=None +) + +def render_template(s: Any, context: dict[str, str]) -> Any: if isinstance(s, str): env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) - ctx = context if context is not None else dict(os.environ) - return env.from_string(s).render(**ctx) + return env.from_string(s).render(**context) if isinstance(s, dict): - return {render_template(k, context=context): render_template(v, context=context) for k, v in s.items()} + return {render_template(k, context): render_template(v, context) for k, v in s.items()} if isinstance(s, list): - return [render_template(v, context=context) for v in s] + return [render_template(v, context) for v in s] return s ``` `SandboxedEnvironment` blocks access to: + - Dunder attributes (`__class__`, `__bases__`, `__subclasses__`, `__globals__`, etc.) - Built-in functions (`eval`, `exec`, `open`, `import`, etc.) - Methods flagged as unsafe (`__call__` on callables that are not explicitly safe) -`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This prevents silent misconfigurations caused by typos or missing env vars. +`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This +prevents silent misconfigurations caused by typos or missing env vars. -The `context` parameter controls which environment variables are exposed to the template. When `None` (default), the full `os.environ` is used. This is the behavior for startup config rendering. +`render_template` is a pure function: it always requires an explicit `context` dict. Callers decide what context to +pass. The `render_template_context` ContextVar is used only by `ImportStringWithParams._render_template` (see below) to +inject the restricted context during runtime agent registration. When the ContextVar is not set, +`ImportStringWithParams._render_template` falls back to the full `os.environ`. This is the behavior for startup config +rendering. -This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined with `StrictUndefined`, it blocks code execution and prevents silent misconfigurations. +This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined +with `StrictUndefined`, it blocks code execution and prevents silent misconfigurations. ### 2. Configurable Allowlist for Runtime Agent Registration @@ -81,69 +111,127 @@ class DynamicAgentConfig(BaseModel, RenderableMixin): allowed_env_vars: list[str] = Field(default_factory=list) ``` -The default is an empty list: **no environment variables are exposed to dynamic agents by default**. The admin must explicitly list each variable name that should be available in templates during runtime agent registration. +The default is an empty list: **no environment variables are exposed to dynamic agents by default**. The admin must +explicitly list each variable name that should be available in templates during runtime agent registration. -For runtime agent registration, construct a restricted context dict from the configured allowlist: +For runtime agent registration, the restricted context is constructed inline in the FastAPI dependency from the +configured allowlist. This context is **only** passed to `render_template` during `ImportStringWithParams` validation +for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used for startup config rendering. -```python - -def _build_restricted_template_context(allowed_vars: list[str]) -> dict[str, str]: - return { - k: v for k, v in os.environ.items() - if k in allowed_vars - } -``` - -This restricted context is **only** passed to `render_template` during `ImportStringWithParams` validation for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used for startup config rendering. - -**Passing the context to runtime rendering:** `render_template` is called from Pydantic validators (`ImportStringWithParams._render_field_templates` and `_render_param_items`), which are class methods and do not receive request state directly. Use a `contextvars.ContextVar` to pass the restricted context implicitly during request parsing. A FastAPI `yield` dependency on the dynamic agents routes sets the context variable before request body parsing and clears it afterward: +**Passing the context to runtime rendering:** `render_template` is called from Pydantic validators +(`ImportStringWithParams._render_field_templates` and `_render_param_items`), which are class methods and do not receive +request state directly. The `render_template_context` ContextVar bridges this gap: a FastAPI `yield` dependency on the +dynamic agents routes sets the context variable before request body parsing. ```python -import contextvars - -_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar("_template_context", default=None) - -def render_template(s: Any, *, context: dict[str, str] | None = None) -> Any: - if isinstance(s, str): - env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) - ctx = context if context is not None else (_template_context.get() or dict(os.environ)) - return env.from_string(s).render(**ctx) - ... - async def _set_restricted_template_context(config: Config = Depends(get_config)): - _template_context.set(_build_restricted_template_context(config.agents.dynamic.allowed_env_vars)) + allowed_vars = config.agents.dynamic.allowed_env_vars + ctx = {k: v for k, v in os.environ.items() if k in allowed_vars} + render_template_context.set(ctx) yield - _template_context.set(None) # Applied to dynamic agents routes: @router.post("", dependencies=[Depends(_set_restricted_template_context)]) +async def register_agent( + data: RegisterAgentData, + user: User = Depends(authorized_user_with("agents:write")), +) -> AgentInfo: + agent = data.agent() + ... ``` -For startup config, `_template_context` is not set, so `render_template` falls back to the full `os.environ`. +The `yield` dependency runs before request body parsing. `render_template_context` is set, then Pydantic validators run +and call `ImportStringWithParams._render_template`, which reads the ContextVar and receives the restricted context. +After the response is generated, the `yield` resumes and clears the context. + +**Why clear the context?** FastAPI `yield` dependencies are context managers — the code after `yield` runs as cleanup. +While each request runs in its own asyncio task (so the ContextVar would naturally be garbage collected), resetting it +via `set(None)` is defensive and explicit. It prevents stale state if the task is reused (e.g., in some async frameworks +or test scenarios) and follows the `yield` dependency pattern expected by FastAPI developers. For startup config, +`render_template_context` is not set, so `ImportStringWithParams._render_template` falls back to the full `os.environ`. -**Why default-deny:** The attack model is a user with `agents:write` registering an agent that exfiltrates arbitrary env vars via `getCapabilities()`. The admin controls the ravnar config file (or deployment configuration), so they can explicitly decide which env vars are safe to expose to dynamic agents. A hardcoded whitelist would either be too permissive (blocking legitimate use cases) or require code changes for every deployment. A config value lets the admin make the security decision at deployment time. +**Why default-deny:** The attack model is a user with `agents:write` registering an agent that exfiltrates arbitrary env +vars via `getCapabilities()`. The admin controls the ravnar config file (or deployment configuration), so they can +explicitly decide which env vars are safe to expose to dynamic agents. A hardcoded whitelist would either be too +permissive (blocking legitimate use cases) or require code changes for every deployment. A config value lets the admin +make the security decision at deployment time. -**Note:** Because `StrictUndefined` is used, any template referencing a non-allowed env var will raise `UndefinedError` at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context and triggers `UndefinedError`. +**Note:** Because `StrictUndefined` is used, any template referencing a non-allowed env var will raise `UndefinedError` +at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` +test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context +and triggers `UndefinedError`. ### 3. Dict Key Rendering -The function `ImportStringWithParams._render_param_items` currently calls `render_template()` on both keys and values of the params dict: +The function `ImportStringWithParams._render_param_items` currently calls `render_template()` on both keys and values of +the params dict: ```python -return {render_template(k, context=context): render_template(v, context=context) for k, v in params.items()} +return {cls._render_template(k): cls._render_template(v) for k, v in params.items()} ``` -This means env-var-derived strings can also be parameter names. With the sandboxing change this is less dangerous, but it is still surprising behavior. The fix here is narrower: only render keys if they are strings containing template syntax (`{{` or `{%`). A simpler approach is to apply `render_template` to all string keys as before — the sandboxing is the real defense, and changing key-rendering behavior could break someone who relies on it (unlikely but possible). **Decision: leave key rendering as-is, since sandboxing covers the risk.** +This means env-var-derived strings can also be parameter names. With the sandboxing change this is less dangerous, but +it is still surprising behavior. The fix here is narrower: only render keys if they are strings containing template +syntax (`{{` or `{%`). A simpler approach is to apply `render_template` to all string keys as before — the sandboxing is +the real defense, and changing key-rendering behavior could break someone who relies on it (unlikely but possible). +**Decision: leave key rendering as-is, since sandboxing covers the risk.** -`ImportStringWithParams._render_field_templates` and `_render_param_items` must pass the restricted context when called during runtime agent registration. For startup config rendering, `RenderableMixin._render_templates` already pre-renders values with the full `os.environ` context, so the `ImportStringWithParams` validators will receive plain strings (mostly no-op). +`ImportStringWithParams` validators call `cls._render_template`, which injects the context. The +`RenderableMixin._render_templates` validator calls `render_template` directly with the full `os.environ` context: + +```python +class RenderableMixin: + @field_validator("*", mode="before") + @classmethod + def _render_templates(cls, data: Any) -> Any: + return render_template(data, context=dict(os.environ)) + +class ImportStringWithParams(BaseModel, Generic[T]): + @field_validator("cls_or_fn", "params", mode="before") + @classmethod + def _render_field_templates(cls, f: Any) -> Any: + if isinstance(f, str): + return cls._render_template(f) + return f + + @field_validator("params", mode="after") + @classmethod + def _render_param_items(cls, params: dict[str, Any]) -> dict[str, Any]: + return {cls._render_template(k): cls._render_template(v) for k, v in params.items()} + + @classmethod + def _render_template(cls, s: Any) -> Any: + ctx = render_template_context.get() + if ctx is None: + ctx = dict(os.environ) + return render_template(s, ctx) +``` + +During runtime agent registration, the ContextVar is set to the restricted context, so +`ImportStringWithParams._render_template` reads it and passes it to `render_template`. For startup config, +`RenderableMixin._render_templates` calls `render_template` directly with `os.environ`, bypassing the ContextVar +entirely. The `ImportStringWithParams` validators that run afterward receive plain strings (mostly no-op), but if they +do call `_render_template`, the ContextVar is not set, so they fall back to the full `os.environ`. ### 4. SecurityError Handling -`SandboxedEnvironment` raises `jinja2.exceptions.SecurityError` when a template attempts dunder access, dangerous builtins, or other sandboxed operations. The handling depends on when the template is rendered: +`SandboxedEnvironment` raises `jinja2.exceptions.SecurityError` when a template attempts dunder access, dangerous +builtins, or other sandboxed operations. The handling depends on when the template is rendered: + +**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during startup model validation): +`SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or +a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason +for a config template to trigger a `SecurityError`. -**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during startup model validation): `SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason for a config template to trigger a `SecurityError`. +**Runtime agent registration** (`RegisterAgentData.agent` via API): `SecurityError` and `UndefinedError` must be +**caught and converted to an HTTPException** (e.g., `400 Bad Request` or `422 Unprocessable Entity`) so the API client +receives a proper error response instead of an unhandled `500 Internal Server Error`. -**Runtime agent registration** (`RegisterAgentData.agent` via API): `SecurityError` must be **caught and converted to an HTTPException** (e.g., `400 Bad Request` or `422 Unprocessable Entity`) so the API client receives a proper error response instead of an unhandled `500 Internal Server Error`. The error message should indicate that the template contains disallowed content. +**Important:** The HTTPException response must use a **generic error message** (e.g., "Invalid agent configuration" or +"Template rendering failed") to avoid leaking information about which environment variables are allowed, which are +present, or which template syntax was rejected. The detailed error (including the variable name, the offending template, +and the suggestion to add it to `agents.dynamic.allowed_env_vars`) is **logged server-side** with `structlog` at the +`warning` level. The admin sees the detailed log message; the API client does not. ### 5. Startup Logging @@ -151,11 +239,26 @@ No explicit warning is added for the sandboxing change — it is a silent fix. N ## Tradeoffs & Risks -- **Template features that are sandboxed:** `SandboxedEnvironment` blocks dunder access, dangerous builtins, and arbitrary calls. It does **not** block `range()`, `cycler()`, `joiner()`, `namespace()`, or `{% extends %}` / `{% include %}` — but since no template loader is configured, file-based inheritance is already non-functional. None of these features are used in ravnar's config templates (they are short, single-line expressions like `{{ HOME }}/data`). -- **StrictUndefined breakage:** `StrictUndefined` raises on any access to a missing variable. This breaks the common Jinja2 idiom `{{ VAR if VAR else "default" }}` because the boolean check counts as an access. The supported alternatives are `{{ VAR | default("default") }}` and `{% if VAR is defined %}...{% endif %}`. This is a deliberate breaking change for security: silent empty strings are unacceptable for config values. -- **Allowlist configuration:** The admin must explicitly configure `agents.dynamic.allowed_env_vars` for each env var needed by dynamic agents. This is a one-time deployment decision per variable. The default-deny model is secure but requires admin awareness. If a user tries to register a dynamic agent that references an unallowed env var, the registration fails with `UndefinedError`. The error message should be clear enough to guide the admin to add the variable to the allowlist. -- **Jinja2 sandbox security:** The sandbox is not perfect — sandbox escapes have been found in Jinja2 before. However, it raises the bar significantly. The restricted context further limits what an attacker could reach even with a sandbox escape. -- **Performance:** `SandboxedEnvironment` has a small overhead compared to `Environment`. Config rendering happens once at startup, so this is negligible. +- **Template features that are sandboxed:** `SandboxedEnvironment` blocks dunder access, dangerous builtins, and + arbitrary calls. It does **not** block `range()`, `cycler()`, `joiner()`, `namespace()`, or `{% extends %}` / + `{% include %}` — but since no template loader is configured, file-based inheritance is already non-functional. None + of these features are used in ravnar's config templates (they are short, single-line expressions like + `{{ HOME }}/data`). +- **StrictUndefined breakage:** `StrictUndefined` raises on any access to a missing variable. This breaks the common + Jinja2 idiom `{{ VAR if VAR else "default" }}` because the boolean check counts as an access. The supported + alternatives are `{{ VAR | default("default") }}` and `{% if VAR is defined %}...{% endif %}`. This is a deliberate + breaking change for security: silent empty strings are unacceptable for config values. +- **Allowlist configuration:** The admin must explicitly configure `agents.dynamic.allowed_env_vars` for each env var + needed by dynamic agents. This is a one-time deployment decision per variable. The default-deny model is secure but + requires admin awareness. If a user tries to register a dynamic agent that references an unallowed env var, the + registration fails with `UndefinedError`. The server log includes a clear message naming the undefined variable and + suggesting the admin add it to `agents.dynamic.allowed_env_vars`. The HTTP response is generic to avoid leaking + information. +- **Jinja2 sandbox security:** The sandbox is not perfect — sandbox escapes have been found in Jinja2 before. However, + it raises the bar significantly. The restricted context further limits what an attacker could reach even with a + sandbox escape. +- **Performance:** `SandboxedEnvironment` has a small overhead compared to `Environment`. Config rendering happens once + at startup, so this is negligible. ## Testing Strategy @@ -163,22 +266,28 @@ No explicit warning is added for the sandboxing change — it is a silent fix. N - Render `{{ 7 * 7 }}` → `49` (basic math still works). - Render `{{ HOME }}` → value of `$HOME` (env var access works). - Render `{{ RAVNAR_SOME_VAR }}` → value of that env var. - - Render `{{ config.__class__ }}` → raises `SecurityError` (fail-closed at startup, converted to HTTPException at runtime). + - Render `{{ config.__class__ }}` → raises `SecurityError` (fail-closed at startup, converted to HTTPException at + runtime). - Render `{{ self.__class__.__mro__ }}` → raises `SecurityError`. - Render `{{ ''.__class__.__mro__ }}` → raises `SecurityError`. - **Unit tests for restricted context (runtime only):** - Env vars in `allowed_env_vars` are accessible during runtime agent registration. - Env vars not in `allowed_env_vars` raise `UndefinedError` during runtime agent registration. - Default empty allowlist: any env var reference raises `UndefinedError`. - - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. + - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders + fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. - **Integration tests:** - Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. - - Start ravnar with a config that uses an arbitrary env var (e.g., `{{ MY_SECRET }}`) — verify the path resolves correctly (full context is available for config). - - Register a dynamic agent via API with `allowed_env_vars` empty — verify any env var reference fails with `UndefinedError`. - - Register a dynamic agent via API with `allowed_env_vars` containing `AWS_SECRET_ACCESS_KEY` — verify the template renders correctly. - - Register a dynamic agent via API with `allowed_env_vars` configured but a non-allowed env var referenced — verify it fails with `UndefinedError`. + - Start ravnar with a config that uses an arbitrary env var (e.g., `{{ MY_SECRET }}`) — verify the path resolves + correctly (full context is available for config). + - Register a dynamic agent via API with `allowed_env_vars` empty — verify any env var reference fails with + `UndefinedError`. + - Register a dynamic agent via API with `allowed_env_vars` containing `AWS_SECRET_ACCESS_KEY` — verify the template + renders correctly. + - Register a dynamic agent via API with `allowed_env_vars` configured but a non-allowed env var referenced — verify it + fails with `UndefinedError`. - **No e2e tests needed.** ## Open Questions -*(none — all design decisions are resolved)* +_(none — all design decisions are resolved)_ From f7d233764065bb5b93e8c7bd11e5f2cff3b27854 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 13:22:34 +0000 Subject: [PATCH 05/14] add design document for jinja2 sandboxing --- DESIGN.md | 135 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 112 insertions(+), 23 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 022bfed..dbed801 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -120,24 +120,46 @@ for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used f **Passing the context to runtime rendering:** `render_template` is called from Pydantic validators (`ImportStringWithParams._render_field_templates` and `_render_param_items`), which are class methods and do not receive -request state directly. The `render_template_context` ContextVar bridges this gap: a FastAPI `yield` dependency on the -dynamic agents routes sets the context variable before request body parsing. +request state directly. The `render_template_context` ContextVar bridges this gap. A FastAPI `yield` dependency on the +dynamic agents routes sets the context variable before request body parsing, using the `AgentHandler` instance that is +already available in the router. ```python -async def _set_restricted_template_context(config: Config = Depends(get_config)): - allowed_vars = config.agents.dynamic.allowed_env_vars - ctx = {k: v for k, v in os.environ.items() if k in allowed_vars} - render_template_context.set(ctx) - yield - -# Applied to dynamic agents routes: -@router.post("", dependencies=[Depends(_set_restricted_template_context)]) -async def register_agent( - data: RegisterAgentData, - user: User = Depends(authorized_user_with("agents:write")), -) -> AgentInfo: - agent = data.agent() - ... +class AgentHandler: + def __init__(self, agent_config: AgentConfig) -> None: + self._static_agents: dict[str, Agent] = {id: factory() for id, factory in agent_config.static.items()} + self._dynamic_agents: dict[str, Agent] = {} + self._event_encoder = ag_ui.encoder.EventEncoder() + self._dynamic_enabled = agent_config.dynamic.enabled + self._dynamic_config = agent_config.dynamic + + def get_dynamic_render_template_context(self) -> dict[str, str]: + allowed = self._dynamic_config.allowed_env_vars + return {k: v for k, v in os.environ.items() if k in allowed} +``` + +The `_make_dynamic_agents_router` function receives `agent_handler` by closure, so it can define the `yield` dependency +inline without adding a new `Depends(get_config)`: + +```python +def _make_dynamic_agents_router( + router: schema.APIRouter, + *, + agent_handler: AgentHandler, + authorized_user_with: Callable[..., Any], +) -> None: + async def _set_restricted_template_context(): + render_template_context.set(agent_handler.get_dynamic_render_template_context()) + yield + render_template_context.set(None) + + @router.post("", dependencies=[Depends(_set_restricted_template_context)]) + async def register_agent( + data: RegisterAgentData, + user: User = Depends(authorized_user_with("agents:write")), + ) -> AgentInfo: + agent = data.agent() + ... ``` The `yield` dependency runs before request body parsing. `render_template_context` is set, then Pydantic validators run @@ -223,15 +245,82 @@ builtins, or other sandboxed operations. The handling depends on when the templa a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason for a config template to trigger a `SecurityError`. -**Runtime agent registration** (`RegisterAgentData.agent` via API): `SecurityError` and `UndefinedError` must be -**caught and converted to an HTTPException** (e.g., `400 Bad Request` or `422 Unprocessable Entity`) so the API client +**Runtime restricted-context rendering** (`RegisterAgentData` via API, or any future feature that uses the restricted +context): `SecurityError` and `UndefinedError` must be caught and converted to a client-friendly error so the API client receives a proper error response instead of an unhandled `500 Internal Server Error`. -**Important:** The HTTPException response must use a **generic error message** (e.g., "Invalid agent configuration" or -"Template rendering failed") to avoid leaking information about which environment variables are allowed, which are -present, or which template syntax was rejected. The detailed error (including the variable name, the offending template, -and the suggestion to add it to `agents.dynamic.allowed_env_vars`) is **logged server-side** with `structlog` at the -`warning` level. The admin sees the detailed log message; the API client does not. +Because Pydantic validators wrap every exception they catch in `ValidationError`, `HTTPException` raised from a validator +would not reach FastAPI's `HTTPException` handler. Instead, the exception is trapped inside `RequestValidationError` → +`422`. To avoid this, `ImportStringWithParams._render_template` catches `SecurityError`/`UndefinedError` and re-raises a +custom `TemplateRenderError` when `render_template_context` is set (restricted context). The custom exception carries +the relevant data (template, reason, message) so an exception handler can convert it to a `400 Bad Request` with a +generic client-facing message while preserving the full details for server-side logging. + +```python +# utils.py +class TemplateRenderError(Exception): + """Raised when template rendering fails in a restricted context.""" + + def __init__(self, *, template: str, reason: str, message: str) -> None: + self.template = template + self.reason = reason + self.message = message + super().__init__(message) + + +class ImportStringWithParams(BaseModel, Generic[T]): + @classmethod + def _render_template(cls, s: Any) -> Any: + ctx = render_template_context.get() + if ctx is None: + ctx = dict(os.environ) + try: + return render_template(s, ctx) + except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as e: + if render_template_context.get() is not None: + structlog.get_logger().warning( + "Template rendering blocked", + template=str(s), + reason=type(e).__name__, + error=str(e), + ) + raise TemplateRenderError( + template=str(s), + reason=type(e).__name__, + message="Invalid configuration", + ) from e + raise +``` + +A FastAPI exception handler for `RequestValidationError` is added in `Ravnar._make_app`. It inspects the error list +and, if any error is a `value_error` whose `ctx.error` is a `TemplateRenderError`, returns a `400 Bad Request` with a +generic `{"detail": "Invalid configuration"}` response. The detailed error is logged server-side by the validator +itself with `structlog` at the `warning` level. The admin sees the detailed log message; the API client does not. + +```python +from fastapi.exception_handlers import request_validation_exception_handler +from fastapi.exceptions import RequestValidationError + +@app.exception_handler(RequestValidationError) +async def _template_render_validation_handler(request, exc): + for error in exc.errors(): + if error.get("type") == "value_error": + original = error.get("ctx", {}).get("error") + if isinstance(original, TemplateRenderError): + return JSONResponse( + status_code=400, + content={"detail": original.message}, + ) + return await request_validation_exception_handler(request, exc) +``` + +**Why this approach:** +- The `_render_template` classmethod already knows whether it's in a restricted context because `render_template_context` is set. +- It catches the exception, logs it with full context (`template`, `reason`, `error`), and replaces it with a distinct `TemplateRenderError`. +- Pydantic wraps `TemplateRenderError` in a `ValidationError` with `type: "value_error"`, but the original exception lives in `ctx.error`. +- The exception handler inspects `RequestValidationError.errors()` and looks for `TemplateRenderError` in `ctx.error`. +- If found, it returns `400` (no `422`). Otherwise, it falls back to FastAPI's default validation handler. +- The `TemplateRenderError` class has no agent-specific connotation — it is purely about template rendering — so it can be reused if other features introduce restricted-context rendering in the future. ### 5. Startup Logging From f7e3d35395013593970b3a802447e1fed73de101 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 13:27:45 +0000 Subject: [PATCH 06/14] update design: remove context var clearing, move logging into exception handler --- DESIGN.md | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index dbed801..764542e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -151,7 +151,6 @@ def _make_dynamic_agents_router( async def _set_restricted_template_context(): render_template_context.set(agent_handler.get_dynamic_render_template_context()) yield - render_template_context.set(None) @router.post("", dependencies=[Depends(_set_restricted_template_context)]) async def register_agent( @@ -164,13 +163,9 @@ def _make_dynamic_agents_router( The `yield` dependency runs before request body parsing. `render_template_context` is set, then Pydantic validators run and call `ImportStringWithParams._render_template`, which reads the ContextVar and receives the restricted context. -After the response is generated, the `yield` resumes and clears the context. - -**Why clear the context?** FastAPI `yield` dependencies are context managers — the code after `yield` runs as cleanup. -While each request runs in its own asyncio task (so the ContextVar would naturally be garbage collected), resetting it -via `set(None)` is defensive and explicit. It prevents stale state if the task is reused (e.g., in some async frameworks -or test scenarios) and follows the `yield` dependency pattern expected by FastAPI developers. For startup config, -`render_template_context` is not set, so `ImportStringWithParams._render_template` falls back to the full `os.environ`. +After the response is generated, the `yield` dependency exits. Each request runs in its own asyncio task, so the +ContextVar is naturally garbage collected. For startup config, `render_template_context` is not set, so +`ImportStringWithParams._render_template` falls back to the full `os.environ`. **Why default-deny:** The attack model is a user with `agents:write` registering an agent that exfiltrates arbitrary env vars via `getCapabilities()`. The admin controls the ravnar config file (or deployment configuration), so they can @@ -278,12 +273,6 @@ class ImportStringWithParams(BaseModel, Generic[T]): return render_template(s, ctx) except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as e: if render_template_context.get() is not None: - structlog.get_logger().warning( - "Template rendering blocked", - template=str(s), - reason=type(e).__name__, - error=str(e), - ) raise TemplateRenderError( template=str(s), reason=type(e).__name__, @@ -293,9 +282,9 @@ class ImportStringWithParams(BaseModel, Generic[T]): ``` A FastAPI exception handler for `RequestValidationError` is added in `Ravnar._make_app`. It inspects the error list -and, if any error is a `value_error` whose `ctx.error` is a `TemplateRenderError`, returns a `400 Bad Request` with a -generic `{"detail": "Invalid configuration"}` response. The detailed error is logged server-side by the validator -itself with `structlog` at the `warning` level. The admin sees the detailed log message; the API client does not. +and, if any error is a `value_error` whose `ctx.error` is a `TemplateRenderError`, logs the full details server-side +with `structlog` at the `warning` level and returns a `400 Bad Request` with a generic `{"detail": "Invalid configuration"}` +response. The admin sees the detailed log message; the API client does not. ```python from fastapi.exception_handlers import request_validation_exception_handler @@ -307,6 +296,12 @@ async def _template_render_validation_handler(request, exc): if error.get("type") == "value_error": original = error.get("ctx", {}).get("error") if isinstance(original, TemplateRenderError): + structlog.get_logger().warning( + "Template rendering blocked", + template=original.template, + reason=original.reason, + error=str(original.__cause__), + ) return JSONResponse( status_code=400, content={"detail": original.message}, @@ -316,10 +311,10 @@ async def _template_render_validation_handler(request, exc): **Why this approach:** - The `_render_template` classmethod already knows whether it's in a restricted context because `render_template_context` is set. -- It catches the exception, logs it with full context (`template`, `reason`, `error`), and replaces it with a distinct `TemplateRenderError`. +- It catches the exception and replaces it with a distinct `TemplateRenderError` that carries the relevant data. - Pydantic wraps `TemplateRenderError` in a `ValidationError` with `type: "value_error"`, but the original exception lives in `ctx.error`. - The exception handler inspects `RequestValidationError.errors()` and looks for `TemplateRenderError` in `ctx.error`. -- If found, it returns `400` (no `422`). Otherwise, it falls back to FastAPI's default validation handler. +- If found, it logs the full details and returns `400` (no `422`). Otherwise, it falls back to FastAPI's default validation handler. - The `TemplateRenderError` class has no agent-specific connotation — it is purely about template rendering — so it can be reused if other features introduce restricted-context rendering in the future. ### 5. Startup Logging From 2920638ac0c1b776ea1abfb1226a0671faba3d6d Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Mon, 8 Jun 2026 13:34:41 +0000 Subject: [PATCH 07/14] Implement Jinja2 sandboxing for config template rendering --- src/_ravnar/api/agents.py | 8 +++- src/_ravnar/config.py | 3 +- src/_ravnar/core.py | 33 +++++++++++++-- src/_ravnar/utils.py | 46 +++++++++++++++++--- tests/api/test_agents.py | 88 +++++++++++++++++++++++++++++++++++++++ tests/test_config.py | 59 ++++++++++++++++++++++++++ tests/test_utils.py | 78 ++++++++++++++++++++++++++++++++++ 7 files changed, 304 insertions(+), 11 deletions(-) create mode 100644 tests/test_utils.py diff --git a/src/_ravnar/api/agents.py b/src/_ravnar/api/agents.py index 50b5b28..f3a55d2 100644 --- a/src/_ravnar/api/agents.py +++ b/src/_ravnar/api/agents.py @@ -1,6 +1,7 @@ from __future__ import annotations from collections.abc import Callable +from collections.abc import AsyncIterator from typing import TYPE_CHECKING, Annotated, Any import ag_ui.core @@ -9,6 +10,7 @@ from _ravnar import schema from _ravnar.security import User +from _ravnar.utils import render_template_context if TYPE_CHECKING: from _ravnar.core import AgentHandler @@ -49,7 +51,11 @@ def _make_dynamic_agents_router( "Can be checked with [`GET /api/config`](#/API/get_config_api_config_get)." ) - @router.post("", description=description) + async def _set_restricted_template_context() -> AsyncIterator[None]: + render_template_context.set(agent_handler.get_dynamic_render_template_context()) + yield + + @router.post("", description=description, dependencies=[Depends(_set_restricted_template_context)]) async def register_agent( data: schema.RegisterAgentData, user: User = Depends(authorized_user_with("agents:write")), # noqa: B008 diff --git a/src/_ravnar/config.py b/src/_ravnar/config.py index 19813ad..0086d85 100644 --- a/src/_ravnar/config.py +++ b/src/_ravnar/config.py @@ -31,7 +31,7 @@ class RenderableMixin: @field_validator("*", mode="before") @classmethod def _render_templates(cls, data: Any) -> Any: - return render_template(data) + return render_template(data, context=dict(os.environ)) class LoggingConfig(BaseModel, RenderableMixin): @@ -81,6 +81,7 @@ class StorageConfig(BaseModel, RenderableMixin): class DynamicAgentConfig(BaseModel, RenderableMixin): enabled: bool = False + allowed_env_vars: list[str] = Field(default_factory=list) class AgentConfig(BaseModel, RenderableMixin): diff --git a/src/_ravnar/core.py b/src/_ravnar/core.py index 7ffb232..c4961f2 100644 --- a/src/_ravnar/core.py +++ b/src/_ravnar/core.py @@ -1,15 +1,19 @@ from __future__ import annotations import asyncio +import os from collections.abc import AsyncIterator, Awaitable, Callable from typing import TYPE_CHECKING, cast import ag_ui.core import ag_ui.encoder import fastsse -from fastapi import FastAPI, HTTPException, status +import structlog +from fastapi import FastAPI, HTTPException, Request, status +from fastapi.exception_handlers import request_validation_exception_handler +from fastapi.exceptions import RequestValidationError from fastapi.middleware.cors import CORSMiddleware -from fastapi.responses import RedirectResponse, Response +from fastapi.responses import JSONResponse, RedirectResponse, Response from opentelemetry import trace from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor @@ -18,7 +22,7 @@ from _ravnar.mixin import SetupTeardownMixin from _ravnar.observability import configure_logging, configure_tracing from _ravnar.security import SecurityHeadersMiddleware, make_authorized_user_factory -from _ravnar.utils import as_awaitable +from _ravnar.utils import TemplateRenderError, as_awaitable from .api import make_router as make_api_router from .config import AgentConfig, BaseConfig, Config @@ -74,6 +78,24 @@ async def health() -> Response: async def version() -> str: return __version__ + @app.exception_handler(RequestValidationError) + async def _template_render_validation_handler(request: Request, exc: RequestValidationError) -> JSONResponse: + for error in exc.errors(): + if error.get("type") == "value_error": + original = error.get("ctx", {}).get("error") + if isinstance(original, TemplateRenderError): + structlog.get_logger().warning( + "Template rendering blocked", + template=original.template, + reason=original.reason, + error=str(original.__cause__), + ) + return JSONResponse( + status_code=400, + content={"detail": original.message}, + ) + return await request_validation_exception_handler(request, exc) + app.include_router( make_api_router( storage_config=config.storage, @@ -111,6 +133,11 @@ def __init__(self, agent_config: AgentConfig) -> None: self._dynamic_agents: dict[str, Agent] = {} self._event_encoder = ag_ui.encoder.EventEncoder() self._dynamic_enabled = agent_config.dynamic.enabled + self._dynamic_config = agent_config.dynamic + + def get_dynamic_render_template_context(self) -> dict[str, str]: + allowed = self._dynamic_config.allowed_env_vars + return {k: v for k, v in os.environ.items() if k in allowed} @staticmethod async def _setup_agent(agent: Agent) -> None: diff --git a/src/_ravnar/utils.py b/src/_ravnar/utils.py index 17ca2be..080b46f 100644 --- a/src/_ravnar/utils.py +++ b/src/_ravnar/utils.py @@ -1,6 +1,7 @@ from __future__ import annotations import contextlib +import contextvars import functools import inspect import json @@ -11,6 +12,7 @@ from typing import Any, Generic, TypeVar, cast, get_type_hints import jinja2 +import jinja2.sandbox from pydantic import ( BaseModel, Field, @@ -107,13 +109,29 @@ def now() -> datetime: return datetime.now(tz=UTC) -def render_template(s: Any) -> Any: +render_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar( + "render_template_context", default=None +) + + +class TemplateRenderError(ValueError): + """Raised when template rendering fails in a restricted context.""" + + def __init__(self, *, template: str, reason: str, message: str) -> None: + self.template = template + self.reason = reason + self.message = message + super().__init__(message) + + +def render_template(s: Any, context: dict[str, str]) -> Any: if isinstance(s, str): - return jinja2.Environment().from_string(s).render(**os.environ) + env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) + return env.from_string(s).render(**context) if isinstance(s, dict): - return {render_template(k): render_template(v) for k, v in s.items()} + return {render_template(k, context): render_template(v, context) for k, v in s.items()} if isinstance(s, list): - return [render_template(v) for v in s] + return [render_template(v, context) for v in s] return s @@ -172,14 +190,30 @@ def validate(v: Any, loc: tuple[str | int, ...]) -> Any: @classmethod def _render_field_templates(cls, f: Any) -> Any: if isinstance(f, str): - return render_template(f) + return cls._render_template(f) return f @field_validator("params", mode="after") @classmethod def _render_param_items(cls, params: dict[str, Any]) -> dict[str, Any]: - return {render_template(k): render_template(v) for k, v in params.items()} + return {cls._render_template(k): cls._render_template(v) for k, v in params.items()} + + @classmethod + def _render_template(cls, s: Any) -> Any: + ctx = render_template_context.get() + if ctx is None: + ctx = dict(os.environ) + try: + return render_template(s, ctx) + except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as e: + if render_template_context.get() is not None: + raise TemplateRenderError( + template=str(s), + reason=type(e).__name__, + message="Invalid configuration", + ) from e + raise @model_serializer(mode="wrap") def _serialize(self, nxt: SerializerFunctionWrapHandler) -> Any: diff --git a/tests/api/test_agents.py b/tests/api/test_agents.py index 74ee4aa..acaf9b9 100644 --- a/tests/api/test_agents.py +++ b/tests/api/test_agents.py @@ -1,3 +1,5 @@ +import os + import compyre import pydantic import pytest @@ -5,10 +7,20 @@ import ravnar.agents from _ravnar import schema +from _ravnar.agents import Agent from _ravnar.config import BaseConfig from tests.utils import HeaderAuthenticator, make_app_client +class MockAgent(Agent): + def __init__(self, param="unset"): + self.param = param + + async def run(self, input): + raise AssertionError + yield + + def make_config(*, dynamic_enabled=False): return BaseConfig.model_validate( { @@ -203,3 +215,79 @@ def test_unregister_twice_returns_404(self, client): response = client.delete("/api/agents/delete-twice") assert response.status_code == status.HTTP_404_NOT_FOUND + + def test_register_agent_with_env_var_default_deny(self, client): + response = client.post( + "/api/agents", + json={ + "id": "env-agent", + "agent": { + "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "params": {"param": "{{ HOME }}"}, + }, + }, + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["detail"] == "Invalid configuration" + + def test_register_agent_with_sandbox_escape(self, client): + response = client.post( + "/api/agents", + json={ + "id": "sandbox-agent", + "agent": { + "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "params": {"param": "{{ ''.__class__ }}"}, + }, + }, + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["detail"] == "Invalid configuration" + + +class TestDynamicAgentsWithAllowedEnvVars: + @pytest.fixture + def client(self, mocker): + mocker.patch.dict(os.environ, {"ALLOWED_VAR": "allowed_value", "DENIED_VAR": "denied_value"}) + with make_app_client( + BaseConfig.model_validate( + { + "agents": { + "static": { + "default": {"cls_or_fn": "ravnar.agents.DefaultAgent"}, + }, + "dynamic": {"enabled": True, "allowed_env_vars": ["ALLOWED_VAR"]}, + }, + } + ) + ) as c: + yield c + + def test_register_agent_with_allowed_env_var(self, client): + response = client.post( + "/api/agents", + json={ + "id": "allowed-env-agent", + "agent": { + "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "params": {"param": "{{ ALLOWED_VAR }}"}, + }, + }, + ) + assert response.status_code == status.HTTP_200_OK + info = schema.AgentInfo.model_validate_json(response.content) + assert info.id == "allowed-env-agent" + + def test_register_agent_with_denied_env_var(self, client): + response = client.post( + "/api/agents", + json={ + "id": "denied-env-agent", + "agent": { + "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "params": {"param": "{{ DENIED_VAR }}"}, + }, + }, + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["detail"] == "Invalid configuration" diff --git a/tests/test_config.py b/tests/test_config.py index 2ce0d17..7fe4042 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -9,6 +9,7 @@ from _ravnar.agents import Agent from _ravnar.config import AgentConfig, BaseConfig, Config, DynamicAgentConfig, ImportStringWithParams +from _ravnar.utils import TemplateRenderError, render_template_context @pytest.fixture() @@ -223,3 +224,61 @@ def test_import_string_with_params_nested_error_localization(): details = ve.errors()[0] assert details["loc"] == ("params", "param", "cls_or_fn") assert "non_existing_module" in details["msg"] + + +class TestImportStringWithParamsRestrictedContext: + def test_allowed_env_var_renders(self, mocker): + mocker.patch.dict(os.environ, {"ALLOWED_VAR": "allowed_value"}) + token = render_template_context.set({"ALLOWED_VAR": "allowed_value"}) + try: + result = ImportStringWithParams.model_validate( + { + "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "params": {"param": "{{ ALLOWED_VAR }}"}, + } + ) + assert result.params["param"] == "allowed_value" + finally: + render_template_context.reset(token) + + def test_denied_env_var_raises_template_render_error(self, mocker): + mocker.patch.dict(os.environ, {"DENIED_VAR": "denied_value"}) + token = render_template_context.set({}) + try: + with pytest.raises(pydantic.ValidationError) as exc_info: + ImportStringWithParams.model_validate( + { + "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "params": {"param": "{{ DENIED_VAR }}"}, + } + ) + errors = exc_info.value.errors() + assert any("Invalid configuration" in e["msg"] for e in errors) + finally: + render_template_context.reset(token) + + def test_security_error_in_restricted_context_raises_template_render_error(self, mocker): + mocker.patch.dict(os.environ, {"SECRET": "secret_value"}) + token = render_template_context.set({"SECRET": "secret_value"}) + try: + with pytest.raises(pydantic.ValidationError) as exc_info: + ImportStringWithParams.model_validate( + { + "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "params": {"param": "{{ ''.__class__ }}"}, + } + ) + errors = exc_info.value.errors() + assert any("Invalid configuration" in e["msg"] for e in errors) + finally: + render_template_context.reset(token) + + def test_no_context_falls_back_to_full_environ(self, mocker): + mocker.patch.dict(os.environ, {"FULL_VAR": "full_value"}) + result = ImportStringWithParams.model_validate( + { + "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "params": {"param": "{{ FULL_VAR }}"}, + } + ) + assert result.params["param"] == "full_value" diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 0000000..cc7024e --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,78 @@ +import os + +import jinja2 +import pytest + +from _ravnar.utils import TemplateRenderError, render_template, render_template_context + + +class TestRenderTemplate: + def test_basic_math(self): + assert render_template("{{ 7 * 7 }}", {}) == "49" + + def test_env_var_access(self, mocker): + mocker.patch.dict(os.environ, {"TEST_HOME": "/home/test"}) + assert render_template("{{ TEST_HOME }}", dict(os.environ)) == "/home/test" + + def test_dunder_access_raises_security_error(self): + with pytest.raises(jinja2.exceptions.SecurityError): + render_template("{{ config.__class__ }}", {"config": "value"}) + + def test_self_dunder_access_raises_security_error(self): + with pytest.raises(jinja2.exceptions.SecurityError): + render_template("{{ ''.__class__.__mro__ }}", {}) + + def test_strict_undefined_raises_on_missing_var(self): + with pytest.raises(jinja2.exceptions.UndefinedError): + render_template("{{ MISSING_VAR }}", {}) + + def test_strict_undefined_default_filter(self): + assert render_template('{{ MISSING_VAR | default("fallback") }}', {}) == "fallback" + + def test_strict_undefined_is_defined_test(self): + assert render_template("{% if MISSING_VAR is defined %}yes{% else %}no{% endif %}", {}) == "no" + + def test_strict_undefined_conditional_access_raises(self): + with pytest.raises(jinja2.exceptions.UndefinedError): + render_template("{{ VAR if VAR else 'x' }}", {}) + + def test_dict_key_rendering(self): + assert render_template("{{ KEY }}", {"KEY": "value"}) == "value" + + def test_nested_dict(self): + result = render_template({"key": "{{ VAL }}"}, {"VAL": "v"}) + assert result == {"key": "v"} + + def test_nested_list(self): + result = render_template(["{{ A }}", "{{ B }}"], {"A": "1", "B": "2"}) + assert result == ["1", "2"] + + def test_non_string_passed_through(self): + assert render_template(42, {}) == 42 + assert render_template(None, {}) is None + + +class TestRenderTemplateContext: + def test_restricted_context_used_when_set(self, mocker): + mocker.patch.dict(os.environ, {"ALLOWED": "yes", "DENIED": "no"}) + token = render_template_context.set({"ALLOWED": "yes"}) + try: + assert render_template("{{ ALLOWED }}", render_template_context.get()) == "yes" + with pytest.raises(jinja2.exceptions.UndefinedError): + render_template("{{ DENIED }}", render_template_context.get()) + finally: + render_template_context.reset(token) + + def test_none_context_falls_back_to_os_environ(self, mocker): + mocker.patch.dict(os.environ, {"FALLBACK_VAR": "fallback_value"}) + assert render_template_context.get() is None + # When called directly with os.environ, it works + assert render_template("{{ FALLBACK_VAR }}", dict(os.environ)) == "fallback_value" + + +class TestTemplateRenderError: + def test_attributes(self): + exc = TemplateRenderError(template="{{ x }}", reason="UndefinedError", message="Invalid configuration") + assert exc.template == "{{ x }}" + assert exc.reason == "UndefinedError" + assert exc.message == "Invalid configuration" From 4d484b542cfa25a66f8488e831ac130adf90809a Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 09:36:47 +0200 Subject: [PATCH 08/14] cleanup --- src/_ravnar/api/agents.py | 15 ++++++++------- src/_ravnar/config.py | 20 ++++++++++---------- src/_ravnar/core.py | 9 ++++++--- src/_ravnar/utils.py | 35 +++++++++++++++++++---------------- tests/test_config.py | 14 +++++++------- tests/test_utils.py | 12 ++++++------ 6 files changed, 56 insertions(+), 49 deletions(-) diff --git a/src/_ravnar/api/agents.py b/src/_ravnar/api/agents.py index f3a55d2..33fcd5a 100644 --- a/src/_ravnar/api/agents.py +++ b/src/_ravnar/api/agents.py @@ -1,7 +1,6 @@ from __future__ import annotations -from collections.abc import Callable -from collections.abc import AsyncIterator +from collections.abc import Callable, Iterator from typing import TYPE_CHECKING, Annotated, Any import ag_ui.core @@ -10,7 +9,7 @@ from _ravnar import schema from _ravnar.security import User -from _ravnar.utils import render_template_context +from _ravnar.utils import ImportStringWithParams if TYPE_CHECKING: from _ravnar.core import AgentHandler @@ -51,11 +50,13 @@ def _make_dynamic_agents_router( "Can be checked with [`GET /api/config`](#/API/get_config_api_config_get)." ) - async def _set_restricted_template_context() -> AsyncIterator[None]: - render_template_context.set(agent_handler.get_dynamic_render_template_context()) - yield + def set_dynamic_agent_render_template_context() -> Iterator[None]: + with ImportStringWithParams.explicit_render_template_context( + agent_handler.get_dynamic_render_template_context() + ): + yield - @router.post("", description=description, dependencies=[Depends(_set_restricted_template_context)]) + @router.post("", description=description, dependencies=[Depends(set_dynamic_agent_render_template_context)]) async def register_agent( data: schema.RegisterAgentData, user: User = Depends(authorized_user_with("agents:write")), # noqa: B008 diff --git a/src/_ravnar/config.py b/src/_ravnar/config.py index 0086d85..a4c3db7 100644 --- a/src/_ravnar/config.py +++ b/src/_ravnar/config.py @@ -27,24 +27,24 @@ def interactive_session() -> bool: return sys.stdout.isatty() -class RenderableMixin: +class RenderableConfigMixin: @field_validator("*", mode="before") @classmethod def _render_templates(cls, data: Any) -> Any: return render_template(data, context=dict(os.environ)) -class LoggingConfig(BaseModel, RenderableMixin): +class LoggingConfig(BaseModel, RenderableConfigMixin): level: l2sl.LogLevel = l2sl.LogLevel("info") as_json: bool = Field(default_factory=lambda: not interactive_session()) -class TracingConfig(BaseModel, RenderableMixin): +class TracingConfig(BaseModel, RenderableConfigMixin): endpoint: str | None = None as_logs: bool = Field(default_factory=lambda values: interactive_session() and values["endpoint"] is None) -class ServerConfig(BaseModel, RenderableMixin): +class ServerConfig(BaseModel, RenderableConfigMixin): hostname: str = "127.0.0.1" port: int = 8000 proxy_headers: bool = False @@ -54,12 +54,12 @@ class ServerConfig(BaseModel, RenderableMixin): tracing: TracingConfig = Field(default_factory=TracingConfig) -class CORSConfig(BaseModel, RenderableMixin): +class CORSConfig(BaseModel, RenderableConfigMixin): allowed_origins: list[str] = Field(default_factory=lambda: ["*"]) allowed_headers: list[str] = Field(default_factory=list) -class SecurityConfig(BaseModel, RenderableMixin): +class SecurityConfig(BaseModel, RenderableConfigMixin): authenticator: ImportStringWithParams[Authenticator] | None = None cors: CORSConfig = Field(default_factory=CORSConfig) @@ -73,18 +73,18 @@ def _local_storage() -> Path: return p -class StorageConfig(BaseModel, RenderableMixin): +class StorageConfig(BaseModel, RenderableConfigMixin): enabled: bool = True database_dsn: str = Field(default_factory=lambda: f"sqlite:///{_local_storage() / 'state.db'}") file_storage_path: UPath = Field(default_factory=lambda: UPath(_local_storage() / "files")) -class DynamicAgentConfig(BaseModel, RenderableMixin): +class DynamicAgentConfig(BaseModel, RenderableConfigMixin): enabled: bool = False allowed_env_vars: list[str] = Field(default_factory=list) -class AgentConfig(BaseModel, RenderableMixin): +class AgentConfig(BaseModel, RenderableConfigMixin): static: dict[str, ImportStringWithParams[Agent]] = Field( default_factory=lambda: { # type: ignore[arg-type] "default": ImportStringWithParams(cls_or_fn=DefaultAgent), @@ -99,7 +99,7 @@ def _ensure_not_agentless(self) -> Self: return self -class BaseConfig(BaseSettings, RenderableMixin): +class BaseConfig(BaseSettings, RenderableConfigMixin): server: ServerConfig = Field(default_factory=ServerConfig) security: SecurityConfig = Field(default_factory=SecurityConfig) storage: StorageConfig = Field(default_factory=StorageConfig) diff --git a/src/_ravnar/core.py b/src/_ravnar/core.py index c4961f2..0855803 100644 --- a/src/_ravnar/core.py +++ b/src/_ravnar/core.py @@ -133,11 +133,14 @@ def __init__(self, agent_config: AgentConfig) -> None: self._dynamic_agents: dict[str, Agent] = {} self._event_encoder = ag_ui.encoder.EventEncoder() self._dynamic_enabled = agent_config.dynamic.enabled - self._dynamic_config = agent_config.dynamic + self._dynamic_allowed_env_vars = agent_config.dynamic.allowed_env_vars def get_dynamic_render_template_context(self) -> dict[str, str]: - allowed = self._dynamic_config.allowed_env_vars - return {k: v for k, v in os.environ.items() if k in allowed} + ctx = dict(os.environ) + if "*" in self._dynamic_allowed_env_vars: + return ctx + + return {k: v for k, v in ctx.items() if k in self._dynamic_allowed_env_vars} @staticmethod async def _setup_agent(agent: Agent) -> None: diff --git a/src/_ravnar/utils.py b/src/_ravnar/utils.py index 080b46f..b7a2bc6 100644 --- a/src/_ravnar/utils.py +++ b/src/_ravnar/utils.py @@ -9,7 +9,7 @@ import re from collections.abc import AsyncIterator, Awaitable, Callable, Iterator from datetime import UTC, datetime -from typing import Any, Generic, TypeVar, cast, get_type_hints +from typing import Any, ClassVar, Generic, TypeVar, cast, get_type_hints import jinja2 import jinja2.sandbox @@ -109,14 +109,7 @@ def now() -> datetime: return datetime.now(tz=UTC) -render_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar( - "render_template_context", default=None -) - - class TemplateRenderError(ValueError): - """Raised when template rendering fails in a restricted context.""" - def __init__(self, *, template: str, reason: str, message: str) -> None: self.template = template self.reason = reason @@ -136,6 +129,17 @@ def render_template(s: Any, context: dict[str, str]) -> Any: class ImportStringWithParams(BaseModel, Generic[T]): + _render_template_context: ClassVar[contextvars.ContextVar[dict[str, str] | None]] = contextvars.ContextVar( + "render_template_context", default=None + ) + + @contextlib.contextmanager + @classmethod + def explicit_render_template_context(cls, ctx: dict[str, str]) -> Iterator[dict[str, str]]: + cls._render_template_context.set(ctx) + yield ctx + cls._render_template_context.set(None) + cls_or_fn: ImportString[type[T] | Callable[..., T]] params: dict[str, Any] = Field(default_factory=dict) @@ -201,18 +205,17 @@ def _render_param_items(cls, params: dict[str, Any]) -> dict[str, Any]: @classmethod def _render_template(cls, s: Any) -> Any: - ctx = render_template_context.get() - if ctx is None: - ctx = dict(os.environ) + explicit_ctx = cls._render_template_context.get() + ctx = explicit_ctx if explicit_ctx is not None else dict(os.environ) try: return render_template(s, ctx) - except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as e: - if render_template_context.get() is not None: + except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as exc: + if explicit_ctx is not None: raise TemplateRenderError( - template=str(s), - reason=type(e).__name__, + template=str(exc), + reason=type(exc).__name__, message="Invalid configuration", - ) from e + ) from exc raise @model_serializer(mode="wrap") diff --git a/tests/test_config.py b/tests/test_config.py index 7fe4042..1e047a9 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -9,7 +9,7 @@ from _ravnar.agents import Agent from _ravnar.config import AgentConfig, BaseConfig, Config, DynamicAgentConfig, ImportStringWithParams -from _ravnar.utils import TemplateRenderError, render_template_context +from _ravnar.utils import _render_template_context @pytest.fixture() @@ -229,7 +229,7 @@ def test_import_string_with_params_nested_error_localization(): class TestImportStringWithParamsRestrictedContext: def test_allowed_env_var_renders(self, mocker): mocker.patch.dict(os.environ, {"ALLOWED_VAR": "allowed_value"}) - token = render_template_context.set({"ALLOWED_VAR": "allowed_value"}) + token = _render_template_context.set({"ALLOWED_VAR": "allowed_value"}) try: result = ImportStringWithParams.model_validate( { @@ -239,11 +239,11 @@ def test_allowed_env_var_renders(self, mocker): ) assert result.params["param"] == "allowed_value" finally: - render_template_context.reset(token) + _render_template_context.reset(token) def test_denied_env_var_raises_template_render_error(self, mocker): mocker.patch.dict(os.environ, {"DENIED_VAR": "denied_value"}) - token = render_template_context.set({}) + token = _render_template_context.set({}) try: with pytest.raises(pydantic.ValidationError) as exc_info: ImportStringWithParams.model_validate( @@ -255,11 +255,11 @@ def test_denied_env_var_raises_template_render_error(self, mocker): errors = exc_info.value.errors() assert any("Invalid configuration" in e["msg"] for e in errors) finally: - render_template_context.reset(token) + _render_template_context.reset(token) def test_security_error_in_restricted_context_raises_template_render_error(self, mocker): mocker.patch.dict(os.environ, {"SECRET": "secret_value"}) - token = render_template_context.set({"SECRET": "secret_value"}) + token = _render_template_context.set({"SECRET": "secret_value"}) try: with pytest.raises(pydantic.ValidationError) as exc_info: ImportStringWithParams.model_validate( @@ -271,7 +271,7 @@ def test_security_error_in_restricted_context_raises_template_render_error(self, errors = exc_info.value.errors() assert any("Invalid configuration" in e["msg"] for e in errors) finally: - render_template_context.reset(token) + _render_template_context.reset(token) def test_no_context_falls_back_to_full_environ(self, mocker): mocker.patch.dict(os.environ, {"FULL_VAR": "full_value"}) diff --git a/tests/test_utils.py b/tests/test_utils.py index cc7024e..0aea2f4 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,7 +3,7 @@ import jinja2 import pytest -from _ravnar.utils import TemplateRenderError, render_template, render_template_context +from _ravnar.utils import TemplateRenderError, _render_template_context, render_template class TestRenderTemplate: @@ -55,17 +55,17 @@ def test_non_string_passed_through(self): class TestRenderTemplateContext: def test_restricted_context_used_when_set(self, mocker): mocker.patch.dict(os.environ, {"ALLOWED": "yes", "DENIED": "no"}) - token = render_template_context.set({"ALLOWED": "yes"}) + token = _render_template_context.set({"ALLOWED": "yes"}) try: - assert render_template("{{ ALLOWED }}", render_template_context.get()) == "yes" + assert render_template("{{ ALLOWED }}", _render_template_context.get()) == "yes" with pytest.raises(jinja2.exceptions.UndefinedError): - render_template("{{ DENIED }}", render_template_context.get()) + render_template("{{ DENIED }}", _render_template_context.get()) finally: - render_template_context.reset(token) + _render_template_context.reset(token) def test_none_context_falls_back_to_os_environ(self, mocker): mocker.patch.dict(os.environ, {"FALLBACK_VAR": "fallback_value"}) - assert render_template_context.get() is None + assert _render_template_context.get() is None # When called directly with os.environ, it works assert render_template("{{ FALLBACK_VAR }}", dict(os.environ)) == "fallback_value" From 8b8be00dcbb14c90583d75dc8bb87ca06587e8b5 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 07:48:11 +0000 Subject: [PATCH 09/14] Fix tests and runtime bugs in sandboxed template rendering --- src/_ravnar/api/agents.py | 4 ++-- src/_ravnar/utils.py | 13 ++++++++----- tests/test_config.py | 17 ++++------------- tests/test_utils.py | 13 +++++-------- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/_ravnar/api/agents.py b/src/_ravnar/api/agents.py index 33fcd5a..458c13b 100644 --- a/src/_ravnar/api/agents.py +++ b/src/_ravnar/api/agents.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Callable, Iterator +from collections.abc import AsyncIterator, Callable from typing import TYPE_CHECKING, Annotated, Any import ag_ui.core @@ -50,7 +50,7 @@ def _make_dynamic_agents_router( "Can be checked with [`GET /api/config`](#/API/get_config_api_config_get)." ) - def set_dynamic_agent_render_template_context() -> Iterator[None]: + async def set_dynamic_agent_render_template_context() -> AsyncIterator[None]: with ImportStringWithParams.explicit_render_template_context( agent_handler.get_dynamic_render_template_context() ): diff --git a/src/_ravnar/utils.py b/src/_ravnar/utils.py index b7a2bc6..b8c0f2a 100644 --- a/src/_ravnar/utils.py +++ b/src/_ravnar/utils.py @@ -133,12 +133,15 @@ class ImportStringWithParams(BaseModel, Generic[T]): "render_template_context", default=None ) - @contextlib.contextmanager @classmethod - def explicit_render_template_context(cls, ctx: dict[str, str]) -> Iterator[dict[str, str]]: - cls._render_template_context.set(ctx) - yield ctx - cls._render_template_context.set(None) + def explicit_render_template_context(cls, ctx: dict[str, str]) -> contextlib.AbstractContextManager[dict[str, str]]: + @contextlib.contextmanager + def cm() -> Iterator[dict[str, str]]: + cls._render_template_context.set(ctx) + yield ctx + cls._render_template_context.set(None) + + return cm() cls_or_fn: ImportString[type[T] | Callable[..., T]] params: dict[str, Any] = Field(default_factory=dict) diff --git a/tests/test_config.py b/tests/test_config.py index 1e047a9..a103f40 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -9,7 +9,7 @@ from _ravnar.agents import Agent from _ravnar.config import AgentConfig, BaseConfig, Config, DynamicAgentConfig, ImportStringWithParams -from _ravnar.utils import _render_template_context +from _ravnar.utils import ImportStringWithParams @pytest.fixture() @@ -229,8 +229,7 @@ def test_import_string_with_params_nested_error_localization(): class TestImportStringWithParamsRestrictedContext: def test_allowed_env_var_renders(self, mocker): mocker.patch.dict(os.environ, {"ALLOWED_VAR": "allowed_value"}) - token = _render_template_context.set({"ALLOWED_VAR": "allowed_value"}) - try: + with ImportStringWithParams.explicit_render_template_context({"ALLOWED_VAR": "allowed_value"}): result = ImportStringWithParams.model_validate( { "cls_or_fn": f"{__name__}.{MockAgent.__name__}", @@ -238,13 +237,10 @@ def test_allowed_env_var_renders(self, mocker): } ) assert result.params["param"] == "allowed_value" - finally: - _render_template_context.reset(token) def test_denied_env_var_raises_template_render_error(self, mocker): mocker.patch.dict(os.environ, {"DENIED_VAR": "denied_value"}) - token = _render_template_context.set({}) - try: + with ImportStringWithParams.explicit_render_template_context({}): with pytest.raises(pydantic.ValidationError) as exc_info: ImportStringWithParams.model_validate( { @@ -254,13 +250,10 @@ def test_denied_env_var_raises_template_render_error(self, mocker): ) errors = exc_info.value.errors() assert any("Invalid configuration" in e["msg"] for e in errors) - finally: - _render_template_context.reset(token) def test_security_error_in_restricted_context_raises_template_render_error(self, mocker): mocker.patch.dict(os.environ, {"SECRET": "secret_value"}) - token = _render_template_context.set({"SECRET": "secret_value"}) - try: + with ImportStringWithParams.explicit_render_template_context({"SECRET": "secret_value"}): with pytest.raises(pydantic.ValidationError) as exc_info: ImportStringWithParams.model_validate( { @@ -270,8 +263,6 @@ def test_security_error_in_restricted_context_raises_template_render_error(self, ) errors = exc_info.value.errors() assert any("Invalid configuration" in e["msg"] for e in errors) - finally: - _render_template_context.reset(token) def test_no_context_falls_back_to_full_environ(self, mocker): mocker.patch.dict(os.environ, {"FULL_VAR": "full_value"}) diff --git a/tests/test_utils.py b/tests/test_utils.py index 0aea2f4..7546a80 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,7 +3,7 @@ import jinja2 import pytest -from _ravnar.utils import TemplateRenderError, _render_template_context, render_template +from _ravnar.utils import ImportStringWithParams, TemplateRenderError, render_template class TestRenderTemplate: @@ -55,17 +55,14 @@ def test_non_string_passed_through(self): class TestRenderTemplateContext: def test_restricted_context_used_when_set(self, mocker): mocker.patch.dict(os.environ, {"ALLOWED": "yes", "DENIED": "no"}) - token = _render_template_context.set({"ALLOWED": "yes"}) - try: - assert render_template("{{ ALLOWED }}", _render_template_context.get()) == "yes" + with ImportStringWithParams.explicit_render_template_context({"ALLOWED": "yes"}): + assert render_template("{{ ALLOWED }}", ImportStringWithParams._render_template_context.get()) == "yes" with pytest.raises(jinja2.exceptions.UndefinedError): - render_template("{{ DENIED }}", _render_template_context.get()) - finally: - _render_template_context.reset(token) + render_template("{{ DENIED }}", ImportStringWithParams._render_template_context.get()) def test_none_context_falls_back_to_os_environ(self, mocker): mocker.patch.dict(os.environ, {"FALLBACK_VAR": "fallback_value"}) - assert _render_template_context.get() is None + assert ImportStringWithParams._render_template_context.get() is None # When called directly with os.environ, it works assert render_template("{{ FALLBACK_VAR }}", dict(os.environ)) == "fallback_value" From 84188ba7ebfb595a5a33564891fffc6c4c7d5c2b Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 09:58:38 +0200 Subject: [PATCH 10/14] add wildcard to allowlist --- src/_ravnar/config.py | 24 ++++++++++++++---------- tests/test_config.py | 1 - 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/_ravnar/config.py b/src/_ravnar/config.py index a4c3db7..37b8808 100644 --- a/src/_ravnar/config.py +++ b/src/_ravnar/config.py @@ -3,15 +3,10 @@ import os import sys from pathlib import Path -from typing import Any, Self, TypeVar +from typing import Annotated, Any, Self, TypeVar import l2sl -from pydantic import ( - BaseModel, - Field, - field_validator, - model_validator, -) +from pydantic import AfterValidator, BaseModel, Field, field_validator, model_validator from pydantic_settings import BaseSettings, PydanticBaseSettingsSource, SettingsConfigDict, YamlConfigSettingsSource from upath import UPath @@ -27,6 +22,15 @@ def interactive_session() -> bool: return sys.stdout.isatty() +def _validate_allowlist_wildcard(allowlist: list[str]) -> list[str]: + if "*" in allowlist and len(allowlist) > 1: + raise ValueError('Wildcard "*" must be the sole allowlist entry. It cannot be combined with other entries.') + return allowlist + + +Allowlist = Annotated[list[str], AfterValidator(_validate_allowlist_wildcard)] + + class RenderableConfigMixin: @field_validator("*", mode="before") @classmethod @@ -55,8 +59,8 @@ class ServerConfig(BaseModel, RenderableConfigMixin): class CORSConfig(BaseModel, RenderableConfigMixin): - allowed_origins: list[str] = Field(default_factory=lambda: ["*"]) - allowed_headers: list[str] = Field(default_factory=list) + allowed_origins: Allowlist = Field(default_factory=lambda: ["*"]) + allowed_headers: Allowlist = Field(default_factory=list) class SecurityConfig(BaseModel, RenderableConfigMixin): @@ -81,7 +85,7 @@ class StorageConfig(BaseModel, RenderableConfigMixin): class DynamicAgentConfig(BaseModel, RenderableConfigMixin): enabled: bool = False - allowed_env_vars: list[str] = Field(default_factory=list) + allowed_env_vars: Allowlist = Field(default_factory=list) class AgentConfig(BaseModel, RenderableConfigMixin): diff --git a/tests/test_config.py b/tests/test_config.py index a103f40..5f9d4af 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -9,7 +9,6 @@ from _ravnar.agents import Agent from _ravnar.config import AgentConfig, BaseConfig, Config, DynamicAgentConfig, ImportStringWithParams -from _ravnar.utils import ImportStringWithParams @pytest.fixture() From 98b3a5c3ddce72e11f2a539a5f993028060fb32f Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 08:04:23 +0000 Subject: [PATCH 11/14] Add tests for Allowlist type wildcard validation --- tests/test_config.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_config.py b/tests/test_config.py index 5f9d4af..3cde140 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -272,3 +272,17 @@ def test_no_context_falls_back_to_full_environ(self, mocker): } ) assert result.params["param"] == "full_value" + + +class TestAllowlist: + def test_valid_entries(self): + config = DynamicAgentConfig(enabled=True, allowed_env_vars=["HOME", "USER"]) + assert config.allowed_env_vars == ["HOME", "USER"] + + def test_wildcard_only(self): + config = DynamicAgentConfig(enabled=True, allowed_env_vars=["*"]) + assert config.allowed_env_vars == ["*"] + + def test_wildcard_with_other_entries_raises(self, matches="Wildcard"): + with pytest.raises(pydantic.ValidationError): + DynamicAgentConfig(enabled=True, allowed_env_vars=["*", "HOME"]) From 131eb9381eca0244394cbf877ae1cc4c5675381d Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 10:07:05 +0200 Subject: [PATCH 12/14] remove design --- DESIGN.md | 377 ------------------------------------------------------ 1 file changed, 377 deletions(-) delete mode 100644 DESIGN.md diff --git a/DESIGN.md b/DESIGN.md deleted file mode 100644 index 764542e..0000000 --- a/DESIGN.md +++ /dev/null @@ -1,377 +0,0 @@ -# Design: Jinja2 Sandboxing for Config Template Rendering - -## Summary - -Replace the unrestricted `jinja2.Environment` used in config template rendering with -`jinja2.sandbox.SandboxedEnvironment`. For **startup config rendering**, use the full `os.environ` with -`StrictUndefined` to catch typos. For **runtime agent registration** (dynamic agents via API), use a configurable -allowlist of environment variables (default empty) to prevent a privilege-escalation exfiltration attack where a user -with `agents:write` permission reads arbitrary secrets via `getCapabilities()`. The admin explicitly controls which env -vars are exposed to dynamic agents via `agents.dynamic.allowed_env_vars` in the config. - -## Goals - -- Prevent Jinja2 template injection from escalating to arbitrary code execution on the server. -- Limit the attack surface in the event an attacker gains write access to a config file or environment variable. -- Maintain all existing functionality for legitimate template usage (e.g., referencing `$HOME`, `$RAVNAR_*` variables in - config values). -- Keep the change small and contained to `utils.py`. - -## Non-Goals - -- Removing the template rendering feature entirely — it is useful for dynamic configuration. -- Adding per-value access control (different env vars for different config values). -- Replacing Jinja2 with a different templating engine. -- Auditing each config value to determine whether it is safe to render (the concern is the renderer, not the value). - -## Background / Motivation - -Ravnar's configuration system applies Jinja2 template rendering to all config values via `render_template()` in -`utils.py`: - -```python -def render_template(s: Any) -> Any: - if isinstance(s, str): - return jinja2.Environment().from_string(s).render(**os.environ) -``` - -This function is called as part of `RenderableMixin._render_templates`, which processes every value in the YAML config -tree and every `RAVNAR_*` environment variable. The rendered values are then used to construct the final `Config` -object. - -The current code has two problems: - -1. **No sandbox:** `jinja2.Environment()` is the standard environment, which allows access to Python builtins through - the template sandbox escape technique (`{{ config.__class__.__init__.__globals__ }}`). While this requires an - attacker to control a config value or env var, the lack of sandboxing means a single bypass of config-file protection - leads to code execution. - -2. **Runtime exfiltration via dynamic agents:** A user with `agents:write` permission can register an agent whose - parameters contain template expressions like `{{ AWS_SECRET_ACCESS_KEY }}`. The template renders during - `ImportStringWithParams` validation, substituting the secret into the agent's config. The agent's `getCapabilities()` - returns this value, and the user can read it via `GET /api/agents` (requires only `agents:read`, a standard - permission). This is a privilege escalation: a write-scoped attacker exfiltrates read-scoped secrets from the - environment. - -The fix uses `SandboxedEnvironment` for all rendering, with a restricted context only for runtime agent registration. -This blocks code execution in all cases and prevents the exfiltration attack while preserving the UX of full env-var -access for legitimate config file authoring. - -## Design - -### 1. Sandboxed Environment - -Replace `jinja2.Environment()` with `jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined)`: - -```python -import contextvars -import jinja2 -import jinja2.sandbox - -render_template_context: contextvars.ContextVar[dict[str, str] | None] = contextvars.ContextVar( - "render_template_context", default=None -) - -def render_template(s: Any, context: dict[str, str]) -> Any: - if isinstance(s, str): - env = jinja2.sandbox.SandboxedEnvironment(undefined=jinja2.StrictUndefined) - return env.from_string(s).render(**context) - if isinstance(s, dict): - return {render_template(k, context): render_template(v, context) for k, v in s.items()} - if isinstance(s, list): - return [render_template(v, context) for v in s] - return s -``` - -`SandboxedEnvironment` blocks access to: - -- Dunder attributes (`__class__`, `__bases__`, `__subclasses__`, `__globals__`, etc.) -- Built-in functions (`eval`, `exec`, `open`, `import`, etc.) -- Methods flagged as unsafe (`__call__` on callables that are not explicitly safe) - -`StrictUndefined` raises `UndefinedError` if any variable referenced in a template is missing from the context. This -prevents silent misconfigurations caused by typos or missing env vars. - -`render_template` is a pure function: it always requires an explicit `context` dict. Callers decide what context to -pass. The `render_template_context` ContextVar is used only by `ImportStringWithParams._render_template` (see below) to -inject the restricted context during runtime agent registration. When the ContextVar is not set, -`ImportStringWithParams._render_template` falls back to the full `os.environ`. This is the behavior for startup config -rendering. - -This is the standard mitigation recommended by Jinja2's own documentation for security-sensitive applications. Combined -with `StrictUndefined`, it blocks code execution and prevents silent misconfigurations. - -### 2. Configurable Allowlist for Runtime Agent Registration - -Add `allowed_env_vars` to `DynamicAgentConfig`: - -```python -class DynamicAgentConfig(BaseModel, RenderableMixin): - enabled: bool = False - allowed_env_vars: list[str] = Field(default_factory=list) -``` - -The default is an empty list: **no environment variables are exposed to dynamic agents by default**. The admin must -explicitly list each variable name that should be available in templates during runtime agent registration. - -For runtime agent registration, the restricted context is constructed inline in the FastAPI dependency from the -configured allowlist. This context is **only** passed to `render_template` during `ImportStringWithParams` validation -for runtime agent registration (i.e., `RegisterAgentData`). It is **not** used for startup config rendering. - -**Passing the context to runtime rendering:** `render_template` is called from Pydantic validators -(`ImportStringWithParams._render_field_templates` and `_render_param_items`), which are class methods and do not receive -request state directly. The `render_template_context` ContextVar bridges this gap. A FastAPI `yield` dependency on the -dynamic agents routes sets the context variable before request body parsing, using the `AgentHandler` instance that is -already available in the router. - -```python -class AgentHandler: - def __init__(self, agent_config: AgentConfig) -> None: - self._static_agents: dict[str, Agent] = {id: factory() for id, factory in agent_config.static.items()} - self._dynamic_agents: dict[str, Agent] = {} - self._event_encoder = ag_ui.encoder.EventEncoder() - self._dynamic_enabled = agent_config.dynamic.enabled - self._dynamic_config = agent_config.dynamic - - def get_dynamic_render_template_context(self) -> dict[str, str]: - allowed = self._dynamic_config.allowed_env_vars - return {k: v for k, v in os.environ.items() if k in allowed} -``` - -The `_make_dynamic_agents_router` function receives `agent_handler` by closure, so it can define the `yield` dependency -inline without adding a new `Depends(get_config)`: - -```python -def _make_dynamic_agents_router( - router: schema.APIRouter, - *, - agent_handler: AgentHandler, - authorized_user_with: Callable[..., Any], -) -> None: - async def _set_restricted_template_context(): - render_template_context.set(agent_handler.get_dynamic_render_template_context()) - yield - - @router.post("", dependencies=[Depends(_set_restricted_template_context)]) - async def register_agent( - data: RegisterAgentData, - user: User = Depends(authorized_user_with("agents:write")), - ) -> AgentInfo: - agent = data.agent() - ... -``` - -The `yield` dependency runs before request body parsing. `render_template_context` is set, then Pydantic validators run -and call `ImportStringWithParams._render_template`, which reads the ContextVar and receives the restricted context. -After the response is generated, the `yield` dependency exits. Each request runs in its own asyncio task, so the -ContextVar is naturally garbage collected. For startup config, `render_template_context` is not set, so -`ImportStringWithParams._render_template` falls back to the full `os.environ`. - -**Why default-deny:** The attack model is a user with `agents:write` registering an agent that exfiltrates arbitrary env -vars via `getCapabilities()`. The admin controls the ravnar config file (or deployment configuration), so they can -explicitly decide which env vars are safe to expose to dynamic agents. A hardcoded whitelist would either be too -permissive (blocking legitimate use cases) or require code changes for every deployment. A config value lets the admin -make the security decision at deployment time. - -**Note:** Because `StrictUndefined` is used, any template referencing a non-allowed env var will raise `UndefinedError` -at runtime. For conditional values, use the `default` filter (`{{ VAR | default("fallback") }}`) or the `is defined` -test (`{% if VAR is defined %}...{% endif %}`) instead of `if VAR else`, which evaluates the variable in boolean context -and triggers `UndefinedError`. - -### 3. Dict Key Rendering - -The function `ImportStringWithParams._render_param_items` currently calls `render_template()` on both keys and values of -the params dict: - -```python -return {cls._render_template(k): cls._render_template(v) for k, v in params.items()} -``` - -This means env-var-derived strings can also be parameter names. With the sandboxing change this is less dangerous, but -it is still surprising behavior. The fix here is narrower: only render keys if they are strings containing template -syntax (`{{` or `{%`). A simpler approach is to apply `render_template` to all string keys as before — the sandboxing is -the real defense, and changing key-rendering behavior could break someone who relies on it (unlikely but possible). -**Decision: leave key rendering as-is, since sandboxing covers the risk.** - -`ImportStringWithParams` validators call `cls._render_template`, which injects the context. The -`RenderableMixin._render_templates` validator calls `render_template` directly with the full `os.environ` context: - -```python -class RenderableMixin: - @field_validator("*", mode="before") - @classmethod - def _render_templates(cls, data: Any) -> Any: - return render_template(data, context=dict(os.environ)) - -class ImportStringWithParams(BaseModel, Generic[T]): - @field_validator("cls_or_fn", "params", mode="before") - @classmethod - def _render_field_templates(cls, f: Any) -> Any: - if isinstance(f, str): - return cls._render_template(f) - return f - - @field_validator("params", mode="after") - @classmethod - def _render_param_items(cls, params: dict[str, Any]) -> dict[str, Any]: - return {cls._render_template(k): cls._render_template(v) for k, v in params.items()} - - @classmethod - def _render_template(cls, s: Any) -> Any: - ctx = render_template_context.get() - if ctx is None: - ctx = dict(os.environ) - return render_template(s, ctx) -``` - -During runtime agent registration, the ContextVar is set to the restricted context, so -`ImportStringWithParams._render_template` reads it and passes it to `render_template`. For startup config, -`RenderableMixin._render_templates` calls `render_template` directly with `os.environ`, bypassing the ContextVar -entirely. The `ImportStringWithParams` validators that run afterward receive plain strings (mostly no-op), but if they -do call `_render_template`, the ContextVar is not set, so they fall back to the full `os.environ`. - -### 4. SecurityError Handling - -`SandboxedEnvironment` raises `jinja2.exceptions.SecurityError` when a template attempts dunder access, dangerous -builtins, or other sandboxed operations. The handling depends on when the template is rendered: - -**Startup config rendering** (`RenderableMixin`, `ImportStringWithParams` during startup model validation): -`SecurityError` must **propagate** (fail-closed). If a malicious or malformed template is present in the YAML config or -a `RAVNAR_*` environment variable, the server should crash at startup with a clear error. There is no legitimate reason -for a config template to trigger a `SecurityError`. - -**Runtime restricted-context rendering** (`RegisterAgentData` via API, or any future feature that uses the restricted -context): `SecurityError` and `UndefinedError` must be caught and converted to a client-friendly error so the API client -receives a proper error response instead of an unhandled `500 Internal Server Error`. - -Because Pydantic validators wrap every exception they catch in `ValidationError`, `HTTPException` raised from a validator -would not reach FastAPI's `HTTPException` handler. Instead, the exception is trapped inside `RequestValidationError` → -`422`. To avoid this, `ImportStringWithParams._render_template` catches `SecurityError`/`UndefinedError` and re-raises a -custom `TemplateRenderError` when `render_template_context` is set (restricted context). The custom exception carries -the relevant data (template, reason, message) so an exception handler can convert it to a `400 Bad Request` with a -generic client-facing message while preserving the full details for server-side logging. - -```python -# utils.py -class TemplateRenderError(Exception): - """Raised when template rendering fails in a restricted context.""" - - def __init__(self, *, template: str, reason: str, message: str) -> None: - self.template = template - self.reason = reason - self.message = message - super().__init__(message) - - -class ImportStringWithParams(BaseModel, Generic[T]): - @classmethod - def _render_template(cls, s: Any) -> Any: - ctx = render_template_context.get() - if ctx is None: - ctx = dict(os.environ) - try: - return render_template(s, ctx) - except (jinja2.exceptions.SecurityError, jinja2.exceptions.UndefinedError) as e: - if render_template_context.get() is not None: - raise TemplateRenderError( - template=str(s), - reason=type(e).__name__, - message="Invalid configuration", - ) from e - raise -``` - -A FastAPI exception handler for `RequestValidationError` is added in `Ravnar._make_app`. It inspects the error list -and, if any error is a `value_error` whose `ctx.error` is a `TemplateRenderError`, logs the full details server-side -with `structlog` at the `warning` level and returns a `400 Bad Request` with a generic `{"detail": "Invalid configuration"}` -response. The admin sees the detailed log message; the API client does not. - -```python -from fastapi.exception_handlers import request_validation_exception_handler -from fastapi.exceptions import RequestValidationError - -@app.exception_handler(RequestValidationError) -async def _template_render_validation_handler(request, exc): - for error in exc.errors(): - if error.get("type") == "value_error": - original = error.get("ctx", {}).get("error") - if isinstance(original, TemplateRenderError): - structlog.get_logger().warning( - "Template rendering blocked", - template=original.template, - reason=original.reason, - error=str(original.__cause__), - ) - return JSONResponse( - status_code=400, - content={"detail": original.message}, - ) - return await request_validation_exception_handler(request, exc) -``` - -**Why this approach:** -- The `_render_template` classmethod already knows whether it's in a restricted context because `render_template_context` is set. -- It catches the exception and replaces it with a distinct `TemplateRenderError` that carries the relevant data. -- Pydantic wraps `TemplateRenderError` in a `ValidationError` with `type: "value_error"`, but the original exception lives in `ctx.error`. -- The exception handler inspects `RequestValidationError.errors()` and looks for `TemplateRenderError` in `ctx.error`. -- If found, it logs the full details and returns `400` (no `422`). Otherwise, it falls back to FastAPI's default validation handler. -- The `TemplateRenderError` class has no agent-specific connotation — it is purely about template rendering — so it can be reused if other features introduce restricted-context rendering in the future. - -### 5. Startup Logging - -No explicit warning is added for the sandboxing change — it is a silent fix. No logging changes are required. - -## Tradeoffs & Risks - -- **Template features that are sandboxed:** `SandboxedEnvironment` blocks dunder access, dangerous builtins, and - arbitrary calls. It does **not** block `range()`, `cycler()`, `joiner()`, `namespace()`, or `{% extends %}` / - `{% include %}` — but since no template loader is configured, file-based inheritance is already non-functional. None - of these features are used in ravnar's config templates (they are short, single-line expressions like - `{{ HOME }}/data`). -- **StrictUndefined breakage:** `StrictUndefined` raises on any access to a missing variable. This breaks the common - Jinja2 idiom `{{ VAR if VAR else "default" }}` because the boolean check counts as an access. The supported - alternatives are `{{ VAR | default("default") }}` and `{% if VAR is defined %}...{% endif %}`. This is a deliberate - breaking change for security: silent empty strings are unacceptable for config values. -- **Allowlist configuration:** The admin must explicitly configure `agents.dynamic.allowed_env_vars` for each env var - needed by dynamic agents. This is a one-time deployment decision per variable. The default-deny model is secure but - requires admin awareness. If a user tries to register a dynamic agent that references an unallowed env var, the - registration fails with `UndefinedError`. The server log includes a clear message naming the undefined variable and - suggesting the admin add it to `agents.dynamic.allowed_env_vars`. The HTTP response is generic to avoid leaking - information. -- **Jinja2 sandbox security:** The sandbox is not perfect — sandbox escapes have been found in Jinja2 before. However, - it raises the bar significantly. The restricted context further limits what an attacker could reach even with a - sandbox escape. -- **Performance:** `SandboxedEnvironment` has a small overhead compared to `Environment`. Config rendering happens once - at startup, so this is negligible. - -## Testing Strategy - -- **Unit tests for sandbox:** - - Render `{{ 7 * 7 }}` → `49` (basic math still works). - - Render `{{ HOME }}` → value of `$HOME` (env var access works). - - Render `{{ RAVNAR_SOME_VAR }}` → value of that env var. - - Render `{{ config.__class__ }}` → raises `SecurityError` (fail-closed at startup, converted to HTTPException at - runtime). - - Render `{{ self.__class__.__mro__ }}` → raises `SecurityError`. - - Render `{{ ''.__class__.__mro__ }}` → raises `SecurityError`. -- **Unit tests for restricted context (runtime only):** - - Env vars in `allowed_env_vars` are accessible during runtime agent registration. - - Env vars not in `allowed_env_vars` raise `UndefinedError` during runtime agent registration. - - Default empty allowlist: any env var reference raises `UndefinedError`. - - `StrictUndefined` tests: `{{ VAR | default("x") }}` → `"x"`; `{% if VAR is defined %}...{% endif %}` → renders - fallback; `{{ VAR if VAR else "x" }}` → `UndefinedError`. -- **Integration tests:** - - Start ravnar with a config that uses `{{ HOME }}` in a path — verify the path resolves correctly. - - Start ravnar with a config that uses an arbitrary env var (e.g., `{{ MY_SECRET }}`) — verify the path resolves - correctly (full context is available for config). - - Register a dynamic agent via API with `allowed_env_vars` empty — verify any env var reference fails with - `UndefinedError`. - - Register a dynamic agent via API with `allowed_env_vars` containing `AWS_SECRET_ACCESS_KEY` — verify the template - renders correctly. - - Register a dynamic agent via API with `allowed_env_vars` configured but a non-allowed env var referenced — verify it - fails with `UndefinedError`. -- **No e2e tests needed.** - -## Open Questions - -_(none — all design decisions are resolved)_ From 8e8215fe9228ff947cd55aa273e996215aac0f0e Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 10:59:43 +0200 Subject: [PATCH 13/14] test cleanup --- tests/api/test_agents.py | 4 +-- tests/test_config.py | 65 +++++++++++++++++++++++----------------- tests/test_utils.py | 25 +--------------- 3 files changed, 40 insertions(+), 54 deletions(-) diff --git a/tests/api/test_agents.py b/tests/api/test_agents.py index acaf9b9..2a3bd78 100644 --- a/tests/api/test_agents.py +++ b/tests/api/test_agents.py @@ -269,7 +269,7 @@ def test_register_agent_with_allowed_env_var(self, client): json={ "id": "allowed-env-agent", "agent": { - "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "cls_or_fn": MockAgent, "params": {"param": "{{ ALLOWED_VAR }}"}, }, }, @@ -284,7 +284,7 @@ def test_register_agent_with_denied_env_var(self, client): json={ "id": "denied-env-agent", "agent": { - "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "cls_or_fn": MockAgent, "params": {"param": "{{ DENIED_VAR }}"}, }, }, diff --git a/tests/test_config.py b/tests/test_config.py index 3cde140..cb8347e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -227,51 +227,60 @@ def test_import_string_with_params_nested_error_localization(): class TestImportStringWithParamsRestrictedContext: def test_allowed_env_var_renders(self, mocker): - mocker.patch.dict(os.environ, {"ALLOWED_VAR": "allowed_value"}) - with ImportStringWithParams.explicit_render_template_context({"ALLOWED_VAR": "allowed_value"}): + var = "ALLOWED_VAR" + value = "allowed_value" + + mocker.patch.dict(os.environ, {var: value}) + with ImportStringWithParams.explicit_render_template_context({var: value}): result = ImportStringWithParams.model_validate( { - "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "cls_or_fn": MockAgent, "params": {"param": "{{ ALLOWED_VAR }}"}, } ) - assert result.params["param"] == "allowed_value" + assert result.params["param"] == value def test_denied_env_var_raises_template_render_error(self, mocker): - mocker.patch.dict(os.environ, {"DENIED_VAR": "denied_value"}) - with ImportStringWithParams.explicit_render_template_context({}): - with pytest.raises(pydantic.ValidationError) as exc_info: - ImportStringWithParams.model_validate( - { - "cls_or_fn": f"{__name__}.{MockAgent.__name__}", - "params": {"param": "{{ DENIED_VAR }}"}, - } - ) - errors = exc_info.value.errors() - assert any("Invalid configuration" in e["msg"] for e in errors) + var = "DENIED_VAR" + value = "denied_value" + + mocker.patch.dict(os.environ, {var: value}) + with ( + ImportStringWithParams.explicit_render_template_context({}), + pytest.raises(pydantic.ValidationError, match="Invalid configuration"), + ): + ImportStringWithParams.model_validate( + { + "cls_or_fn": MockAgent, + "params": {"param": "{{ DENIED_VAR }}"}, + } + ) def test_security_error_in_restricted_context_raises_template_render_error(self, mocker): mocker.patch.dict(os.environ, {"SECRET": "secret_value"}) - with ImportStringWithParams.explicit_render_template_context({"SECRET": "secret_value"}): - with pytest.raises(pydantic.ValidationError) as exc_info: - ImportStringWithParams.model_validate( - { - "cls_or_fn": f"{__name__}.{MockAgent.__name__}", - "params": {"param": "{{ ''.__class__ }}"}, - } - ) - errors = exc_info.value.errors() - assert any("Invalid configuration" in e["msg"] for e in errors) + with ( + ImportStringWithParams.explicit_render_template_context({"SECRET": "secret_value"}), + pytest.raises(pydantic.ValidationError, match="Invalid configuration"), + ): + ImportStringWithParams.model_validate( + { + "cls_or_fn": MockAgent, + "params": {"param": "{{ ''.__class__ }}"}, + } + ) def test_no_context_falls_back_to_full_environ(self, mocker): - mocker.patch.dict(os.environ, {"FULL_VAR": "full_value"}) + var = "FULL_VAR" + value = "full_value" + + mocker.patch.dict(os.environ, {var: value}) result = ImportStringWithParams.model_validate( { - "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "cls_or_fn": MockAgent, "params": {"param": "{{ FULL_VAR }}"}, } ) - assert result.params["param"] == "full_value" + assert result.params["param"] == value class TestAllowlist: diff --git a/tests/test_utils.py b/tests/test_utils.py index 7546a80..b923566 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -3,7 +3,7 @@ import jinja2 import pytest -from _ravnar.utils import ImportStringWithParams, TemplateRenderError, render_template +from _ravnar.utils import render_template class TestRenderTemplate: @@ -50,26 +50,3 @@ def test_nested_list(self): def test_non_string_passed_through(self): assert render_template(42, {}) == 42 assert render_template(None, {}) is None - - -class TestRenderTemplateContext: - def test_restricted_context_used_when_set(self, mocker): - mocker.patch.dict(os.environ, {"ALLOWED": "yes", "DENIED": "no"}) - with ImportStringWithParams.explicit_render_template_context({"ALLOWED": "yes"}): - assert render_template("{{ ALLOWED }}", ImportStringWithParams._render_template_context.get()) == "yes" - with pytest.raises(jinja2.exceptions.UndefinedError): - render_template("{{ DENIED }}", ImportStringWithParams._render_template_context.get()) - - def test_none_context_falls_back_to_os_environ(self, mocker): - mocker.patch.dict(os.environ, {"FALLBACK_VAR": "fallback_value"}) - assert ImportStringWithParams._render_template_context.get() is None - # When called directly with os.environ, it works - assert render_template("{{ FALLBACK_VAR }}", dict(os.environ)) == "fallback_value" - - -class TestTemplateRenderError: - def test_attributes(self): - exc = TemplateRenderError(template="{{ x }}", reason="UndefinedError", message="Invalid configuration") - assert exc.template == "{{ x }}" - assert exc.reason == "UndefinedError" - assert exc.message == "Invalid configuration" From 563b17b3e477c7e7e1c3e7c8abac271ec9a7f4d8 Mon Sep 17 00:00:00 2001 From: Philip Meier Date: Tue, 9 Jun 2026 15:21:00 +0200 Subject: [PATCH 14/14] fix tests --- tests/api/test_agents.py | 20 +++++--------------- tests/test_config.py | 15 +++------------ tests/utils.py | 10 ++++++++++ 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/tests/api/test_agents.py b/tests/api/test_agents.py index 2a3bd78..cef4bb6 100644 --- a/tests/api/test_agents.py +++ b/tests/api/test_agents.py @@ -7,18 +7,8 @@ import ravnar.agents from _ravnar import schema -from _ravnar.agents import Agent from _ravnar.config import BaseConfig -from tests.utils import HeaderAuthenticator, make_app_client - - -class MockAgent(Agent): - def __init__(self, param="unset"): - self.param = param - - async def run(self, input): - raise AssertionError - yield +from tests.utils import HeaderAuthenticator, MockAgent, make_app_client def make_config(*, dynamic_enabled=False): @@ -222,7 +212,7 @@ def test_register_agent_with_env_var_default_deny(self, client): json={ "id": "env-agent", "agent": { - "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "cls_or_fn": f"{MockAgent.__module__}.{MockAgent.__name__}", "params": {"param": "{{ HOME }}"}, }, }, @@ -236,7 +226,7 @@ def test_register_agent_with_sandbox_escape(self, client): json={ "id": "sandbox-agent", "agent": { - "cls_or_fn": f"tests.api.test_agents.{MockAgent.__name__}", + "cls_or_fn": f"{MockAgent.__module__}.{MockAgent.__name__}", "params": {"param": "{{ ''.__class__ }}"}, }, }, @@ -269,7 +259,7 @@ def test_register_agent_with_allowed_env_var(self, client): json={ "id": "allowed-env-agent", "agent": { - "cls_or_fn": MockAgent, + "cls_or_fn": f"{MockAgent.__module__}.{MockAgent.__name__}", "params": {"param": "{{ ALLOWED_VAR }}"}, }, }, @@ -284,7 +274,7 @@ def test_register_agent_with_denied_env_var(self, client): json={ "id": "denied-env-agent", "agent": { - "cls_or_fn": MockAgent, + "cls_or_fn": f"{MockAgent.__module__}.{MockAgent.__name__}", "params": {"param": "{{ DENIED_VAR }}"}, }, }, diff --git a/tests/test_config.py b/tests/test_config.py index cb8347e..1b49eba 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -7,8 +7,8 @@ import yaml from pydantic_settings import BaseSettings, PydanticBaseSettingsSource, YamlConfigSettingsSource -from _ravnar.agents import Agent from _ravnar.config import AgentConfig, BaseConfig, Config, DynamicAgentConfig, ImportStringWithParams +from tests.utils import MockAgent @pytest.fixture() @@ -167,19 +167,10 @@ def test_template_rendering_in_list(mocker, make_test_config, source): assert config.security.cors.allowed_origins == [f"https://{app_domain}"] -class MockAgent(Agent): - def __init__(self, param="unset"): - self.param = param - - async def run(self, input): - raise AssertionError - yield - - @pytest.mark.parametrize("source", ["file", "env", "env_json"]) @pytest.mark.parametrize("input_type", ["plain", "object"]) def test_import_string_with_params(make_test_config, source, input_type): - import_path = f"{__name__}.{MockAgent.__name__}" + import_path = f"{MockAgent.__module__}.{MockAgent.__name__}" default_param = "unset" explicit_param = "sentinel" @@ -211,7 +202,7 @@ def test_import_string_with_params_nested_error_localization(): with pytest.raises(pydantic.ValidationError) as exc_info: ImportStringWithParams.model_validate( { - "cls_or_fn": f"{__name__}.{MockAgent.__name__}", + "cls_or_fn": f"{MockAgent.__module__}.{MockAgent.__name__}", "params": {"param": {"cls_or_fn": "non_existing_module.NonExistingClass"}}, } ) diff --git a/tests/utils.py b/tests/utils.py index 601e99a..e30d166 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -9,6 +9,7 @@ from fastapi.security import APIKeyHeader from fastapi.testclient import TestClient as _TestClient +from _ravnar.agents import Agent from _ravnar.config import BaseConfig from _ravnar.core import Ravnar from _ravnar.security import ALL_PERMISSIONS, User @@ -109,3 +110,12 @@ def safe_extract_response_content(response): decoded_content = content.decode() decoded_content = f"\n{json.dumps(json.loads(content), indent=2)}" return decoded_content + + +class MockAgent(Agent): + def __init__(self, param="unset"): + self.param = param + + async def run(self, input): + raise AssertionError + yield