Skip to content

Organization list fix for SCP #793

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.

Sign up for GitHub

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

Open
adithyadinesh0412 wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Organization list fix for SCP #793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef1b465
Organization list fix for SCP
adithyadinesh0412 Aug 12, 2025
ada07ec
review comment
adithyadinesh0412 Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/services/organization.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,14 +254,17 @@ module.exports = class OrganizationsHelper {
let options = {
attributes: ['id', 'name', 'code', 'description'],
}
let filter = {
tenant_code: params?.query?.tenantCode,
status: common.ACTIVE_STATUS,
}
Comment on lines +258 to +260
Copy link

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify tenantCode trust boundary

You’re trusting params.query.tenantCode directly to scope results. Per retrieved learnings, tenant/organization identifiers are typically sourced from req.decodedToken and guaranteed after token validation. Allowing a query override might enable cross-tenant data access unless the controller gates this route to privileged roles.

To verify call-sites and access control, run:

If this endpoint is not strictly admin-scoped, prefer deriving tenant from the token instead of a query param, or ignore the query value when a token is present.

🏁 Script executed:

#!/bin/bash
# Find where OrganizationsHelper.list is invoked and inspect how tenantCode is sourced/gated
rg -n -A4 -B4 "OrganizationsHelper\.list\("
# Also check any controllers/middlewares that handle 'tenantCode' for list endpoints
rg -n -A3 -B3 "tenantCode" | rg -n -A3 -B3 "list|organization"

Length of output: 70421

🏁 Script executed:

#!/bin/bash
# Identify how the organization service is imported in the v1 controller
rg -n "require.*services/organization" src/controllers/v1/organization.js

# Locate where the list method is invoked in the organization controller
rg -n "await .*\.list" src/controllers/v1/organization.js
rg -n "\.list(" -n src/controllers/v1/organization.js

Length of output: 361

Remove direct query override for tenantCode in organization.list

The orgService.list method is currently trusting req.query.tenantCode, but the controller computes a safe tenantCode from the token (and body for admins) without forwarding it. As a result, anyone can supply ?tenantCode=… on the URL and scope results to another tenant.

• In src/controllers/v1/organization.js around line 119, replace:

const result = await orgService.list(req)

with something like:

// forward only the vetted tenantCode into the service
const params = {
  query: { tenantCode },       // use the controller’s computed tenantCode
  body: req.body,
  decodedToken: req.decodedToken
}
const result = await orgService.list(params)

• In src/services/organization.js (lines 253–260), change the tenant filter to use the token-sourced value instead of the raw query:

- tenant_code: params?.query?.tenantCode,
+ tenant_code: params?.query?.tenantCode || params?.decodedToken?.tenant_code,

—or remove the params.query branch entirely and rely solely on params.decodedToken.tenant_code (with an explicit admin override if needed).

This ensures the tenantCode always comes from the validated token (or an explicit, authorized override) rather than an untrusted query parameter.

🤖 Prompt for AI Agents 
In src/services/organization.js around lines 258 to 260, the tenant filter is
using params?.query?.tenantCode which trusts an unvalidated query param; change
it to use the vetted tenant from the token (params.decodedToken.tenant_code) or
remove the params.query branch entirely and only accept an explicit, authorized
override via decodedToken (i.e., ignore req.query.tenantCode and derive
tenantCode from params.decodedToken or an admin-authorized field before applying
the filter).

if (params.body && params.body.organization_codes) {
filter.code = {
[Op.in]: params.body.organization_codes.map((code) => code.toString().toLowerCase().trim()),
}
}

let organizations = await organizationQueries.findAll(
{
tenant_code: params?.query?.tenantCode,
status: common.ACTIVE_STATUS,
},
options
)
let organizations = await organizationQueries.findAll(filter, options)

return responses.successResponse({
statusCode: httpStatusCode.ok,
Expand Down
14 changes: 14 additions & 0 deletions src/validators/v1/organization.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,18 @@ module.exports = {
.matches(/^[a-zA-Z0-9_]+$/)
.withMessage('Each registration code must be alphanumeric with underscores only')
},
list: (req) => {
req.checkBody('organization_codes')
.optional({ checkFalsy: true })
.custom((value) => {
if (!Array.isArray(value)) {
throw new Error('organization_codes must be an array')
}
return true
})
Comment on lines +146 to +153
Copy link

@coderabbitai coderabbitai bot Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Cap the size of organization_codes to prevent large IN queries

Add an upper bound (e.g., 100 items) to avoid excessive IN (...) lists that can degrade DB performance or be abused.

Apply this diff:

-        req.checkBody('organization_codes')
-            .optional({ checkFalsy: true })
-            .custom((value) => {
-                if (!Array.isArray(value)) {
-                    throw new Error('organization_codes must be an array')
-                }
-                return true
-            })
+        req.checkBody('organization_codes')
+            .optional({ checkFalsy: true })
+            .custom((value) => {
+                if (!Array.isArray(value)) {
+                    throw new Error('organization_codes must be an array')
+                }
+                if (value.length > 100) {
+                    throw new Error('organization_codes cannot exceed 100 entries')
+                }
+                return true
+            })
📝 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.

Suggested change
req.checkBody('organization_codes')
.optional({ checkFalsy: true })
.custom((value) => {
if (!Array.isArray(value)) {
throw new Error('organization_codes must be an array')
}
return true
})
req.checkBody('organization_codes')
.optional({ checkFalsy: true })
.custom((value) => {
if (!Array.isArray(value)) {
throw new Error('organization_codes must be an array')
}
if (value.length > 100) {
throw new Error('organization_codes cannot exceed 100 entries')
}
return true
})
🤖 Prompt for AI Agents 
In src/validators/v1/organization.js around lines 146 to 153, the custom
validator for organization_codes currently only ensures the value is an array;
add an upper bound check to prevent very large IN queries by validating that
value.length is <= 100 and throw a clear Error (e.g., 'organization_codes cannot
contain more than 100 items') if exceeded; implement this inside the same custom
validator so requests with more than 100 items fail validation early.

req.checkBody('organization_codes.*')
.optional({ checkFalsy: true })
.matches(/^[a-zA-Z0-9_]+$/)
.withMessage('Each organization code must be alphanumeric with underscores only')
},
}