fix(security): harden template path read against path traversal#977
Merged
Conversation
Resolves the SAST CWE-22 finding on _converter_endpoint.py's _endpoint_template_fallback by introducing _safe_read_template_path, which: (a) rejects symlinks before resolution, (b) resolves via Path.resolve(strict=True) so non-existent paths fall back to the literal-template-body branch, (c) requires the resolved target to be a regular file, and (d) reads with explicit UTF-8 encoding. The identical pattern in _endpoint_template_from_extra (line 33) is rewired through the same helper so a re-scan does not re-flag it. Behavior change to note: --extra-inputs payload_template=<symlink> now treats the path string as a literal template body instead of following the symlink. Inline template strings and regular file paths are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@1f26187f37ea4301b0fd2a6a7c91745104e98a4fRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@1f26187f37ea4301b0fd2a6a7c91745104e98a4fLast updated for commit: |
Contributor
|
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:
WalkthroughAdds a hardened safe_read_template_path helper that safely reads UTF-8 template files (rejecting symlinks, symlinked parents, non-regular targets, and failed resolves) and uses it in converter and endpoint template-loading paths; extensive unit tests and docs/guidance cover successful reads and all failure/fallback cases. ChangesTemplate Path Safety
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/config/test_converter_endpoint_template_paths.py (1)
34-34: ⚡ Quick winRename these tests to include the function under test.
Names like
test_regular_file_is_read_as_bodyare duplicated across both classes and hide whether the case exercises_endpoint_template_from_extraor_endpoint_template_fallback. Please align them totest_<function>_<scenario>_<expected>.As per coding guidelines
tests/**/*.py: Test naming convention:test_<function>_<scenario>_<expected>.Also applies to: 44-44, 54-56, 68-68, 77-77, 89-89, 101-103, 117-117, 135-137
🤖 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/unit/config/test_converter_endpoint_template_paths.py` at line 34, Rename the duplicated test methods to follow the convention test_<function>_<scenario>_<expected> and explicitly indicate which function is under test: replace ambiguous names like test_regular_file_is_read_as_body with names such as test__endpoint_template_from_extra_regular_file_read_as_body or test__endpoint_template_fallback_regular_file_read_as_body depending on which helper the test exercises; do the same for the other listed tests (lines referenced) so each test name includes either _endpoint_template_from_extra or _endpoint_template_fallback plus a brief scenario and expected outcome, keeping names consistent across both test classes.
🤖 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 `@src/aiperf/config/flags/_converter_endpoint.py`:
- Around line 39-45: The code in _safe_read_template_path currently only checks
path.is_symlink() on the leaf and then resolves, which allows symlinked parent
directories to be followed; update _safe_read_template_path to iterate over path
and path.parents and return None (or raise as the function convention) if any
component .is_symlink() is True before calling path.resolve(strict=True), so
that symlinked parent components like link_dir/template.json are rejected;
locate the checks around the existing path.is_symlink() and path.resolve(...)
calls and add the parents iteration there to enforce the hardening.
In `@tests/unit/config/test_converter_endpoint_template_paths.py`:
- Around line 128-134: Replace usages of pytest.param with param in the
`@pytest.mark.parametrize` block for the ep_type cases in
tests/unit/config/test_converter_endpoint_template_paths.py by adding an import
"from pytest import param" at the top of the test file and updating the two
entries to use param(id="chat") and param(id="completions"); also add a "# fmt:
skip" comment on the closing ")" of the multiline `@pytest.mark.parametrize`(...)
decorator to prevent formatter reflow. Ensure you update the decorator that
defines ep_type (the pytest.mark.parametrize block) and the file-level imports
accordingly.
---
Nitpick comments:
In `@tests/unit/config/test_converter_endpoint_template_paths.py`:
- Line 34: Rename the duplicated test methods to follow the convention
test_<function>_<scenario>_<expected> and explicitly indicate which function is
under test: replace ambiguous names like test_regular_file_is_read_as_body with
names such as test__endpoint_template_from_extra_regular_file_read_as_body or
test__endpoint_template_fallback_regular_file_read_as_body depending on which
helper the test exercises; do the same for the other listed tests (lines
referenced) so each test name includes either _endpoint_template_from_extra or
_endpoint_template_fallback plus a brief scenario and expected outcome, keeping
names consistent across both test classes.
🪄 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: 89da1b90-3d36-4d04-b06a-3fc22aba5531
📒 Files selected for processing (2)
src/aiperf/config/flags/_converter_endpoint.pytests/unit/config/test_converter_endpoint_template_paths.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
dynamo-ops
reviewed
May 21, 2026
Adds a focused regression for the read-step OSError handler: chmod 0o000 on a real file passes is_file() but fails read_text() with PermissionError, which the helper must swallow and return None so the caller falls back to the literal-template-body branch. Backfills the codecov gap reported on #977. Skipped on Windows (ACL semantics differ) and when running as root (mode bits bypassed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
…path
Parametrized over int, None, list, dict to exercise the
``(TypeError, ValueError)`` catch on ``Path(ts).expanduser()``.
Defensive coverage against an upstream parser smuggling a non-str
value through ``dict.get("payload_template")``.
Brings the helper to 100% line coverage; closes the remaining
codecov gap on #977.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/config/test_converter_endpoint_template_paths.py (2)
189-197: ⚡ Quick winAlign parametrized style with repo test conventions.
At Line 192 and Line 197, switch to
from pytest import param+param(...), and add# fmt: skipon the decorator closing)line.As per coding guidelines: “Use
from pytest import paramand put# fmt: skipon the)line for parametrized 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/unit/config/test_converter_endpoint_template_paths.py` around lines 189 - 197, Update the pytest param style in the pytest.mark.parametrize call for the bad_input cases: replace pytest.param(...) entries with param(...) by adding "from pytest import param" to the imports, and convert each pytest.param(42, id="int") etc. to param(42, id="int") for the bad_input list used with the pytest.mark.parametrize decorator; also add a trailing comment "# fmt: skip" on the closing ")" of that decorator to preserve formatting. Locate the decorator and the bad_input arguments around the test name (the pytest.mark.parametrize block in tests/unit/config/test_converter_endpoint_template_paths.py) and apply these changes.
198-198: ⚡ Quick winRename test to include the target function in the pattern.
At Line 198, prefer a name like
test_safe_read_template_path_non_string_input_returns_noneto matchtest_<function>_<scenario>_<expected>.As per coding guidelines: “Test naming:
test_<function>_<scenario>_<expected>.”🤖 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/unit/config/test_converter_endpoint_template_paths.py` at line 198, Rename the test method to follow the project's naming convention by including the target function name and scenario; change the method currently named test_non_string_input_returns_none to test_safe_read_template_path_non_string_input_returns_none so it follows the test_<function>_<scenario>_<expected> pattern and clearly indicates it covers safe_read_template_path handling non-string inputs returning None.
🤖 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.
Nitpick comments:
In `@tests/unit/config/test_converter_endpoint_template_paths.py`:
- Around line 189-197: Update the pytest param style in the
pytest.mark.parametrize call for the bad_input cases: replace pytest.param(...)
entries with param(...) by adding "from pytest import param" to the imports, and
convert each pytest.param(42, id="int") etc. to param(42, id="int") for the
bad_input list used with the pytest.mark.parametrize decorator; also add a
trailing comment "# fmt: skip" on the closing ")" of that decorator to preserve
formatting. Locate the decorator and the bad_input arguments around the test
name (the pytest.mark.parametrize block in
tests/unit/config/test_converter_endpoint_template_paths.py) and apply these
changes.
- Line 198: Rename the test method to follow the project's naming convention by
including the target function name and scenario; change the method currently
named test_non_string_input_returns_none to
test_safe_read_template_path_non_string_input_returns_none so it follows the
test_<function>_<scenario>_<expected> pattern and clearly indicates it covers
safe_read_template_path handling non-string inputs returning None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f646cd15-f646-4c17-be60-908031fab46d
📒 Files selected for processing (1)
tests/unit/config/test_converter_endpoint_template_paths.py
Folds in four review findings from PR #977: 1. Symlinked parent directories no longer bypass the leaf-only ``is_symlink()`` check. The helper now iterates ``(path, *path.parents)`` and rejects any symlinked component, so ``link_dir/template.json`` is treated as a literal template body instead of having its target read via ``resolve()`` following the parent symlink. 2. ``Path(ts).expanduser()`` raising ``RuntimeError`` (e.g. for ``~unknownuser/...`` prefixes) is now caught — previously such inputs crashed config conversion instead of falling back to the literal-template-body branch. 3. ``TemplateEndpoint.__init__`` shared the same unhardened ``Path(...).is_file() + read_text()`` pattern when AIPerfConfig is consumed via YAML/direct (bypassing the CLI converter). The helper now lives in ``src/aiperf/common/path_safety.py`` and is reused by both ``_converter_endpoint`` and ``template_endpoint``, closing the second SAST-flaggable site. 4. Parametrized tests now use ``param(...)`` (via ``from pytest import param``) with ``# fmt: skip`` on the closing ``)`` line, per CLAUDE.md style. New tests cover (a) symlinked-parent rejection, (b) RuntimeError expanduser fallback, and (c) TemplateEndpoint symlink rejection through ``payload_template``. Watched each fail RED first; all GREEN after the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unit/endpoints/test_template_endpoint.py`:
- Line 470: The test function test_symlinked_payload_template_treated_as_literal
is missing type annotations; add parameter and return type hints by annotating
tmp_path as pathlib.Path (or Path after adding "from pathlib import Path") and
specify a return type of None, and ensure the appropriate import (from pathlib
import Path) is present at the top of the test module so the signature becomes
def test_symlinked_payload_template_treated_as_literal(tmp_path: Path) -> None.
🪄 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: 6cd44ad8-ce1c-4ef7-a84c-8fa3e04e29d2
📒 Files selected for processing (5)
src/aiperf/common/path_safety.pysrc/aiperf/config/flags/_converter_endpoint.pysrc/aiperf/endpoints/template_endpoint.pytests/unit/config/test_converter_endpoint_template_paths.pytests/unit/endpoints/test_template_endpoint.py
…ad pattern Adds a Coding Standards bullet to the four synced agent files (AGENTS.md, CLAUDE.md, .github/copilot-instructions.md, .cursor/rules/python.mdc) directing contributors and AI agents to use aiperf.common.path_safety.safe_read_template_path instead of inline Path().read_text() / open().read(), so the CWE-22 hardening landed in this PR is preserved across future code changes. Adds a "Safe Filesystem Reads Pattern" section to docs/dev/patterns.md with the sanitizer chain explained (the order SAST engines walk it) and an explicit "does NOT apply to" list (trusted-string path joining, binary reads, hard-fail semantics) so the rule doesn't generate false-positive review pushback on legitimate internal reads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
dynamo-ops
reviewed
May 22, 2026
dynamo-ops
approved these changes
May 22, 2026
dynamo-ops
approved these changes
May 22, 2026
dynamo-ops
approved these changes
May 22, 2026
Round 2 of PR #977 review feedback: 1. dynamo-ops (MEMBER): a non-UTF-8 file passing every prior sanitizer check still crashed at ``read_text(encoding="utf-8")`` with ``UnicodeDecodeError``, which is a ``ValueError`` subclass — NOT caught by the existing ``except OSError``. Widened the read-step catch to ``except (OSError, UnicodeError)`` so non-UTF-8 inputs collapse to ``None`` and the caller falls back to the literal template body. 2. CodeRabbit (MINOR): added type hints to ``test_symlinked_payload_template_treated_as_literal`` (CLAUDE.md: "Type hints on ALL functions") and pulled in the ``Path`` import. 3. codecov: lines 60-61 of ``template_endpoint.py`` (the "loaded from file" branch I introduced) were unexercised — only the symlink rejection path was tested. Added ``test_regular_file_payload_template_is_loaded`` to load a real template via ``payload_template=<path>`` through ``TemplateEndpoint``. New tests (RED-verified for the behavior change): - ``TestSafeReadTemplatePathDecodeFailures::test_non_utf8_file_returns_none`` - ``TestTemplateEndpointPathSafety::test_regular_file_payload_template_is_loaded`` ``safe_read_template_path`` is now at 100% line coverage; the TemplateEndpoint lines this PR introduced (58-61) are fully exercised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Frank Di Natale <3429989+FrankD412@users.noreply.github.com>
dynamo-ops
approved these changes
May 22, 2026
dynamo-ops
left a comment
dynamo-ops
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. Approving.
ajcasagrande
approved these changes
May 22, 2026
debermudez
approved these changes
May 22, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/aiperf/config/flags/_converter_endpoint.py::_endpoint_template_fallback(and the identical pattern in_endpoint_template_from_extra, which would otherwise re-flag on a re-scan)._safe_read_template_pathhelper that rejects symlinks pre-resolution, resolves viaPath.resolve(strict=True)(the canonical sanitizer SAST tools recognize), requires the resolved target to be a regular file, and reads with explicit UTF-8 encoding. Both call sites are rewired through it.Behavior change to note
Users who passed a symlink as
--extra-inputs payload_template=<path>previously had the symlink target's contents read in as the template body. After this change the symlink path string is treated as a literal template body (no symlink following). Inline strings and direct file paths are unchanged.Test plan
tests/unit/config/test_converter_endpoint_template_paths.py(10 cases) cover: regular file read, explicit UTF-8 decode, symlink rejection (the regression), missing path → literal fallback, directory path → literal fallback, and non-TEMPLATE endpoints skip the fallback. Verified RED first — the 2 symlink tests failed against pre-fix code (they read the target contents instead of the literal path).uv run pytest tests/unit/config/ tests/unit/endpoints/ -n auto→ 2245 passed, 55 skipped, 0 failed.uv run pytest tests/unit/property/ -n auto→ 59 passed.pre-commit runon staged files → all 30 hooks pass (incl.check-ergonomics,check-ruff-baselined,ruff format,test-imports).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation