Fix Mass Assignment on Save Custom Template#6059
Fix Mass Assignment on Save Custom Template#6059christopherholland-workday wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the saveCustomTemplate functionality in both the controller and service layers to prevent mass assignment vulnerabilities. In the controller, it introduces an explicit allowlist for properties from the request body before passing them to the service. In the service, it replaces Object.assign with explicit assignments for CustomTemplate properties. The review comments suggest improving the maintainability of these explicit assignments by using arrays and loops for the allowed/optional fields.
| const templateBody: Record<string, unknown> = {} | ||
| if (body.name !== undefined) templateBody.name = body.name | ||
| if (body.description !== undefined) templateBody.description = body.description | ||
| if (body.badge !== undefined) templateBody.badge = body.badge | ||
| if (body.usecases !== undefined) templateBody.usecases = body.usecases | ||
| if (body.type !== undefined) templateBody.type = body.type | ||
| if (body.chatflowId !== undefined) templateBody.chatflowId = body.chatflowId | ||
| if (body.tool !== undefined) templateBody.tool = body.tool |
There was a problem hiding this comment.
The explicit allow-listing of properties is great for security. To make this more maintainable and less verbose, you could define an array of allowed keys and loop through them to build the templateBody object. This would make it easier to add or remove allowed properties in the future.
| const templateBody: Record<string, unknown> = {} | |
| if (body.name !== undefined) templateBody.name = body.name | |
| if (body.description !== undefined) templateBody.description = body.description | |
| if (body.badge !== undefined) templateBody.badge = body.badge | |
| if (body.usecases !== undefined) templateBody.usecases = body.usecases | |
| if (body.type !== undefined) templateBody.type = body.type | |
| if (body.chatflowId !== undefined) templateBody.chatflowId = body.chatflowId | |
| if (body.tool !== undefined) templateBody.tool = body.tool | |
| const allowedFields = ['name', 'description', 'badge', 'usecases', 'type', 'chatflowId', 'tool']; | |
| const templateBody: Record<string, unknown> = {}; | |
| for (const field of allowedFields) { | |
| if (body[field] !== undefined) { | |
| templateBody[field] = body[field]; | |
| } | |
| } |
References
- This suggestion improves the maintainability and readability of explicitly mapping allowed properties from a request body, which is a best practice to avoid mass assignment vulnerabilities as per the rule 'Avoid mass assignment from request bodies to entities'.
| if (body.description !== undefined) customTemplate.description = body.description | ||
| if (body.badge !== undefined) customTemplate.badge = body.badge | ||
| if (body.usecases !== undefined) customTemplate.usecases = body.usecases | ||
| if (body.type !== undefined) customTemplate.type = body.type |
There was a problem hiding this comment.
Similar to the controller, you can make this block more concise and maintainable by iterating over a list of optional fields. This approach makes it easier to manage which fields are assignable.
const optionalFields: (keyof CustomTemplate)[] = ['description', 'badge', 'usecases', 'type'];
for (const field of optionalFields) {
if (body[field] !== undefined) {
customTemplate[field] = body[field];
}
}References
- This suggestion improves the maintainability and readability of explicitly mapping allowed properties from a request body, which is a best practice to avoid mass assignment vulnerabilities as per the rule 'Avoid mass assignment from request bodies to entities'.
| let flowDataStr = '' | ||
| let derivedFramework = '' | ||
| const customTemplate = new CustomTemplate() | ||
| Object.assign(customTemplate, body) |
There was a problem hiding this comment.
Is this change still required since we're overwriting body.workspaceId in the controller?
|
Something went wrong and the branches diverged, I have created a new PR addressing the fixes. Moved to #6129 |
Flowise-323 (See comments on CustomTemplate)