Skip to content

Commit 9ec1377

Browse files
AnExiledDevAnExiledDev
committed
Fix critical security findings from codebase review
- Remove env var injection in agent redirect log path (S2-01) - Protected files guards fail closed on unexpected errors (S2-04) - Harden dangerous command regexes: prefix stripping, symbolic chmod, setuid, docker system/volume, git filter-branch, plus-refspec (S2-03) - Unify write-target patterns across guards (5 → 18 patterns) (C1-02) - Fix greedy alternation in redirect regex (>>|> order) (Q3-01) - Block bare git stash in read-only mode (Q3-04) - Narrow configApply allowed destinations to /usr/local/share (S2-09) - Add pytest to CI pipeline (Q3-08) - Add 36 new test cases covering all fixes (241 → 277 tests)
1 parent 5df63d7 commit 9ec1377

13 files changed

Lines changed: 441 additions & 61 deletions

File tree

‎.devcontainer/CHANGELOG.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
## [Unreleased]
44

5+
### Security
6+
- Removed environment variable injection vector in agent redirect log path (S2-01)
7+
- Narrowed config deployment allowed destinations from `/usr/local` to `/usr/local/share` (S2-09)
8+
59
### Added
610

711
#### Testing
@@ -13,6 +17,7 @@
1317
- `guard-readonly-bash.py` (63 tests) — general-readonly and git-readonly modes, bypass prevention
1418
- `redirect-builtin-agents.py` (13 tests) — redirect mapping, passthrough, output structure
1519
- Added `test:plugins` and `test:all` npm scripts for running plugin tests
20+
- Python plugin tests (`pytest`) added to CI pipeline (Q3-08)
1621

1722
### Fixed
1823

@@ -21,6 +26,12 @@
2126
- **Block `--force-with-lease`** — was slipping through regex; all force push variants now blocked uniformly
2227
- **Block remote branch deletion** — `git push origin --delete` and colon-refspec deletion (`git push origin :branch`) now blocked; deleting remote branches closes associated PRs
2328
- **Fixed README** — error handling was documented as "fails open" but code actually fails closed; corrected to match behavior
29+
- Dangerous command blocker handles prefix bypasses (`\rm`, `command rm`, `env rm`) and symbolic chmod (S2-03)
30+
- Fixed greedy alternation in write-target regex — `>>` now matched before `>` (Q3-01)
31+
- Bare `git stash` (equivalent to push) now blocked in read-only mode (Q3-04)
32+
33+
#### Protected Files Guard
34+
- Protected files guard now fails closed on unexpected errors instead of failing open (S2-04)
2435

2536
#### Documentation
2637
- **DevContainer CLI guide** — dedicated Getting Started page for terminal-only workflows without VS Code
@@ -34,6 +45,9 @@
3445

3546
### Changed
3647

48+
#### Guards
49+
- Unified write-target extraction patterns across guards — protected-files bash guard expanded from 5 to 18 patterns (C1-02)
50+
3751
#### Performance
3852
- Commented out Rust toolchain feature — saves ~1.23 GB image size; uncomment in `devcontainer.json` if needed
3953
- Commented out ccms feature pending replacement tool (requires Rust)

‎.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,9 @@ def check_git_readonly(command: str) -> str | None:
550550

551551
elif sub == "stash":
552552
# Only allow "stash list" and "stash show"
553-
if len(words) > 2 and words[2] not in ("list", "show"):
553+
if len(words) <= 2:
554+
return "Blocked: bare 'git stash' (equivalent to push) is not allowed in read-only mode"
555+
if words[2] not in ("list", "show"):
554556
return f"Blocked: 'git stash {words[2]}' is not allowed in read-only mode"
555557

556558
else:

