Release v1.9.0: Critical production fixes for error handling, state management, and security #16
Conversation
Thavarshan
added
bug
…nd version formats
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bda71b5383
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
phpvm.sh
Outdated
| # Validate version format BEFORE using it in path check (security) | ||
| # This prevents path traversal attacks with malicious version strings | ||
| if ! validate_php_version "$version"; then | ||
| phpvm_err "Invalid PHP version format: $version" | ||
| return "$PHPVM_EXIT_INVALID_ARG" |
There was a problem hiding this comment.
Because phpvm_alias now calls validate_php_version before phpvm_resolve_version, non-numeric keywords such as latest/stable (which phpvm_resolve_version explicitly supports and are documented as version arguments) are rejected with “Invalid PHP version format.” This makes commands like phpvm alias default latest or phpvm alias prod stable fail even though they previously resolved to a concrete version; consider resolving first and then validating the resolved value, or allow these keywords explicitly before the validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces v1.9.0 of phpvm with critical production fixes addressing error handling, state management, and security vulnerabilities discovered during production use. The release focuses on making the tool more robust and secure through proper error propagation, atomic state management, path traversal protection, and package manager abstraction.
Changes:
- Fixed critical error propagation failures in
phpvm_resolve_versionthat caused silent failures in install and use commands - Implemented atomic state management ensuring
active_versionfiles are only written after operations succeed - Added comprehensive path traversal protection for alias operations with input validation before file access
- Introduced package manager abstraction layer (DRY) reducing code duplication across brew/apt/dnf/yum/pacman
- Enhanced lock mechanism with stale lock detection and proper trap restoration for shell integration
- Added 4 new security tests for path traversal protection, bringing total test count to 51 passing tests
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| phpvm.sh | Major refactoring with 500+ line additions: error propagation fixes, state management improvements, security hardening (alias validation), package manager abstraction layer, lock mechanism, helper function consolidation (command_exists, ensure_phpvm_dirs, brew_link_php_unversioned, etc.) |
| tests/03_error_handling.bats | Added 4 security tests validating path traversal protection in alias name validation and phpvm alias command |
| README.md | Updated test command reference from deprecated phpvm test to bats tests/ |
| CONTRIBUTING.md | Removed outdated built-in test documentation, updated to reference BATS tests only |
| CLAUDE.md | Updated development documentation to reflect BATS-only testing approach and new testing commands |
| CHANGELOG.md | Comprehensive v1.9.0 release notes documenting 17 bug fixes, 11 improvements, 2 additions, and 5 internal changes |
| .gitignore | Added notes files to ignore list (notes.md, FEATURE_CHECKLIST.md, IMPLEMENTATION_GUIDE.md) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Symmetrical with install: remove cli+common+fpm packages | ||
| run_with_sudo apt-get remove -y \ | ||
| "php${normalized_version}-cli" \ | ||
| "php${normalized_version}-common" \ | ||
| "php${normalized_version}-fpm" 2> /dev/null || | ||
| run_with_sudo apt-get remove -y "$package_name" |
There was a problem hiding this comment.
The apt uninstall logic tries to remove three packages but falls back to removing just the main package if that fails. However, this could leave orphaned packages (common/fpm) if only the cli package removal succeeds. The 2> /dev/null suppresses errors, and the || fallback means we can't distinguish between "all three failed" vs "some succeeded". Consider checking each package individually or using apt-get remove without error suppression to provide better feedback about what was actually removed.
| # Symmetrical with install: remove cli+common+fpm packages | |
| run_with_sudo apt-get remove -y \ | |
| "php${normalized_version}-cli" \ | |
| "php${normalized_version}-common" \ | |
| "php${normalized_version}-fpm" 2> /dev/null || | |
| run_with_sudo apt-get remove -y "$package_name" | |
| # Symmetrical with install: remove cli+common+fpm packages explicitly | |
| local remove_failed=false | |
| for pkg in \ | |
| "php${normalized_version}-cli" \ | |
| "php${normalized_version}-common" \ | |
| "php${normalized_version}-fpm"; do | |
| if ! run_with_sudo apt-get remove -y "$pkg"; then | |
| remove_failed=true | |
| fi | |
| done | |
| # Also attempt to remove the aggregate package name if any component failed | |
| if [ "$remove_failed" = true ]; then | |
| run_with_sudo apt-get remove -y "$package_name" | |
| fi |
|
|
||
| # Check if lock is stale (process no longer exists) | ||
| if [ -f "$lockdir/pid" ]; then | ||
| lock_pid=$(cat "$lockdir/pid" 2> /dev/null || echo "") |
There was a problem hiding this comment.
The lock mechanism uses cat to read PID file without sanitization. If the PID file contains unexpected content (e.g., newlines, spaces, malicious shell metacharacters), the kill -0 test could behave unexpectedly. Consider using a safer approach with explicit field extraction or validation of PID format before use.
| lock_pid=$(cat "$lockdir/pid" 2> /dev/null || echo "") | |
| # Safely read PID from file and validate it is numeric | |
| if ! IFS= read -r lock_pid 2> /dev/null < "$lockdir/pid"; then | |
| lock_pid="" | |
| fi | |
| # Ensure PID consists only of digits; otherwise ignore it | |
| if ! [[ "$lock_pid" =~ ^[0-9]+$ ]]; then | |
| lock_pid="" | |
| fi |
| # Returns: 0 if valid, 1 if invalid | ||
| is_valid_version_format() { | ||
| local version="$1" | ||
| echo "$version" | command grep -qE '^[0-9]+\.[0-9]+(\.[0-9]+)?$' |
There was a problem hiding this comment.
The function is_valid_version_format still uses echo instead of printf for piping to grep. This is inconsistent with the security improvements made elsewhere in the code (lines 635, 648, 688) where printf was specifically adopted to avoid echo interpretation of strings like -n, -e, or backslash escapes. While version strings are unlikely to contain these patterns, consistency is important for maintainability and defense-in-depth.
| echo "$version" | command grep -qE '^[0-9]+\.[0-9]+(\.[0-9]+)?$' | |
| printf '%s\n' "$version" | command grep -qE '^[0-9]+\.[0-9]+(\.[0-9]+)?$' |
| if echo "$version" | command grep -qE '^[0-9]+\.[0-9]+(\.[0-9]+)?$'; then | ||
| return "$PHPVM_EXIT_SUCCESS" | ||
| if is_valid_version_format "$version"; then | ||
| echo "$version" | command awk -F. '{print $1 "." $2}' |
There was a problem hiding this comment.
The awk script uses echo piping instead of direct command substitution which adds unnecessary overhead. More importantly, this function still uses echo piping where printf would be safer and more consistent with the codebase's security improvements. Consider replacing with printf '%s\n' "$version" | command awk -F. '...'.
| echo "$version" | command awk -F. '{print $1 "." $2}' | |
| printf '%s\n' "$version" | command awk -F. '{print $1 "." $2}' |
| install_php_yum "$normalized_version" || return $? | ||
| ;; | ||
| pacman) | ||
| install_php_pacman "$normalized_version" "$package_name" || return $? |
There was a problem hiding this comment.
The function signature shows install_php_pacman takes two parameters (version and package_name), but the other install functions (brew, apt, dnf, yum) only take version. This inconsistency in the API design makes the abstraction layer less uniform. Consider either making all install functions accept package_name or having install_php_pacman derive package_name internally like the others do.
| if command -v auto_switch_php_version > /dev/null 2>&1; then | ||
| auto_switch_php_version 2> /dev/null || true | ||
| # Initialize environment when sourced (do not exit on failure) | ||
| phpvm_init_if_needed || true |
There was a problem hiding this comment.
When sourced, the script calls phpvm_init_if_needed || true to suppress errors. However, this means that if directory creation or system detection fails, the script continues silently without properly initialized state. Commands run later might fail with confusing errors because PHPVM_DIR or PKG_MANAGER are not set. Consider storing the initialization failure state and having commands check it, or at minimum, emit a warning when initialization fails during sourcing.
| if [ -n "$saved_exit_trap" ]; then | ||
| eval "$saved_exit_trap" |
There was a problem hiding this comment.
The eval usage here is potentially dangerous. While the trap output from trap -p is generally safe, using eval on dynamically stored values could introduce security risks if trap contents are somehow manipulated. Consider using a safer approach without eval, such as reconstructing traps programmatically or validating trap strings before evaluation.
| @@ -1445,7 +2265,8 @@ phpvm_list_aliases() { | |||
| alias_name=$(basename "$alias_file") | |||
| alias_target=$(command cat "$alias_file" 2> /dev/null | command tr -d '[:space:]') | |||
| if [ -n "$pattern" ]; then | |||
| if echo "$alias_name" | command grep -qi "$pattern"; then | |||
| # Use fixed-string literal match to avoid regex interpretation | |||
| if echo "$alias_name" | command grep -Fqi "$pattern"; then | |||
There was a problem hiding this comment.
Line 2269 uses command grep -Fqi (fixed-string case-insensitive match) but the comment on line 2268 says "fixed-string literal match". The -F flag means fixed string (literal), which is correct, but the -i flag for case-insensitive matching may not be appropriate for exact alias matching. If alias names are case-sensitive (as implied by the validation regex ^[a-zA-Z0-9_-]+$), then this grep should not use -i. Consider whether case-insensitive pattern filtering is the intended behavior.
| if echo "$alias_name" | command grep -Fqi "$pattern"; then | |
| if echo "$alias_name" | command grep -Fq "$pattern"; then |
| if [ -n "$MACOS_MAJOR" ] && [ "$MACOS_MAJOR" -ge 12 ]; then | ||
| if [ -d "$HOMEBREW_PREFIX/Cellar/php" ]; then | ||
| phpvm_debug "Linking Homebrew php formula as system default (macOS $MACOS_VERSION)..." | ||
| brew_link_php_unversioned || { | ||
| phpvm_err "Failed to link Homebrew php formula." | ||
| return "$PHPVM_EXIT_ERROR" | ||
| } | ||
| # Only set state AFTER successful switch | ||
| set_active_version "system" || return "$PHPVM_EXIT_FILE_ERROR" | ||
| update_current_symlink || true | ||
| phpvm_echo "Switched to system PHP (Homebrew default)." | ||
| return "$PHPVM_EXIT_SUCCESS" | ||
| fi | ||
|
|
||
| if [ "$version" = "system" ]; then | ||
| # Special case for switching to system PHP | ||
| phpvm_atomic_write "$PHPVM_ACTIVE_VERSION_FILE" "system" || { | ||
| phpvm_err "Failed to write active version file" | ||
| return 1 | ||
| } | ||
| # Remove the current symlink since we're using system PHP | ||
| rm -f "$PHPVM_CURRENT_SYMLINK" | ||
|
|
||
| # Handle different macOS versions | ||
| # Apple removed PHP from macOS starting with macOS Monterey 12.0 | ||
| if [ -n "$MACOS_MAJOR" ] && [ "$MACOS_MAJOR" -ge 12 ]; then | ||
| # macOS 12+ doesn't have built-in PHP, use Homebrew | ||
| if [ -d "$HOMEBREW_PREFIX/Cellar/php" ]; then | ||
| phpvm_debug "Linking Homebrew php formula as system default (macOS $MACOS_VERSION)..." | ||
| brew link php --force --overwrite > /dev/null 2>&1 || { | ||
| phpvm_err "Failed to link Homebrew php formula." | ||
| return 1 | ||
| } | ||
| phpvm_echo "Switched to system PHP (Homebrew default)." | ||
| return 0 | ||
| else | ||
| phpvm_warn "No system PHP available on macOS $MACOS_VERSION. Installing Homebrew PHP..." | ||
| if brew install php > /dev/null 2>&1; then | ||
| phpvm_echo "Installed and switched to system PHP (Homebrew)." | ||
| return 0 | ||
| else | ||
| phpvm_err "Failed to install system PHP via Homebrew." | ||
| return 1 | ||
| fi | ||
| fi | ||
| else | ||
| # macOS 11 and earlier may have built-in PHP | ||
| if [ -x "/usr/bin/php" ]; then | ||
| phpvm_echo "Switched to system PHP (built-in macOS PHP)." | ||
| return 0 | ||
| elif [ -d "$HOMEBREW_PREFIX/Cellar/php" ]; then | ||
| phpvm_debug "Linking Homebrew php formula as system default..." | ||
| brew link php --force --overwrite > /dev/null 2>&1 || { | ||
| phpvm_err "Failed to link Homebrew php formula." | ||
| return 1 | ||
| } | ||
| phpvm_echo "Switched to system PHP (Homebrew default)." | ||
| return 0 | ||
| elif command -v php > /dev/null 2>&1; then | ||
| phpvm_echo "Switched to system PHP." | ||
| return 0 | ||
| else | ||
| phpvm_echo "Switched to system PHP." | ||
| phpvm_warn "No system PHP found. You may need to install PHP with 'brew install php' or switch to a specific version." | ||
| return 0 | ||
| fi | ||
| phpvm_warn "No system PHP available on macOS $MACOS_VERSION. Installing Homebrew PHP..." | ||
| if brew install php > /dev/null 2>&1; then | ||
| set_active_version "system" || return "$PHPVM_EXIT_FILE_ERROR" | ||
| update_current_symlink || true | ||
| phpvm_echo "Installed and switched to system PHP (Homebrew)." | ||
| return "$PHPVM_EXIT_SUCCESS" | ||
| fi | ||
| phpvm_err "Failed to install system PHP via Homebrew." | ||
| return "$PHPVM_EXIT_ERROR" |
There was a problem hiding this comment.
The function returns $PHPVM_EXIT_SUCCESS on line 1769 after successfully linking, but then continues to execute lines 1772-1780 which will also execute if the condition on line 1759 is true but the directory check on line 1759 fails. This creates a logic flow issue where success messages could be followed by failure handling. The return statement on line 1769 should prevent execution of subsequent code blocks, but the structure is confusing. Consider restructuring with else-if blocks for clarity.
| if command -v find_phpvmrc > /dev/null 2>&1 && find_phpvmrc > /dev/null 2>&1; then | ||
| if command -v auto_switch_php_version > /dev/null 2>&1; then | ||
| auto_switch_php_version 2> /dev/null || true | ||
| fi |
There was a problem hiding this comment.
The auto-switch logic checks command -v find_phpvmrc and command -v auto_switch_php_version even though these functions are defined in the same file that was just sourced. These checks should never fail unless the script is corrupted. This defensive programming is good, but consider whether it's necessary given that PHPVM_FUNCTIONS_LOADED is already set to true on line 2990. If you keep the checks, consider adding error messages if they fail to help debug script corruption issues.
| if command -v find_phpvmrc > /dev/null 2>&1 && find_phpvmrc > /dev/null 2>&1; then | |
| if command -v auto_switch_php_version > /dev/null 2>&1; then | |
| auto_switch_php_version 2> /dev/null || true | |
| fi | |
| if command -v find_phpvmrc > /dev/null 2>&1; then | |
| if find_phpvmrc > /dev/null 2>&1; then | |
| if command -v auto_switch_php_version > /dev/null 2>&1; then | |
| auto_switch_php_version 2> /dev/null || true | |
| else | |
| phpvm_err "auto_switch_php_version function not found - phpvm.sh may be corrupted" | |
| fi | |
| fi | |
| else | |
| phpvm_err "find_phpvmrc function not found - phpvm.sh may be corrupted" |
Purpose
Release v1.9.0 with critical production fixes for error handling, state management, and security hardening.
This release addresses multiple issues discovered during production use:
install_phpanduse_php_versionswitch_to_system_phpbefore operations completeApproach
Error Propagation
phpvm_resolve_versionoutput and check exit status before proceedingState Management
switch_to_system_phpnow only writesactive_version=systemafter brew link succeedsupdate_current_symlinknow usescommand -v phpto get actual resolved pathPackage Symmetry
php{version}-cli,php{version}-common, andphp{version}-fpmsymmetrically with installAlias Directory
Symlink Robustness
Security Hardening
phpvm_validate_alias_name()validation before any file path operations inphpvm_alias()phpvm_resolve_version()before accessing alias filesOpen Questions and Pre-Merge TODOs
Learning
Research
$?)References