Repository navigation
Conversation
Reviewer's GuideImplementationPlace Windows-managed Python environments under the configured temp directory and build their executable paths from that directory, while preserving the fixed POSIX location.
Resolve Windows commands before dispatch and invoke native executables literally while escaping batch-wrapper inputs, including npm shim arguments.
TestsVerify Windows venv placement and executable paths, and confirm POSIX environments retain their fixed
Exercise Windows command resolution, native argument handling, batch escaping, and unchanged POSIX invocation at the subprocess boundary.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/providers/python_controller.js" line_range="112" />
<code_context>
+ // drive-aware even when the Windows branch runs on another host (tests).
+ const venvPath = isWindows ? path.win32 : path.posix
+ this.pythonEnvDir = isWindows
+ ? venvPath.join(os.tmpdir(), "trustify_da_env_js")
+ : venvPath.join("/", "tmp", "trustify_da_env_js")
try {
</code_context>
<issue_to_address>
**Temp paths execute shell commands**
When the configured Windows temp path contains an attacker-controlled shell metacharacter such as `&` but no spaces, `invokeCommand` passes `pythonEnvDir` through `shell: true`, and `handleSpacesInPath` leaves the path unquoted, so `cmd.exe` treats `&` as a command separator and runs the injected command; venv creation or the pip upgrade fails and the analysis process runs attacker-controlled commands.
Quote or escape Windows paths containing shell metacharacters before passing them to the shell, or avoid invoking commands through a shell.
</issue_to_address>
### Comment 2
<location path="src/providers/python_controller.js" line_range="112" />
<code_context>
+ // drive-aware even when the Windows branch runs on another host (tests).
+ const venvPath = isWindows ? path.win32 : path.posix
+ this.pythonEnvDir = isWindows
+ ? venvPath.join(os.tmpdir(), "trustify_da_env_js")
+ : venvPath.join("/", "tmp", "trustify_da_env_js")
try {
</code_context>
<issue_to_address>
**Shared temp venv can be preseeded**
When a Windows process uses a temp directory writable by an untrusted local user, who pre-creates the venv path with a malicious startup module, `pythonEnvDir` is a predictable child of the shared temp root, and `prepareEnvironment()` runs `python -m venv` there before executing the venv interpreter for the pip upgrade. Python reuses existing venv contents, so a planted `sitecustomize.py` can execute with the analysis process's privileges.
Create the venv in a securely created private directory and verify that it is owned by the process and is not a reparse point before invoking Python.
</issue_to_address>
### Comment 3
<location path="src/providers/python_controller.js" line_range="112" />
<code_context>
+ // drive-aware even when the Windows branch runs on another host (tests).
+ const venvPath = isWindows ? path.win32 : path.posix
+ this.pythonEnvDir = isWindows
+ ? venvPath.join(os.tmpdir(), "trustify_da_env_js")
+ : venvPath.join("/", "tmp", "trustify_da_env_js")
try {
</code_context>
<issue_to_address>
**Overlapping scans corrupt shared environments**
When two Windows analyses using the same configured temp directory overlap, both controllers run `python -m venv` and install dependencies into the same `pythonEnvDir`; one setup can disrupt the other's environment, and `getDependencies()` can read the other scan's packages after yielding, so setup fails or the scan returns an incorrect SBOM.
Give each analysis its own isolated virtual-environment directory, or coordinate access so concurrent scans cannot modify or read the same environment.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: src/providers/python_controller.js:112, src/providers/python_controller.js:112, src/providers/python_controller.js:112
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #696 +/- ##
==========================================
+ Coverage 91.60% 91.68% +0.07%
==========================================
Files 44 44
Lines 11638 11700 +62
Branches 2346 2371 +25
==========================================
+ Hits 10661 10727 +66
+ Misses 977 973 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
prepareEnvironment() built the managed venv at path.join(path.sep, "tmp", "trustify_da_env_js"), which becomes a root-relative \tmp path on Windows, then re-anchored the interpreter and pip paths to the current drive root from process.cwd(). Use os.tmpdir() on Windows so the venv lives in the OS-configured temp directory, and join Scripts\python.exe and Scripts\pip.exe from that complete venv directory. The venv creation and pip upgrade now share one directory even when TEMP and the working directory are on different drives. POSIX keeps the fixed /tmp/trustify_da_env_js path with the existing reuse and pip uninstall behavior. Adds one failing-first Windows placement test backed by an explicit path.win32 seam; Linux path preservation is covered by the existing POSIX tests running with a non-default temp dir. Native Windows execution remains unverified on this POSIX host. Implements TC-7494 Signed-off-by: Adva Oren <aoren@redhat.com> Assisted-by: Claude Code
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — all three items in review 5879037787 are classified as code change requests; their validity and scope are assessed separately from the language classification.
Individual inline threads contain the corresponding reasoning. These dispositions do not assert that the broader lifecycle risks are fixed. |
Verification Report for TC-7494 (commit b6d2f58)Reviewed HEAD:
Overall: FAILRequired fix
This is a valid-path portability failure and a command-injection exposure when the configured path is attacker-influenced. The unsafe helper predates the PR, but exposing configurable TEMP content to it is new. No injected command was executed; native Windows/cmd.exe execution remains unverified. No remote exploitability or privilege escalation is claimed. See Node's shell-input warning. TC-7561 tracks a narrow command-boundary fix on this existing PR, preserving supported batch-wrapper behavior and the fixed venv lifecycle. It is a standalone Task, as authorized, because TC-7494 is already a Sub-task and Jira does not nest sub-tasks. Inline Other review dispositions
Verification detail and limitations
Root causeThe supportable preventive gap is an implement-task analysis method: trace newly sourced input through unchanged downstream helpers and verify literal data remains literal at parsing/execution boundaries. The task's helper-mocked placement tests verify path values, not the actual subprocess boundary. No parent Feature or Feature-to-task omission was evidenced, so no define-feature/plan-feature attribution is asserted; historical skill invocation/responsibility is unproven. Existing TC-5951 already covers this downstream re-parsing method, including shell and format-active input verification. It was reused, linked Related to TC-7494, and received supporting root-cause analysis. No duplicate preventive Task was created or branch retargeted. Node/Windows API facts stay out of general-purpose skill rules. TC-7504 remains separate relational-oracle work. Evidence retained locally in Code unchanged; no merge or Jira status transition performed. TC-7494 remains in Review. Test Quality, Test Change Classification, and Root-Cause Investigation are informational and excluded from the Overall calculation. This comment was AI-generated by sdlc-workflow/verify-pr v0.13.10. |
Invoke native executables without a shell so temp paths cannot become cmd syntax. Resolve PATH/PATHEXT wrappers before escaping batch inputs and npm shim reparsing. Preserve POSIX invocation and Python lifecycle. Implements TC-7561 Signed-off-by: Adva Oren <aoren@redhat.com> Assisted-by: Claude Code
Summary
Create the controller-managed Python venv under
os.tmpdir()on Windows instead of a root-relative\tmppath on the current drive.Build
Scripts\python.exeandScripts\pip.exefrom the complete venv directory, so venv creation and the pip upgrade agree even when TEMP and the working directory are on different drives.Keep Linux's fixed
/tmp/trustify_da_env_jspath, executable names, and reuse/uninstall behavior unchanged.TC-7561: keep Windows command paths literal at the subprocess boundary (
0724cb7), addressing review discussion 4553943998.&as both venv targets and interpreter paths..bat/.cmdsupport via explicit escapedcmd.exeinvocation; resolve PATH/PATHEXT first, including explicit.cmdnames, so npm.binshims receive the required reparse escaping. Reject CR/LF in batch inputs before spawning.npm test1,063 passing / 15 failing, compared with unmodified PR HEAD 1,050 passing / 15 failing. All 15 failure names, primary errors, and causes match: 3 OCI fixture assertions, 2 broken-local-pipdeptree cases, and 10 missing-Poetry cases. Proceeding with these matching failures was explicitly approved.cmd.exe/wrapper execution. Python placement, executable names, fixed venv name, and lifecycle remain unchanged.Tests
path.win32seam (TEMP onD:), asserting creation and pip upgrade use<TEMP>\trustify_da_env_js\Scripts\python.exe.python3->python.exenaming; POSIX tests now run with a non-default temp dir to prove/tmpremains fixed.npm run tests -- --grep "Python_controller"- 13 passing.npm test- 1052 passing; the 15 failures are pre-existing environment issues that fail identically on unmodifiedmain(missing poetry, brokenpipdeptreeinstall, OCI image fixture drift).npm run lint- 0 errors, same 15 pre-existing warnings asmain.Native Windows execution was not available in this environment and remains unverified.
Implements TC-7494
Summary by Sourcery
Correct Windows Python environment placement and make cross-platform command invocation reliable and safe.
Bug Fixes:
Enhancements:
Tests: