Fix GHSA-873f-pvrv-4x83: warn before executing a bundle's config in load()/run() - #9057
Fix GHSA-873f-pvrv-4x83: warn before executing a bundle's config in load()/run()#9057garciadias wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe bundle APIs document that configuration parsing can resolve importable callables and evaluate Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/bundle/test_bundle_download.py (1)
509-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd docstrings for the new methods.
Document
_stage_malicious_bundlewith its arguments and return value. Document each new test method with its asserted behavior.As per path instructions, “Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.”
🤖 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 `@tests/bundle/test_bundle_download.py` around lines 509 - 550, Add Google-style docstrings to _stage_malicious_bundle describing tempdir, marker, and the returned bundle name, and to test_default_warns_and_executes_config, test_explicit_model_skips_config_parsing, and test_run_warns_on_config_execution describing each test’s asserted behavior. Include raised-exception details where applicable.Source: Path instructions
🤖 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/bundle/test_bundle_download.py`:
- Around line 532-538: Update test_explicit_model_skips_config_parsing to import
warnings and wrap the load() call in warnings.catch_warnings() with UserWarning
configured as an error, ensuring the explicit-model path remains warning-free
while preserving the existing marker assertion.
---
Nitpick comments:
In `@tests/bundle/test_bundle_download.py`:
- Around line 509-550: Add Google-style docstrings to _stage_malicious_bundle
describing tempdir, marker, and the returned bundle name, and to
test_default_warns_and_executes_config,
test_explicit_model_skips_config_parsing, and test_run_warns_on_config_execution
describing each test’s asserted behavior. Include raised-exception details where
applicable.
🪄 Autofix
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 Plus
Run ID: c43268e0-5488-4668-b5b2-04b26161a216
📒 Files selected for processing (2)
monai/bundle/scripts.pytests/bundle/test_bundle_download.py
4b1138e to
95c1dce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/bundle/test_bundle_download.py (2)
509-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style docstrings to the new definitions.
_stage_malicious_bundle()and the three test methods have no method docstrings. Documenttempdir,marker, the returned bundle name, and each test's behavior.As per path instructions, definitions in
**/*.pyshould have Google-style docstrings that describe variables, return values, and raised exceptions.Also applies to: 520-530, 532-538, 540-551
🤖 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 `@tests/bundle/test_bundle_download.py` around lines 509 - 518, Add Google-style docstrings to _stage_malicious_bundle and the three newly added test methods. Document tempdir and marker in Args, state that the helper returns the staged bundle name, and describe each test’s behavior; include Returns or Raises sections where applicable without changing test logic.Source: Path instructions
524-529: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the warning message, not only its type.
assertWarns(UserWarning)can pass for an unrelated warning. Capture the warning and assert stable text for executable configuration andGHSA-873f-pvrv-4x83in both paths.Proposed fix
- with self.assertWarns(UserWarning): + with self.assertWarns(UserWarning) as warning: with self.assertRaises(AttributeError): load(...) + self.assertIn("GHSA-873f-pvrv-4x83", str(warning.warning))Also applies to: 546-550
🤖 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 `@tests/bundle/test_bundle_download.py` around lines 524 - 529, Update both warning assertions around the malicious bundle load calls to capture the emitted warning object, rather than checking only UserWarning. Assert that each warning message contains stable references to executable configuration and GHSA-873f-pvrv-4x83, while preserving the existing exception assertions and load calls.
🤖 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/bundle/test_bundle_download.py`:
- Around line 515-517: Update the malicious payload construction in the bundle
download tests at both occurrences to avoid shell parsing: embed the marker path
using a representation-safe escaping method and invoke a direct Python
file-write operation instead of os.system. Preserve the test’s expected marker
content while ensuring paths with spaces, shell characters, or Windows
backslashes remain valid.
---
Nitpick comments:
In `@tests/bundle/test_bundle_download.py`:
- Around line 509-518: Add Google-style docstrings to _stage_malicious_bundle
and the three newly added test methods. Document tempdir and marker in Args,
state that the helper returns the staged bundle name, and describe each test’s
behavior; include Returns or Raises sections where applicable without changing
test logic.
- Around line 524-529: Update both warning assertions around the malicious
bundle load calls to capture the emitted warning object, rather than checking
only UserWarning. Assert that each warning message contains stable references to
executable configuration and GHSA-873f-pvrv-4x83, while preserving the existing
exception assertions and load calls.
🪄 Autofix
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 Plus
Run ID: d296833d-0f0d-4e6e-a75b-4bd86a405b76
📒 Files selected for processing (1)
tests/bundle/test_bundle_download.py
| malicious_config = {"network_def": f"$__import__('os').system('echo pwned > {marker}')", "initialize": []} | ||
| with open(os.path.join(bundle_root, "configs", "train.json"), "w") as f: | ||
| json.dump(malicious_config, f) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the marker write independent of shell quoting.
Both payloads insert marker into an unquoted os.system() command. A temporary directory with spaces or shell characters can write to the wrong path. Windows backslashes can also make the generated Python expression invalid. Use a representation-escaped path and a direct Python write.
Proposed fix
- malicious_config = {"network_def": f"$__import__('os').system('echo pwned > {marker}')", "initialize": []}
+ malicious_config = {
+ "network_def": f"$__import__('pathlib').Path({marker!r}).write_text('pwned')",
+ "initialize": [],
+ }
...
- json.dump({"initialize": [f"$__import__('os').system('echo pwned > {marker}')"]}, f)
+ json.dump(
+ {"initialize": [f"$__import__('pathlib').Path({marker!r}).write_text('pwned')"]},
+ f,
+ )Also applies to: 545-545
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 515-515: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(bundle_root, "configs", "train.json"), "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@tests/bundle/test_bundle_download.py` around lines 515 - 517, Update the
malicious payload construction in the bundle download tests at both occurrences
to avoid shell parsing: embed the marker path using a representation-safe
escaping method and invoke a direct Python file-write operation instead of
os.system. Preserve the test’s expected marker content while ensuring paths with
spaces, shell characters, or Windows backslashes remain valid.
monai.bundle.load(), with its default model=None, builds a bundle's network by parsing the bundle's own config through create_workflow(). That parsing resolves any "_target_" value to an importable callable with no allow list, and passes any "$"-prefixed value to Python eval(). monai.bundle.run() reaches the same path via a caller-supplied config_file. Either way, loading or running a bundle whose config hasn't been reviewed can execute arbitrary code. create_workflow() -- the shared path both load() and run() use to parse a config file -- now raises a UserWarning immediately before doing so, describing what "_target_"/"$"-expression content can do and linking the advisory. This applies uniformly to every caller of create_workflow(), not just load(). No behavior is blocked: the config is still parsed and executed exactly as before, just with a warning first. An earlier version of this fix added an opt-in trust_remote_code flag to load(), but that was dropped after review: MONAI has no way to establish whether a bundle is actually trustworthy, so a flag like that would only teach callers to set it once and forget about it. Update docstrings on load(), run(), and create_workflow() to describe the risk and point at the advisory. Add TestLoadWarnsOnConfigExecution to tests/bundle/test_bundle_download.py: default load() warns and still executes the config, explicit model= skips config parsing entirely and warns about nothing, and run() warns via the same create_workflow() path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
95c1dce to
6b5fa64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/bundle/test_bundle_download.py (1)
499-512: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Google-style docstrings to the new methods.
Document
_stage_malicious_bundle()parameters and return value. Add concise docstrings to each test method.
tests/bundle/test_bundle_download.py#L499-L512: documenttempdir,marker, and the returned bundle name.tests/bundle/test_bundle_download.py#L514-L524: document the defaultload()warning and execution test.tests/bundle/test_bundle_download.py#L526-L532: document the explicit-model parsing bypass test.tests/bundle/test_bundle_download.py#L534-L546: document therun()warning and execution test.As per path instructions, “Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.”
🤖 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 `@tests/bundle/test_bundle_download.py` around lines 499 - 512, Add concise Google-style docstrings to _stage_malicious_bundle and each affected test method in tests/bundle/test_bundle_download.py:499-512, 514-524, 526-532, and 534-546. Document tempdir, marker, and the returned bundle name with Args and Returns sections; document each test’s covered behavior, including the default load() warning/execution, explicit-model parsing bypass, and run() warning/execution, adding Raises only if applicable.Source: Path instructions
🤖 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/bundle/test_bundle_download.py`:
- Around line 518-523: Update both warning assertions in
tests/bundle/test_bundle_download.py at lines 518-523 and 541-545 to use
assertWarnsRegex(UserWarning, r"GHSA-873f-pvrv-4x83") instead of
assertWarns(UserWarning), while preserving the existing exception assertions and
test flow.
---
Nitpick comments:
In `@tests/bundle/test_bundle_download.py`:
- Around line 499-512: Add concise Google-style docstrings to
_stage_malicious_bundle and each affected test method in
tests/bundle/test_bundle_download.py:499-512, 514-524, 526-532, and 534-546.
Document tempdir, marker, and the returned bundle name with Args and Returns
sections; document each test’s covered behavior, including the default load()
warning/execution, explicit-model parsing bypass, and run() warning/execution,
adding Raises only if applicable.
🪄 Autofix
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 Plus
Run ID: 4df8d078-b465-4202-a7a2-86a016fb3a2c
📒 Files selected for processing (2)
monai/bundle/scripts.pytests/bundle/test_bundle_download.py
🚧 Files skipped from review as they are similar to previous changes (1)
- monai/bundle/scripts.py
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Looks good to me, the Coderabbit comments look worthwhile however. I can rereview if you want to address
- tests/bundle/test_bundle_download.py: assert the advisory-specific warning message (GHSA-873f-pvrv-4x83) instead of any UserWarning in test_default_warns_and_executes_config and test_run_warns_on_config_execution - tests/bundle/test_bundle_download.py: fail test_explicit_model_skips_config_parsing if load() emits a UserWarning, enforcing that the explicit-model path never parses the bundle config
- tests/bundle/test_bundle_download.py: assert the advisory-specific warning message (GHSA-873f-pvrv-4x83) instead of any UserWarning in test_default_warns_and_executes_config and test_run_warns_on_config_execution - tests/bundle/test_bundle_download.py: fail test_explicit_model_skips_config_parsing if load() emits a UserWarning, enforcing that the explicit-model path never parses the bundle config Signed-off-by: R. Garcia-Dias <rafaelagd@gmail.com>
b079e79 to
9b51a65
Compare
2 participants
Summary
Fixes GHSA-873f-pvrv-4x83: https://github.com/Project-MONAI/MONAI/security/advisories/GHSA-873f-pvrv-4x83
monai.bundle.load(), with its defaultmodel=None, builds a bundle's network by parsing the bundle's own config throughcreate_workflow(). That parsing resolves any"_target_"value to an importable callable with no allow list, and passes any"$"-prefixed value to Pythoneval().monai.bundle.run()reaches the same code path via a caller-suppliedconfig_file. Either way, this means loading or running a bundle whose config you haven't reviewed can execute arbitrary code.Design
An earlier version of this fix added an opt-in
trust_remote_codeflag toload(). Per review discussion, that was dropped: MONAI has no mechanism to actually establish whether a bundle is trustworthy (unlike, say, a per-repo "has custom code" check), so a flag like that mostly teaches people to set it once and forget about it, without giving them a real basis to decide.Instead:
create_workflow()— the shared path bothload()andrun()use to parse a config file — now raises aUserWarningimmediately before doing so, spelling out exactly what"_target_"/"$"-expression content can do and linking this advisory.load()/run()still parse and execute the config exactly as before.create_workflow(), not justload().Changes
monai/bundle/scripts.py: warning added increate_workflow(); docstrings onload(),run(), andcreate_workflow()updated to describe the risk and point at the advisory.tests/bundle/test_bundle_download.py:TestLoadWarnsOnConfigExecution— defaultload()warns and still executes the config (no flag needed), explicitmodel=still skips config parsing entirely (and warns about nothing), andrun()warns via the samecreate_workflow()path.Test plan
python3 -m unittest tests.bundle.test_bundle_download.TestLoadWarnsOnConfigExecution -vtests/bundle/test_bundle_download.py,tests/bundle/test_config_parser.py— no new failures vs.dev(remaining failures are pre-existing environment gaps: missingrequests/nibabel, onepdb/bdbquirk)🤖 Generated with Claude Code