fix(test): replace shell=True with shell=False in cli_coverage_steps.py subprocess call #734

New issue

Closed

opened 2026-03-12 13:19:31 +00:00 by CoreRasurae · 1 comment

CoreRasurae commented

2026-03-12 13:19:31 +00:00

Member

Metadata

Commit Message: fix(test): replace shell=True with shell=False in cli_coverage_steps.py subprocess call
Branch: fix/m3-shell-true-subprocess

Summary

features/steps/cli_coverage_steps.py:44 uses subprocess.run(command, ..., shell=True) where command is a string built from BDD step definitions. While the command strings originate from .feature files authored by the development team (not runtime user input), shell=True with string input is a well-known command injection vector and violates defense-in-depth principles.

Background and Context

Identified during the security review of PR #727 (Security Finding #3 by @hurui200320). This is a pre-existing issue not introduced by that PR. The reviewer noted that the same pattern has already been correctly implemented with shell=False in cli_plan_context_commands_steps.py:351, making this an inconsistency.

Current Behavior

# features/steps/cli_coverage_steps.py:44
subprocess.run(command, ..., shell=True)

The command is a string passed from BDD step patterns like @when('I run shell command "{command}"'). With shell=True, the string is passed to the system shell for parsing, which could interpret shell metacharacters.

Expected Behavior

import shlex
subprocess.run(shlex.split(command), ..., shell=False)

This is consistent with the pattern already used in cli_plan_context_commands_steps.py:351.

Acceptance Criteria

shell=True is replaced with shell=False in cli_coverage_steps.py
shlex.split() is used to properly tokenize the command string
All scenarios using the I run shell command step still pass
No other shell=True usages remain in step files (audit)

Supporting Information

Identified in: PR #727 Security Review, Finding #3
Review URL: #727
Reviewer: @hurui200320
Reference implementation: cli_plan_context_commands_steps.py:351 (already uses shell=False)

Subtasks

Replace shell=True with shell=False + shlex.split() in cli_coverage_steps.py:44
Audit all step files for other shell=True usages
Fix any additional shell=True instances found
Run scenarios that use I run shell command step to verify correctness
Run nox -s unit_tests to verify no regressions

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `fix(test): replace shell=True with shell=False in cli_coverage_steps.py subprocess call` - **Branch**: `fix/m3-shell-true-subprocess` ## Summary `features/steps/cli_coverage_steps.py:44` uses `subprocess.run(command, ..., shell=True)` where `command` is a string built from BDD step definitions. While the command strings originate from `.feature` files authored by the development team (not runtime user input), `shell=True` with string input is a well-known command injection vector and violates defense-in-depth principles. ## Background and Context Identified during the security review of PR #727 (Security Finding #3 by @hurui200320). This is a pre-existing issue not introduced by that PR. The reviewer noted that the same pattern has already been correctly implemented with `shell=False` in `cli_plan_context_commands_steps.py:351`, making this an inconsistency. ## Current Behavior ```python # features/steps/cli_coverage_steps.py:44 subprocess.run(command, ..., shell=True) ``` The `command` is a string passed from BDD step patterns like `@when('I run shell command "{command}"')`. With `shell=True`, the string is passed to the system shell for parsing, which could interpret shell metacharacters. ## Expected Behavior ```python import shlex subprocess.run(shlex.split(command), ..., shell=False) ``` This is consistent with the pattern already used in `cli_plan_context_commands_steps.py:351`. ## Acceptance Criteria - [x] `shell=True` is replaced with `shell=False` in `cli_coverage_steps.py` - [x] `shlex.split()` is used to properly tokenize the command string - [x] All scenarios using the `I run shell command` step still pass - [x] No other `shell=True` usages remain in step files (audit) ## Supporting Information - Identified in: PR #727 Security Review, Finding #3 - Review URL: https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/727 - Reviewer: @hurui200320 - Reference implementation: `cli_plan_context_commands_steps.py:351` (already uses `shell=False`) ## Subtasks - [x] Replace `shell=True` with `shell=False` + `shlex.split()` in `cli_coverage_steps.py:44` - [x] Audit all step files for other `shell=True` usages - [x] Fix any additional `shell=True` instances found - [x] Run scenarios that use `I run shell command` step to verify correctness - [x] Run `nox -s unit_tests` to verify no regressions ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

CoreRasurae added the

Session	Result
lint	PASS
typecheck	PASS
unit_tests	PASS (11,455 scenarios)
integration_tests	PASS (1,600 tests)
coverage_report	PASS (97.0%)

Rows
Columns

fix(test): replace shell=True with shell=False in cli_coverage_steps.py subprocess call #734

Metadata

Summary

Background and Context

Current Behavior

Expected Behavior

Acceptance Criteria

Supporting Information

Subtasks

Definition of Done

Implementation Notes

Changes Made (commit 6077e921)

Design Decisions

Audit Results

Quality Gates

PR

Changes Made (commit `6077e921`)