Potential fix for code scanning alert no. 1: Shell command built from environment values

plugin/core/devcontainer.js

@@ -18,6 +18,7 @@
 import { createClone, getClonePath, removeClone } from './clones.js'
 import { getCurrentBranch, getRepoRoot } from './git.js'
 import { startJob, updateJob, JOB_STATUS, removeJob } from './jobs.js'
+import { sanitizeDockerFilterValue } from './docker-filter.js'
 
 /**
  * Run a command and return a promise with the result
@@ -29,13 +30,13 @@
  * @returns {Promise<{stdout: string, stderr: string, exitCode: number, success: boolean}>}
  */
 async function runCommand(cmd, args, options = {}) {
   return new Promise((resolve, reject) => {
     const child = spawn(cmd, args, {
       stdio: ['ignore', 'pipe', 'pipe'],
       // Use SIGKILL for abort to ensure process termination even if SIGTERM is ignored
       // This is important for the devcontainer CLI which may spawn child processes
       killSignal: 'SIGKILL',
       ...options,
     })
 
     let stdout = ''
@@ -431,9 +432,10 @@
  */
 async function findContainerId(workspace, dockerPath = 'docker') {
   try {
+    const safeWorkspace = sanitizeDockerFilterValue(workspace)
     const result = await runCommand(dockerPath, [
       'ps', '-a',
-      '--filter', `label=devcontainer.local_folder=${workspace}`,
+      '--filter', `label=devcontainer.local_folder=${safeWorkspace}`,
       '--format', '{{.ID}}',
     ])
     if (result.success && result.stdout) {
@@ -684,12 +686,13 @@
  */
 export async function isContainerRunning(workspace) {
   try {
+    const safeWorkspace = sanitizeDockerFilterValue(workspace)
     const config = await loadUserConfig()
     const dockerPath = config.dockerPath || 'docker'
     // Look for container with devcontainer.local_folder label
     const result = await runCommand(dockerPath, [
       'ps',
-      '--filter', `label=devcontainer.local_folder=${workspace}`,
+      '--filter', `label=devcontainer.local_folder=${safeWorkspace}`,
       '--format', '{{.ID}}',
     ])
 

plugin/core/docker-filter.js

@@ -0,0 +1,27 @@
+/**
+ * Shared validation for values embedded in Docker `--filter` arguments
+ *
+ * Docker's `--filter label=key=value` syntax has no escaping mechanism for
+ * the value portion. Control characters (NUL, carriage return, line feed,
+ * ...) in the value could corrupt the filter or be misinterpreted by the
+ * Docker CLI, so any user-controlled value (e.g. a workspace path) must be
+ * validated before it is embedded in a `--filter` argument.
+ */
+
+/**
+ * Validate a value used in a Docker label filter expression.
+ *
+ * Rejects values containing control characters that could alter CLI
+ * parsing semantics (NUL, CR, LF, and other C0/DEL control characters).
+ * Valid workspace paths, including those containing spaces, are unaffected.
+ *
+ * @param {string} value - Value to validate (e.g. a workspace path)
+ * @returns {string} The validated value, unchanged
+ * @throws {Error} If value is not a string or contains control characters
+ */
+export function sanitizeDockerFilterValue(value) {
+  if (typeof value !== 'string' || /[\x00-\x1f\x7f]/.test(value)) {
+    throw new Error('Invalid value for docker filter: contains disallowed control characters')
+  }
+  return value
+}

plugin/core/ports.js

@@ -14,6 +14,7 @@ import { createServer } from 'net'
 import childProcess from 'child_process'
 import { PATHS } from './paths.js'
 import { loadUserConfig } from './config.js'
+import { sanitizeDockerFilterValue } from './docker-filter.js'
 
 /**
  * File-based locking using mkdir (atomic on all platforms)
@@ -244,13 +245,14 @@ async function runCommand(cmd, args) {
  */
 export async function getContainerPort(workspace) {
   try {
+    const safeWorkspace = sanitizeDockerFilterValue(workspace)
     const config = await loadUserConfig()
     const dockerPath = config.dockerPath || 'docker'
 
     // Find container with matching workspace label
     const result = await runCommand(dockerPath, [
       'ps',
-      '--filter', `label=devcontainer.local_folder=${workspace}`,
+      '--filter', `label=devcontainer.local_folder=${safeWorkspace}`,
       '--format', '{{.ID}}',
     ])
 

test/unit/devcontainer.test.js

@@ -209,6 +209,20 @@ describe('isContainerRunning', () => {
     const result = await isContainerRunning('/nonexistent/workspace')
     assert.strictEqual(typeof result, 'boolean')
   })
+
+  test('returns false instead of throwing for workspace containing a newline', async () => {
+    // Security regression test: a workspace value with an embedded newline
+    // could previously corrupt the Docker `--filter` argument (see code
+    // scanning alert #1 / GitHub issue #150). It must be rejected before
+    // reaching the Docker CLI, not passed through.
+    const result = await isContainerRunning('/workspace\n--filter label=foo=bar')
+    assert.strictEqual(result, false)
+  })
+
+  test('returns false instead of throwing for workspace containing a NUL byte', async () => {
+    const result = await isContainerRunning('/workspace\0injected')
+    assert.strictEqual(result, false)
+  })
 })
 
 // Integration-style tests (mock the devcontainer CLI)
@@ -671,4 +685,19 @@ describe('remove', () => {
     assert.strictEqual(second.cloneDeleted, false, 'no clone on second call')
     assert.strictEqual(second.errors.length, 0, 'no errors on repeat')
   })
+
+  test('handles workspace containing a newline without throwing (findContainerId sink)', async () => {
+    // Security regression test: remove() calls the internal findContainerId(),
+    // which also embeds workspace in a Docker `--filter` argument. A
+    // control character must be rejected before reaching the Docker CLI.
+    mkdirSync(testDir, { recursive: true })
+    writeFileSync(join(testDir, 'ports.json'), '{}')
+    writeFileSync(join(testDir, 'jobs.json'), '{}')
+
+    const maliciousWorkspace = '/workspace\n--filter label=foo=bar'
+    const summary = await remove(maliciousWorkspace, 'test', 'malicious')
+
+    assert.strictEqual(summary.containerFound, false)
+    assert.strictEqual(summary.errors.length, 0)
+  })
 })

test/unit/docker-filter.test.js

@@ -0,0 +1,49 @@
+/**
+ * Tests for plugin/core/docker-filter.js
+ *
+ * Run with: node --test test/unit/docker-filter.test.js
+ */
+
+import { test, describe } from 'node:test'
+import assert from 'node:assert'
+
+// Module under test
+import { sanitizeDockerFilterValue } from '../../plugin/core/docker-filter.js'
+
+describe('sanitizeDockerFilterValue', () => {
+  test('returns a plain absolute path unchanged', () => {
+    assert.strictEqual(
+      sanitizeDockerFilterValue('/Users/dev/my-project'),
+      '/Users/dev/my-project'
+    )
+  })
+
+  test('accepts a path containing spaces', () => {
+    assert.strictEqual(
+      sanitizeDockerFilterValue('/Users/dev/my project (copy)'),
+      '/Users/dev/my project (copy)'
+    )
+  })
+
+  test('rejects a value containing a newline', () => {
+    assert.throws(() => sanitizeDockerFilterValue('/workspace\n--filter label=foo=bar'))
+  })
+
+  test('rejects a value containing a carriage return', () => {
+    assert.throws(() => sanitizeDockerFilterValue('/workspace\rinjected'))
+  })
+
+  test('rejects a value containing a NUL byte', () => {
+    assert.throws(() => sanitizeDockerFilterValue('/workspace\0injected'))
+  })
+
+  test('rejects other C0 control characters', () => {
+    assert.throws(() => sanitizeDockerFilterValue('/workspace\x1binjected'))
+  })
+
+  test('rejects a non-string value', () => {
+    assert.throws(() => sanitizeDockerFilterValue(undefined))
+    assert.throws(() => sanitizeDockerFilterValue(null))
+    assert.throws(() => sanitizeDockerFilterValue(42))
+  })
+})

test/unit/ports.test.js

@@ -311,6 +311,36 @@ describe('getContainerPort', () => {
     // Should not throw, just return null
     assert.strictEqual(port, null)
   })
+
+  test('returns null and never invokes docker for a workspace with a newline', async (t) => {
+    // Security regression test: a workspace value with an embedded newline
+    // could previously corrupt the Docker `--filter` argument (code
+    // scanning alert #1 / issue #150). It must be rejected before spawning
+    // docker, not passed through.
+    let spawned = false
+    t.mock.method(childProcess, 'spawn', () => {
+      spawned = true
+      throw new Error('spawn should not be called for an invalid workspace')
+    })
+
+    const port = await getContainerPort('/workspace\n--filter label=foo=bar')
+
+    assert.strictEqual(port, null)
+    assert.strictEqual(spawned, false)
+  })
+
+  test('returns null and never invokes docker for a workspace with a NUL byte', async (t) => {
+    let spawned = false
+    t.mock.method(childProcess, 'spawn', () => {
+      spawned = true
+      throw new Error('spawn should not be called for an invalid workspace')
+    })
+
+    const port = await getContainerPort('/workspace\0injected')
+
+    assert.strictEqual(port, null)
+    assert.strictEqual(spawned, false)
+  })
 })
 
 describe('getContainerPort with a configured runtime', () => {