Skip to content

Commit 677396c

Browse files
committed
fix(secure-cli): prevent deny_verbose from blocking --version via substring match
matchesBinaryDeny used unanchored regex on joined args, causing `-v` pattern to false-positive on `--version`. Split deny_verbose into matchesBinaryVerbose with start-anchored per-arg matching: `-v` blocks `-v`, `-vv`, `-v=1` but not `--version`. deny_args keeps joined matching for multi-token patterns.
1 parent 221cd78 commit 677396c

2 files changed

Lines changed: 124 additions & 3 deletions

File tree

internal/tools/credentialed_exec.go

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ func resolveAndMatchBinary(binaryName string, configPath *string) (string, error
140140
}
141141

142142
// matchesBinaryDeny checks if the joined args string matches any per-binary deny pattern.
143+
// Used for deny_args where patterns span multiple args (e.g. `auth\s+login`, `repo\s+delete`).
143144
// Returns the matched pattern string, or empty if allowed.
144145
func matchesBinaryDeny(args []string, denyPatternsJSON json.RawMessage) string {
145146
if len(denyPatternsJSON) == 0 {
@@ -163,6 +164,40 @@ func matchesBinaryDeny(args []string, denyPatternsJSON json.RawMessage) string {
163164
return ""
164165
}
165166

167+
// matchesBinaryVerbose checks each arg token against verbose/debug flag patterns.
168+
// Patterns are anchored at the START of each arg (but not the end), which allows:
169+
// - `-v` to match `-v`, `-vv`, `-vvv` (verbosity escalation), `-v=1`, `-vq` (combined flags)
170+
// - `--verbose` to match `--verbose`, `--verbose=true`
171+
// - `-v` to NOT match `--version` (char 1 is `-`, not `v`)
172+
// - `--verbose` to NOT match `--version` (diverges at char 5)
173+
//
174+
// This is intentional: verbose flags leak sensitive output (tokens in HTTP headers,
175+
// API response bodies, OAuth flows). Start-anchored per-arg matching catches the
176+
// real verbose family without false-positive on safe flags like `--version`.
177+
// Returns the matched pattern string, or empty if allowed.
178+
func matchesBinaryVerbose(args []string, denyPatternsJSON json.RawMessage) string {
179+
if len(denyPatternsJSON) == 0 {
180+
return ""
181+
}
182+
var patterns []string
183+
if err := json.Unmarshal(denyPatternsJSON, &patterns); err != nil || len(patterns) == 0 {
184+
return ""
185+
}
186+
for _, p := range patterns {
187+
re, err := regexp.Compile("^(?:" + p + ")")
188+
if err != nil {
189+
slog.Warn("secure_cli.invalid_deny_pattern", "pattern", p, "error", err)
190+
continue
191+
}
192+
for _, arg := range args {
193+
if re.MatchString(arg) {
194+
return p
195+
}
196+
}
197+
}
198+
return ""
199+
}
200+
166201
// executeCredentialed runs a CLI command in Direct Exec Mode (no shell).
167202
// Credentials are injected as env vars into the child process only.
168203
// rawCommand is the original command string before shell-word parsing (preserves quoting).
@@ -196,8 +231,9 @@ func (t *ExecTool) executeCredentialed(ctx context.Context, cred *store.SecureCL
196231
if p := matchesBinaryDeny(args, cred.DenyArgs); p != "" {
197232
return credentialedDenyError(binary, args, p)
198233
}
199-
// Per-binary verbose deny check (deny_verbose)
200-
if p := matchesBinaryDeny(args, cred.DenyVerbose); p != "" {
234+
// Per-binary verbose deny check (deny_verbose) — per-arg start-anchored match
235+
// so `-v` blocks `-v`/`-vv`/`-v=1` but not `--version`.
236+
if p := matchesBinaryVerbose(args, cred.DenyVerbose); p != "" {
201237
return credentialedDenyError(binary, args, p)
202238
}
203239

internal/tools/credentialed_exec_test.go

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package tools
22

3-
import "testing"
3+
import (
4+
"encoding/json"
5+
"testing"
6+
)
47

58
func TestDetectShellOperators(t *testing.T) {
69
tests := []struct {
@@ -120,3 +123,85 @@ func TestParseCommandBinary(t *testing.T) {
120123
})
121124
}
122125
}
126+
127+
// TestMatchesBinaryVerbose verifies start-anchored per-arg matching for
128+
// deny_verbose patterns. Regression guard: `-v` must NOT false-positive on
129+
// `--version` (used by the system to probe CLI availability), but MUST still
130+
// block real verbose flags (`-v`, `-vv`, `-v=1`, `--verbose=true`) to prevent
131+
// leakage of tokens/request bodies via verbose output.
132+
func TestMatchesBinaryVerbose(t *testing.T) {
133+
ghPatterns, _ := json.Marshal([]string{"--verbose", "-v"})
134+
gcloudPatterns, _ := json.Marshal([]string{"--verbosity=debug", "--log-http"})
135+
awsPatterns, _ := json.Marshal([]string{"--debug"})
136+
137+
tests := []struct {
138+
name string
139+
patterns json.RawMessage
140+
args []string
141+
wantHit bool
142+
}{
143+
// --- regression: safe flags must pass ---
144+
{"gh --version not blocked", ghPatterns, []string{"--version"}, false},
145+
{"gh version subcmd not blocked", ghPatterns, []string{"version"}, false},
146+
{"gh --help not blocked", ghPatterns, []string{"--help"}, false},
147+
{"gh api repos/x not blocked", ghPatterns, []string{"api", "repos/x"}, false},
148+
149+
// --- real verbose flags still blocked ---
150+
{"gh -v blocked", ghPatterns, []string{"-v"}, true},
151+
{"gh --verbose blocked", ghPatterns, []string{"--verbose"}, true},
152+
{"gh -vv blocked (escalation)", ghPatterns, []string{"-vv"}, true},
153+
{"gh -vvv blocked (escalation)", ghPatterns, []string{"-vvv"}, true},
154+
{"gh --verbose=true blocked (equals form)", ghPatterns, []string{"--verbose=true"}, true},
155+
{"gh -v in middle of args blocked", ghPatterns, []string{"api", "-v", "repos/x"}, true},
156+
157+
// --- gcloud patterns: exact flag=value ---
158+
{"gcloud --verbosity=debug blocked", gcloudPatterns, []string{"--verbosity=debug"}, true},
159+
{"gcloud --verbosity=info not blocked", gcloudPatterns, []string{"--verbosity=info"}, false},
160+
{"gcloud --log-http blocked", gcloudPatterns, []string{"--log-http"}, true},
161+
{"gcloud version not blocked", gcloudPatterns, []string{"version"}, false},
162+
163+
// --- aws ---
164+
{"aws --debug blocked", awsPatterns, []string{"--debug"}, true},
165+
{"aws --debugger not blocked-worthy (prefix match)", awsPatterns, []string{"--debugger"}, true}, // acceptable: still debug family
166+
{"aws --version not blocked", awsPatterns, []string{"--version"}, false},
167+
168+
// --- empty / no patterns ---
169+
{"empty patterns", json.RawMessage(nil), []string{"--verbose"}, false},
170+
{"empty args", ghPatterns, []string{}, false},
171+
}
172+
for _, tt := range tests {
173+
t.Run(tt.name, func(t *testing.T) {
174+
got := matchesBinaryVerbose(tt.args, tt.patterns)
175+
if (got != "") != tt.wantHit {
176+
t.Errorf("matchesBinaryVerbose(%v) = %q, wantHit=%v", tt.args, got, tt.wantHit)
177+
}
178+
})
179+
}
180+
}
181+
182+
// TestMatchesBinaryDenyJoinedArgs verifies deny_args keeps joined-string
183+
// matching so multi-token patterns like `auth\s+login` and `repo\s+delete`
184+
// still work.
185+
func TestMatchesBinaryDenyJoinedArgs(t *testing.T) {
186+
ghPatterns, _ := json.Marshal([]string{`auth\s+`, `repo\s+delete`, `secret\s+`})
187+
188+
tests := []struct {
189+
name string
190+
args []string
191+
wantHit bool
192+
}{
193+
{"gh auth login blocked", []string{"auth", "login"}, true},
194+
{"gh repo delete blocked", []string{"repo", "delete", "foo/bar"}, true},
195+
{"gh secret set blocked", []string{"secret", "set", "TOKEN"}, true},
196+
{"gh api repos allowed", []string{"api", "repos/x"}, false},
197+
{"gh repo view allowed", []string{"repo", "view", "foo/bar"}, false},
198+
}
199+
for _, tt := range tests {
200+
t.Run(tt.name, func(t *testing.T) {
201+
got := matchesBinaryDeny(tt.args, ghPatterns)
202+
if (got != "") != tt.wantHit {
203+
t.Errorf("matchesBinaryDeny(%v) = %q, wantHit=%v", tt.args, got, tt.wantHit)
204+
}
205+
})
206+
}
207+
}

0 commit comments

Comments
 (0)