fix(t2i): validate template content to prevent Jinja2 SSTI injection#8077
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The SSTI blacklist regexes look quite broad (e.g., matching any
os.orsubprocess.usage and any__class__-like dunder chain), which may block legitimate safe templates; consider tightening patterns or scoping them to Jinja expression blocks only to reduce false positives. - The validation error messages returned from the backend are currently English-only and passed straight through to the toast UI; if you care about localization/UX consistency, you may want to map these to i18n-friendly keys or wrap them in a more user-oriented message on the frontend.
- You now repeat the
error?.response?.data?.message || error?.message || String(error)pattern in several places inT2ITemplateEditor.vue; consider pulling this into a small helper (e.g.,extractAxiosErrorMessage(error)) to keep the error-handling logic consistent and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SSTI blacklist regexes look quite broad (e.g., matching any `os.` or `subprocess.` usage and any `__class__`-like dunder chain), which may block legitimate safe templates; consider tightening patterns or scoping them to Jinja expression blocks only to reduce false positives.
- The validation error messages returned from the backend are currently English-only and passed straight through to the toast UI; if you care about localization/UX consistency, you may want to map these to i18n-friendly keys or wrap them in a more user-oriented message on the frontend.
- You now repeat the `error?.response?.data?.message || error?.message || String(error)` pattern in several places in `T2ITemplateEditor.vue`; consider pulling this into a small helper (e.g., `extractAxiosErrorMessage(error)`) to keep the error-handling logic consistent and easier to maintain.
## Individual Comments
### Comment 1
<location path="astrbot/core/utils/t2i/template_manager.py" line_range="20" />
<code_context>
+ ("flask_context", re.compile(r"\{\{.*?\b(config|request|session|g)\b.*?\}\}")),
+]
+
+_VAR_RE = re.compile(r"\{\{\s*(\w+)\s*(\|[^}]*)?\}\}")
+
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Current variable regex allows bypassing strict validation via more complex Jinja2 expressions.
`_VAR_RE` only matches `{{ name }}` with an optional filter, so any more complex Jinja2 expression (e.g. attribute/index access, grouped expressions) won’t be checked against the allowlist and can slip through strict mode. To avoid bypasses and a false sense of safety, either inspect templates via a proper Jinja2 parser/sandbox (AST-level) or constrain the template surface so that only simple `{{ var }}` placeholders are allowed and reject anything else using `{{` that doesn’t match this rule.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request implements Server-Side Template Injection (SSTI) protection for T2I templates by introducing a validation utility that uses blacklists and variable allowlisting. It also enhances the frontend dashboard by adding toast notifications for template operations and updates existing tests to comply with the new variable restrictions. Feedback focuses on critical security improvements to the validation logic to prevent bypasses via Jinja2 control structures or complex expressions and recommends adding dedicated unit tests for the security features.
| ] | ||
|
|
||
| _VAR_RE = re.compile(r"\{\{\s*(\w+)\s*(\|[^}]*)?\}\}") | ||
|
|
||
|
|
||
| def validate_template_content(content: str, *, strict: bool = False) -> None: | ||
| for label, pattern in _SSTI_BLACKLIST: | ||
| if pattern.search(content): | ||
| logger.warning(f"SSTI validation blocked template: matched rule [{label}]") |
There was a problem hiding this comment.
The strict validation logic is currently bypassable. It uses _VAR_RE.finditer(content), which only iterates over expressions that match the specific {{ var }} pattern. Any expression that does not match this regex (e.g., {{ config.DEBUG }} or {{ text + config }}) is simply skipped by the loop and thus accepted, leading to a security vulnerability.
In strict mode, the logic should ensure that every template expression found in the content is validated against the allowlist and restricted to simple variable access. Additionally, {% ... %} blocks should be disallowed if the goal is to only permit the three provided variables.
if strict:
# Block control structures {% ... %}
if re.search(r"\{%.*?%\}", content, re.DOTALL):
logger.warning("SSTI validation blocked template: matched control structure")
raise ValueError("Control structures ({% ... %}) are not allowed in strict mode.")
# Ensure all {{ ... }} blocks match the allowlist and are simple
for expr in re.findall(r"\{\{.*?\}\}", content, re.DOTALL):
m = _VAR_RE.fullmatch(expr)
if not m:
logger.warning(f"SSTI validation blocked template: complex expression '{expr}'")
raise ValueError(f"Complex expressions are not allowed in templates: {expr}")
var = m.group(1)
if var not in _ALLOWED_VARS:
logger.warning(f"SSTI validation blocked template: unauthorized variable '{var}'")
raise ValueError(
f"Unauthorized Jinja2 variable '{var}'; "
f"allowed: {', '.join(sorted(_ALLOWED_VARS))}."
)| _SSTI_BLACKLIST: list[tuple[str, re.Pattern]] = [ | ||
| ( | ||
| "dunder_chain", | ||
| re.compile( |
There was a problem hiding this comment.
The flask_context blacklist rule currently only checks for sensitive keywords within {{ ... }} blocks. An attacker could potentially bypass this by using Jinja2 control structures like {% if config.DEBUG %} or {% set x = config %}. It is safer to check for these keywords within any template tag.
| re.compile( | |
| ("flask_context", re.compile(r"\{[\{%].*?\b(config|request|session|g)\b.*?[\}%]\}")), |
| json={ | ||
| "name": template_name, | ||
| "content": "<html><body>{{ content }}</body></html>", | ||
| "content": "<html><body>{{ text }}</body></html>", |
There was a problem hiding this comment.
While the existing tests have been updated to accommodate the new variable names, there are no new unit tests specifically verifying the SSTI protection logic. It is highly recommended to add test cases that attempt to bypass the validation using various payloads (e.g., dunder access, complex expressions, control blocks, and context variables like config) to ensure the security implementation is robust and to prevent regressions.
References
- New functionality should be accompanied by corresponding unit tests.
…strBotDevs#8077) * fix(t2i): validate template content to prevent Jinja2 SSTI injection * fix(t2i): add error feedback * fix(test): update assertion to match previous commits * style: format code
Fixes #7330
Modifications / 改动点
fix(t2i): prevent Jinja2 SSTI via template content validation
Added input validation for T2I template create/update operations to block malicious Jinja2 payloads (CWE-1336). Previously, templates submitted through the dashboard API were stored unsanitized and sent as-is to the remote Jinja2 renderer, allowing authenticated users to inject server-side template expressions.
Validation uses a two-tier approach:
Security warnings are logged when templates are blocked, and the dashboard editor now surfaces API errors as toast notifications instead of silently swallowing them.
Plugin-side rendering via html_render() is unaffected — it passes template strings directly and does not go through the TemplateManager path.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Validate T2I HTML templates to prevent unsafe Jinja2 usage and surface validation errors in the dashboard UI.
Bug Fixes:
Enhancements:
Tests: