[Cloned] feat(Todoist Node): Add more resources and operations #3
Conversation
Original PR: n8n-io/n8n#17925 Author: mutdmour ## Summary Add more resources and operations to Todoist node https://github.com/user-attachments/assets/4bbee79f-ea0e-4c20-9e34-e00add862cd9 <!-- Describe what the PR does and how to test. Photos and videos are recommended. --> ## Related Linear tickets, Github issues, and Community forum posts <!-- Include links to **Linear ticket** or Github issue or Community forum post. Important in order to close *automatically* and provide context to reviewers. https://linear.app/n8n/issue/ --> <!-- Use "closes #<issue-number>", "fixes #<issue-number>", or "resolves #<issue-number>" to automatically close issues when the PR is merged. --> ## Review / Merge checklist - [ ] PR title and summary are descriptive. ([conventions](../blob/master/.github/pull_request_title_conventions.md)) <!-- **Remember, the title automatically goes into the changelog. Use `(no-changelog)` otherwise.** --> - [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up ticket created. - [ ] Tests included. <!-- A bug is not considered fixed, unless a test is added to prevent it from happening again. A feature is not complete without tests. --> - [ ] PR Labeled with `release/backport` (if the PR is an urgent fix that needs to be backported) --- This is a cloned PR for AI code review benchmarking.
🤖 Refacto.AI: PR Summary & Reviewer Guide🎯 1. The Goal (Why was this PR raised?)This PR aims to enhance the Todoist Node by adding more resources and operations to the existing functionality. The implementation expands the node's capabilities beyond basic task management to include projects, sections, comments, and labels. 🗺️ 2. The Changes (What was implemented?)This section provides a walkthrough of the changes, connecting them to the PR's goals.
🤔 3. Key Areas for Human ReviewHere are the most important areas to focus your review on, including specific testing instructions. Area of Concern: Type Validation Implementation
Area of Concern: Service Class Expansion
Area of Concern: Node Definition Updates
Area of Concern: Operation Handler Implementation
|
|
/do-refacto Please review this code for potential issues, security vulnerabilities, and improvements. Review triggered at: 2025-08-05T10:44:23.121Z |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Strong Type Safety Foundation - Security & Reliability Improvements Needed
|
![refacto-bot[bot]](https://avatars.githubusercontent.com/in/1142269?s=60&v=4){kind=small}
| | 'string[]' | ||
| | 'number[]' | ||
| | 'boolean[]' | ||
| | 'object'; | ||
|
|
||
| function assertIsValidObject(value: unknown): asserts value is Record<string, unknown> { |
There was a problem hiding this comment.
Type Assertion Bypass in assertIsValidObject Function
The assertIsValidObject function only checks that the value is an object and not null, but doesn't verify that it's actually a Record<string, unknown>. This allows arrays and other object types to pass validation. An attacker could exploit this by passing an array or a specially crafted object that would bypass validation but cause unexpected behavior or potentially code execution when used as a Record<string, unknown> elsewhere in the code.
function assertIsValidObject(value: unknown): asserts value is Record<string, unknown> {
assert(typeof value === 'object' && value !== null && !Array.isArray(value) && Object.getPrototypeOf(value) === Object.prototype, 'Value is not a valid plain object');References
Standard: CWE-20: Improper Input Validation
Standard: OWASP Top 10 2021: A03 - Injection
| parameterName: string, | ||
| value: unknown, | ||
| type: ParameterType, | ||
| ): boolean { | ||
| try { | ||
| if (type === 'resource-locator') { | ||
| assertIsResourceLocator(parameterName, value); | ||
| } else if (type === 'object') { | ||
| assertIsObjectType(parameterName, value); | ||
| } else if (type.endsWith('[]')) { | ||
| assertIsArrayType(parameterName, value, type); | ||
| } else { | ||
| assertIsPrimitiveType(parameterName, value, type); | ||
| } | ||
| return true; | ||
| } catch { | ||
| return false; |
There was a problem hiding this comment.
Unsafe Type Assertion in validateParameterType Function
The validateParameterType function silently catches all exceptions and returns false, potentially hiding critical errors. This could allow malicious input to bypass validation if an unexpected error occurs during validation. Additionally, the function doesn't properly validate complex types, relying on separate functions that might have their own vulnerabilities.
} catch (error) {
// Only catch assertion errors, not other unexpected errors
if (error instanceof Error && error.name === 'AssertionError') {
return false;
}
throw error; // Re-throw unexpected errors
}References
Standard: CWE-703: Improper Check or Handling of Exceptional Conditions
Standard: OWASP Top 10 2021: A03 - Injection
| | 'number[]' | ||
| | 'boolean[]' | ||
| | 'object'; | ||
|
|
||
| function assertIsValidObject(value: unknown): asserts value is Record<string, unknown> { | ||
| assert(typeof value === 'object' && value !== null, 'Value is not a valid object'); | ||
| } | ||
|
|
||
| function assertIsRequiredParameter( | ||
| parameterName: string, |
There was a problem hiding this comment.
Inconsistent Error Handling in Type Validation
The assertIsRequiredParameter function only checks for undefined values but not for null values. This inconsistency with assertIsValidObject (which checks for both null and undefined) creates a reliability issue where null values might pass the required parameter check but fail later in type-specific validations. This can lead to confusing error messages and potential runtime failures when null values are processed as if they were valid.
function assertIsRequiredParameter(
parameterName: string,
value: unknown,
isOptional: boolean,
): void {
if (!isOptional && (value === undefined || value === null)) {
assert(
false,
`Required parameter "${parameterName}" is ${value === null ? 'null' : 'missing'}`
);
}
}References
Standard: ISO/IEC 25010 Functional Correctness - Precondition Validation
| const service = new TodoistService(); | ||
| let responseData; | ||
| const resource = this.getNodeParameter('resource', 0); | ||
| const operation = this.getNodeParameter('operation', 0) as OperationType; | ||
| const operation = this.getNodeParameter('operation', 0) as TaskOperationType; | ||
| for (let i = 0; i < length; i++) { | ||
| try { | ||
| if (resource === 'task') { | ||
| responseData = await service.execute(this, operation, i); | ||
| responseData = await service.executeTask(this, operation, i); | ||
| } else if (resource === 'project') { | ||
| if (!isProjectOperationType(operation)) { | ||
| throw new NodeOperationError( | ||
| this.getNode(), | ||
| `Invalid operation '${operation}' for project resource`, | ||
| ); | ||
| } | ||
| responseData = await service.executeProject(this, operation, i); | ||
| } else if (resource === 'section') { | ||
| if (!isSectionOperationType(operation)) { | ||
| throw new NodeOperationError( | ||
| this.getNode(), | ||
| `Invalid operation '${operation}' for section resource`, | ||
| ); |
There was a problem hiding this comment.
Unhandled Promise Rejection in TodoistV2.execute
The execute method doesn't handle potential errors from the service execution methods for project, section, comment, and label resources. While task operations have error handling, the other resource operations lack try-catch blocks, which could lead to unhandled promise rejections if an error occurs. This violates ISO/IEC 25010 Reliability requirements for fault tolerance and could cause the workflow to terminate unexpectedly.
try {
if (resource === 'task') {
responseData = await service.executeTask(this, operation, i);
} else if (resource === 'project') {
if (!isProjectOperationType(operation)) {
throw new NodeOperationError(
this.getNode(),
`Invalid operation '${operation}' for project resource`,
);
}
responseData = await service.executeProject(this, operation, i);
} else if (resource === 'section') {
if (!isSectionOperationType(operation)) {
throw new NodeOperationError(
this.getNode(),
`Invalid operation '${operation}' for section resource`,
);
}
responseData = await service.executeSection(this, operation, i);
} else if (resource === 'comment') {
if (!isCommentOperationType(operation)) {
throw new NodeOperationError(
this.getNode(),
`Invalid operation '${operation}' for comment resource`,
);
}
responseData = await service.executeComment(this, operation, i);
} else if (resource === 'label') {
if (!isLabelOperationType(operation)) {
throw new NodeOperationError(
this.getNode(),
`Invalid operation '${operation}' for label resource`,
);
}
responseData = await service.executeLabel(this, operation, i);
} else {
throw new NodeOperationError(this.getNode(), `Unsupported resource: ${resource}`);
}
} catch (error) {
if (error instanceof NodeOperationError) {
throw error;
}
throw new NodeOperationError(this.getNode(), `Error executing ${resource} ${operation}: ${error.message}`, {
itemIndex: i,
});
}References
Standard: ISO/IEC 25010 Reliability - Fault Tolerance
| itemIndex: number, | ||
| ): Promise<TodoistResponse> { | ||
| return await this.sectionHandlers[operation].handleOperation(ctx, itemIndex); | ||
| } | ||
|
|
||
| async executeComment( | ||
| ctx: Context, | ||
| operation: CommentOperationType, | ||
| itemIndex: number, | ||
| ): Promise<TodoistResponse> { | ||
| return await this.commentHandlers[operation].handleOperation(ctx, itemIndex); | ||
| } | ||
|
|
||
| async executeLabel( | ||
| ctx: Context, |
There was a problem hiding this comment.
Incorrect Type Guards in Resource Operation Functions
All type guard functions (isProjectOperationType, isSectionOperationType, isCommentOperationType, and isLabelOperationType) have a logical flaw in their implementation. They attempt to check if an operation string is valid, but first cast the string to the target type before checking if it's included in the array. This defeats the purpose of the type guard, as it will accept any string that's not in the array but cast it to the type anyway, which could lead to runtime errors when using operations that passed the guard but aren't actually valid.
export function isProjectOperationType(operation: string): operation is ProjectOperationType {
return (PROJECT_OPERATIONS as readonly string[]).includes(operation);
}
export function isSectionOperationType(operation: string): operation is SectionOperationType {
return (SECTION_OPERATIONS as readonly string[]).includes(operation);
}
export function isCommentOperationType(operation: string): operation is CommentOperationType {
return (COMMENT_OPERATIONS as readonly string[]).includes(operation);
}
export function isLabelOperationType(operation: string): operation is LabelOperationType {
return (LABEL_OPERATIONS as readonly string[]).includes(operation);References
Standard: TypeScript Type Safety - Type Guards and Type Narrowing
2 participants
Cloned Pull Request
This PR is a clone of the original PR: n8n-io/n8n#17925
Original Author: mutdmour
Original PR Number: #17925
Original Repository: n8n-io/n8n
Original Description:
Summary
Add more resources and operations to Todoist node
Screen.Recording.2025-08-03.at.15.40.57.mov
Related Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)Files Changed:
packages/nodes-base/nodes/Todoist/GenericFunctions.tspackages/nodes-base/nodes/Todoist/v2/OperationHandler.tspackages/nodes-base/nodes/Todoist/v2/Service.tspackages/nodes-base/nodes/Todoist/v2/TodoistV2.node.tspackages/nodes-base/nodes/Todoist/v2/test/OperationHandler.test.tspackages/nodes-base/nodes/Todoist/v2/test/TodoistV2.node.test.tspackages/nodes-base/nodes/Todoist/v2/test/workflow.jsonpackages/nodes-base/utils/__tests__/types.test.tspackages/nodes-base/utils/types.tsThis PR was automatically cloned for AI code review benchmarking purposes.