Skip to content

Commit 4c2fbef

Browse files
OffLine911OffLine911
committed
feat(security,execution): Add cycle detection and zip slip prevention
- Add cycle detection in workflow executor to prevent infinite loops during node execution - Implement zip slip vulnerability prevention in archive extraction with path validation - Replace unsafe eval() with Function constructor for sandboxed condition evaluation - Add executingStack tracking to detect and skip nodes already in execution call stack - Improve condition evaluation in both WorkflowExecutor and conditions handler - Add LoadExecution import and enhance setSelectedExecution to load full execution data from storage - Update ImportExport component to use nodeCount property when available - Add string import to actions.go for path validation logic
1 parent 6d472a0 commit 4c2fbef

11 files changed

Lines changed: 113 additions & 29 deletions

File tree

‎actions.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"os"
1111
"os/exec"
1212
"path/filepath"
13+
"strings"
1314
"time"
1415
)
1516

@@ -240,7 +241,11 @@ func (as *ActionService) Extract(src, dest string) error {
240241
os.MkdirAll(dest, 0755)
241242

242243
for _, f := range r.File {
244+
// Prevent Zip Slip: ensure resolved path stays within dest
243245
path := filepath.Join(dest, f.Name)
246+
if !strings.HasPrefix(filepath.Clean(path), filepath.Clean(dest)+string(os.PathSeparator)) {
247+
return fmt.Errorf("illegal file path in archive: %s", f.Name)
248+
}
244249
if f.FileInfo().IsDir() {
245250
os.MkdirAll(path, f.Mode())
246251
continue

‎engine.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -150,19 +150,25 @@ func (e *Engine) executeFlow(ctx context.Context, flow *Flow, execution *FlowExe
150150
}
151151
}
152152

153+
executed := make(map[string]bool)
153154
for _, nodeID := range startNodes {
154155
if ctx.Err() != nil {
155156
return
156157
}
157-
e.executeNode(ctx, nodeMap[nodeID], execution, nodeMap, adjacency)
158+
e.executeNode(ctx, nodeMap[nodeID], execution, nodeMap, adjacency, executed)
158159
}
159160
}
160161

161-
func (e *Engine) executeNode(ctx context.Context, node *FlowNode, execution *FlowExecution, nodeMap map[string]*FlowNode, adjacency map[string][]string) {
162+
func (e *Engine) executeNode(ctx context.Context, node *FlowNode, execution *FlowExecution, nodeMap map[string]*FlowNode, adjacency map[string][]string, executed map[string]bool) {
162163
if ctx.Err() != nil {
163164
return
164165
}
165166

167+
if executed[node.ID] {
168+
return
169+
}
170+
executed[node.ID] = true
171+
166172
start := time.Now()
167173
result := ExecutionResult{
168174
NodeID: node.ID,
@@ -186,7 +192,7 @@ func (e *Engine) executeNode(ctx context.Context, node *FlowNode, execution *Flo
186192

187193
for _, nextID := range adjacency[node.ID] {
188194
if nextNode, ok := nodeMap[nextID]; ok {
189-
e.executeNode(ctx, nextNode, execution, nodeMap, adjacency)
195+
e.executeNode(ctx, nextNode, execution, nodeMap, adjacency, executed)
190196
}
191197
}
192198
}

‎frontend/src/components/layout/ImportExport.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,7 +294,7 @@ export default function ImportExport({ onClose }: ImportExportProps) {
294294
{flow.name}
295295
</div>
296296
<div className="text-xs text-[#858585]">
297-
{flow.nodes.length} nodes • Updated {new Date(flow.updatedAt).toLocaleDateString()}
297+
{(flow as any).nodeCount ?? flow.nodes.length} nodes • Updated {new Date(flow.updatedAt).toLocaleDateString()}
298298
</div>
299299
</div>
300300
</div>

‎frontend/src/executor/WorkflowExecutor.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ export class WorkflowExecutor {
2424
private onProgress: (results: NodeResult[]) => void;
2525
private onLog: LogCallback;
2626
private isAborted: boolean = false;
27+
private executingStack: Set<string> = new Set();
2728

2829
constructor(
2930
nodes: FlowNode[],
@@ -86,6 +87,12 @@ export class WorkflowExecutor {
8687
const node = this.nodes.find(n => n.id === nodeId);
8788
if (!node) return null;
8889

90+
// Cycle detection: skip if this node is already on the call stack
91+
if (this.executingStack.has(nodeId)) {
92+
this.onLog('warn', `⚠️ Cycle detected at: ${node.data.label}, skipping`, nodeId);
93+
return null;
94+
}
95+
8996
// Check if disabled (using config.disabled instead of status)
9097
if (node.data.config?.disabled === true) {
9198
this.updateNodeResult(nodeId, {
@@ -106,6 +113,7 @@ export class WorkflowExecutor {
106113
// Execute node
107114
this.onLog('info', `▶️ Executing: ${node.data.label}`, nodeId);
108115
this.updateNodeResult(nodeId, { status: 'running', startedAt: Date.now() });
116+
this.executingStack.add(nodeId);
109117

110118
try {
111119
const output = await this.runNode(node);
@@ -191,8 +199,10 @@ export class WorkflowExecutor {
191199

192200
let result = false;
193201
try {
194-
// Note: Simple eval for now. In production, use a safe evaluator.
195-
result = !!eval(currentCondition);
202+
const varKeys = Object.keys(this.variables);
203+
const varValues = varKeys.map(k => this.variables[k]);
204+
const fn = new Function(...varKeys, `"use strict"; return (${currentCondition});`);
205+
result = !!fn(...varValues);
196206
} catch (e) {
197207
this.onLog('error', `❌ Loop condition error: ${e}`, nodeId);
198208
break;
@@ -397,6 +407,8 @@ export class WorkflowExecutor {
397407
});
398408
this.onLog('error', `❌ Failed: ${node.data.label} - ${errorMsg}`, nodeId);
399409
throw error;
410+
} finally {
411+
this.executingStack.delete(nodeId);
400412
}
401413
}
402414

‎frontend/src/handlers/conditions.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,11 @@ export const conditionHandlers: Record<string, (ctx: HandlerContext) => Promise<
1717
evaluatedCondition = evaluatedCondition.replace(regex, JSON.stringify(value));
1818
}
1919

20-
// Simple evaluation (UNSAFE - use proper evaluator in production)
21-
const result = eval(evaluatedCondition);
20+
// Sandboxed evaluation via Function constructor (no access to local scope)
21+
const varKeys = Object.keys(variables);
22+
const varValues = varKeys.map(k => variables[k]);
23+
const fn = new Function(...varKeys, `"use strict"; return (${evaluatedCondition});`);
24+
const result = fn(...varValues);
2225
const boolResult = Boolean(result);
2326

2427
onLog('success', `${boolResult ? '✓' : '✗'} Condition: ${boolResult ? 'TRUE' : 'FALSE'}`);

‎frontend/src/stores/executionStore.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { create } from "zustand";
22
import type { FlowExecution } from "@/types/flow";
3-
import { ListExecutions, DeleteExecution, SaveExecution } from "../../wailsjs/go/main/Storage";
3+
import { ListExecutions, DeleteExecution, SaveExecution, LoadExecution } from "../../wailsjs/go/main/Storage";
44
import { toast } from "@/stores/dialogStore";
55

66
interface ExecutionState {
@@ -111,5 +111,18 @@ export const useExecutionStore = create<ExecutionState>()((set, get) => ({
111111
}
112112
},
113113

114-
setSelectedExecution: (execution) => set({ selectedExecution: execution }),
114+
setSelectedExecution: async (execution) => {
115+
if (!execution) {
116+
set({ selectedExecution: null });
117+
return;
118+
}
119+
// Load full execution data (with results) from storage
120+
try {
121+
const fullJSON = await LoadExecution(execution.id);
122+
const full = JSON.parse(fullJSON) as FlowExecution;
123+
set({ selectedExecution: full });
124+
} catch {
125+
set({ selectedExecution: execution });
126+
}
127+
},
115128
}));

‎frontend/src/stores/flowStore.ts‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -446,14 +446,17 @@ export const useFlowStore = create<FlowState>()((set, get) => ({
446446

447447
const flowName = flow.name || 'Unknown';
448448

449-
// Unregister triggers before deleting
450-
if (flow.nodes && flow.nodes.length > 0) {
451-
try {
449+
// Load full flow data to get nodes for trigger unregistration
450+
try {
451+
const { LoadFlow } = await import('../../wailsjs/go/main/Storage');
452+
const fullFlowJSON = await LoadFlow(flowId);
453+
const fullFlow = JSON.parse(fullFlowJSON);
454+
if (fullFlow.nodes && fullFlow.nodes.length > 0) {
452455
const { TriggerService } = await import('@/services/triggerService');
453-
await TriggerService.unregisterWorkflowTriggers(flowId, flow.nodes);
454-
} catch (error) {
455-
console.error('Failed to unregister triggers:', error);
456+
await TriggerService.unregisterWorkflowTriggers(flowId, fullFlow.nodes);
456457
}
458+
} catch (error) {
459+
console.error('Failed to unregister triggers:', error);
457460
}
458461

459462
try {

‎frontend/wailsjs/go/main/Storage.d.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ export function ListExecutions(arg1:number):Promise<Array<Record<string, any>>>;
1919

2020
export function ListFlows():Promise<Array<Record<string, any>>>;
2121

22+
export function LoadExecution(arg1:string):Promise<string>;
23+
2224
export function LoadFlow(arg1:string):Promise<string>;
2325

2426
export function LoadSettings():Promise<string>;

‎frontend/wailsjs/go/main/Storage.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ export function ListFlows() {
3838
return window['go']['main']['Storage']['ListFlows']();
3939
}
4040

41+
export function LoadExecution(arg1) {
42+
return window['go']['main']['Storage']['LoadExecution'](arg1);
43+
}
44+
4145
export function LoadFlow(arg1) {
4246
return window['go']['main']['Storage']['LoadFlow'](arg1);
4347
}

‎storage.go‎

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,9 @@ func (s *Storage) getFlowsDir() string {
4242
}
4343

4444
func (s *Storage) SaveFlow(flowJSON string) (string, error) {
45-
s.Init()
45+
if err := s.Init(); err != nil {
46+
return "", err
47+
}
4648

4749
// Parse as generic map to avoid struct issues
4850
var flowData map[string]interface{}
@@ -74,7 +76,9 @@ func (s *Storage) SaveFlow(flowJSON string) (string, error) {
7476
}
7577

7678
func (s *Storage) LoadFlow(flowID string) (string, error) {
77-
s.Init()
79+
if err := s.Init(); err != nil {
80+
return "", err
81+
}
7882

7983
filePath := filepath.Join(s.getFlowsDir(), flowID+".json")
8084
data, err := os.ReadFile(filePath)
@@ -89,7 +93,9 @@ func (s *Storage) GetFlow(flowID string) (string, error) {
8993
}
9094

9195
func (s *Storage) ListFlows() ([]map[string]interface{}, error) {
92-
s.Init()
96+
if err := s.Init(); err != nil {
97+
return nil, err
98+
}
9399

94100
flowsDir := s.getFlowsDir()
95101
entries, err := os.ReadDir(flowsDir)
@@ -142,19 +148,25 @@ func (s *Storage) ListFlows() ([]map[string]interface{}, error) {
142148
}
143149

144150
func (s *Storage) DeleteFlow(flowID string) error {
145-
s.Init()
151+
if err := s.Init(); err != nil {
152+
return err
153+
}
146154
filePath := filepath.Join(s.getFlowsDir(), flowID+".json")
147155
return os.Remove(filePath)
148156
}
149157

150158
func (s *Storage) SaveSettings(settingsJSON string) error {
151-
s.Init()
159+
if err := s.Init(); err != nil {
160+
return err
161+
}
152162
filePath := filepath.Join(s.dataDir, "settings.json")
153163
return os.WriteFile(filePath, []byte(settingsJSON), 0644)
154164
}
155165

156166
func (s *Storage) LoadSettings() (string, error) {
157-
s.Init()
167+
if err := s.Init(); err != nil {
168+
return "", err
169+
}
158170
filePath := filepath.Join(s.dataDir, "settings.json")
159171
data, err := os.ReadFile(filePath)
160172
if err != nil {
@@ -170,7 +182,9 @@ func (s *Storage) getExecutionsDir() string {
170182
}
171183

172184
func (s *Storage) SaveExecution(executionJSON string) error {
173-
s.Init()
185+
if err := s.Init(); err != nil {
186+
return err
187+
}
174188

175189
var execution map[string]interface{}
176190
if err := json.Unmarshal([]byte(executionJSON), &execution); err != nil {
@@ -192,7 +206,9 @@ func (s *Storage) SaveExecution(executionJSON string) error {
192206
}
193207

194208
func (s *Storage) ListExecutions(limit int) ([]map[string]interface{}, error) {
195-
s.Init()
209+
if err := s.Init(); err != nil {
210+
return nil, err
211+
}
196212

197213
execDir := s.getExecutionsDir()
198214
entries, err := os.ReadDir(execDir)
@@ -260,16 +276,34 @@ func (s *Storage) ListExecutions(limit int) ([]map[string]interface{}, error) {
260276
}
261277

262278
func (s *Storage) DeleteExecution(execID string) error {
263-
s.Init()
279+
if err := s.Init(); err != nil {
280+
return err
281+
}
264282
filePath := filepath.Join(s.getExecutionsDir(), execID+".json")
265283
return os.Remove(filePath)
266284
}
267285

286+
func (s *Storage) LoadExecution(execID string) (string, error) {
287+
if err := s.Init(); err != nil {
288+
return "", err
289+
}
290+
filePath := filepath.Join(s.getExecutionsDir(), execID+".json")
291+
data, err := os.ReadFile(filePath)
292+
if err != nil {
293+
return "", fmt.Errorf("execution not found: %s", execID)
294+
}
295+
return string(data), nil
296+
}
297+
268298
func (s *Storage) ExportFlow(flowID string) (string, error) {
269299
return s.LoadFlow(flowID)
270300
}
271301

272302
func (s *Storage) ImportFlow(flowJSON string) (string, error) {
303+
if err := s.Init(); err != nil {
304+
return "", err
305+
}
306+
273307
var flow Flow
274308
if err := json.Unmarshal([]byte(flowJSON), &flow); err != nil {
275309
return "", fmt.Errorf("invalid flow JSON: %w", err)

0 commit comments

Comments
 (0)