Skip to content

Commit 436a26f

Browse files
committed
Merge branch 'w2/policy' into fix/danger-classifier-red-bugs
# Conflicts: # internal/danger/analysis.go
2 parents 98ad7ba + a44527a commit 436a26f

18 files changed

Lines changed: 1430 additions & 38 deletions

‎cmd/odek/approver_display_test.go‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package main
2+
3+
import (
4+
"bytes"
5+
"fmt"
6+
"strings"
7+
"testing"
8+
"time"
9+
10+
"github.com/BackendStack21/odek/internal/config"
11+
"github.com/BackendStack21/odek/internal/danger"
12+
"github.com/BackendStack21/odek/internal/mcpclient"
13+
)
14+
15+
const hostileApprovalText = "echo ok\x1b[2K\r\x1b]0;safe\x07 \u202egnirts\u202c \u2066x\u2069 \u200bz"
16+
17+
func assertNoRawControl(t *testing.T, label, s string) {
18+
t.Helper()
19+
for _, r := range s {
20+
if r == '\n' || r == '\t' {
21+
continue
22+
}
23+
if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) || r == 0x200b ||
24+
(r >= 0x202a && r <= 0x202e) || (r >= 0x2066 && r <= 0x2069) {
25+
t.Errorf("%s holds raw control or bidi character U+%04X: %q", label, r, s)
26+
return
27+
}
28+
}
29+
}
30+
31+
// The WebSocket approval frame carries sanitized command and description text:
32+
// the UI renders these fields as text, so control and bidi characters must
33+
// already be visible escapes.
34+
func TestWSApprover_FramesCarrySanitizedText(t *testing.T) {
35+
frames := make(chan approvalRequest, 1)
36+
a := newWSApprover(func(v any) error {
37+
if req, ok := v.(approvalRequest); ok {
38+
frames <- req
39+
}
40+
return nil
41+
})
42+
done := make(chan error, 1)
43+
go func() {
44+
done <- a.PromptCommand(danger.NetworkEgress, hostileApprovalText, "why "+hostileApprovalText)
45+
}()
46+
select {
47+
case req := <-frames:
48+
assertNoRawControl(t, "command", req.Command)
49+
assertNoRawControl(t, "description", req.Description)
50+
if !strings.Contains(req.Command, `\x1b`) || !strings.Contains(req.Command, `\u202e`) {
51+
t.Errorf("command escapes not visible: %q", req.Command)
52+
}
53+
a.HandleResponse(req.ID, "deny")
54+
case <-time.After(5 * time.Second):
55+
t.Fatal("no approval frame")
56+
}
57+
<-done
58+
}
59+
60+
func TestWSApprover_OperationFramesAreSanitized(t *testing.T) {
61+
frames := make(chan approvalRequest, 1)
62+
a := newWSApprover(func(v any) error {
63+
if req, ok := v.(approvalRequest); ok {
64+
frames <- req
65+
}
66+
return nil
67+
})
68+
done := make(chan error, 1)
69+
go func() {
70+
done <- a.PromptOperation(danger.ToolOperation{Name: "write_file\x1b[2K", Resource: "/tmp/\u202eexe.txt", Risk: danger.LocalWrite})
71+
}()
72+
select {
73+
case req := <-frames:
74+
assertNoRawControl(t, "command", req.Command)
75+
assertNoRawControl(t, "description", req.Description)
76+
a.HandleResponse(req.ID, "deny")
77+
case <-time.After(5 * time.Second):
78+
t.Fatal("no approval frame")
79+
}
80+
<-done
81+
}
82+
83+
// The project MCP approval prompt prints repo-controlled command, args and
84+
// env values; none of them may carry raw control or bidi characters.
85+
func TestMCPApprovalPrompt_SanitizesProjectControlledText(t *testing.T) {
86+
setupTestHome(t)
87+
t.Setenv("ODEK_APPROVE_MCP", "")
88+
nonce := fmt.Sprintf("srv-%d", time.Now().UnixNano())
89+
resolved := config.ResolvedConfig{
90+
MCPServers: map[string]mcpclient.ServerConfig{
91+
"project": {
92+
Command: "node\x1b[2K" + nonce,
93+
Args: []string{"\u202egnirts", "a\rb"},
94+
Env: map[string]string{"K\x07": "v\x1b]0;x\x07"},
95+
},
96+
},
97+
ProjectMCPServerNames: []string{"project"},
98+
}
99+
var out bytes.Buffer
100+
_ = approveMCPServersWithTTY(resolved, strings.NewReader("\n"), &out, true)
101+
assertNoRawControl(t, "MCP approval prompt", out.String())
102+
if !strings.Contains(out.String(), `\x1b[2K`) {
103+
t.Errorf("escape not visible in prompt: %q", out.String())
104+
}
105+
}

