security: validate credential commands and expand .gitattributes binary markers (#1291)

* security: validate credential commands and warn on plaintext secrets

- Add validateKeyCmd() in internal/llm/keycmd.go to reject suspicious
  shell patterns (command separators, backticks, redirections) in
  api_key_cmd and auth_token_cmd before the shell executes them.
  Provides a clear diagnostic for tampered or accidentally malformed
  config rather than silently running destructive expressions.

- Add plaintext credential warnings in internal/llm/resolver.go when
  api_key or auth_token are stored as static strings in the config
  file. Credential is still accepted; the warning nudges users toward
  api_key_cmd / auth_token_cmd so secrets are not at rest in JSON.

- Extend .gitattributes with explicit binary markers for web fonts
  (woff, woff2, ttf, otf, eot), WebAssembly (.wasm), compressed
  archives (.zip, .tar, .gz, .tgz), and other image formats (.webp,
  .bmp, .tiff) to prevent CRLF corruption on Windows clones.

* fix(keycmd): tighten validateKeyCmd per review feedback

Three bugs flagged by the code review bot:

- Backtick regex was backtick-space, missing the common case of`cmd`
  with no space. Changed to match any backtick character.

- Command-separator [;|&] and redirection \d*>>?\s*\S were too broad:
  they rejected existing tests that use semicolons, ampersands, and
  2>/dev/null, which are legitimate in helpers that suppress stderr
  or chain commands. Both patterns are removed; only backtick and
  null byte checks remain, each with zero plausible false-positives.

- Custom min() shadowed Go built-in min (Go 1.21+). go.mod declares
  go 1.25.5, so removed the custom helper and inlined the bounds check.

* security: gate plaintext credential warnings and add tests

Address review feedback on the credential-hardening change:

- Emit the plaintext api_key / auth_token warning only when no
  corresponding api_key_cmd / auth_token_cmd is configured. This removes
  the duplicate warning when both a static key and a command were set,
  and limits the nudge to configs that actually lack a secret-manager
  path. The legacy check now also runs after the completeness guard so
  an incomplete llm block no longer warns spuriously.
- Trim the oversized suspiciousPatterns / validateKeyCmd comments to the
  essentials.
- Add tests: validateKeyCmd backtick / NUL rejection (unit and through
  resolveKeyCmd), and the plaintext-warning fire / suppress paths for
  both the provider and legacy config.

* Revert plaintext-credential warning from #1291

The nudge printed a stderr WARNING on every run for the common,
supported case of a static api_key/auth_token in config, producing
unsuppressable noise with no functional benefit and risking CI stderr
assertions. Restore silent acceptance of static credentials; the
both-set warning and the keycmd backtick/NUL validation are unchanged.

---------

Co-authored-by: kite <[email protected]>

.gitattributes

@@ -1,9 +1,30 @@
 * text=auto eol=lf
 
-*.png binary
-*.jpg binary
-*.jpeg binary
-*.gif binary
-*.ico binary
-*.jar binary
+*.png   binary
+*.jpg   binary
+*.jpeg  binary
+*.gif   binary
+*.ico   binary
 *.tiktoken binary
+
+# Web fonts — must never receive CRLF normalisation
+*.woff  binary
+*.woff2 binary
+*.ttf   binary
+*.otf   binary
+*.eot   binary
+
+# WebAssembly bytecode
+*.wasm  binary
+
+# Compressed archives
+*.zip   binary
+*.jar   binary
+*.tar   binary
+*.gz    binary
+*.tgz   binary
+
+# Other binary image formats
+*.webp  binary
+*.bmp   binary
+*.tiff  binary

internal/llm/keycmd.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"regexp"
 	"strings"
 	"time"
 )
@@ -30,6 +31,40 @@ const keyCmdMaxOutput = 64 << 10
 // reaches the caller: cappedBuffer.overflow is what produces the error message.
 var errKeyCmdOutputTooLarge = errors.New("credential command output exceeds cap")
 
