security: fix SQL-injection and path-injection (CodeQL)

cmd/server/main.go

@@ -538,6 +538,11 @@ func (app *serverApp) initStores(logger *slog.Logger) error {
 
 	// Create V1 API handler
 	v1Handler := module.NewV1APIHandler(store, secret)
+	// Constrain server-local path reads (loadWorkflowFromPath) to the configured
+	// data directory. Without this, the path-injection guard in the handler is
+	// inactive and an authenticated admin could read arbitrary files off the host
+	// via the /workflows/load-from-path endpoint.
+	v1Handler.SetDataDir(*dataDir)
 	v1Handler.SetReloadFunc(func(configYAML string) error {
 		cfg, parseErr := config.LoadFromString(configYAML)
 		if parseErr != nil {

module/api_v1_handler.go

@@ -50,9 +50,22 @@
 	h.runtimeManager = rm
 }
 
-// SetDataDir sets the base data directory used for workspace extraction during import.
+// SetDataDir sets the base data directory used for workspace extraction during
+// import and for containing server-local path reads. The directory is normalised
+// to an absolute, cleaned path so that the containment check in
+// loadWorkflowFromPath compares like with like (absolute vs absolute).
 func (h *V1APIHandler) SetDataDir(dir string) {
-	h.dataDir = dir
+	if dir == "" {
+		h.dataDir = ""
+		return
+	}
+	if abs, err := filepath.Abs(dir); err == nil {
+		h.dataDir = filepath.Clean(abs)
+		return
+	}
+	// Fall back to the cleaned (possibly relative) path if Abs fails; the
+	// containment check itself also calls filepath.Abs on both operands.
+	h.dataDir = filepath.Clean(dir)
 }
 
 // ServeHTTP implements http.Handler for config-driven delegate dispatch.
@@ -960,26 +973,59 @@
 		req.ProjectID = "00000000-0000-0000-0000-000000000002"
 	}
 
-	// Resolve the config file path
-	configPath := filepath.Clean(req.Path)
+	// Resolve the config file path and enforce path containment.
+	//
+	// The resolved path MUST stay inside an allowed base directory so that an
+	// authenticated request cannot read arbitrary files from the host filesystem
+	// (e.g. /etc/passwd or ../../secrets). Containment is active BY DEFAULT:
+	//   - In production, cmd/server wires SetDataDir(--data-dir), so the base is
+	//     the configured data directory.
+	//   - If no data directory was configured, we fall back to the process
+	//     working directory (filepath.Abs(".")) rather than leaving the read
+	//     unguarded. This keeps the endpoint safe by default.
+	base := h.dataDir
+	if base == "" {
+		// Default-deny posture: contain to the current working directory.
+		if wd, wdErr := filepath.Abs("."); wdErr == nil {
+			base = wd
+		}
+	}
+
+	absConfig, absErr := filepath.Abs(filepath.Clean(req.Path))
+	if absErr != nil {
+		writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid path"})
+		return
+	}
+	configPath := absConfig
+
+	if base != "" {
+		baseClean := filepath.Clean(base) + string(os.PathSeparator)
+		if !strings.HasPrefix(absConfig+string(os.PathSeparator), baseClean) {
+			writeJSON(w, http.StatusForbidden, map[string]string{"error": "path is outside the allowed data directory"})
+			return
+		}
+	}
+
 	info, err := os.Stat(configPath)
 	if err != nil {
-		writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("path not found: %s", configPath)})
+		// Do not echo the resolved absolute path back to the client.
+		writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path not found"})
 		return
 	}
 
 	// If directory, look for workflow.yaml
 	if info.IsDir() {
 		yamlPath := filepath.Join(configPath, "workflow.yaml")
 		if _, err := os.Stat(yamlPath); err != nil {
-			writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("no workflow.yaml in %s", configPath)})
+			writeJSON(w, http.StatusBadRequest, map[string]string{"error": "no workflow.yaml in the specified directory"})
 			return
 		}
 		configPath = yamlPath
 	}
 
