Skip to content
Merged
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ bin/
tmp/
**/*.log.dccache

# Vim swap files
*.swp
*.swo
*~

# Development cache and temporary files
.dccache
*.log
Expand All @@ -60,3 +65,4 @@ tmp/
*_improvements.md
*_notes.md
*_todo.md
TODO.md
Binary file removed .swo
Binary file not shown.
100 changes: 41 additions & 59 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,17 @@ For detailed technical architecture, design patterns, and implementation guideli

**Cross-Package Manager Compatibility**: SysPkg normalizes package states for consistent behavior across different package managers. For example, APT's "config-files" state (packages removed but with configuration files remaining) is normalized to "available" status to match the semantics used by other package managers like YUM and Snap.

## Current Session Todos

@TODO.md

## Project Improvement Roadmap

*Note: To-do list consolidated 2025-05-30 - removed duplicates, feature creep items, and over-engineering. Focused on core security, testing, and platform support.*

### 🔴 High Priority (Security & Critical Bugs) - 15 items
1. **Fix command injection vulnerability** - validate/sanitize package names before exec.Command
2. **Implement input validation helper function** for package names and arguments
1. **Fix command injection vulnerability** - validate/sanitize package names before exec.Command (PR #25)
2. **Implement input validation helper function** for package names and arguments (PR #25)
3. **Fix resource leaks** in error handling paths
4. **Add security scanning with Snyk** to CI/CD pipeline
5. **Review and merge PR #12** - fix GetPackageManager("") panic bug ✅
Expand All @@ -179,64 +183,42 @@ For detailed technical architecture, design patterns, and implementation guideli
10. **Implement CommandBuilder interface (Issue #20)** - Replace direct exec.Command calls with testable CommandBuilder pattern
11. **Add exit code documentation** ✅ - Created comprehensive exit code docs for all package managers

### ✅ CRITICAL INVESTIGATION COMPLETED
**Investigation Results: No design flaw found - tests are correct**
8. **RESOLVED: Tests are correctly testing parser functions** ✅ - `behavior_test.go` tests `ParseFindOutput()` directly, not `Find()` method
9. **RESOLVED: ParseFindOutput() vs Find() distinction clarified** ✅:
- `ParseFindOutput()`: Pure parser, returns all packages as "available" (YUM limitation)
- `Find()`: Enhanced method that adds rpm -q status detection for accurate results
10. **RESOLVED: CommandRunner interface verified** ✅ - Correctly implemented in `enhancePackagesWithStatus()`
11. **RESOLVED: Test execution paths confirmed** ✅ - Tests correctly test parsers, not enhanced methods
12. **RESOLVED: Fixtures validated as authentic** ✅ - Real YUM output that correctly lacks status info
13. **RESOLVED: Interface implementation verified** ✅ - All methods properly implemented and registered
14. **RESOLVED: Created integration tests** ✅ - Added `yum_integration_test.go` demonstrating three-layer testing approach

### 🟡 Medium Priority (Code Quality & Testing) - 8 items
**Testing:**
- **Create integration tests** ✅ - Added `yum_integration_test.go` with three-layer testing approach
- **Document testing strategy** ✅ - Added comprehensive testing documentation to CONTRIBUTING.md
- **Implement CommandRunner dependency injection (Issue #20)** 🚧 - YUM partially implemented, architecture decision updated to Option C (CommandBuilder)
- Add unit tests for snap package manager
- Add unit tests for flatpak package manager
- **APT fixture cleanup and behavior testing** ✅ - Reduced 16→7 fixtures, full test coverage
- **Cross-platform parsing robustness** ✅ - CRLF/whitespace handling, regex optimization
- **YUM fixture analysis and cleanup** ✅ **COMPLETED** (Issue #16) - Following APT pattern:
- ✅ Analyzed YUM fixtures to determine what's needed for comprehensive testing
- ✅ Removed redundant/duplicate files (search-vim-rockylinux.txt)
- ✅ Standardized fixture naming convention (rocky8 vs rockylinux inconsistency)
- ✅ Renamed info-vim-rockylinux.txt → info-vim-installed-rocky8.txt for clarity
- ✅ Added missing edge case fixtures (empty results, not found, clean, refresh)
- ✅ Created comprehensive behavior_test.go following APT fixture pattern
- ✅ Converted YUM tests from inline data to fixture-based tests
- ✅ Verified fixture compatibility and completeness with all tests passing
- **YUM operations implementation** ✅ **COMPLETED** - Comprehensive YUM package manager:
- ✅ Implemented all missing operations: Install, Delete, ListUpgradable, Upgrade, UpgradeAll, AutoRemove
- ✅ Added complete parser functions for all operation outputs
- ✅ Created comprehensive behavior tests covering all operations and edge cases
- ✅ Generated real fixtures using Rocky Linux Docker for authentic test data
- ✅ Documented YUM-specific behaviors and cross-package manager compatibility
- ✅ **YUM Find() status detection** - Implemented rpm -q integration for accurate installation status
- ✅ **Cross-package manager API consistency** - YUM Find() now matches APT behavior exactly
- ✅ All tests passing with 100% security scan clearance

**Documentation:**
- **API and behavior documentation** ✅ - Enhanced interface docs, status normalization, cross-PM compatibility
- **Error handling best practices** ✅ - Fixed ignored errors in documentation examples
- **Accuracy improvements** ✅ - Fixed misleading comments about status handling
- **YUM documentation updates** ✅ - Updated all outdated behavior comments to reflect Find() status detection capabilities

**Code Improvements:**
- Implement context support for cancellation and timeouts
- Create custom error types for better error handling
- Extract common parsing logic to shared utilities (DRY principle)
- Replace magic strings/numbers with named constants
- **Fix APT multi-arch package parsing** (Issue #15) - cosmetic fix for empty package names
### ✅ COMPLETED INVESTIGATIONS (Collapsed)
<details>
<summary>Critical investigation results - tests are correctly implemented</summary>

**Removed from roadmap (2025-05-30):**
- ~~Structured logging~~ (over-engineering for project scope)
- ~~Progress indicators~~ (feature creep for CLI/library)
- ~~Architecture diagrams~~ (low ROI for library project)
- ~~TODO comment fixes~~ (covered by security improvements)
**Investigation Results: No design flaw found - tests are correct**
- ✅ **Parser vs enhanced method distinction clarified**
- ✅ **CommandRunner interface verified**
- ✅ **Test execution paths confirmed**
- ✅ **Fixtures validated as authentic**
- ✅ **Integration tests created**
</details>

### 🟡 Medium Priority & Completed Items (Collapsed)
<details>
<summary>Code quality, testing achievements, and removed items</summary>

**Completed Testing Work:**
- ✅ YUM comprehensive implementation (Issue #16)
- ✅ APT fixture cleanup and behavior testing
- ✅ Integration tests and testing strategy documentation
- ✅ Cross-platform parsing robustness

**Completed Documentation:**
- ✅ API and behavior documentation enhanced
- ✅ Error handling best practices documented
- ✅ YUM documentation updates

**Remaining Code Improvements:**
- Context support for cancellation and timeouts
- Custom error types for better error handling
- Extract common parsing logic (DRY principle)
- Replace magic strings/numbers with constants

**Items Removed from Roadmap (2025-05-30):**
- ~~Structured logging, progress indicators, architecture diagrams~~ (over-engineering/feature creep)
</details>

### 🟢 Low Priority (Platform Support) - 3 items
**New Package Managers:**
Expand Down
27 changes: 27 additions & 0 deletions manager/apt/apt.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ func (a *PackageManager) GetPackageManager() string {

// Install installs the provided packages using the apt package manager.
func (a *PackageManager) Install(pkgs []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate package names to prevent command injection
if err := manager.ValidatePackageNames(pkgs); err != nil {
return nil, err
}

args := append([]string{"install", ArgsFixBroken}, pkgs...)

if opts == nil {
Expand Down Expand Up @@ -125,6 +130,11 @@ func (a *PackageManager) Install(pkgs []string, opts *manager.Options) ([]manage

// Delete removes the provided packages using the apt package manager.
func (a *PackageManager) Delete(pkgs []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate package names to prevent command injection
if err := manager.ValidatePackageNames(pkgs); err != nil {
return nil, err
}

// args := append([]string{"remove", ArgsFixBroken, ArgsPurge, ArgsAutoRemove}, pkgs...)
args := append([]string{"remove", ArgsFixBroken, ArgsAutoRemove}, pkgs...)
if opts == nil {
Expand Down Expand Up @@ -192,6 +202,11 @@ func (a *PackageManager) Refresh(opts *manager.Options) error {

// Find searches for packages matching the provided keywords using the apt package manager.
func (a *PackageManager) Find(keywords []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate keywords to prevent command injection
if err := manager.ValidatePackageNames(keywords); err != nil {
return nil, err
}

args := append([]string{"search"}, keywords...)
cmd := exec.Command("apt", args...)
cmd.Env = append(os.Environ(), ENV_NonInteractive...)
Expand Down Expand Up @@ -229,6 +244,13 @@ func (a *PackageManager) ListUpgradable(opts *manager.Options) ([]manager.Packag

// Upgrade upgrades the provided packages using the apt package manager.
func (a *PackageManager) Upgrade(pkgs []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate package names to prevent command injection
if len(pkgs) > 0 {
if err := manager.ValidatePackageNames(pkgs); err != nil {
return nil, err
}
}

args := []string{"upgrade"}
if len(pkgs) > 0 {
args = append(args, pkgs...)
Expand Down Expand Up @@ -307,6 +329,11 @@ func (a *PackageManager) Clean(opts *manager.Options) error {

// GetPackageInfo retrieves package information for the specified package using the apt package manager.
func (a *PackageManager) GetPackageInfo(pkg string, opts *manager.Options) (manager.PackageInfo, error) {
// Validate package name to prevent command injection
if err := manager.ValidatePackageName(pkg); err != nil {
return manager.PackageInfo{}, err
}

cmd := exec.Command("apt-cache", "show", pkg)
cmd.Env = ENV_NonInteractive
out, err := cmd.Output()
Expand Down
5 changes: 5 additions & 0 deletions manager/apt/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,11 @@ func logDebugPackages(packages map[string]manager.PackageInfo, opts *manager.Opt

// runDpkgQuery executes dpkg-query command and handles errors appropriately
func runDpkgQuery(packageNames []string, opts *manager.Options) ([]byte, error) {
// Validate package names to prevent command injection
if err := manager.ValidatePackageNames(packageNames); err != nil {
return nil, err
}

args := []string{"-W", "--showformat", "${binary:Package} ${Status} ${Version}\n"}
args = append(args, packageNames...)
cmd := exec.Command("dpkg-query", args...)
Expand Down
20 changes: 20 additions & 0 deletions manager/flatpak/flatpak.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ func (a *PackageManager) GetPackageManager() string {

// Install installs the given packages using Flatpak with the provided options.
func (a *PackageManager) Install(pkgs []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate package names to prevent command injection
if err := manager.ValidatePackageNames(pkgs); err != nil {
return nil, err
}

args := append([]string{"install", ArgsFixBroken, ArgsUpsert, ArgsVerbose}, pkgs...)

if opts == nil {
Expand Down Expand Up @@ -104,6 +109,11 @@ func (a *PackageManager) Install(pkgs []string, opts *manager.Options) ([]manage

// Delete removes the given packages using Flatpak with the provided options.
func (a *PackageManager) Delete(pkgs []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate package names to prevent command injection
if err := manager.ValidatePackageNames(pkgs); err != nil {
return nil, err
}

args := append([]string{"uninstall", ArgsFixBroken, ArgsVerbose}, pkgs...)

if opts == nil {
Expand Down Expand Up @@ -154,6 +164,11 @@ func (a *PackageManager) Refresh(opts *manager.Options) error {

// Find searches for packages matching the given keywords using Flatpak with the provided options.
func (a *PackageManager) Find(keywords []string, opts *manager.Options) ([]manager.PackageInfo, error) {
// Validate keywords to prevent command injection
if err := manager.ValidatePackageNames(keywords); err != nil {
return nil, err
}

args := append([]string{"search", ArgsVerbose}, keywords...)

if opts == nil {
Expand Down Expand Up @@ -248,6 +263,11 @@ func (a *PackageManager) UpgradeAll(opts *manager.Options) ([]manager.PackageInf

// GetPackageInfo retrieves package information for a single package using Flatpak with the provided options.
func (a *PackageManager) GetPackageInfo(pkg string, opts *manager.Options) (manager.PackageInfo, error) {
// Validate package name to prevent command injection
if err := manager.ValidatePackageName(pkg); err != nil {
return manager.PackageInfo{}, err
}

cmd := exec.Command(pm, "info", pkg)
cmd.Env = ENV_NonInteractive
out, err := cmd.Output()
Expand Down
83 changes: 83 additions & 0 deletions manager/security.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Package manager provides security utilities for package manager operations
package manager

import (
"errors"
"regexp"
)

// packageNameRegex defines the allowed pattern for package names
// This pattern allows:
// - Letters (a-z, A-Z)
// - Numbers (0-9)
// - Dash/hyphen (-)
// - Underscore (_)
// - Period/dot (.)
// - Plus sign (+)
// - Colon (:) for architecture specifiers (e.g., package:amd64)
// - Forward slash (/) for repository specifiers (e.g., repo/package)
// The pattern requires at least one valid character
var packageNameRegex = regexp.MustCompile(`^[a-zA-Z0-9\-_.+:/]+$`)

// ErrInvalidPackageName is returned when a package name contains invalid characters
var ErrInvalidPackageName = errors.New("invalid package name: contains potentially dangerous characters")

// ValidatePackageName validates that a package name only contains safe characters
// to prevent command injection attacks.
//
// Valid package names may contain:
// - Alphanumeric characters (a-z, A-Z, 0-9)
// - Dash/hyphen (-)
// - Underscore (_)
// - Period/dot (.)
// - Plus sign (+)
// - Colon (:) for architecture specifiers
// - Forward slash (/) for repository specifiers
//
// This function rejects any package names containing:
// - Shell metacharacters (;, |, &, $, `, \, ", ', <, >, (, ), {, }, [, ], *, ?, ~)
// - Whitespace characters
// - Control characters
// - Null bytes
//
// Example valid names:
// - "vim"
// - "libssl1.1"
// - "gcc-9-base"
// - "python3.8"
// - "package:amd64"
// - "repo/package"
//
// Example invalid names:
// - "package; rm -rf /"
// - "package && malicious-command"
// - "package`evil`"
// - "package$(bad)"
func ValidatePackageName(name string) error {
// Check for empty string
if name == "" {
return errors.New("package name cannot be empty")
}

// Check length limit (most package managers have reasonable limits)
if len(name) > 255 {
return errors.New("package name too long (max 255 characters)")
}

// Validate against regex pattern
if !packageNameRegex.MatchString(name) {
return ErrInvalidPackageName
}

return nil
}

// ValidatePackageNames validates multiple package names
func ValidatePackageNames(names []string) error {
for _, name := range names {
if err := ValidatePackageName(name); err != nil {
return err
}
}
return nil
}
Loading