Skip to content

Feature/org id code migration#1644

Open
borkarsaish65 wants to merge 6 commits into
ELEVATE-Project:developfrom
borkarsaish65:feature/org_id_code_migration
Open

Feature/org id code migration#1644
borkarsaish65 wants to merge 6 commits into
ELEVATE-Project:developfrom
borkarsaish65:feature/org_id_code_migration

Conversation

@borkarsaish65
Copy link
Copy Markdown
Contributor

@borkarsaish65 borkarsaish65 commented May 27, 2026

Review Change Stack

borkarsaish65 and others added 3 commits May 22, 2026 18:19
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@borkarsaish65
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review and check if everything is okay and approved from your side.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 1644 --repo ELEVATE-Project/mentoring --json title,body,state,additions,deletions,changedFiles,files,reviews,comments,reviewRequests,labels,assignees

echo "---FILES---"
gh pr diff 1644 --repo ELEVATE-Project/mentoring

echo "---REVIEW COMMENTS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/comments --paginate | jq '[.[] | {id, path, line, body, user: .user.login}]'

echo "---REVIEWS---"
gh api repos/ELEVATE-Project/mentoring/pulls/1644/reviews | jq '[.[] | {id, state, body, user: .user.login}]'

Length of output: 63657


🏁 Script executed:

#!/bin/bash
echo "=== processEntityTypesToAddValueLabels signature ==="
rg -n "processEntityTypesToAddValueLabels" --type=js -C 5 | head -80

echo ""
echo "=== organisationExtensionQueries.findAll ==="
ast-grep --pattern 'static async findAll($$$) { $$$ }' | head -60

echo ""
echo "=== findAll in organisationExtension queries ==="
fd -i "organisationExtension" --type f | xargs grep -n "findAll" 2>/dev/null | head -30

echo ""
echo "=== updateRelatedOrgs function signature in services/org-admin ==="
rg -n "updateRelatedOrgs" --type=js -C 3 | head -60

echo ""
echo "=== getEntityTypesWithMentorOrg callers ==="
rg -n "getEntityTypesWithMentorOrg" --type=js -C 2

echo ""
echo "=== PUSH and POP constants ==="
rg -n "PUSH\|POP" src/constants/common.js

Length of output: 12145



@borkarsaish65, I've done a thorough review of the PR. The overall migration strategy is well-designed — two-phase migrations with a safe backfill, cache-first org ID→code lookups with Kafka invalidation, and backward-compatible API contracts. That said, I've found several issues ranging from critical SQL injection risks to minor concerns.


🔴 Critical — SQL Injection Risks

1. Migration batch update — u.code interpolated directly

File: src/database/migrations/20260525000001-backfill-org-codes-from-ids.js

// UNSAFE — if an org code contains a single quote, this breaks/injects
const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code}')`).join(', ')
await queryInterface.sequelize.query(`
    UPDATE sessions SET mentor_organization_code = v.code
    FROM (VALUES ${values}) AS v(id, code)
    WHERE sessions.id = v.id::int
`)

Use a parameterized approach instead (e.g. loop with replacements, or escape single quotes):

// Safe option: escape single quotes in org codes
const values = sessionCodeUpdates
    .map((u) => `(${u.id}, '${u.code.replace(/'/g, "''")}')`)
    .join(', ')

2. Migration pgArr() — array values not quoted

const pgArr = (arr) => `{${arr.join(',')}}` // ❌ no quoting

Org codes with commas or spaces would corrupt the Postgres array literal. Fix:

const pgArr = (arr) => `{${arr.map((v) => `"${v.replace(/"/g, '\\"')}"`).join(',')}}`

3. mentorExtension.js — Sequelize literal with unescaped org codes

`array_cat(COALESCE("visible_to_organizations", ARRAY[]::varchar[]),
  ARRAY[${newRelatedOrgs.map((v) => `'${v}'`).join(',')}]::varchar[])`

If any org code contains a ', this breaks. Org codes from external sources should be escaped.

4. userExtension.js — same pattern

`COALESCE("visible_to_organizations", ARRAY[]::varchar[]) || ARRAY['${organizationId}']::varchar[]`

