fix: replace unsafe eval() with safe AST parser in README example#1626
Conversation
#1625) - Replace eval(expression) with AST-based arithmetic parser in custom tools example - Add comprehensive security warning about eval(), exec(), subprocess usage - Prevent arbitrary code execution while preserving arithmetic functionality - Blocks malicious inputs like __import__('os').system('whoami') Co-authored-by: praisonai-triage-agent[bot] <praisonai-triage-agent[bot]@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe README's "Custom Tools" example is updated to eliminate a critical security vulnerability. The ChangesREADME Security Documentation Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@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 replaces the unsafe
Confidence Score: 4/5Safe to merge after addressing the integer exponentiation resource exhaustion issue in the The AST-based rewrite correctly blocks code injection and README.md — specifically the Important Files Changed
Reviews (2): Last reviewed commit: "docs: normalize README calculator error ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.gh.lumenfield.work/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@README.md`:
- Around line 334-335: Remove the empty quoted line inside the blockquote that
contains the security note; locate the blockquote containing "⚠️ **Security
Note:** Never use `eval()`, `exec()`, or `subprocess`..." and ensure the
blockquote has no blank lines (keep the warning text immediately after the "> "
marker) so the MD028 `no-blanks-blockquote` lint rule is satisfied.
- Around line 322-325: The try/except around ast.parse(...).body -> _safe_eval
currently catches (ValueError, SyntaxError, TypeError) but lets
ZeroDivisionError (e.g., from evaluating "1/0") escape; update the except clause
in the function containing the call to _safe_eval (the block that returns
_safe_eval(ast.parse(expression, mode="eval").body)) to also catch
ZeroDivisionError so that any division-by-zero errors are converted to the same
ValueError("Invalid arithmetic expression") path for consistent behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| try: | ||
| return _safe_eval(ast.parse(expression, mode="eval").body) | ||
| except (ValueError, SyntaxError, TypeError): | ||
| raise ValueError("Invalid arithmetic expression") |
There was a problem hiding this comment.
Handle division-by-zero in the normalized error path.
1/0 currently raises ZeroDivisionError and bypasses your ValueError("Invalid arithmetic expression") contract. Add ZeroDivisionError to the caught exceptions for consistent behavior.
Suggested fix
- except (ValueError, SyntaxError, TypeError):
+ except (ValueError, SyntaxError, TypeError, ZeroDivisionError):
raise ValueError("Invalid arithmetic expression")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| return _safe_eval(ast.parse(expression, mode="eval").body) | |
| except (ValueError, SyntaxError, TypeError): | |
| raise ValueError("Invalid arithmetic expression") | |
| try: | |
| return _safe_eval(ast.parse(expression, mode="eval").body) | |
| except (ValueError, SyntaxError, TypeError, ZeroDivisionError): | |
| raise ValueError("Invalid arithmetic expression") |
🤖 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 `@README.md` around lines 322 - 325, The try/except around ast.parse(...).body
-> _safe_eval currently catches (ValueError, SyntaxError, TypeError) but lets
ZeroDivisionError (e.g., from evaluating "1/0") escape; update the except clause
in the function containing the call to _safe_eval (the block that returns
_safe_eval(ast.parse(expression, mode="eval").body)) to also catch
ZeroDivisionError so that any division-by-zero errors are converted to the same
ValueError("Invalid arithmetic expression") path for consistent behavior.
| > ⚠️ **Security Note:** Never use `eval()`, `exec()`, or `subprocess` in tool functions that process LLM-generated or user-supplied input. Always validate and sanitize inputs to prevent code injection attacks. | ||
|
|
There was a problem hiding this comment.
Resolve markdownlint MD028 (no-blanks-blockquote).
There is a blank line inside the blockquote around the security note (Line 335), which triggers lint warning MD028. Remove the blank quoted line.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 335-335: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@README.md` around lines 334 - 335, Remove the empty quoted line inside the
blockquote that contains the security note; locate the blockquote containing "⚠️
**Security Note:** Never use `eval()`, `exec()`, or `subprocess`..." and ensure
the blockquote has no blank lines (keep the warning text immediately after the
"> " marker) so the MD028 `no-blanks-blockquote` lint rule is satisfied.
…tting Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/f86d8d08-03ae-4f02-8c4b-21dc53e084dc Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
Reviewed all available reviewer input and validated against HEAD d7f1fb5. Findings:
Reviewer feedback incorporated:
|
|
@claude You are the FINAL architecture reviewer. 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 comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
📋 Final Architecture Review Summary✅ APPROVED - All reviewer issues have been addressed and the PR is ready to merge. 🔍 Review Findings
📁 Files Modified
|
| elif isinstance(node, ast.BinOp) and type(node.op) in _OPS: | ||
| return _OPS[type(node.op)](_safe_eval(node.left), _safe_eval(node.right)) |
There was a problem hiding this comment.
operator.pow on two Python int nodes produces an arbitrary-precision integer, not a float. A request like 2**9999999 will never trigger OverflowError — Python will happily allocate gigabytes of memory computing the result. An LLM (or a test call) that emits a large exponent can therefore exhaust the process's memory/CPU with no error. Coercing operands to float before the power operation keeps finite-precision semantics and surfaces OverflowError for values that are too large.
| elif isinstance(node, ast.BinOp) and type(node.op) in _OPS: | |
| return _OPS[type(node.op)](_safe_eval(node.left), _safe_eval(node.right)) | |
| elif isinstance(node, ast.BinOp) and type(node.op) in _OPS: | |
| left, right = _safe_eval(node.left), _safe_eval(node.right) | |
| if isinstance(node.op, ast.Pow): | |
| left, right = float(left), float(right) | |
| return _OPS[type(node.op)](left, right) |
✅ LGTMThe Verified:
One minor note (non-blocking): Clean fix, ship it. |
|
@cladue that is too many lines getting added to readme, keep it minimal very minimal or alternate or remove that |
Fixes #1625
Summary
Changes Made
Security Impact
Test Results
✅ All legitimate arithmetic expressions work correctly
✅ All code injection attempts are blocked
✅ Clear error messages for invalid expressions
Generated with Claude Code
Summary by CodeRabbit
eval(),exec(), orsubprocesswhen processing external input.