-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix:react shell runtime policy #3169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
05f290e
79b57a7
8768b90
63fa724
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import ast | ||
| from pathlib import Path | ||
| from types import SimpleNamespace | ||
|
|
||
| import pytest | ||
|
|
||
| from dbgpt_app.openapi.api_v1.agentic_data_api import ( | ||
| _normalize_sandbox_execution_status, | ||
| _shell_validation_error, | ||
| ) | ||
| from dbgpt_sandbox.sandbox.execution_layer.base import ExecutionStatus | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("result", "expected"), | ||
| [ | ||
| ( | ||
| SimpleNamespace(status=ExecutionStatus.SUCCESS, exit_code=0), | ||
| ExecutionStatus.SUCCESS, | ||
| ), | ||
| ( | ||
| SimpleNamespace(status="success", exit_code=0), | ||
| ExecutionStatus.SUCCESS, | ||
| ), | ||
| ( | ||
| SimpleNamespace(status="error", exit_code=124), | ||
| ExecutionStatus.TIMEOUT, | ||
| ), | ||
| ( | ||
| SimpleNamespace(status="error", exit_code=1), | ||
| ExecutionStatus.ERROR, | ||
| ), | ||
| ( | ||
| SimpleNamespace(status="unexpected", exit_code=1), | ||
| ExecutionStatus.ERROR, | ||
| ), | ||
| ], | ||
| ) | ||
| def test_normalize_sandbox_execution_status(result, expected): | ||
| assert _normalize_sandbox_execution_status(result) == expected | ||
|
|
||
|
|
||
| def test_shell_validation_allows_benign_code(): | ||
| assert _shell_validation_error("printf 'hello\\n'") is None | ||
|
|
||
|
|
||
| def test_shell_validation_rejects_existing_dangerous_pattern(): | ||
| error = _shell_validation_error("rm -rf /") | ||
|
|
||
| assert error is not None | ||
| assert "代码安全检查失败" in error | ||
|
|
||
|
|
||
| def test_shell_interpreter_uses_runtime_factory_and_entry_validation(): | ||
| source_path = ( | ||
| Path(__file__).resolve().parents[1] | ||
| / "openapi" | ||
| / "api_v1" | ||
| / "agentic_data_api.py" | ||
| ) | ||
| tree = ast.parse(source_path.read_text(encoding="utf-8")) | ||
| shell_interpreter = next( | ||
| node | ||
| for node in ast.walk(tree) | ||
| if isinstance(node, ast.AsyncFunctionDef) | ||
| and node.name == "shell_interpreter" | ||
| ) | ||
|
|
||
| call_names = { | ||
| ast.unparse(node.func) | ||
| for node in ast.walk(shell_interpreter) | ||
| if isinstance(node, ast.Call) | ||
| } | ||
|
|
||
| assert "RuntimeFactory.create" in call_names | ||
| assert "LocalRuntime" not in call_names | ||
| assert "_shell_validation_error" in call_names | ||
|
Comment on lines
+54
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Test runtime policy through observable execution behavior. This AST-only check passes even if validation occurs after session creation or runtime-policy selection is bypassed at runtime. Add a focused test that invokes the tool with mocked factory/runtime/session objects, asserting unsafe shell input creates no runtime/session and an allowed command uses the factory-selected runtime. As per path instructions, “应断言可观察行为和对外相关契约” and “对于安全修复,在能够安全测试时,应包含一个具体的绕过方式或恶意输入场景。” Source: Path instructions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the real-world deployment, the runtime policy is defaultly set as local. So the fallback is right. No need to test it. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| from datetime import datetime | ||
| from typing import Any, Callable, Dict, List, Optional, Tuple, Type, cast, final | ||
|
|
||
| from jinja2 import Template | ||
| from jinja2.sandbox import SandboxedEnvironment | ||
|
|
||
| from dbgpt._private.pydantic import ConfigDict, Field | ||
| from dbgpt.core import LLMClient, ModelMessageRoleType, PromptTemplate | ||
|
|
@@ -1255,7 +1255,13 @@ def __missing__(self, key): | |
| if self.bind_prompt.template_format == "f-string": | ||
| system_prompt = self.bind_prompt.format(**prompt_param) | ||
| elif self.bind_prompt.template_format == "jinja2": | ||
| system_prompt = Template(self.bind_prompt.template).render(prompt_param) | ||
| # Render in a sandbox: bind_prompt.template may contain | ||
| # user-controlled content (e.g. a selected skill's instructions), | ||
| # so a plain jinja2.Template would allow SSTI -> RCE. | ||
| _env = SandboxedEnvironment() | ||
| system_prompt = _env.from_string(self.bind_prompt.template).render( | ||
| prompt_param | ||
| ) | ||
|
Comment on lines
+1312
to
+1315
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'packages/dbgpt-core/src/dbgpt/(agent/core/base_agent.py|agent/core/role.py|core/interface/prompt.py)$|requirements|pyproject|poetry.lock'
echo
echo "== relevant snippets =="
for f in packages/dbgpt-core/src/dbgpt/agent/core/base_agent.py packages/dbgpt-core/src/dbgpt/agent/core/role.py packages/dbgpt-core/src/dbgpt/core/interface/prompt.py; do
echo "--- $f"
sed -n '1,180p' "$f"
done
echo
echo "== imports/usages of template_env/SandboxedEnvironment/render in agent core =="
rg -n "SandboxedEnvironment|ImmutableSandboxedEnvironment|from_string|render\(|template_env|bind_prompt|class .*Prompt|class .*Role" packages/dbgpt-core/src/dbgpt/agent -S
rg -n "SandboxedEnvironment|ImmutableSandboxedEnvironment|from_string|render\(|template_env" packages/dbgpt-core/src/dbgpt/core/interface/prompt.py packages/dbgpt-core/src/dbgpt/core -SRepository: eosphoros-ai/DB-GPT Length of output: 25738 🌐 Web query:
💡 Result: The Jinja2 sandbox is designed to render untrusted templates by intercepting and restricting access to attributes, methods, and operations that could have security side effects [1][2]. SandboxedEnvironment The core class for sandboxing is jinja2.sandbox.SandboxedEnvironment [1][2]. It allows you to define what is "safe" through several mechanisms: Unsafe Callables: By default, the sandbox considers callables safe unless they are decorated with Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== agent profile base renderer =="
sed -n '200,360p' packages/dbgpt-core/src/dbgpt/agent/core/profile/base.py
echo
echo "== role rest of template usage =="
sed -n '180,270p' packages/dbgpt-core/src/dbgpt/agent/core/role.py
echo
echo "== core prompt render callsites =="
rg -n "_jinja2_formatter|template_format == \"jinja2\"|prompt_template_format" packages/dbgpt-core/src/dbgpt packages/dbgpt-app/src packages/dbgpt-serve/src packages/dbgpt-client/src -S || true
echo
echo "== pyproject jinja2 version pin =="
rg -n 'jinja2|SandboxedEnvironment' pyproject.toml packages/dbgpt-core/pyproject.toml requirements -S || trueRepository: eosphoros-ai/DB-GPT Length of output: 10141 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Behavioral source probe: demonstrate Jinja2 sandbox default callable acceptability
# without executing repository code or requiring jinja2/runtime artifacts in the repo.
python3 - <<'PY'
def print_fn(value):
pass
class SafeCallable:
def __init__(self, value):
self.value = value
class UnsafeCallable:
func = type("f", (), {"alters_data": True})
def __call__(self, value):
raise RuntimeError("unsafe")
print("default callable accept examples:")
for obj in [print_fn, SafeCallable(1), UnsafeCallable]:
safe = not getattr(obj, "unsafe_callable", False) and (
getattr(getattr(obj, "func", None), "alters_data", False) is not True
and getattr(getattr(obj, "func", None), "unsafe_callable", False) is not True
)
# This mirrors Jinja2.sandbox.is_safe_callable: callables are unsafe only
# when decorated `@unsafe` or func.alters_data/func.unsafe_callable is true.
print(type(obj).__name__, safe)
PYRepository: eosphoros-ai/DB-GPT Length of output: 234 Use one shared deny-by-default Jinja sandbox across prompt renderers. Default 📍 Affects 3 files
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The system_prompt is hardcoded. There's no necessity to worry about this. |
||
| else: | ||
| logger.warning("Bind prompt template not exsit or format not support!") | ||
| if not system_prompt: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,16 +23,18 @@ | |
|
|
||
|
|
||
| def _jinja2_formatter(template: str, **kwargs: Any) -> str: | ||
| """Format a template using jinja2.""" | ||
| """Format a template using jinja2 in a secure sandbox.""" | ||
| try: | ||
| from jinja2 import Template | ||
| from jinja2.sandbox import SandboxedEnvironment | ||
| except ImportError: | ||
| raise ImportError( | ||
| "jinja2 not installed, which is needed to use the jinja2_formatter. " | ||
| "Please install it with `pip install jinja2`." | ||
| ) | ||
|
|
||
| return Template(template).render(**kwargs) | ||
| env = SandboxedEnvironment() | ||
|
|
||
| return env.from_string(template).render(**kwargs) | ||
|
Comment on lines
+26
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'prompt\.py$|jinja|sandbox|tests?/.*/prompt|prompt.*test' | sed -n '1,120p'
echo "== prompt.py excerpt =="
cat -n packages/dbgpt-core/src/dbgpt/core/interface/prompt.py | sed -n '1,80p'
echo "== references to jinja2_formatter =="
rg -n "jinja2_formatter|jinja2|SandboxedEnvironment" packages/dbgpt-core/src packages/dbgpt-core/test packages/tests 2>/dev/null | sed -n '1,200p'Repository: eosphoros-ai/DB-GPT Length of output: 9769 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== all jinja2 references in core interface and tests =="
rg -n "jinja2|template_format=.+jinja2|_jinja2_formatter|format_jinja2|SandboxedEnvironment|assertRaises|forbidden_attribute|test_.*jinja|blocked|missing|empty|immutability|immutable" packages/dbgpt-core/src/dbgpt/core/interface packages/dbgpt-core -g '*.py' --glob '!**/__pycache__/**' | sed -n '1,260p'
echo "== prompt tests outline/size =="
wc -l packages/dbgpt-core/src/dbgpt/core/interface/tests/test_prompt.py
cat -n packages/dbgpt-core/src/dbgpt/core/interface/tests/test_prompt.py | sed -n '1,220p'
echo "== prompt public formatter usage/definition =="
cat -n packages/dbgpt-core/src/dbgpt/core/interface/prompt.py | sed -n '720,810p'Repository: eosphoros-ai/DB-GPT Length of output: 44117 Add regression coverage for the Jinja2 formatter contract. The current 🧰 Tools🪛 Ruff (0.16.0)[warning] 30-33: Within an (B904) Source: Path instructions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is introduced in other pr rather than this. |
||
|
|
||
|
|
||
| _DEFAULT_FORMATTER_MAPPING: Dict[str, Callable] = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -346,6 +346,7 @@ def _create_code_file(self, code: str) -> str: | |
| "go": ".go", | ||
| "rust": ".rs", | ||
| "python-vnc": ".py", | ||
| "bash": ".sh", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Enforce the configured execution timeout for Bash sessions. Bash now reaches As per path instructions, “限制 CPU、memory、process、execution time、output、file size 和 disk usage。” Source: Path instructions
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with that. But this is somehow too heavy for a runtime policy fix. |
||
| } | ||
| ext = extensions.get(self.config.language, ".txt") | ||
| timestamp = int(time.time() * 1000) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| from pathlib import Path | ||
|
|
||
| from dbgpt_sandbox.sandbox.config import ( | ||
| LANGUAGE_IMAGES, | ||
| get_command_by_language, | ||
| ) | ||
| from dbgpt_sandbox.sandbox.execution_layer.base import SessionConfig | ||
| from dbgpt_sandbox.sandbox.execution_layer.docker_runtime import ( | ||
| DockerSandboxSession, | ||
| ) | ||
|
|
||
|
|
||
| def test_bash_reuses_python_slim_image(): | ||
| assert LANGUAGE_IMAGES["bash"] == LANGUAGE_IMAGES["python"] | ||
|
|
||
|
|
||
| def test_bash_executes_with_posix_shell(): | ||
| assert get_command_by_language("bash", "run.sh") == "sh run.sh" | ||
|
|
||
|
|
||
| def test_docker_bash_code_uses_shell_extension(tmp_path, monkeypatch): | ||
| monkeypatch.setattr( | ||
| "dbgpt_sandbox.sandbox.execution_layer.docker_runtime.tempfile.gettempdir", | ||
| lambda: str(tmp_path), | ||
| ) | ||
| session = DockerSandboxSession( | ||
| "bash-test", | ||
| SessionConfig(language="bash"), | ||
| docker_client=None, | ||
| ) | ||
|
|
||
| code_file = Path(session._create_code_file("printf 'hello\\n'")) | ||
|
|
||
| assert code_file.suffix == ".sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prioritize exit code 124 before enum status normalization.
An enum-shaped result with
status=ExecutionStatus.ERRORandexit_code=124returnsERRORat Line 993, so the timeout-specific response path is skipped.packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L992-L995: checkexit_code == 124before returning an existing enum status.packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py#L14-L40: addExecutionStatus.ERRORplus exit code124and expectExecutionStatus.TIMEOUT.📍 Affects 2 files
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L992-L995(this comment)packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py#L14-L40There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the difference between local runtime and docker runtime. The docker (also other sandboxed runtime) returns with an object instead of the enum. To ensure the minimal fix, I pass the result directly if it runs in the local runtime, which is the first branch of this.