Skip to content

fix(deps): prevent CMake manifest injection - #235

Open
wyli wants to merge 1 commit into
mainfrom
fix/prevent-cmake-manifest-injection
Open

fix(deps): prevent CMake manifest injection#235
wyli wants to merge 1 commit into
mainfrom
fix/prevent-cmake-manifest-injection

Conversation

@wyli

@wyli wyli commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • encode metadata-derived manifest values as literal CMake bracket arguments
  • select a delimiter that cannot be closed by the encoded value
  • use bracket comments for metadata-derived diagnostic text
  • retain existing HTTPS, SSH, SCP-style, and local-path Git source support

Vulnerability confirmation

This issue is real and reachable. Local build paths used by build, install, and run parse project-controlled metadata.json and modules/module-sites.json values into ModuleDep records. write_external_operators_manifest previously interpolated those values into double-quoted CMake arguments, and the generated manifest is included during CMake configuration before FetchContent processes the dependency.

A crafted Git URL can close the quoted argument and declaration, introduce a new CMake command, and make the remaining generated tokens part of a valid command. A cmake -P reproduction reached the injected command before this change.

Why this fixes it

CMake bracket arguments treat their contents literally: quotes, parentheses, backslashes, variable references, and newlines cannot alter the surrounding syntax. _cmake_bracket_argument chooses an equals-sign delimiter whose closing sequence does not occur in the value, preventing the value from terminating its own argument. The same encoding is applied consistently to Git URLs, refs, local paths, operator names, cache descriptions, and metadata-derived comments so another field cannot provide an equivalent breakout.

This preserves accepted source formats instead of imposing a URL allowlist. It is independent defense in depth alongside #234, which separately requires immutable remote refs.

Testing

  • python -m pytest -q -o addopts='' tests/unit (494 passed, 1 skipped)
  • PYTHONPATH=src PRE_COMMIT_HOME=/tmp/holoscan-cli-pre-commit-cache-mutable-refs python -m holoscan_cli lint
  • generated a manifest with breakout payloads in both URL and ref, then verified it completed under cmake -P without executing the injected command

AI-assisted: Created with Codex/GPT at the user's request.

Summary by CodeRabbit

  • Bug Fixes
    • Improved CMake manifest generation to safely handle special characters in metadata and Git values.
    • Prevented malformed or unexpectedly interpreted configuration values when generating manifests.
    • Updated generated repository, tag, source, operator, comment, and override settings for more reliable parsing.

Encode metadata-derived manifest values as collision-safe CMake bracket arguments and comments. This keeps URLs, refs, paths, and operator names literal without narrowing supported Git source forms.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Wenqi Li <wenqil@nvidia.com>
@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b404d995-d310-4d83-b30f-026cbb12ae9d

📥 Commits

Reviewing files that changed from the base of the PR and between 655a34b and b843ca4.

📒 Files selected for processing (2)
  • src/holoscan_cli/utils/cmake_manifest.py
  • tests/unit/test_cmake_manifest.py

Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.


Walkthrough

The CMake manifest generator now emits metadata and module arguments as dynamically delimited bracket arguments. Unit tests cover delimiter collisions, multiline Git values, operator names, and local override paths.

Changes

CMake manifest encoding

Layer / File(s) Summary
Bracket argument encoding contract
src/holoscan_cli/utils/cmake_manifest.py, tests/unit/test_cmake_manifest.py
Added _cmake_bracket_argument with collision-free delimiters and tests for bracket-like and embedded injection text.
Manifest value integration
src/holoscan_cli/utils/cmake_manifest.py, tests/unit/test_cmake_manifest.py
Updated comments, local overrides, external module arguments, Git values, source directories, and operator lists to use bracket arguments. Updated manifest assertions accordingly.

Estimated code review effort: 2 (Simple) | ~15 minutes

Merge Risk: ⚪ Minimal · up to b843c

The change narrowly hardens generated CMake manifest values against injection while preserving supported Git source formats, and no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: preventing injection in generated CMake dependency manifests.
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.

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant