Fix multiWorkflowAddr undeclared variable, path traversal in bundle import, and test robustness (#154)

* Initial plan

* Apply review feedback: add multiWorkflowAddr flag, path traversal validation, error logging, and test improvements

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

Author: Copilot

cmd/server/main.go

@@ -86,8 +86,9 @@ var (
 	multiWorkflowAddr = flag.String("multi-workflow-addr", ":8080", "Listen address for the multi-workflow API server")
 	databaseDSN       = flag.String("database-dsn", "", "PostgreSQL connection string for multi-workflow mode")
 	jwtSecret         = flag.String("jwt-secret", "", "JWT signing secret for API authentication")
-	adminEmail    = flag.String("admin-email", "", "Initial admin user email (first-run bootstrap)")
-	adminPassword = flag.String("admin-password", "", "Initial admin user password (first-run bootstrap)")
+	adminEmail        = flag.String("admin-email", "", "Initial admin user email (first-run bootstrap)")
+	adminPassword     = flag.String("admin-password", "", "Initial admin user password (first-run bootstrap)")
+	multiWorkflowAddr = flag.String("multi-workflow-addr", ":8081", "HTTP listen address (multi-workflow API)")
 
 	// License flags
 	licenseKey = flag.String("license-key", "", "License key for the workflow engine (or set WORKFLOW_LICENSE_KEY env var)")
@@ -1197,10 +1198,29 @@ func (app *serverApp) importBundles(logger *slog.Logger) error {
 			continue
 		}
 
+		// Ensure the extracted workflow.yaml path is within the expected destination directory
+		absDestDir, absDestErr := filepath.Abs(destDir)
+		if absDestErr != nil {
+			logger.Error("Failed to resolve destination directory", "destDir", destDir, "error", absDestErr)
+			continue
+		}
+
+		absWorkflowPath, absWorkflowErr := filepath.Abs(workflowPath)
+		if absWorkflowErr != nil {
+			logger.Error("Failed to resolve workflow path", "path", workflowPath, "error", absWorkflowErr)
+			continue
+		}
+
+		rel, relErr := filepath.Rel(absDestDir, absWorkflowPath)
+		if relErr != nil || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) {
+			logger.Error("Workflow path is outside destination directory; possible path traversal", "path", absWorkflowPath, "destDir", absDestDir, "error", relErr)
+			continue
+		}
+
 		// Read the extracted workflow.yaml
-		yamlData, err := os.ReadFile(workflowPath) //nolint:gosec // G703: path from trusted bundle extraction
+		yamlData, err := os.ReadFile(absWorkflowPath) //nolint:gosec // G703: path validated to be within destDir
 		if err != nil {
-			logger.Error("Failed to read workflow.yaml", "path", workflowPath, "error", err)
+			logger.Error("Failed to read workflow.yaml", "path", absWorkflowPath, "error", err)
 			continue
 		}
 		yamlContent := string(yamlData)
@@ -1326,10 +1346,14 @@ func run(ctx context.Context, app *serverApp, listenAddr string) error {
 
 	// Clean up temp files and directories
 	for _, f := range app.cleanupFiles {
-		os.Remove(f) //nolint:gosec // G703: cleaning up server-managed temp files
+		if err := os.Remove(f); err != nil && !os.IsNotExist(err) { //nolint:gosec // G703: cleaning up server-managed temp files
+			app.logger.Error("Temp file cleanup error", "path", f, "error", err)
+		}
 	}
 	for _, d := range app.cleanupDirs {
-		os.RemoveAll(d) //nolint:gosec // G703: cleaning up server-managed temp dirs
+		if err := os.RemoveAll(d); err != nil && !os.IsNotExist(err) { //nolint:gosec // G703: cleaning up server-managed temp dirs
+			app.logger.Error("Temp directory cleanup error", "path", d, "error", err)
+		}
 	}
 
 	return nil

engine_structural_test.go

@@ -14,6 +14,17 @@ import (
 
 // --- Negative tests: core engine rejects unknown module types ---
 
+// findPluginByName returns the plugin with the given name from allPlugins,
+// or nil if no such plugin exists.
+func findPluginByName(name string) plugin.EnginePlugin {
+	for _, p := range allPlugins() {
+		if p.Name() == name {
+			return p
+		}
+	}
+	return nil
+}
+
 // TestCoreRejectsUnknownModuleType verifies that the engine returns a clear
 // error when a config references a module type that no plugin provides.
 func TestCoreRejectsUnknownModuleType(t *testing.T) {
@@ -201,7 +212,10 @@ func TestSelectivePluginLoading_HTTPOnly(t *testing.T) {
 	engine := NewStdEngine(app, &mock.Logger{})
 
 	// Load only the HTTP plugin.
-	httpPlugin := allPlugins()[0] // http plugin is first
+	httpPlugin := findPluginByName("workflow-plugin-http")
+	if httpPlugin == nil {
+		t.Fatalf("HTTP plugin not found in allPlugins")
+	}
 	if err := engine.LoadPlugin(httpPlugin); err != nil {
 		t.Fatalf("LoadPlugin(http) failed: %v", err)
 	}
@@ -274,13 +288,28 @@ func TestEngineFactoryMapPopulatedByPlugins(t *testing.T) {
 		"auth.jwt",
 	}
 
+	// Build a set of known module types from the public schema API.
+	known := schema.KnownModuleTypes()
+	knownSet := make(map[string]bool, len(known))
+	for _, k := range known {
+		knownSet[k] = true
+	}
+
 	for _, mt := range expectedTypes {
-		if _, ok := engine.moduleFactories[mt]; !ok {
-			t.Errorf("module type %q not found in factory map after loading all plugins", mt)
+		if !knownSet[mt] {
+			t.Errorf("module type %q not found in schema.KnownModuleTypes() after loading all plugins", mt)
 		}
 	}
 }
 
+func getEngineModuleTypes(engine *StdEngine) []string {
+	types := make([]string, 0, len(engine.moduleFactories))
+	for mt := range engine.moduleFactories {
+		types = append(types, mt)
+	}
+	return types
+}
+
 // TestSchemaKnowsPluginModuleTypes verifies that schema.RegisterModuleType is
 // called for each plugin's module types during LoadPlugin.
 func TestSchemaKnowsPluginModuleTypes(t *testing.T) {
@@ -295,7 +324,7 @@ func TestSchemaKnowsPluginModuleTypes(t *testing.T) {
 	}
 
 	// Every module type in the factory map should be in the schema.
-	for mt := range engine.moduleFactories {
+	for _, mt := range getEngineModuleTypes(engine) {
 		if !knownSet[mt] {
 			t.Errorf("module type %q is in factory map but not in schema.KnownModuleTypes()", mt)
 		}

module/api_v1_test.go

@@ -29,6 +29,35 @@ func setupTestStore(t *testing.T) *V1Store {
 
 // --- V1Store Tests ---
 
+func mustCreateCompany(t *testing.T, store *V1Store, name, desc, userID string) *V1Company {
+	t.Helper()
+	company, err := store.CreateCompany(name, desc, userID)
+	if err != nil {
+		t.Fatalf("CreateCompany(%q) failed: %v", name, err)
+	}
+	return company
+}
+
+func mustCreateOrganization(t *testing.T, store *V1Store, companyID, name, desc, userID string) *V1Company {
+	t.Helper()
+	org, err := store.CreateOrganization(companyID, name, desc, userID)
+	if err != nil {
+		t.Fatalf("CreateOrganization(%q) failed: %v", name, err)
+	}
+	return org
+}
+
+func mustCreateProject(t *testing.T, store *V1Store, orgID, name, desc, userID string) *V1Project {
+	t.Helper()
+	proj, err := store.CreateProject(orgID, name, desc, userID)
+	if err != nil {
+		t.Fatalf("CreateProject(%q) failed: %v", name, err)
+	}
+	return proj
+}
+
+// --- V1Store Tests ---
+
 func TestV1Store_CreateAndListCompanies(t *testing.T) {
 	store := setupTestStore(t)
 
@@ -116,8 +145,14 @@ func TestV1Store_CreateAndListOrganizations(t *testing.T) {
 func TestV1Store_CreateAndListProjects(t *testing.T) {
 	store := setupTestStore(t)
 
-	company, _ := store.CreateCompany("Co", "", "u1")
-	org, _ := store.CreateOrganization(company.ID, "Org", "", "u1")
+	company, err := store.CreateCompany("Co", "", "u1")
+	if err != nil {
+		t.Fatalf("CreateCompany: %v", err)
+	}
+	org, err := store.CreateOrganization(company.ID, "Org", "", "u1")
+	if err != nil {
+		t.Fatalf("CreateOrganization: %v", err)
+	}
 
 	p, err := store.CreateProject(org.ID, "My Project", "", "A cool project")
 	if err != nil {
@@ -151,9 +186,9 @@ func TestV1Store_CreateAndListProjects(t *testing.T) {
 func TestV1Store_WorkflowCRUD(t *testing.T) {
 	store := setupTestStore(t)
 
-	company, _ := store.CreateCompany("Co", "", "u1")
-	org, _ := store.CreateOrganization(company.ID, "Org", "", "u1")
-	proj, _ := store.CreateProject(org.ID, "Proj", "", "")
+	company := mustCreateCompany(t, store, "Co", "", "u1")
+	org := mustCreateOrganization(t, store, company.ID, "Org", "", "u1")
+	proj := mustCreateProject(t, store, org.ID, "Proj", "", "")
 
 	// Create
 	wf, err := store.CreateWorkflow(proj.ID, "Test Workflow", "", "desc", "modules: []", "u1")
@@ -221,7 +256,10 @@ func TestV1Store_WorkflowCRUD(t *testing.T) {
 		t.Fatalf("DeleteWorkflow: %v", err)
 	}
 
-	wfs, _ = store.ListWorkflows(proj.ID)
+	wfs, err = store.ListWorkflows(proj.ID)
+	if err != nil {
+		t.Fatalf("ListWorkflows after delete: %v", err)
+	}
 	if len(wfs) != 0 {
 		t.Errorf("got %d workflows after delete, want 0", len(wfs))
 	}
@@ -230,15 +268,21 @@ func TestV1Store_WorkflowCRUD(t *testing.T) {
 func TestV1Store_WorkflowVersioning(t *testing.T) {
 	store := setupTestStore(t)
 
-	company, _ := store.CreateCompany("Co", "", "u1")
-	org, _ := store.CreateOrganization(company.ID, "Org", "", "u1")
-	proj, _ := store.CreateProject(org.ID, "Proj", "", "")
+	company := mustCreateCompany(t, store, "Co", "", "u1")
+	org := mustCreateOrganization(t, store, company.ID, "Org", "", "u1")
+	proj := mustCreateProject(t, store, org.ID, "Proj", "", "")
 	wf, _ := store.CreateWorkflow(proj.ID, "WF", "", "", "v1 config", "u1")
 
 	// Update config 3 times to create versions 2, 3, 4
-	store.UpdateWorkflow(wf.ID, "", "", "v2 config", "u1")
-	store.UpdateWorkflow(wf.ID, "", "", "v3 config", "u1")
-	store.UpdateWorkflow(wf.ID, "", "", "v4 config", "u1")
+	if _, err := store.UpdateWorkflow(wf.ID, "", "", "v2 config", "u1"); err != nil {
+		t.Fatalf("UpdateWorkflow v2: %v", err)
+	}
+	if _, err := store.UpdateWorkflow(wf.ID, "", "", "v3 config", "u1"); err != nil {
+		t.Fatalf("UpdateWorkflow v3: %v", err)
+	}
+	if _, err := store.UpdateWorkflow(wf.ID, "", "", "v4 config", "u1"); err != nil {
+		t.Fatalf("UpdateWorkflow v4: %v", err)
+	}
 
 	versions, err := store.ListVersions(wf.ID)
 	if err != nil {
@@ -311,12 +355,18 @@ func TestV1Store_EnsureSystemHierarchy(t *testing.T) {
 	if err != nil {
 		t.Fatalf("EnsureSystemHierarchy (second call): %v", err)
 	}
+	if c2 != companyID {
+		t.Errorf("expected same company ID %q, got %q", companyID, c2)
+	}
+	if o2 != orgID {
+		t.Errorf("expected same organization ID %q, got %q", orgID, o2)
+	}
+	if p2 != projectID {
+		t.Errorf("expected same project ID %q, got %q", projectID, p2)
+	}
 	if w2 != workflowID {
 		t.Errorf("expected same workflow ID %q, got %q", workflowID, w2)
 	}
-	_ = c2
-	_ = o2
-	_ = p2
 }
 
 func TestV1Store_ResetSystemWorkflow(t *testing.T) {

plugins/admin/plugin_test.go

@@ -9,7 +9,19 @@ import (
 
 func TestPluginImplementsEnginePlugin(t *testing.T) {
 	p := New()
+	// Compile-time assertion that *Plugin implements plugin.EnginePlugin.
 	var _ plugin.EnginePlugin = p
+
+	// Runtime checks via the EnginePlugin interface.
+	var ep plugin.EnginePlugin = p
+
+	manifest := ep.EngineManifest()
+	if manifest == nil {
+		t.Fatal("EngineManifest() returned nil")
+	}
+	if err := manifest.Validate(); err != nil {
+		t.Fatalf("EngineManifest.Validate() failed: %v", err)
+	}
 }
 
 func TestPluginManifest(t *testing.T) {
@@ -55,9 +67,14 @@ func TestModuleFactories(t *testing.T) {
 			t.Errorf("missing factory for %q", typ)
 			continue
 		}
-		mod := factory("test-"+typ, map[string]any{})
+		name := "test-" + typ
+		mod := factory(name, map[string]any{})
 		if mod == nil {
 			t.Errorf("factory for %q returned nil", typ)
+			continue
+		}
+		if mod.Name() != name {
+			t.Errorf("factory for %q produced module with unexpected name: got %q, want %q", typ, mod.Name(), name)
 		}
 	}
 }
@@ -141,6 +158,7 @@ func TestWiringHookMergesAdminConfig(t *testing.T) {
 		},
 	}
 
+	originalLen := len(cfg.Modules)
 	err := hooks[0].Hook(nil, cfg)
 	if err != nil {
 		t.Fatalf("wiring hook failed: %v", err)
@@ -157,6 +175,11 @@ func TestWiringHookMergesAdminConfig(t *testing.T) {
 	if !found {
 		t.Error("admin-server module not found after wiring hook merge")
 	}
+
+	// Ensure that additional admin modules have been merged
+	if len(cfg.Modules) <= originalLen {
+		t.Errorf("expected modules to be added by wiring hook, before=%d after=%d", originalLen, len(cfg.Modules))
+	}
 }
 
 func TestWiringHookSkipsIfAlreadyPresent(t *testing.T) {