-	// Read the config file
-	data, err := os.ReadFile(configPath)
+	// Read the config file. configPath has been validated above: it is either
+	// contained within h.dataDir (when set) or passed through filepath.Clean.
+	data, err := os.ReadFile(configPath) //nolint:gosec // G304: path validated against h.dataDir base directory above
 	if err != nil {
 		writeJSON(w, http.StatusInternalServerError, map[string]string{"error": fmt.Sprintf("failed to read file: %v", err)})
 		return

module/api_v1_test.go

@@ -1100,3 +1100,91 @@ func TestV1Store_ExecutionFailure(t *testing.T) {
 		t.Errorf("got failed count %d, want 1", counts["failed"])
 	}
 }
+
+// TestLoadWorkflowFromPath_PathTraversal verifies that the loadWorkflowFromPath
+// endpoint rejects paths that escape h.dataDir when that field is configured.
+// This covers the go/path-injection CodeQL alerts #56, #57, #58 in
+// module/api_v1_handler.go (os.Stat / filepath.Join / os.ReadFile sinks).
+func TestLoadWorkflowFromPath_PathTraversal(t *testing.T) {
+	handler, _, secret := setupTestHandler(t)
+
+	// Create a temporary directory to act as the allowed data directory.
+	allowedDir := t.TempDir()
+	handler.SetDataDir(allowedDir)
+
+	token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+	// Each of these resolves to a path OUTSIDE allowedDir. The containment guard
+	// is the only code path that produces 403 Forbidden — os.Stat on a missing
+	// file would yield 400, and a readable file would yield 201/500. Asserting
+	// EXACTLY 403 makes this test revert-sensitive: removing the guard changes
+	// the status code (to 400 for the nonexistent traversal targets), failing it.
+	maliciousPaths := []string{
+		"../../etc/passwd",
+		"/etc/passwd",
+		allowedDir + "/../../etc/passwd",
+		"../outside",
+	}
+
+	for _, p := range maliciousPaths {
+		t.Run(p, func(t *testing.T) {
+			body := fmt.Sprintf(`{"path": %q, "project_id": "00000000-0000-0000-0000-000000000002"}`, p)
+			rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+			if rr.Code != http.StatusForbidden {
+				t.Errorf("path %q: expected 403 Forbidden from containment guard, got %d (body: %s)",
+					p, rr.Code, rr.Body.String())
+			}
+		})
+	}
+}
+
+// TestLoadWorkflowFromPath_ContainedByDefault verifies that even with NO dataDir
+// configured, the endpoint is contained to the process working directory (default-
+// deny posture) rather than reading arbitrary absolute paths. Covers CRITICAL: the
+// production handler does not always have SetDataDir called.
+func TestLoadWorkflowFromPath_ContainedByDefault(t *testing.T) {
+	handler, _, secret := setupTestHandler(t)
+	// Intentionally do NOT call SetDataDir — exercise the default-deny fallback.
+
+	token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+	// /etc/passwd is outside the process working directory, so the default
+	// containment base (filepath.Abs(".")) must reject it with 403.
+	body := `{"path": "/etc/passwd", "project_id": "00000000-0000-0000-0000-000000000002"}`
+	rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+	if rr.Code != http.StatusForbidden {
+		t.Errorf("expected 403 (default containment to working dir), got %d (body: %s)",
+			rr.Code, rr.Body.String())
+	}
+}
+
+// TestLoadWorkflowFromPath_AllowedPath verifies that a path inside dataDir is
+// still accepted when loadWorkflowFromPath has a dataDir configured.
+func TestLoadWorkflowFromPath_AllowedPath(t *testing.T) {
+	handler, _, secret := setupTestHandler(t)
+
+	// Set up a data dir with a workflow config inside it.
+	dataDir := t.TempDir()
+	handler.SetDataDir(dataDir)
+
+	// Create a valid workflow.yaml inside the allowed dir.
+	wfDir := filepath.Join(dataDir, "my-workflow")
+	if err := os.MkdirAll(wfDir, 0o750); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+	wfFile := filepath.Join(wfDir, "workflow.yaml")
+	if err := os.WriteFile(wfFile, []byte("name: test\npipelines: []\n"), 0o640); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	token := generateTestToken(secret, "1", "admin@test.com", "admin")
+
+	body := fmt.Sprintf(`{"path": %q, "project_id": "00000000-0000-0000-0000-000000000002"}`, wfDir)
+	rr := doRequest(handler, "POST", "/api/v1/workflows/load-from-path", body, token)
+
+	if rr.Code != http.StatusCreated {
+		t.Errorf("expected 201, got %d (body: %s)", rr.Code, rr.Body.String())
+	}
+}

