fix(careful): a pipe, process substitution or parameter expansion after the message unmasks it; redirections and comments parse like bash (#1060)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

careful/bin/mask-commit-message.awk

@@ -6,16 +6,18 @@
 #
 # Only git commit's own arguments are masked: the scanner splits the command
 # at unquoted ; & | ( ) and newlines, and masks a message flag only in a
-# segment whose first word is git and that has a commit word after it. A
-# commit piped into another command is printed unchanged, because git prints
-# the message on stdout. Quotes and backslashes follow bash: a backslash
-# escapes the next character outside single quotes, and nothing inside single
-# quotes. An unterminated quote or escape prints the command unchanged.
+# segment whose first word is git and that has a commit word after it. The &
+# of a redirection (2>&1, &>) does not split. Any pipe after a masked message
+# prints the command unchanged, because git prints the message on stdout.
+# Quotes, backslashes and comments follow bash: a backslash escapes the next
+# character outside single quotes, and nothing inside single quotes; an
+# unquoted # starting a word runs to the end of its line. An unterminated
+# quote or escape prints the command unchanged.
 #
 # A message written as "$(cat <<'EOF' ... EOF )" (a quoted delimiter, so the
-# body is never expanded) is masked whole. Any other command substitution,
-# backtick, $'...' string or heredoc changes how bash reads the quotes after
-# it, so the command is printed unchanged.
+# body is never expanded) is masked whole. Any other command, parameter or
+# process substitution, backtick, $'...' string or heredoc changes how bash
+# reads the quotes after it, so the command is printed unchanged.
 # heredoc_msg S N I — S[I] opens a double-quoted "$(cat <<'DELIM' ... DELIM )"
 # message: the index after its closing quote, else 0.
 function heredoc_msg(s, n, i,    p, rest, strip, delim, e, line) {
@@ -77,20 +79,26 @@ function flag(s, i,    rest, m) {
 END {
   s = cmd
   n = length(s); sq = 0; dq = 0; bs = 0; i = 1
-  atword = 1; words = 0; git = 0; commit = 0; piped = 0; unsure = 0
+  atword = 1; words = 0; git = 0; commit = 0; masked = 0; piped = 0; unsure = 0
   while (i <= n) {
     c = substr(s, i, 1)
     if (bs) { result = result c; bs = 0; i++; continue }
     if (sq) { if (c == "'") sq = 0; result = result c; i++; continue }
-    if (c == "`" || substr(s, i, 2) == "$(") unsure = 1
+    two = substr(s, i, 2)
+    if (c == "`" || two == "$(" || two == "${") unsure = 1
     if (dq) {
       if (c == "\\") bs = 1
       else if (c == "\"") dq = 0
       result = result c; i++; continue
     }
-    if (substr(s, i, 2) == "<<" || substr(s, i, 2) == "$'") unsure = 1
+    if (two == "<<" || two == "$'" || two == "<(" || two == ">(") unsure = 1
+    if (atword && c == "#") {
+      while (i <= n && substr(s, i, 1) != "\n") { result = result substr(s, i, 1); i++ }
+      continue
+    }
+    if (c == "&" && (substr(s, i - 1, 1) ~ /[<>]/ || substr(s, i + 1, 1) == ">")) { atword = 0; result = result c; i++; continue }
     if (c ~ /[;&|()\n]/) {
-      if (c == "|" && commit && substr(s, i + 1, 1) != "|") piped = 1
+      if (c == "|" && masked && substr(s, i + 1, 1) != "|" && substr(s, i - 1, 1) != "|") piped = 1
       atword = 1; words = 0; git = 0; commit = 0
       result = result c; i++; continue
     }
@@ -103,6 +111,7 @@ END {
       else if (commit && (len = flag(s, i)) > 0) {
         result = result substr(s, i, len); i += len
         j = handle_msg(s, n, i)
+        if (j > i) masked = 1
         atword = (j == i); i = j
         continue
       }

test/hook-scripts.test.ts

@@ -510,6 +510,31 @@ describe('check-careful.sh', () => {
       expect(output.hookSpecificOutput?.permissionDecisionReason).toContain(reason);
     });
 
+    // git prints the message on stdout, so any pipe after a masked message,
+    // a process substitution or a parameter expansion leaves it unmasked; a
+    // redirection's & is not a separator, and a comment's quote is not a quote.
+    test.each([
+      "git commit -m 'rm -rf build' 2>&1 | sh",
+      "(git commit -m 'rm -rf build') | sh",
+      "git commit -m 'rm -rf build' > >(sh)",
+      'git commit -m "${MSG:-rm -rf build}"',
+    ])('a message that can reach a shell stays unmasked: %j', (command) => {
+      const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
+      expect(exitCode).toBe(0);
+      expect(output.hookSpecificOutput?.permissionDecision).toBe('ask');
+      expect(output.hookSpecificOutput?.permissionDecisionReason).toContain('recursive delete');
+    });
+
+    test.each([
+      "git commit -m 'drop table notes' 2>&1 && git log -1 --oneline",
+      "git commit -m 'drop table notes' || echo failed",
+      "git commit -m 'drop table notes' # it's the docs commit",
+    ])('redirections, || and comments keep the message masked: %j', (command) => {
+      const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput(command));
+      expect(exitCode).toBe(0);
+      expect(output.hookSpecificOutput?.permissionDecision).toBeUndefined();
+    });
+
     test('a partial install without the masking program still asks on the message text', () => {
       const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-careful-nomask-'));
       try {