feat: enhance workflow runner status, cleanup step execution, and shell injection detection, while optimizing context cloning and improving secret decryption error handling.

Author: mark-hingston

src/adapters/secrets.ts

@@ -82,20 +82,20 @@ export interface EncryptedData {
  */
 export function isSecretExpression(expression: string, secretInputs: string[]): boolean {
   const trimmed = expression.trim();
-  
+
   // All environment variables are treated as secrets
   if (trimmed.startsWith("env.")) {
     return true;
   }
-  
+
   // Check if it's a secret input
   if (trimmed.startsWith("inputs.")) {
     const inputPath = trimmed.slice("inputs.".length);
     // Get the root input name (before any dots for nested access)
     const inputName = inputPath.split(".")[0];
     return secretInputs.includes(inputName);
   }
-  
+
   return false;
 }
 
@@ -109,14 +109,14 @@ export function isSecretExpression(expression: string, secretInputs: string[]):
 export function extractSecretExpressions(template: string, secretInputs: string[]): string[] {
   const pattern = /\{\{([^}]+)\}\}/g;
   const secrets: string[] = [];
-  
+
   for (const match of template.matchAll(pattern)) {
     const expression = match[1].trim();
     if (isSecretExpression(expression, secretInputs)) {
       secrets.push(expression);
     }
   }
-  
+
   return secrets;
 }
 
@@ -148,12 +148,12 @@ export function maskSecretValue(value: string): string {
   if (!value || value.length === 0) {
     return SECRET_MASK;
   }
-  
+
   // For short values, completely mask
   if (value.length <= 4) {
     return SECRET_MASK;
   }
-  
+
   // For longer values, show first character + mask
   return value[0] + SECRET_MASK;
 }
@@ -172,18 +172,18 @@ export function maskInterpolatedString(
   secretValues: Map<string, string>
 ): string {
   let masked = interpolated;
-  
+
   // Sort by value length (longest first) to handle overlapping values correctly
   const sortedSecrets = Array.from(secretValues.entries())
     .sort(([, a], [, b]) => b.length - a.length);
-  
+
   for (const [, value] of sortedSecrets) {
     if (value && value.length > 0) {
       // Replace all occurrences of the secret value with the mask
       masked = masked.split(value).join(SECRET_MASK);
     }
   }
-  
+
   return masked;
 }
 
@@ -201,7 +201,7 @@ function validateEncryptionKey(encryptionKey: string): void {
   if (!encryptionKey || encryptionKey.length < MIN_KEY_LENGTH) {
     throw new Error(
       `Encryption key must be at least ${MIN_KEY_LENGTH} characters long for security. ` +
-      `Provided key is ${encryptionKey?.length || 0} characters.`
+      `Provided key does not meet security requirements.`
     );
   }
 }
