Skip to content

Fix unsafe bash command replacement - use token-by-token matching#56

Merged
yannrichet merged 1 commit intomainfrom
fix-fz_shell_path-first
Nov 27, 2025
Merged

Fix unsafe bash command replacement - use token-by-token matching#56
yannrichet merged 1 commit intomainfrom
fix-fz_shell_path-first

Conversation

@yannrichet
Copy link
Member

Summary

Fixes unsafe bash command replacement in fz/shell.py that could incorrectly replace "bash" when it's part of URIs, file paths, or filenames. Implements safe token-by-token replacement that only replaces standalone "bash" command.

Problem

The previous implementation used simple string replacement or regex patterns that could incorrectly replace "bash" in:

  • URIs: sh://C:/dir/bash.exe
  • File paths: /usr/bin/bash, C:\msys64\usr\bin\bash.exe
  • Filenames: bash.sh, mybash
  • Variable names: BASH_VERSION, womealphbash

Solution

Implemented token-by-token safe replacement in fz/shell.py:

def safe_replace_bash(part):
    # Only replace if the entire part is exactly 'bash' (case-insensitive)
    if part.lower() == 'bash':
        return executable
    return part
command = [safe_replace_bash(s) for s in command_parts]

This approach:

  • Splits command into tokens first
  • Only replaces when entire token equals "bash" (case-insensitive)
  • Prevents partial replacements in paths/URIs/filenames
  • Clear and explicit logic, easy to understand

Changes

Code Changes

  • fz/shell.py (lines 198-208): Replaced unsafe replacement with safe_replace_bash() function
  • fz/shell.py (lines 371-445): replace_commands_in_string() already uses safe regex patterns

Test Coverage

  • tests/test_shell_path.py: Added TestSafeBashReplacement class with 3 comprehensive test methods:
    • test_bash_replacement_only_standalone_word() - Integration test
    • test_safe_replace_bash_function_logic() - Unit test with 12 test cases
    • test_command_parsing_preserves_non_bash_tokens() - 5 command parsing scenarios

Test Cases Covered

✅ Should NOT be replaced:

  • "bash.exe" - bash with extension
  • "mybash" - bash as part of word
  • "womealphbash" - bash as suffix
  • "sh://C:/dir/bash.exe" - bash in URI path
  • "/usr/bin/bash" - bash in Unix path
  • "C:\msys64\usr\bin\bash.exe" - bash in Windows path
  • "bash.sh" - bash with file extension
  • "BASH_VERSION" - bash in variable name
  • "bashrc", ".bashrc" - bash in config files

✅ SHOULD be replaced:

  • "bash" - standalone command
  • "BASH", "Bash" - case-insensitive matching
  • "bash script.sh" - bash as first token
  • "run bash -c test" - bash in middle of command

Testing

All 28 tests in test_shell_path.py pass:

24 passed, 4 skipped (Windows-specific tests on Linux)

Codebase Audit

Performed comprehensive search for unsafe replacements:

  • ✅ No unsafe .replace('bash') in source code
  • ✅ No unsafe .replace('grep'), .replace('awk'), etc.
  • ✅ All shell command replacements use safe patterns
  • replace_commands_in_string() method already uses proper regex word boundaries

Related

This PR is rebased on main and includes the merge of safe bash replacement improvements.

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@yannrichet yannrichet merged commit 1e75806 into main Nov 27, 2025
17 of 18 checks passed
yannrichet-asnr pushed a commit that referenced this pull request Feb 13, 2026
Squash-merge of implement-algorithms branch onto current main (v0.9.1).
Resolved conflicts in:
- fz/__init__.py: Added both fzl and fzd exports
- fz/core.py: Merged imports, kept callbacks from main + added fzd functions
- fz/helpers.py: Kept main's format_time (Windows bash moved to shell.py)
- fz/cli.py: Kept fzl list command + added algorithm install subcommands
- fz/shell.py: Kept main's safer regex replacement from #56
- README.md: Listed both fzl and fzd, kept improved env var docs
- tests/test_cli_commands.py: Added new test methods from PR
- Removed CLAUDE.md (moved to claude/ dir on main)
- Removed setup.py (replaced by pyproject.toml on main)
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.

1 participant