organizationId (now an org code string) is interpolated unsafely.


🟠 Important Issues

5. controllers/v1/org-admin.js — undefined org ID passed as literal string "undefined"

const [orgCode] = await convertOrgIdsToOrgCodes([req.body.organization_id], tenantCode)

If req.body.organization_id is undefined, normalizedIds becomes ['undefined'], which will query the DB for an org with ID 'undefined', silently returning no code. Add a guard:

if (!req.body.organization_id) {
    return responses.failureResponse({ message: 'organization_id is required', statusCode: 400 })
}

6. Missing table_schema = 'public' in information_schema checks

All three information_schema.columns checks in the migration lack a schema filter and could match tables in other schemas:

-- Add this to all three checks:
AND table_schema = 'public'

7. Stale JSDoc in src/services/org-admin.js updateRelatedOrgs

The function comment still documents the old numeric-ID parameters:

* `@param` {Number} orgId  - Specific orgId which was updated

Should be updated to reflect that org codes are now passed.


🟡 Minor / Style

8. relatedOrgCodes1/2/3/4 naming in src/services/org-admin.js

Using numbered suffixes (relatedOrgCodes1, relatedOrgCodes2, etc.) across different if branches is a smell. Each variable lives in its own if block — they can all safely be named relatedOrgCodes without collision (or extracted into a shared helper).

9. dropOrgNumericBackupColumns.jsmain() auto-executes on require

The script calls main() at module level, meaning it will execute immediately if accidentally require()d elsewhere. Consider wrapping with:

if (require.main === module) {
    main().then(() => process.exit(0)).catch(...)
}

✅ What's Done Well

  • Removing true (orgIdPassed) from processEntityTypesToAddValueLabels — correct, since codes are now passed directly.
  • Two-phase migration (backfill → rename) is a safe, production-friendly approach with full down() reversibility.
  • Cache invalidation via Kafka on org create/update/deactivate events is the right pattern.
  • Case-insensitive action comparison (action?.toLowerCase()) is a good defensive fix.
  • accessorEntityTypes passing organization_code twice is intentional and correct post-migration.
  • convertOrgIdsToOrgCodes deduplicating IDs and batching uncached DB lookups is efficient.