‎cmd/odek/mcp_approval.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"strings"
1717

1818
"github.com/BackendStack21/odek/internal/config"
19+
"github.com/BackendStack21/odek/internal/danger"
1920
"github.com/BackendStack21/odek/internal/fsatomic"
2021
"github.com/BackendStack21/odek/internal/guard"
2122
"github.com/BackendStack21/odek/internal/mcpclient"
@@ -117,21 +118,21 @@ func approveMCPServersWithTTY(resolved config.ResolvedConfig, stdin io.Reader, s
117118

118119
if cfg.URL != "" {
119120
fmt.Fprintf(stdout, "\nProject-level MCP server %q wants to connect:\n", name)
120-
fmt.Fprintf(stdout, " url: %s\n", cfg.URL)
121+
fmt.Fprintf(stdout, " url: %s\n", danger.SanitizeInline(cfg.URL))
121122
if cfg.TokenEnv != "" {
122-
fmt.Fprintf(stdout, " token_env: %s\n", cfg.TokenEnv)
123+
fmt.Fprintf(stdout, " token_env: %s\n", danger.SanitizeInline(cfg.TokenEnv))
123124
}
124125
} else {
125126
fmt.Fprintf(stdout, "\nProject-level MCP server %q wants to run:\n", name)
126-
fmt.Fprintf(stdout, " command: %s\n", cfg.Command)
127+
fmt.Fprintf(stdout, " command: %s\n", danger.SanitizeInline(cfg.Command))
127128
if len(cfg.Args) > 0 {
128-
fmt.Fprintf(stdout, " args: %s\n", strings.Join(cfg.Args, " "))
129+
fmt.Fprintf(stdout, " args: %s\n", danger.SanitizeInline(strings.Join(cfg.Args, " ")))
129130
}
130131
if len(cfg.Env) > 0 {
131132
envKeys := sortedEnvKeys(cfg.Env)
132133
fmt.Fprintf(stdout, " env:\n")
133134
for _, k := range envKeys {
134-
fmt.Fprintf(stdout, " %s=%s\n", k, cfg.Env[k])
135+
fmt.Fprintf(stdout, " %s=%s\n", danger.SanitizeInline(k), danger.SanitizeInline(cfg.Env[k]))
135136
}
136137
}
137138
}
@@ -145,7 +146,7 @@ func approveMCPServersWithTTY(resolved config.ResolvedConfig, stdin io.Reader, s
145146
fmt.Fprintf(stdout, " max_result_chars: %d\n", cfg.MaxResultChars)
146147
}
147148
if len(cfg.ArtifactRoots) > 0 {
148-
fmt.Fprintf(stdout, " artifact_roots: %s\n", strings.Join(cfg.ArtifactRoots, ", "))
149+
fmt.Fprintf(stdout, " artifact_roots: %s\n", danger.SanitizeInline(strings.Join(cfg.ArtifactRoots, ", ")))
149150
}
150151
fmt.Fprintf(stdout, "Approve? [y/N] ")
151152

@@ -293,7 +294,7 @@ func approveMCPToolsWithTTY(projectDir, serverName string, cfg mcpclient.ServerC
293294

294295
fmt.Fprintf(stdout, "\nMCP server %q wants to register tool %q\n", serverName, def.Name)
295296
if def.Description != "" {
296-
fmt.Fprintf(stdout, " description: %s\n", sanitizeTerminal(truncateDescription(def.Description, 200)))
297+
fmt.Fprintf(stdout, " description: %s\n", danger.SanitizeInline(sanitizeTerminal(truncateDescription(def.Description, 200))))
297298
}
298299
fmt.Fprintf(stdout, " schema: sha256:%s (%d bytes)\n", schemaHash[:16], schemaSize)
299300
fmt.Fprintf(stdout, "Approve? [y/N] ")

‎cmd/odek/project_sandbox_approval.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"sync"
1515

1616
"github.com/BackendStack21/odek/internal/config"
17+
"github.com/BackendStack21/odek/internal/danger"
1718
"github.com/BackendStack21/odek/internal/fsatomic"
1819
"github.com/BackendStack21/odek/internal/sandbox"
1920
"golang.org/x/term"
@@ -110,28 +111,28 @@ func approveProjectSandboxWithTTY(resolved config.ResolvedConfig, stdin io.Reade
110111
if hasOverride {
111112
fmt.Fprintf(stdout, "WARNING: project config (%s) requests sandbox overrides:\n", config.ProjectConfigPath())
112113
if o.HasImage {
113-
fmt.Fprintf(stdout, " image: %s\n", o.Image)
114+
fmt.Fprintf(stdout, " image: %s\n", danger.SanitizeInline(o.Image))
114115
}
115116
if o.HasNetwork {
116-
fmt.Fprintf(stdout, " network: %s\n", o.Network)
117+
fmt.Fprintf(stdout, " network: %s\n", danger.SanitizeInline(o.Network))
117118
}
118119
if o.HasEnv {
119-
fmt.Fprintf(stdout, " env: %s\n", strings.Join(o.EnvKeys, ", "))
120+
fmt.Fprintf(stdout, " env: %s\n", danger.SanitizeInline(strings.Join(o.EnvKeys, ", ")))
120121
if o.EnvHasInterpolation {
121122
fmt.Fprintln(stdout, " ⚠️ sandbox_env values contain ${...} interpolation against host environment variables")
122123
}
123124
}
124125
if o.HasVolumes {
125-
fmt.Fprintf(stdout, " volumes: %s\n", strings.Join(o.Volumes, ", "))
126+
fmt.Fprintf(stdout, " volumes: %s\n", danger.SanitizeInline(strings.Join(o.Volumes, ", ")))
126127
}
127128
if o.HasUser {
128-
fmt.Fprintf(stdout, " user: %s\n", o.User)
129+
fmt.Fprintf(stdout, " user: %s\n", danger.SanitizeInline(o.User))
129130
}
130131
if o.HasMemory {
131-
fmt.Fprintf(stdout, " memory: %s\n", o.Memory)
132+
fmt.Fprintf(stdout, " memory: %s\n", danger.SanitizeInline(o.Memory))
132133
}
133134
if o.HasCPUs {
134-
fmt.Fprintf(stdout, " cpus: %s\n", o.CPUs)
135+
fmt.Fprintf(stdout, " cpus: %s\n", danger.SanitizeInline(o.CPUs))
135136
}
136137
fmt.Fprintln(stdout)
137138
fmt.Fprintln(stdout, "Allowing this means code in the sandbox can read workspace files and,")

‎cmd/odek/wsapprover.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,12 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string
196196
}
197197

198198
id := a.newID()
199-
decision := session.Decision{ID: id, Kind: "approval", Command: cmd, Risk: string(cls), State: "interrupted"}
199+
// The UI renders these fields as text, so the human must see what the
200+
// bytes are: control characters and bidi/invisible format characters
201+
// arrive as visible escapes, never raw.
202+
shownCmd := danger.SanitizeForDisplay(cmd)
203+
shownDescription := danger.SanitizeForDisplay(description)
204+
decision := session.Decision{ID: id, Kind: "approval", Command: shownCmd, Risk: string(cls), State: "interrupted"}
200205
defer func() { a.recordDecision(decision) }()
201206
resp := make(chan string, 1)
202207

@@ -236,8 +241,8 @@ func (a *wsApprover) PromptCommand(cls danger.RiskClass, cmd, description string
236241
Type: "approval_request",
237242
ID: id,
238243
Risk: string(cls),
239-
Command: cmd,
240-
Description: description,
244+
Command: shownCmd,
245+
Description: shownDescription,
241246
IsOperation: false,
242247
AllowTrust: allowTrust,
243248
Friction: friction,

‎docs/CONFIG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ The `dangerous` section is the operator's safety policy for tool calls. Every sh
351351
|-------|---------|-------------|
352352
| `classes` | see below | Map of risk class → action. Only non-default overrides need to be set |
353353
| `allowlist` | `[]` | Command strings that are **always allowed** regardless of classification. **Exact match**; takes priority over `denylist` |
354-
| `denylist` | `[]` | Command strings that are **always denied** regardless of classification. **Prefix match** (after trimming) |
354+
| `denylist` | `[]` | Command strings that are **always denied** regardless of classification. **Token-prefix match** against every command the line would run: each `;`/`&&`/`\|\|` segment and pipe stage, the command left after wrappers (`env`, `command`, `nohup`, `timeout`, `sudo`, `xargs`, …) and leading `VAR=value` are stripped, the program by basename (`/usr/bin/git` matches `git`), `git` without its global options (`git -C dir push` matches `git push`), shell `-c`/`eval` payloads, `find -exec` commands, and `$(…)`/backtick/process-substitution bodies. Entry tokens must equal the leading tokens of the command, so `git push` matches `git push origin` but not `git push-notes`. Values only known at run time (variable expansions, command output) are not resolved |
355355
| `action` | *(per-class defaults)* | Global default action for **all** classes — `"allow"` (everything runs unprompted) or `"deny"` (lockdown: nothing runs unless explicitly allowed). Per-class `classes` entries still win |
356356
| `non_interactive` | `"read_only"` | What happens to prompt-class operations when no TTY is available (CI, headless, piped input): `"read_only"` (safe inspection proceeds; writes/exec/egress denied), `"deny"` (block all prompted operations), `"allow"` (run everything — not recommended) |
357357
| `strip_secrets_env_children` | `false` | Remove `secrets.env` names from the environment of **host-mode** child processes spawned by `shell` and background jobs. Default `false`: children inherit, so workflows that legitimately need credentials in shell children (`gh`, `curl`) keep working. Sub-agent and MCP stdio spawns strip unconditionally regardless of this knob; sandbox-mode containers never see host secrets |

‎docs/DOCKER_COMPOSE_USER_GUIDE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ inspection proceeds without a human channel, and anything that would prompt is d
210210
| `non_interactive` | What to do with a **prompt**‑level command when there is no human channel (no TTY, no Web UI). `"deny"` blocks it; `"allow"` runs it; `"read_only"` (the shipped Restricted value) lets read‑only/inspection commands proceed and denies the rest. |
211211
| `classes` | Per‑class action overrides. The most specific setting — it wins over `action` and the built‑in defaults. Only list the classes you want to pin. |
212212
| `allowlist` | Commands that always run, **exact string match**, no classification. Highest priority of all. Use for a handful of trusted exact commands (e.g. `"npm run deploy"`). |
213-
| `denylist` | Commands that are always denied, **prefix match** after trimming. Beats classification and even godmode — but **not** the allowlist. |
213+
| `denylist` | Commands that are always denied, **token-prefix match** against every command the line runs (chain segments, pipe stages, wrapper-stripped commands, `bash -c` payloads, substitutions). Beats classification and even godmode — but **not** the allowlist. |
214214

215215
#### How the classes map (built‑in risk model)
216216

@@ -240,7 +240,7 @@ or relax the class with `"unknown": "prompt"`.
240240
#### How an action is resolved (precedence, first match wins)
241241

242242
1. Command exactly matches an **`allowlist`** entry → **allow**.
243-
2. Command starts with a **`denylist`** entry → **deny**.
243+
2. Any command in the line starts with a **`denylist`** entry → **deny**.
244244
3. Otherwise classify it, then: explicit **`classes`** entry → `blocked` is **always deny** → global **`action`** (if set) → built‑in class default.
245245
4. If the result is **prompt** and there's no human channel, **`non_interactive`** decides.
246246

‎docs/SECURITY.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ The classifier resists the common evasion families (see the package doc in `inte
151151
- `npx --version` / `bunx --help` stay `safe`; a real `npx <pkg>` is still `code_execution`. `php -l` / `ruby -c` / `node --check` are syntax checks (`safe`), not execution.
152152
- `rubocop` / `stylua` / `shfmt` / `shellcheck` / `hadolint` / `yamllint` / `swiftc` / `kotlinc` / `swift build` / `ffprobe` / `identify` / `sqlite3 .tables` are `safe`. `swift run` and `sqlite3 '.shell …'` are `code_execution`. `pandoc` / `ffmpeg` / `convert` are `local_write`.
153153
- `nvm ls` / `pyenv versions` / `asdf list` are `safe`; `nvm install` / `pyenv install` are `install`. `direnv status` is `safe`; `direnv exec` is `code_execution`; `direnv allow` is `persistence` (trusts a `.envrc`). `gdb --version` is `safe`; `gdb ./bin` is `code_execution`.
154-
- `printenv PATH` is `safe` (one variable); bare `printenv` / `env` still dump the process environment (`system_write`). `env -u FOO ls` unwraps to `ls`.
154+
- `printenv PATH` is `safe` (one variable), but any reference to a secret-bearing variable (`$NAME`, `${NAME…}`, `printenv NAME`, `declare -p NAME`, `os.environ['NAME']`; names ending in `_TOKEN`, `_SECRET`, `_API_KEY`, `_PASSWORD`, `_PRIVATE_KEY`, `_ACCESS_KEY`, `_CREDENTIALS`, plus well-known names such as `DATABASE_URL`) is `system_write`, even through `echo`/`printf`; bare `printenv` / `env` still dump the process environment (`system_write`). `env -u FOO ls` unwraps to `ls`.
155155
- `kubectl get` / `logs` / `helm list` / `terraform plan` are `network_egress`. `kubectl apply` / `delete`, `helm install`, and `terraform apply` / `destroy` are `system_write`. `kubectl exec` is `code_execution`. Unrecognised infra verbs stay `unknown`.
156156
- `aws --version` / `gcloud --version` / `az --version` are `safe`; other aws/gcloud/az verbs stay `unknown` (deny).
157157
- `hugo --help` is `safe`; bare `hugo` builds the site (`local_write`); `hugo server` is `code_execution`.
@@ -163,6 +163,8 @@ The classifier resists the common evasion families (see the package doc in `inte
163163
- `echo rm -rf / | sh` — a pipe-fed shell whose stdin is a static literal is classified as that command, so a root wipe is `destructive` (deny), not merely `code_execution` (prompt). Dynamic payloads (`cat file | bash`) stay `code_execution`.
164164
- `cp x /etc/cron.d/job`, `tee /usr/bin/foo`, `mv x /etc/profile.d/y`, `ln -s … /etc/systemd/system/…`, `install … /usr/local/bin/…` — a file-mutating command whose target is a system path is `system_write` (prompt), not auto-allowed `local_write`. `chmod u+s` / `chmod 4755` / `chmod 04755` (setuid/setgid, including a leading-zero octal) and `chmod --reference` (mode copy that can plant setuid) are `system_write` regardless of path. `chmod 0755` stays `local_write`.
165165
- `wipefs`, `blkdiscard`, `sgdisk`/`gdisk`/`cfdisk`/`sfdisk`, `mkswap`, `badblocks`, `cryptsetup`, and the `mkfs.*` family are `destructive`; `shred` is target-aware (local file → `local_write`, raw device / wipe target → `destructive`); `shutdown`, `reboot`, `halt`, `poweroff`, `init 0`/`init 6` are machine power-control `destructive` (deny-by-default).
166+
- Approval prompts (terminal, WebSocket UI frames, Telegram, the batch card, project MCP and sandbox prompts) print model- or repo-supplied text through `danger.SanitizeForDisplay` / `SanitizeInline`: control characters, ANSI/OSC escapes, carriage returns, bidi controls and invisible format characters become visible escapes (`\x1b`, `\u202e`), multi-line values are indented so they cannot forge a prompt field, and an over-long value keeps its head and last kilobyte around an explicit `…[N more bytes]` marker.
167+
- Credential files anywhere in the workspace (`.env` and `.env.*` except `.example`/`.sample`/`.template`, `credentials.json`, `service-account*.json`, `*.pem`, `*.key`, `id_rsa`-style keys, `.netrc`, `.npmrc`, `.pypirc`, `.git-credentials`, `kubeconfig`, `*.tfstate`, `*.tfvars`, `secrets.<data ext>`, `*.keystore`/`*.jks`/`*.p12`/`*.pfx`) are `system_write` to read or write; name-only inspection (`ls`, `stat`, `test`, `wc`, `du`, `file`), search patterns and `find -name` operands are not reads.
166168
- `env` and `printenv` — a full process-environment dump is `system_write` because it can leak secrets the redaction scanner does not recognise. `env FOO=bar <cmd>` classifies the real `<cmd>` normally.
167169
- `git -c alias.x='!id' x`, `git -c core.pager='sh -c id' --paginate log`, `git config --global alias.pwn '!cmd'` — the `git config` subcommand is always `code_execution`, and `git -c` / `--config-env` overrides are `code_execution` when the key can define a command (`alias.*` with a `!` value, `core.pager`, `core.fsmonitor`, `credential.helper`); inert keys classify by their subcommand.
168170
- `find . -delete`, `rsync -a --delete /empty/ ~`, `rsync --remove-source-files` — bulk-deletion flags are `destructive`; `find -fprint` / `-fprintf` are `local_write` because they write match lists to arbitrary files.

‎internal/danger/analysis.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,11 @@ func analyzeWithState(cmd string, depth int, inherited *shellAnalysisState) Anal
130130
return result
131131
}
132132
main, subs := normalize(cmd)
133+
// Here-document bodies are consumed by normalize but still expand
134+
// variables when their delimiter is unquoted, so the raw text is scanned too.
135+
if referencesSensitiveEnv(main) || (strings.Contains(cmd, "<<") && referencesSensitiveEnv(cmd)) {
136+
result.add(SystemWrite)
137+
}
133138
tokens := tokenize(main)
134139
cwd, err := os.Getwd()
135140
state := shellAnalysisState{cwd: cwd, vars: make(map[string]string), uncertain: err != nil, written: make(map[string]bool)}
@@ -250,6 +255,9 @@ func analyzeWithState(cmd string, depth int, inherited *shellAnalysisState) Anal
250255
chainVars[assigned] = true
251256
}
252257
}
258+
if secretNameOperand(name, inner[1:]) || stageTouchesCredentialFile(stage, inner, displayVerbs[name]) {
259+
result.add(SystemWrite)
260+
}
253261
if isCodeExecution(name, inner) || explicitUntrustedExecutable(inner[0]) || (i > 0 && (pipedShells[name] || isStdinExecInterpreter(name) || embeddedShellInterpreters[name])) {
254262
result.add(CodeExecution)
255263
}
@@ -734,7 +742,11 @@ func (s *shellAnalysisState) targetRisk(target, cwd string, known bool) RiskClas
734742
return LocalWrite
735743
}
736744
path := resolveInDirectory(target, cwd)
737-
return worstOf(ClassifyPathWrite(path), classifyResourceToken(path))
745+
risk := worstOf(ClassifyPathWrite(path), classifyResourceToken(path))
746+
if credentialPathToken(path) {
747+
risk = worstOf(risk, SystemWrite)
748+
}
749+
return risk
738750
}
739751

740752
// isInputOutputRedirect reports whether tok is a redirection operator whose

0 commit comments

Comments
 (0)