Fix For ghsa-rghg-q7wp-9767

[ericspod]

Fixes advisory ghsa-rghg-q7wp-9767.

Description

A few sentences describing the changes proposed in this pull request.

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Integration tests passed locally by running ./runtests.sh -f -u --net --coverage.
• Quick tests passed locally by running ./runtests.sh --quick --unittests --disttests.
• In-line docstrings updated.
• Documentation updated, tested make html command in the docs/ folder.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR hardens dataset identifier handling in nnUNetV2Runner. It introduces DATASET_ID_FORMAT regex to validate dataset identifiers at initialization, rejecting invalid patterns. The runner now passes raw identifiers to maybe_convert_to_dataset_name rather than pre-converting to int. Command construction in train_single_model_command is refactored with explicit list building and prefix selection (single-dash for p and pretrained_weights, double-dash otherwise). Three methods—train_parallel_cmd, train_parallel, and predict_ensemble_postprocessing—now guard against None dataset_name with explicit ValueError checks. Integration tests verify that valid identifiers (Dataset123, integer 123) succeed while shell-injection attempts fail.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	Description is incomplete. Only the advisory link and checklist are provided; the actual description of changes is missing.	Add detailed explanation of what changes were made to fix the vulnerability and why they were necessary.
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title references the security advisory being fixed (GHSA-rghg-q7wp-9767), which is the main objective of the PR. It's specific and directly relevant.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ericspod]

Hi @garciadias please review this again, it's the same code you've approved elsewhere.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

tests/integration/test_integration_nnunetv2_runner.py (1)

23-34: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Scope the logger suppression to this test class.

Line 33 mutates the runner logger at import time and never restores it, so unrelated tests in the same process lose warning output too. Patch it in setUpClass/tearDownClass or in the specific tests that need it.

🤖 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/integration/test_integration_nnunetv2_runner.py` around lines 23 - 34,
The global logger suppression call
monai.apps.nnunet.nnunetv2_runner.logger.setLevel(logging.ERROR) is applied at
import time and should be scoped to the test class; move this mutation into the
test class lifecycle (e.g., setUpClass to save the original level and set ERROR,
and tearDownClass to restore the saved level) or apply/reset inside individual
tests that need it so unrelated tests are not affected; ensure you reference
monai.apps.nnunet.nnunetv2_runner.logger and restore its original level when
done.

monai/apps/nnunet/nnunetv2_runner.py (1)

200-213: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Normalize dataset_name_or_id once here.

Line 204 accepts "Dataset123", but Line 212 immediately does int(self.dataset_name_or_id). That sends valid Dataset### inputs through the exception path, leaves self.dataset_name as None, and makes the new guards reject an input this constructor already accepted. The remaining int(self.dataset_name_or_id) call sites in this class have the same mismatch, so the new contract is only partially implemented.

As per coding guidelines, "Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these."

🤖 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/nnunet/nnunetv2_runner.py` around lines 200 - 213, The constructor
normalizes dataset_name_or_id incorrectly by always calling
int(self.dataset_name_or_id) which fails for valid non-numeric names like
"Dataset123"; change the logic in the block referencing self.dataset_name_or_id
and maybe_convert_to_dataset_name so that if self.dataset_name_or_id is purely
numeric (e.g., str.isdigit() or re.fullmatch(r"\d+")), call
maybe_convert_to_dataset_name(int(self.dataset_name_or_id)) and assign to
self.dataset_name, otherwise treat the value as an explicit dataset name and set
self.dataset_name = self.dataset_name_or_id (no int conversion); keep the same
exception handling but ensure downstream code that currently calls int(...) on
self.dataset_name_or_id is updated to only do that when the value is numeric.

🧹 Nitpick comments (1)

tests/integration/test_integration_nnunetv2_runner.py (1)

150-163: ⚡ Quick win

Exercise a path that reads runner.dataset_name.