‎.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
"""
1919

2020
import json
21-
import os
2221
import sys
2322
from datetime import datetime, timezone
2423

@@ -39,7 +38,7 @@
3938
# Handles cases where the model uses the short name directly
4039
UNQUALIFIED_MAP = {v: f"{PLUGIN_PREFIX}:{v}" for v in REDIRECT_MAP.values()}
4140

42-
LOG_FILE = os.environ.get("AGENT_REDIRECT_LOG", "/tmp/agent-redirect.log")
41+
LOG_FILE = "/tmp/agent-redirect.log"
4342

4443

4544
def log(message: str) -> None:

‎.devcontainer/plugins/devs-marketplace/plugins/dangerous-command-blocker/scripts/block-dangerous.py‎

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,18 +110,71 @@
110110
"Blocked: push with colon-refspec deletes remote branches and closes "
111111
"associated pull requests. Do not use as a workaround for force push blocks.",
112112
),
113+
# Symbolic chmod equivalents of 777
114+
(
115+
r"\bchmod\s+a=rwx\b",
116+
"Blocked: chmod a=rwx is equivalent to 777 — security vulnerability",
117+
),
118+
(
119+
r"\bchmod\s+0777\b",
120+
"Blocked: chmod 0777 creates security vulnerability",
121+
),
122+
# SetUID/SetGID bits
123+
(
124+
r"\bchmod\s+u\+s\b",
125+
"Blocked: chmod u+s sets SetUID bit — privilege escalation risk",
126+
),
127+
(
128+
r"\bchmod\s+g\+s\b",
129+
"Blocked: chmod g+s sets SetGID bit — privilege escalation risk",
130+
),
131+
# Destructive Docker operations (additional)
132+
(
133+
r"\bdocker\s+system\s+prune\b",
134+
"Blocked: docker system prune removes all unused data",
135+
),
136+
(
137+
r"\bdocker\s+volume\s+rm\b",
138+
"Blocked: docker volume rm destroys persistent data",
139+
),
140+
# Git history rewriting
141+
(
142+
r"\bgit\s+filter-branch\b",
143+
"Blocked: git filter-branch rewrites repository history",
144+
),
145+
# Plus-refspec force push (git push origin +main)
146+
(
147+
r"\bgit\s+push\s+\S+\s+\+\S",
148+
"Blocked: plus-refspec push (+ref) is a force push that destroys history",
149+
),
150+
# Force push variant: --force-if-includes
151+
(r"\bgit\s+push\s+.*--force-if-includes\b", FORCE_PUSH_SUGGESTION),
113152
]
114153

115154

116-
def check_command(command: str) -> tuple[bool, str]:
117-
"""Check if command matches any dangerous pattern.
155+
def strip_command_prefixes(command: str) -> str:
156+
"""Strip common command prefixes that bypass word-boundary matching.
118157
119-
Returns:
120-
(is_dangerous, message)
158+
Handles: backslash prefix (\\rm), command prefix, env prefix.
121159
"""
122-
for pattern, message in DANGEROUS_PATTERNS:
123-
if re.search(pattern, command, re.IGNORECASE):
124-
return True, message
160+
stripped = command
161+
# Strip leading backslash from commands (e.g. \rm -> rm)
162+
stripped = re.sub(r"(?:^|(?<=\s))\\(?=\w)", "", stripped)
163+
# Strip 'command' prefix (e.g. 'command rm' -> 'rm')
164+
stripped = re.sub(r"\bcommand\s+", "", stripped)
165+
# Strip 'env' prefix with optional VAR=val args (e.g. 'env VAR=x rm' -> 'rm')
166+
stripped = re.sub(r"\benv\s+(?:\w+=\S+\s+)*", "", stripped)
167+
return stripped
168+
169+
170+
def check_command(command: str) -> tuple[bool, str]:
171+
"""Check if command matches any dangerous pattern."""
172+
stripped = strip_command_prefixes(command)
173+
# Check both original and stripped versions
174+
for cmd in (command, stripped):
175+
for pattern, message in DANGEROUS_PATTERNS:
176+
if re.search(pattern, cmd, re.IGNORECASE):
177+
return True, message
125178
return False, ""
126179

127180

