Dynamic surveys: RCE, SSRF, cross-tenant cache, and unvalidated launch answers - #6
Merged
Merged
Conversation
It stubs Django by assigning MagicMocks into sys.modules, but not sys.modules['django.db'] -- and forail/__init__.py does `from django.db import connection` at import time. A bare MagicMock under 'django' is not a package, so collecting the file on its own failed before a single test ran. It only ever passed as a side effect of another test module importing first, which is why CI excluded it as "order-dependent". The exclusion made that permanent: 37 tests covering every dynamic_choices source type have not executed in CI. Stub django.db too, and put the file back in the matrix.
`_resolve_jinja2` rendered the survey's `dynamic_choices.template` through a
plain `jinja2.Environment`. Jinja seeds every environment with `cycler`,
`joiner`, `lipsum` and `namespace` -- class instances whose `__init__.__globals__`
is Python's module table. Anyone able to edit a job template's survey could
store a payload there, and any user with `start` permission then executed it by
opening the launch prompt, with the privileges of the web process.
Confirmed against the pre-fix code rather than argued from the source:
["x", "{{ cycler.__init__.__globals__.os.name }}"] -> ['x', 'posix']
["x", "{{ joiner.__init__.__globals__.os.getcwd() }}"] -> ['x', '/']
["x", "{{ cycler.__init__.__globals__.os.environ.get('PATH') }}"]
-> ['x', '/usr/local/bin:...:/bin']
Wrapping the expression in a JSON list is what makes the read observable: bare,
the rendered repr fails `json.loads` and the caller sees an empty list, so the
same payload looks harmless while it has already run.
The source type is now refused. `validate_dynamic_choices_config` rejects it, so
it cannot be saved, and `resolve_dynamic_choices` refuses before dispatching, so
specs already in the database stop executing without a migration. The refusal is
not cached -- caching it would hide the warning for the whole TTL.
`SURVEY_DYNAMIC_CHOICES_JINJA2_ENABLED` re-enables it, and is deliberately a
settings *file* value rather than a registered API setting: the surface used to
plant a payload must not also be able to switch on its execution. The flag is
read with `is True`, so a stray "true" or a 1 does not count. Even enabled, the
template renders in a `SandboxedEnvironment` with globals cleared, tests removed
and filters reduced to an allowlist -- hardening, not a boundary. Same payloads
against that path: SecurityError, empty list.
Tests cover both postures: refused by default (renderer never reached, nothing
cached), and, with the flag forced on, twelve known template-to-Python routes
returning nothing.
The page described the Jinja2 source type as a supported option and stated that its templates "run in a restricted sandbox (no file I/O)". There was no sandbox: the template rendered through a plain jinja2.Environment in the web process. That sentence is the one that most needed fixing -- it told a reader the risk had already been handled. Documents the withdrawal, what happens to surveys already saved with it, and the settings-file flag for an operator who accepts the risk.
`_resolve_api_endpoint` checked only that `url` was a non-empty string, then
issued the request from inside the cluster with redirects followed and arbitrary
headers and body. Whoever could edit a job template chose the address; any user
with `start` permission triggered the fetch and got the JSON back through the
dynamic-choices endpoint.
Demonstrated against the pre-fix code with a server bound to loopback:
http://127.0.0.1:8099/admin -> ['internal-secret-1', 'internal-secret-2']
Destinations are now named by an operator in
`SURVEY_DYNAMIC_CHOICES_API_ALLOWLIST`, matched host-exact -- no wildcards, no
suffix matching -- and empty by default, so the source type is off until someone
turns it on. Same reasoning as the jinja2 flag: it is a settings-file value, not
a registered API setting.
An allowlisted name is re-checked after resolution, which is what a bare
allowlist misses: a listed host whose DNS answer points at 169.254.169.254 is
refused. Private ranges are deliberately permitted -- an on-prem CMDB on
RFC1918 is the ordinary use of this feature, and naming the host is the trust
decision. Loopback, link-local, multicast, reserved and unspecified are not.
Also: https only, redirects never followed (the first hop is the one that was
checked; every hop after it is the peer's choice), methods limited to GET/POST,
the survey-supplied timeout capped at 30s so a hanging request cannot hold a web
worker indefinitely, the response read bounded at 1 MiB rather than trusting a
peer-supplied Content-Length, and the extracted list capped at MAX_CHOICES like
the other sources.
Validation refuses a non-permitted destination at save time too, so the editor
is told rather than left with a survey that silently resolves to nothing.
Not addressed here: `dynamic_choices.headers` still stores its values in the
survey spec in plaintext. That is now a secret sent only to an operator-named
host, but it belongs in a credential.
The key was the survey variable name plus an MD5 of the `dynamic_choices` config. Two job templates holding the same question with the same config -- different inventories, or different organizations -- produced the same key. The first request filled the cache with one tenant's host names and every later request was served them until the TTL expired. Even without a tenancy boundary crossed, the dropdown showed options from the wrong inventory. The key now hashes the question, the config, and the scope the answer was resolved in: template pk, organization id, inventory id. When there is no template to scope by, `_cache_key` returns None and the caller does not cache at all. Resolving every time costs a query; writing an entry that every tenant reads costs correctness. Switched to SHA-256 while touching it -- not for collision resistance in a cache key, but so no part of the codebase reads as if MD5 were an acceptable default.
For `multiplechoice` and `multiselect` with `dynamic_choices.enabled`, the
validator did nothing:
if dc and dc.get('enabled'):
pass # Dynamic choices are validated at resolve time
They were not. The resolve endpoint hands options to the UI and returns; nothing
on the launch path compared the submitted value against them. A direct API
client could send any extra_var it liked for a question that presents to a human
as a fixed dropdown -- which is the whole reason the dropdown exists.
The launch path now resolves the list and checks membership. A failed resolution
yields an empty list, and an empty list rejects every answer: if the permitted
values cannot be determined the request cannot be validated, and accepting it
would trust the caller for exactly the field this constrains. The options could
not have been offered in the UI either.
The logic went into the service rather than the mixin so it can actually be
tested -- forail/main/models/mixins.py cannot be imported by the standalone
suite, and a test that only runs on tag builds is not a test that guards a merge.
The mixin now delegates.
The failure message counts the permitted values instead of printing them (the
list runs to MAX_CHOICES), and says so plainly when the source resolved to
nothing, since that points at a broken source rather than a bad answer.
Documents what an administrator has to do before the api_endpoint source works at all, and the rules that apply even to a listed host. Also corrects two statements that were true of the old behaviour: that launch skips validation for dynamic questions, and that the cache key is the variable name plus a config hash. Says out loud that `headers` values sit in the survey spec in plaintext. They now only travel to an operator-named host, but that is a mitigation, not a secret store.
krlex
force-pushed
the
fix/dynamic-survey-jinja2-rce
branch
2 times, most recently
from
August 19, 2026 20:31
59b7961 to
3c0de8a
Compare
krlex
force-pushed
the
fix/dynamic-survey-jinja2-rce
branch
from
August 19, 2026 21:17
306766d to
be0507f
Compare
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
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.
Closes C1, H1, H2 and M1 from the 2026-08-19 Codex review — every finding in
the dynamic-survey subsystem. Each is demonstrated against the code as it stands
on
develop, not argued from reading it.C1 — Jinja2 rendered arbitrary Python in the web process
_resolve_jinja2used a plainjinja2.Environment. Jinja seeds everyenvironment with
cycler,joiner,lipsumandnamespace— class instanceswhose
__init__.__globals__is Python's module table.Wrapping the expression in a JSON list is what makes the read observable — bare,
the rendered repr fails
json.loadsand the caller sees[], so the payloadlooks inert while it has already run. Present in
mainand in every publishedtag from
v2026.05.0throughv2026.07.2-rc1.Refused at validation and at resolve, so specs already in the database stop
executing without a migration.
SURVEY_DYNAMIC_CHOICES_JINJA2_ENABLEDre-enablesit — a settings-file value, deliberately not a registered API setting, so the
surface used to plant a payload cannot switch on its execution. Enabled, it
renders sandboxed with globals cleared and filters reduced: same payloads come
back
SecurityError, empty list.H1 — the API source was an SSRF primitive
urlwas checked only for being a non-empty string. Against the pre-fix code,with a server bound to loopback:
Destinations are now named by an operator in
SURVEY_DYNAMIC_CHOICES_API_ALLOWLIST, host-exact, empty by default. Anallowlisted name is re-checked after DNS resolution — a listed host answering
169.254.169.254is refused — while private ranges stay allowed, because anon-prem CMDB is the ordinary use and naming the host is the trust decision.
HTTPS only, redirects never followed, methods limited to GET/POST, timeout capped
at 30s, response read bounded at 1 MiB.
H2 — the cache key crossed tenants
The key was the variable name plus an MD5 of the config. Two templates with the
same question and different inventories produced the same key, so the first
request filled the cache and the second was served its host names. The key now
covers template, organization and inventory; with no template to scope by,
nothing is cached at all.
M1 — launch never validated the answer
For dynamic questions the validator was literally
pass, with a comment sayingvalues were checked at resolve time. They were not: the resolve endpoint hands
options to the UI and returns. A direct API client could send any extra_var for a
question that presents as a fixed dropdown. Launch now resolves and checks
membership, failing closed — an answer that cannot be validated is not accepted.
The finding behind the findings
The test file covering all of this had never run. It stubs Django through
sys.modulesbut notdjango.db, andforail/__init__.pydoesfrom django.db import connectionat import — so collection failed before anytest executed. It passed only as a side effect of another module importing first,
which is why CI excluded it as "order-dependent". 37 tests covering every
dynamic_choicessource type had not run in CI.Fixed and back in the matrix. Also:
docs/13-dynamic-surveys.mdstated thetemplates "run in a restricted sandbox (no file I/O)". They did not.
Verified
tests_standalone/test_dynamic_survey_standalone.py: 96 passed (37 before,and none of those actually ran).
develophas 7.Pairs with forail-frontend#5, which stops offering the withdrawn source type.
Left open, deliberately
dynamic_choices.headersstill stores its values in the survey spec inplaintext. They now travel only to an operator-named host, but they belong in a
credential — that is a feature, not a fix, and it is called out in the docs.