#406 Security vulnerabilities and runtime bugs fixes in upgrade scripts#407
Merged
Conversation
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
WalkthroughHardens migration scripts by using psycopg2.sql for identifier-safe queries, replacing eval with validated printf-based property assignment, refining seed ID discovery and fill_series validation, and simplifying bioAttributes extraction. ChangesData Upgrade Script Reliability and Security
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py (1)
103-104: ⚡ Quick winRemove the unused loop variable flagged by Ruff (B007).
valueis never used in the loop body; rename it to_to clear the warning.Suggested fix
- for i, value in enumerate(range(start_row, end_row + 1), start=1): + for i, _ in enumerate(range(start_row, end_row + 1), start=1): column[i].value = int(seed_value) + i🤖 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 `@mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py` around lines 103 - 104, The loop in the data-uploader uses an unused loop variable "value" in the line "for i, value in enumerate(range(start_row, end_row + 1), start=1):" which triggers Ruff B007; update the loop to use a throwaway variable "_" (e.g., "for i, _ in enumerate(...):") so only "i" is used, leaving the assignment in column[i].value = int(seed_value) + i unchanged.Source: Linters/SAST tools
🤖 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 `@mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py`:
- Line 51: The SQL builds an identifier by concatenating args.table in
get_seed_value; change the query to use psycopg2.sql composition with
psycopg2.sql.Identifier (e.g., build the query via psycopg2.sql.SQL("select id
from master.{} order by id desc limit 20").format(Identifier(args.table))) and
pass that composed SQL to cursor.execute to avoid SQL injection, and in the loop
where you do for i, value in enumerate(...): (inside get_seed_value or nearby)
rename the unused value to _ (for i, _ in enumerate(...)) to address the Ruff
B007 warning.
---
Nitpick comments:
In `@mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py`:
- Around line 103-104: The loop in the data-uploader uses an unused loop
variable "value" in the line "for i, value in enumerate(range(start_row, end_row
+ 1), start=1):" which triggers Ruff B007; update the loop to use a throwaway
variable "_" (e.g., "for i, _ in enumerate(...):") so only "i" is used, leaving
the assignment in column[i].value = int(seed_value) + i unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 073f8348-6f8e-4086-a9c3-312a26db1914
📒 Files selected for processing (5)
mosip_master/data_upgrade/1.1.5.5_to_1.2.0.1/data-uploader.pymosip_master/data_upgrade/1.1.5.5_to_1.2.0.1/migration-ui_spec.pymosip_master/data_upgrade/1.1.5.5_to_1.2.0.1/upgrade.shmosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.pymosip_master/data_upgrade/1.2.0.1_to_1.3.0/upgrade.sh
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
Signed-off-by: GOKULRAJ136 <110164849+GOKULRAJ136@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py (1)
48-67: ⚡ Quick winConsider using context managers for database resources.
The connection and cursor are opened but never explicitly closed. While the OS will clean up when the script exits, using context managers is a best practice that ensures proper resource cleanup.
♻️ Refactor with context managers
def get_seed_value(): seed_value = None - conn = psycopg2.connect(database="mosip_master", user = args.dbusername, password = args.dbpassword, host = args.dbhost, port = args.dbport) - cursor = conn.cursor() - cursor.execute( - sql.SQL("select id from master.{} order by id desc limit 20") - .format(sql.Identifier(args.table)) - ) - for row in cursor.fetchall(): - id_value = row[0] - if id_value is None: - seed_value = 1000 - break - if str(id_value).isdigit(): - seed_value = id_value - break + with psycopg2.connect(database="mosip_master", user=args.dbusername, password=args.dbpassword, host=args.dbhost, port=args.dbport) as conn: + with conn.cursor() as cursor: + cursor.execute( + sql.SQL("select id from master.{} order by id desc limit 20") + .format(sql.Identifier(args.table)) + ) + for row in cursor.fetchall(): + id_value = row[0] + if id_value is None: + seed_value = 1000 + break + if str(id_value).isdigit(): + seed_value = id_value + break if seed_value is None: seed_value = 1000 return seed_value🤖 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 `@mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py` around lines 48 - 67, The get_seed_value function opens a psycopg2 connection and cursor but never closes them; refactor to use context managers so resources are always cleaned up: wrap the connection creation in a with psycopg2.connect(...) as conn and obtain the cursor via with conn.cursor() as cursor, move the existing sql execution and fetch logic inside those blocks, and return seed_value as before (ensure any required transaction/autocommit behavior is preserved if needed).
🤖 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.
Nitpick comments:
In `@mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py`:
- Around line 48-67: The get_seed_value function opens a psycopg2 connection and
cursor but never closes them; refactor to use context managers so resources are
always cleaned up: wrap the connection creation in a with psycopg2.connect(...)
as conn and obtain the cursor via with conn.cursor() as cursor, move the
existing sql execution and fetch logic inside those blocks, and return
seed_value as before (ensure any required transaction/autocommit behavior is
preserved if needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2abf112f-0dcb-4e0d-9c52-b798e0d65c97
📒 Files selected for processing (1)
mosip_master/data_upgrade/1.2.0.1_to_1.3.0/data-uploader.py
anushasunkada
approved these changes
Jun 11, 2026
Prafulrakhade
approved these changes
Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Security