Skip to content

Commit 620b683

Browse files
jkyberneeesclaude
andcommitted
harden(danger): address code-review cleanup/altitude findings
Follow-up to the Unknown-class fixes — the lower-severity review items: 5. shell tool description (cmd/odek/shell.go): add the `unknown` class and state the fail-closed default, so the model driving the tool can reason about why an unrecognised command was denied. 6. sensitivePathFragments (classifier.go): document why it is deliberately separate from ClassifyPath's home-sensitive-dir list (token-substring, credential reads vs absolute-path write-risk) so the overlap doesn't get "deduped" into a behavior change, and so a maintainer updating one considers the other. 7. safeCommands (classifier.go): extend the read-only allowlist with common modern CLIs (fd, eza/exa/lsd, htop/btop/glances, pstree/procs, duf, dust, delta, hexyl, glow) so fail-closed doesn't deny routine inspection. 8. classifyStage (classifier.go): pass a pipedInto flag instead of having classifyPipeline re-run unwrapWrappers on each non-head stage purely to read the head command — one unwrap per stage now. 9. hasSystemRedirectTarget → touchesSystemPath (classifier.go): the function flags ANY system-path token, not just redirect targets, and is NOT redundant with isSystemWrite (verified: it catches `cat /etc/foo` and unknown tools pointed at /usr, and runs after isLocalWrite). Renamed for accuracy with a comment on why both checks exist. Behavior change is limited to #7 (more tools classify Safe). Regression test for the new safe tools added. Full suite green; go vet clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e288f40 commit 620b683

4 files changed

Lines changed: 38 additions & 21 deletions

File tree

cmd/odek/shell.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,9 @@ Use for: reading files, listing directories, running tests, building code, and g
7171
In sandbox mode (--sandbox), commands run inside the Docker container with restricted permissions.
7272
In host mode (default), commands run with the same permissions as the odek process.
7373
74-
Risk classes: safe, local_write, system_write, destructive, network_egress, code_execution, install, blocked
75-
High-risk operations may prompt for approval (configurable via dangerous section in odek.json).`
74+
Risk classes: safe, local_write, system_write, destructive, network_egress, code_execution, install, unknown, blocked
75+
High-risk operations may prompt for approval (configurable via dangerous section in odek.json).
76+
The gate fails closed: an unrecognised command classifies as "unknown" and is denied by default.`
7677
}
7778