‎.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.py‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@
5757
# Patterns that indicate a bash command is writing to a file
5858
# Each captures the target file path for checking against PROTECTED_PATTERNS
5959
WRITE_PATTERNS = [
60-
# Redirect: > file, >> file
61-
r"(?:>|>>)\s*([^\s;&|]+)",
60+
# Redirect: >> file, > file (>> before > to avoid greedy match)
61+
r"(?:>>|>)\s*([^\s;&|]+)",
6262
# tee: tee file, tee -a file
6363
r"\btee\s+(?:-a\s+)?([^\s;&|]+)",
6464
# cp/mv: cp src dest, mv src dest
@@ -67,6 +67,22 @@
6767
r'\bsed\s+-i[^\s]*\s+(?:\'[^\']*\'\s+|"[^"]*"\s+|[^\s]+\s+)*([^\s;&|]+)',
6868
# cat > file (heredoc style)
6969
r"\bcat\s+(?:<<[^\s]*\s+)?>\s*([^\s;&|]+)",
70+
# --- Extended patterns (unified with guard-workspace-scope.py) ---
71+
r"\btouch\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # touch file
72+
r"\bmkdir\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # mkdir [-p] dir
73+
r"\brm\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # rm [-rf] path
74+
r"\bln\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # ln [-s] src dest
75+
r"\binstall\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # install src dest
76+
r"\brsync\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # rsync src dest
77+
r"\bchmod\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # chmod mode path
78+
r"\bchown\s+(?:-[^\s]+\s+)*[^\s:]+(?::[^\s]+)?\s+([^\s;&|]+)", # chown owner[:group] path
79+
r"\bdd\b[^;|&]*\bof=([^\s;&|]+)", # dd of=path
80+
r"\bwget\s+(?:-[^\s]+\s+)*-O\s+([^\s;&|]+)", # wget -O path
81+
r"\bcurl\s+(?:-[^\s]+\s+)*-o\s+([^\s;&|]+)", # curl -o path
82+
r"\btar\s+(?:-[^\s]+\s+)*-C\s+([^\s;&|]+)", # tar -C dir
83+
r"\bunzip\s+(?:-[^\s]+\s+)*-d\s+([^\s;&|]+)", # unzip -d dir
84+
r"\b(?:gcc|g\+\+|cc|c\+\+|clang)\s+(?:-[^\s]+\s+)*-o\s+([^\s;&|]+)", # gcc -o out
85+
r"\bsqlite3\s+([^\s;&|]+)", # sqlite3 dbpath
7086
]
7187

7288

@@ -113,9 +129,9 @@ def main():
113129
# Fail closed: can't parse means can't verify safety
114130
sys.exit(2)
115131
except Exception as e:
116-
# Log error but don't block on hook failure
132+
# Fail closed: unexpected errors should block, not allow
117133
print(f"Hook error: {e}", file=sys.stderr)
118-
sys.exit(0)
134+
sys.exit(2)
119135

120136

121137
if __name__ == "__main__":

‎.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,9 +100,9 @@ def main():
100100
# Fail closed: can't parse means can't verify safety
101101
sys.exit(2)
102102
except Exception as e:
103-
# Log error but don't block on hook failure
103+
# Fail closed: unexpected errors should block, not allow
104104
print(f"Hook error: {e}", file=sys.stderr)
105-
sys.exit(0)
105+
sys.exit(2)
106106

107107

108108
if __name__ == "__main__":

‎.devcontainer/plugins/devs-marketplace/plugins/workspace-scope-guard/scripts/guard-workspace-scope.py‎

Lines changed: 68 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@
3030
# Paths always allowed regardless of working directory
3131
_home = os.environ.get("HOME", "/home/vscode")
3232
ALLOWED_PREFIXES = [
33-
f"{_home}/.claude/", # Claude config, plans, rules
34-
"/tmp/", # System scratch
33+
f"{_home}/.claude/", # Claude config, plans, rules
34+
"/tmp/", # System scratch
3535
]
3636

