fix(eval): Use safe dumper and yaml load - #2082
Merged
Merged
Conversation
github-actions
Bot
added
status: needs triage
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
tgasser-nv
added
status: triaged
tgasser-nv
marked this pull request as ready for review
June 26, 2026 16:49
Contributor
Greptile SummaryThis PR fixes a YAML deserialization and serialization security issue in the
|
| Filename | Overview |
|---|---|
| nemoguardrails/eval/utils.py | Replaced unsafe yaml.load/CDumper with yaml.safe_load/CSafeDumper (falling back to SafeDumper); the unused CLoader/Loader import is also removed. The change is minimal and correct. |
| tests/eval/test_utils_safe_yaml.py | New test file covering malicious-tag rejection (load and directory walk), benign YAML/JSON loading, safe-dumper fallback, serialization refusal for arbitrary objects, and a full round-trip — thorough coverage of the security fix. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Caller] --> B{load_dict_from_file}
B -->|.yaml / .yml| C[yaml.safe_load]
B -->|.json| D[json.load]
C -->|contains !!python/... tag| E[ConstructorError raised]
C -->|benign YAML| F[Return dict]
D --> F
G[Caller] --> H{save_dict_to_file}
H -->|.yaml / .yml| I{CSafeDumper available?}
H -->|.json| J[json.dumps]
I -->|Yes - libyaml present| K[yaml.dump with CSafeDumper]
I -->|No - pure Python| L[yaml.dump with SafeDumper]
K -->|contains arbitrary Python object| M[RepresenterError raised]
K -->|primitive data| N[Write YAML file]
L -->|primitive data| N
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Caller] --> B{load_dict_from_file}
B -->|.yaml / .yml| C[yaml.safe_load]
B -->|.json| D[json.load]
C -->|contains !!python/... tag| E[ConstructorError raised]
C -->|benign YAML| F[Return dict]
D --> F
G[Caller] --> H{save_dict_to_file}
H -->|.yaml / .yml| I{CSafeDumper available?}
H -->|.json| J[json.dumps]
I -->|Yes - libyaml present| K[yaml.dump with CSafeDumper]
I -->|No - pure Python| L[yaml.dump with SafeDumper]
K -->|contains arbitrary Python object| M[RepresenterError raised]
K -->|primitive data| N[Write YAML file]
L -->|primitive data| N
Reviews (3): Last reviewed commit: "Add escaping to YAML" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughYAML loading now uses ChangesSafe YAML utilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 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 `@tests/eval/test_utils_safe_yaml.py`:
- Line 42: The YAML fixtures in the safe YAML tests interpolate a raw filesystem
path into a double-quoted YAML string, which breaks parsing on Windows because
backslashes are treated as escapes. Update the path handling in the affected
test cases in test_utils_safe_yaml.py by escaping the path before embedding it
in the YAML content, so the tests reach the expected ConstructorError instead of
failing during YAML parsing.
🪄 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: Enterprise
Run ID: 1afddb2e-d0e1-4d7d-b726-66e563a0d92f
📒 Files selected for processing (2)
nemoguardrails/eval/utils.pytests/eval/test_utils_safe_yaml.py
cparisien
approved these changes
Jun 26, 2026
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
2 participants
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.
Description
The nemoguardrails
evaltool deserializes and serializes data using theload_dict_from_file()andsave_dict_to_file()functions respectively. Prior to this PR, unsafe libraries were used to save and load files. This PR changes them to use the safe equivalent (which can't deserialize and execute arbitrary python code), and adds unit-tests to make sure this isn't possible.Changes made are:
yaml.load()->yaml.safe_load()CDumper/Dumper->CSafeDumper/SafeDumper.No functional changes were made.
Related Issue(s)
Verification
Pre-commit
Unit-test
AI Assistance
Checklist
Summary by CodeRabbit