Harden Jinja2 prompt rendering with ImmutableSandboxedEnvironment (CWE-1336 defense-in-depth)#2408
Harden Jinja2 prompt rendering with ImmutableSandboxedEnvironment (CWE-1336 defense-in-depth)#2408JAE0Y2N wants to merge 3 commits into
Conversation
…WE-1336 defense-in-depth) Switch all 13 Environment(undefined=StrictUndefined) instantiations across the 11 tools that render prompts loaded from settings to ImmutableSandboxedEnvironment. Defense-in-depth against template-injection escalation when prompt templates are sourced from .pr_agent.toml or other configuration that may be influenced by untrusted input. Shipped templates use only the safe feature subset (variable substitution, conditionals, loops, filters), so the change is non-breaking for first-party prompts. Sister AI-agent frameworks (marvin, griptape) recently adopted the same defense for the same class of risk.
Review Summary by QodoHarden Jinja2 prompt rendering with ImmutableSandboxedEnvironment
WalkthroughsDescription• Replace 13 Jinja2 Environment instances with ImmutableSandboxedEnvironment • Defense-in-depth against template-injection escalation (CWE-1336/CWE-94) • Blocks unsafe attribute access and sandbox-escape primitives • Non-breaking change; all shipped prompts use only safe features Diagramflowchart LR
A["Jinja2 Environment<br/>undefined=StrictUndefined"] -->|"Replace with"| B["ImmutableSandboxedEnvironment<br/>undefined=StrictUndefined"]
B -->|"Blocks"| C["Unsafe attributes<br/>__init__, __globals__, __class__"]
B -->|"Prevents"| D["Sandbox-escape<br/>primitives"]
D -->|"Mitigates"| E["CWE-1336<br/>Template Injection"]
File Changes1. pr_agent/algo/token_handler.py
|
Code Review by Qodo
1. pr_reviewer missing SecurityError handling
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…_docs imports, add sandbox unit tests
(1) F401 unused-import fix: ruff -- the 11 files that touch jinja2 had
'from jinja2 import Environment, StrictUndefined' but after the
sandbox migration Environment is no longer referenced. Surgical
removal of just Environment, preserving StrictUndefined.
(2) pr_help_docs.py import order: stdlib (math/os/re/tempfile)
re-ordered above third-party (jinja2) per PEP 8 / isort grouping.
(3) Regression coverage: tests/unittest/test_jinja_sandbox.py exercises
both arms of the sandbox swap — safe templates (variable
substitution, conditionals, for-loops, dict-attr, filters) render
identically, and unsafe templates (__class__, __subclasses__,
cycler.__init__.__globals__.os.popen) raise SecurityError.
Addresses all three rule violations flagged in the Qodo code review on
PR The-PR-Agent#2408.
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
…party Addresses Qodo review item 2: stdlib imports (math, os, re, tempfile) were placed after third-party jinja2 imports. Reordered to put stdlib first per PEP 8 / isort grouping conventions.
|
Persistent review updated to latest commit 1e98f05 |
| environment = ImmutableSandboxedEnvironment(undefined=StrictUndefined) | ||
| system_prompt = environment.from_string(get_settings().pr_review_prompt.system).render(variables) | ||
| user_prompt = environment.from_string(get_settings().pr_review_prompt.user).render(variables) |
There was a problem hiding this comment.
1. pr_reviewer missing securityerror handling 📘 Rule violation ☼ Reliability
ImmutableSandboxedEnvironment rendering can raise jinja2.exceptions.SecurityError (and other template errors) when repo-supplied prompts contain unsafe or invalid constructs. _get_prediction() does not catch these exceptions, so the review flow can crash instead of failing safely with a clear error.
Agent Prompt
## Issue description
Switching to `ImmutableSandboxedEnvironment` introduces explicit `SecurityError` failures for unsafe template access; rendering can also raise other `jinja2` template exceptions. The current code does not handle these errors, which can crash the tool instead of returning a safe/clear failure.
## Issue Context
Prompt templates are configurable and may be influenced by repository configuration. With the sandbox enabled, unsafe prompt payloads will now deterministically raise `SecurityError` during `.render()`.
## Fix Focus Areas
- pr_agent/tools/pr_reviewer.py[214-226]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
Switch all 13 Jinja2
Environment(undefined=StrictUndefined)instantiations toImmutableSandboxedEnvironment(undefined=StrictUndefined). This is defense-in-depth against template-injection escalation (CWE-1336 / CWE-94) when prompt templates are sourced from configuration that may be influenced by untrusted input.Background
apply_repo_settings(pr_agent/git_providers/utils.py:14-80) loads the target repo's.pr_agent.tomlvia Dynaconf withmerge_enabled=Trueand no key-allowlist:The repo-side
.pr_agent.tomltherefore can override any settings value — including the prompt templates under[pr_code_suggestions_prompt],[pr_review_prompt],[pr_description_prompt], etc.Those prompts are subsequently fed into Jinja2's
Environment(undefined=StrictUndefined)and rendered:The same pattern recurs across 13 sites in 11 files:
pr_agent/algo/token_handler.pypr_agent/tools/pr_add_docs.pypr_agent/tools/pr_code_suggestions.pypr_agent/tools/pr_description.pypr_agent/tools/pr_generate_labels.pypr_agent/tools/pr_help_docs.pypr_agent/tools/pr_help_message.pypr_agent/tools/pr_line_questions.pypr_agent/tools/pr_questions.pypr_agent/tools/pr_reviewer.pypr_agent/tools/pr_update_changelog.pyThe vanilla
Environmentpermits Jinja2 sandbox-escape primitives —__class__.__mro__.__subclasses__(),cycler.__init__.__globals__.os, etc. — that can reach Python's stdlib and execute arbitrary code in pr-agent's runtime context.ImmutableSandboxedEnvironmentblocks unsafe attribute access (__init__,__globals__,__class__,__mro__, …) and mutation operations on shared collections, which is Jinja2's canonical defense for this class of issue. The same defense was recently adopted by sister AI-agent frameworksPrefectHQ/marvinandgriptape-ai/griptapefor the same threat class.Verification
Local smoke test against the patched
Environmentconstructor:All shipped
[*_prompt]template sections inpr_agent/settings/*.tomluse only the safe feature subset (variable substitution,{% if %},{% for %}, filters liketojson); none of them depend on__class__,__init__,__globals__, or any other surface thatImmutableSandboxedEnvironmentrestricts. The change is therefore non-breaking for current first-party prompts.Patch shape
Per file, two lines change:
The
Environmentimport is left in place because the existingfrom jinja2 import Environment, StrictUndefinedline still serves theStrictUndefinedimport (andImmutableSandboxedEnvironmentis a subclass ofEnvironment, so any code that downstream-type-checks againstEnvironmentcontinues to work).Defense-in-depth framing
get_repo_settings()inpr_agent/git_providers/github_provider.py:733-738deliberately reads.pr_agent.tomlfrom the repository's default branch — not from the PR's head — which already prevents PR-authors from injecting malicious settings via a single PR. That existing defense is sound.This change complements it by ensuring that even in the case where the repo's own default-branch configuration is somehow influenced by untrusted input (compromised maintainer account, supply-chain incident, accidental inclusion of user-data fields in prompt templates, etc.), the Jinja2 rendering step itself is sandboxed and cannot escalate to RCE in the pr-agent runtime. The runtime typically holds the GitHub App token, LLM API keys, and OS environment, so making this layer safe-by-construction reduces blast radius across the entire deployment population.
Related
PrefectHQ/marvin#1348,griptape-ai/griptape#2183