[WRONG BRANCH] fix(config): avoid untrusted process lookup in PID validation

[luvs01]

Motivation

• Prevent a local PATH/working-directory hijack where PID validation executed external tools by unqualified name, which could run attacker-controlled binaries during common CLI operations.

Description

• Replace unqualified external lookups in the PID command-line probe with native or absolute-path probes by updating readProcessCommandLine in src/config.ts.
• On Linux, read the kernel-provided /proc/<pid>/cmdline instead of spawning ps.
• On Windows, prefer WMIC via an absolute SystemRoot\System32\wbem\WMIC.exe path and fall back to PowerShell at SystemRoot\System32\WindowsPowerShell\v1.0\powershell.exe rather than invoking powershell.exe by name.
• On other Unix platforms, call /bin/ps by absolute path instead of ps to avoid PATH resolution.
• Add a regression test to tests/config.test.ts that injects a fake ps earlier in PATH and asserts readPid() does not execute it, and import readPid for the test.

Testing

• Ran bun test tests/config.test.ts; the focused suite passed (117 tests passed).
• Ran bun run typecheck (bun x tsc --noEmit) which succeeded.
• Ran bun run privacy:scan which succeeded.
• Ran full bun run test and observed unrelated pre-existing failures in tests/server-management-auth.test.ts (HTTP expectations returned 403); the failures are not introduced by this focused change.

---

Codex Task

Summary by CodeRabbit

• Bug Fixes
  • Improved process command detection across Linux, Windows, and other supported platforms.
  • Prevented unintended executables found through the system PATH from being used during process identification.
  • Preserved existing fallback behavior and error handling.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[github-actions]

⚠️ Wrong target branch

This pull request currently targets main, but pull requests must target one of dev.

@luvs01 Please retarget this PR to dev. All contributions go to dev; main receives only release promotions. See our Contributing guide for details. Thanks! 🙏

Its title has been prefixed with [WRONG BRANCH].

This pull request is being kept as a draft automatically. Once every issue above is resolved, it will be marked ready for review again.

[coderabbitai]

📝 Walkthrough

Walkthrough

readProcessCommandLine now uses explicit system paths for Linux, Windows, and other platforms. A Linux regression test verifies that PID validation does not execute a PATH-injected ps binary.

Changes

Process lookup hardening

Layer / File(s)	Summary
Platform-specific executable resolution src/config.ts	At lines 2835–2845, Linux uses /proc/<pid>/cmdline, and Windows derives executable paths from a validated SystemRoot. Lines 2856–2857 use the resolved PowerShell path. Line 2868 invokes /bin/ps explicitly.
PATH injection regression coverage tests/config.test.ts	The test import at line 21 enables readPid coverage. Lines 1707–1725 verify that a malicious PATH entry does not execute during Linux PID validation and that PATH is restored.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• luvs01/opencodex#62: Hardens process scanning with absolute ps and PowerShell executable paths.

Suggested labels: bug

Suggested reviewers: lidge-jun, wibias

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the configuration fix that prevents untrusted process lookups during PID validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/propose-fix-for-pid-validation-vulnerability

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/config.test.ts`:
- Around line 1713-1717: Ensure the PATH mutation in the test setup is always
covered by the existing try/finally cleanup: move writePid(process.pid) before
assigning process.env.PATH, or move the PATH assignment and writePid call
together inside the try block. Preserve restoration of the previous PATH on
every setup failure path.

🪄 Autofix

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: ASSERTIVE

Plan: Pro Plus

Run ID: c2701915-a897-44a7-99db-38f1d1ac2ee7

📥 Commits

Reviewing files that changed from the base of the PR and between 8a9c0ef and e812b56.

📒 Files selected for processing (2)

• src/config.ts
• tests/config.test.ts

[coderabbitai]

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Keep PATH cleanup around all setup that follows the mutation.

Line [1714] changes the process-wide PATH, but Line [1715] calls writePid before the try at Line [1717]. If writePid throws, the finally block does not run and later Bun tests inherit the attacker directory in PATH. Move writePid before the PATH assignment or place both operations inside the try.

Proposed fix

 const previousPath = process.env.PATH;
-process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
-writePid(process.pid);
-
 try {
+  process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
+  writePid(process.pid);
   expect(readPid()).toBeNull();

As per path instructions, tests are flat Bun tests under tests/, so process-wide PATH state must be restored on every setup path.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const previousPath = process.env.PATH;
	process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
	writePid(process.pid);
	try {
	const previousPath = process.env.PATH;
	try {
	process.env.PATH = `${attackerDir}:${previousPath ?? ""}`;
	writePid(process.pid);

🤖 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/config.test.ts` around lines 1713 - 1717, Ensure the PATH mutation in
the test setup is always covered by the existing try/finally cleanup: move
writePid(process.pid) before assigning process.env.PATH, or move the PATH
assignment and writePid call together inside the try block. Preserve restoration
of the previous PATH on every setup failure path.

Source: Path instructions