Safe eval#8936
Open
ericspod wants to merge 10 commits into
Open
Conversation
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
for more information, see https://pre-commit.ci
![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: 3
🤖 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 `@monai/bundle/scripts.py`:
- Line 165: Add direct unit tests for the `_get_fake_spatial_shape` function in
an appropriate test file. Create test cases that validate both successful
handling of valid arithmetic expressions (such as "32 * n" and "2**p") and
rejection of unsafe expressions (such as "foo()" and "x.bar"). These tests
should directly test the `_get_fake_spatial_shape` function itself rather than
relying solely on integration tests that call it indirectly through
`verify_net_in_out`, ensuring the function meets the direct test coverage
requirement for modified definitions.
In `@monai/utils/safe_eval.py`:
- Around line 40-41: Rename the parameters `globals` and `locals` in the
function signature to avoid shadowing Python builtins (for example,
`globals_dict` and `locals_dict`), and update any usage of these parameters
throughout the function to use the new names. Additionally, fix the docstring
inconsistency by changing the documented parameter name from `allows_types` to
`allowed_types` to match the actual parameter name in the function signature.
In `@tests/utils/test_safe_eval.py`:
- Around line 33-36: The test method test_good_exprs uses parameter names
globals and locals which shadow Python builtins, violating PEP8 guidelines and
triggering Ruff A002. Rename these parameters to non-shadowing alternatives such
as globals_dict and locals_dict in both the method signature and the safe_eval
function call to improve code clarity and comply with style guidelines.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3126c742-d09d-4287-a158-a500535cffb8
📒 Files selected for processing (5)
monai/bundle/scripts.pymonai/utils/__init__.pymonai/utils/ordering.pymonai/utils/safe_eval.pytests/utils/test_safe_eval.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
monai/utils/safe_eval.py (1)
53-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring parameter names still reference old
globals/locals.Lines 55-56 document
globals:andlocals:but the parameters are nowglobals_varsandlocals_vars. Line 63 also uses the old names.Proposed fix
Args: expr: expression to evaluate, this will be stripped before parsing to avoid indentation complaints - globals: global variable mapping - locals: local variable mapping + globals_vars: global variable mapping + locals_vars: local variable mapping allowed_types: sequence of allowed AST types which can be found in `expr` when parsed Raises: ValueError: raised when any node in the AST parsed from `expr` has a type not in `allowed_types` Returns: - The evaluated expression value, using `eval` with `globals` and `locals` + The evaluated expression value, using `eval` with `globals_vars` and `locals_vars`🤖 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 `@monai/utils/safe_eval.py` around lines 53 - 64, The docstring in the function documents parameter names `globals` and `locals` on lines 55-56, but the actual function parameters have been renamed to `globals_vars` and `locals_vars`. Update the docstring parameter documentation to use the new parameter names `globals_vars:` and `locals_vars:` instead of `globals:` and `locals:`, and also update any other references to these parameter names within the docstring (including line 63) to use the corrected names.
🤖 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.
Duplicate comments:
In `@monai/utils/safe_eval.py`:
- Around line 53-64: The docstring in the function documents parameter names
`globals` and `locals` on lines 55-56, but the actual function parameters have
been renamed to `globals_vars` and `locals_vars`. Update the docstring parameter
documentation to use the new parameter names `globals_vars:` and `locals_vars:`
instead of `globals:` and `locals:`, and also update any other references to
these parameter names within the docstring (including line 63) to use the
corrected names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25cc6d98-e150-4bc3-839e-0b3e5813d590
📒 Files selected for processing (4)
monai/bundle/scripts.pymonai/utils/ordering.pymonai/utils/safe_eval.pytests/utils/test_safe_eval.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/utils/test_safe_eval.py
- monai/utils/ordering.py
- monai/bundle/scripts.py
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/utils/safeeval.py (1)
21-21: 💤 Low valueIncorrect type annotation for
SAFE_TYPES.The annotation
Sequence[ast.AST]describes a sequence of AST node instances, but the value is a tuple of AST node types (classes). Should beSequence[type[ast.AST]].Suggested fix
-SAFE_TYPES: Sequence[ast.AST] = ( +SAFE_TYPES: Sequence[type[ast.AST]] = (🤖 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 `@monai/utils/safeeval.py` at line 21, The type annotation for SAFE_TYPES is incorrect because it declares Sequence[ast.AST] which represents a sequence of AST node instances, but the actual value is a tuple of AST node classes/types. Change the type annotation for SAFE_TYPES from Sequence[ast.AST] to Sequence[type[ast.AST]] to correctly represent that it contains AST node types rather than instances.Source: Coding guidelines
🤖 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 `@monai/utils/safeeval.py`:
- Line 73: The eval function call on this line converts globals_vars to a
dictionary using dict(globals_vars), but when globals_vars has its default value
of None, this raises a TypeError because None is not iterable. Fix this by
either checking if globals_vars is None before converting it to a dict and
providing an empty dict as fallback, or change the default parameter value of
globals_vars from None to an empty dictionary instead.
---
Nitpick comments:
In `@monai/utils/safeeval.py`:
- Line 21: The type annotation for SAFE_TYPES is incorrect because it declares
Sequence[ast.AST] which represents a sequence of AST node instances, but the
actual value is a tuple of AST node classes/types. Change the type annotation
for SAFE_TYPES from Sequence[ast.AST] to Sequence[type[ast.AST]] to correctly
represent that it contains AST node types rather than instances.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 523656fc-0e5b-4fe5-9f7e-7cade595b509
📒 Files selected for processing (3)
monai/utils/__init__.pymonai/utils/safeeval.pytests/utils/test_alias.py
✅ Files skipped from review due to trivial changes (1)
- tests/utils/test_alias.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/utils/init.py
Signed-off-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@monai/utils/safeeval.py`:
- Line 73: The truthiness check on globals_vars in the eval call incorrectly
treats an empty dictionary as falsy, converting it to None which exposes
builtins instead of blocking them. Replace the condition `if globals_vars` with
an identity check `if globals_vars is not None` in the return statement to
ensure that empty dictionaries are preserved and passed to eval, while still
allowing None values to pass through unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9397626c-6b78-4aea-98f0-b1bdd255fa45
📒 Files selected for processing (1)
monai/utils/safeeval.py
Signed-off-by: Eric Kerfoot <17726042+ericspod@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
1 participant
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.
Addresses GHSA-h89g-r5pc-wxfm.
Description
This introduces a
safe_evalfunction to evaluate known safe expressions which do not contain member access, calls, indexing, or other expressions which could be used for code injection. Use ofevalis replaced where appropriate.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.