Fix improper mass assignment in account registration#5689
Fix improper mass assignment in account registration#5689christopherholland-workday wants to merge 4 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a mass assignment vulnerability in the account registration flow by introducing an allow-list of fields in account.service.ts. This is a great security improvement.
However, I've found a critical issue in the changes made to user.service.ts. The refactoring of createNewUser to also perform sanitization breaks the existing user invitation functionality. Since the registration flow is already secured by the changes in account.service.ts, I recommend reverting the changes in user.service.ts.
Additionally, I've left a comment in account.service.ts with a suggestion to improve type safety by removing as any casts.
| if (data.user && typeof data.user === 'object' && !Array.isArray(data.user)) { | ||
| for (const field of allowedUserFields) { | ||
| if (data.user[field] !== undefined) { | ||
| sanitized.user[field] = data.user[field] as any | ||
| } | ||
| } | ||
| // Referral is used for Stripe referral tracking in CLOUD; not a User entity column. | ||
| if ('referral' in data.user && data.user.referral !== undefined) { | ||
| ;(sanitized.user as any).referral = data.user.referral | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of as any in this block undermines TypeScript's type safety and should be avoided.
- On line 89, the cast is likely unnecessary. If
data.userandsanitized.userare both typed asPartial<User>, a direct assignmentsanitized.user[field] = data.user[field]should be valid. - On line 94, casting to
anyto add thereferralproperty is a workaround. A better, more type-safe solution would be to extend the type of theuserobject in theAccountDTOto include this optional property, e.g.,user: Partial<User> & { referral?: string }.
Refactoring to remove these as any casts would improve code quality and maintainability.
1 participant
Fix improper mass assignment in account registration by creating an allow-list of fields that can be copied over, while leaving the server-side generated fields to be generated server-side.