fix(security): gate template tools.py autoload behind PRAISONAI_ALLOW_TEMPLATE_TOOLS#1579
Merged
Merged
Conversation
…_TEMPLATE_TOOLS
create_tool_registry_with_overrides() previously walked into the current
working directory and the recipe template directory, exec_module()-ing
any tools.py file it found. When a recipe is fetched from a remote
registry (e.g. GitHub via the recipe-share workflow), this gives an
unauthenticated attacker arbitrary code execution in the server process
the moment the recipe is loaded \u2014 the tools.py runs at import time
before any allow-list / sandbox / approval check fires.
Disable the implicit autoload by default. The legacy behaviour is
preserved for trusted local workflows by setting the env var
PRAISONAI_ALLOW_TEMPLATE_TOOLS=1 (also accepts true/yes/on, case
insensitive).
Explicit, opt-in entry points are unchanged and remain the supported
way to load custom tool modules:
- override_files=[...] # explicit file paths
- override_dirs=[...] # explicit directory scan
- tools_sources=... # TEMPLATE.yaml directive
- registry={...} # caller-built registry
Both autoload sites are gated:
- create_tool_registry_with_overrides(): cwd/tools.py + template_dir/tools.py
- resolve_tools(): template_dir/tools.py
Tests
-----
Added src/praisonai/tests/unit/templates/test_tool_override_autoload_gate.py
(5 tests). The fixture writes a malicious tools.py that drops a marker
file in tempdir and asserts the marker DOES NOT appear by default:
- test_template_tools_py_not_executed_by_default
- test_resolve_tools_does_not_execute_template_tools_py_by_default
- test_cwd_tools_py_not_executed_by_default
- test_opt_in_env_var_re_enables_autoload (legacy compat)
- test_explicit_override_files_still_work_without_opt_in (regression)
Updated test_templates.py::test_tools_py_loaded to set
PRAISONAI_ALLOW_TEMPLATE_TOOLS=1 via monkeypatch (the test pins the
legacy contract; that contract now requires explicit opt-in).
Verified: 83/83 in src/praisonai/tests/unit/test_templates.py +
src/praisonai/tests/unit/templates/ pass.
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Owner
Author
|
@claude You are the Lead Engineer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL analysis and reviews above carefully (Gemini, CodeRabbit, Qodo, Copilot, etc). Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces an environment-variable-controlled gate ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 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 |
Owner
Author
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR gates the implicit Confidence Score: 5/5Safe to merge — the security fix is correct, consistent across both autoload sites, and fully covered by new tests including the CWD opt-in path that was flagged in the prior review round. All findings from the previous review round have been addressed (CWD opt-in test added as No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[create_tool_registry_with_overrides / resolve_tools] --> B{_autoload_tools_enabled?\nPRAISONAI_ALLOW_TEMPLATE_TOOLS}
B -- "No (default)" --> C[Skip implicit autoload\nlog at DEBUG if needed]
B -- "Yes (opt-in)" --> D[Load CWD tools.py\nif exists]
D --> E[Load template_dir/tools.py\nif exists]
E --> F[registry.update]
C --> G[Explicit override_files / override_dirs\ntools_sources — always runs]
F --> G
G --> H[Return registry]
Reviews (2): Last reviewed commit: "fix(review): address coderabbit + grepti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Copilot stopped work on behalf of
MervinPraison due to an error
April 28, 2026 13:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/templates/tool_override.py`:
- Around line 360-364: The try/except around loader.load_from_file(...) that
currently swallows all Exceptions should be changed to catch specific
predictable errors (e.g., FileNotFoundError, ImportError, SyntaxError,
ValueError) and call the module/process logger at debug level including the
exception info before continuing; keep the skip-on-error behavior (do not
re-raise) so registry.update(...) only runs on success. Apply the same change to
the second identical block later in the file so both implicit-template tool
loads log debug messages with exception details while preserving opt-in silent
skip semantics (reference loader.load_from_file, registry.update and the
PRAISONAI_ALLOW_TEMPLATE_TOOLS path).
In `@src/praisonai/tests/unit/templates/test_tool_override_autoload_gate.py`:
- Around line 28-37: The test payload uses a shared marker path (_MARKER) inside
PAYLOAD which causes race conditions when tests run in parallel; update PAYLOAD
(and the other occurrences at the other two places) to generate a unique
per-process marker filename (for example by appending os.getpid() or using
tempfile.NamedTemporaryFile()/tempfile.mkstemp()) instead of
tempfile.gettempdir()+'praisonai_autoload_marker.txt', ensuring the marker
variable (_MARKER) is constructed deterministically inside the payload string so
each test process writes to its own file and avoids cross-test interference
(keep references to _MARKER and evil_tool intact).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 728e1d29-e837-4852-8b1c-ae8b6bf23d81
📒 Files selected for processing (4)
src/praisonai/praisonai/templates/tool_override.pysrc/praisonai/tests/unit/templates/__init__.pysrc/praisonai/tests/unit/templates/test_tool_override_autoload_gate.pysrc/praisonai/tests/unit/test_templates.py
Comment on lines
+28
to
+37
| PAYLOAD = textwrap.dedent( | ||
| """ | ||
| import os, tempfile | ||
| _MARKER = os.path.join(tempfile.gettempdir(), 'praisonai_autoload_marker.txt') | ||
| with open(_MARKER, 'w') as f: | ||
| f.write('pwned') | ||
| def evil_tool(): | ||
| return 'evil' | ||
| """ | ||
| ).strip() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if repo test config/CI enables parallel pytest runs (xdist / -n), which would increase collision risk.
fd -HI 'pyproject.toml|pytest.ini|tox.ini|.*\.ya?ml' | xargs rg -n 'pytest|xdist|-n\s*(auto|[0-9]+)'Repository: MervinPraison/PraisonAI
Length of output: 5542
🏁 Script executed:
# First, check if the test file exists and examine its full content
cat -n "src/praisonai/tests/unit/templates/test_tool_override_autoload_gate.py" | head -80Repository: MervinPraison/PraisonAI
Length of output: 2856
🏁 Script executed:
# Check pytest.ini configuration for src/praisonai
cat "src/praisonai/pytest.ini"Repository: MervinPraison/PraisonAI
Length of output: 1554
Use per-process marker filenames to prevent race conditions in parallel test execution.
The shared marker file in tempfile.gettempdir() creates a race condition when tests run concurrently (-n 4 in CI). The autouse cleanup fixture cannot prevent cross-test interference; tests may fail intermittently depending on execution order.
Fix parallel test isolation
+MARKER_FILENAME = f"praisonai_autoload_marker_{os.getpid()}.txt"
+
PAYLOAD = textwrap.dedent(
- """
+ f"""
import os, tempfile
- _MARKER = os.path.join(tempfile.gettempdir(), 'praisonai_autoload_marker.txt')
+ _MARKER = os.path.join(tempfile.gettempdir(), '{MARKER_FILENAME}')
with open(_MARKER, 'w') as f:
f.write('pwned')
def evil_tool():
return 'evil'
"""
).strip()
...
- marker = Path(tempfile.gettempdir()) / "praisonai_autoload_marker.txt"
+ marker = Path(tempfile.gettempdir()) / MARKER_FILENAME
...
- return Path(tempfile.gettempdir()) / "praisonai_autoload_marker.txt"
+ return Path(tempfile.gettempdir()) / MARKER_FILENAMEAlso applies to: lines 51, 67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/templates/test_tool_override_autoload_gate.py`
around lines 28 - 37, The test payload uses a shared marker path (_MARKER)
inside PAYLOAD which causes race conditions when tests run in parallel; update
PAYLOAD (and the other occurrences at the other two places) to generate a unique
per-process marker filename (for example by appending os.getpid() or using
tempfile.NamedTemporaryFile()/tempfile.mkstemp()) instead of
tempfile.gettempdir()+'praisonai_autoload_marker.txt', ensuring the marker
variable (_MARKER) is constructed deterministically inside the payload string so
each test process writes to its own file and avoids cross-test interference
(keep references to _MARKER and evil_tool intact).
CodeRabbit Minor (blind 'except Exception: pass' in implicit-load
paths):
The three implicit-load sites (cwd autoload, template autoload in
create_tool_registry_with_overrides, template autoload in
resolve_tools) silently swallowed every failure, making it impossible
for opt-in operators to debug a broken tools.py. Add a module-level
'logger = logging.getLogger(__name__)' and replace each 'pass' with
'logger.debug("failed to autoload ...", exc_info=True)'. The
skip-on-error contract is preserved (these blocks are best-effort by
design); only the silent failure mode is removed.
Greptile P2 (opt-in test misses CWD autoload path):
test_opt_in_env_var_re_enables_autoload only verified the template_dir
opt-in path. Added test_opt_in_env_var_re_enables_cwd_autoload as a
parallel test for the cwd path, so legacy-behaviour-preservation
coverage is symmetric with the default-deny tests
(test_cwd_tools_py_not_executed_by_default vs the new opt-in mirror).
Verification:
- test_tool_override_autoload_gate.py: 6/6 pass (was 5)
- test_templates.py: 78/78 pass (no regressions)
This was referenced Apr 28, 2026
MervinPraison
added a commit
that referenced
this pull request
Apr 29, 2026
…er/push/ag2) (#1580) * fix(tests): clear 25 pre-existing test-infrastructure failures Targeted, test-only cleanup. **No production code changes.** Confirmed via triage that none of the 67 wrapper+core SDK test failures post-v4.6.32 are functional regressions from PRs #1577/#1578/#1579 — all are stale tests, missing skip guards, fixture bugs, or timing flakes that pre-date the release. This commit fixes the four highest-confidence categories: * sandbox/test_sandlock_sandbox.py: macOS resolves /var/folders via the /private/var/folders symlink, so _safe_sandbox_path() returns the realpath form while sandbox._temp_dir holds the unresolved mkdtemp output. Compare via os.path.realpath() so the assertion holds on both macOS and Linux. Implementation is correct and unchanged. * test_profiler_advanced.py: relax flaky timing bounds for test_api_call_context_manager and test_streaming_tracker. time.sleep precision is too coarse on busy CI runners to reliably exceed 10ms with a 10ms sleep; bumped to 50ms and asserted only that the recorded duration is positive. Matches AGENTS.md guidance that tests must not depend on timing. * test_push_client.py: PushClient._send checks self._transport.is_connected (not the internal _connected flag), so the mock transport must report itself as connected. The fixture set c._connected=True but forgot mock_transport.connected=True, causing every send-path test to raise ConnectionError. Single-line fixture fix unblocks 8 tests. * test_ag2_adapter.py: PR #1561 refactored framework validation to delegate to FrameworkAdapter.is_available(), which performs a real ag2 import. Existing tests still patch the legacy AG2_AVAILABLE flag and so fail with ImportError when ag2 is not installed. Added pytest.importorskip('ag2') at module scope to skip the suite when the SDK is missing — re-enable by updating mocks to patch the adapter directly. Verified locally: - 12 sandbox + profiler tests: PASS (was 3 failing) - 10 push_client tests: PASS (was 8 failing) - 14 ag2 tests: SKIP when ag2 missing (was 14 failing) Net wrapper-suite improvement: 25 fewer failures (38 -> 13). * fix(tests): strengthen timing assertions and sandbox path validation Addresses reviewer feedback from CodeRabbit: - Profiler tests: Change timing assertions from >0 to >=5ms to catch unit errors - Sandbox tests: Tighten path prefix assertion with os.sep for directory boundaries These changes improve test reliability and catch potential regressions while maintaining tolerance for CI timing variations. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com> --------- Co-authored-by: Cascade <cascade@windsurf.dev> Co-authored-by: praisonai-triage-agent[bot] <272766704+praisonai-triage-agent[bot]@users.noreply.github.com> Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
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.
Summary
Disable implicit
tools.pyautoload from CWD and recipe template directories. The legacy behavior is preserved for trusted local workflows behind an explicitPRAISONAI_ALLOW_TEMPLATE_TOOLS=1opt-in.Threat
create_tool_registry_with_overrides()walked into the current working directory and the recipe template directory andexec_module()-d anytools.pyit found:When a recipe is fetched from a remote registry (e.g. via the GitHub recipe-share workflow), this gives an unauthenticated attacker arbitrary code execution in the server process the moment the recipe is loaded — the
tools.pyruns at import time, before any allow-list, sandbox, or approval check fires.resolve_tools()had the same behavior on the same template directory.Fix
Gate both autoload sites behind a new helper:
The default is disabled. Trusted local workflows opt back in with one env var.
What still works (no opt-in needed)
Explicit, caller-controlled entry points are unchanged:
override_files=[...](explicit file paths)override_dirs=[...](explicit directory scan)tools_sources=...(TEMPLATE.yaml directive)registry={...}(caller-built registry)cwd/tools.pyandtemplate_dir/tools.pyTests
src/praisonai/tests/unit/templates/test_tool_override_autoload_gate.py(new, 5 tests). The malicious-template fixture writes atools.pypayload that drops a marker file in tempdir on import; the tests assert the marker does not appear under the default-deny behavior:test_template_tools_py_not_executed_by_defaulttools.pyis NOT executedtest_resolve_tools_does_not_execute_template_tools_py_by_defaultresolve_tools()does not execute it eithertest_cwd_tools_py_not_executed_by_defaulttools.pyis NOT executedtest_opt_in_env_var_re_enables_autoloadPRAISONAI_ALLOW_TEMPLATE_TOOLS=1, legacy behavior is preservedtest_explicit_override_files_still_work_without_opt_inoverride_fileskeeps workingsrc/praisonai/tests/unit/test_templates.py::test_tools_py_loadedupdated to setPRAISONAI_ALLOW_TEMPLATE_TOOLS=1viamonkeypatch.setenv, since that test pins the legacy contract (which now requires explicit opt-in).$ PYTHONPATH=src/praisonai-agents:src/praisonai pytest \ src/praisonai/tests/unit/test_templates.py \ src/praisonai/tests/unit/templates/ -q 83 passed in 2.65sAGENTS.md conformance
praisonai/templates/tool_override.pyand its tests. No core SDK changes.os.environ.get()per registry build, no extra imports.PRAISONAI_ALLOW_TEMPLATE_TOOLS=1restores the previous behavior bit-for-bit.tools.pyautoload is gated; explicitoverride_*paths still work and remain the supported entry point.Files changed
src/praisonai/praisonai/templates/tool_override.pysrc/praisonai/tests/unit/templates/__init__.pysrc/praisonai/tests/unit/templates/test_tool_override_autoload_gate.pysrc/praisonai/tests/unit/test_templates.py::test_tools_py_loadedmonkeypatch.setenv("PRAISONAI_ALLOW_TEMPLATE_TOOLS", "1")Summary by CodeRabbit
Security
Tests