Harden remote artifact path reads

[fallintoplace]

Summary

• Restrict read_cluster_artifact(path=...) to safe relative POSIX paths before loading the experiment.
• Quote the resolved remote artifact path and use cat -- when invoking the tunnel.
• Add path-mode regression tests for quoted reads and unsafe paths.

Why

The previous implementation interpolated the artifact path into a remote shell command. A path containing shell syntax could run extra commands on the remote host.

Tests

• uv run --with pytest python -m pytest tests/
• uv run --with ruff ruff check modelopt_mcp tests
• uv run --with ruff ruff format --check modelopt_mcp tests

Summary by CodeRabbit

• Bug Fixes

  • Remote artifact path reading now includes validation to prevent access through invalid or unsafe path configurations. Invalid paths return detailed error messages.

• Tests

  • Expanded test coverage for artifact path validation, including detection of unsafe path patterns and error scenarios.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR hardens remote artifact path reading by introducing validation before tunnel execution. A new _validate_remote_artifact_path() function rejects unsafe paths (absolute, traversal, command injection), integrates into read_cluster_artifact_impl with early validation and shlex.quote() for safe command quoting, and is covered by comprehensive positive and negative test cases.

Changes

Remote Artifact Path Validation

Layer / File(s)	Summary
Path validation contract and helper tools/mcp/modelopt_mcp/bridge.py	_SAFE_REMOTE_ARTIFACT_PATH regex allowlist and _validate_remote_artifact_path() function reject absolute paths, empty/./.. segments, and disallowed characters, returning a structured error reason or None.
Integration into artifact reading flow tools/mcp/modelopt_mcp/bridge.py	shlex import added; read_cluster_artifact_impl validates the path argument early and returns invalid_artifact_path error on failure; remote_path is built with sanitized join using rstrip('/'), and tunnel cat command is invoked with shlex.quote() wrapping.
Test coverage for validation and path handling tools/mcp/tests/test_bridge.py	Test imports (sys, types) and _install_fake_experiment helper enable mocking; positive test verifies relative paths resolve to shell-quoted remote cat commands with warn=True; parametrized negative tests confirm unsafe paths (traversal, absolute, injection) fail early with reason="invalid_artifact_path".

Sequence Diagram(s)

sequenceDiagram
  participant Client as read_cluster_artifact_impl
  participant Validator as _validate_remote_artifact_path
  participant Regex as _SAFE_REMOTE_ARTIFACT_PATH
  participant Tunnel as tunnel.run_command
  
  Client->>Validator: validate(path)
  Validator->>Regex: match segments and characters
  alt Path is invalid
    Validator-->>Client: error reason string
    Client-->>Client: return invalid_artifact_path
  else Path is valid
    Validator-->>Client: None
    Client->>Client: build remote_path (sanitized join)
    Client->>Tunnel: cat -- (shlex.quoted path)
    Tunnel-->>Client: artifact content
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
Security Anti-Patterns	❌ Error	tools/mcp/modelopt_mcp/bridge.py contains multiple “# nosec …” Bandit suppressions (e.g., “import subprocess # nosec B404”), which this check forbids.	Remove “# nosec” suppressions (or replace with safer code). If any are unavoidable, add explicit @NVIDIA/modelopt-setup-codeowners-approved justification in the PR description.
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Harden remote artifact path reads' clearly and concisely summarizes the main security-focused change: validating remote POSIX artifact paths to prevent shell injection attacks before constructing tunnel commands.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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