-
Notifications
You must be signed in to change notification settings - Fork 54.4k
chore(core): Add DTOs for workflow create / update validation #23935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(core): Add DTOs for workflow create / update validation #23935
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
|
E2E Tests: n8n tests passed after 9m 21.7s Run Details
Groups
This message was posted automatically by
currents.dev | Integration Settings
|
bafa852 to
7812c2f
Compare
This comment has been minimized.
This comment has been minimized.
e4f16f2 to
902e434
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/@n8n/api-types/src/dto/workflows/base-workflow.dto.ts">
<violation number="1" location="packages/@n8n/api-types/src/dto/workflows/base-workflow.dto.ts:21">
P2: Validator accepts arrays when it should only accept plain objects. In JavaScript, `typeof [] === 'object'` returns `true`, so arrays will pass this validation despite the message stating 'Connections must be an object'. Add `&& !Array.isArray(val)` to reject arrays.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 9 files
| throw new BadRequestError('Invalid timeSavedMode'); | ||
| } | ||
|
|
||
| // Security: Object.assign is now safe because the DTO validates and filters all input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it's good to remind other readers that Object.assign is only safe here because of the Dto schemas. I would consider shorting it to a single line comment like:
Object.assign is safe because the body provided by the user is validated against the UpdateWorkflowDto
in both methods mainly for readibility.
| expect(createdWorkflow?.activeVersionId).toBeNull(); | ||
| }); | ||
|
|
||
| test('should always create workflow as inactive regardless of active flag', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on these unit tests 👍
| description: 'billing bypass', | ||
| assertionType: 'exact' as const, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice us of parameterized tests 💅
|
Got released with |
Summary
This PR implements new DTOs for workflow creation and update, to make sure that user cannot forge a request containing fields that should be updated only internally (like triggerCount, versionCounter, or shared fields)
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-4382/mass-assignment-in-workflow-creation-allows-tampering-with-internal
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)