plugin/storebrowser/handler.go

@@ -172,7 +172,16 @@
 
 	orderClause := ""
 	if sortCol != "" {
-		// Validate the column name exists in this table.
+		// Reject sort column names that contain non-identifier characters before
+		// even querying the database. This gives static analysis tools (e.g. CodeQL)
+		// a clear, local sanitizer to track, and prevents any injection attempt
+		// from reaching the DB-schema lookup.
+		if !isValidTableName(sortCol) {
+			writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid sort column: %s", sortCol))
+			return
+		}
+		// Cross-check the column name against the actual schema of this table so
+		// that we never reference a column that does not exist.
 		validCols, err := getTableColumns(h.db, tableName)
 		if err != nil {
 			writeError(w, http.StatusInternalServerError, fmt.Sprintf("get columns: %v", err))
@@ -180,16 +189,20 @@
 		}
 		if !validCols[sortCol] {
 			writeError(w, http.StatusBadRequest, fmt.Sprintf("invalid sort column: %s", sortCol))
 			return
 		}
-		orderClause = fmt.Sprintf(" ORDER BY %s %s", sortCol, order)
-	}
-
-	// tableName is validated against the allowlist returned by getValidTables() above,
-	// and orderClause uses a column validated against getTableColumns(). Both are safe
-	// from injection. Parameters are bound via ? placeholders.
-	query := fmt.Sprintf("SELECT * FROM %s%s LIMIT ? OFFSET ?", tableName, orderClause) //nolint:gosec // tableName and orderClause are validated against DB schema above
-	rows, err := h.db.QueryContext(r.Context(), query, limit, offset)                   //nolint:gosec // G701: query built from validated table name and column
+		// Both sortCol (identifier-validated + schema-checked) and order (normalised
+		// to the literal strings "ASC"/"DESC" above) are safe to interpolate.
+		orderClause = fmt.Sprintf(" ORDER BY %s %s", sortCol, order) //nolint:gosec // G201: sortCol validated by isValidTableName + schema allowlist; order is "ASC"|"DESC"
+	}
+
+	// tableName is validated against the allowlist returned by getValidTables() above.
+	// orderClause is empty or " ORDER BY <col> ASC|DESC" where <col> passed
+	// isValidTableName (identifier regexp) and was cross-checked against the live
+	// DB schema. LIMIT/OFFSET are bound as ? placeholders. No user input is
+	// interpolated without prior validation.
+	query := fmt.Sprintf("SELECT * FROM %s%s LIMIT ? OFFSET ?", tableName, orderClause) //nolint:gosec // G201: identifiers validated; values parameterised
+	rows, err := h.db.QueryContext(r.Context(), query, limit, offset)
 	if err != nil {
 		writeError(w, http.StatusInternalServerError, fmt.Sprintf("query rows: %v", err))
 		return
@@ -257,7 +270,18 @@
 	}
 	defer func() { _ = tx.Rollback() }()
 
-	rows, err := tx.QueryContext(r.Context(), q) //nolint:gosec // G701: query validated by sanitizeReadOnlyQuery
+	// The query is user-supplied SQL executed in a read-only transaction (BeginTx
+	// with TxOptions{ReadOnly:true}).  Before reaching this point the query has
+	// been validated by:
+	//   1. sanitizeReadOnlyQuery — enforces SELECT-only, no statement separators (;),
+	//      no SQL comments (-- / /*).
+	//   2. dangerousKeywords regexp — blocks DROP/DELETE/INSERT/UPDATE/ALTER/CREATE
+	//      and other write keywords regardless of case.
+	// Parameterisation is intentionally impossible here because the user supplies
+	// the full SQL structure (this is an admin read-only SQL REPL). The read-only
+	// transaction boundary means write operations would be rolled back even if
+	// they somehow bypassed the keyword checks.
+	rows, err := tx.QueryContext(r.Context(), q) //nolint:gosec // G201: admin-only read-only SQL REPL; query validated by sanitizeReadOnlyQuery + dangerousKeywords + ReadOnly tx
 	if err != nil {
 		writeError(w, http.StatusBadRequest, fmt.Sprintf("query error: %v", err))
 		return