🛡️ Sentinel: [CRITICAL] Fix command injection in unix system utilities

.jules/sentinel.md

@@ -1,9 +1,4 @@
-## 2025-05-13 - [SQL Injection Risk via sql.raw in Bulk Insert Retrieval]
-**Vulnerability:** A potential SQL injection vulnerability in `createMany` inside `packages/core/src/db/repositories/tasks.ts`. The code used `sql.raw` to interpolate task IDs into an `IN` clause manually without parameterization.
-**Learning:** Using `sql.raw` to join dynamically generated strings (like UUID arrays) bypasses Drizzle's parameterization, exposing the app to SQL injection if any ID string is manipulated.
-**Prevention:** Always use Drizzle ORM's built-in parameterization functions like `inArray` instead of manual string concatenation or `sql.raw`.
-
-## 2024-05-21 - [CRITICAL] Fix config.yaml file permissions
-**Vulnerability:** The daemon configuration file (`~/.agor/config.yaml`) and its parent directory (`~/.agor`) were created with default file permissions (e.g., `0o755`/`0o644`), which made them readable by other users on the system. This file stores extremely sensitive information such as API keys and master JWT secrets.
-**Learning:** Default Node.js filesystem operations (`fs.writeFile` and `fs.mkdir`) do not enforce strict permissions unless explicitly specified with a `mode` parameter. When handling sensitive files, relying on the system `umask` is insufficient.
-**Prevention:** Always specify `mode: 0o600` for sensitive files and `mode: 0o700` for their parent directories. Additionally, use `fs.chmod` to retroactively secure existing files and directories that might have been created with permissive defaults.
+## 2025-02-26 - Command Injection via execSync with String Interpolation
+**Vulnerability:** Shell command injection vulnerability in ID lookup and user existence checks (e.g., `execSync(\`id "${username}" > /dev/null 2>&1\`)`). Malicious usernames could escape quotes and execute arbitrary shell commands.
+**Learning:** `execSync` executes commands via `/bin/sh` which parses metacharacters. Even if quoted, inputs can sometimes break out depending on the shell or if the input isn't fully validated elsewhere.
+**Prevention:** Replace `execSync` with `execFileSync` (or `execFile` for async) when executing static binaries with dynamic arguments. Pass dynamic inputs explicitly via the `args` array preceded by `--` to prevent argument injection, and strictly avoid string interpolation. E.g., `execFileSync('id', ['--', String(username)], { stdio: 'ignore' })`.

packages/core/src/unix/id-lookups.ts

@@ -4,7 +4,7 @@
  * Supports both Linux (using getent) and macOS (parsing /etc/group and /etc/passwd)
  */
 
-import { execSync } from 'node:child_process';
+import { execFileSync } from 'node:child_process';
 import fs from 'node:fs';
 
 /**
@@ -26,7 +26,7 @@ export function getGidFromGroupName(groupName: string | undefined | null): numbe
   try {
     // Try getent first (Linux, some BSD)
     try {
-      const result = execSync(`getent group "${groupName}"`, {
+      const result = execFileSync('getent', ['group', '--', String(groupName)], {
         encoding: 'utf-8',
         stdio: 'pipe',
         timeout: 2000,
@@ -91,7 +91,7 @@ export function getUidFromUsername(username: string | undefined | null): number
   try {
     // Try `id -u username` first (most reliable)
     try {
-      const result = execSync(`id -u "${username}"`, {
+      const result = execFileSync('id', ['-u', '--', String(username)], {
         encoding: 'utf-8',
         stdio: 'pipe',
         timeout: 2000,
@@ -107,7 +107,7 @@ export function getUidFromUsername(username: string | undefined | null): number
 
     // Try getent (Linux, some BSD)
     try {
-      const result = execSync(`getent passwd "${username}"`, {
+      const result = execFileSync('getent', ['passwd', '--', String(username)], {
         encoding: 'utf-8',
         stdio: 'pipe',
         timeout: 2000,
@@ -171,7 +171,7 @@ export function getHomedirFromUsername(username: string | undefined | null): str
   try {
     // Try getent first (Linux, some BSD)
     try {
-      const result = execSync(`getent passwd "${username}"`, {
+      const result = execFileSync('getent', ['passwd', '--', String(username)], {
         encoding: 'utf-8',
         stdio: 'pipe',
         timeout: 2000,

packages/core/src/unix/user-manager.test.ts

@@ -27,14 +27,14 @@ import {
   validateResolvedUnixUser,
 } from './user-manager.js';
 
-// Mock execSync for system-dependent tests
+// Mock execFileSync for system-dependent tests
 vi.mock('node:child_process', () => ({
-  execSync: vi.fn(),
+  execFileSync: vi.fn(),
 }));
 
-import { execSync } from 'node:child_process';
+import { execFileSync } from 'node:child_process';
 
-const mockedExecSync = vi.mocked(execSync);
+const mockedExecFileSync = vi.mocked(execFileSync);
 
 describe('user-manager', () => {
   beforeEach(() => {
@@ -312,15 +312,15 @@ describe('user-manager', () => {
     // Note: These tests use mocked execSync since we can't create real users
 
     it('returns true when user exists', async () => {
-      mockedExecSync.mockReturnValueOnce(Buffer.from('')); // Success (no throw)
+      mockedExecFileSync.mockReturnValueOnce(Buffer.from('')); // Success
 
       // Import fresh to use mock
       const { unixUserExists } = await import('./user-manager.js');
       expect(unixUserExists('root')).toBe(true);
     });
 
     it('returns false when user does not exist', async () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('id: nonexistent: no such user');
       });
 
@@ -456,7 +456,7 @@ describe('user-manager', () => {
     });
 
     it('validates user exists for strict mode', () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('no such user');
       });
 
@@ -466,7 +466,7 @@ describe('user-manager', () => {
     });
 
     it('validates user exists for insulated mode', () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('no such user');
       });
 
@@ -482,13 +482,13 @@ describe('user-manager', () => {
     });
 
     it('passes when user exists', () => {
-      mockedExecSync.mockReturnValueOnce(Buffer.from('')); // user exists
+      mockedExecFileSync.mockReturnValueOnce(Buffer.from('')); // user exists
 
       expect(() => validateResolvedUnixUser('strict', 'alice')).not.toThrow();
     });
 
     it('error message includes mode context', () => {
-      mockedExecSync.mockImplementationOnce(() => {
+      mockedExecFileSync.mockImplementationOnce(() => {
         throw new Error('no such user');
       });
 

packages/core/src/unix/user-manager.ts

@@ -7,7 +7,7 @@
  * @see context/guides/rbac-and-unix-isolation.md
  */
 
-import { execSync } from 'node:child_process';
+import { execFileSync } from 'node:child_process';
 import type { UnixUserMode } from '../config/types.js';
 import { formatShortId } from '../lib/ids.js';
 import type { UserID, UUID } from '../types/index.js';
@@ -339,7 +339,7 @@ export class UnixUserNotFoundError extends Error {
  */
 export function unixUserExists(username: string): boolean {
   try {
-    execSync(`id "${username}" > /dev/null 2>&1`, { stdio: 'pipe' });
+    execFileSync('id', ['--', String(username)], { stdio: 'ignore' });
     return true;
   } catch {
     return false;