Skip to content

Replace eval() of model output in task handlers with literal parser (close #1567) - #1568

Open
Jiangrong-W wants to merge 1 commit into
TransformerOptimus:mainfrom
Jiangrong-W:harness-fix/superagi-eval-output-handlers-consolidated
Open

Jiangrong-W wants to merge 1 commit into
TransformerOptimus:mainfrom
Jiangrong-W:harness-fix/superagi-eval-output-handlers-consolidated

Conversation

@Jiangrong-W

Copy link
Copy Markdown

Description

The task/step output handlers in the agent loop evaluated raw model output with
Python eval(), which allowed a prompt-injected or adversarial model reply to run
arbitrary code on the backend host. This change replaces the unsafe eval() on
model output with a single shared literal-only parser, so a legitimate array of
task strings is still accepted while any embedded code (function calls, imports)
is rejected. There is no user-facing behavior change for legitimate replies.

Related Issues

Closes #1567

Solution and Design

Root cause: three handlers took the raw LLM completion (response['content']),
passed it through the purely cosmetic JsonCleaner.extract_json_array_section()
(which only slices the substring between the first [ and the last ], with no
content validation), and then called eval() on the result. Because the input is
attacker-influenceable model text, a reply such as [__import__('os').system(...)]
or [__import__('subprocess').getoutput(...)] was evaluated as Python and executed
on the host.

The fix introduces one shared guard and wires it into all three call-sites:

  • New JsonCleaner.parse_array_section() in superagi/helper/json_cleaner.py.
    It reuses extract_json_array_section() to isolate the array section, then
    parses it with ast.literal_eval (stdlib only — no new dependency). It returns
    a list for a literal array and raises ValueError for anything that is not a
    literal list. ast.literal_eval accepts both single-quoted (str(dict)-style,
    as OpenAI emits) and double-quoted (JSON-style) arrays, so legitimate replies
    parse exactly as before, but embedded function calls/imports can never execute.
    Public signatures are unchanged.
  • TaskOutputHandler.handle(), ReplaceTaskOutputHandler.handle() and
    QueueStepHandler._process_reply() now call JsonCleaner.parse_array_section()
    instead of extract_json_array_section() followed by eval().

Why this closes the whole class: all three sinks shared the same root cause
(eval() of model output after a cosmetic slice). Routing every call-site through
one literal-only parser removes eval() from each path, so the same payload that
previously executed is now rejected with ValueError at every site.

Backwards compatibility: a legitimate literal list of task strings (single- or
double-quoted) parses to the same list it did before, the downstream
np.array(...).flatten().tolist() handling is unchanged, and all pre-existing
unit tests continue to pass.

Affected sites (one underlying vulnerability, three call-sites — all fixed):

  • superagi/agent/output_handler.py:149TaskOutputHandler.handle (eval(assistant_reply) on the output_type='tasks' branch of the default-seeded Dynamic Task Queue-I iteration workflow)
  • superagi/agent/output_handler.py:180ReplaceTaskOutputHandler.handle (eval(assistant_reply) on the output_type='replace_tasks' step of the same default workflow)
  • superagi/agent/queue_step_handler.py:79QueueStepHandler._process_reply (np.array(eval(assistant_reply))... on the TASK_QUEUE tool step seeded into the built-in Sales Engagement and Recruitment workflows)

Test Plan

Added focused regression tests covering all three call-sites, plus a
backwards-compatibility assertion:

  • tests/unit_tests/agent/test_output_handler.py::test_task_output_handle_rejects_code_execution
    — an os.system payload wrapped in [...] raises ValueError and the patched
    os.system is never called.
  • tests/unit_tests/agent/test_output_handler.py::test_replace_task_output_handle_rejects_code_execution
    — a subprocess.getoutput payload raises ValueError and the patched
    subprocess.getoutput is never called.
  • tests/unit_tests/agent/test_queue_step_handler.py::test_process_reply_rejects_code_execution
    — an os.system payload raises ValueError and no task is enqueued.
  • tests/unit_tests/agent/test_queue_step_handler.py::test_process_reply_parses_literal_array
    — a legitimate literal array is still parsed and the tasks are enqueued
    (backwards compatibility).

Before the fix these tests fail (the payloads are executed / no ValueError is
raised); after the fix they pass. The full unit suite for the touched modules was
run locally and is green:

ENV=DEV IS_TESTING=True ENCRYPTION_KEY=<32 chars> REDIS_URL=localhost:6379 \
  pytest tests/unit_tests/agent/test_output_handler.py \
         tests/unit_tests/agent/test_queue_step_handler.py \
         tests/unit_tests/helper/test_json_cleaner.py -v
...
======================== 16 passed, 1 warning =========================

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs update

Checklist

  • My pull request is atomic and focuses on a single change.
  • I have read the contributing guide and my code conforms to the guidelines.
  • I have documented my changes clearly and comprehensively.
  • I have added the required tests.

Three handlers passed the raw LLM reply through a cosmetic
JsonCleaner.extract_json_array_section() slice and then straight to eval():

  - TaskOutputHandler.handle()         (superagi/agent/output_handler.py)
  - ReplaceTaskOutputHandler.handle()  (superagi/agent/output_handler.py)
  - QueueStepHandler._process_reply()  (superagi/agent/queue_step_handler.py)

The cleaner only slices the substring between the first '[' and last ']'
without validating its contents, so a model reply such as
[__import__('os').system(...)] or [__import__('subprocess').getoutput(...)]
was evaluated as Python and executed arbitrary code on the backend host.
All three sinks ship on default-seeded paths: the
TaskOutputHandler/ReplaceTaskOutputHandler branches back the default
'Dynamic Task Queue-I' iteration workflow, and the TASK_QUEUE tool step that
reaches QueueStepHandler is seeded into the built-in Sales Engagement and
Recruitment workflows.

This is a single underlying vulnerability (unsafe eval of model output)
reachable through three distinct handler call-sites. The fix adds one shared
guard and wires it into all three:

  - Add JsonCleaner.parse_array_section(), which extracts the array section
    (reusing extract_json_array_section) and parses it with ast.literal_eval.
    Legitimate replies (a literal list of task strings, in either single- or
    double-quoted form as emitted by OpenAI) parse exactly as before, but
    embedded function calls/imports now raise ValueError instead of executing.
    Uses the stdlib ast module only; public signatures are unchanged.
  - TaskOutputHandler.handle(), ReplaceTaskOutputHandler.handle() and
    QueueStepHandler._process_reply() now call parse_array_section() instead
    of extract_json_array_section()+eval().

Adds regression tests for each call-site asserting that os.system /
subprocess.getoutput payloads are rejected (ValueError) and never invoked,
and that a legitimate literal array is still parsed and enqueued.

Signed-off-by: christop <825583681@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task/step output handlers run eval() on raw model output (code execution via model reply)

1 participant