3737
WRITE_TOOLS = {"Write", "Edit", "NotebookEdit"}
@@ -54,27 +54,27 @@
5454
# ---------------------------------------------------------------------------
5555
WRITE_PATTERNS = [
5656
# --- Ported from guard-protected-bash.py ---
57-
r"(?:>|>>)\s*([^\s;&|]+)", # > file, >> file
58-
r"\btee\s+(?:-a\s+)?([^\s;&|]+)", # tee file
59-
r"\b(?:cp|mv)\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # cp/mv src dest
57+
r"(?:>>|>)\s*([^\s;&|]+)", # >> file, > file
58+
r"\btee\s+(?:-a\s+)?([^\s;&|]+)", # tee file
59+
r"\b(?:cp|mv)\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # cp/mv src dest
6060
r'\bsed\s+-i[^\s]*\s+(?:\'[^\']*\'\s+|"[^"]*"\s+|[^\s]+\s+)*([^\s;&|]+)', # sed -i
61-
r"\bcat\s+(?:<<[^\s]*\s+)?>\s*([^\s;&|]+)", # cat > file
61+
r"\bcat\s+(?:<<[^\s]*\s+)?>\s*([^\s;&|]+)", # cat > file
6262
# --- New patterns ---
63-
r"\btouch\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # touch file
64-
r"\bmkdir\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # mkdir [-p] dir
65-
r"\brm\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # rm [-rf] path
66-
r"\bln\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # ln [-s] src dest
67-
r"\binstall\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # install src dest
68-
r"\brsync\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # rsync src dest
69-
r"\bchmod\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # chmod mode path
70-
r"\bchown\s+(?:-[^\s]+\s+)*[^\s:]+(?::[^\s]+)?\s+([^\s;&|]+)", # chown owner[:group] path
71-
r"\bdd\b[^;|&]*\bof=([^\s;&|]+)", # dd of=path
72-
r"\bwget\s+(?:-[^\s]+\s+)*-O\s+([^\s;&|]+)", # wget -O path
73-
r"\bcurl\s+(?:-[^\s]+\s+)*-o\s+([^\s;&|]+)", # curl -o path
74-
r"\btar\s+(?:-[^\s]+\s+)*-C\s+([^\s;&|]+)", # tar -C dir
75-
r"\bunzip\s+(?:-[^\s]+\s+)*-d\s+([^\s;&|]+)", # unzip -d dir
63+
r"\btouch\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # touch file
64+
r"\bmkdir\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # mkdir [-p] dir
65+
r"\brm\s+(?:-[^\s]+\s+)*([^\s;&|]+)", # rm [-rf] path
66+
r"\bln\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # ln [-s] src dest
67+
r"\binstall\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # install src dest
68+
r"\brsync\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # rsync src dest
69+
r"\bchmod\s+(?:-[^\s]+\s+)*[^\s]+\s+([^\s;&|]+)", # chmod mode path
70+
r"\bchown\s+(?:-[^\s]+\s+)*[^\s:]+(?::[^\s]+)?\s+([^\s;&|]+)", # chown owner[:group] path
71+
r"\bdd\b[^;|&]*\bof=([^\s;&|]+)", # dd of=path
72+
r"\bwget\s+(?:-[^\s]+\s+)*-O\s+([^\s;&|]+)", # wget -O path
73+
r"\bcurl\s+(?:-[^\s]+\s+)*-o\s+([^\s;&|]+)", # curl -o path
74+
r"\btar\s+(?:-[^\s]+\s+)*-C\s+([^\s;&|]+)", # tar -C dir
75+
r"\bunzip\s+(?:-[^\s]+\s+)*-d\s+([^\s;&|]+)", # unzip -d dir
7676
r"\b(?:gcc|g\+\+|cc|c\+\+|clang)\s+(?:-[^\s]+\s+)*-o\s+([^\s;&|]+)", # gcc -o out
77-
r"\bsqlite3\s+([^\s;&|]+)", # sqlite3 dbpath
77+
r"\bsqlite3\s+([^\s;&|]+)", # sqlite3 dbpath
7878
]
7979

8080
# ---------------------------------------------------------------------------
@@ -86,26 +86,55 @@
8686
# ---------------------------------------------------------------------------
8787
# System command exemption (Layer 1 only)
8888
# ---------------------------------------------------------------------------
89-
SYSTEM_COMMANDS = frozenset({
90-
"git", "pip", "pip3", "npm", "npx", "yarn", "pnpm",
91-
"apt-get", "apt", "cargo", "go", "docker", "make", "cmake",
92-
"node", "python3", "python", "ruby", "gem", "bundle",
93-
})
89+
SYSTEM_COMMANDS = frozenset(
90+
{
91+
"git",
92+
"pip",
93+
"pip3",
94+
"npm",
95+
"npx",
96+
"yarn",
97+
"pnpm",
98+
"apt-get",
99+
"apt",
100+
"cargo",
101+
"go",
102+
"docker",
103+
"make",
104+
"cmake",
105+
"node",
106+
"python3",
107+
"python",
108+
"ruby",
109+
"gem",
110+
"bundle",
111+
}
112+
)
94113

