Potential fix for code scanning alert no. 1: Shell command built from environment values - #150
Potential fix for code scanning alert no. 1: Shell command built from environment values#150athal7 wants to merge 1 commit into
Conversation
… environment values Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
WalkthroughAdded 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/core/devcontainer.js (1)
683-689: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new validation.
The existing
isContainerRunningtest covers only/nonexistent/workspace. Add cases for\0,\r,\n, and paths containing spaces. Assert that invalid values returnfalseand do not invoke Docker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49c5a24c-fe59-4e24-9ca4-42d0cd75e565
📒 Files selected for processing (1)
plugin/core/devcontainer.js
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.
| 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}`, |
There was a problem hiding this comment.
🔒 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:
- 1: https://docs.docker.com/reference/cli/docker/container/ls/
- 2: https://spacelift.io/blog/docker-ps
- 3: Unable to use docker ps --filter when a label value has a comma moby/moby#30575
- 4: https://docs.docker.com/engine/cli/filter/
- 5: https://docs.docker.com/reference/cli/docker/container/prune/
- 6: --filter does not work with negatives anywhere other than prune docker/cli#6021
- 7: https://man.archlinux.org/man/docker-container-ls.1.en.txt
🏁 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()}')
PYRepository: 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()}')
PYRepository: 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:
- 1: https://fuchsia.googlesource.com/third_party/github.com/docker/docker/+/49c3a7c4bac2877265ef8c4eaf210159560f08b4/api/types/filters/parse.go
- 2: Unable to use docker ps --filter when a label value has a comma moby/moby#30575
- 3: https://docs.docker.com/reference/cli/docker/image/ls/
- 4: https://docs.docker.com/engine/cli/filter/
- 5: https://docs.docker.com/reference/cli/docker/system/events/
🏁 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(),
})
PYRepository: 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.
1 participant
Potential fix for https://github.com/athal7/opencode-devcontainers/security/code-scanning/1
To fix this without changing functionality, sanitize/validate
workspacebefore embedding it into the Docker label filter so it cannot inject or corrupt filter syntax. The best minimal fix is to reject workspace values containing control characters (especially newline/NUL) and normalize to string before use. This keeps valid paths working (including spaces) while blocking dangerous parser-breaking characters and prevents uncontrolled argument semantics.Apply this in
plugin/core/devcontainer.jsinsideisContainerRunning(workspace), right before constructing therunCommandargs. Add a small helper (local function) to validate label values and use its sanitized output in the--filterargument. No new dependencies are required.Suggested fixes powered by Copilot Autofix. Review carefully before merging.