Issue #000 fix: creating usres in bulk - TypeError: Cannot read prope…#868
Issue #000 fix: creating usres in bulk - TypeError: Cannot read prope…#868vaivk369 wants to merge 29 commits intoELEVATE-Project:developfrom
Conversation
…rties of undefined (reading 'outputFilePath')
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded tenant-aware account creation with optional post-registration login/session/token generation and Redis persistence; enhanced user listing/search with meta/status filters and per-user metadata processing; made generics/utils and invite processing more defensive and dynamic; added two GitHub Actions workflows for build & deploy. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TenantDB as TenantDB
participant AccountSvc as AccountService
participant Auth as TokenService
participant Redis as RedisStore
Client->>AccountSvc: POST /tenant/accountCreate(body, deviceInfo, domain?, registerWithLogin?, req)
alt domain provided
AccountSvc->>TenantDB: resolve tenant by domain
TenantDB-->>AccountSvc: tenantDetail / not found
else domain absent
AccountSvc->>TenantDB: resolve tenant by tenant-code header
TenantDB-->>AccountSvc: tenantDetail / not found
end
AccountSvc->>AccountSvc: validate input, determine roles
AccountSvc->>AccountSvc: create user record
alt registerWithLogin == true
AccountSvc->>Auth: generate access & refresh tokens
Auth-->>AccountSvc: tokens
AccountSvc->>Redis: persist session + refresh token
Redis-->>AccountSvc: ack
AccountSvc-->>Client: return user + tokens
else
AccountSvc-->>Client: return created user (no tokens)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/generics/utils.js (2)
1022-1059: Note: Discrepancy between PR title and code change.The PR title mentions "reading 'outputFilePath'" but the actual code change addresses accessing the
data_typeproperty on a potentially undefinedfindEntity. While the fix correctly prevents undefined property access errors, please verify that this change addresses the specific error mentioned in the PR title, or update the PR description to accurately reflect the fix.
1037-1055: Address fallback behavior when entity type is missing.When
findEntityis undefined (no matching entity inprunedEntities), the code falls through to the else block (lines 1049–1055). This processes metadata without entity type information and returns incomplete results like{name: undefined, id: undefined, externalId: undefined}if the corresponding data isn't found infeederData. The returned data is merged without validation into event bodies and change tracking structures across multiple call sites (account.js, user.js, userInvite.js), which could cause downstream issues. Either add validation to reject incomplete metadata or document why partial results are acceptable.
🧹 Nitpick comments (1)
src/generics/utils.js (1)
1041-1041: Defensive null-check correctly prevents the runtime error.The added check for
findEntityexistence before accessing itsdata_typeproperty prevents the "Cannot read properties of undefined" error when no matching entity is found in theprunedEntitiesarray.Minor note: The optional chaining
findEntity?.data_typeis redundant after the explicitfindEntity &&check, though it's harmless. Consider simplifying to:🔎 Suggested simplification
-if (findEntity && (findEntity?.data_type == 'ARRAY' || findEntity?.data_type == 'ARRAY[STRING]')) { +if (findEntity && (findEntity.data_type == 'ARRAY' || findEntity.data_type == 'ARRAY[STRING]')) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/api-doc/bulkUser.mdis excluded by!src/api-doc/**,!**/*.md
📒 Files selected for processing (1)
src/generics/utils.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/generics/utils.js (1)
src/helpers/userInvite.js (1)
findEntity(579-579)
…crypting phone and passing correct meta in account search
Issue #000 fix: added new endpoint to create account in teanat and decrypting phone and passing correct meta in account search
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/account.js (1)
322-334: Type safety issue withreq.body.roles.If
req.body.rolesis truthy but not a string (e.g., already an array), calling.split(',')will throw a TypeError. Consider adding type validation.Proposed fix
role = await roleQueries.findAll( { title: { - [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','), + [Op.in]: req.body.roles + ? (Array.isArray(req.body.roles) ? req.body.roles : req.body.roles.split(',')) + : process.env.DEFAULT_ROLE.split(','), }, tenant_code: tenantDetail.code, },
🤖 Fix all issues with AI agents
In `@src/controllers/v1/tenant.js`:
- Around line 162-168: The JSDoc for the "Create account user" endpoint has an
incorrect `@name` value; change the `@name` annotation from "bulkUserCreate" to
"accountCreate" in the JSDoc block above the accountCreate handler so the docs
match the actual method/intent (look for the comment block containing "Create
account user" and update its `@name`).
In `@src/services/account.js`:
- Around line 1946-1969: The code reads user.user_organizations[0] without
checking existence which can throw if a user has no organizations; update the
loop in the section using user_organizations to first verify
user.user_organizations && user.user_organizations.length > 0, and if absent
either skip processing that user (continue) or fall back to
defaultOrganizationCode for userOrg and handle the missing id when calling
removeDefaultOrgEntityTypes (e.g., pass null/undefined or guard that call).
Ensure the branches adjust the values passed to findUserEntityTypesAndEntities
and removeDefaultOrgEntityTypes and still call utils.processDbResponse only when
safe.
- Around line 384-385: Remove the two debug console.log statements that print
validationData and userModel; either delete them entirely or replace them with
structured logging using the project's logger (e.g., logger.debug or
processLogger.debug) and ensure sensitive fields from validationData/userModel
are omitted or redacted before logging; update occurrences of
console.log('validationData', validationData) and console.log('userModel',
userModel) in src/services/account.js accordingly.
- Line 42: Remove the unused import "use" from i18next in this module: delete
the line that destructures use from the require call (the symbol "use" in the
const { use } = require('i18next') statement) so the file no longer imports an
unused binding; if the require call is otherwise unnecessary, replace or
simplify it accordingly (e.g., remove the entire require if nothing else from
i18next is used).
- Around line 70-84: tenantDomain can be undefined if neither domain nor
req.headers[common.TENANT_CODE_HEADER] are present, causing a TypeError when
accessing tenantDomain.tenant_code in the tenantQueries.findOne call; update the
logic in the block using tenantDomain and tenantId (the code around
tenantDomain, tenantId, TENANT_CODE_HEADER and the notFoundResponse call) to
explicitly handle the missing-case by returning a notFoundResponse (or similar
error) when both domain and the tenant header are absent, or ensure tenantDomain
is initialized (e.g., set tenantDomain = { tenant_code: tenantId }) before
calling tenantQueries.findOne so tenantQueries.findOne always gets a valid
tenant_code.
🧹 Nitpick comments (3)
src/services/account.js (3)
115-116: Remove empty else block.The empty
elseblock adds no value and should be removed for code cleanliness.Proposed fix
if (!domainDetails) { return responses.failureResponse({ message: 'INVALID_ORG_registration_code', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) } - } else { }
506-512: Remove commented-out code.Commented-out code should be removed. If this logic is needed in the future, it can be retrieved from version control.
Proposed fix
- /* let tenantDetails = await organizationQueries.findOne( - { id: user.organization_id }, - { attributes: ['related_orgs'] } - ) - - const tenant_id = - tenantDetails && tenantDetails.parent_id !== null ? tenantDetails.parent_id : user.organization_id */
1730-1730: Good defensive error handling.Swallowing Redis deletion errors with
.catch((err) => {})is appropriate here since this is a fire-and-forget cache invalidation that shouldn't fail the main operation. However, consider logging the error for observability.- utilsHelper.redisDel(redisUserKey).catch((err) => {}) + utilsHelper.redisDel(redisUserKey).catch((err) => { + console.error('Failed to delete Redis cache:', err) + })
| /** | ||
| * Create account user | ||
| * @method POST | ||
| * @name bulkUserCreate | ||
| * @param {Object} req -request data. | ||
| * @returns {JSON} - success or error message | ||
| */ |
There was a problem hiding this comment.
Fix JSDoc @name annotation.
The @name should be accountCreate, not bulkUserCreate.
Proposed fix
/**
* Create account user
* `@method` POST
- * `@name` bulkUserCreate
+ * `@name` accountCreate
* `@param` {Object} req -request data.
* `@returns` {JSON} - success or error message
*/🤖 Prompt for AI Agents
In `@src/controllers/v1/tenant.js` around lines 162 - 168, The JSDoc for the
"Create account user" endpoint has an incorrect `@name` value; change the `@name`
annotation from "bulkUserCreate" to "accountCreate" in the JSDoc block above the
accountCreate handler so the docs match the actual method/intent (look for the
comment block containing "Create account user" and update its `@name`).
| const notificationUtils = require('@utils/notification') | ||
| const userHelper = require('@helpers/userHelper') | ||
| const { broadcastEvent } = require('@helpers/eventBroadcasterMain') | ||
| const { use } = require('i18next') |
There was a problem hiding this comment.
Remove unused import.
The use function is imported from i18next but never used in this file.
Proposed fix
-const { use } = require('i18next')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { use } = require('i18next') |
🤖 Prompt for AI Agents
In `@src/services/account.js` at line 42, Remove the unused import "use" from
i18next in this module: delete the line that destructures use from the require
call (the symbol "use" in the const { use } = require('i18next') statement) so
the file no longer imports an unused binding; if the require call is otherwise
unnecessary, replace or simplify it accordingly (e.g., remove the entire require
if nothing else from i18next is used).
| let tenantDomain | ||
|
|
||
| const tenantDomain = await tenantDomainQueries.findOne({ domain }) | ||
|
|
||
| if (!tenantDomain) { | ||
| return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN') | ||
| if (domain) { | ||
| tenantDomain = await tenantDomainQueries.findOne({ domain }) | ||
|
|
||
| if (!tenantDomain) { | ||
| return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN') | ||
| } | ||
| } else if (req.headers?.[common.TENANT_CODE_HEADER]) { | ||
| tenantId = req.headers?.[common.TENANT_CODE_HEADER] | ||
| tenantDomain = { tenant_code: tenantId } | ||
| } | ||
|
|
||
| const tenantDetail = await tenantQueries.findOne({ | ||
| code: tenantDomain.tenant_code, |
There was a problem hiding this comment.
Potential TypeError when both domain and tenant header are missing.
If domain is falsy and req.headers?.[common.TENANT_CODE_HEADER] is also undefined, tenantDomain remains undefined. Accessing tenantDomain.tenant_code on line 84 will throw a TypeError.
Proposed fix
if (domain) {
tenantDomain = await tenantDomainQueries.findOne({ domain })
if (!tenantDomain) {
return notFoundResponse('TENANT_DOMAIN_NOT_FOUND_PING_ADMIN')
}
} else if (req.headers?.[common.TENANT_CODE_HEADER]) {
tenantId = req.headers?.[common.TENANT_CODE_HEADER]
tenantDomain = { tenant_code: tenantId }
+ } else {
+ return notFoundResponse('TENANT_DOMAIN_OR_CODE_REQUIRED')
}🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 70 - 84, tenantDomain can be undefined
if neither domain nor req.headers[common.TENANT_CODE_HEADER] are present,
causing a TypeError when accessing tenantDomain.tenant_code in the
tenantQueries.findOne call; update the logic in the block using tenantDomain and
tenantId (the code around tenantDomain, tenantId, TENANT_CODE_HEADER and the
notFoundResponse call) to explicitly handle the missing-case by returning a
notFoundResponse (or similar error) when both domain and the tenant header are
absent, or ensure tenantDomain is initialized (e.g., set tenantDomain = {
tenant_code: tenantId }) before calling tenantQueries.findOne so
tenantQueries.findOne always gets a valid tenant_code.
| console.log('validationData', validationData) | ||
| console.log('userModel', userModel) |
There was a problem hiding this comment.
Remove debug console.log statements.
These debug statements should be removed before merging to production or converted to structured logging with appropriate log levels.
Proposed fix
- console.log('validationData', validationData)
- console.log('userModel', userModel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('validationData', validationData) | |
| console.log('userModel', userModel) |
🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 384 - 385, Remove the two debug
console.log statements that print validationData and userModel; either delete
them entirely or replace them with structured logging using the project's logger
(e.g., logger.debug or processLogger.debug) and ensure sensitive fields from
validationData/userModel are omitted or redacted before logging; update
occurrences of console.log('validationData', validationData) and
console.log('userModel', userModel) in src/services/account.js accordingly.
| const defaultOrganizationCode = process.env.DEFAULT_ORGANISATION_CODE | ||
|
|
||
| for (let i = 0; i < users.data.length; i++) { | ||
| let user = users.data[i] | ||
|
|
||
| // Convert Sequelize instance to plain object | ||
| if (user.toJSON) { | ||
| user = user.toJSON() | ||
| users.data[i] = user | ||
| } | ||
|
|
||
| let userOrg = user.user_organizations[0].organization_code | ||
| let validationData = await entityTypeQueries.findUserEntityTypesAndEntities({ | ||
| status: 'ACTIVE', | ||
| organization_code: { | ||
| [Op.in]: [userOrg, defaultOrganizationCode], | ||
| }, | ||
| tenant_code: params.query.tenant_code, | ||
| model_names: { [Op.contains]: [await userQueries.getModelName()] }, | ||
| }) | ||
| const prunedEntities = removeDefaultOrgEntityTypes(validationData, user.user_organizations[0].id) | ||
| const processedUser = await utils.processDbResponse(user, prunedEntities) | ||
| Object.assign(user, processedUser) | ||
| } |
There was a problem hiding this comment.
Missing bounds check on user_organizations array.
Accessing user.user_organizations[0] without checking if the array exists and has elements could cause a runtime error if a user has no organizations.
Proposed fix
for (let i = 0; i < users.data.length; i++) {
let user = users.data[i]
// Convert Sequelize instance to plain object
if (user.toJSON) {
user = user.toJSON()
users.data[i] = user
}
+ if (!user.user_organizations?.length) {
+ continue
+ }
+
let userOrg = user.user_organizations[0].organization_code🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 1946 - 1969, The code reads
user.user_organizations[0] without checking existence which can throw if a user
has no organizations; update the loop in the section using user_organizations to
first verify user.user_organizations && user.user_organizations.length > 0, and
if absent either skip processing that user (continue) or fall back to
defaultOrganizationCode for userOrg and handle the missing id when calling
removeDefaultOrgEntityTypes (e.g., pass null/undefined or guard that call).
Ensure the branches adjust the values passed to findUserEntityTypesAndEntities
and removeDefaultOrgEntityTypes and still call utils.processDbResponse only when
safe.
Added AWS secret access key exports for deployment.
Updated environment variable export syntax for AWS credentials.
Issue #000 fix: serach Users with one or more meta params
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/services/account.js`:
- Around line 324-327: Replace the direct use of req.body.roles in the query by
validating/sanitizing it: parse req.body.roles, intersect it with an explicit
whitelist of allowed self-assignable roles (define a constant like
SELF_ASSIGNABLE_ROLES), and only use the resulting safe list for the title
[Op.in] clause; if no valid roles remain, fall back to
process.env.DEFAULT_ROLE.split(','). Additionally, enforce that role assignment
from the request is only allowed for internal/admin contexts (check the existing
auth/admin flag or throw) before honoring req.body.roles, and ensure tenant_code
still uses tenantDetail.code.
🧹 Nitpick comments (3)
src/database/queries/users.js (1)
431-443: JSONB meta filtering may benefit from an index for performance.The implementation correctly validates the key with a regex to prevent injection, and
Sequelize.whereparameterizes the value. However, queries usingmeta->>'key'won't leverage indexes unless a functional GIN index exists on themetacolumn.Consider adding an index if meta filtering will be used frequently:
CREATE INDEX idx_users_meta ON users USING GIN (meta);Also, a minor improvement: the empty
Op.andarray initialization could be moved inside theifblock to avoid creating it when unused.Suggested improvement
// Filter by meta fields (generic - supports any meta field) if (metaFilters && typeof metaFilters === 'object' && Object.keys(metaFilters).length > 0) { - userWhere[Op.and] = userWhere[Op.and] || [] + const metaConditions = [] for (const [key, value] of Object.entries(metaFilters)) { if (value !== null && value !== undefined && value !== '') { // Use Sequelize.where with JSONB operator for safe parameterized queries // The key is validated to be alphanumeric/underscore only to prevent SQL injection if (/^[a-zA-Z0-9_]+$/.test(key)) { - userWhere[Op.and].push(Sequelize.where(Sequelize.literal(`"User".meta->>'${key}'`), value)) + metaConditions.push(Sequelize.where(Sequelize.literal(`"User".meta->>'${key}'`), value)) } } } + if (metaConditions.length > 0) { + userWhere[Op.and] = (userWhere[Op.and] || []).concat(metaConditions) + } }src/services/account.js (2)
506-512: Remove or address commented-out code.This commented block for tenant details lookup should either be removed if no longer needed, or uncommented and integrated if the functionality is required. Leaving commented code in production adds noise.
1730-1730: Empty catch block silently swallows Redis errors.While fire-and-forget is acceptable here, the empty catch makes debugging difficult. Consider logging at debug level.
Suggested improvement
- utilsHelper.redisDel(redisUserKey).catch((err) => {}) + utilsHelper.redisDel(redisUserKey).catch((err) => { + console.debug('Redis cache clear failed for user:', redisUserKey, err.message) + })
| title: { | ||
| [Op.in]: process.env.DEFAULT_ROLE.split(','), | ||
| [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','), | ||
| }, | ||
| tenant_code: tenantDetail.code, |
There was a problem hiding this comment.
Potential privilege escalation: roles accepted directly from request body.
Allowing req.body.roles to directly control role assignment without validation could enable users to assign themselves privileged roles (e.g., org_admin, admin). This bypasses the invitation-based role assignment flow.
Consider:
- Validating that the requested roles are within an allowed set for self-registration
- Restricting this feature to internal/admin API calls only
- Adding explicit role whitelist validation
Suggested safeguard
+ // Only allow specific roles for self-registration
+ const allowedSelfRegistrationRoles = (process.env.ALLOWED_SELF_REGISTRATION_ROLES || process.env.DEFAULT_ROLE).split(',')
+ const requestedRoles = req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(',')
+ const validRoles = requestedRoles.filter(role => allowedSelfRegistrationRoles.includes(role.trim()))
+
role = await roleQueries.findAll(
{
title: {
- [Op.in]: req.body.roles ? req.body.roles.split(',') : process.env.DEFAULT_ROLE.split(','),
+ [Op.in]: validRoles.length > 0 ? validRoles : process.env.DEFAULT_ROLE.split(','),
},🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 324 - 327, Replace the direct use of
req.body.roles in the query by validating/sanitizing it: parse req.body.roles,
intersect it with an explicit whitelist of allowed self-assignable roles (define
a constant like SELF_ASSIGNABLE_ROLES), and only use the resulting safe list for
the title [Op.in] clause; if no valid roles remain, fall back to
process.env.DEFAULT_ROLE.split(','). Additionally, enforce that role assignment
from the request is only allowed for internal/admin contexts (check the existing
auth/admin flag or throw) before honoring req.body.roles, and ensure tenant_code
still uses tenantDetail.code.
…into Issue#253287
Issue#253287 Feat: User account search API > Add status filter
…into Issue#252552
Issue#252552 Feat: User bulk upload with all entity types
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.github/workflows/brac-dev-deployment.yaml:
- Around line 76-79: The heredoc block used to write .env is indented so leading
whitespace will be injected into every line of ${{ secrets.DEV_ENV_BRAC }}; fix
by unindenting the heredoc (place the cat << 'EOF' > .env and the terminating
EOF at column 0) or switch to a strip-leading-tabs form (use cat <<-'EOF' > .env
and ensure any indentation is tabs) so that the generated .env contains exact
KEY=VALUE lines; update the lines around the symbols "cat << 'EOF' > .env", "${{
secrets.DEV_ENV_BRAC }}" and the terminating "EOF" accordingly.
In @.github/workflows/brac-qa-deplyment.yaml:
- Line 1: The workflow filename contains a typo: rename the file from
brac-qa-deplyment.yaml to brac-qa-deployment.yaml and update any references to
the old filename (e.g., in workflow dispatch calls, docs, README badges, or
other CI config) so tooling and triggers point to the new name; ensure the
workflow's internal "name: Tag Build & Deploy User Service (BRAC)" remains
unchanged and commit the renamed file so GitHub Actions recognizes the corrected
workflow.
- Around line 65-66: The workflow currently exports AWS_ACCESS_KEY_ID and
AWS_SECRET_ACCESS_KEY as plain-text environment variables in the remote shell;
replace this by avoiding direct export — either use an IAM instance role on the
target host or configure credentials via the GitHub Actions-provided steps
(e.g., aws-actions/configure-aws-credentials) instead of exporting secrets into
the shell, or at minimum wrap the export with history suppression (run "set +o
history" before exporting and "set -o history" after) and immediately unset the
variables after use; look for the exported symbols AWS_ACCESS_KEY_ID and
AWS_SECRET_ACCESS_KEY in the workflow and remove direct export usage
accordingly.
In `@src/database/queries/users.js`:
- Around line 413-415: Guard against non-string status values before calling
toUpperCase on status in the users query: check the type of the status variable
(and reject arrays/null/objects) and only call status.toUpperCase() when typeof
status === 'string' (otherwise either coerce safely with
String(status).toUpperCase() or skip/normalize the value). Update the logic
around userWhere.status in the users query (the block that currently does
userWhere.status = status.toUpperCase()) to perform this type
check/normalization so it cannot throw when status is an array, number, or other
non-string.
In `@src/services/account.js`:
- Around line 485-550: The code overwrites result = { access_token...,
refresh_token... } when registerWithLogin is true, discarding the previously set
result.user and causing later calls (utils.processDbResponse(result.user,
prunedEntities)) to receive undefined; fix by preserving the user object when
adding tokens—e.g., instead of replacing result, merge tokens into the existing
result (result = { ...result, access_token: accessToken, refresh_token:
refreshToken } or assign result.access_token = accessToken; result.refresh_token
= refreshToken) so result.user remains defined for subsequent calls to
utils.processDbResponse and updates via
userSessionsService.updateUserSessionAndsetRedisData remain correct.
- Around line 1970-1991: The loop over users calls
entityTypeQueries.findUserEntityTypesAndEntities for each user (see users.data
loop and user.user_organizations[0].organization_code), causing an N+1 query;
instead collect unique organization_code values from users.data (and include
defaultOrganizationCode), call findUserEntityTypesAndEntities once (or once per
batch) to fetch validationData for all orgs (using Op.in on organization_code
and tenant_code and model_names), build a map keyed by organization_code (or
user_organization id) to the returned entity types, then inside the loop reuse
that map and call removeDefaultOrgEntityTypes and utils.processDbResponse with
the pre-fetched prunedEntities (and optionally memoize per org id) to eliminate
per-user DB calls.
🧹 Nitpick comments (7)
.github/workflows/brac-dev-deployment.yaml (2)
54-56: Pinappleboy/ssh-actionto a specific version or commit SHA instead of@master.Using
@masterexposes the workflow to supply-chain attacks if the upstream repo is compromised — any new commit is automatically trusted. Pin to a release tag (e.g.,@v1.0.3) or a full commit SHA.
65-68: Commented-out AWS credential exports leak intent; active exports may be unnecessary.Lines 67-68 are commented out but still expose secret names. If ECR login on the remote host requires these credentials, uncomment them; otherwise remove the comments entirely to avoid confusion. Also note that
export AWS_REGIONandexport AWS_ACCOUNT_IDon lines 65-66 expose secrets in the remote shell environment — ensure the remote server's shell history and process table are appropriately secured..github/workflows/brac-qa-deplyment.yaml (1)
55-56: Pinappleboy/ssh-actionto a specific version or commit SHA instead of@master.Same concern as the dev workflow — using
@masteris a supply-chain risk.src/helpers/userInvite.js (2)
341-388: Inconsistent fallback values between hardcoded and dynamic meta field processing.The hardcoded block (lines 282-339) uses mixed fallback values:
nullfor some fields,''(empty string) for others (e.g.,professional_roleuses'',blockusesnull). The new dynamic block usesnullfor non-array types (line 379) and.filter(Boolean)for arrays (line 374). This inconsistency may cause subtle differences in validation behavior downstream (e.g.,nullvs''truthiness checks).Consider aligning the fallback values, or better yet, consolidating the hardcoded block into the dynamic loop to eliminate the duplication entirely.
352-382:validationData.find()is called per field per row — consider pre-building a lookup map.For large CSV files with many entity fields, calling
.find()onvalidationDatafor every field of every row is O(rows × fields × validationData.length). A simpleMapbuilt once before the row loop would make lookups O(1).Suggested improvement (outside the changed lines)
// Build once before the Promise.all loop (e.g., after line 262) const entityDefMap = new Map(validationData.map((e) => [e.value, e]))Then inside the loop:
-const entityDef = validationData.find((e) => e.value === field) +const entityDef = entityDefMap.get(field)src/database/queries/users.js (1)
436-448: JSONB meta field queries may not leverage indexes — consider adding expression indexes.The
"User".meta->>'key'queries generated here will perform a full table scan on themetacolumn unless there are GIN indexes or expression-based indexes defined. For frequently filtered meta fields, this could be a performance bottleneck on large user tables.Consider adding a GIN index on the
metacolumn if one doesn't already exist:CREATE INDEX idx_users_meta ON users USING GIN (meta);Or for specific high-cardinality fields, expression indexes:
CREATE INDEX idx_users_meta_province ON users ((meta->>'province'));As per coding guidelines, "Review database queries for performance… and ensure indexes can be used."
src/services/account.js (1)
1907-1922: Silently ignoring JSON parse errors on meta filter could hide malformed input.Line 1915-1917 catches the JSON parse error and silently ignores it. Consider logging a warning or returning a validation error to the caller so they know their meta filter was malformed rather than silently returning unfiltered results.
| # Write env safely (MULTILINE SAFE) | ||
| cat << 'EOF' > .env | ||
| ${{ secrets.DEV_ENV_BRAC }} | ||
| EOF |
There was a problem hiding this comment.
Heredoc indentation will inject leading whitespace into .env values.
Because the cat << 'EOF' block is indented (line 78), every line of ${{ secrets.DEV_ENV_BRAC }} will be prefixed with leading spaces/tabs. This can break environment variable parsing (e.g., KEY=value instead of KEY=value).
Proposed fix
Remove indentation or use <<- with tabs:
- cat << 'EOF' > .env
- ${{ secrets.DEV_ENV_BRAC }}
- EOF
+ cat << 'EOF' > .env
+${{ secrets.DEV_ENV_BRAC }}
+EOF🤖 Prompt for AI Agents
In @.github/workflows/brac-dev-deployment.yaml around lines 76 - 79, The heredoc
block used to write .env is indented so leading whitespace will be injected into
every line of ${{ secrets.DEV_ENV_BRAC }}; fix by unindenting the heredoc (place
the cat << 'EOF' > .env and the terminating EOF at column 0) or switch to a
strip-leading-tabs form (use cat <<-'EOF' > .env and ensure any indentation is
tabs) so that the generated .env contains exact KEY=VALUE lines; update the
lines around the symbols "cat << 'EOF' > .env", "${{ secrets.DEV_ENV_BRAC }}"
and the terminating "EOF" accordingly.
| @@ -0,0 +1,87 @@ | |||
| name: Tag Build & Deploy User Service (BRAC) | |||
There was a problem hiding this comment.
Filename typo: brac-qa-deplyment.yaml should be brac-qa-deployment.yaml.
🤖 Prompt for AI Agents
In @.github/workflows/brac-qa-deplyment.yaml at line 1, The workflow filename
contains a typo: rename the file from brac-qa-deplyment.yaml to
brac-qa-deployment.yaml and update any references to the old filename (e.g., in
workflow dispatch calls, docs, README badges, or other CI config) so tooling and
triggers point to the new name; ensure the workflow's internal "name: Tag Build
& Deploy User Service (BRAC)" remains unchanged and commit the renamed file so
GitHub Actions recognizes the corrected workflow.
| export AWS_ACCESS_KEY_ID=${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| export AWS_SECRET_ACCESS_KEY=${{ secrets.AWS_SECRET_ACCESS_KEY }} |
There was a problem hiding this comment.
AWS credentials exported in plain text on remote host.
AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are exported directly into the remote shell session. If the remote host has shell history enabled or process listing is accessible, these credentials could be leaked. Consider using an IAM instance role on the EC2 host instead, or at minimum ensure set +o history is used.
🤖 Prompt for AI Agents
In @.github/workflows/brac-qa-deplyment.yaml around lines 65 - 66, The workflow
currently exports AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY as plain-text
environment variables in the remote shell; replace this by avoiding direct
export — either use an IAM instance role on the target host or configure
credentials via the GitHub Actions-provided steps (e.g.,
aws-actions/configure-aws-credentials) instead of exporting secrets into the
shell, or at minimum wrap the export with history suppression (run "set +o
history" before exporting and "set -o history" after) and immediately unset the
variables after use; look for the exported symbols AWS_ACCESS_KEY_ID and
AWS_SECRET_ACCESS_KEY in the workflow and remove direct export usage
accordingly.
| if (status) { | ||
| userWhere.status = status.toUpperCase() | ||
| } |
There was a problem hiding this comment.
status.toUpperCase() will throw if status is not a string (e.g., passed as array or number).
While the caller currently passes params.query.status || false, if this query function is reused with unexpected input, it could throw. A quick guard would help:
Proposed fix
if (status) {
- userWhere.status = status.toUpperCase()
+ userWhere.status = String(status).toUpperCase()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (status) { | |
| userWhere.status = status.toUpperCase() | |
| } | |
| if (status) { | |
| userWhere.status = String(status).toUpperCase() | |
| } |
🤖 Prompt for AI Agents
In `@src/database/queries/users.js` around lines 413 - 415, Guard against
non-string status values before calling toUpperCase on status in the users
query: check the type of the status variable (and reject arrays/null/objects)
and only call status.toUpperCase() when typeof status === 'string' (otherwise
either coerce safely with String(status).toUpperCase() or skip/normalize the
value). Update the logic around userWhere.status in the users query (the block
that currently does userWhere.status = status.toUpperCase()) to perform this
type check/normalization so it cannot throw when status is an array, number, or
other non-string.
| let tokenDetail = {} | ||
| let result = { user } | ||
| if (registerWithLogin) { | ||
| /** | ||
| * create user session entry and add session_id to token data | ||
| * Entry should be created first, the session_id has to be added to token creation data | ||
| */ | ||
| const userSessionDetails = await userSessionsService.createUserSession( | ||
| user.id, // userid | ||
| '', // refresh token | ||
| '', // Access token | ||
| deviceInfo, | ||
| user.tenant_code | ||
| ) | ||
|
|
||
| /** | ||
| * create user session entry and add session_id to token data | ||
| * Entry should be created first, the session_id has to be added to token creation data | ||
| */ | ||
| const userSessionDetails = await userSessionsService.createUserSession( | ||
| user.id, // userid | ||
| '', // refresh token | ||
| '', // Access token | ||
| deviceInfo, | ||
| user.tenant_code | ||
| ) | ||
| /** | ||
| * Based on user organisation id get user org parent Id value | ||
| * If parent org id is present then set it to tenant of user | ||
| * if not then set user organisation id to tenant | ||
| */ | ||
|
|
||
| /** | ||
| * Based on user organisation id get user org parent Id value | ||
| * If parent org id is present then set it to tenant of user | ||
| * if not then set user organisation id to tenant | ||
| */ | ||
| /* let tenantDetails = await organizationQueries.findOne( | ||
| { id: user.organization_id }, | ||
| { attributes: ['related_orgs'] } | ||
| ) | ||
|
|
||
| /* let tenantDetails = await organizationQueries.findOne( | ||
| { id: user.organization_id }, | ||
| { attributes: ['related_orgs'] } | ||
| ) | ||
| const tenant_id = | ||
| tenantDetails && tenantDetails.parent_id !== null ? tenantDetails.parent_id : user.organization_id */ | ||
|
|
||
| const tenant_id = | ||
| tenantDetails && tenantDetails.parent_id !== null ? tenantDetails.parent_id : user.organization_id */ | ||
| tokenDetail = { | ||
| data: { | ||
| id: user.id, | ||
| name: user.name, | ||
| session_id: userSessionDetails.result.id, | ||
| organization_ids: user.organizations.map((org) => String(org.id)), // Convert to string | ||
| organization_codes: user.organizations.map((org) => String(org.code)), // Convert to string | ||
| tenant_code: tenantDetail.code, | ||
| organizations: user.organizations, | ||
| }, | ||
| } | ||
|
|
||
| const tokenDetail = { | ||
| data: { | ||
| id: user.id, | ||
| name: user.name, | ||
| session_id: userSessionDetails.result.id, | ||
| organization_ids: user.organizations.map((org) => String(org.id)), // Convert to string | ||
| organization_codes: user.organizations.map((org) => String(org.code)), // Convert to string | ||
| tenant_code: tenantDetail.code, | ||
| organizations: user.organizations, | ||
| }, | ||
| const accessToken = utilsHelper.generateToken( | ||
| tokenDetail, | ||
| process.env.ACCESS_TOKEN_SECRET, | ||
| common.accessTokenExpiry | ||
| ) | ||
|
|
||
| const refreshToken = utilsHelper.generateToken( | ||
| tokenDetail, | ||
| process.env.REFRESH_TOKEN_SECRET, | ||
| common.refreshTokenExpiry | ||
| ) | ||
|
|
||
| /** | ||
| * This function call will do below things | ||
| * 1: create redis entry for the session | ||
| * 2: update user-session with token and refresh_token | ||
| */ | ||
| await userSessionsService.updateUserSessionAndsetRedisData( | ||
| userSessionDetails.result.id, | ||
| accessToken, | ||
| refreshToken | ||
| ) | ||
|
|
||
| result = { access_token: accessToken, refresh_token: refreshToken } | ||
| } |
There was a problem hiding this comment.
Critical bug: result.user is undefined when registerWithLogin is true.
On line 486, result is initialized as { user }. On line 549, when registerWithLogin is true, result is overwritten to { access_token, refresh_token } — discarding the user property. Then line 619 attempts result.user = await utils.processDbResponse(result.user, prunedEntities), passing undefined to processDbResponse, and lines 620-622 attempt to set properties on the (likely undefined/null) return value.
Proposed fix
- result = { access_token: accessToken, refresh_token: refreshToken }
+ result = { access_token: accessToken, refresh_token: refreshToken, user }Also applies to: 619-622
🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 485 - 550, The code overwrites result =
{ access_token..., refresh_token... } when registerWithLogin is true, discarding
the previously set result.user and causing later calls
(utils.processDbResponse(result.user, prunedEntities)) to receive undefined; fix
by preserving the user object when adding tokens—e.g., instead of replacing
result, merge tokens into the existing result (result = { ...result,
access_token: accessToken, refresh_token: refreshToken } or assign
result.access_token = accessToken; result.refresh_token = refreshToken) so
result.user remains defined for subsequent calls to utils.processDbResponse and
updates via userSessionsService.updateUserSessionAndsetRedisData remain correct.
| for (let i = 0; i < users.data.length; i++) { | ||
| let user = users.data[i] | ||
|
|
||
| // Convert Sequelize instance to plain object | ||
| if (user.toJSON) { | ||
| user = user.toJSON() | ||
| users.data[i] = user | ||
| } | ||
|
|
||
| let userOrg = user.user_organizations[0].organization_code | ||
| let validationData = await entityTypeQueries.findUserEntityTypesAndEntities({ | ||
| status: 'ACTIVE', | ||
| organization_code: { | ||
| [Op.in]: [userOrg, defaultOrganizationCode], | ||
| }, | ||
| tenant_code: params.query.tenant_code, | ||
| model_names: { [Op.contains]: [await userQueries.getModelName()] }, | ||
| }) | ||
| const prunedEntities = removeDefaultOrgEntityTypes(validationData, user.user_organizations[0].id) | ||
| const processedUser = await utils.processDbResponse(user, prunedEntities) | ||
| Object.assign(user, processedUser) | ||
| } |
There was a problem hiding this comment.
N+1 query: findUserEntityTypesAndEntities is called inside a loop for each user.
This fires a separate DB query per user to fetch entity types and validation data. For large result sets, this degrades performance significantly. Consider grouping users by organization and batching the validation data lookups.
As per coding guidelines, "This is core business logic. Please check for correctness, efficiency, and potential edge cases."
🤖 Prompt for AI Agents
In `@src/services/account.js` around lines 1970 - 1991, The loop over users calls
entityTypeQueries.findUserEntityTypesAndEntities for each user (see users.data
loop and user.user_organizations[0].organization_code), causing an N+1 query;
instead collect unique organization_code values from users.data (and include
defaultOrganizationCode), call findUserEntityTypesAndEntities once (or once per
batch) to fetch validationData for all orgs (using Op.in on organization_code
and tenant_code and model_names), build a map keyed by organization_code (or
user_organization id) to the returned entity types, then inside the loop reuse
that map and call removeDefaultOrgEntityTypes and utils.processDbResponse with
the pre-fetched prunedEntities (and optionally memoize per org id) to eliminate
per-user DB calls.
Profile read while creating project giving error
Issue#254093 Feat: Update > LC update participant address
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/services/org-admin.js (1)
595-602: Remove the commented-out legacy check block.Keeping large commented logic in the active method makes this path harder to audit and maintain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/org-admin.js` around lines 595 - 602, Remove the commented-out legacy user existence check in the org admin flow: delete the commented block that declares checkUser and calls userQueries.findOne({ id: userId }) and the subsequent CLIENT_ERROR response; this unused commented logic should be removed from the method in src/services/org-admin.js to keep the codebase clean and maintainable (look for the commented checkUser / userQueries.findOne and the USER_NOT_FOUND response).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/org-admin.js`:
- Around line 621-633: The current role assignment block (condition using
isAdmin, bodyData.roles) uses roleQueries.findAll({ title: roles }) which can
drop invalid titles and is not tenant/active scoped; change the logic around
roleQueries.findAll (the call in src/services/org-admin.js) to query roles by
title AND tenant (and active status) so results are tenant-scoped, then verify
that the number of returned records matches the number of requested roles
exactly; if there is any mismatch, return the same INVALID_ROLE_ASSIGNMENTS
failure instead of silently accepting a partial set; keep assigning
updateData.roles = roleIds and setting hasRoleUpdate only after the exact-match
check passes.
- Around line 671-674: The role-update path currently calls
userQueries.updateUser({ id: userId }, updateData) without scoping to the tenant
or checking the update result; change the filter passed to
userQueries.updateUser to include the tenant identifier (e.g. { id: userId,
tenantId: currentTenantId }) so you only affect users in the same tenant, then
inspect the update result (rowsAffected / returned count / updated record) from
userQueries.updateUser and throw or return an error when no rows were updated
(to surface missing/invalid user in this tenant). Apply this change where
hasRoleUpdate is handled so role updates are tenant-scoped and validated.
In `@src/validators/v1/account.js`:
- Around line 73-87: The custom validator for req.checkBody('phone_code') only
checks a trimmed version but doesn't persist it; update the custom function in
src/validators/v1/account.js (the phone_code validator) to assign the normalized
value back to req.body.phone_code (e.g., set req.body.phone_code = trimmed)
after successful validation so whitespace is removed; apply the same change to
the other phone_code validator instance around the 194-208 block to ensure both
validators persist the normalized trimmed value.
- Around line 73-87: The phone_code validator in req.checkBody('phone_code')
only checks length and allows non-numeric values; update it to match the
login/account rules by trimming the value then validating it against a strict
country-code pattern (e.g., digits-only with optional leading '+' or just
digits) and enforcing the correct length (1–4 digits as used elsewhere),
throwing the same error messages when invalid or missing (keep the existing
conditional that requires phone_code when req.body.phone is present) so create
uses the same numeric/country-code validation as the login validation.
---
Nitpick comments:
In `@src/services/org-admin.js`:
- Around line 595-602: Remove the commented-out legacy user existence check in
the org admin flow: delete the commented block that declares checkUser and calls
userQueries.findOne({ id: userId }) and the subsequent CLIENT_ERROR response;
this unused commented logic should be removed from the method in
src/services/org-admin.js to keep the codebase clean and maintainable (look for
the commented checkUser / userQueries.findOne and the USER_NOT_FOUND response).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/services/org-admin.jssrc/services/user.jssrc/validators/v1/account.jssrc/validators/v1/tenant.js
| if (isAdmin && bodyData.roles && Array.isArray(bodyData.roles) && bodyData.roles.length > 0) { | ||
| let roles = _.without(bodyData.roles, common.ADMIN_ROLE) | ||
| let getRoleIds = await roleQueries.findAll({ title: roles }, { attributes: ['id'] }) | ||
| if (!getRoleIds) { | ||
| if (!getRoleIds || getRoleIds.length === 0) { | ||
| return responses.failureResponse({ | ||
| message: 'INVALID_ROLE_ASSIGNMENTS', | ||
| statusCode: httpStatusCode.bad_request, | ||
| responseCode: 'CLIENT_ERROR', | ||
| }) | ||
| } | ||
| let roleIds = getRoleIds.map((roleId) => roleId.id) | ||
| let checkUser = await userQueries.findOne({ id: userId }) | ||
| if (!checkUser) { | ||
| return responses.failureResponse({ | ||
| responseCode: 'CLIENT_ERROR', | ||
| statusCode: httpStatusCode.bad_request, | ||
| message: 'USER_NOT_FOUND', | ||
| }) | ||
| updateData.roles = roleIds | ||
| hasRoleUpdate = true |
There was a problem hiding this comment.
Reject partial/unsafely-scoped role resolution.
Line 623 resolves roles by title only and Line 624 only checks > 0. This can silently accept partial matches (invalid roles dropped) and isn’t scoped to tenant/active roles.
Suggested fix
- let roles = _.without(bodyData.roles, common.ADMIN_ROLE)
- let getRoleIds = await roleQueries.findAll({ title: roles }, { attributes: ['id'] })
- if (!getRoleIds || getRoleIds.length === 0) {
+ const roleTitles = [...new Set(_.without(bodyData.roles, common.ADMIN_ROLE))]
+ const getRoleIds = await roleQueries.findAll(
+ {
+ title: roleTitles,
+ tenant_code: tokenInformation.tenant_code,
+ status: common.ACTIVE_STATUS,
+ },
+ { attributes: ['id', 'title'] }
+ )
+ if (!getRoleIds || getRoleIds.length !== roleTitles.length) {
return responses.failureResponse({
message: 'INVALID_ROLE_ASSIGNMENTS',
statusCode: httpStatusCode.bad_request,
responseCode: 'CLIENT_ERROR',
})
}
- let roleIds = getRoleIds.map((roleId) => roleId.id)
+ const roleIds = getRoleIds.map(({ id }) => id)As per coding guidelines, src/services/** is core business logic and should be checked for correctness, efficiency, and potential edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/org-admin.js` around lines 621 - 633, The current role
assignment block (condition using isAdmin, bodyData.roles) uses
roleQueries.findAll({ title: roles }) which can drop invalid titles and is not
tenant/active scoped; change the logic around roleQueries.findAll (the call in
src/services/org-admin.js) to query roles by title AND tenant (and active
status) so results are tenant-scoped, then verify that the number of returned
records matches the number of requested roles exactly; if there is any mismatch,
return the same INVALID_ROLE_ASSIGNMENTS failure instead of silently accepting a
partial set; keep assigning updateData.roles = roleIds and setting hasRoleUpdate
only after the exact-match check passes.
| // Update roles if provided (only for admin) | ||
| if (hasRoleUpdate) { | ||
| await userQueries.updateUser({ id: userId }, updateData) | ||
| } |
There was a problem hiding this comment.
Scope role update by tenant and validate affected rows.
Line 673 updates by { id: userId } only and does not verify update count. This can mutate the wrong tenant’s user and can return success even when no user was updated.
Suggested fix
if (hasRoleUpdate) {
- await userQueries.updateUser({ id: userId }, updateData)
+ const [rowsAffected] = await userQueries.updateUser(
+ { id: userId, tenant_code: tokenInformation.tenant_code },
+ updateData
+ )
+ if (rowsAffected === 0) {
+ return responses.failureResponse({
+ responseCode: 'CLIENT_ERROR',
+ statusCode: httpStatusCode.bad_request,
+ message: 'USER_NOT_FOUND',
+ })
+ }
}As per coding guidelines, src/services/** is core business logic and should be checked for correctness, efficiency, and potential edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/org-admin.js` around lines 671 - 674, The role-update path
currently calls userQueries.updateUser({ id: userId }, updateData) without
scoping to the tenant or checking the update result; change the filter passed to
userQueries.updateUser to include the tenant identifier (e.g. { id: userId,
tenantId: currentTenantId }) so you only affect users in the same tenant, then
inspect the update result (rowsAffected / returned count / updated record) from
userQueries.updateUser and throw or return an error when no rows were updated
(to surface missing/invalid user in this tenant). Apply this change where
hasRoleUpdate is handled so role updates are tenant-scoped and validated.
| req.checkBody('phone_code').custom((value) => { | ||
| // If phone is provided, phone_code is mandatory | ||
| if (req.body.phone && !value) { | ||
| throw new Error('phone_code is required when phone is provided') | ||
| } | ||
| // If phone_code is provided, validate its format | ||
| if (value) { | ||
| // Convert to string and trim if it's not already a string | ||
| const trimmed = typeof value === 'string' ? value.trim() : String(value || '').trim() | ||
| if (trimmed.length < 2 || trimmed.length > 4) { | ||
| throw new Error('Phone code must be between 2 and 4 characters') | ||
| } | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
Normalize phone_code after validation to avoid whitespace persistence.
Lines 81 and 202 validate against a trimmed value but never persist that normalized value, so req.body.phone_code can remain untrimmed.
Proposed fix
req.checkBody('phone_code').custom((value) => {
...
if (value) {
const trimmed = typeof value === 'string' ? value.trim() : String(value || '').trim()
...
+ req.body.phone_code = trimmed
}
return true
})As per coding guidelines: src/validators/**: Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Also applies to: 194-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/validators/v1/account.js` around lines 73 - 87, The custom validator for
req.checkBody('phone_code') only checks a trimmed version but doesn't persist
it; update the custom function in src/validators/v1/account.js (the phone_code
validator) to assign the normalized value back to req.body.phone_code (e.g., set
req.body.phone_code = trimmed) after successful validation so whitespace is
removed; apply the same change to the other phone_code validator instance around
the 194-208 block to ensure both validators persist the normalized trimmed
value.
create accepts invalid phone_code formats.
On Line 82, only length is enforced, so non-country-code values can pass. This is inconsistent with Line 203 and login validation, and can create accounts with unusable phone credentials.
Proposed fix
- req.checkBody('phone_code').custom((value) => {
+ req.checkBody('phone_code').custom((value) => {
// If phone is provided, phone_code is mandatory
if (req.body.phone && !value) {
throw new Error('phone_code is required when phone is provided')
}
// If phone_code is provided, validate its format
if (value) {
- // Convert to string and trim if it's not already a string
const trimmed = typeof value === 'string' ? value.trim() : String(value || '').trim()
- if (trimmed.length < 2 || trimmed.length > 4) {
- throw new Error('Phone code must be between 2 and 4 characters')
+ if (!/^\+[1-9][0-9]{0,3}$/.test(trimmed)) {
+ throw new Error('phone_code must be a valid country code (e.g., +1, +91)')
}
+ req.body.phone_code = trimmed
}
return true
})As per coding guidelines: src/validators/**: Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| req.checkBody('phone_code').custom((value) => { | |
| // If phone is provided, phone_code is mandatory | |
| if (req.body.phone && !value) { | |
| throw new Error('phone_code is required when phone is provided') | |
| } | |
| // If phone_code is provided, validate its format | |
| if (value) { | |
| // Convert to string and trim if it's not already a string | |
| const trimmed = typeof value === 'string' ? value.trim() : String(value || '').trim() | |
| if (trimmed.length < 2 || trimmed.length > 4) { | |
| throw new Error('Phone code must be between 2 and 4 characters') | |
| } | |
| } | |
| return true | |
| }) | |
| req.checkBody('phone_code').custom((value) => { | |
| // If phone is provided, phone_code is mandatory | |
| if (req.body.phone && !value) { | |
| throw new Error('phone_code is required when phone is provided') | |
| } | |
| // If phone_code is provided, validate its format | |
| if (value) { | |
| const trimmed = typeof value === 'string' ? value.trim() : String(value || '').trim() | |
| if (!/^\+[1-9][0-9]{0,3}$/.test(trimmed)) { | |
| throw new Error('phone_code must be a valid country code (e.g., +1, +91)') | |
| } | |
| req.body.phone_code = trimmed | |
| } | |
| return true | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/validators/v1/account.js` around lines 73 - 87, The phone_code validator
in req.checkBody('phone_code') only checks length and allows non-numeric values;
update it to match the login/account rules by trimming the value then validating
it against a strict country-code pattern (e.g., digits-only with optional
leading '+' or just digits) and enforcing the correct length (1–4 digits as used
elsewhere), throwing the same error messages when invalid or missing (keep the
existing conditional that requires phone_code when req.body.phone is present) so
create uses the same numeric/country-code validation as the login validation.
4 participants
…rties of undefined (reading 'outputFilePath')
Summary by CodeRabbit
New Features
Bug Fixes
Chores