mirror of
https://github.com/czlonkowski/n8n-mcp.git
synced 2026-01-30 14:32:04 +00:00
Compare commits
2 Commits
fix/issue-
...
v2.18.4
| Author | SHA1 | Date | |
|---|---|---|---|
|
c52bbcbb83 | ||
|
|
5fb63cd725 |
@@ -1,478 +0,0 @@
|
||||
# DEEP CODE REVIEW: Similar Bugs Analysis
|
||||
## Context: Version Extraction and Validation Issues (v2.17.4)
|
||||
|
||||
**Date**: 2025-10-07
|
||||
**Scope**: Identify similar bugs to the two issues fixed in v2.17.4:
|
||||
1. Version Extraction Bug: Checked non-existent `instance.baseDescription.defaultVersion`
|
||||
2. Validation Bypass Bug: Langchain nodes skipped ALL validation before typeVersion check
|
||||
|
||||
---
|
||||
|
||||
## CRITICAL FINDINGS
|
||||
|
||||
### BUG #1: CRITICAL - Version 0 Incorrectly Rejected in typeVersion Validation
|
||||
**Severity**: CRITICAL
|
||||
**Affects**: AI Agent ecosystem specifically
|
||||
|
||||
**Location**: `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/services/workflow-validator.ts:462`
|
||||
|
||||
**Issue**:
|
||||
```typescript
|
||||
// Line 462 - INCORRECT: Rejects typeVersion = 0
|
||||
else if (typeof node.typeVersion !== 'number' || node.typeVersion < 1) {
|
||||
result.errors.push({
|
||||
type: 'error',
|
||||
nodeId: node.id,
|
||||
nodeName: node.name,
|
||||
message: `Invalid typeVersion: ${node.typeVersion}. Must be a positive number`
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Why This is Critical**:
|
||||
- n8n allows `typeVersion: 0` as a valid version (rare but legal)
|
||||
- The check `node.typeVersion < 1` rejects version 0
|
||||
- This is inconsistent with how we handle version extraction
|
||||
- Could break workflows using nodes with version 0
|
||||
|
||||
**Similar to Fixed Bug**:
|
||||
- Makes incorrect assumptions about version values
|
||||
- Breaks for edge cases (0 is valid, just like checking wrong property paths)
|
||||
- Uses wrong comparison operator (< 1 instead of <= 0 or !== undefined)
|
||||
|
||||
**Test Case**:
|
||||
```typescript
|
||||
const node = {
|
||||
id: 'test',
|
||||
name: 'Test Node',
|
||||
type: 'nodes-base.someNode',
|
||||
typeVersion: 0, // Valid but rejected!
|
||||
parameters: {}
|
||||
};
|
||||
// Current code: ERROR "Invalid typeVersion: 0. Must be a positive number"
|
||||
// Expected: Should be valid
|
||||
```
|
||||
|
||||
**Recommended Fix**:
|
||||
```typescript
|
||||
// Line 462 - CORRECT: Allow version 0
|
||||
else if (typeof node.typeVersion !== 'number' || node.typeVersion < 0) {
|
||||
result.errors.push({
|
||||
type: 'error',
|
||||
nodeId: node.id,
|
||||
nodeName: node.name,
|
||||
message: `Invalid typeVersion: ${node.typeVersion}. Must be a non-negative number (>= 0)`
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
**Verification**: Check if n8n core uses version 0 anywhere:
|
||||
```bash
|
||||
# Need to search n8n source for nodes with version 0
|
||||
grep -r "typeVersion.*:.*0" node_modules/n8n-nodes-base/
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### BUG #2: HIGH - Inconsistent baseDescription Checks in simple-parser.ts
|
||||
**Severity**: HIGH
|
||||
**Affects**: Node loading and parsing
|
||||
|
||||
**Locations**:
|
||||
1. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/simple-parser.ts:195-196`
|
||||
2. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/simple-parser.ts:208-209`
|
||||
|
||||
**Issue #1 - Instance Check**:
|
||||
```typescript
|
||||
// Lines 195-196 - POTENTIALLY WRONG for VersionedNodeType
|
||||
if (instance?.baseDescription?.defaultVersion) {
|
||||
return instance.baseDescription.defaultVersion.toString();
|
||||
}
|
||||
```
|
||||
|
||||
**Issue #2 - Class Check**:
|
||||
```typescript
|
||||
// Lines 208-209 - POTENTIALLY WRONG for VersionedNodeType
|
||||
if (nodeClass.baseDescription?.defaultVersion) {
|
||||
return nodeClass.baseDescription.defaultVersion.toString();
|
||||
}
|
||||
```
|
||||
|
||||
**Why This is Similar**:
|
||||
- **EXACTLY THE SAME BUG** we just fixed in `node-parser.ts`!
|
||||
- VersionedNodeType stores base info in `description`, not `baseDescription`
|
||||
- These checks will FAIL for VersionedNodeType instances
|
||||
- `simple-parser.ts` was not updated when `node-parser.ts` was fixed
|
||||
|
||||
**Evidence from Fixed Code** (node-parser.ts):
|
||||
```typescript
|
||||
// Line 149 comment:
|
||||
// "Critical Fix (v2.17.4): Removed check for non-existent instance.baseDescription.defaultVersion"
|
||||
|
||||
// Line 167 comment:
|
||||
// "VersionedNodeType stores baseDescription as 'description', not 'baseDescription'"
|
||||
```
|
||||
|
||||
**Impact**:
|
||||
- `simple-parser.ts` is used as a fallback parser
|
||||
- Will return incorrect versions for VersionedNodeType nodes
|
||||
- Could cause version mismatches between parsers
|
||||
|
||||
**Recommended Fix**:
|
||||
```typescript
|
||||
// REMOVE Lines 195-196 entirely (non-existent property)
|
||||
// REMOVE Lines 208-209 entirely (non-existent property)
|
||||
|
||||
// Instead, use the correct property path:
|
||||
if (instance?.description?.defaultVersion) {
|
||||
return instance.description.defaultVersion.toString();
|
||||
}
|
||||
|
||||
if (nodeClass.description?.defaultVersion) {
|
||||
return nodeClass.description.defaultVersion.toString();
|
||||
}
|
||||
```
|
||||
|
||||
**Test Case**:
|
||||
```typescript
|
||||
// Test with AI Agent (VersionedNodeType)
|
||||
const AIAgent = require('@n8n/n8n-nodes-langchain').Agent;
|
||||
const instance = new AIAgent();
|
||||
|
||||
// BUG: simple-parser checks instance.baseDescription.defaultVersion (doesn't exist)
|
||||
// CORRECT: Should check instance.description.defaultVersion (exists)
|
||||
console.log('baseDescription exists?', !!instance.baseDescription); // false
|
||||
console.log('description exists?', !!instance.description); // true
|
||||
console.log('description.defaultVersion?', instance.description?.defaultVersion);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### BUG #3: MEDIUM - Inconsistent Math.max Usage Without Validation
|
||||
**Severity**: MEDIUM
|
||||
**Affects**: All versioned nodes
|
||||
|
||||
**Locations**:
|
||||
1. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:19`
|
||||
2. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:75`
|
||||
3. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/property-extractor.ts:181`
|
||||
4. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/node-parser.ts:175`
|
||||
5. `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/parsers/node-parser.ts:202`
|
||||
|
||||
**Issue**:
|
||||
```typescript
|
||||
// property-extractor.ts:19 - NO VALIDATION
|
||||
if (instance?.nodeVersions) {
|
||||
const versions = Object.keys(instance.nodeVersions);
|
||||
const latestVersion = Math.max(...versions.map(Number)); // DANGER!
|
||||
const versionedNode = instance.nodeVersions[latestVersion];
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Why This is Problematic**:
|
||||
1. **No empty array check**: `Math.max()` returns `-Infinity` for empty arrays
|
||||
2. **No NaN check**: Non-numeric keys cause `Math.max(NaN, NaN) = NaN`
|
||||
3. **Ignores defaultVersion**: Should check `defaultVersion` BEFORE falling back to max
|
||||
4. **Inconsistent with fixed code**: node-parser.ts was fixed to prioritize `currentVersion` and `defaultVersion`
|
||||
|
||||
**Edge Cases That Break**:
|
||||
```typescript
|
||||
// Case 1: Empty nodeVersions
|
||||
const nodeVersions = {};
|
||||
const versions = Object.keys(nodeVersions); // []
|
||||
const latestVersion = Math.max(...versions.map(Number)); // -Infinity
|
||||
const versionedNode = nodeVersions[-Infinity]; // undefined
|
||||
|
||||
// Case 2: Non-numeric keys
|
||||
const nodeVersions = { 'v1': {}, 'v2': {} };
|
||||
const versions = Object.keys(nodeVersions); // ['v1', 'v2']
|
||||
const latestVersion = Math.max(...versions.map(Number)); // Math.max(NaN, NaN) = NaN
|
||||
const versionedNode = nodeVersions[NaN]; // undefined
|
||||
```
|
||||
|
||||
**Similar to Fixed Bug**:
|
||||
- Assumes data structure without validation
|
||||
- Could return undefined and cause downstream errors
|
||||
- Doesn't follow the correct priority: `currentVersion` > `defaultVersion` > `max(nodeVersions)`
|
||||
|
||||
**Recommended Fix**:
|
||||
```typescript
|
||||
// property-extractor.ts - Consistent with node-parser.ts fix
|
||||
if (instance?.nodeVersions) {
|
||||
// PRIORITY 1: Check currentVersion (already computed by VersionedNodeType)
|
||||
if (instance.currentVersion !== undefined) {
|
||||
const versionedNode = instance.nodeVersions[instance.currentVersion];
|
||||
if (versionedNode?.description?.properties) {
|
||||
return this.normalizeProperties(versionedNode.description.properties);
|
||||
}
|
||||
}
|
||||
|
||||
// PRIORITY 2: Check defaultVersion
|
||||
if (instance.description?.defaultVersion !== undefined) {
|
||||
const versionedNode = instance.nodeVersions[instance.description.defaultVersion];
|
||||
if (versionedNode?.description?.properties) {
|
||||
return this.normalizeProperties(versionedNode.description.properties);
|
||||
}
|
||||
}
|
||||
|
||||
// PRIORITY 3: Fallback to max with validation
|
||||
const versions = Object.keys(instance.nodeVersions);
|
||||
if (versions.length > 0) {
|
||||
const numericVersions = versions.map(Number).filter(v => !isNaN(v));
|
||||
if (numericVersions.length > 0) {
|
||||
const latestVersion = Math.max(...numericVersions);
|
||||
const versionedNode = instance.nodeVersions[latestVersion];
|
||||
if (versionedNode?.description?.properties) {
|
||||
return this.normalizeProperties(versionedNode.description.properties);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Applies to 5 locations** - all need same fix pattern.
|
||||
|
||||
---
|
||||
|
||||
### BUG #4: MEDIUM - Expression Validation Skip for Langchain Nodes (Line 972)
|
||||
**Severity**: MEDIUM
|
||||
**Affects**: AI Agent ecosystem
|
||||
|
||||
**Location**: `/Users/romualdczlonkowski/Pliki/n8n-mcp/n8n-mcp/src/services/workflow-validator.ts:972`
|
||||
|
||||
**Issue**:
|
||||
```typescript
|
||||
// Line 969-974 - Another early skip for langchain
|
||||
// Skip expression validation for langchain nodes
|
||||
// They have AI-specific validators and different expression rules
|
||||
const normalizedType = NodeTypeNormalizer.normalizeToFullForm(node.type);
|
||||
if (normalizedType.startsWith('nodes-langchain.')) {
|
||||
continue; // Skip ALL expression validation
|
||||
}
|
||||
```
|
||||
|
||||
**Why This Could Be Problematic**:
|
||||
- Similar to the bug we fixed where langchain nodes skipped typeVersion validation
|
||||
- Langchain nodes CAN use expressions (especially in AI Agent system prompts, tool configurations)
|
||||
- Skipping ALL expression validation means we won't catch:
|
||||
- Syntax errors in expressions
|
||||
- Invalid node references
|
||||
- Missing input data references
|
||||
|
||||
**Similar to Fixed Bug**:
|
||||
- Early return/continue before running validation
|
||||
- Assumes langchain nodes don't need a certain type of validation
|
||||
- We already fixed this pattern once for typeVersion - might need fixing here too
|
||||
|
||||
**Investigation Required**:
|
||||
Need to determine if langchain nodes:
|
||||
1. Use n8n expressions in their parameters? (YES - AI Agent uses expressions)
|
||||
2. Need different expression validation rules? (MAYBE)
|
||||
3. Should have AI-specific expression validation? (PROBABLY YES)
|
||||
|
||||
**Recommended Action**:
|
||||
1. **Short-term**: Add comment explaining WHY we skip (currently missing)
|
||||
2. **Medium-term**: Implement langchain-specific expression validation
|
||||
3. **Long-term**: Never skip validation entirely - always have appropriate validation
|
||||
|
||||
**Example of Langchain Expressions**:
|
||||
```typescript
|
||||
// AI Agent system prompt can contain expressions
|
||||
{
|
||||
type: '@n8n/n8n-nodes-langchain.agent',
|
||||
parameters: {
|
||||
text: 'You are an assistant. User input: {{ $json.userMessage }}' // Expression!
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### BUG #5: LOW - Inconsistent Version Property Access Patterns
|
||||
**Severity**: LOW
|
||||
**Affects**: Code maintainability
|
||||
|
||||
**Locations**: Multiple files use different patterns
|
||||
|
||||
**Issue**: Three different patterns for accessing version:
|
||||
```typescript
|
||||
// Pattern 1: Direct access with fallback (SAFE)
|
||||
const version = nodeInfo.version || 1;
|
||||
|
||||
// Pattern 2: Direct access without fallback (UNSAFE)
|
||||
if (nodeInfo.version && node.typeVersion < nodeInfo.version) { ... }
|
||||
|
||||
// Pattern 3: Falsy check (BREAKS for version 0)
|
||||
if (nodeInfo.version) { ... } // Fails if version = 0
|
||||
```
|
||||
|
||||
**Why This Matters**:
|
||||
- Pattern 3 breaks for `version = 0` (falsy but valid)
|
||||
- Inconsistency makes code harder to maintain
|
||||
- Similar issue to version < 1 check
|
||||
|
||||
**Examples**:
|
||||
```typescript
|
||||
// workflow-validator.ts:471 - UNSAFE for version 0
|
||||
else if (nodeInfo.version && node.typeVersion < nodeInfo.version) {
|
||||
// If nodeInfo.version = 0, this never executes (falsy check)
|
||||
}
|
||||
|
||||
// workflow-validator.ts:480 - UNSAFE for version 0
|
||||
else if (nodeInfo.version && node.typeVersion > nodeInfo.version) {
|
||||
// If nodeInfo.version = 0, this never executes (falsy check)
|
||||
}
|
||||
```
|
||||
|
||||
**Recommended Fix**:
|
||||
```typescript
|
||||
// Use !== undefined for version checks
|
||||
else if (nodeInfo.version !== undefined && node.typeVersion < nodeInfo.version) {
|
||||
// Now works correctly for version 0
|
||||
}
|
||||
|
||||
else if (nodeInfo.version !== undefined && node.typeVersion > nodeInfo.version) {
|
||||
// Now works correctly for version 0
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### BUG #6: LOW - Missing Type Safety for VersionedNodeType Properties
|
||||
**Severity**: LOW
|
||||
**Affects**: TypeScript type safety
|
||||
|
||||
**Issue**: No TypeScript interface for VersionedNodeType properties
|
||||
|
||||
**Current Code**:
|
||||
```typescript
|
||||
// We access these properties everywhere but no type definition:
|
||||
instance.currentVersion // any
|
||||
instance.description // any
|
||||
instance.nodeVersions // any
|
||||
instance.baseDescription // any (doesn't exist but not caught!)
|
||||
```
|
||||
|
||||
**Why This Matters**:
|
||||
- TypeScript COULD HAVE caught the `baseDescription` bug
|
||||
- Using `any` everywhere defeats type safety
|
||||
- Makes refactoring dangerous
|
||||
|
||||
**Recommended Fix**:
|
||||
```typescript
|
||||
// Create types/versioned-node.ts
|
||||
export interface VersionedNodeTypeInstance {
|
||||
currentVersion: number;
|
||||
description: {
|
||||
name: string;
|
||||
displayName: string;
|
||||
defaultVersion?: number;
|
||||
version?: number | number[];
|
||||
properties?: any[];
|
||||
// ... other properties
|
||||
};
|
||||
nodeVersions: {
|
||||
[version: number]: {
|
||||
description: {
|
||||
properties?: any[];
|
||||
// ... other properties
|
||||
};
|
||||
};
|
||||
};
|
||||
}
|
||||
|
||||
// Then use in code:
|
||||
const instance = new nodeClass() as VersionedNodeTypeInstance;
|
||||
instance.baseDescription // TypeScript error: Property 'baseDescription' does not exist
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## SUMMARY OF FINDINGS
|
||||
|
||||
### By Severity:
|
||||
|
||||
**CRITICAL (1 bug)**:
|
||||
1. Version 0 incorrectly rejected (workflow-validator.ts:462)
|
||||
|
||||
**HIGH (1 bug)**:
|
||||
2. Inconsistent baseDescription checks in simple-parser.ts (EXACT DUPLICATE of fixed bug)
|
||||
|
||||
**MEDIUM (2 bugs)**:
|
||||
3. Unsafe Math.max usage in property-extractor.ts (5 locations)
|
||||
4. Expression validation skip for langchain nodes (workflow-validator.ts:972)
|
||||
|
||||
**LOW (2 issues)**:
|
||||
5. Inconsistent version property access patterns
|
||||
6. Missing TypeScript types for VersionedNodeType
|
||||
|
||||
### By Category:
|
||||
|
||||
**Property Name Assumptions** (Similar to Bug #1):
|
||||
- BUG #2: baseDescription checks in simple-parser.ts
|
||||
|
||||
**Validation Order Issues** (Similar to Bug #2):
|
||||
- BUG #4: Expression validation skip for langchain nodes
|
||||
|
||||
**Version Logic Issues**:
|
||||
- BUG #1: Version 0 rejected incorrectly
|
||||
- BUG #3: Math.max without validation
|
||||
- BUG #5: Inconsistent version checks
|
||||
|
||||
**Type Safety Issues**:
|
||||
- BUG #6: Missing VersionedNodeType types
|
||||
|
||||
### Affects AI Agent Ecosystem:
|
||||
- BUG #1: Critical - blocks valid typeVersion values
|
||||
- BUG #2: High - affects AI Agent version extraction
|
||||
- BUG #4: Medium - skips expression validation
|
||||
- All others: Indirectly affect stability
|
||||
|
||||
---
|
||||
|
||||
## RECOMMENDED ACTIONS
|
||||
|
||||
### Immediate (Critical):
|
||||
1. Fix version 0 rejection in workflow-validator.ts:462
|
||||
2. Fix baseDescription checks in simple-parser.ts
|
||||
|
||||
### Short-term (High Priority):
|
||||
3. Add validation to all Math.max usages in property-extractor.ts
|
||||
4. Investigate and document expression validation skip for langchain
|
||||
|
||||
### Medium-term:
|
||||
5. Standardize version property access patterns
|
||||
6. Add TypeScript types for VersionedNodeType
|
||||
|
||||
### Testing:
|
||||
7. Add test cases for version 0
|
||||
8. Add test cases for empty nodeVersions
|
||||
9. Add test cases for langchain expression validation
|
||||
|
||||
---
|
||||
|
||||
## VERIFICATION CHECKLIST
|
||||
|
||||
For each bug found:
|
||||
- [x] File and line number identified
|
||||
- [x] Code snippet showing issue
|
||||
- [x] Why it's similar to fixed bugs
|
||||
- [x] Severity assessment
|
||||
- [x] Test case provided
|
||||
- [x] Fix recommended with code
|
||||
- [x] Impact on AI Agent ecosystem assessed
|
||||
|
||||
---
|
||||
|
||||
## NOTES
|
||||
|
||||
1. **Pattern Recognition**: The baseDescription bug in simple-parser.ts is EXACTLY the same bug we just fixed in node-parser.ts, suggesting these files should be refactored to share version extraction logic.
|
||||
|
||||
2. **Validation Philosophy**: We're seeing a pattern of skipping validation for langchain nodes. This was correct for PARAMETER validation but WRONG for typeVersion. Need to review each skip carefully.
|
||||
|
||||
3. **Version 0 Edge Case**: If n8n doesn't use version 0 in practice, the critical bug might be theoretical. However, rejecting valid values is still a bug.
|
||||
|
||||
4. **Math.max Safety**: The Math.max pattern is used 5+ times. Should extract to a utility function with proper validation.
|
||||
|
||||
5. **Type Safety**: Adding proper TypeScript types would have prevented the baseDescription bug entirely. Strong recommendation for future work.
|
||||
BIN
data/nodes.db
BIN
data/nodes.db
Binary file not shown.
Reference in New Issue
Block a user