Skip to content

Commit 26a3a0b

Browse files
committed
fix(security): trim allowlist/denylist patterns to prevent config-side bypass (M4)
ActionForCommand trims the command string before matching, but the allowlist and denylist patterns from the user's config were never trimmed. If a user accidentally included trailing whitespace in a pattern (e.g. via copy-paste), the match would silently fail. Fix: apply strings.TrimSpace() to patterns before comparing, so config-side whitespace is ignored just like command-side whitespace. Also corrected the Denylist struct doc: it says "Exact match only" but the code uses strings.HasPrefix (prefix match), which is the intended behavior since the deny list is for broad-stroke blocking. Fixes M4 from security audit.
1 parent b62bb8d commit 26a3a0b

2 files changed

Lines changed: 31 additions & 7 deletions

File tree

internal/danger/classifier.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ type DangerousConfig struct {
217217
Allowlist []string `json:"allowlist,omitempty"`
218218

219219
// Denylist is a list of command strings that are always denied,
220-
// regardless of their risk classification. Exact match only.
220+
// regardless of their risk classification. Prefix match (after trimming).
221221
Denylist []string `json:"denylist,omitempty"`
222222

223223
// DefaultAction is the global default action applied to ALL risk classes
@@ -279,22 +279,22 @@ func (c *DangerousConfig) ActionFor(cls RiskClass) Action {
279279
}
280280

281281
// ActionForCommand returns the action for a specific command string.
282-
// Allowlist and denylist are checked first (exact match), then falls
283-
// back to the risk-class-based action.
282+
// Allowlist and denylist are checked first (exact match for allowlist,
283+
// prefix match for denylist), then falls back to the risk-class-based action.
284284
func (c *DangerousConfig) ActionForCommand(cmd string) Action {
285285
cmd = strings.TrimSpace(cmd)
286286
if cmd == "" {
287287
return Allow
288288
}
289-
// Allowlist has highest priority
289+
// Allowlist has highest priority — exact match after trimming both sides.
290290
for _, pattern := range c.Allowlist {
291-
if cmd == pattern {
291+
if cmd == strings.TrimSpace(pattern) {
292292
return Allow
293293
}
294294
}
295-
// Denylist is checked before classification
295+
// Denylist is checked before classification — prefix match after trimming.
296296
for _, pattern := range c.Denylist {
297-
if strings.HasPrefix(cmd, pattern) {
297+
if strings.HasPrefix(cmd, strings.TrimSpace(pattern)) {
298298
return Deny
299299
}
300300
}

internal/danger/classifier_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,30 @@ func TestActionForCommand_EmptyCommand(t *testing.T) {
649649
}
650650
}
651651

652+
func TestActionForCommand_PatternTrimmed(t *testing.T) {
653+
// Patterns with trailing whitespace should still match after trimming.
654+
cfg := &DangerousConfig{
655+
Allowlist: []string{"git push origin main "}, // trailing space
656+
Denylist: []string{" rm -rf / "}, // leading + trailing space
657+
}
658+
// Allowlist: trimmed pattern matches trimmed command
659+
if action := cfg.ActionForCommand("git push origin main"); action != Allow {
660+
t.Errorf("allowlist with trailing space should match, got %s", action)
661+
}
662+
// Allowlist: command with trailing space also matches
663+
if action := cfg.ActionForCommand("git push origin main "); action != Allow {
664+
t.Errorf("allowlist should match command with trailing space, got %s", action)
665+
}
666+
// Denylist: trimmed pattern matches trimmed command
667+
if action := cfg.ActionForCommand("rm -rf / --no-preserve-root"); action != Deny {
668+
t.Errorf("denylist with leading space should match, got %s", action)
669+
}
670+
// Denylist: command with trailing space still matches
671+
if action := cfg.ActionForCommand("rm -rf / "); action != Deny {
672+
t.Errorf("denylist should match command with trailing space, got %s", action)
673+
}
674+
}
675+
652676
func TestParseAction(t *testing.T) {
653677
if got := parseAction("allow"); got != Allow {
654678
t.Errorf("parseAction(allow) = %s", got)

0 commit comments

Comments
 (0)