7879
func (t *shellTool) Schema() any {

internal/danger/classifier.go

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -668,6 +668,10 @@ var safeCommands = map[string]bool{
668668
"local": true, "declare": true, "typeset": true, "readonly": true,
669669
"alias": true, "unalias": true, "jobs": true, "bg": true, "fg": true,
670670
"disown": true, "let": true, "ulimit": true, "times": true,
671+
// common modern read-only CLIs (ls/find/cat/ps/df/du/diff/hex viewers)
672+
"fd": true, "fdfind": true, "eza": true, "exa": true, "lsd": true,
673+
"htop": true, "btop": true, "glances": true, "pstree": true, "procs": true,
674+
"duf": true, "dust": true, "delta": true, "hexyl": true, "glow": true,
671675
}
672676

673677
// ── Classifier ─────────────────────────────────────────────────────────
@@ -744,15 +748,8 @@ func classifyPipeline(tokens []string) RiskClass {
744748
stages := splitPipes(tokens)
745749
worst := Safe
746750
for idx, stage := range stages {
747-
cls := classifyStage(stage)
748-
if idx > 0 {
749-
// Data piped into a shell interpreter executes fetched code.
750-
cmdTokens, _ := unwrapWrappers(stage)
751-
if len(cmdTokens) > 0 && pipedShells[commandName(cmdTokens[0])] {
752-
cls = worstOf(cls, CodeExecution)
753-
}
754-
}
755-
worst = worstOf(worst, cls)
751+
// idx > 0 means this stage receives piped input from the previous one.
752+
worst = worstOf(worst, classifyStage(stage, idx > 0))
756753
}
757754
return worst
758755
}
@@ -762,7 +759,9 @@ func classifyPipeline(tokens []string) RiskClass {
762759
// underneath is the one classified, while privileged wrappers still set a
763760
// system_write floor. It then escalates for shell `-c` payloads, `find
764761
// -exec`, and any reverse-shell or sensitive-resource tokens in the stage.
765-
func classifyStage(tokens []string) RiskClass {
762+
// pipedInto reports whether the stage's stdin comes from an upstream pipe, in
763+
// which case feeding it to a shell interpreter is code execution.
764+
func classifyStage(tokens []string, pipedInto bool) RiskClass {
766765
if len(tokens) == 0 {
767766
return Safe
768767
}
@@ -772,9 +771,12 @@ func classifyStage(tokens []string) RiskClass {
772771
cls = worstOf(cls, classifyCommand(cmdTokens))
773772

774773
name := commandName(cmdTokens[0])
775-
// A shell interpreter given something to execute — a -c payload, a
776-
// script file, or a process substitution like <(curl …) — runs code.
774+
// A shell interpreter that executes code: piped-in data (`… | bash`),
775+
// a -c payload, a script file, or a process substitution `<(curl …)`.
777776
if pipedShells[name] {
777+
if pipedInto {
778+
cls = worstOf(cls, CodeExecution)
779+
}
778780
if arg := flagArg(cmdTokens, "-c"); arg != "" {
779781
cls = worstOf(cls, CodeExecution)
780782
cls = worstOf(cls, Classify(arg))
@@ -1256,6 +1258,13 @@ func classifyResourceToken(tok string) RiskClass {
12561258
// Matching is substring-based so it catches ~, /root, /home/<user>, and
12571259
// absolute variants alike. /etc/passwd is intentionally excluded — it is
12581260
// world-readable and accessed routinely, so flagging it is pure noise.
1261+
//
1262+
// This is deliberately distinct from ClassifyPath's home-sensitive-dir list:
1263+
// that classifies the *write* risk of an absolute filesystem path (for the
1264+
// file tool), whereas this flags *credential reads/writes* in a raw shell
1265+
// token (which may be ~-relative or carry an `of=`-style prefix). They
1266+
// overlap (~/.ssh, ~/.aws, ~/.gnupg) but are not interchangeable; if you add
1267+
// a credential location to one, consider whether the other needs it too.
12591268
var sensitivePathFragments = []string{
12601269
"/etc/shadow", "/etc/gshadow", "/etc/sudoers", "/etc/ssl/private",
12611270
"/.ssh", "id_rsa", "id_dsa", "id_ecdsa", "id_ed25519",
@@ -1402,8 +1411,10 @@ func classifyCommand(tokens []string) RiskClass {
14021411
return LocalWrite
14031412
}
14041413

1405-
// Check for redirect targets that are system paths
1406-
if hasSystemRedirectTarget(tokens) {
1414+
// Any argument that names a system path (read or write) — broader than
1415+
// isSystemWrite's redirect-only check above, which runs earlier so a
1416+
// redirect to a system path beats the LocalWrite classification.
1417+
if touchesSystemPath(tokens) {
14071418
return SystemWrite
14081419
}
14091420

@@ -1734,8 +1745,11 @@ func hasArgAfter(tokens []string, after, target string) bool {
17341745
return false
17351746
}
17361747

1737-
// hasSystemRedirectTarget checks if any redirect target is a system path.
1738-
func hasSystemRedirectTarget(tokens []string) bool {
1748+
// touchesSystemPath reports whether any token names a system path (an
1749+
// argument or a redirect target alike). It is intentionally broader than the
1750+
// redirect-only scan in isSystemWrite — it catches reads/args such as
1751+
// `cat /etc/foo` or an unknown tool pointed at /usr — so both checks exist.
1752+
func touchesSystemPath(tokens []string) bool {
17391753
for _, tok := range tokens {
17401754
if tok == ">" || tok == ">>" {
17411755
continue

internal/danger/classifier_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -764,9 +764,9 @@ func TestHasSystemRedirectTarget(t *testing.T) {
764764
}
765765
for _, tt := range tests {
766766
t.Run(tt.name, func(t *testing.T) {
767-
got := hasSystemRedirectTarget(tt.tokens)
767+
got := touchesSystemPath(tt.tokens)
768768
if got != tt.want {
769-
t.Errorf("hasSystemRedirectTarget(%v) = %v, want %v", tt.tokens, got, tt.want)
769+
t.Errorf("touchesSystemPath(%v) = %v, want %v", tt.tokens, got, tt.want)
770770
}
771771
})
772772
}
@@ -893,7 +893,7 @@ func TestClassify_ForkBomb_StillDetected(t *testing.T) {
893893
}
894894

895895
func TestClassify_SystemRedirectTarget(t *testing.T) {
896-
// Regression: isSystemWrite and hasSystemRedirectTarget now share
896+
// Regression: isSystemWrite and touchesSystemPath now share
897897
// isSystemPath. Verify system path redirect detection works.
898898
tests := []struct {
899899
cmd string

internal/danger/hardening_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,8 @@ func TestHardening_SafeAllowlistStillSafe(t *testing.T) {
231231
"printf '%s' x", "date", "uname -a", "id", "whoami", "ps aux",
232232
"which go", "type ls", "sha256sum f", "xxd f", "cd /etc", "export FOO=1",
233233
"true", "test -f x", "seq 1 5", "sleep 1",
234+
// common modern read-only CLIs added to the allowlist
235+
"fd pattern", "eza -la", "delta a b", "procs", "duf", "dust", "htop",
234236
}
235237
for _, cmd := range safe {
236238
if got := Classify(cmd); got != Safe {

0 commit comments

Comments
 (0)