diff --git a/cmd/pypi/pip.go b/cmd/pypi/pip.go index 13ea71b..e7e3c57 100644 --- a/cmd/pypi/pip.go +++ b/cmd/pypi/pip.go @@ -4,7 +4,6 @@ import ( "context" "fmt" - packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1" "github.com/safedep/dry/log" "github.com/safedep/pmg/config" "github.com/safedep/pmg/internal/flows" @@ -52,12 +51,6 @@ func executePipFlow(ctx context.Context, args []string) error { packageResolverConfig.IncludeDevDependencies = config.IncludeDevDependencies packageResolverConfig.PackageInstallTargets = parsedCommand.InstallTargets - extractorConfig := packagemanager.NewDefaultExtractorConfig() - extractorConfig.ExtractorEcosystem = packagev1.Ecosystem_ECOSYSTEM_PYPI - extractorConfig.ExtractorsName = packagemanager.PyPiExtractors - - // extractor := packagemanager.NewExtractor(*extractorConfig) - packageResolver, err := packagemanager.NewPypiDependencyResolver(packageResolverConfig) if err != nil { ui.Fatalf("Failed to create dependency resolver: %s", err) diff --git a/code-review-checklist.md b/code-review-checklist.md deleted file mode 100644 index a056791..0000000 --- a/code-review-checklist.md +++ /dev/null @@ -1,55 +0,0 @@ -# Code Review Checklist: Package Extractor - -## ๐Ÿ” Code Structure & Organization -- [ ] Are the responsibilities clearly separated between configuration and extraction logic? -- [ ] Could the extractor struct be made more testable by extracting interfaces? -- [ ] Is the naming consistent throughout the file (e.g., `ExtractorConfig` vs `extractor`)? -- [ ] Should the global variables (`NpmExtractors`, `PyPiExtractors`) be constants or part of a configuration? - -## ๐Ÿ›ก๏ธ Error Handling & Robustness -- [ ] What happens if `e.Config.context` is nil? Should there be validation? -- [ ] Are all potential error cases from the OSV-Scalibr library handled appropriately? -- [ ] Could the error message in `ExtractManifestFiles()` be more descriptive? -- [ ] What if `scanResult.Inventory.Packages` is nil or empty? - -## ๐Ÿ“Š Data Validation & Edge Cases -- [ ] Should `ExtractorConfig` validate that `ExtractorsName` and `ExtractorType` are compatible? -- [ ] What happens if `ScanDir` doesn't exist or isn't readable? -- [ ] Are there any assumptions about package name/version formats that could break? -- [ ] Should there be limits on the number of packages processed? - -## ๐Ÿš€ Performance & Resource Management -- [ ] Is the scanner being reused efficiently, or should it be cached/pooled? -- [ ] Could memory usage be optimized when processing large package lists? -- [ ] Are there any potential goroutine leaks in the scanning process? -- [ ] Should there be timeout handling for long-running scans? - -## ๐Ÿงช Testing & Maintainability -- [ ] How would you unit test the `ExtractManifestFiles()` method? -- [ ] Are the dependencies (OSV-Scalibr) easily mockable for testing? -- [ ] Could the configuration creation be simplified or made more fluent? -- [ ] Is the code following Go conventions for package structure? - -## ๐Ÿ”ง API Design Questions -- [ ] Should `DefaultExtractorConfig()` return a pointer or a value? -- [ ] Is the `NewExtractor` constructor providing enough validation? -- [ ] Could the extractor support multiple ecosystems in a single scan? -- [ ] Should there be a way to filter or transform packages during extraction? - -## ๐Ÿ’ญ Think About These Scenarios -- [ ] What if a manifest file is corrupted or has unexpected format? -- [ ] How would this code behave with very large codebases (1000+ dependencies)? -- [ ] What happens if the scan is interrupted or cancelled via context? -- [ ] Should there be logging or progress reporting for long scans? - -## ๐ŸŽฏ Next Steps to Explore -1. **Research Question**: Look up Go best practices for factory patterns - is `NewExtractor` following them? -2. **Deep Dive**: Investigate the OSV-Scalibr documentation - what other configuration options might be useful? -3. **Design Pattern**: Consider the Single Responsibility Principle - is this struct doing too much? -4. **Error Handling**: Research Go error wrapping patterns - could `fmt.Errorf` be improved? - -## ๐Ÿค” Questions for Self-Reflection -- How would you explain what this code does to someone unfamiliar with package management? -- If you had to add support for a new package manager, how much code would you need to change? -- What's the most fragile part of this implementation, and why? -- How confident would you feel deploying this code to production? \ No newline at end of file diff --git a/guard/guard.go b/guard/guard.go index 8e05384..d4a45e9 100644 --- a/guard/guard.go +++ b/guard/guard.go @@ -73,6 +73,12 @@ func (g *packageManagerGuard) Run(ctx context.Context, args []string, parsedComm log.Debugf("Running package manager guard with args: %v", args) if !parsedCommand.HasInstallTarget() { + // Check if this is a manifest-based installation + if parsedCommand.ShouldExtractFromManifest() { + log.Debugf("Detected manifest-based installation, extracting packages from manifest files") + return g.handleManifestInstallation(ctx, parsedCommand) + } + log.Debugf("No install target found, continuing execution") return g.continueExecution(ctx, parsedCommand) } @@ -268,3 +274,77 @@ func (g *packageManagerGuard) clearStatus() { g.interaction.ClearStatus() } + +func (g *packageManagerGuard) handleManifestInstallation(ctx context.Context, parsedCommand *packagemanager.ParsedCommand) error { + g.setStatus("Extracting packages from manifest files") + + // Create extractor with appropriate ecosystem + var ecosystem packagev1.Ecosystem + switch g.packageManager.Name() { + case "pip": + ecosystem = packagev1.Ecosystem_ECOSYSTEM_PYPI + case "npm", "pnpm": + ecosystem = packagev1.Ecosystem_ECOSYSTEM_NPM + default: + return fmt.Errorf("unsupported package manager for manifest extraction: %s", g.packageManager.Name()) + } + + extractorConfig := packagemanager.NewDefaultExtractorConfig() + extractorConfig.ExtractorEcosystem = ecosystem + + switch ecosystem { + case packagev1.Ecosystem_ECOSYSTEM_PYPI: + extractorConfig.ExtractorsName = packagemanager.PyPiExtractors + case packagev1.Ecosystem_ECOSYSTEM_NPM: + extractorConfig.ExtractorsName = packagemanager.NpmExtractors + } + + extractor := packagemanager.NewExtractor(*extractorConfig) + + packages, err := extractor.ExtractManifestFiles() + if err != nil { + return fmt.Errorf("failed to extract packages from manifest files: %w", err) + } + + if len(packages) == 0 { + log.Debugf("No packages found in manifest files, continuing execution") + return g.continueExecution(ctx, parsedCommand) + } + + log.Debugf("Extracted %d packages from manifest files", len(packages)) + + // Analyze the extracted packages + g.setStatus(fmt.Sprintf("Analyzing %d packages from manifest files", len(packages))) + + analysisResults, err := g.concurrentAnalyzePackages(ctx, packages) + if err != nil { + return fmt.Errorf("failed to analyze packages: %w", err) + } + + confirmableMalwarePackages := []*analyzer.PackageVersionAnalysisResult{} + for _, result := range analysisResults { + if result.Action == analyzer.ActionBlock { + return g.blockInstallation(result) + } + + if result.Action == analyzer.ActionConfirm { + confirmableMalwarePackages = append(confirmableMalwarePackages, result) + } + } + + if len(confirmableMalwarePackages) > 0 { + confirmed, err := g.getConfirmationOnMalware(ctx, confirmableMalwarePackages) + if err != nil { + return fmt.Errorf("failed to get confirmation on malware: %w", err) + } + + if !confirmed { + return g.blockInstallation(confirmableMalwarePackages...) + } + } + + log.Debugf("No malicious packages found in manifest files, continuing execution") + + g.clearStatus() + return g.continueExecution(ctx, parsedCommand) +} diff --git a/packagemanager/npm.go b/packagemanager/npm.go index 60154d0..debb006 100644 --- a/packagemanager/npm.go +++ b/packagemanager/npm.go @@ -51,7 +51,7 @@ func (npm *npmPackageManager) ParseCommand(args []string) (*ParsedCommand, error command := Command{Exe: npm.Config.CommandName, Args: args} // No command specified - if len(args) < 2 { + if len(args) < 1 { return &ParsedCommand{ Command: command, }, nil @@ -59,8 +59,12 @@ func (npm *npmPackageManager) ParseCommand(args []string) (*ParsedCommand, error // Extract packages from args var packages []string + var isManifestInstall bool + var foundInstallCmd bool + for idx, arg := range args { if slices.Contains(npm.Config.InstallCommands, arg) { + foundInstallCmd = true // All subsequent args are packages except for flags for i := idx + 1; i < len(args); i++ { if strings.HasPrefix(args[i], "-") { @@ -74,8 +78,14 @@ func (npm *npmPackageManager) ParseCommand(args []string) (*ParsedCommand, error } } - // No packages found - if len(packages) == 0 { + // If install command was found but no explicit packages, + // this is a manifest-based installation (install from package.json) + if foundInstallCmd && len(packages) == 0 { + isManifestInstall = true + } + + // No packages found and not a manifest install + if len(packages) == 0 && !isManifestInstall { return &ParsedCommand{ Command: command, }, nil @@ -105,9 +115,17 @@ func (npm *npmPackageManager) ParseCommand(args []string) (*ParsedCommand, error }) } + var manifestFiles []string + if isManifestInstall { + // npm/pnpm installs from package.json by default + manifestFiles = append(manifestFiles, "package.json") + } + return &ParsedCommand{ - Command: command, - InstallTargets: installTargets, + Command: command, + InstallTargets: installTargets, + IsManifestInstall: isManifestInstall, + ManifestFiles: manifestFiles, }, nil } diff --git a/packagemanager/npm_test.go b/packagemanager/npm_test.go index f807fc8..d15b0d6 100644 --- a/packagemanager/npm_test.go +++ b/packagemanager/npm_test.go @@ -121,3 +121,90 @@ func TestNpmParseCommand(t *testing.T) { }) } } + +func TestNpmParseCommand_ManifestInstallation(t *testing.T) { + pm, err := NewNpmPackageManager(DefaultNpmPackageManagerConfig()) + assert.NoError(t, err) + + cases := []struct { + name string + args []string + expectedManifest bool + expectedFiles []string + expectedTargets int + }{ + { + name: "npm install without args (bare install)", + args: []string{"install"}, + expectedManifest: true, + expectedFiles: []string{"package.json"}, + expectedTargets: 0, + }, + { + name: "npm i without args (short form)", + args: []string{"i"}, + expectedManifest: true, + expectedFiles: []string{"package.json"}, + expectedTargets: 0, + }, + { + name: "npm install with explicit package", + args: []string{"install", "react"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 1, + }, + { + name: "npm install with multiple packages", + args: []string{"install", "react", "vue"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 2, + }, + { + name: "npm install with flags but no packages", + args: []string{"install", "--save-dev"}, + expectedManifest: true, + expectedFiles: []string{"package.json"}, + expectedTargets: 0, + }, + { + name: "npm install with mixed args", + args: []string{"install", "react", "--save"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 1, + }, + { + name: "non-install command", + args: []string{"run", "build"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 0, + }, + { + name: "pnpm install without args", + args: []string{"install"}, + expectedManifest: true, + expectedFiles: []string{"package.json"}, + expectedTargets: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + parsed, err := pm.ParseCommand(tc.args) + assert.NoError(t, err) + + assert.Equal(t, tc.expectedManifest, parsed.IsManifestInstall, "IsManifestInstall mismatch") + assert.Equal(t, tc.expectedFiles, parsed.ManifestFiles, "ManifestFiles mismatch") + assert.Equal(t, tc.expectedTargets, len(parsed.InstallTargets), "InstallTargets count mismatch") + + // Test helper methods + assert.Equal(t, tc.expectedManifest, parsed.HasManifestInstall(), "HasManifestInstall mismatch") + + expectedShouldExtract := tc.expectedManifest && tc.expectedTargets == 0 + assert.Equal(t, expectedShouldExtract, parsed.ShouldExtractFromManifest(), "ShouldExtractFromManifest mismatch") + }) + } +} diff --git a/packagemanager/packagemanager.go b/packagemanager/packagemanager.go index fbedd87..fc10be4 100644 --- a/packagemanager/packagemanager.go +++ b/packagemanager/packagemanager.go @@ -30,12 +30,28 @@ type ParsedCommand struct { // Parsed install target if this is an install command InstallTargets []*PackageInstallTarget + + // IsManifestInstall indicates if this is a manifest-based installation + // (e.g., npm install, pip install -r requirements.txt) + IsManifestInstall bool + + // ManifestFiles contains the list of manifest files to install from + // (e.g., ["requirements.txt"] for pip install -r requirements.txt) + ManifestFiles []string } func (pc *ParsedCommand) HasInstallTarget() bool { return len(pc.InstallTargets) > 0 } +func (pc *ParsedCommand) HasManifestInstall() bool { + return pc.IsManifestInstall +} + +func (pc *ParsedCommand) ShouldExtractFromManifest() bool { + return pc.IsManifestInstall && !pc.HasInstallTarget() +} + // PackageManager is the contract for implementing a package manager type PackageManager interface { // Name of the package manager implementation diff --git a/packagemanager/pypi.go b/packagemanager/pypi.go index b437355..77acb52 100644 --- a/packagemanager/pypi.go +++ b/packagemanager/pypi.go @@ -43,24 +43,70 @@ func (pip *pipPackageManager) ParseCommand(args []string) (*ParsedCommand, error } command := Command{Exe: pip.Config.CommandName, Args: args} - if len(args) < 2 { + if len(args) < 1 { return &ParsedCommand{ Command: command, }, nil } var packages []string + var manifestFiles []string + var isManifestInstall bool + var foundInstallCmd bool + for idx, arg := range args { if slices.Contains(pip.Config.InstallCommands, arg) { + foundInstallCmd = true + // Check for manifest-based installation flags for i := idx + 1; i < len(args); i++ { - if strings.HasPrefix(args[i], "-") { + currentArg := args[i] + + // Handle -r/--requirement flags + if currentArg == "-r" || currentArg == "--requirement" { + isManifestInstall = true + if i+1 < len(args) { + manifestFiles = append(manifestFiles, args[i+1]) + i++ // skip the filename + } continue } - packages = append(packages, args[i]) + + // Handle combined -r flag (e.g., -rrequirements.txt) + if strings.HasPrefix(currentArg, "-r") && len(currentArg) > 2 { + isManifestInstall = true + manifestFiles = append(manifestFiles, currentArg[2:]) + continue + } + + // Handle other flags that indicate manifest installation + if currentArg == "-e" || currentArg == "--editable" || + currentArg == "-c" || currentArg == "--constraint" { + if i+1 < len(args) { + i++ // skip the next argument + } + continue + } + + // If it's a flag, skip it + if strings.HasPrefix(currentArg, "-") { + continue + } + + // Otherwise, it's a package name + packages = append(packages, currentArg) } break } } + + // If install command was found but no explicit packages and no manifest flags, + // check if it's a bare "pip install" (which should look for default manifest files) + if foundInstallCmd && len(packages) == 0 && len(manifestFiles) == 0 { + isManifestInstall = true + // pip install without args typically looks for requirements.txt + manifestFiles = append(manifestFiles, "requirements.txt") + } + var installTargets []*PackageInstallTarget for _, pkg := range packages { @@ -96,8 +142,10 @@ func (pip *pipPackageManager) ParseCommand(args []string) (*ParsedCommand, error } return &ParsedCommand{ - Command: command, - InstallTargets: installTargets, + Command: command, + InstallTargets: installTargets, + IsManifestInstall: isManifestInstall, + ManifestFiles: manifestFiles, }, nil } diff --git a/packagemanager/pypi_test.go b/packagemanager/pypi_test.go index 928a33e..fa31c45 100644 --- a/packagemanager/pypi_test.go +++ b/packagemanager/pypi_test.go @@ -111,6 +111,93 @@ func TestPipParsePackageInfo(t *testing.T) { } } +func TestPipParseCommand_ManifestInstallation(t *testing.T) { + pm, err := NewPipPackageManager(DefaultPipPackageManagerConfig()) + assert.NoError(t, err) + + cases := []struct { + name string + args []string + expectedManifest bool + expectedFiles []string + expectedTargets int + }{ + { + name: "pip install with -r flag", + args: []string{"install", "-r", "requirements.txt"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt"}, + expectedTargets: 0, + }, + { + name: "pip install with --requirement flag", + args: []string{"install", "--requirement", "requirements.txt"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt"}, + expectedTargets: 0, + }, + { + name: "pip install with combined -r flag", + args: []string{"install", "-rrequirements.txt"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt"}, + expectedTargets: 0, + }, + { + name: "pip install without args (bare install)", + args: []string{"install"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt"}, + expectedTargets: 0, + }, + { + name: "pip install with explicit package", + args: []string{"install", "django"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 1, + }, + { + name: "pip install with mixed args", + args: []string{"install", "django", "-r", "requirements.txt"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt"}, + expectedTargets: 1, + }, + { + name: "pip install with multiple -r flags", + args: []string{"install", "-r", "requirements.txt", "-r", "dev-requirements.txt"}, + expectedManifest: true, + expectedFiles: []string{"requirements.txt", "dev-requirements.txt"}, + expectedTargets: 0, + }, + { + name: "non-install command", + args: []string{"list"}, + expectedManifest: false, + expectedFiles: nil, + expectedTargets: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + parsed, err := pm.ParseCommand(tc.args) + assert.NoError(t, err) + + assert.Equal(t, tc.expectedManifest, parsed.IsManifestInstall, "IsManifestInstall mismatch") + assert.Equal(t, tc.expectedFiles, parsed.ManifestFiles, "ManifestFiles mismatch") + assert.Equal(t, tc.expectedTargets, len(parsed.InstallTargets), "InstallTargets count mismatch") + + // Test helper methods + assert.Equal(t, tc.expectedManifest, parsed.HasManifestInstall(), "HasManifestInstall mismatch") + + expectedShouldExtract := tc.expectedManifest && tc.expectedTargets == 0 + assert.Equal(t, expectedShouldExtract, parsed.ShouldExtractFromManifest(), "ShouldExtractFromManifest mismatch") + }) + } +} + func TestPipConvertCompatibleRelease(t *testing.T) { cases := []struct { name string