Skip to content

Commit 79b1250

Browse files
fix: address Copilot review comments on PR #32
- Move filterUserInstalledExtensions to model package (was duplicated) - Thread context.Context through Eclipse detection pipeline (was using context.Background()) - Drive letter probes check if drive exists before probing (avoids network drive timeouts) - Fix eclipseBundledPrefixes comment to match actual Source values - Add --include-bundled-plugins to help text - Add 14 unit tests for Eclipse detection pipeline (validation, bundles.info parsing, p2 director output parsing, dropins, etc.)
1 parent 5891fc7 commit 79b1250

7 files changed

Lines changed: 329 additions & 41 deletions

File tree

internal/cli/cli.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,10 @@ Options:
148148
--disable-npm-scan Disable Node.js package scanning
149149
--enable-brew-scan Enable Homebrew package scanning
150150
--disable-brew-scan Disable Homebrew package scanning
151-
--enable-python-scan Enable Python package scanning
152-
--disable-python-scan Disable Python package scanning
153-
--verbose Show progress messages (suppressed by default)
151+
--enable-python-scan Enable Python package scanning
152+
--disable-python-scan Disable Python package scanning
153+
--include-bundled-plugins Include bundled/platform plugins in output (Windows)
154+
--verbose Show progress messages (suppressed by default)
154155
--color=WHEN Color mode: auto | always | never (default: auto)
155156
-v, --version Show version
156157
-h, --help Show this help

