Skip to content

Conversation

@junhoyeo
Copy link
Contributor

@junhoyeo junhoyeo commented Dec 14, 2025

Problem

Screenshot 2025-12-15 at 4 31 13 AM

The interactive-bash-blocker hook was producing false positives by blocking commands containing common words like:

  • "startup"
  • "support"
  • "result"
  • "resume"
  • "issue"

This happened because STDIN_REQUIRING_COMMANDS used .includes("su") which matched any string containing "su" as a substring.

Example of False Positive

# This was incorrectly blocked:
git commit -m "Add support for new feature"
#                  ^^^
#                  "su" matched here!

Root Cause

// constants.ts
export const STDIN_REQUIRING_COMMANDS = [
  "passwd",
  "su",        // <-- This innocent "su"...
  "sudo -S",
  // ...
]

// index.ts  
for (const cmd of STDIN_REQUIRING_COMMANDS) {
  if (normalizedCmd.includes(cmd)) {  // <-- ...matches ANYWHERE in the string
    // BLOCKED!
  }
}

Solution

Changed from string array with .includes() to regex patterns with word boundaries (\b):

// Before
export const STDIN_REQUIRING_COMMANDS = [
  "passwd",
  "su",
  "sudo -S",
  "gpg --gen-key",
  "ssh-keygen",
]

// After
export const STDIN_REQUIRING_PATTERNS = [
  /\bpasswd\b/,
  /\bsu\b(?!\s*[|&;]|\s+-c)/,        // Won't match "support", allows "su -c"
  /\bsudo\s+-S\b/,
  /\bgpg\s+--gen-key\b/,
  /\bssh-keygen\b(?!\s+.*-[fNPqy])/,  // Allows non-interactive flags
]

Pattern Details

Pattern Matches Doesn't Match
\bsu\b(?!\s*[&|;]|\s+-c) su, su -, su root support, startup, su -c "cmd"
\bssh-keygen\b(?!\s+.*-[fNPqy]) ssh-keygen ssh-keygen -f key -N ""

Testing

✓ echo "startup toast"           (allowed - was incorrectly blocked)
✓ git commit -m "support feature" (allowed - was incorrectly blocked)
✓ echo "result is good"          (allowed - was incorrectly blocked)
✓ su                             (blocked - correct)
✓ su root                        (blocked - correct)
✓ su -c "whoami"                 (allowed - non-interactive mode)
✓ ssh-keygen -f mykey -N ""      (allowed - non-interactive flags)

14/14 tests passed

Files Changed

File Description
constants.ts STDIN_REQUIRING_COMMANDSSTDIN_REQUIRING_PATTERNS with regex
index.ts Use .test() instead of .includes()

…d matches

Changed STDIN_REQUIRING_COMMANDS from string .includes() to regex patterns

with word boundaries for accurate command detection.
@junhoyeo junhoyeo changed the title fix(interactive-bash-blocker): prevent false positives on partial word matches fix(interactive-bash-blocker): prevent false positives on partial word matches (URGENT) Dec 14, 2025
@junhoyeo junhoyeo changed the title fix(interactive-bash-blocker): prevent false positives on partial word matches (URGENT) fix(interactive-bash-blocker): prevent false positives on partial word matches (CRITICAL) Dec 14, 2025
@code-yeongyu
Copy link
Owner

Analysis: A More Robust Approach

Thanks for identifying this false positive issue! After deep analysis, I'd like to propose a fundamentally different approach.

The Problem with Regex-based Detection

Perfect static detection of interactive commands is theoretically impossible (similar to the Halting Problem):

Approach Limitation
Regex patterns False positives/negatives inevitable
Whitelist/Blacklist Can't handle new commands/flag combinations
Word boundaries Still misses edge cases (e.g., ssh-keygen -t ed25519 -C "email" blocked incorrectly)

Proposed Solution: Environment Configuration Instead of Blocking

Rather than blocking commands, we should configure the environment as non-interactive:

// Instead of pattern matching and throwing errors...
const NON_INTERACTIVE_ENV = {
  CI: 'true',
  DEBIAN_FRONTEND: 'noninteractive', 
  GIT_TERMINAL_PROMPT: '0',
  SSH_ASKPASS: '/bin/false',
}

// Wrap command with environment + stdin redirect
output.args.command = `${envPrefix} ${command} < /dev/null`

Why This Is Better

Aspect Regex Blocking Environment Config
False positives High (startup, support) None
False negatives Possible (new commands) Auto-handled
Maintenance Need to add patterns Zero maintenance
Error messages "Blocked" Natural failure
Industry standard Custom impl CI/CD standard

Example Behavior

# Before (regex blocking)
$ su root
> "Blocked: Command requires stdin interaction"

# After (environment config)  
$ CI=true su root < /dev/null
> "su: must be run from a terminal" (natural failure)

I'll implement this approach instead. Thanks for bringing attention to the false positive issue!

@code-yeongyu
Copy link
Owner

Closing in favor of environment configuration approach. Will implement a better solution that doesn't rely on regex pattern matching.

code-yeongyu added a commit that referenced this pull request Dec 14, 2025
…onment configuration

Instead of blocking commands via regex pattern matching (which caused false
positives like 'startup', 'support'), now wraps all bash commands with:
- CI=true
- DEBIAN_FRONTEND=noninteractive
- GIT_TERMINAL_PROMPT=0
- stdin redirected to /dev/null

TUI programs (text editors, system monitors, etc.) are still blocked as they
require full PTY. Other interactive commands now fail naturally when stdin
is unavailable.

Closes #55 via alternative approach.
@junhoyeo
Copy link
Contributor Author

Closing in favor of environment configuration approach. Will implement a better solution that doesn't rely on regex pattern matching.

👍👍👍

@junhoyeo junhoyeo deleted the fix/interactive-bash-blocker-false-positives branch December 15, 2025 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants