Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
athal7 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion plugin/core/devcontainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@
*/
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,
})

Check warning

Code scanning / CodeQL

Shell command built from environment values Medium

This shell command depends on an uncontrolled
file name
Loading
.
This shell command depends on an uncontrolled
file name
Loading
.
This shell command depends on an uncontrolled
file name
Loading
.
This shell command depends on an uncontrolled
file name
Loading
.
This shell command depends on an uncontrolled
file name
Loading
.

let stdout = ''
let stderr = ''
Expand Down Expand Up @@ -673,6 +673,21 @@
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
*
Expand All @@ -686,10 +701,11 @@
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}`,
Comment on lines +704 to +708

@coderabbitai coderabbitai Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 6 '\bfindContainerId\s*\(' --glob '*.js' .
rg -n -C 4 'devcontainer\.local_folder=' --glob '*.js' .

Repository: athal7/opencode-devcontainers

Length of output: 4486

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- sanitizer and relevant functions ---'
sed -n '1,90p' plugin/core/devcontainer.js
sed -n '410,455p' plugin/core/devcontainer.js
sed -n '680,725p' plugin/core/devcontainer.js

printf '%s\n' '--- all workspace-label filter sinks ---'
rg -n -C 8 'devcontainer\.local_folder=|sanitizeDockerFilterValue|runCommand\s*\(' plugin test --glob '*.js'

printf '%s\n' '--- exports and callers ---'
rg -n -C 6 'module\.exports|findContainerId|isContainerRunning|remove\s*\(' plugin/core/devcontainer.js plugin test --glob '*.js'

Repository: athal7/opencode-devcontainers

Length of output: 49997

🌐 Web query:

Docker CLI ps --filter label exact value filter syntax special characters comma newline

💡 Result:

The Docker CLI ps filter syntax for labels is designed to match containers based on the presence of a label key or a specific key-value pair [1]. Syntax and Usage: - Existence check: Use --filter "label=key" to match any container that has the specified label, regardless of its value [1]. - Exact match: Use --filter "label=key=value" to match containers where the label key has the exact value value [1][2]. Handling Special Characters: - Commas: Historically, commas in label values could cause parsing errors with the --filter flag [3]. While improvements in later versions of the Docker CLI have mitigated many such issues, it is recommended to wrap the entire filter string in quotes [1][4]. - Newlines: Newline characters are generally not supported within the label values when passed via the CLI --filter flag because the flag expects a standard string format [4]. - General Best Practice: To ensure special characters are handled correctly, always enclose the entire filter argument in single or double quotes, such as --filter "label=my.key=some,value". Filter Operators: - Equality: The = operator is used for standard matching [4]. - Inequality: Some commands support the != operator (e.g., label!=key or label!=key=value) to exclude containers with specific labels [4][5]. However, support for != can vary across different Docker commands and versions; while it is well-supported in prune [5], its use in ps has historically been inconsistent across different versions [6]. If you encounter issues with complex strings or special characters, verify your Docker version and consider using --format combined with standard shell tools (like grep or jq) if the native --filter syntax remains restrictive [7].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- workspace resolution and persistence ---'
rg -n -C 8 'workspace\s*[:=]|saveSession|loadSession|resolve.*workspace|resolved\s*=' plugin --glob '*.js'

printf '%s\n' '--- ports label-filter callers ---'
rg -n -C 10 'getContainerPort\s*\(' plugin test --glob '*.js'

printf '%s\n' '--- complete label-filter inventory ---'
python3 - <<'PY'
from pathlib import Path
import re

pattern = re.compile(r'(?P<line>.*devcontainer\.local_folder=.*)')
for path in map(Path, ['plugin']):
    for file in path.rglob('*.js'):
        for number, line in enumerate(file.read_text(errors='replace').splitlines(), 1):
            if 'devcontainer.local_folder=' in line:
                print(f'{file}:{number}: {line.strip()}')
PY

Repository: athal7/opencode-devcontainers

Length of output: 50385

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
import re

files = [Path('plugin/core/devcontainer.js'), Path('plugin/core/ports.js')]
for file in files:
    text = file.read_text()
    for number, line in enumerate(text.splitlines(), 1):
        if 'devcontainer.local_folder=' in line:
            uses_sanitizer = 'sanitizeDockerFilterValue' in line
            print(f'{file}:{number}: sanitizer_on_same_line={uses_sanitizer}: {line.strip()}')
PY

Repository: athal7/opencode-devcontainers

Length of output: 533

🌐 Web query:

Moby Docker CLI label filter comma parsing source filters label key value

💡 Result:

In the Docker CLI, the --filter flag for labels does not support comma-separated parsing for label values, as the parser interprets the equals sign (=) as the delimiter between the filter key and the filter value [1]. Technical Details and Constraints: 1. Parsing Logic: The Docker CLI filter parser splits the argument on the first equals sign encountered using strings.SplitN(arg, "=", 2) [1]. For a filter provided as --filter "label=mykey=myvalue", the parser identifies "label" as the field name and "mykey=myvalue" as the value to match [1]. 2. Comma Interpretation: If a label value contains a comma (e.g., "value1,value2"), the CLI treats the entire string (including the comma) as part of the value [2]. Historically, early versions of the Docker CLI had bugs where commas in filter values could cause parsing errors [2], but modern versions correctly handle them as literal characters within the value [2]. 3. Multiple Labels: To filter by multiple labels or multiple label values, you must provide the --filter flag multiple times [3][4]. - Logical OR: Using the same filter key multiple times (e.g., --filter "label=key1=val1" --filter "label=key1=val2") acts as a logical OR in some contexts like event filtering [5], though behavior varies by specific command implementation. - Logical AND: Combining different filter keys acts as a logical AND [4]. 4. Recommended Approach: If you need to match a label value that includes a comma, ensure your filter string correctly defines the key and value. For example, to match a label "env" with the value "prod,internal", use: docker ps --filter "label=env=prod,internal" The key point is that the CLI does not treat commas as list delimiters; it treats them as literal characters within the value component of the key-value pair [2].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- persisted port keys and getContainerPort path ---'
sed -n '1,180p' plugin/core/ports.js
sed -n '235,275p' plugin/core/ports.js
sed -n '635,655p' plugin/core/devcontainer.js

printf '%s\n' '--- workspace resolver and status tests ---'
sed -n '115,185p' plugin/helpers.js
sed -n '200,215p' test/unit/devcontainer.test.js
rg -n -C 5 'isContainerRunning|getContainerPort|local_folder|Invalid workspace value' test --glob '*.js'

printf '%s\n' '--- deterministic sink audit ---'
python3 - <<'PY'
from pathlib import Path

for file in Path('plugin').rglob('*.js'):
    for number, line in enumerate(file.read_text(errors='replace').splitlines(), 1):
        if 'devcontainer.local_folder=' not in line:
            continue
        before = '\n'.join(file.read_text(errors='replace').splitlines()[max(0, number-8):number])
        print({
            'file': str(file),
            'line': number,
            'uses_safe_workspace': 'safeWorkspace' in line,
            'sanitizer_in_preceding_context': 'sanitizeDockerFilterValue' in before,
            'code': line.strip(),
        })
PY

Repository: athal7/opencode-devcontainers

Length of output: 17562

Sanitize every workspace value used in a Docker label filter.

findContainerId and getContainerPort still interpolate workspace without sanitizeDockerFilterValue. Move the helper to a shared module and apply it at both sinks. Add regression tests for rejected control characters.

'--format', '{{.ID}}',
])

Expand Down