internal/detector/eclipse_plugins.go

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@ import (
1212
// eclipseBundledPrefixes are bundle ID prefixes that ship as part of the
1313
// base Eclipse platform. Bundles matching these are tagged as "bundled".
1414
// eclipseBundledPrefixes identifies bundles that ship as part of the Eclipse
15-
// platform or are standard dependencies. Everything NOT matching is classified
16-
// as "marketplace" (user-installed from Eclipse Marketplace or update sites).
15+
// platform or are standard dependencies. Bundles that do not match these
16+
// prefixes are treated as non-bundled and may be classified into source
17+
// categories such as "marketplace", "user_installed", or "dropins".
1718
var eclipseBundledPrefixes = []string{
1819
// Eclipse platform
1920
"org.eclipse.",
@@ -96,7 +97,7 @@ var eclipseFeatureDirsDarwin = []string{
9697
// On macOS: scans features/dropins directories.
9798
// On Windows: multi-stage pipeline using detected IDE paths, path probes,
9899
// and drive letter scanning, with validation before reporting.
99-
func (d *ExtensionDetector) DetectEclipsePlugins(ides []model.IDE) []model.Extension {
100+
func (d *ExtensionDetector) DetectEclipsePlugins(ctx context.Context, ides []model.IDE) []model.Extension {
100101
if d.exec.GOOS() != "windows" {
101102
var results []model.Extension
102103
for _, dir := range eclipseFeatureDirsDarwin {
@@ -106,12 +107,12 @@ func (d *ExtensionDetector) DetectEclipsePlugins(ides []model.IDE) []model.Exten
106107
}
107108
return results
108109
}
109-
return d.detectEclipsePluginsWindows(ides)
110+
return d.detectEclipsePluginsWindows(ctx, ides)
110111
}
111112

112113
// ---------- Windows multi-stage pipeline ----------
113114

114-
func (d *ExtensionDetector) detectEclipsePluginsWindows(ides []model.IDE) []model.Extension {
115+
func (d *ExtensionDetector) detectEclipsePluginsWindows(ctx context.Context, ides []model.IDE) []model.Extension {
115116
// Stage 1+2: Collect candidate paths from detected IDEs + well-known locations
116117
candidates := d.gatherEclipseCandidates(ides)
117118

@@ -134,7 +135,7 @@ func (d *ExtensionDetector) detectEclipsePluginsWindows(ides []model.IDE) []mode
134135
pluginSeen := make(map[string]bool)
135136
var results []model.Extension
136137
for _, installDir := range validInstalls {
137-
plugins := d.enumerateEclipsePlugins(installDir)
138+
plugins := d.enumerateEclipsePlugins(ctx, installDir)
138139
for _, p := range plugins {
139140
dedupKey := p.ID + "@" + p.Version
140141
if pluginSeen[dedupKey] {
@@ -200,10 +201,14 @@ func (d *ExtensionDetector) gatherEclipseCandidates(ides []model.IDE) []string {
200201
candidates = append(candidates, d.globDirs(filepath.Join(localAppData, "Programs", "Spring*"))...)
201202
}
202203

203-
// Drive letter probe: D:\eclipse through Z:\eclipse (fixed drives only)
204+
// Drive letter probe: D:\eclipse through Z:\eclipse.
205+
// Only probe drives that actually exist to avoid slow network drive timeouts.
204206
for drive := 'D'; drive <= 'Z'; drive++ {
205-
driveRoot := string(drive) + `:\eclipse`
206-
candidates = append(candidates, driveRoot)
207+
driveRoot := string(drive) + `:\`
208+
if !d.exec.DirExists(driveRoot) {
209+
continue
210+
}
211+
candidates = append(candidates, string(drive)+`:\eclipse`)
207212
}
208213

209214
return candidates
@@ -257,9 +262,9 @@ func (d *ExtensionDetector) validateEclipseInstall(installDir string) bool {
257262
// enumerateEclipsePlugins collects plugins from a validated Eclipse install.
258263
// Uses the p2 director API (eclipsec.exe -listInstalledRoots) to get authoritative
259264
// installed features, then enriches with bundles.info for full bundle details.
260-
func (d *ExtensionDetector) enumerateEclipsePlugins(installDir string) []model.Extension {
265+
func (d *ExtensionDetector) enumerateEclipsePlugins(ctx context.Context, installDir string) []model.Extension {
261266
// Try p2 director first — returns authoritative list of installed root features
262-
roots := d.queryP2InstalledRoots(installDir)
267+
roots := d.queryP2InstalledRoots(ctx, installDir)
263268

264269
// Build a set of root feature prefixes for marketplace classification.
265270
// Root features that are NOT org.eclipse.* or epp.* are marketplace-installed.
@@ -336,7 +341,7 @@ func (d *ExtensionDetector) enumerateEclipsePlugins(installDir string) []model.E
336341
// queryP2InstalledRoots invokes Eclipse's p2 director to get the authoritative
337342
// list of installed root features. Returns nil if eclipsec.exe is not available
338343
// or the command fails. Output format: "feature.id/version" per line.
339-
func (d *ExtensionDetector) queryP2InstalledRoots(installDir string) []model.Extension {
344+
func (d *ExtensionDetector) queryP2InstalledRoots(ctx context.Context, installDir string) []model.Extension {
340345
// Find eclipsec.exe (console launcher)
341346
eclipsec := filepath.Join(installDir, "eclipsec.exe")
342347
if !d.exec.FileExists(eclipsec) {
@@ -347,7 +352,6 @@ func (d *ExtensionDetector) queryP2InstalledRoots(installDir string) []model.Ext
347352
}
348353
}
349354

350-
ctx := context.Background()
351355
stdout, _, exitCode, err := d.exec.RunWithTimeout(ctx, 30*time.Second,
352356
eclipsec, "-nosplash",
353357
"-application", "org.eclipse.equinox.p2.director",
Lines changed: 293 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,293 @@
1+
package detector
2+
3+
import (
4+
"context"
5+
"os"
6+
"testing"
7+
8+
"github.com/step-security/dev-machine-guard/internal/executor"
9+
"github.com/step-security/dev-machine-guard/internal/model"
10+
)
11+
12+
func TestValidateEclipseInstall(t *testing.T) {
13+
mock := executor.NewMock()
14+
mock.SetGOOS("windows")
15+
16+
installDir := `C:\eclipse`
17+
mock.SetDir(installDir)
18+
// filepath.Join on macOS uses "/" between parts but preserves existing "\"
19+
mock.SetFile(installDir+"/eclipse.ini", []byte{})
20+
mock.SetDir(installDir + "/plugins")
21+
mock.SetDir(installDir + "/configuration")
22+
23+
det := &ExtensionDetector{exec: mock}
24+
if !det.validateEclipseInstall(installDir) {
25+
t.Error("expected valid Eclipse install")
26+
}
27+
}
28+
29+
func TestValidateEclipseInstall_MissingIni(t *testing.T) {
30+
mock := executor.NewMock()
31+
mock.SetGOOS("windows")
32+
33+
installDir := `C:\eclipse`
34+
mock.SetDir(installDir)
35+
// No eclipse.ini
36+
mock.SetDir(installDir + `\plugins`)
37+
mock.SetDir(installDir + `\configuration`)
38+
39+
det := &ExtensionDetector{exec: mock}
40+
if det.validateEclipseInstall(installDir) {
41+
t.Error("expected invalid — missing eclipse.ini")
42+
}
43+
}
44+
45+
func TestValidateEclipseInstall_BrandedIni(t *testing.T) {
46+
mock := executor.NewMock()
47+
mock.SetGOOS("windows")
48+
49+
installDir := `C:\sts`
50+
mock.SetDir(installDir)
51+
mock.SetFile(installDir+"/sts.ini", []byte{}) // Spring Tool Suite
52+
mock.SetDir(installDir + "/plugins")
53+
mock.SetDir(installDir + "/configuration")
54+
55+
det := &ExtensionDetector{exec: mock}
56+
if !det.validateEclipseInstall(installDir) {
57+
t.Error("expected valid — branded sts.ini should count")
58+
}
59+
}
60+
61+
func TestParseEclipseBundlesInfo(t *testing.T) {
62+
mock := executor.NewMock()
63+
mock.SetFile("/test/bundles.info", []byte(`#encoding=UTF-8
64+
#version=1
65+
org.eclipse.platform,4.39.0,file:/plugins/org.eclipse.platform_4.39.0.jar,4,false
66+
com.anthropic.claudecode.eclipse,2.3.11,file:/plugins/com.anthropic.claudecode.eclipse_2.3.11.jar,4,false
67+
`))
68+
69+
det := &ExtensionDetector{exec: mock}
70+
results := det.parseEclipseBundlesInfo("/test/bundles.info")
71+
72+
if len(results) != 2 {
73+
t.Fatalf("expected 2 bundles, got %d", len(results))
74+
}
75+
76+
if results[0].ID != "org.eclipse.platform" {
77+
t.Errorf("expected org.eclipse.platform, got %s", results[0].ID)
78+
}
79+
if results[0].Source != "bundled" {
80+
t.Errorf("expected bundled, got %s", results[0].Source)
81+
}
82+
83+
if results[1].ID != "com.anthropic.claudecode.eclipse" {
84+
t.Errorf("expected com.anthropic.claudecode.eclipse, got %s", results[1].ID)
85+
}
86+
if results[1].Source != "user_installed" {
87+
t.Errorf("expected user_installed, got %s", results[1].Source)
88+
}
89+
}
90+
91+
func TestParseEclipseBundlesInfo_MissingFile(t *testing.T) {
92+
mock := executor.NewMock()
93+
det := &ExtensionDetector{exec: mock}
94+
results := det.parseEclipseBundlesInfo("/nonexistent")
95+
if len(results) != 0 {
96+
t.Errorf("expected 0, got %d", len(results))
97+
}
98+
}
99+
100+
func TestQueryP2InstalledRoots_ParsesOutput(t *testing.T) {
101+
mock := executor.NewMock()
102+
mock.SetGOOS("windows")
103+
// filepath.Join on macOS uses "/" between parts
104+
eclipsec := `C:\eclipse` + "/eclipsec.exe"
105+
mock.SetFile(eclipsec, []byte{})
106+
107+
mock.SetCommand(
108+
"civerooni.com.putman.feature.feature.group/1.0.0\n"+
109+
"com.anthropic.claudecode.eclipse.feature.feature.group/2.3.11\n"+
110+
"org.eclipse.jdt.feature.group/3.20.0\n"+
111+
"epp.package.java/4.39.0\n"+
112+
"Operation completed in 2131 ms.\n",
113+
"", 0,
114+
eclipsec, "-nosplash",
115+
"-application", "org.eclipse.equinox.p2.director",
116+
"-listInstalledRoots",
117+
)
118+
119+
det := &ExtensionDetector{exec: mock}
120+
results := det.queryP2InstalledRoots(context.Background(), `C:\eclipse`)
121+
122+
if len(results) != 4 {
123+
t.Fatalf("expected 4 root features, got %d", len(results))
124+
}
125+
126+
marketplace := 0
127+
bundled := 0
128+
for _, r := range results {
129+
if r.Source == "marketplace" {
130+
marketplace++
131+
} else if r.Source == "bundled" {
132+
bundled++
133+
}
134+
}
135+
136+
if marketplace != 2 {
137+
t.Errorf("expected 2 marketplace, got %d", marketplace)
138+
}
139+
if bundled != 2 {
140+
t.Errorf("expected 2 bundled, got %d", bundled)
141+
}
142+
}
143+
144+
func TestQueryP2InstalledRoots_SkipsDebugLines(t *testing.T) {
145+
mock := executor.NewMock()
146+
mock.SetGOOS("windows")
147+
eclipsec := `C:\eclipse` + "/eclipsec.exe"
148+
mock.SetFile(eclipsec, []byte{})
149+
150+
mock.SetCommand(
151+
"18:53:22.314 [Start Level] DEBUG org.eclipse.jgit -- some debug\n"+
152+
"org.eclipse.platform.feature.group/4.39.0\n"+
153+
"Operation completed in 100 ms.\n",
154+
"", 0,
155+
eclipsec, "-nosplash",
156+
"-application", "org.eclipse.equinox.p2.director",
157+
"-listInstalledRoots",
158+
)
159+
160+
det := &ExtensionDetector{exec: mock}
161+
results := det.queryP2InstalledRoots(context.Background(), `C:\eclipse`)
162+
163+
if len(results) != 1 {
164+
t.Fatalf("expected 1 (debug lines filtered), got %d", len(results))
165+
}
166+
if results[0].ID != "org.eclipse.platform.feature.group" {
167+
t.Errorf("expected org.eclipse.platform.feature.group, got %s", results[0].ID)
168+
}
169+
}
170+
171+
func TestQueryP2InstalledRoots_NoEclipsec(t *testing.T) {
172+
mock := executor.NewMock()
173+
mock.SetGOOS("windows")
174+
// No eclipsec.exe or eclipse.exe
175+
176+
det := &ExtensionDetector{exec: mock}
177+
results := det.queryP2InstalledRoots(context.Background(), `C:\eclipse`)
178+
179+
if len(results) != 0 {
180+
t.Errorf("expected 0 when no exe found, got %d", len(results))
181+
}
182+
}
183+
184+
func TestCollectDropins_DirectJAR(t *testing.T) {
185+
mock := executor.NewMock()
186+
dropinsDir := "/eclipse/dropins"
187+
mock.SetDir(dropinsDir)
188+
mock.SetDirEntries(dropinsDir, []os.DirEntry{
189+
executor.MockDirEntry("com.example.plugin_1.0.0.jar", false),
190+
})
191+
192+
det := &ExtensionDetector{exec: mock}
193+
results := det.collectDropins(dropinsDir)
194+
195+
if len(results) != 1 {
196+
t.Fatalf("expected 1 dropin, got %d", len(results))
197+
}
198+
if results[0].ID != "com.example.plugin" {
199+
t.Errorf("expected com.example.plugin, got %s", results[0].ID)
200+
}
201+
if results[0].Version != "1.0.0" {
202+
t.Errorf("expected 1.0.0, got %s", results[0].Version)
203+
}
204+
if results[0].Source != "dropins" {
205+
t.Errorf("expected dropins source, got %s", results[0].Source)
206+
}
207+
}
208+
209+
func TestCollectDropins_Empty(t *testing.T) {
210+
mock := executor.NewMock()
211+
det := &ExtensionDetector{exec: mock}
212+
results := det.collectDropins("/nonexistent")
213+
if len(results) != 0 {
214+
t.Errorf("expected 0, got %d", len(results))
215+
}
216+
}
217+
218+
func TestParseEclipsePluginName(t *testing.T) {
219+
tests := []struct {
220+
input string
221+
id string
222+
version string
223+
}{
224+
{"com.github.spotbugs.plugin.eclipse_4.9.8.r202510181643-c1fa7f2", "com.github.spotbugs.plugin.eclipse", "4.9.8.r202510181643-c1fa7f2"},
225+
{"org.eclipse.jdt.core_3.36.0.v20240103-1234", "org.eclipse.jdt.core", "3.36.0.v20240103-1234"},
226+
{"simple.plugin_1.0", "simple.plugin", "1.0"},
227+
}
228+
229+
for _, tt := range tests {
230+
ext := parseEclipsePluginName(tt.input)
231+
if ext == nil {
232+
t.Errorf("parseEclipsePluginName(%q) returned nil", tt.input)
233+
continue
234+
}
235+
if ext.ID != tt.id {
236+
t.Errorf("ID: expected %s, got %s", tt.id, ext.ID)
237+
}
238+
if ext.Version != tt.version {
239+
t.Errorf("Version: expected %s, got %s", tt.version, ext.Version)
240+
}
241+
}
242+
}
243+
244+
func TestParseEclipsePluginName_Invalid(t *testing.T) {
245+
invalid := []string{
246+
"nounderscore",
247+
"no_version_starts_with_letter",
248+
"",
249+
}
250+
for _, input := range invalid {
251+
if ext := parseEclipsePluginName(input); ext != nil {
252+
t.Errorf("parseEclipsePluginName(%q) should return nil, got %+v", input, ext)
253+
}
254+
}
255+
}
256+
257+
func TestIsEclipseBundled(t *testing.T) {
258+
bundled := []string{"org.eclipse.jdt.core", "javax.annotation", "ch.qos.logback.classic", "bcprov"}
259+
for _, id := range bundled {
260+
if !isEclipseBundled(id) {
261+
t.Errorf("%q should be bundled", id)
262+
}
263+
}
264+
265+
notBundled := []string{"com.anthropic.claudecode", "civerooni.com.putman", "my.custom.plugin"}
266+
for _, id := range notBundled {
267+
if isEclipseBundled(id) {
268+
t.Errorf("%q should NOT be bundled", id)
269+
}
270+
}
271+
}
272+
273+
// Ensure model.FilterUserInstalledExtensions works correctly
274+
func TestFilterUserInstalledExtensions(t *testing.T) {
275+
exts := []model.Extension{
276+
{ID: "bundled1", Source: "bundled"},
277+
{ID: "marketplace1", Source: "marketplace"},
278+
{ID: "user1", Source: "user_installed"},
279+
{ID: "dropin1", Source: "dropins"},
280+
{ID: "nosource"},
281+
}
282+
283+
filtered := model.FilterUserInstalledExtensions(exts)
284+
if len(filtered) != 4 {
285+
t.Fatalf("expected 4 (all except bundled), got %d", len(filtered))
286+
}
287+
for _, e := range filtered {
288+
if e.Source == "bundled" {
289+
t.Errorf("bundled extension should have been filtered: %s", e.ID)
290+
}
291+
}
292+
}
293+

0 commit comments

Comments
 (0)