@@ -229,22 +229,22 @@ function deriveKey(password: string, salt: Buffer): Buffer {
  */
 export function encryptForStorage(data: string, encryptionKey: string): EncryptedData {
   validateEncryptionKey(encryptionKey);
-  
+
   const salt = randomBytes(SALT_LENGTH);
   const key = deriveKey(encryptionKey, salt);
   const iv = randomBytes(IV_LENGTH);
-  
+
   const cipher = createCipheriv(ENCRYPTION_ALGORITHM, key, iv);
   const encrypted = Buffer.concat([
     cipher.update(data, "utf8"),
     cipher.final(),
   ]);
-  
+
   const authTag = cipher.getAuthTag();
-  
+
   // Combine salt + iv + authTag + encrypted data
   const combined = Buffer.concat([salt, iv, authTag, encrypted]);
-  
+
   return {
     encrypted: true,
     data: combined.toString("base64"),
@@ -261,25 +261,25 @@ export function encryptForStorage(data: string, encryptionKey: string): Encrypte
  */
 export function decryptFromStorage(encryptedData: EncryptedData, encryptionKey: string): string {
   validateEncryptionKey(encryptionKey);
-  
+
   const combined = Buffer.from(encryptedData.data, "base64");
-  
+
   // Extract components
   const salt = combined.subarray(0, SALT_LENGTH);
   const iv = combined.subarray(SALT_LENGTH, SALT_LENGTH + IV_LENGTH);
   const authTag = combined.subarray(SALT_LENGTH + IV_LENGTH, SALT_LENGTH + IV_LENGTH + AUTH_TAG_LENGTH);
   const encrypted = combined.subarray(SALT_LENGTH + IV_LENGTH + AUTH_TAG_LENGTH);
-  
+
   const key = deriveKey(encryptionKey, salt);
-  
+
   const decipher = createDecipheriv(ENCRYPTION_ALGORITHM, key, iv);
   decipher.setAuthTag(authTag);
-  
+
   const decrypted = Buffer.concat([
     decipher.update(encrypted),
     decipher.final(),
   ]);
-  
+
   return decrypted.toString("utf8");
 }
 
@@ -315,7 +315,7 @@ export function encryptSecretInputs(
   encryptionKey: string
 ): Record<string, unknown> {
   const result: Record<string, unknown> = {};
-  
+
   for (const [key, value] of Object.entries(inputs)) {
     if (secretKeys.includes(key) && value !== undefined && value !== null) {
       // Encrypt the secret value
@@ -325,7 +325,7 @@ export function encryptSecretInputs(
       result[key] = value;
     }
   }
-  
+
   return result;
 }
 
@@ -341,7 +341,7 @@ export function decryptSecretInputs(
   encryptionKey: string
 ): Record<string, unknown> {
   const result: Record<string, unknown> = {};
-  
+
   for (const [key, value] of Object.entries(inputs)) {
     if (isEncryptedData(value)) {
       try {
@@ -352,14 +352,16 @@ export function decryptSecretInputs(
         } catch {
           result[key] = decrypted;
         }
-      } catch {
-        // If decryption fails, keep the encrypted value
+      } catch (error) {
+        // Log warning for decryption failure - helps debug key rotation issues
+        console.warn(`[secrets] Failed to decrypt input '${key}': ${error instanceof Error ? error.message : 'Unknown error'}`);
+        // Keep the encrypted value to avoid data loss
         result[key] = value;
       }
     } else {
       result[key] = value;
     }
   }
-  
+
   return result;
 }

src/adapters/steps.ts

@@ -216,13 +216,21 @@ export async function cleanupAllProcesses(): Promise<void> {
  * These are logged as warnings but not blocked to maintain flexibility.
  */
 const SHELL_DANGEROUS_PATTERNS = [
-  /;\s*rm\s/i,      // rm after semicolon
-  /\|\s*sh\b/i,     // piping to shell
-  /\|\s*bash\b/i,   // piping to bash
-  /`[^`]+`/,        // backtick command substitution
-  /\$\([^)]+\)/,    // $() command substitution
-  />\s*\/etc\//i,   // writing to /etc
-  />\s*\/bin\//i,   // writing to /bin
+  /;\s*rm\s/i,        // rm after semicolon
+  /&&\s*rm\s/i,       // rm after &&
+  /\|\|\s*rm\s/i,     // rm after ||
+  /&\s*rm\s/i,        // rm after background operator
+  /\|\s*sh\b/i,       // piping to shell
+  /\|\s*bash\b/i,     // piping to bash
+  /\|\s*zsh\b/i,      // piping to zsh
+  /`[^`]+`/,          // backtick command substitution
+  /\$\([^)]+\)/,      // $() command substitution
+  /\$\{[^}]+\}/,      // ${} parameter expansion with commands
+  />\s*\/etc\//i,     // writing to /etc
+  />\s*\/bin\//i,     // writing to /bin
+  />\s*\/usr\/bin\//i, // writing to /usr/bin
+  /curl.*\|.*sh/i,    // curl piped to shell
+  /wget.*\|.*sh/i,    // wget piped to shell
 ];
 
 /**
@@ -1596,8 +1604,9 @@ function createSandboxContext(
 
   return {
     // Workflow context (read-only via frozen copies)
-    inputs: Object.freeze(JSON.parse(JSON.stringify(inputs))),
-    steps: Object.freeze(JSON.parse(JSON.stringify(steps))),
+    // Use structuredClone for better performance than JSON.parse(JSON.stringify())
+    inputs: Object.freeze(structuredClone(inputs)),
+    steps: Object.freeze(structuredClone(steps)),
     env: frozenEnv,
 
     // Console mapped to plugin logger (or silent if no logger provided)

src/commands/runner.ts

@@ -201,10 +201,15 @@ export class WorkflowRunner {
         this.stopStatusInterval(runId);
         return;
       }
-      const totalSteps = Object.keys(run.stepResults || {}).length;
+      const completedSteps = Object.keys(run.stepResults || {}).length;
       const current = run.currentStepId ? `current: ${run.currentStepId}` : "current: n/a";
       const summary = this.progress?.getStepSummary(runId);
-      const msg = `Status: ${run.status} (${current}, completed steps: ${totalSteps})${summary ? ` • last: ${summary}` : ""}`;
+      // Get total steps from workflow definition if available
+      const compiled = this.factory.get(run.workflowId);
+      const totalSteps = compiled?.stepCount ?? 0;
+      const progressPct = totalSteps > 0 ? Math.round((completedSteps / totalSteps) * 100) : 0;
+      const progressStr = totalSteps > 0 ? ` (${completedSteps}/${totalSteps} steps, ${progressPct}%)` : ` (${completedSteps} steps completed)`;
+      const msg = `Status: ${run.status}${progressStr} • ${current}${summary ? ` • last: ${summary}` : ""}`;
       void this.progress?.emit(msg, { runId, level: "info", force: true });
     }, this.statusIntervalMs);
     this.statusIntervals.set(runId, interval);
@@ -258,8 +263,8 @@ export class WorkflowRunner {
       });
 
     // Load recent completed runs in background with pagination (non-blocking)
-    // Only load the most recent 100 runs to avoid memory issues with large histories
-    const RECENT_RUNS_LIMIT = 100;
+    // Only load the most recent 30 runs to reduce memory usage and startup time
+    const RECENT_RUNS_LIMIT = 30;
     this.storage
       .loadAllRuns(undefined, RECENT_RUNS_LIMIT, 0)
       .then((recentRuns) => {
@@ -367,12 +372,14 @@ export class WorkflowRunner {
    * @param steps - The cleanup step definitions to execute
    * @param run - The current workflow run (for context)
    * @param compiled - The compiled workflow result
+   * @param prefix - Prefix for step result keys ('onFailure' or 'finally')
    * @param errorInfo - Optional error information if this is an onFailure block
    */
   private async executeCleanupSteps(
     steps: StepDefinition[],
     run: WorkflowRun,
     compiled: WorkflowFactoryResult,
+    prefix: 'onFailure' | 'finally',
     errorInfo?: { message: string; stepId?: string }
   ): Promise<void> {
     const client = this.factory.getClient();
@@ -394,8 +401,9 @@ export class WorkflowRunner {
     };
 
     for (const stepDef of steps) {
+      const prefixedId = `${prefix}:${stepDef.id}`;
       try {
-        this.log.info(`Executing cleanup step: ${stepDef.id}`);
+        this.log.info(`Executing ${prefix} step: ${stepDef.id}`);
 
         const result = await executeInnerStep(
           stepDef,
@@ -407,22 +415,22 @@ export class WorkflowRunner {
         // Store the result for subsequent cleanup steps to reference
         ctx.steps[stepDef.id] = result;
 
-        // Save to run results with a "cleanup:" prefix to distinguish from main steps
-        run.stepResults[`cleanup:${stepDef.id}`] = {
-          stepId: `cleanup:${stepDef.id}`,
+        // Save to run results with prefix to distinguish cleanup type
+        run.stepResults[prefixedId] = {
+          stepId: prefixedId,
           status: "success",
           output: result as StepOutput,
           startedAt: new Date(),
           completedAt: new Date(),
         };
 
-        this.log.info(`Cleanup step ${stepDef.id} completed`);
+        this.log.info(`${prefix} step ${stepDef.id} completed`);
       } catch (error) {
         // Log but don't throw - cleanup steps should not mask the original error
-        this.log.error(`Cleanup step ${stepDef.id} failed: ${error}`);
+        this.log.error(`${prefix} step ${stepDef.id} failed: ${error}`);
 
-        run.stepResults[`cleanup:${stepDef.id}`] = {
-          stepId: `cleanup:${stepDef.id}`,
+        run.stepResults[prefixedId] = {
+          stepId: prefixedId,
           status: "failed",
           error: String(error),
           startedAt: new Date(),
@@ -724,6 +732,7 @@ export class WorkflowRunner {
         compiled.onFailureSteps,
         run,
         compiled,
+        'onFailure',
         { message: workflowError.message, stepId: failedStepId }
       );
     }
@@ -735,6 +744,7 @@ export class WorkflowRunner {
         compiled.finallySteps,
         run,
         compiled,
+        'finally',
         workflowError ? { message: workflowError.message, stepId: failedStepId } : undefined
       );
     }
@@ -953,6 +963,7 @@ export class WorkflowRunner {
         compiled.onFailureSteps,
         run,
         compiled,
+        'onFailure',
         { message: workflowError.message, stepId: failedStepId }
       );
     }
@@ -964,6 +975,7 @@ export class WorkflowRunner {
         compiled.finallySteps,
         run,
         compiled,
+        'finally',
         workflowError ? { message: workflowError.message, stepId: failedStepId } : undefined
       );
     }