T09 · Insecure Skill Coding Practices
Error
- Location
- src/gate.py:77
- Finding
- Git Diff Failures Are Misclassified as Safe Review Skips<![CDATA[ ## Vulnerability Details **File Location**: `src/gate.py:77-83`, `src/gate.py:125-128`, and `src/gate.py:145-146` **Vulnerability Type**: Fail-open security gate caused by ignored subprocess errors **Risk Level**: High **Classification**: T09: Insecure Skill Coding Practices ### Vulnerable Code ```python result = subprocess.run( cmd, capture_output=True, text=True, encoding="utf-8", timeout=30, ) return result.stdout ``` ```python def should_skip(self) -> bool: """判断是否应该跳过审查。""" diff = self.get_diff() if not diff.strip(): print("No changes detected. Skipping review.") return True ``` ```python def run(self) -> int: """执行门禁检查，返回退出码。""" if self.should_skip(): return 3 # 跳过 ``` ### Technical Analysis The `get_diff()` method does not inspect `subprocess.run()`'s return code and does not use `check=True`. If `git diff` fails, Git normally writes an error to stderr, returns a nonzero status, and may leave stdout empty. The method discards that failure state and returns only stdout. The empty result is subsequently interpreted by `should_skip()` as evidence that there are no changes. `run()` then returns exit code 3 without executing any of the seven review checkers. The documented gate behavior identifies exit code 3 as a non-blocking skip, so a Git error can become an approval-equivalent outcome. The subprocess call uses an argument array and does not enable a shell, so this is not a command-injection vulnerability. The issue is specifically the failure to distinguish a valid empty diff from an unsuccessful Git operation. ### Attack Path 1. An attacker or misconfigured CI invocation supplies an invalid, missing, or ambiguous revision through `--base` or `--head`. 2. The application constructs and runs `git diff <base>..<head>`. 3. Git returns a nonzero exit status and writes diagnostic information to stderr. 4. `get_diff()` ignores both the nonzero status and stderr and returns emp ...[truncated 749 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require successful Git execution before accepting stdout: ```python try: result = subprocess.run( cmd, capture_output=True, text=True, encoding="utf-8", timeout=30, check=True, ) except subprocess.CalledProcessError as exc: print( f"Error: git diff failed with exit code {exc.returncode}: " f"{exc.stderr.strip()}", file=sys.stderr, ) raise GateExecutionError("Unable to obtain Git diff") from exc ``` 2. Convert `CalledProcessError` and other operational failures into exit code 2, not skip code 3. 3. Apply equivalent return-code validation to `get_diff_stats()`. Statistics may remain nonessential, but failures should not conceal problems in the primary diff operation. 4. Retrieve and cache the diff once per run. The current implementation invokes `get_diff()` in both `should_skip()` and `run()`, allowing inconsistent results if repository state changes between calls. 5. Add regression tests for invalid revisions, ambiguous revisions, non-repository directories, permission failures, and mocked nonzero Git return codes. 6. Configure CI to treat unexpected exit codes and execution errors as blocking outcomes. ]]>