Ps 513 hotfix #49
Ps 513 hotfix #49
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AUTH_SECRET: process.env.AUTH_SECRET || 'mysecret', | ||
| VALID_ISSUERS: process.env.VALID_ISSUERS || '["https://api.topcoder-dev.com", "https://api.topcoder.com", "https://topcoder-dev.auth0.com/", "https://auth.topcoder-dev.com/"]', | ||
| IDENTITY_DB_URL: process.env.IDENTITY_DB_URL, | ||
| VANILLA_DB_URL: process.env.VANILLA_DB_URL, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that VANILLA_DB_URL is properly validated and sanitized before use to prevent potential security vulnerabilities such as SQL injection.
| M2M_UPDATE_ACCESS_TOKEN: process.env.M2M_UPDATE_ACCESS_TOKEN || 'eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJodHRwczovL3RvcGNvZGVyLWRldi5hdXRoMC5jb20vIiwic3ViIjoiZW5qdzE4MTBlRHozWFR3U08yUm4yWTljUVRyc3BuM0JAY2xpZW50cyIsImF1ZCI6Imh0dHBzOi8vbTJtLnRvcGNvZGVyLWRldi5jb20vIiwiaWF0IjoxNTUwOTA2Mzg4LCJleHAiOjE2ODA5OTI3ODgsImF6cCI6ImVuancxODEwZUR6M1hUd1NPMlJuMlk5Y1FUcnNwbjNCIiwic2NvcGUiOiJ1cGRhdGU6bWVtYmVycyIsImd0eSI6ImNsaWVudC1jcmVkZW50aWFscyJ9.wImcvhkF9QPOCSEfZ01U-YxYM8NZi1yqgRmw3eiNn1Q', | ||
| S3_ENDPOINT: process.env.S3_ENDPOINT || 'localhost:9000' | ||
| S3_ENDPOINT: process.env.S3_ENDPOINT || 'localhost:9000', | ||
| VANILLA_DB_URL: process.env.VANILLA_DB_URL |
There was a problem hiding this comment.
[correctness]
The VANILLA_DB_URL is being added without a fallback value. Ensure that this environment variable is always set in the test environment to avoid potential runtime errors.
| Update the member handle. | ||
|
|
||
| Authorization: | ||
| - JWT roles: Only the profile owner or users with `administrator`/`admin` roles may update member data. |
There was a problem hiding this comment.
[security]
The authorization description mentions administrator and admin roles separately. Ensure that these roles are distinct and correctly implemented in the authorization logic, as redundancy or misconfiguration could lead to security issues.
| - in: path | ||
| name: handle | ||
| required: true | ||
| type: string |
There was a problem hiding this comment.
[correctness]
Consider specifying a pattern or length constraint for the handle parameter to prevent potential issues with unexpected input formats.
| name: body | ||
| required: true | ||
| schema: | ||
| $ref: '#/definitions/MemberHandleUpdate' |
There was a problem hiding this comment.
[correctness]
The MemberHandleUpdate schema should include validation constraints for newHandle, such as length and allowed characters, to ensure data integrity and prevent potential issues with invalid handle formats.
| let vanillaPool | ||
|
|
||
| function getVanillaPool () { | ||
| if (!config.VANILLA_DB_URL) { |
There was a problem hiding this comment.
[correctness]
Using config.VANILLA_DB_URL directly without validation could lead to runtime errors if the URL is malformed. Consider validating the URL format before creating the pool.
| } | ||
|
|
||
| if (!vanillaPool) { | ||
| vanillaPool = mysql.createPool(config.VANILLA_DB_URL) |
There was a problem hiding this comment.
[performance]
The createPool method should be configured with additional options such as connection limits, timeouts, etc., to ensure optimal performance and resource management.
| * @param {Object} res the response | ||
| */ | ||
| async function updateHandle (req, res) { | ||
| const result = await service.updateHandle(req.authUser, req.params.handle, req.query, req.body) |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the service.updateHandle call to ensure that any exceptions are properly managed and do not result in unhandled promise rejections.
| method: 'updateHandle', | ||
| auth: 'jwt', | ||
| access: constants.ADMIN_ROLES, | ||
| scopes: [MEMBERS.UPDATE, MEMBERS.ALL] |
There was a problem hiding this comment.
[security]
The access property is set to constants.ADMIN_ROLES, which is consistent with other routes requiring admin access. However, ensure that constants.ADMIN_ROLES is correctly defined and includes all necessary roles for this operation.
| const existingMember = await prisma.member.findUnique({ | ||
| where: { handleLower: newHandleLower } | ||
| }) | ||
| if (existingMember && String(existingMember.userId) !== String(member.userId)) { |
There was a problem hiding this comment.
[correctness]
The check String(existingMember.userId) !== String(member.userId) could lead to unexpected behavior if userId is not always a string. Consider ensuring userId is consistently typed or using a more robust comparison method.
| const existingIdentity = await identityPrisma.user.findFirst({ | ||
| where: { handle_lower: newHandleLower } | ||
| }) | ||
| if (existingIdentity && Number(existingIdentity.user_id) !== identityUserId) { |
There was a problem hiding this comment.
[correctness]
The check Number(existingIdentity.user_id) !== identityUserId could lead to unexpected behavior if user_id is not always a number. Consider ensuring user_id is consistently typed or using a more robust comparison method.
| } catch (err) { | ||
| if (vanillaUpdated) { | ||
| try { | ||
| await updateVanillaHandle(newHandle, member.handle, vanillaPool) |
There was a problem hiding this comment.
[maintainability]
The rollback logic for updateVanillaHandle does not handle errors that might occur during the rollback itself, which could leave the system in an inconsistent state. Consider adding a mechanism to handle or log rollback errors more gracefully.
| } | ||
| if (memberUpdated) { | ||
| try { | ||
| await prisma.member.update({ |
There was a problem hiding this comment.
[maintainability]
The rollback logic for prisma.member.update does not handle errors that might occur during the rollback itself, which could leave the system in an inconsistent state. Consider adding a mechanism to handle or log rollback errors more gracefully.
| } | ||
| if (identityUpdated) { | ||
| try { | ||
| await updateIdentityHandle(identityUserId, newHandle, member.handle, new Date()) |
There was a problem hiding this comment.
[maintainability]
The rollback logic for updateIdentityHandle does not handle errors that might occur during the rollback itself, which could leave the system in an inconsistent state. Consider adding a mechanism to handle or log rollback errors more gracefully.
| throw err | ||
| } | ||
|
|
||
| if (userResult === 0 || securityUserResult === 0) { |
There was a problem hiding this comment.
[💡 readability]
The error message Identity records not updated for user ${userId} is generic and may not provide enough context for debugging. Consider including more details about which specific update failed.
https://topcoder.atlassian.net/browse/PS-513