The happy-path test only proves __init__ does not raise. It never touches a method that consumes self.dataset_name, so it would miss the valid-Dataset123 regression where initialization succeeds but later guarded methods fail. Add an assertion on runner.dataset_name or call one guarded method with a prepared temp layout.

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/integration/test_integration_nnunetv2_runner.py` around lines 150 -
163, The tests currently only construct nnUNetV2Runner but never exercise the
attribute or methods that consume dataset_name; update
test_nnunetv2runner_good_dataset_name to either assert the created
runner.dataset_name equals the expected value for each good_yml (or call a
method that uses it, e.g., any method that references self.dataset_name) so the
code path that reads/validates dataset_name is executed; reference the
nnUNetV2Runner class and the dataset_name attribute in your change and similarly
ensure test_nnunetv2runner_bad_dataset_name still triggers the ValueError path
when using inject_yml.

🤖 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/integration/test_integration_nnunetv2_runner.py`:
- Around line 110-148: The test YAMLs currently use relative paths (./data,
./work, ./nnUNet_raw, ./nnUNet_preprocessed, ./nnUNet_results) so
nnUNetV2Runner.__init__ creates directories in the repo cwd; update the YAML
strings (good_yml_content1, good_yml_content2, injecting_yml_content) to build
all generated paths inside the test temp dir (use self.test_dir) for keys
dataroot, datalist, work_dir, nnunet_raw, nnunet_preprocessed, and
nnunet_results so the runner creates those folders under self.test_dir and
tearDown can clean them up (also ensure the injected dataset_name_or_id test
keeps the path substitution to self.test_dir).

---

Outside diff comments:
In `@monai/apps/nnunet/nnunetv2_runner.py`:
- Around line 200-213: The constructor normalizes dataset_name_or_id incorrectly
by always calling int(self.dataset_name_or_id) which fails for valid non-numeric
names like "Dataset123"; change the logic in the block referencing
self.dataset_name_or_id and maybe_convert_to_dataset_name so that if
self.dataset_name_or_id is purely numeric (e.g., str.isdigit() or
re.fullmatch(r"\d+")), call
maybe_convert_to_dataset_name(int(self.dataset_name_or_id)) and assign to
self.dataset_name, otherwise treat the value as an explicit dataset name and set
self.dataset_name = self.dataset_name_or_id (no int conversion); keep the same
exception handling but ensure downstream code that currently calls int(...) on
self.dataset_name_or_id is updated to only do that when the value is numeric.

In `@tests/integration/test_integration_nnunetv2_runner.py`:
- Around line 23-34: The global logger suppression call
monai.apps.nnunet.nnunetv2_runner.logger.setLevel(logging.ERROR) is applied at
import time and should be scoped to the test class; move this mutation into the
test class lifecycle (e.g., setUpClass to save the original level and set ERROR,
and tearDownClass to restore the saved level) or apply/reset inside individual
tests that need it so unrelated tests are not affected; ensure you reference
monai.apps.nnunet.nnunetv2_runner.logger and restore its original level when
done.

---

Nitpick comments:
In `@tests/integration/test_integration_nnunetv2_runner.py`:
- Around line 150-163: The tests currently only construct nnUNetV2Runner but
never exercise the attribute or methods that consume dataset_name; update
test_nnunetv2runner_good_dataset_name to either assert the created
runner.dataset_name equals the expected value for each good_yml (or call a
method that uses it, e.g., any method that references self.dataset_name) so the
code path that reads/validates dataset_name is executed; reference the
nnUNetV2Runner class and the dataset_name attribute in your change and similarly
ensure test_nnunetv2runner_bad_dataset_name still triggers the ValueError path
when using inject_yml.

🪄 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: 9e6b46ff-3d01-4d50-b1b1-3975cd0db459

📥 Commits

Reviewing files that changed from the base of the PR and between dc0f59c and 0a7f413.

📒 Files selected for processing (2)

• monai/apps/nnunet/nnunetv2_runner.py
• tests/integration/test_integration_nnunetv2_runner.py