From f7bac616c44671a49b6984dfb72bb4a5d1c1ca70 Mon Sep 17 00:00:00 2001 From: Andrew Thal <467872+athal7@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:06:41 -0500 Subject: [PATCH 1/2] Potential fix for code scanning alert no. 1: Shell command built from environment values Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- plugin/core/devcontainer.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/plugin/core/devcontainer.js b/plugin/core/devcontainer.js index e089bb5..d0312fe 100644 --- a/plugin/core/devcontainer.js +++ b/plugin/core/devcontainer.js @@ -673,6 +673,21 @@ export async function list(options = {}) { return results } +/** + * Validate a value used in docker label filter expressions. + * Reject control characters that can alter CLI parsing semantics. + * + * @param {string} value + * @returns {string} + */ +function sanitizeDockerFilterValue(value) { + const normalized = String(value) + if (/[\0\r\n]/.test(normalized)) { + throw new Error('Invalid workspace value for docker filter') + } + return normalized +} + /** * Check if a container is running for a workspace * @@ -686,10 +701,11 @@ export async function isContainerRunning(workspace) { try { const config = await loadUserConfig() const dockerPath = config.dockerPath || 'docker' + const safeWorkspace = sanitizeDockerFilterValue(workspace) // 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}}', ]) From aa919c85d9ffc9252b444f2ff775a11022503110 Mon Sep 17 00:00:00 2001 From: Andrew Thal <467872+athal7@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:23:18 -0500 Subject: [PATCH 2/2] fix(devcontainer): sanitize workspace at every docker filter sink CodeRabbit flagged that the autofix only sanitized isContainerRunning, leaving findContainerId (devcontainer.js) and getContainerPort (ports.js) still vulnerable to filter-syntax injection via control characters in the workspace path. - Move sanitizeDockerFilterValue into a shared plugin/core/docker-filter.js module and apply it at all three --filter sinks. - Validate before loadUserConfig() runs, so an invalid workspace fails fast without doing unrelated config/auto-detect work. - Add regression tests: direct unit tests for the shared validator, and per-sink tests proving malicious workspace values (newline, NUL) are rejected without ever invoking the Docker CLI. Co-Authored-By: anthropic/claude-sonnet-5 --- plugin/core/devcontainer.js | 21 +++----------- plugin/core/docker-filter.js | 27 ++++++++++++++++++ plugin/core/ports.js | 4 ++- test/unit/devcontainer.test.js | 29 +++++++++++++++++++ test/unit/docker-filter.test.js | 49 +++++++++++++++++++++++++++++++++ test/unit/ports.test.js | 30 ++++++++++++++++++++ 6 files changed, 142 insertions(+), 18 deletions(-) create mode 100644 plugin/core/docker-filter.js create mode 100644 test/unit/docker-filter.test.js diff --git a/plugin/core/devcontainer.js b/plugin/core/devcontainer.js index d0312fe..2866d13 100644 --- a/plugin/core/devcontainer.js +++ b/plugin/core/devcontainer.js @@ -18,6 +18,7 @@ import { generateOverrideConfig, getOverridePath, loadUserConfig } from './confi 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 @@ -431,9 +432,10 @@ export async function exec(workspace, command, options = {}) { */ 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) { @@ -673,21 +675,6 @@ export async function list(options = {}) { return results } -/** - * Validate a value used in docker label filter expressions. - * Reject control characters that can alter CLI parsing semantics. - * - * @param {string} value - * @returns {string} - */ -function sanitizeDockerFilterValue(value) { - const normalized = String(value) - if (/[\0\r\n]/.test(normalized)) { - throw new Error('Invalid workspace value for docker filter') - } - return normalized -} - /** * Check if a container is running for a workspace * @@ -699,9 +686,9 @@ function sanitizeDockerFilterValue(value) { */ export async function isContainerRunning(workspace) { try { + const safeWorkspace = sanitizeDockerFilterValue(workspace) const config = await loadUserConfig() const dockerPath = config.dockerPath || 'docker' - const safeWorkspace = sanitizeDockerFilterValue(workspace) // Look for container with devcontainer.local_folder label const result = await runCommand(dockerPath, [ 'ps', diff --git a/plugin/core/docker-filter.js b/plugin/core/docker-filter.js new file mode 100644 index 0000000..43961cb --- /dev/null +++ b/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 +} diff --git a/plugin/core/ports.js b/plugin/core/ports.js index c48bcaa..5e207e6 100644 --- a/plugin/core/ports.js +++ b/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}}', ]) diff --git a/test/unit/devcontainer.test.js b/test/unit/devcontainer.test.js index 9b1927f..cd12ca7 100644 --- a/test/unit/devcontainer.test.js +++ b/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) + }) }) diff --git a/test/unit/docker-filter.test.js b/test/unit/docker-filter.test.js new file mode 100644 index 0000000..c79b56d --- /dev/null +++ b/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)) + }) +}) diff --git a/test/unit/ports.test.js b/test/unit/ports.test.js index 48cb8e6..e5a266c 100644 --- a/test/unit/ports.test.js +++ b/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', () => {