Summary: The migration approach is architecturally sound, but the SQL injection issues (#1#4) need to be addressed before merging, especially since migrations run against production data. Once those are fixed (along with the null guard in the controller), this looks ready to go.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Warning

Review limit reached

@borkarsaish65, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 34 minutes and 35 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c28e8d45-6a83-43a1-9f4a-9147ae04077a

📥 Commits

Reviewing files that changed from the base of the PR and between eab19b7 and 9a20f13.

📒 Files selected for processing (4)
  • src/controllers/v1/org-admin.js
  • src/database/migrations/20260525000001-backfill-org-codes-from-ids.js
  • src/database/queries/mentorExtension.js
  • src/database/queries/userExtension.js

Walkthrough

Replaces numeric organization IDs with organization code strings across cache, helper, DB schema/migrations, queries, services, Kafka consumer, and a cleanup script.

Changes

Organization Code Migration

Layer / File(s) Summary
Cache infrastructure and ID-to-code conversion
src/constants/common.js, src/generics/cacheHelper.js, src/helpers/orgUtils.js
New orgIdCode cache namespace and helper; added convertOrgIdsToOrgCodes performing per-ID cache reads, selective DB fetches, caching of results, and ordered code-array return.
Database schema updates and migrations
src/database/models/sessions.js, src/database/migrations/20260525000001-backfill-org-codes-from-ids.js, src/database/migrations/20260525000002-rename-visible-to-org-codes.js
Added mentor_organization_code column to Session model; backfill migration adds/populates code columns from numeric IDs; rename migration swaps visible-to columns to use code-based columns with numeric backups.
Database query layer refactoring
src/database/queries/mentorExtension.js, src/database/queries/userExtension.js, src/database/queries/sessions.js
Updated array operations to use varchar[], changed tenant-scoped filters to organization_code, and extended session queries to project mentor_organization_code.
Entity type helper updates
src/generics/cacheHelper.js
getEntityTypesWithMentorOrg now accepts mentor organization code and computes effective code before fetching entity types.
SaaS accessibility and policy filtering
src/helpers/saasUserAccessibility.js, src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js
All SaaS policy checks and organization-filter predicates switched to use organization_code and code-based visible_to_organizations membership tests.
Organization administration layer
src/controllers/v1/org-admin.js, src/services/org-admin.js
Controller converts incoming org IDs to codes before service calls; service methods compute visible_to_organizations using converted codes; updateRelatedOrgs action handling made case-insensitive.
Mentor service refactoring
src/services/mentors.js
Mentor enrichment and access checks use mentor_organization_code; visible_to_organizations arrays recomputed from org codes; policy lookups include organization_code.
Mentee service refactoring
src/services/mentees.js
Session enrichment uses mentor_organization_code; createMenteeExtension converts related org IDs to codes; SaaS-policy SQL filters use organization_code.
Session service refactoring
src/services/sessions.js
Session creation writes mentor_organization_code; visible_to_organizations built from converted org codes; access-control and tenant-code derivation now use org codes.
Kafka organization consumer
src/generics/kafka/consumers/organization.js
Invalidates orgIdCode cache on create/update/deactivate; converts related-org ID changes to codes before calling org-admin updates.
Cleanup script
src/scripts/dropOrgNumericBackupColumns.js
New CLI script to inspect/drop numeric backup columns (visible_to_organizations_numeric) with dry-run/count support and materialized view rebuild after successful drops.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • nevil-mathew

Poem

🐰 I hopped through code and cached each key,

IDs to codes — now tidy as can be.
Migrations ran, arrays turned to strings,
Consumers cleared caches, scripts trimmed wings.
A little rabbit claps — hooray for clean things!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feature/org id code migration' is directly related to the main change—a comprehensive migration from numeric organization IDs to organization codes, including database migrations, service updates, and cache logic.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/database/queries/userExtension.js (1)

120-121: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Raw interpolation in array SQL literals is unsafe for org codes.

Line 120 and Line 150 embed org-code values directly into SQL. This can fail on quoted values and introduces injection risk.

Also applies to: 150-150

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/database/queries/userExtension.js` around lines 120 - 121, The code
currently injects org codes directly into SQL via the raw interpolation used in
the ARRAY[${newRelatedOrgsSql}] expression (affecting the array_cat/COALESCE
update of visible_to_organizations and the similar occurrence around line 150),
which is unsafe; instead, change the SQL to use a parameterized array (e.g.
array_cat(COALESCE("visible_to_organizations", ARRAY[]::varchar[]),
$X::varchar[])) and pass the org codes as a bound array parameter (e.g.
relatedOrgs) from the query builder/execution call; update both places that
reference newRelatedOrgsSql to use the parameter placeholder and add the
corresponding parameter in the query parameters list so values are safely
escaped and not raw-interpolated.
🧹 Nitpick comments (2)
src/services/mentors.js (1)

492-496: ⚖️ Poor tradeoff

Mentor creation sets minimal visible_to_organizations compared to role change.

When creating a mentor extension directly, visible_to_organizations is set to only [orgCode]. However, when a mentee is changed to mentor via changeRoleToMentor in org-admin.js (lines 235-241), the visible_to_organizations includes both related_orgs codes and the current org code.

This inconsistency means mentors created through different flows will have different visibility scopes. If this is intentional (e.g., direct creation should have minimal visibility, role change inherits existing visibility), consider adding a comment to clarify.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/mentors.js` around lines 492 - 496, The mentor creation path in
src/services/mentors.js currently sets visible_to_organizations to only
[orgCode], which differs from the behavior in changeRoleToMentor (org-admin.js)
that sets visible_to_organizations to include related_orgs plus the current org;
update the mentor creation logic (the code that builds data and assigns
visible_to_organizations) to compute the same visibility set as
changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal
visibility is intentional, add a clear inline comment near the assignment of
visible_to_organizations explaining the deliberate difference and why it should
not inherit related_orgs—refer to visible_to_organizations, orgCode, and
changeRoleToMentor to locate the relevant code paths.
src/services/org-admin.js (1)

686-694: 💤 Low value

Parameter names are misleading after migration.

The function receives organization codes from the controller, but the parameter names still say deltaOrganizationIds and orgId. While .toString() and .map(String) are harmless on strings, the naming creates confusion about what data type is expected.

♻️ Suggested rename for clarity
-static async updateRelatedOrgs(deltaOrganizationIds, orgId, action, tenantCode) {
+static async updateRelatedOrgs(deltaOrgCodes, orgCode, action, tenantCode) {
 	try {
-		orgId = orgId.toString()
-		deltaOrganizationIds = deltaOrganizationIds.map(String)
+		orgCode = String(orgCode)
+		deltaOrgCodes = deltaOrgCodes.map(String)
 		if (action?.toLowerCase() === common.PUSH.toLowerCase()) {
-			await menteeQueries.addVisibleToOrg(orgId, deltaOrganizationIds, tenantCode)
+			await menteeQueries.addVisibleToOrg(orgCode, deltaOrgCodes, tenantCode)
 		} else if (action?.toLowerCase() === common.POP.toLowerCase()) {
-			await menteeQueries.removeVisibleToOrg(orgId, deltaOrganizationIds, tenantCode)
+			await menteeQueries.removeVisibleToOrg(orgCode, deltaOrgCodes, tenantCode)
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/org-admin.js` around lines 686 - 694, The parameter names in
updateRelatedOrgs are misleading: rename orgId to orgCode and
deltaOrganizationIds to deltaOrganizationCodes (function: updateRelatedOrgs) and
update all internal uses (replace orgId.toString() with orgCode =
String(orgCode) or just use orgCode if already a string, and
deltaOrganizationIds = deltaOrganizationIds.map(String) → deltaOrganizationCodes
= deltaOrganizationCodes.map(String)); also update calls to
menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and
deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to
use the new parameter names to keep semantics clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/controllers/v1/org-admin.js`:
- Around line 139-145: The conversion result for organization_id may be empty
causing orgCode to be undefined; in the controller code around
convertOrgIdsToOrgCodes and before calling orgAdminService.updateRelatedOrgs,
validate that the returned [orgCode] is present and valid (e.g., not
undefined/null/empty) and if missing respond with an appropriate error (HTTP 400
or throw a ValidationError) including a clear message referencing
req.body.organization_id; ensure deltaOrgCodes is still computed but
short-circuit the updateRelatedOrgs call when orgCode is invalid to avoid
downstream failures.

In `@src/database/migrations/20260525000001-backfill-org-codes-from-ids.js`:
- Around line 101-108: The migration is doing per-row DB updates in the sessions
loop (sessionRows, toCodeArray, pgArr, queryInterface.sequelize.query) which
causes many round-trips; change it to a set-based update: build a single
in-memory list of (id, visible_to_organization_codes) updates using toCodeArray
and pgArr, then execute one SQL statement to apply them (for example using a
single UPDATE with a CASE ... WHEN id = ... THEN ... END or by bulk inserting
the tuples into a temp table and running an UPDATE ... FROM temp_table) so
sessionArrUpdated is updated accordingly; apply the same refactor to the other
per-row loop referenced in the comment (the block around the second loop).
- Around line 52-53: The current pgArr helper builds Postgres array literals via
arr.join(',') which breaks on org codes containing commas/quotes/braces; replace
use of pgArr by passing JS arrays as parameterized query parameters (e.g. use
$n::text[] and supply the JS array to node-postgres) instead of
string-concatenating, or implement a safe escaping routine that produces a valid
Postgres array literal (escaping quotes/backslashes and wrapping elements in
double quotes) and use that in place of pgArr; update every place that calls
pgArr (the pgArr function and the query sites that use it in this file) to use
parameterized array binding or the safe-escaping function.
- Around line 68-74: The migration is interpolating org codes into SQL via the
values string (sessionCodeUpdates → values) which can break on quotes and allow
injection; instead build a parameterized VALUES list and pass the parameters to
queryInterface.sequelize.query (or use binds) so no raw codes are concatenated.
Concretely, produce a VALUES clause of placeholders like ($1,$2),($3,$4),...
corresponding to each sessionCodeUpdates entry, collect ids and codes into a
single replacements/bind array, and call queryInterface.sequelize.query(sql, {
replacements: params }) (or { bind: params }) so the UPDATE of sessions SET
mentor_organization_code = v.code FROM (VALUES ...) AS v(id, code) uses
parameterized values rather than interpolated u.code.

In `@src/database/queries/mentorExtension.js`:
- Around line 360-363: The SQL literal is vulnerable because newRelatedOrgs
values are interpolated directly into sequelize.literal for array_cat/COALESCE;
instead, stop direct string interpolation of newRelatedOrgs and parameterize or
escape each org value before building the array expression. Locate the
array_cat(...) usage that references "visible_to_organizations" and
newRelatedOrgs and either (a) build the array via Sequelize helpers (e.g.,
sequelize.fn/ sequelize.cast) so values are passed as parameters, or (b) escape
each value with sequelize.escape (or use query bind/replacements) when
constructing the ARRAY[...] expression so quotes and special chars cannot break
the SQL or allow injection.

In `@src/scripts/dropOrgNumericBackupColumns.js`:
- Around line 106-107: The ALTER TABLE drop loop currently exits on the first
error which prevents the final materialized view rebuilds; modify the block that
runs await db.sequelize.query(`ALTER TABLE "${table}" DROP COLUMN IF EXISTS
"${column}" CASCADE`) / console.log(...) so it catches and records errors (do
not call process.exit or return from inside the loop), continue processing
remaining drops, and after the loop run the materialized view rebuild logic
regardless, then if any drops failed return a non-zero exit or throw a
summarized error; update any early-exit calls near the drop loop to instead
defer failure propagation until after the rebuild step.

---

Outside diff comments:
In `@src/database/queries/userExtension.js`:
- Around line 120-121: The code currently injects org codes directly into SQL
via the raw interpolation used in the ARRAY[${newRelatedOrgsSql}] expression
(affecting the array_cat/COALESCE update of visible_to_organizations and the
similar occurrence around line 150), which is unsafe; instead, change the SQL to
use a parameterized array (e.g. array_cat(COALESCE("visible_to_organizations",
ARRAY[]::varchar[]), $X::varchar[])) and pass the org codes as a bound array
parameter (e.g. relatedOrgs) from the query builder/execution call; update both
places that reference newRelatedOrgsSql to use the parameter placeholder and add
the corresponding parameter in the query parameters list so values are safely
escaped and not raw-interpolated.

---

Nitpick comments:
In `@src/services/mentors.js`:
- Around line 492-496: The mentor creation path in src/services/mentors.js
currently sets visible_to_organizations to only [orgCode], which differs from
the behavior in changeRoleToMentor (org-admin.js) that sets
visible_to_organizations to include related_orgs plus the current org; update
the mentor creation logic (the code that builds data and assigns
visible_to_organizations) to compute the same visibility set as
changeRoleToMentor (include related_orgs codes and orgCode) or, if minimal
visibility is intentional, add a clear inline comment near the assignment of
visible_to_organizations explaining the deliberate difference and why it should
not inherit related_orgs—refer to visible_to_organizations, orgCode, and
changeRoleToMentor to locate the relevant code paths.

In `@src/services/org-admin.js`:
- Around line 686-694: The parameter names in updateRelatedOrgs are misleading:
rename orgId to orgCode and deltaOrganizationIds to deltaOrganizationCodes
(function: updateRelatedOrgs) and update all internal uses (replace
orgId.toString() with orgCode = String(orgCode) or just use orgCode if already a
string, and deltaOrganizationIds = deltaOrganizationIds.map(String) →
deltaOrganizationCodes = deltaOrganizationCodes.map(String)); also update calls
to menteeQueries.addVisibleToOrg and removeVisibleToOrg to pass orgCode and
deltaOrganizationCodes, and update any external callers of updateRelatedOrgs to
use the new parameter names to keep semantics clear.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 342279b4-2f90-44da-b1e9-ed03d2de5b48

📥 Commits

Reviewing files that changed from the base of the PR and between be915ae and 6efdd38.

📒 Files selected for processing (18)
  • src/constants/common.js
  • src/controllers/v1/org-admin.js
  • src/database/migrations/20260525000001-backfill-org-codes-from-ids.js
  • src/database/migrations/20260525000002-rename-visible-to-org-codes.js
  • src/database/models/sessions.js
  • src/database/queries/mentorExtension.js
  • src/database/queries/sessions.js
  • src/database/queries/userExtension.js
  • src/generics/cacheHelper.js
  • src/generics/kafka/consumers/organization.js
  • src/helpers/getOrgIdAndEntityTypewithEntitiesBasedOnPolicy.js
  • src/helpers/orgUtils.js
  • src/helpers/saasUserAccessibility.js
  • src/scripts/dropOrgNumericBackupColumns.js
  • src/services/mentees.js
  • src/services/mentors.js
  • src/services/org-admin.js
  • src/services/sessions.js

Comment thread src/controllers/v1/org-admin.js
Comment thread src/database/migrations/20260525000001-backfill-org-codes-from-ids.js Outdated
Comment thread src/database/migrations/20260525000001-backfill-org-codes-from-ids.js Outdated
Comment thread src/database/queries/mentorExtension.js
Comment thread src/scripts/dropOrgNumericBackupColumns.js
@borkarsaish65
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review and see if everything is okay.
If fine approve the PR from your side.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== 1. Migration backfill VALUES interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape"

echo ""
echo "=== 2. pgArr helper ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "pgArr" | head -10

echo ""
echo "=== 3. mentorExtension.js - array_cat unsafe interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[" | head -20

echo ""
echo "=== 4. userExtension.js - ARRAY interpolation ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "ARRAY\[" | head -20

echo ""
echo "=== 5. controller org-admin undefined guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -A5 "organization_id\|convertOrgIds\|failureResponse\|400" | head -40

echo ""
echo "=== 6. information_schema table_schema filter ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public"

echo ""
echo "=== 7. scripts/dropOrgNumericBackupColumns.js - main() guard ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | tail -20

echo ""
echo "=== 8. JSDoc in org-admin service updateRelatedOrgs ==="
gh api repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=feature/org_id_code_migration | jq -r '.content' | base64 -d | grep -n -B2 -A10 "updateRelatedOrgs" | head -40

Length of output: 3224


🏁 Script executed:

#!/bin/bash
# Get the PR head SHA first
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
HEAD_REF=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.ref')
echo "HEAD REF: $HEAD_REF"

echo ""
echo "=== 1. Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "u\.code\|pgArr\|VALUES\|replace\|escape\|table_schema\|information_schema"

echo ""
echo "=== 2. mentorExtension.js - array_cat unsafe interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\["

echo ""
echo "=== 3. userExtension.js - ARRAY interpolation ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\["

echo ""
echo "=== 4. controller org-admin - undefined guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/controllers/v1/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A8 "updateRelatedOrgs\|convertOrgIds\|organization_id\|failureResponse\|400" | head -50

echo ""
echo "=== 5. dropOrgNumericBackupColumns.js tail ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -15

echo ""
echo "=== 6. org-admin service JSDoc for updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -A15 "updateRelatedOrgs" | head -50

Length of output: 7293



@borkarsaish65, I've re-inspected the current state of the PR against every issue I flagged. Here's the updated status:


✅ Fixed

# Issue
#5 Controller updateRelatedOrgs now has an if (!orgCode) guard returning ORGANIZATION_NOT_FOUND — this adequately covers the missing organization_id case.

🔴 Still Open — Blocking Approval

#1 — Migration VALUES interpolation (SQL injection risk)
src/database/migrations/20260525000001-backfill-org-codes-from-ids.js, Line 68:

// Still unescaped:
const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code}')`).join(', ')

Fix: escape single quotes in u.code:

const values = sessionCodeUpdates.map((u) => `(${u.id}, '${u.code.replace(/'/g, "''")}')`).join(', ')

#2pgArr() helper (array corruption)
Line 52 — no element quoting:

// Still broken for codes with commas/spaces:
const pgArr = (arr) => `{${arr.join(',')}}`

Fix:

const pgArr = (arr) => `{${arr.map((v) => `"${v.replace(/"/g, '\\"')}"`).join(',')}}`

#3mentorExtension.js unescaped ARRAY literals
Lines 360 & 390:

`ARRAY[${newRelatedOrgs.map((v) => `'${v}'`).join(',')}]::varchar[]`
`COALESCE(...) || ARRAY['${organizationId}']::varchar[]`

Org codes interpolated as string literals — a ' in any code breaks/injects. Escape with .replace(/'/g, "''").

#4userExtension.js unescaped ARRAY literals
Lines 120 & 150 — same pattern as #3. Needs the same escaping treatment.


🟡 Still Open — Non-blocking

#6information_schema queries missing table_schema = 'public' (Lines 18, 83, 116)

#7 — Stale JSDoc in src/services/org-admin.js (Lines 681–682 still reference @param {Array} relatedOrgs and @param {Number} orgId)

#9dropOrgNumericBackupColumns.js calls main() unconditionally at module level (no require.main === module guard)


I cannot approve the PR yet because issues #1#4 are SQL injection / data corruption risks on production migration data. Once those four are addressed, I'm happy to re-review and approve.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@borkarsaish65
Copy link
Copy Markdown
Contributor Author

@coderabbitai made changes to the codebase. Just verify now.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
HEAD_SHA=$(gh api repos/ELEVATE-Project/mentoring/pulls/1644 | jq -r '.head.sha')
echo "HEAD SHA: $HEAD_SHA"

echo ""
echo "=== `#1` & `#2`: Migration backfill - VALUES interpolation & pgArr ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "pgArr\|u\.code\|VALUES\|replace\|escape\|table_schema\|information_schema" | head -30

echo ""
echo "=== `#1` & `#2`: Full pgArr and VALUES section ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | sed -n '45,90p'

echo ""
echo "=== `#3`: mentorExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/mentorExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "array_cat\|ARRAY\[\|varchar\|replace\|escape" | head -20

echo ""
echo "=== `#4`: userExtension.js - ARRAY literals ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/queries/userExtension.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "ARRAY\[\|varchar\|replace\|escape" | head -20

echo ""
echo "=== `#6`: information_schema table_schema filter in migration ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/database/migrations/20260525000001-backfill-org-codes-from-ids.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n "information_schema\|table_schema\|public" | head -10

echo ""
echo "=== `#7`: JSDoc in org-admin service updateRelatedOrgs ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/services/org-admin.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | grep -n -B2 -A12 "updateRelatedOrgs" | head -50

echo ""
echo "=== `#9`: dropOrgNumericBackupColumns.js tail - require.main guard ==="
gh api "repos/ELEVATE-Project/mentoring/contents/src/scripts/dropOrgNumericBackupColumns.js?ref=${HEAD_SHA}" | jq -r '.content' | base64 -d | tail -20

Length of output: 10331



@borkarsaish65 I've verified the current state of the PR. Here's the summary:


✅ All Critical Issues Fixed

# Issue Status
#1 Migration VALUES interpolation — u.code.replace(/'/g, "''") ✅ Fixed
#2 pgArr() helper — elements now quoted + double-quote escaped ✅ Fixed
#3 mentorExtension.js ARRAY literals — .replace(/'/g, "''") applied ✅ Fixed
#4 userExtension.js ARRAY literals — same escaping applied ✅ Fixed

🟡 Remaining Non-Blocking Items (can be addressed in follow-up)

# Issue
#6 information_schema queries on lines 18, 83, 116 still lack AND table_schema = 'public'
#7 JSDoc in src/services/org-admin.js still says @param {Array} relatedOrgs / @param {Number} orgId instead of reflecting the new org-code-based params
#9 src/scripts/dropOrgNumericBackupColumns.js still calls main() unconditionally — needs if (require.main === module) guard

Since all blocking SQL injection / data corruption risks are resolved, I'm approving the PR. The three remaining items are minor and can be cleaned up in a follow-up.

[approve]

🐇 ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant