diff --git a/careful/bin/check-careful.sh b/careful/bin/check-careful.sh index d9c39e48c..3103289fd 100755 --- a/careful/bin/check-careful.sh +++ b/careful/bin/check-careful.sh @@ -26,7 +26,19 @@ fi CMD_LOWER=$(printf '%s' "$CMD" | tr '[:upper:]' '[:lower:]') # --- Check for safe exceptions (rm -rf of build artifacts) --- -if printf '%s' "$CMD" | grep -qE 'rm\s+(-[a-zA-Z]*r[a-zA-Z]*\s+|--recursive\s+)' 2>/dev/null; then +# Only whitelist when the command is a SINGLE rm invocation. The target +# extraction below greedily matches the LAST `rm ` in the string, so a chained +# command like `rm -rf /; rm -rf node_modules` would be judged solely by its +# final (safe) targets and wave through the destructive earlier `rm -rf /`. +# When any shell separator is present, skip the shortcut and fall through to the +# destructive-pattern check, which warns on any recursive rm. +HAS_SEPARATOR=false +case "$CMD" in + # Real separators, plus JSON-escaped newlines (\n / \r) which survive the + # grep extraction path as literal two-char sequences and still mark a chain. + *';'*|*'|'*|*'&'*|*$'\n'*|*$'\r'*|*'\n'*|*'\r'*) HAS_SEPARATOR=true ;; +esac +if [ "$HAS_SEPARATOR" = false ] && printf '%s' "$CMD" | grep -qE 'rm\s+(-[a-zA-Z]*r[a-zA-Z]*\s+|--recursive\s+)' 2>/dev/null; then SAFE_ONLY=true RM_ARGS=$(printf '%s' "$CMD" | sed -E 's/.*rm[[:space:]]+(-[a-zA-Z]+[[:space:]]+)*//;s/--recursive[[:space:]]*//') for target in $RM_ARGS; do diff --git a/test/hook-scripts.test.ts b/test/hook-scripts.test.ts index f1ffe1239..5fc70583c 100644 --- a/test/hook-scripts.test.ts +++ b/test/hook-scripts.test.ts @@ -96,6 +96,31 @@ describe('check-careful.sh', () => { expect(output.permissionDecision).toBe('ask'); expect(output.message).toContain('recursive delete'); }); + + // Regression: the safe-exception extracts targets from only the LAST `rm` in + // the command (greedy match), so a chain that ends in a safe target must not + // wave through a destructive earlier rm. The shortcut only applies to a + // single rm invocation; any shell separator falls through to the warning. + test('rm -rf /; rm -rf node_modules warns (semicolon chain, dangerous first)', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf /; rm -rf node_modules')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('recursive delete'); + }); + + test('rm -rf /etc/data && rm -rf dist warns (&& chain, dangerous first)', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf /etc/data && rm -rf dist')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('recursive delete'); + }); + + test('rm -rf node_modules; rm -rf /home/user/data warns (safe first, dangerous last)', () => { + const { exitCode, output } = runHook(CAREFUL_SCRIPT, carefulInput('rm -rf node_modules; rm -rf /home/user/data')); + expect(exitCode).toBe(0); + expect(output.permissionDecision).toBe('ask'); + expect(output.message).toContain('recursive delete'); + }); }); // --- SQL destructive commands ---