[AAP-76179] fix(security): enforce sandboxed Jinja2 rendering in EDA server - #1577
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces per-call Jinja NativeTemplate rendering with a shared ImmutableSandboxedEnvironment (StrictUndefined) across string utilities, credentials validation, and activation ports; it maps sandbox SecurityError/UndefinedError to appropriate exceptions, removes substitute_extra_vars, adds SSTI regression unit tests, and updates one integration expected payload. ChangesSSTI Prevention Implementation and Testing
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aap_eda/core/utils/credentials.py (1)
517-524:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize Jinja sandbox
SecurityErrorinto injector validation errors
ImmutableSandboxedEnvironmentraisesjinja2.exceptions.SecurityErrorfor blocked attribute access, but_check_jinja_string()only remapsUndefinedError, andvalidate_injectors()doesn’t catchSecurityError, so it can escape instead of being collected in the"injectors"error list.Suggested fix
def _check_jinja_string(value: str, context: dict) -> str: try: if "{{" in value and "}}" in value: result = _SANDBOXED_ENV.from_string(value).render(context) if isinstance(result, jinja2.runtime.StrictUndefined): raise InjectorMissingKeyException(f"{value} is undefined") except jinja2.exceptions.UndefinedError: raise InjectorMissingKeyException(f"{value} is undefined") + except jinja2.exceptions.SecurityError as e: + raise InjectorInvalidTemplateKey(str(e))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/core/utils/credentials.py` around lines 517 - 524, The _check_jinja_string function currently only remaps jinja2.exceptions.UndefinedError to InjectorMissingKeyException, so add handling for jinja2.exceptions.SecurityError (raised by ImmutableSandboxedEnvironment on blocked attribute access) by catching SecurityError and raising InjectorMissingKeyException with the same message as for UndefinedError; update the exception handling in _check_jinja_string (and ensure validate_injectors continues to treat InjectorMissingKeyException as a validation error) so SecurityError is normalized into the existing "injectors" error collection instead of escaping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/core/utils/strings.py`:
- Around line 27-32: _render_string currently renders templates via
_SANDBOXED_ENV.from_string(...).render(), which always returns strings and can
break typed extra_vars; change _render_string to use a Jinja environment that
preserves native result types (or explicitly coerce rendered outputs back to
native types) so templated bool/int/list/dict values remain native for
downstream yaml.safe_load; update or add regression tests for
substitute_variables/substitute_extra_vars to cover scalar and collection typed
outputs (booleans, ints, lists, dicts) to ensure round-tripping.
In `@src/aap_eda/services/activation/engine/ports.py`:
- Around line 17-20: The find_ports() error handling currently only maps
ValueError and jinja2.exceptions.UndefinedError to ActivationStartError, letting
jinja2.sandbox.SecurityError escape; update the exception handler in
find_ports() to also catch jinja2.sandbox.SecurityError (import it from
jinja2.sandbox) and re-raise it as exceptions.ActivationStartError with an
appropriate message so sandbox violations from render_string() (which uses
_SANDBOXED_ENV.from_string) are normalized to ActivationStartError.
---
Outside diff comments:
In `@src/aap_eda/core/utils/credentials.py`:
- Around line 517-524: The _check_jinja_string function currently only remaps
jinja2.exceptions.UndefinedError to InjectorMissingKeyException, so add handling
for jinja2.exceptions.SecurityError (raised by ImmutableSandboxedEnvironment on
blocked attribute access) by catching SecurityError and raising
InjectorMissingKeyException with the same message as for UndefinedError; update
the exception handling in _check_jinja_string (and ensure validate_injectors
continues to treat InjectorMissingKeyException as a validation error) so
SecurityError is normalized into the existing "injectors" error collection
instead of escaping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e61dbb57-8a2d-4979-8d59-c0194c77db52
📒 Files selected for processing (4)
src/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/strings.pysrc/aap_eda/services/activation/engine/ports.pytests/unit/test_ssti_prevention.py
B-Whitt
force-pushed
the
fix-cwe/AAP-76179-Jinja2-rendering
branch
from
June 1, 2026 21:32
0332a3e to
ecfbf54
Compare
Contributor
Author
|
/run-e2e |
mkanoor
reviewed
Jun 1, 2026
B-Whitt
force-pushed
the
fix-cwe/AAP-76179-Jinja2-rendering
branch
from
June 1, 2026 22:14
ecfbf54 to
5a42e36
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/aap_eda/services/activation/engine/ports.py (1)
73-76: 💤 Low valueChain exception for consistency.
Same as credentials.py - consider chaining the SecurityError for better debugging. The UndefinedError on line 74 has the same pattern but wasn't flagged by static analysis (both could benefit from chaining).
♻️ Proposed fix
except UndefinedError as e: - raise exceptions.ActivationStartError(str(e)) + raise exceptions.ActivationStartError(str(e)) from e except SecurityError as e: - raise exceptions.ActivationStartError(str(e)) + raise exceptions.ActivationStartError(str(e)) from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/engine/ports.py` around lines 73 - 76, The except blocks that catch UndefinedError and SecurityError should chain the original exception into the ActivationStartError to preserve traceback and context; update both handlers that currently do "raise exceptions.ActivationStartError(str(e))" to raise exceptions.ActivationStartError(...) from e so the original UndefinedError/SecurityError is preserved (look for the except UndefinedError and except SecurityError handling ActivationStartError in this module).src/aap_eda/core/utils/credentials.py (1)
523-526: 💤 Low valueChain exceptions for better debugging.
Static analysis correctly flags that exceptions raised within except blocks should chain to the original exception for clearer tracebacks.
♻️ Proposed fix to chain exceptions
except jinja2.exceptions.UndefinedError: - raise InjectorMissingKeyException(f"{value} is undefined") + raise InjectorMissingKeyException(f"{value} is undefined") from None except jinja2.exceptions.SecurityError as e: - raise InjectorInvalidTemplateKey(str(e)) + raise InjectorInvalidTemplateKey(str(e)) from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/core/utils/credentials.py` around lines 523 - 526, The except handlers should chain the original jinja2 exceptions to preserve tracebacks: change the first handler to catch jinja2.exceptions.UndefinedError as e and raise InjectorMissingKeyException(f"{value} is undefined") from e, and change the second handler to raise InjectorInvalidTemplateKey(str(e)) from e (it already captures e). Update the except clauses in credentials.py around the jinja2 handlers to use "as e" where missing and append "from e" to the raised custom exceptions so the original exception is chained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/aap_eda/core/utils/credentials.py`:
- Around line 523-526: The except handlers should chain the original jinja2
exceptions to preserve tracebacks: change the first handler to catch
jinja2.exceptions.UndefinedError as e and raise
InjectorMissingKeyException(f"{value} is undefined") from e, and change the
second handler to raise InjectorInvalidTemplateKey(str(e)) from e (it already
captures e). Update the except clauses in credentials.py around the jinja2
handlers to use "as e" where missing and append "from e" to the raised custom
exceptions so the original exception is chained.
In `@src/aap_eda/services/activation/engine/ports.py`:
- Around line 73-76: The except blocks that catch UndefinedError and
SecurityError should chain the original exception into the ActivationStartError
to preserve traceback and context; update both handlers that currently do "raise
exceptions.ActivationStartError(str(e))" to raise
exceptions.ActivationStartError(...) from e so the original
UndefinedError/SecurityError is preserved (look for the except UndefinedError
and except SecurityError handling ActivationStartError in this module).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 31c639ff-525e-4e9a-850b-ba99285c453a
📒 Files selected for processing (5)
src/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/strings.pysrc/aap_eda/services/activation/engine/ports.pytests/integration/wsapi/test_consumer.pytests/unit/test_ssti_prevention.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_ssti_prevention.py
- src/aap_eda/core/utils/strings.py
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1577 +/- ##
==========================================
+ Coverage 92.32% 92.37% +0.05%
==========================================
Files 244 244
Lines 11214 11215 +1
==========================================
+ Hits 10353 10360 +7
+ Misses 861 855 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
wfealdel
reviewed
Jun 2, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_ssti_prevention.py (1)
63-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a regression that proves
settingsis no longer in template context.This file covers sandboxing, but it no longer verifies the other security fix called out in the PR: removing Django
settingsfrom the render context. Without an assertion that something like{{ settings.SECRET_KEY }}is unavailable, a future change could reintroduce secret exposure while all SSTI tests still pass.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Suggested test shape
class TestSandboxEnforcement: """Each rendering call site rejects SSTI payloads.""" + def test_settings_are_not_available_in_template_context(self): + with pytest.raises(Exception): + substitute_variables( + {"a": "{{ settings.SECRET_KEY }}"}, + {}, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_ssti_prevention.py` around lines 63 - 79, Add a regression test to TestLegitimateTemplates that renders "{{ settings.SECRET_KEY }}" via _render_string (and/or ports_render_string/substitute_variables variants) with an empty context and asserts the output does NOT expose django.conf.settings.SECRET_KEY; import django.conf.settings in the test and assert the rendered value is either empty/Undefined or at minimum not equal to settings.SECRET_KEY to ensure the Django settings object is not present in the template context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/test_ssti_prevention.py`:
- Around line 63-79: Add a regression test to TestLegitimateTemplates that
renders "{{ settings.SECRET_KEY }}" via _render_string (and/or
ports_render_string/substitute_variables variants) with an empty context and
asserts the output does NOT expose django.conf.settings.SECRET_KEY; import
django.conf.settings in the test and assert the rendered value is either
empty/Undefined or at minimum not equal to settings.SECRET_KEY to ensure the
Django settings object is not present in the template context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 3222812e-1082-49fc-bb7b-1046c71d5c24
📒 Files selected for processing (2)
src/aap_eda/core/utils/strings.pytests/unit/test_ssti_prevention.py
💤 Files with no reviewable changes (1)
- src/aap_eda/core/utils/strings.py
jcraiglo1
previously approved these changes
Jun 2, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aap_eda/core/utils/strings.py (1)
25-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
StrictUndefinedfailures in_render_string
_SANDBOXED_ENVusesjinja2.StrictUndefined, but_render_stringonly catchesjinja2.exceptions.SecurityError; missing variables raisejinja2.exceptions.UndefinedErrorwhich bubbles up throughsubstitute_variables()into bothsrc/aap_eda/wsapi/consumers.pyandsrc/aap_eda/api/serializers/activation.pywithout being handled.Suggested fix
-from jinja2.exceptions import SecurityError +from jinja2.exceptions import SecurityError, UndefinedError @@ - except SecurityError: - raise ValueError( - f"Template contains unsafe operations: {value}" - ) + except (SecurityError, UndefinedError) as err: + raise ValueError( + f"Invalid template: {value}" + ) from err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/core/utils/strings.py` around lines 25 - 37, The function _render_string should normalize StrictUndefined failures so UndefinedError doesn't bubble up; update _render_string to catch jinja2.exceptions.UndefinedError in addition to SecurityError (or catch Exception subclasses from jinja2 that represent undefined/strict failures) and re-raise a ValueError with a clear message (same pattern used for SecurityError) so substitute_variables(), consumers and serializers receive a consistent ValueError; reference _SANDBOXED_ENV and _render_string when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/aap_eda/core/utils/strings.py`:
- Around line 25-37: The function _render_string should normalize
StrictUndefined failures so UndefinedError doesn't bubble up; update
_render_string to catch jinja2.exceptions.UndefinedError in addition to
SecurityError (or catch Exception subclasses from jinja2 that represent
undefined/strict failures) and re-raise a ValueError with a clear message (same
pattern used for SecurityError) so substitute_variables(), consumers and
serializers receive a consistent ValueError; reference _SANDBOXED_ENV and
_render_string when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2f9f16ed-acf6-47b8-bfa8-ea2f7eef8a6f
📒 Files selected for processing (2)
src/aap_eda/core/utils/strings.pytests/unit/test_ssti_prevention.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/test_ssti_prevention.py
B-Whitt
force-pushed
the
fix-cwe/AAP-76179-Jinja2-rendering
branch
from
June 2, 2026 15:00
18ed9ec to
2f53453
Compare
wfealdel
reviewed
Jun 2, 2026
AlexSCorey
previously approved these changes
Jun 2, 2026
AlexSCorey
left a comment
AlexSCorey
left a comment
Member
There was a problem hiding this comment.
Looks good to me. It addresses the most pressing vulnerabilities.
Replace unsandboxed jinja2.nativetypes.NativeTemplate with jinja2.sandbox.ImmutableSandboxedEnvironment in all three rendering call sites to prevent server-side template injection (SSTI): - strings.py: sandbox _render_string(); remove Django settings object (SECRET_KEY, DATABASES, REDIS_URL) from substitute_extra_vars() context - credentials.py: sandbox _check_jinja_string() in credential validation - ports.py: sandbox render_string() in port resolution Add 9 regression tests covering sandbox enforcement across all three call sites, legitimate template rendering, and settings removal. Resolves: AAP-76179 Assisted-by: Claude Code / Opus 4.6 (Anthropic)
B-Whitt
force-pushed
the
fix-cwe/AAP-76179-Jinja2-rendering
branch
from
June 3, 2026 19:09
2f53453 to
2b30e2f
Compare
|
wfealdel
approved these changes
Jun 3, 2026
Contributor
|
/run-e2e |
mkanoor
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replace unsandboxed
jinja2.nativetypes.NativeTemplatewithjinja2.sandbox.ImmutableSandboxedEnvironmentin all three renderingcall sites to prevent server-side template injection (SSTI):
strings.py: sandbox_render_string(); remove Django settings object(
SECRET_KEY,DATABASES,REDIS_URL) fromsubstitute_extra_vars()contextcredentials.py: sandbox_check_jinja_string()in credential validationports.py: sandboxrender_string()in port resolutionWhy
The EDA server uses unsandboxed
NativeTemplatewhich allows arbitraryPython object traversal via template expressions. The
substitute_extra_vars()function also passes the entire Django settings object into the template
context, exposing secrets to any user who can craft activation extra_vars.
This matches the approach AWX already uses
(
jinja2.sandbox.ImmutableSandboxedEnvironmentatcredential/__init__.py:519).Test plan
Resolves: AAP-76179
Assisted-by: Claude Code / Opus 4.6 (Anthropic)
Summary by CodeRabbit