Raise exception for uninstalled LoadImage reader and print writer pkg…#8915
Raise exception for uninstalled LoadImage reader and print writer pkg…#8915VedhSontha wants to merge 8 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR updates optional-dependency handling for image writer resolution and explicit reader construction, adds warnings and clearer errors for unsafe archive extraction paths and links, and changes two README lines. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/data/test_image_rw.py (1)
142-153: ⚡ Quick winMake the missing-backend path deterministic.
Lines 144-153 only exercise the new
OptionalImportErrormessage when bothnibabelanditkare absent. In most environments at least one is installed, so the new diagnostic path can regress without any test coverage. Isolate this branch with a temporary registry or mocked writer constructors instead of ambient package state.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 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/data/test_image_rw.py` around lines 142 - 153, The current test relies on ambient package presence; make the missing-backend path deterministic by mocking or isolating the registry so resolve_writer and OptionalImportError are exercised reliably: in the test for the "else" branch, patch optional_import (or monkeypatch the module that defines optional_import) to return (_, False) for both "nibabel" and "itk", or temporarily replace the writer registry looked up by resolve_writer with an empty/mock registry, then call resolve_writer(".nrrd") and resolve_writer("unknown") to assert OptionalImportError contains "Please install: itk or nibabel."; ensure you restore the original state after the assertions.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/transforms/io/array.py`:
- Line 217: The warning call in LoadImage.__init__ currently uses
warnings.warn(f"{_r} is not supported with the given parameters {args}
{kwargs}.") which points the stack to MONAI internals; update this call to pass
stacklevel=2 so the emitted warning points to the user's call site (i.e., change
the warnings.warn call inside the LoadImage.__init__ method to include
stacklevel=2).
In `@tests/data/test_init_reader.py`:
- Around line 47-53: TestInitLoadImage.test_load_image currently calls
optional_import(pkg_map[r]) (defaulting allow_namespace_pkg=False) to set
has_pkg, but runtime code imports ITK (and NRRD) with optional_import(...,
allow_namespace_pkg=True), causing false negatives; update the test to call
optional_import(pkg_map[r], allow_namespace_pkg=True) when probing readers for
"itk" and "nrrd" (or set allow_namespace_pkg=True for those pkg_map entries) so
has_pkg matches the runtime import behavior used by LoadImaged and the
OptionalImportError expectation is correct.
---
Nitpick comments:
In `@tests/data/test_image_rw.py`:
- Around line 142-153: The current test relies on ambient package presence; make
the missing-backend path deterministic by mocking or isolating the registry so
resolve_writer and OptionalImportError are exercised reliably: in the test for
the "else" branch, patch optional_import (or monkeypatch the module that defines
optional_import) to return (_, False) for both "nibabel" and "itk", or
temporarily replace the writer registry looked up by resolve_writer with an
empty/mock registry, then call resolve_writer(".nrrd") and
resolve_writer("unknown") to assert OptionalImportError contains "Please
install: itk or nibabel."; ensure you restore the original state after the
assertions.
🪄 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: 056e8222-bb33-4ffe-899d-a967383763a4
📒 Files selected for processing (4)
monai/data/image_writer.pymonai/transforms/io/array.pytests/data/test_image_rw.pytests/data/test_init_reader.py
| ) | ||
| ) from e | ||
| except TypeError: # the reader doesn't have the corresponding args/kwargs | ||
| warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show the relevant section around line 217
ls -la monai/transforms/io/array.py
python3 - <<'PY'
import itertools, pathlib
p = pathlib.Path("monai/transforms/io/array.py")
lines = p.read_text().splitlines()
start, end = 200, 240
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PY
# Find all warnings.warn occurrences in this file
rg -n "warnings\.warn\s*\(" monai/transforms/io/array.pyRepository: Project-MONAI/MONAI
Length of output: 2624
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
p = pathlib.Path("monai/transforms/io/array.py")
lines = p.read_text().splitlines()
# show more context to identify surrounding def/class
for i in range(150, 230):
if i < 0 or i >= len(lines):
continue
print(f"{i+1:4d}: {lines[i]}")
PYRepository: Project-MONAI/MONAI
Length of output: 5311
Add stacklevel=2 to the warning in LoadImage.__init__.
monai/transforms/io/array.py line 217 uses warnings.warn(...) without stacklevel, so the warning location points at MONAI internals instead of the user call site.
Suggested change
- warnings.warn(f"{_r} is not supported with the given parameters {args} {kwargs}.")
+ warnings.warn(
+ f"{_r} is not supported with the given parameters {args} {kwargs}.",
+ stacklevel=2,
+ )🧰 Tools
🪛 Ruff (0.15.15)
[warning] 217-217: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🤖 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/transforms/io/array.py` at line 217, The warning call in
LoadImage.__init__ currently uses warnings.warn(f"{_r} is not supported with the
given parameters {args} {kwargs}.") which points the stack to MONAI internals;
update this call to pass stacklevel=2 so the emitted warning points to the
user's call site (i.e., change the warnings.warn call inside the
LoadImage.__init__ method to include stacklevel=2).
Source: Linters/SAST tools
| _, has_pkg = optional_import(pkg_map[r]) | ||
| if has_pkg: | ||
| inst = LoadImaged("image", reader=r) | ||
| self.assertIsInstance(inst, LoadImaged) | ||
| else: | ||
| with self.assertRaises(OptionalImportError): | ||
| LoadImaged("image", reader=r) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n --type=py 'optional_import\("itk"' -C2Repository: Project-MONAI/MONAI
Length of output: 5327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact code under review
sed -n '1,140p' tests/data/test_init_reader.py | nl -ba | sed -n '35,90p'
# Find the pkg_map definition / ITKReader mapping and the exact optional_import call site
rg -n "pkg_map|ITKReader|LoadImaged|test_load_image|optional_import" tests/data/test_init_reader.py -n
# Inspect optional_import signature/docs in repo
optional_import_file="$(rg -n --type=py "def optional_import" -S . | head -n 1 | cut -d: -f1)"
echo "Found optional_import definition in: $optional_import_file"
sed -n '1,200p' "$optional_import_file" | nl -ba | sed -n '1,200p'Repository: Project-MONAI/MONAI
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show reviewed section with line numbers (no `nl` in this sandbox)
sed -n '1,120p' tests/data/test_init_reader.py | cat -n | sed -n '35,90p'
# Locate the exact test and the ITK probe line
rg -n "TestInitLoadImage|test_load_image|pkg_map|optional_import|ITKReader|LoadImaged" tests/data/test_init_reader.py
# Find optional_import implementation and show its signature/defaults
opt_file="$(rg -n --type=py "def optional_import" -S . | head -n 1 | cut -d: -f1)"
echo "optional_import definition: $opt_file"
sed -n '1,220p' "$opt_file" | cat -n | sed -n '1,220p'Repository: Project-MONAI/MONAI
Length of output: 13955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line number of optional_import definition
opt_def_line="$(rg -n --type=py "^def optional_import\\b" monai/utils/module.py | head -n 1 | cut -d: -f1)"
echo "optional_import def line: $opt_def_line"
# Print a window around the function start (signature + docstring header)
start=$((opt_def_line-20))
end=$((opt_def_line+120))
sed -n "${start},${end}p" monai/utils/module.py | cat -n
# Also find any occurrences of allow_namespace_pkg inside the function/file
rg -n "allow_namespace_pkg" monai/utils/module.py | head -n 50Repository: Project-MONAI/MONAI
Length of output: 7091
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where ITKReader is resolved/used during LoadImaged/LoadImage
rg -n "ITKReader" monai -S
rg -n "LoadImaged\\(|LoadImage\\(" monai/transforms -S
rg -n "reader=.*ITKReader|resolve_reader|resolve_reader\\(|image_reader" monai -S
# Inspect relevant reader resolution code (likely monai/data/image_reader.py and monai/data/__init__.py / transforms loader)
sed -n '1,220p' monai/data/image_reader.py | cat -n
# Also inspect image_writer if needed (for writer mapping consistency)
sed -n '1,140p' monai/data/image_writer.py | cat -nRepository: Project-MONAI/MONAI
Length of output: 21170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --type=py "^def require_pkg\\b" monai/utils/module.py
req_file="monai/utils/module.py"
req_line="$(rg -n --type=py "^def require_pkg\\b" "$req_file" | head -n 1 | cut -d: -f1)"
echo "require_pkg def line: $req_line"
start=$((req_line-20))
end=$((req_line+220))
sed -n "${start},${end}p" "$req_file" | cat -n
# See whether allow_namespace_pkg is referenced inside require_pkg
rg -n "allow_namespace_pkg" "$req_file" | head -n 50Repository: Project-MONAI/MONAI
Length of output: 10172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "has_itk|\\bitk\\b" monai/data/image_reader.pyRepository: Project-MONAI/MONAI
Length of output: 1442
Handle ITK as a namespace package in TestInitLoadImage.test_load_image
tests/data/test_init_reader.py:47 probes ITK with optional_import(pkg_map[r]) (defaults allow_namespace_pkg=False), but runtime monai/data/image_reader.py loads ITK with optional_import("itk", allow_namespace_pkg=True, so namespace-style ITK installs can make has_pkg false and cause a false OptionalImportError expectation.
- Same mismatch likely applies to
nrrd(runtime also usesallow_namespace_pkg=Truethere, but this test doesn’t special-case it).
Suggested change
- _, has_pkg = optional_import(pkg_map[r])
+ import_kwargs = {"allow_namespace_pkg": True} if pkg_map[r] == "itk" else {}
+ _, has_pkg = optional_import(pkg_map[r], **import_kwargs)📝 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.
| _, has_pkg = optional_import(pkg_map[r]) | |
| if has_pkg: | |
| inst = LoadImaged("image", reader=r) | |
| self.assertIsInstance(inst, LoadImaged) | |
| else: | |
| with self.assertRaises(OptionalImportError): | |
| LoadImaged("image", reader=r) | |
| import_kwargs = {"allow_namespace_pkg": True} if pkg_map[r] == "itk" else {} | |
| _, has_pkg = optional_import(pkg_map[r], **import_kwargs) | |
| if has_pkg: | |
| inst = LoadImaged("image", reader=r) | |
| self.assertIsInstance(inst, LoadImaged) | |
| else: | |
| with self.assertRaises(OptionalImportError): | |
| LoadImaged("image", reader=r) |
🤖 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/data/test_init_reader.py` around lines 47 - 53,
TestInitLoadImage.test_load_image currently calls optional_import(pkg_map[r])
(defaulting allow_namespace_pkg=False) to set has_pkg, but runtime code imports
ITK (and NRRD) with optional_import(..., allow_namespace_pkg=True), causing
false negatives; update the test to call optional_import(pkg_map[r],
allow_namespace_pkg=True) when probing readers for "itk" and "nrrd" (or set
allow_namespace_pkg=True for those pkg_map entries) so has_pkg matches the
runtime import behavior used by LoadImaged and the OptionalImportError
expectation is correct.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monai/apps/utils.py (1)
136-156: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winQuote untrusted member paths in warnings to avoid log injection.
member_pathcomes straight from the archive (attacker-controlled). Logging it raw lets crafted names inject newlines/control chars into logs. Use!rto quote.🛡️ Proposed fix
- logger.warning(f"Unsafe path guard: Symbolic link blocked: {member_path}") + logger.warning(f"Unsafe path guard: Symbolic link blocked: {member_path!r}") raise ValueError(f"Symbolic link detected in archive: {member_path}") if hasattr(member, "islnk") and member.islnk(): - logger.warning(f"Unsafe path guard: Hard link blocked: {member_path}") + logger.warning(f"Unsafe path guard: Hard link blocked: {member_path!r}") raise ValueError(f"Hard link detected in archive: {member_path}") member_path = os.path.normpath(member_path) if os.path.isabs(member_path) or ".." in member_path.split(os.sep): - logger.warning(f"Unsafe path guard: Absolute/relative path traversal blocked: {member_path}") + logger.warning(f"Unsafe path guard: Absolute/relative path traversal blocked: {member_path!r}") raise ValueError(f"Unsafe path detected in archive: {member_path}")And likewise at Line 155:
- logger.warning(f"Unsafe path guard: Out-of-bounds path traversal blocked: {member_path}") + logger.warning(f"Unsafe path guard: Out-of-bounds path traversal blocked: {member_path!r}")🤖 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/apps/utils.py` around lines 136 - 156, The unsafe path guard in utils logs attacker-controlled member paths directly, which can allow log injection. Update the warning calls in the archive extraction path to quote member_path using a repr-style format so control characters and newlines are escaped. Apply this consistently in the same guard logic that handles symbolic links, hard links, path traversal, and out-of-bounds checks, using the existing logger.warning statements and the member_path variable.
🧹 Nitpick comments (1)
monai/apps/utils.py (1)
125-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand the docstring to Google style.
The single-line docstring omits the now-relevant
Raises: ValueError, plusArgs/Returns. As per path instructions, docstrings should describe each variable, return value, and raised exception in Google-style sections.📝 Proposed docstring
def safe_extract_member(member, extract_to): - """Securely verify compressed package member paths to prevent path traversal attacks""" + """Securely verify compressed package member paths to prevent path traversal attacks. + + Args: + member: archive entry (`zipfile.ZipInfo` or `tarfile.TarInfo`) to validate. + extract_to: destination root directory for extraction. + + Returns: + The validated absolute target path within ``extract_to``. + + Raises: + ValueError: if the member is a symbolic/hard link, or its path is absolute, + relative, or otherwise resolves outside ``extract_to``. + """🤖 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/apps/utils.py` around lines 125 - 146, The docstring on safe_extract_member is too minimal for the current behavior, so expand it to Google style and document the parameters, the return value, and the ValueError cases raised for symbolic links, hard links, and unsafe paths. Keep the description aligned with the existing safe_extract_member logic and clearly describe extract_to, the member path handling, and the exception conditions in Args, Returns, and Raises sections.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.
Outside diff comments:
In `@monai/apps/utils.py`:
- Around line 136-156: The unsafe path guard in utils logs attacker-controlled
member paths directly, which can allow log injection. Update the warning calls
in the archive extraction path to quote member_path using a repr-style format so
control characters and newlines are escaped. Apply this consistently in the same
guard logic that handles symbolic links, hard links, path traversal, and
out-of-bounds checks, using the existing logger.warning statements and the
member_path variable.
---
Nitpick comments:
In `@monai/apps/utils.py`:
- Around line 125-146: The docstring on safe_extract_member is too minimal for
the current behavior, so expand it to Google style and document the parameters,
the return value, and the ValueError cases raised for symbolic links, hard
links, and unsafe paths. Keep the description aligned with the existing
safe_extract_member logic and clearly describe extract_to, the member path
handling, and the exception conditions in Args, Returns, and Raises sections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f92135dd-b06f-4fb5-8339-f7a0606319bf
📒 Files selected for processing (1)
monai/apps/utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/apps/utils.py (1)
126-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring lacks
Args/Returns/Raises.Given the function now raises
ValueErroron several paths, a Google-style docstring documenting params, return, and raised exceptions would help. 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 `@monai/apps/utils.py` at line 126, The docstring for the path-verification helper is incomplete and should follow Google style. Update the docstring on the function that checks compressed package member paths to include an Args section for its parameters, a Returns section for the boolean/result it produces, and a Raises section documenting the ValueError cases now emitted from this logic. Keep the wording aligned with the function’s purpose and mention the specific validation behavior in the docstring.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 `@monai/apps/utils.py`:
- Around line 154-161: The ValueError raised for a confirmed out-of-bounds
member in the path check is being caught by the same except block, which causes
double logging and misclassifies a real traversal as a drive-mismatch case. In
the utility that compares `extract_root` and `target_real` with
`os.path.commonpath`, limit the `try/except ValueError` to just the
`commonpath()` call, then handle the out-of-bounds result separately in the
surrounding logic so only genuine `commonpath()` errors hit the fallback logger
and re-raise path-traversal errors once with the correct message.
---
Nitpick comments:
In `@monai/apps/utils.py`:
- Line 126: The docstring for the path-verification helper is incomplete and
should follow Google style. Update the docstring on the function that checks
compressed package member paths to include an Args section for its parameters, a
Returns section for the boolean/result it produces, and a Raises section
documenting the ValueError cases now emitted from this logic. Keep the wording
aligned with the function’s purpose and mention the specific validation behavior
in the docstring.
🪄 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: 40a9185a-1343-435e-81ab-91f3f82c0a2e
📒 Files selected for processing (1)
monai/apps/utils.py
… if extra_keys mismatch in PrepareBatchHoVerNet
…t subsequent run corruption
1 participant
… suggestion
Fixes # .
Description
A few sentences describing the changes proposed in this pull request.
Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.