Security fix: remove hardcoded credentials and improve exception handling#1095
Open
SachinPremkumar wants to merge 1 commit into
Open
Security fix: remove hardcoded credentials and improve exception handling#1095SachinPremkumar wants to merge 1 commit into
SachinPremkumar wants to merge 1 commit into
Conversation
Signed-off-by: Sachin S P <52343650+SachinPremkumar@users.noreply.github.com>
WalkthroughThe PR bumps the Flutter package and README branch reference to 1.1.x, removes the per-app Sonar Gradle plugin and replaces hardcoded build credentials with placeholders, converts all ChangesRelease 1.1.x preparation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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: 4
🤖 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
`@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java`:
- Around line 230-233: The catch blocks in the exception paths at lines 230,
257, 282, 391, and 416 log errors and update job status via onSyncJobComplete
but fail to complete the Pigeon result object, causing the Flutter caller to
hang indefinitely. In each of these exception handlers, after calling
onSyncJobComplete, add a call to resolve the result object (typically
result.reply or similar Pigeon completion method) with an appropriate response
value to ensure the Flutter caller receives a response when an exception occurs.
Apply this pattern consistently to all five affected catch blocks.
In `@android/build.gradle`:
- Around line 26-30: The Sonar configuration properties in the
android/build.gradle file contain hardcoded placeholder credentials including
sonar.token, sonar.projectKey, and sonar.organization. Replace these hardcoded
values with references to environment variables or Gradle properties that can be
provided at build time. For each credential property (sonar.token,
sonar.projectKey, sonar.organization), use System.getenv() or
project.findProperty() to load the actual values from external sources, allowing
CI/CD pipelines to inject real credentials without modifying the Gradle file.
This keeps the build configuration credential-free and prevents hardcoded values
from overriding CI-provided authentication.
- Line 67: The debugPassword variable contains unnecessary escaped quotes that
create literal quote characters in the actual string value used for Room
database encryption. Locate the debugPassword assignment and remove the
backslash escape sequences around the password text, changing from a string with
escaped inner quotes to a plain string value. This ensures the Room database
password contains only the intended password text without the unintended literal
quote characters.
In
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PreRegistrationDataSyncServiceImpl.java`:
- Around line 223-231: The catch block in getPreRegistration() method catches
all Exception types and unconditionally clears the RegistrationDto documents and
demographics, which reintroduces unwanted rollback even for
InterruptedException. Modify the exception handler to check if the caught
exception is caused by or is an InterruptedException before performing the
rollback. If the exception is an InterruptedException, log it without clearing
the registration data, and rethrow or handle it appropriately. Apply this same
fix to all three occurrences mentioned at lines 223-231, 247-250, and 263-266
where this rollback pattern appears.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f09db03a-4d91-481f-8ced-35fa48243aae
📒 Files selected for processing (12)
README.mdandroid/app/build.gradleandroid/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.javaandroid/build.gradleandroid/clientmanager/src/main/assets/biosdk/matchsdk-debug.aarandroid/clientmanager/src/main/assets/biosdk/matchsdk-release.aarandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/entity/UserBiometric.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/LoginService.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PreRegistrationDataSyncServiceImpl.javaandroid/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/UserOnboardService.javaandroid/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/LoginServiceTest.javapubspec.yaml
💤 Files with no reviewable changes (2)
- android/clientmanager/src/test/java/io/mosip/registration/clientmanager/service/LoginServiceTest.java
- android/app/build.gradle
Comment on lines
230
to
233
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| Log.e(TAG, "Policy Key Sync Failed.", e); | ||
| onSyncJobComplete(jobId, false, isManualSync); | ||
| } |
There was a problem hiding this comment.
Complete the Pigeon result in exception paths.
At Line 230, Line 257, Line 282, Line 391, and Line 416 catch blocks, the method logs and updates job status but does not resolve result. This can leave the Flutter caller waiting indefinitely when an exception occurs.
Suggested fix pattern (apply to each affected method)
} catch (Exception e) {
Log.e(TAG, "Policy Key Sync Failed.", e);
onSyncJobComplete(jobId, false, isManualSync);
+ result.error(e);
} } catch (Exception e) {
Log.e(TAG, "Global Params Sync Failed.", e);
onSyncJobComplete(jobId, false, isManualSync);
+ result.error(e);
} } catch (Exception e) {
Log.e(TAG, "User Details Sync Failed.", e);
onSyncJobComplete(jobId, false, isManualSync);
+ result.error(e);
} } catch (Exception e) {
Log.e(TAG, "Pre-Reg IDs Sync Failed.", e);
onSyncJobComplete(jobId, false, false);
+ result.error(e);
} } catch (Exception e) {
Log.e(TAG, "Kernel Certs Sync Failed.", e);
onSyncJobComplete(jobId, false, isManualSync);
+ result.error(e);
}Also applies to: 257-260, 282-285, 391-394, 416-419
🤖 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
`@android/app/src/main/java/io/mosip/registration_client/api_services/MasterDataSyncApi.java`
around lines 230 - 233, The catch blocks in the exception paths at lines 230,
257, 282, 391, and 416 log errors and update job status via onSyncJobComplete
but fail to complete the Pigeon result object, causing the Flutter caller to
hang indefinitely. In each of these exception handlers, after calling
onSyncJobComplete, add a call to resolve the result object (typically
result.reply or similar Pigeon completion method) with an appropriate response
value to ensure the Flutter caller receives a response when an exception occurs.
Apply this pattern consistently to all five affected catch blocks.
Comment on lines
+26
to
30
| property "sonar.projectKey", "your-sonar-project-key" | ||
| property "sonar.organization", "your-sonar-organization" | ||
| property "sonar.host.url", "https://sonarcloud.io" | ||
| property "sonar.login", "13c9437b3fa54b66de5ec14c4fc2bd04fb55ae77" | ||
| property "sonar.token", "your-sonar-token" | ||
| property "sonar.androidLint.reportPaths", "**/reports/lint-results-debug.xml" |
There was a problem hiding this comment.
Do not hardcode placeholder Sonar credentials in executable Gradle config.
Line 29 sets a literal token value ("your-sonar-token"), which can override CI-provided auth and fail analysis. Load credentials from env/Gradle properties and keep this file credential-free.
Suggested fix
- property "sonar.projectKey", "your-sonar-project-key"
- property "sonar.organization", "your-sonar-organization"
+ property "sonar.projectKey", System.getenv("SONAR_PROJECT_KEY") ?: ""
+ property "sonar.organization", System.getenv("SONAR_ORGANIZATION") ?: ""
property "sonar.host.url", "https://sonarcloud.io"
- property "sonar.token", "your-sonar-token"
+ property "sonar.token", System.getenv("SONAR_TOKEN") ?: ""📝 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
| property "sonar.projectKey", "your-sonar-project-key" | |
| property "sonar.organization", "your-sonar-organization" | |
| property "sonar.host.url", "https://sonarcloud.io" | |
| property "sonar.login", "13c9437b3fa54b66de5ec14c4fc2bd04fb55ae77" | |
| property "sonar.token", "your-sonar-token" | |
| property "sonar.androidLint.reportPaths", "**/reports/lint-results-debug.xml" | |
| property "sonar.projectKey", System.getenv("SONAR_PROJECT_KEY") ?: "" | |
| property "sonar.organization", System.getenv("SONAR_ORGANIZATION") ?: "" | |
| property "sonar.host.url", "https://sonarcloud.io" | |
| property "sonar.token", System.getenv("SONAR_TOKEN") ?: "" | |
| property "sonar.androidLint.reportPaths", "**/reports/lint-results-debug.xml" |
🤖 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 `@android/build.gradle` around lines 26 - 30, The Sonar configuration
properties in the android/build.gradle file contain hardcoded placeholder
credentials including sonar.token, sonar.projectKey, and sonar.organization.
Replace these hardcoded values with references to environment variables or
Gradle properties that can be provided at build time. For each credential
property (sonar.token, sonar.projectKey, sonar.organization), use
System.getenv() or project.findProperty() to load the actual values from
external sources, allowing CI/CD pipelines to inject real credentials without
modifying the Gradle file. This keeps the build configuration credential-free
and prevents hardcoded values from overriding CI-provided authentication.
| serverHealthCheckPath = "\"/v1/syncdata/actuator/health\"" | ||
| serverActuatorInfoPath = "\"/v1/syncdata/actuator/info\"" | ||
| debugPassword = "\"APTyKejHxACQyKBSRciR\"" | ||
| debugPassword = "\"your-debug-password\"" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where debugPassword is declared and consumed
rg -n -C3 '\bdebugPassword\b'
# Show signing-related usage context in Gradle files
fd -i 'build.gradle' -t f | xargs -I{} sh -c 'echo "---- {} ----"; rg -n -C2 "storePassword|keyPassword|debugPassword" "{}"'Repository: mosip/android-registration-client
Length of output: 2583
🏁 Script executed:
#!/bin/bash
# Search for BuildConfig.DEBUG_PASSWORD usage
rg -n 'DEBUG_PASSWORD|debugPassword' --type java --type kotlin
# Also search in all text files to understand context
rg -n 'DEBUG_PASSWORD' -A3 -B3Repository: mosip/android-registration-client
Length of output: 4408
🏁 Script executed:
#!/bin/bash
# Check if debugPassword is involved in any keystore or signing operations
rg -n -i 'keystore|signing|keystorefil' --type gradle | head -30Repository: mosip/android-registration-client
Length of output: 729
🏁 Script executed:
#!/bin/bash
# Check RoomModule.java to see how dbPwd is used with the database
cat -n android/clientmanager/src/main/java/io/mosip/registration/clientmanager/config/RoomModule.java | head -120 | tail -30Repository: mosip/android-registration-client
Length of output: 1949
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about password format in the codebase
rg -n 'password.*quote|quote.*password|debug.*password' -iRepository: mosip/android-registration-client
Length of output: 1266
Remove the unnecessary escaped quotes from debugPassword.
The escaped quotes in "\"your-debug-password\"" create a string containing literal quote characters. This value is used as the Room database password in debug mode (RoomModule.java:104), so the effective password becomes "your-debug-password" including the quotes. Change this to debugPassword = "your-debug-password" to remove the unintended quotes.
Note: This affects only debug database encryption, not signing credentials (which use storePassword/keyPassword from key.properties).
🤖 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 `@android/build.gradle` at line 67, The debugPassword variable contains
unnecessary escaped quotes that create literal quote characters in the actual
string value used for Room database encryption. Locate the debugPassword
assignment and remove the backslash escape sequences around the password text,
changing from a string with escaped inner quotes to a plain string value. This
ensures the Room database password contains only the intended password text
without the unintended literal quote characters.
Comment on lines
223
to
+231
| } catch (Exception e) { | ||
| Log.e(TAG,ERROR_FETCH_PRE_REG_PACKET, e); | ||
| try { | ||
| RegistrationDto registrationDto = this.registrationService.getRegistrationDto(); | ||
| registrationDto.getDocuments().clear(); | ||
| registrationDto.getDemographics().clear(); | ||
| } catch (Exception inner) { | ||
| Log.e(TAG, "No active registration to roll back", inner); | ||
| } |
There was a problem hiding this comment.
Interrupted fetch still triggers rollback in caller.
fetchPreRegistration(...) now avoids rollback for InterruptedException, but getPreRegistration(...) catches that wrapped runtime error in catch (Exception e) and still clears active RegistrationDto. This reintroduces unintended rollback and can wipe in-progress data during cancellation/interruption.
Proposed fix
- } catch (Exception e) {
- Log.e(TAG,ERROR_FETCH_PRE_REG_PACKET, e);
- try {
+ } catch (Exception e) {
+ Log.e(TAG, ERROR_FETCH_PRE_REG_PACKET, e);
+ if (Thread.currentThread().isInterrupted() ||
+ (e instanceof RuntimeException && e.getCause() instanceof InterruptedException)) {
+ Thread.currentThread().interrupt();
+ return attributeData;
+ }
+ try {
RegistrationDto registrationDto = this.registrationService.getRegistrationDto();
registrationDto.getDocuments().clear();
registrationDto.getDemographics().clear();
} catch (Exception inner) {
Log.e(TAG, "No active registration to roll back", inner);
}
}Also applies to: 247-250, 263-266
🤖 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
`@android/clientmanager/src/main/java/io/mosip/registration/clientmanager/service/PreRegistrationDataSyncServiceImpl.java`
around lines 223 - 231, The catch block in getPreRegistration() method catches
all Exception types and unconditionally clears the RegistrationDto documents and
demographics, which reintroduces unwanted rollback even for
InterruptedException. Modify the exception handler to check if the caught
exception is caused by or is an InterruptedException before performing the
rollback. If the exception is an InterruptedException, log it without clearing
the registration data, and rethrow or handle it appropriately. Apply this same
fix to all three occurrences mentioned at lines 223-231, 247-250, and 263-266
where this rollback pattern appears.
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
1 participant
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
Documentation
Chores
Bug Fixes