mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-02-07 14:03:08 +00:00
fix: critical telemetry improvements for data quality and security (#421)
* fix: critical telemetry improvements for data quality and security Fixed three critical issues in workflow mutation telemetry: 1. Fixed Inconsistent Sanitization (Security Critical) - Problem: 30% of workflows unsanitized, exposing credentials/tokens - Solution: Use robust WorkflowSanitizer.sanitizeWorkflowRaw() - Impact: 100% sanitization with 17 sensitive patterns redacted - Files: workflow-sanitizer.ts, mutation-tracker.ts 2. Enabled Validation Data Capture (Data Quality) - Problem: Zero validation metrics captured (all NULL) - Solution: Add pre/post mutation validation with WorkflowValidator - Impact: Measure mutation quality, track error resolution - Non-blocking validation that captures errors/warnings - Files: handlers-workflow-diff.ts 3. Improved Intent Capture (Data Quality) - Problem: 92.62% generic "Partial workflow update" intents - Solution: Enhanced docs + automatic intent inference - Impact: Meaningful intents auto-generated from operations - Files: n8n-update-partial-workflow.ts, handlers-workflow-diff.ts Expected Results: - 100% sanitization coverage (up from 70%) - 100% validation capture (up from 0%) - 50%+ meaningful intents (up from 7.33%) Version bumped to 2.22.17 🤖 Generated with [Claude Code](https://claude.com/claude-code) Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en Co-Authored-By: Claude <noreply@anthropic.com> * perf: implement validator instance caching to avoid redundant initialization - Add module-level cached WorkflowValidator instance - Create getValidator() helper to reuse validator across mutations - Update pre/post mutation validation to use cached instance - Avoids redundant NodeSimilarityService initialization on every mutation Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: restore backward-compatible sanitization with context preservation Fixed CI test failures by updating WorkflowSanitizer to use pattern-specific placeholders while maintaining backward compatibility: Changes: - Convert SENSITIVE_PATTERNS to PatternDefinition objects with specific placeholders - Update sanitizeString() to preserve context (Bearer prefix, URL paths) - Refactor sanitizeObject() to handle sensitive fields vs URL fields differently - Remove overly greedy field patterns that conflicted with token patterns Pattern-specific placeholders: - [REDACTED_URL_WITH_AUTH] for URLs with credentials - [REDACTED_TOKEN] for long tokens (32+ chars) - [REDACTED_APIKEY] for OpenAI-style keys - Bearer [REDACTED] for Bearer tokens (preserves "Bearer " prefix) - [REDACTED] for generic sensitive fields Test Results: - All 13 mutation-tracker tests passing - URL with auth: preserves path after credentials - Long tokens: properly detected and marked - OpenAI keys: correctly identified - Bearer tokens: prefix preserved - Sensitive field names: generic redaction for non-URL fields Fixes #421 CI failures Conceived by Romuald Członkowski - https://www.aiadvisors.pl/en 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: prevent double-redaction in workflow sanitizer Added safeguard to stop pattern matching once a placeholder is detected, preventing token patterns from matching text inside placeholders like [REDACTED_URL_WITH_AUTH]. Also expanded database URL pattern to match full URLs including port and path, and updated test expectations to match context-preserving sanitization. Fixes: - Database URLs now properly sanitized to [REDACTED_URL_WITH_AUTH] - Prevents [[REDACTED]] double-redaction issue - All 25 workflow-sanitizer tests passing - No regression in mutation-tracker tests Conceived by Romuald Członkowski - www.aiadvisors.pl/en --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Romuald Członkowski
committed by
GitHub
GitHub
parent
99c5907b71
commit
597bd290b6
@@ -14,6 +14,22 @@ import { InstanceContext } from '../types/instance-context';
|
||||
import { validateWorkflowStructure } from '../services/n8n-validation';
|
||||
import { NodeRepository } from '../database/node-repository';
|
||||
import { WorkflowVersioningService } from '../services/workflow-versioning-service';
|
||||
import { WorkflowValidator } from '../services/workflow-validator';
|
||||
import { EnhancedConfigValidator } from '../services/enhanced-config-validator';
|
||||
|
||||
// Cached validator instance to avoid recreating on every mutation
|
||||
let cachedValidator: WorkflowValidator | null = null;
|
||||
|
||||
/**
|
||||
* Get or create cached workflow validator instance
|
||||
* Reuses the same validator to avoid redundant NodeSimilarityService initialization
|
||||
*/
|
||||
function getValidator(repository: NodeRepository): WorkflowValidator {
|
||||
if (!cachedValidator) {
|
||||
cachedValidator = new WorkflowValidator(repository, EnhancedConfigValidator);
|
||||
}
|
||||
return cachedValidator;
|
||||
}
|
||||
|
||||
// Zod schema for the diff request
|
||||
const workflowDiffSchema = z.object({
|
||||
@@ -62,6 +78,8 @@ export async function handleUpdatePartialWorkflow(
|
||||
const startTime = Date.now();
|
||||
const sessionId = `mutation_${Date.now()}_${Math.random().toString(36).slice(2, 11)}`;
|
||||
let workflowBefore: any = null;
|
||||
let validationBefore: any = null;
|
||||
let validationAfter: any = null;
|
||||
|
||||
try {
|
||||
// Debug logging (only in debug mode)
|
||||
@@ -92,6 +110,24 @@ export async function handleUpdatePartialWorkflow(
|
||||
workflow = await client.getWorkflow(input.id);
|
||||
// Store original workflow for telemetry
|
||||
workflowBefore = JSON.parse(JSON.stringify(workflow));
|
||||
|
||||
// Validate workflow BEFORE mutation (for telemetry)
|
||||
try {
|
||||
const validator = getValidator(repository);
|
||||
validationBefore = await validator.validateWorkflow(workflowBefore, {
|
||||
validateNodes: true,
|
||||
validateConnections: true,
|
||||
validateExpressions: true,
|
||||
profile: 'runtime'
|
||||
});
|
||||
} catch (validationError) {
|
||||
logger.debug('Pre-mutation validation failed (non-blocking):', validationError);
|
||||
// Don't block mutation on validation errors
|
||||
validationBefore = {
|
||||
valid: false,
|
||||
errors: [{ type: 'validation_error', message: 'Validation failed' }]
|
||||
};
|
||||
}
|
||||
} catch (error) {
|
||||
if (error instanceof N8nApiError) {
|
||||
return {
|
||||
@@ -257,6 +293,24 @@ export async function handleUpdatePartialWorkflow(
|
||||
let finalWorkflow = updatedWorkflow;
|
||||
let activationMessage = '';
|
||||
|
||||
// Validate workflow AFTER mutation (for telemetry)
|
||||
try {
|
||||
const validator = getValidator(repository);
|
||||
validationAfter = await validator.validateWorkflow(finalWorkflow, {
|
||||
validateNodes: true,
|
||||
validateConnections: true,
|
||||
validateExpressions: true,
|
||||
profile: 'runtime'
|
||||
});
|
||||
} catch (validationError) {
|
||||
logger.debug('Post-mutation validation failed (non-blocking):', validationError);
|
||||
// Don't block on validation errors
|
||||
validationAfter = {
|
||||
valid: false,
|
||||
errors: [{ type: 'validation_error', message: 'Validation failed' }]
|
||||
};
|
||||
}
|
||||
|
||||
if (diffResult.shouldActivate) {
|
||||
try {
|
||||
finalWorkflow = await client.activateWorkflow(input.id);
|
||||
@@ -298,6 +352,8 @@ export async function handleUpdatePartialWorkflow(
|
||||
operations: input.operations,
|
||||
workflowBefore,
|
||||
workflowAfter: finalWorkflow,
|
||||
validationBefore,
|
||||
validationAfter,
|
||||
mutationSuccess: true,
|
||||
durationMs: Date.now() - startTime,
|
||||
}).catch(err => {
|
||||
@@ -330,6 +386,8 @@ export async function handleUpdatePartialWorkflow(
|
||||
operations: input.operations,
|
||||
workflowBefore,
|
||||
workflowAfter: workflowBefore, // No change since it failed
|
||||
validationBefore,
|
||||
validationAfter: validationBefore, // Same as before since mutation failed
|
||||
mutationSuccess: false,
|
||||
mutationError: error instanceof Error ? error.message : 'Unknown error',
|
||||
durationMs: Date.now() - startTime,
|
||||
@@ -365,11 +423,86 @@ export async function handleUpdatePartialWorkflow(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Infer intent from operations when not explicitly provided
|
||||
*/
|
||||
function inferIntentFromOperations(operations: any[]): string {
|
||||
if (!operations || operations.length === 0) {
|
||||
return 'Partial workflow update';
|
||||
}
|
||||
|
||||
const opTypes = operations.map((op) => op.type);
|
||||
const opCount = operations.length;
|
||||
|
||||
// Single operation - be specific
|
||||
if (opCount === 1) {
|
||||
const op = operations[0];
|
||||
switch (op.type) {
|
||||
case 'addNode':
|
||||
return `Add ${op.node?.type || 'node'}`;
|
||||
case 'removeNode':
|
||||
return `Remove node ${op.nodeName || op.nodeId || ''}`.trim();
|
||||
case 'updateNode':
|
||||
return `Update node ${op.nodeName || op.nodeId || ''}`.trim();
|
||||
case 'addConnection':
|
||||
return `Connect ${op.source || 'node'} to ${op.target || 'node'}`;
|
||||
case 'removeConnection':
|
||||
return `Disconnect ${op.source || 'node'} from ${op.target || 'node'}`;
|
||||
case 'rewireConnection':
|
||||
return `Rewire ${op.source || 'node'} from ${op.from || ''} to ${op.to || ''}`.trim();
|
||||
case 'updateName':
|
||||
return `Rename workflow to "${op.name || ''}"`;
|
||||
case 'activateWorkflow':
|
||||
return 'Activate workflow';
|
||||
case 'deactivateWorkflow':
|
||||
return 'Deactivate workflow';
|
||||
default:
|
||||
return `Workflow ${op.type}`;
|
||||
}
|
||||
}
|
||||
|
||||
// Multiple operations - summarize pattern
|
||||
const typeSet = new Set(opTypes);
|
||||
const summary: string[] = [];
|
||||
|
||||
if (typeSet.has('addNode')) {
|
||||
const count = opTypes.filter((t) => t === 'addNode').length;
|
||||
summary.push(`add ${count} node${count > 1 ? 's' : ''}`);
|
||||
}
|
||||
if (typeSet.has('removeNode')) {
|
||||
const count = opTypes.filter((t) => t === 'removeNode').length;
|
||||
summary.push(`remove ${count} node${count > 1 ? 's' : ''}`);
|
||||
}
|
||||
if (typeSet.has('updateNode')) {
|
||||
const count = opTypes.filter((t) => t === 'updateNode').length;
|
||||
summary.push(`update ${count} node${count > 1 ? 's' : ''}`);
|
||||
}
|
||||
if (typeSet.has('addConnection') || typeSet.has('rewireConnection')) {
|
||||
summary.push('modify connections');
|
||||
}
|
||||
if (typeSet.has('updateName') || typeSet.has('updateSettings')) {
|
||||
summary.push('update metadata');
|
||||
}
|
||||
|
||||
return summary.length > 0
|
||||
? `Workflow update: ${summary.join(', ')}`
|
||||
: `Workflow update: ${opCount} operations`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Track workflow mutation for telemetry
|
||||
*/
|
||||
async function trackWorkflowMutation(data: any): Promise<void> {
|
||||
try {
|
||||
// Enhance intent if it's missing or generic
|
||||
if (
|
||||
!data.userIntent ||
|
||||
data.userIntent === 'Partial workflow update' ||
|
||||
data.userIntent.length < 10
|
||||
) {
|
||||
data.userIntent = inferIntentFromOperations(data.operations);
|
||||
}
|
||||
|
||||
const { telemetry } = await import('../telemetry/telemetry-manager.js');
|
||||
await telemetry.trackWorkflowMutation(data);
|
||||
} catch (error) {
|
||||
|
||||
@@ -9,7 +9,8 @@ export const n8nUpdatePartialWorkflowDoc: ToolDocumentation = {
|
||||
example: 'n8n_update_partial_workflow({id: "wf_123", operations: [{type: "rewireConnection", source: "IF", from: "Old", to: "New", branch: "true"}]})',
|
||||
performance: 'Fast (50-200ms)',
|
||||
tips: [
|
||||
'Include intent parameter in every call - helps to return better responses',
|
||||
'ALWAYS provide intent parameter describing what you\'re doing (e.g., "Add error handling", "Fix webhook URL", "Connect Slack to error output")',
|
||||
'DON\'T use generic intent like "update workflow" or "partial update" - be specific about your goal',
|
||||
'Use rewireConnection to change connection targets',
|
||||
'Use branch="true"/"false" for IF nodes',
|
||||
'Use case=N for Switch nodes',
|
||||
@@ -367,7 +368,7 @@ n8n_update_partial_workflow({
|
||||
],
|
||||
performance: 'Very fast - typically 50-200ms. Much faster than full updates as only changes are processed.',
|
||||
bestPractices: [
|
||||
'Always include intent parameter - it helps provide better responses',
|
||||
'Always include intent parameter with specific description (e.g., "Add error handling to HTTP Request node", "Fix authentication flow", "Connect Slack notification to errors"). Avoid generic phrases like "update workflow" or "partial update"',
|
||||
'Use rewireConnection instead of remove+add for changing targets',
|
||||
'Use branch="true"/"false" for IF nodes instead of sourceIndex',
|
||||
'Use case=N for Switch nodes instead of sourceIndex',
|
||||
|
||||
Reference in New Issue
Block a user