+// suspiciousPatterns matches shell constructs that never appear in a legitimate
+// credential-helper invocation (op, aws, pass, keychain, ...) and so signal
+// config tampering or an accidental compound command. This is defence-in-depth,
+// not a sandbox: api_key_cmd runs arbitrary user config by design, so the goal
+// is only to surface accidental misconfiguration. Command separators (; && || &)
+// and redirections (2>/dev/null) are intentionally omitted -- they appear in
+// documented, tested helper commands and would produce false positives.
+var suspiciousPatterns = []*regexp.Regexp{
+	regexp.MustCompile("`"),    // backtick command substitution
+	regexp.MustCompile(`\x00`), // embedded NUL byte
+}
+
+// validateKeyCmd returns an error when cmd contains patterns that are almost
+// certainly not part of a legitimate credential-helper invocation. It is called
+// before the shell runs so that tampered or malformed config produces a clear
+// diagnostic instead of silently executing destructive commands.
+func validateKeyCmd(cmd, label string) error {
+	for _, re := range suspiciousPatterns {
+		if loc := re.FindStringIndex(cmd); loc != nil {
+			end := loc[1] + 8
+			if end > len(cmd) {
+				end = len(cmd)
+			}
+			return fmt.Errorf(
+				"%s contains a suspicious shell pattern at offset %d (%q); "+
+					"api_key_cmd / auth_token_cmd must be a single credential-helper "+
+					"invocation (e.g. \"op read op://vault/item\"), not a compound shell expression",
+				label, loc[0], cmd[loc[0]:end],
+			)
+		}
+	}
+	return nil
+}
+
 // cappedBuffer collects at most max bytes and records whether more were offered.
 // Refusing the write makes os/exec's copier close the pipe, so a runaway command
 // (`cat /dev/urandom`) dies of SIGPIPE instead of growing our heap without bound.
@@ -57,6 +92,9 @@ func (b *cappedBuffer) Write(p []byte) (int, error) {
 // fallback. The resolved credential is used in memory only and is never written
 // to config or logged.
 func resolveKeyCmd(cmd, label string) (string, error) {
+	if err := validateKeyCmd(cmd, label); err != nil {
+		return "", err
+	}
 	ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout)
 	defer cancel()
 

internal/llm/keycmd_test.go

@@ -45,6 +45,10 @@ func TestResolveKeyCmd(t *testing.T) {
 		// takes the edges).
 		{name: "interior tab kept", cmd: "printf 'sk-a\\tb\\n'", want: "sk-a\tb"},
 		{name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"},
+		// validateKeyCmd runs before the shell: a backtick or a literal NUL byte in
+		// the command string is rejected with a diagnostic, never executed.
+		{name: "backtick substitution rejected", cmd: "printf %s `id`", wantErr: "suspicious shell pattern"},
+		{name: "nul byte in command rejected", cmd: "printf %s\x00", wantErr: "suspicious shell pattern"},
 		// Boundary: exactly the cap is fine, one byte more is refused. The child
 		// dies of SIGPIPE as soon as we stop accepting, so this stays fast.
 		{name: "output exactly at cap", cmd: "head -c 65536 /dev/zero | tr '\\0' a", want: strings.Repeat("a", keyCmdMaxOutput)},
@@ -73,6 +77,40 @@ func TestResolveKeyCmd(t *testing.T) {
 	}
 }
 
+func TestValidateKeyCmd(t *testing.T) {
+	label := `api_key_cmd for provider "x"`
+
+	// Legitimate helper invocations pass untouched, including ones that use the
+	// separators and redirections deliberately left out of suspiciousPatterns.
+	for _, cmd := range []string{
+		"op read op://vault/item/credential",
+		"aws secretsmanager get-secret-value --query SecretString --output text",
+		"pass show api/openai 2>/dev/null",
+		`read -r x; printf %s "$x"`,
+	} {
+		if err := validateKeyCmd(cmd, label); err != nil {
+			t.Errorf("validateKeyCmd(%q) = %v, want nil", cmd, err)
+		}
+	}
+
+	// A backtick is reported with its byte offset and the source label so a
+	// tampered config produces an actionable diagnostic.
+	err := validateKeyCmd("printf %s `id`", label)
+	if err == nil {
+		t.Fatal("validateKeyCmd with backtick = nil, want error")
+	}
+	for _, want := range []string{"suspicious shell pattern", "offset 10", label} {
+		if !strings.Contains(err.Error(), want) {
+			t.Errorf("error %q does not contain %q", err.Error(), want)
+		}
+	}
+
+	// A literal NUL byte in the command string is rejected too.
+	if err := validateKeyCmd("printf %s\x00", label); err == nil {
+		t.Error("validateKeyCmd with NUL byte = nil, want error")
+	}
+}
+
 func TestResolveKeyCmd_Timeout(t *testing.T) {
 	origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay
 	keyCmdTimeout = 50 * time.Millisecond