95114
SYSTEM_PATH_PREFIXES = (
96-
"/usr/", "/bin/", "/sbin/", "/lib/", "/opt/",
97-
"/proc/", "/sys/", "/dev/", "/var/", "/etc/",
115+
"/usr/",
116+
"/bin/",
117+
"/sbin/",
118+
"/lib/",
119+
"/opt/",
120+
"/proc/",
121+
"/sys/",
122+
"/dev/",
123+
"/var/",
124+
"/etc/",
98125
)
99126

100127

101128
# ---------------------------------------------------------------------------
102129
# Core check functions
103130
# ---------------------------------------------------------------------------
104131

132+
105133
def is_blacklisted(resolved_path: str) -> bool:
106134
"""Check if resolved_path is under a permanently blocked directory."""
107-
return (resolved_path == "/workspaces/.devcontainer"
108-
or resolved_path.startswith("/workspaces/.devcontainer/"))
135+
return resolved_path == "/workspaces/.devcontainer" or resolved_path.startswith(
136+
"/workspaces/.devcontainer/"
137+
)
109138

110139

111140
def is_in_scope(resolved_path: str, cwd: str) -> bool:
@@ -135,6 +164,7 @@ def get_target_path(tool_name: str, tool_input: dict) -> str | None:
135164
# Bash enforcement
136165
# ---------------------------------------------------------------------------
137166

167+
138168
def extract_write_targets(command: str) -> list[str]:
139169
"""Extract file paths that the command writes to (Layer 1)."""
140170
targets = []
@@ -157,7 +187,11 @@ def extract_primary_command(command: str) -> str:
157187
while i < len(tokens):
158188
tok = tokens[i]
159189
# Skip inline variable assignments: VAR=value
160-
if "=" in tok and not tok.startswith("-") and tok.split("=", 1)[0].isidentifier():
190+
if (
191+
"=" in tok
192+
and not tok.startswith("-")
193+
and tok.split("=", 1)[0].isidentifier()
194+
):
161195
i += 1
162196
continue
163197
# Skip sudo and its flags
@@ -243,7 +277,9 @@ def check_bash_scope(command: str, cwd: str) -> None:
243277
# Override: if ANY target is under /workspaces/ outside cwd → NOT exempt
244278
if skip_layer1:
245279
for _, resolved in resolved_targets:
246-
if resolved.startswith("/workspaces/") and not is_in_scope(resolved, cwd):
280+
if resolved.startswith("/workspaces/") and not is_in_scope(
281+
resolved, cwd
282+
):
247283
skip_layer1 = False
248284
break
249285

@@ -273,6 +309,7 @@ def check_bash_scope(command: str, cwd: str) -> None:
273309
# Main
274310
# ---------------------------------------------------------------------------
275311

312+
276313
def main():
277314
try:
278315
input_data = json.load(sys.stdin)

‎.github/workflows/ci.yml‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ name: CI
22

33
on:
44
push:
5-
branches: [main]
5+
branches: [main, staging]
66
pull_request:
7-
branches: [main]
7+
branches: [main, staging]
88

99
jobs:
1010
test:
@@ -24,3 +24,13 @@ jobs:
2424
with:
2525
node-version: 18
2626
- run: npx @biomejs/biome check setup.js test.js
27+
28+
test-plugins:
29+
runs-on: ubuntu-latest
30+
steps:
31+
- uses: actions/checkout@v6
32+
- uses: actions/setup-python@v5
33+
with:
34+
python-version: "3.x"
35+
- run: pip install pytest
36+
- run: pytest tests/ -v

0 commit comments

Comments
 (0)