fix(security): validate patterns_path in drift service (sonar S2083)#167
Conversation
Resolves SonarCloud BLOCKER S2083 (path injection): canonicalize and suffix-check patterns_path in analyze_drift() before constructing DriftScorer, breaking the taint chain from MCP tool inputs to the YAML parser. Two new unit tests cover the ValueError and happy-path cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation for the patterns_path in the analyze_drift function, ensuring it has a valid .yaml or .yml suffix and exists on disk after being resolved. Corresponding unit tests have also been added to verify this behavior. The review feedback correctly identifies that the suffix check is case-sensitive and suggests using .lower() to ensure case-insensitive matching for extensions like .YAML.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/guardian review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — no defects found in this diff.
Reviewed diff for logic correctness, type safety, library contracts, ontology compliance, and test coverage. All changes are correct and properly tested.
🤖 mistral-medium-latest · 4,460 prompt + 39 completion = 4,499 tokens · graph 1/2 files (50%)
Path.suffix preserves original casing, so .YAML/.Yml were rejected. Normalize with .lower() before the allow-list check (gemini review). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 participant
Summary
cgis_drift(db_path, patterns_path, ...)inmcp_server.py→analyze_drift(db_path, patterns_path, ...)indrift_service.py→DriftScorer.__init__doingPath(patterns_config).read_text()indrift.py:109. An LLM agent driving the MCP server could feed arbitrary paths (e.g..env,/etc/passwd) — parse-error echoes from the YAML parser could leak content fragments.analyze_drift(the shared service layer used by both MCP and CLI),patterns_pathis now canonicalized viaPath(patterns_path).resolve()before use. The resolved path must have suffix.yamlor.yml(raisesValueErrorotherwise) and must exist as a file (raisesFileNotFoundErrorotherwise). The resolved canonical path string is passed intoDriftScorer, breaking the direct taint chain.path.is_relative_to(cwd)) was added. cgis is a local-first dev tool where pointing at sibling repos is a supported cross-repo use-case; containment would break that workflow.Tests added (
tests/unit/test_drift_service.py)test_analyze_drift_bad_suffix_raises— wrong suffix (.txt) →ValueErrortest_analyze_drift_resolved_path_works— valid.yamlfile → correctDriftAnalysisreturnedGates
All 722 unit + self-parsing tests pass; ruff, mypy strict, and pre-commit hooks clean.
Sonar note
If Sonar's taint engine does not recognize the suffix check +
resolve()as a sanitizer, the issue should be marked accepted with this PR as justification: the path is validated to a YAML-only resolved canonical value before being read, and arbitrary file disclosure is not achievable through this vector.🤖 Generated with Claude Code