Migrate device reports to Android Keystore-backed key#3567
Migrate device reports to Android Keystore-backed key#3567avazirna wants to merge 3 commits intovaz/encrypt-device-reports-with-android-keystorefrom
Conversation
📝 WalkthroughWalkthroughThis pull request migrates the CommCare encryption key provisioning strategy from per-record symmetric keys to session-level keystore-backed encryption. The core API changes refactor EncryptionIO methods to accept generic Key objects, transformation strings, and an Android Keystore flag instead of SecretKeySpec. A new SessionManager provides singleton-like access to session-level encryption keys. A database schema upgrade (v29→v30) includes migration logic that converts existing DeviceReportRecord entries by rekeying device report files from per-record AES keys to the session-based keystore key. Multiple call sites across the codebase are updated to supply the new parameters when invoking encryption/decryption methods. Sequence Diagram(s)sequenceDiagram
participant UserDatabaseUpgrader
participant DeviceReportRecordPreV30
participant SessionManager
participant EncryptionIO
participant FileSystem
UserDatabaseUpgrader->>DeviceReportRecordPreV30: Read old records from storage
DeviceReportRecordPreV30-->>UserDatabaseUpgrader: Return fileName + aesKey
loop For each old record
UserDatabaseUpgrader->>FileSystem: Read encrypted device report file
FileSystem-->>UserDatabaseUpgrader: Encrypted bytes (with old AES key)
UserDatabaseUpgrader->>EncryptionIO: Decrypt with old aesKey
EncryptionIO-->>UserDatabaseUpgrader: Decrypted bytes
UserDatabaseUpgrader->>SessionManager: Get session encryption key
SessionManager-->>UserDatabaseUpgrader: Key + transformation from keystore
UserDatabaseUpgrader->>EncryptionIO: Re-encrypt with session key
EncryptionIO->>FileSystem: Write re-encrypted file
FileSystem-->>EncryptionIO: File written
UserDatabaseUpgrader->>UserDatabaseUpgrader: Create new DeviceReportRecord
end
UserDatabaseUpgrader->>FileSystem: Persist updated DeviceReportRecord entries
FileSystem-->>UserDatabaseUpgrader: Schema v30 committed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/utils/FormUploadUtil.java (1)
73-90:⚠️ Potential issue | 🟠 Major
getDecryptCiphershould fail fast, not returnnull.Returning
nullhere pushes crypto initialization failures into downstreamNullPointerExceptions. Throw a runtime exception at the source instead.💡 Proposed fix
public static Cipher getDecryptCipher(Key key, String transformation, byte[] iv) { Cipher cipher; try { cipher = Cipher.getInstance(Objects.requireNonNullElse(transformation, "AES")); if (iv != null) { cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(iv)); } else { cipher.init(Cipher.DECRYPT_MODE, key); } return cipher; - //TODO: Something smart here; } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidKeyException | InvalidAlgorithmParameterException e) { - e.printStackTrace(); + throw new IllegalStateException("Failed to initialize decrypt cipher", e); } - return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/utils/FormUploadUtil.java` around lines 73 - 90, The getDecryptCipher method currently catches crypto exceptions and returns null; change it to fail fast by rethrowing a runtime exception instead of returning null: inside getDecryptCipher (the method handling Cipher.getInstance and cipher.init) catch the NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, and InvalidAlgorithmParameterException and throw a new RuntimeException (or IllegalStateException) that wraps the caught exception and includes a clear message (e.g., "Failed to initialize decrypt cipher"), so callers never receive null and the original exception is preserved for debugging.
🧹 Nitpick comments (1)
app/src/org/commcare/services/SessionManager.kt (1)
12-25: Prefer a Kotlinobjectdeclaration over aclasswith only a companion object.
SessionManagerhas no instance state and no instance methods — the entire public API lives in the companion object. Idiomatic Kotlin uses a top-levelobjectfor this pattern:♻️ Proposed refactor
-@RequiresApi(Build.VERSION_CODES.M) -class SessionManager { - companion object { - private const val SESSION_KEY_ALIAS = "session_encryption_key" - - private val sessionKeyAndTransformation by lazy { - AesKeyStoreHandler(SESSION_KEY_ALIAS, needsUserAuth = false).getKeyOrGenerate() - } - - `@JvmStatic` - fun getEncryptionKey(): Key = sessionKeyAndTransformation.key - - `@JvmStatic` - fun getKeyTransformation(): String = sessionKeyAndTransformation.transformation - } -} +@RequiresApi(Build.VERSION_CODES.M) +object SessionManager { + private const val SESSION_KEY_ALIAS = "session_encryption_key" + + private val sessionKeyAndTransformation by lazy { + AesKeyStoreHandler(SESSION_KEY_ALIAS, needsUserAuth = false).getKeyOrGenerate() + } + + `@JvmStatic` + fun getEncryptionKey(): Key = sessionKeyAndTransformation.key + + `@JvmStatic` + fun getKeyTransformation(): String = sessionKeyAndTransformation.transformation +}
@JvmStaticcontinues to work onobjectmembers, so Java call sites are unaffected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/services/SessionManager.kt` around lines 12 - 25, SessionManager is implemented as a class with only a companion object; refactor it to a Kotlin object to be idiomatic and remove the unused outer class: replace the class declaration with an object declaration named SessionManager and move the existing companion contents (SESSION_KEY_ALIAS, sessionKeyAndTransformation, getEncryptionKey, getKeyTransformation) into the object body, preserving the `@JvmStatic` annotations and existing symbols (AesKeyStoreHandler, SESSION_KEY_ALIAS, sessionKeyAndTransformation, getEncryptionKey, getKeyTransformation) so Java callers keep the same API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/org/commcare/activities/components/ImageCaptureProcessing.java`:
- Line 70: The call to EncryptionIO.encryptFile in ImageCaptureProcessing
(currently: EncryptionIO.encryptFile(sourcePath, finalFilePath,
formEntryActivity.getSymetricKey(), null, false);) exceeds the 115-char
checkstyle limit — split the argument list across multiple lines or assign long
expressions to temporaries (e.g., extract formEntryActivity.getSymetricKey() to
a variable) so the call is wrapped to conform to checkstyle; keep the same
arguments (sourcePath, finalFilePath, the symmetric key, null, false) and
preserve the legacy encryption behavior.
In `@app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java`:
- Around line 844-849: In UserDatabaseUpgrader, the loop that migrates
DeviceReportRecordPreV30 currently calls
rekeyDeviceReportFileToKeystoreKey(old.getFilePath(), old.getAesKey()) but
always writes the new DeviceReportRecord (newStorage.write(updated)) even if
rekeying failed; change this so that a failed rekey prevents writing: make
rekeyDeviceReportFileToKeystoreKey either throw on failure (or return a boolean)
and in the migration loop around DeviceReportRecordPreV30 catch that failure and
do NOT call newStorage.write(updated) — instead propagate the error (rethrow) or
otherwise abort the upgrade so the DB row is not written for an unreadable file;
apply the same pattern to the other migration block(s) around the same logic
(lines referenced in the review: 857-881).
- Around line 874-879: In UserDatabaseUpgrader (around the block handling
tempPath/filename), check the boolean return values of fileToDelete.delete() and
new File(tempPath).renameTo(new File(filename)); if delete() or renameTo()
returns false, throw an appropriate IOException (or
StreamsUtil.OutputIOException) with a clear message including the
filename/tempPath so callers can handle the failure; ensure you only consider
the operation successful after renameTo() succeeds and mirror the error-handling
style used in UserSandboxUtils/GlobalDatabaseUpgrader.
In `@app/src/org/commcare/models/encryption/EncryptionIO.java`:
- Around line 109-113: When reading the IV in the keystore decrypt path (the
isKeyFromAndroidKeyStore branch), preserve the unsigned byte interpretation
(keep the "& 0xFF" mask when assigning ivLength) and replace the single
is.read(iv, 0, ivLength) call with a loop that repeatedly reads into iv until
ivLength bytes have been filled or EOF occurs; if EOF is reached before the
buffer is full, throw/propagate an IOException so the partially-initialized iv
is never used. Ensure the variables referenced are ivLength, iv, is, and the
isKeyFromAndroidKeyStore branch so the change is localized to that parsing
logic.
In `@app/src/org/commcare/models/FormRecordProcessor.java`:
- Around line 73-75: The call to FormUploadUtil.getDecryptCipher(...) can return
null and feeding it directly into new CipherInputStream(...) leads to a silent
NPE; update each call site in FormRecordProcessor (the process(...) method
around the decrypter creation, performLinearFileScan, and attemptXmlScan) to
check the returned Cipher for null after calling
FormUploadUtil.getDecryptCipher(new SecretKeySpec(record.getAesKey(), "AES"),
...) and handle it explicitly—either throw a clear
IOException/IllegalStateException with context (including record identifier and
the underlying exception message if available) or log an error and abort
processing—so you never pass a null Cipher into CipherInputStream and the
failure surface includes useful diagnostic info.
In `@app/src/org/commcare/network/EncryptedFileBody.java`:
- Around line 55-58: The IV-read code in EncryptedFileBody is not validating
reads and can produce a corrupted IV; update the block that reads from fis (the
read of ivLength and the fis.read(iv, 0, ivLength) into iv) to: check the
initial fis.read() return for -1 and throw an IOException on EOF, validate
ivLength is within expected bounds (e.g., >0 and <= a reasonable max like 256),
allocate iv accordingly, then loop calling fis.read(...) until you have received
exactly ivLength bytes (accumulating bytesRead) and throw an IOException if a
read returns -1 before filling the buffer—this ensures the IV is fully and
safely read before decryption.
In `@app/src/org/commcare/services/SessionManager.kt`:
- Around line 16-18: sessionKeyAndTransformation currently uses Kotlin's default
by lazy which caches thrown exceptions from
AesKeyStoreHandler(SESSION_KEY_ALIAS, needsUserAuth = false).getKeyOrGenerate(),
making keystore-init failures permanent; change to avoid caching exceptions by
wrapping initialization in a safe accessor: replace the lazy usage with a
non-caching approach (e.g., lazy(LazyThreadSafetyMode.NONE) combined with
explicit retry/timeout or use runCatching inside a short-lived initialization
function) and ensure callers getEncryptionKey() and getKeyTransformation()
handle retryable failures and surface clear errors or fallbacks instead of
letting a cached Throwable be rethrown for the process lifetime.
- Around line 11-12: SessionManager is annotated `@RequiresApi`(M) only for
Kotlin, so Java callers can still call getEncryptionKey()/getKeyTransformation()
on older SDKs and crash; update each Java call site
(LogSubmissionTask.doInBackground,
UserDatabaseUpgrader.rekeyDeviceReportFileToKeystoreKey, and
DeviceReportRecord.openOutputStream) to either add
`@RequiresApi`(Build.VERSION_CODES.M) to the method declaration or wrap the calls
in an explicit runtime guard if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M)
before invoking SessionManager.getEncryptionKey() or
SessionManager.getKeyTransformation(); pick the approach consistent with each
method's API contract (annotation if the whole method requires M, runtime check
if only a code path needs guarding).
---
Outside diff comments:
In `@app/src/org/commcare/utils/FormUploadUtil.java`:
- Around line 73-90: The getDecryptCipher method currently catches crypto
exceptions and returns null; change it to fail fast by rethrowing a runtime
exception instead of returning null: inside getDecryptCipher (the method
handling Cipher.getInstance and cipher.init) catch the NoSuchAlgorithmException,
NoSuchPaddingException, InvalidKeyException, and
InvalidAlgorithmParameterException and throw a new RuntimeException (or
IllegalStateException) that wraps the caught exception and includes a clear
message (e.g., "Failed to initialize decrypt cipher"), so callers never receive
null and the original exception is preserved for debugging.
---
Nitpick comments:
In `@app/src/org/commcare/services/SessionManager.kt`:
- Around line 12-25: SessionManager is implemented as a class with only a
companion object; refactor it to a Kotlin object to be idiomatic and remove the
unused outer class: replace the class declaration with an object declaration
named SessionManager and move the existing companion contents
(SESSION_KEY_ALIAS, sessionKeyAndTransformation, getEncryptionKey,
getKeyTransformation) into the object body, preserving the `@JvmStatic`
annotations and existing symbols (AesKeyStoreHandler, SESSION_KEY_ALIAS,
sessionKeyAndTransformation, getEncryptionKey, getKeyTransformation) so Java
callers keep the same API.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/src/org/commcare/activities/components/ImageCaptureProcessing.javaapp/src/org/commcare/android/javarosa/DeviceReportRecord.javaapp/src/org/commcare/android/javarosa/DeviceReportRecordPreV30.javaapp/src/org/commcare/models/FormRecordProcessor.javaapp/src/org/commcare/models/database/HybridFileBackedSqlStorage.javaapp/src/org/commcare/models/database/user/UserDatabaseSchemaManager.javaapp/src/org/commcare/models/database/user/UserDatabaseUpgrader.javaapp/src/org/commcare/models/encryption/EncryptionIO.javaapp/src/org/commcare/network/EncryptedFileBody.javaapp/src/org/commcare/services/SessionManager.ktapp/src/org/commcare/tasks/DumpTask.javaapp/src/org/commcare/tasks/FormLoaderTask.javaapp/src/org/commcare/tasks/FormRecordToFileTask.javaapp/src/org/commcare/tasks/LogSubmissionTask.javaapp/src/org/commcare/tasks/SaveToDiskTask.javaapp/src/org/commcare/utils/FormUploadUtil.javaapp/src/org/commcare/views/widgets/MediaWidget.java
| finalFilePath = finalFilePath + MediaWidget.AES_EXTENSION; | ||
| try { | ||
| EncryptionIO.encryptFile(sourcePath, finalFilePath, formEntryActivity.getSymetricKey()); | ||
| EncryptionIO.encryptFile(sourcePath, finalFilePath, formEntryActivity.getSymetricKey(), null, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the EncryptionIO class
find . -name "EncryptionIO.java" -type fRepository: dimagi/commcare-android
Length of output: 125
🏁 Script executed:
# Also check the ImageCaptureProcessing.java file structure around line 70
find . -name "ImageCaptureProcessing.java" -type fRepository: dimagi/commcare-android
Length of output: 139
🏁 Script executed:
# Search for checkstyle.xml in the repository
find . -name "checkstyle.xml" -type fRepository: dimagi/commcare-android
Length of output: 99
🏁 Script executed:
# Read EncryptionIO.java to find the encryptFile method signature
wc -l ./app/src/org/commcare/models/encryption/EncryptionIO.javaRepository: dimagi/commcare-android
Length of output: 129
🏁 Script executed:
# Read ImageCaptureProcessing.java context around line 70
wc -l ./app/src/org/commcare/activities/components/ImageCaptureProcessing.javaRepository: dimagi/commcare-android
Length of output: 143
🏁 Script executed:
# Get EncryptionIO.java encryptFile method
cat -n ./app/src/org/commcare/models/encryption/EncryptionIO.java | head -150 | tail -100Repository: dimagi/commcare-android
Length of output: 5584
🏁 Script executed:
# Read complete EncryptionIO.java to see the full encryptFile signature
cat -n ./app/src/org/commcare/models/encryption/EncryptionIO.java | head -60Repository: dimagi/commcare-android
Length of output: 2611
🏁 Script executed:
# Read ImageCaptureProcessing.java around line 70
cat -n ./app/src/org/commcare/activities/components/ImageCaptureProcessing.java | sed -n '60,85p'Repository: dimagi/commcare-android
Length of output: 1294
🏁 Script executed:
# Read checkstyle.xml to verify applicable rules for Java style
cat ./.github/linters/checkstyle.xml | head -60Repository: dimagi/commcare-android
Length of output: 2844
🏁 Script executed:
# Get the exact line 70 and measure its length
sed -n '70p' ./app/src/org/commcare/activities/components/ImageCaptureProcessing.java | wc -cRepository: dimagi/commcare-android
Length of output: 70
🏁 Script executed:
# Search for getSymetricKey() method definition to verify null-safety
rg -n "getSymetricKey" --type java -B 2 -A 5Repository: dimagi/commcare-android
Length of output: 3909
🏁 Script executed:
# Check line 70 context and verify it doesn't violate other style rules
sed -n '70p' ./app/src/org/commcare/activities/components/ImageCaptureProcessing.javaRepository: dimagi/commcare-android
Length of output: 184
🏁 Script executed:
# Verify the exact parameter name in createFileOutputStream call at line 49
sed -n '49p' ./app/src/org/commcare/models/encryption/EncryptionIO.javaRepository: dimagi/commcare-android
Length of output: 162
🏁 Script executed:
# Check if symetricKey field can be null and if there are guards at call site
rg -n "symetricKey\s*=" --type java -B 2 -A 2 | head -40Repository: dimagi/commcare-android
Length of output: 2247
Line length exceeds checkstyle limit — requires wrapping.
This call is 118 characters and violates the checkstyle.xml limit of 115. Split the line to conform. For example:
Suggested fix
- EncryptionIO.encryptFile(sourcePath, finalFilePath, formEntryActivity.getSymetricKey(), null, false);
+ EncryptionIO.encryptFile(sourcePath, finalFilePath,
+ formEntryActivity.getSymetricKey(), null, false);The API adaptation itself is correct: passing null for transformation and false for isKeyFromKeystore safely uses the legacy encryption path (defaults to AES cipher, no keystore IV handling), consistent with the PR's stated backward-compatibility contract. Null handling is safe—createFileOutputStream checks for null keys and falls back to unencrypted output stream.
📝 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.
| EncryptionIO.encryptFile(sourcePath, finalFilePath, formEntryActivity.getSymetricKey(), null, false); | |
| EncryptionIO.encryptFile(sourcePath, finalFilePath, | |
| formEntryActivity.getSymetricKey(), null, false); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/activities/components/ImageCaptureProcessing.java` at
line 70, The call to EncryptionIO.encryptFile in ImageCaptureProcessing
(currently: EncryptionIO.encryptFile(sourcePath, finalFilePath,
formEntryActivity.getSymetricKey(), null, false);) exceeds the 115-char
checkstyle limit — split the argument list across multiple lines or assign long
expressions to temporaries (e.g., extract formEntryActivity.getSymetricKey() to
a variable) so the call is wrapped to conform to checkstyle; keep the same
arguments (sourcePath, finalFilePath, the symmetric key, null, false) and
preserve the legacy encryption behavior.
| for (DeviceReportRecordPreV30 old : oldStorage) { | ||
| DeviceReportRecord updated = new DeviceReportRecord(old.getFilePath()); | ||
| rekeyDeviceReportFileToKeystoreKey(old.getFilePath(), old.getAesKey()); | ||
| updated.setID(old.getID()); | ||
| newStorage.write(updated); | ||
| } |
There was a problem hiding this comment.
Do not write migrated records when file rekeying fails.
Right now rekey failures are logged but not propagated, and the upgraded record is still written. That can leave DB rows on the new schema while the file remains in an unreadable/old state.
💡 Proposed fix
for (DeviceReportRecordPreV30 old : oldStorage) {
DeviceReportRecord updated = new DeviceReportRecord(old.getFilePath());
- rekeyDeviceReportFileToKeystoreKey(old.getFilePath(), old.getAesKey());
+ if (!rekeyDeviceReportFileToKeystoreKey(old.getFilePath(), old.getAesKey())) {
+ throw new IllegalStateException("Failed to rekey device report file: " + old.getFilePath());
+ }
updated.setID(old.getID());
newStorage.write(updated);
}
...
-private void rekeyDeviceReportFileToKeystoreKey(String filename, byte[] oldAesKey) {
+private boolean rekeyDeviceReportFileToKeystoreKey(String filename, byte[] oldAesKey) {
try {
InputStream is = EncryptionIO.getFileInputStream(
filename,
new SecretKeySpec(oldAesKey, "AES"),
null,
false
);
String tempPath = filename + ".tmp";
StreamsUtil.writeFromInputToOutputNew(is,
createFileOutputStream(
tempPath,
SessionManager.getEncryptionKey(),
SessionManager.getKeyTransformation(),
true
)
);
- File fileToDelete = new File(filename);
- if (fileToDelete.exists()) {
- fileToDelete.delete();
- }
- new File(tempPath).renameTo(new File(filename));
+ File fileToDelete = new File(filename);
+ File destination = new File(filename);
+ if (fileToDelete.exists() && !fileToDelete.delete()) {
+ throw new java.io.IOException("Unable to delete original file: " + filename);
+ }
+ if (!new File(tempPath).renameTo(destination)) {
+ throw new java.io.IOException("Unable to promote temp rekeyed file: " + filename);
+ }
+ return true;
} catch (FileNotFoundException | StreamsUtil.InputIOException | StreamsUtil.OutputIOException e) {
Logger.exception("Error while rekeying device report file during user db migration", e);
+ return false;
+ } catch (java.io.IOException e) {
+ Logger.exception("Error while replacing rekeyed device report file during user db migration", e);
+ return false;
}
}Also applies to: 857-881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java` around
lines 844 - 849, In UserDatabaseUpgrader, the loop that migrates
DeviceReportRecordPreV30 currently calls
rekeyDeviceReportFileToKeystoreKey(old.getFilePath(), old.getAesKey()) but
always writes the new DeviceReportRecord (newStorage.write(updated)) even if
rekeying failed; change this so that a failed rekey prevents writing: make
rekeyDeviceReportFileToKeystoreKey either throw on failure (or return a boolean)
and in the migration loop around DeviceReportRecordPreV30 catch that failure and
do NOT call newStorage.write(updated) — instead propagate the error (rethrow) or
otherwise abort the upgrade so the DB row is not written for an unreadable file;
apply the same pattern to the other migration block(s) around the same logic
(lines referenced in the review: 857-881).
| File fileToDelete = new File(filename); | ||
| if (fileToDelete.exists()) { | ||
| fileToDelete.delete(); | ||
| } | ||
| new File(tempPath).renameTo(new File(filename)); | ||
| } catch (FileNotFoundException | StreamsUtil.InputIOException | StreamsUtil.OutputIOException e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file around lines 874-879
wc -l app/src/org/commcare/models/database/user/UserDatabaseUpgrader.javaRepository: dimagi/commcare-android
Length of output: 138
🏁 Script executed:
# Read the code with surrounding context (lines 860-890)
sed -n '860,890p' app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java | cat -nRepository: dimagi/commcare-android
Length of output: 1723
🏁 Script executed:
# Check for checkstyle.xml rules
find . -name "checkstyle.xml" -type fRepository: dimagi/commcare-android
Length of output: 99
🏁 Script executed:
# Read checkstyle.xml to find relevant rules
cat ./.github/linters/checkstyle.xmlRepository: dimagi/commcare-android
Length of output: 19447
🏁 Script executed:
# Search for other patterns of file operations in the codebase to see best practices
rg "\.delete\(\)" app/src --A 2 -B 2 | head -80Repository: dimagi/commcare-android
Length of output: 472
🏁 Script executed:
# Search for renameTo patterns
rg "\.renameTo\(" app/src --A 2 -B 2 | head -60Repository: dimagi/commcare-android
Length of output: 472
🏁 Script executed:
# Search for proper File.delete() patterns
rg 'delete\(\)' app/src -B 2 -A 2 | head -100Repository: dimagi/commcare-android
Length of output: 7767
🏁 Script executed:
# Search for proper File.renameTo() patterns
rg 'renameTo\(' app/src -B 2 -A 2 | head -100Repository: dimagi/commcare-android
Length of output: 3137
🏁 Script executed:
# Get more context around the problematic lines - read lines 860-895
sed -n '860,895p' app/src/org/commcare/models/database/user/UserDatabaseUpgrader.javaRepository: dimagi/commcare-android
Length of output: 1741
Check return values of delete() and renameTo() to prevent data loss.
The return values from delete() (line 874-876) and renameTo() (line 879) are ignored. If the delete succeeds but the rename fails, the original file is lost. Verify both operations succeed before proceeding, or throw an exception on failure—as done elsewhere in the codebase (e.g., UserSandboxUtils.java, GlobalDatabaseUpgrader.java).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java` around
lines 874 - 879, In UserDatabaseUpgrader (around the block handling
tempPath/filename), check the boolean return values of fileToDelete.delete() and
new File(tempPath).renameTo(new File(filename)); if delete() or renameTo()
returns false, throw an appropriate IOException (or
StreamsUtil.OutputIOException) with a clear message including the
filename/tempPath so callers can handle the failure; ensure you only consider
the operation successful after renameTo() succeeds and mirror the error-handling
style used in UserSandboxUtils/GlobalDatabaseUpgrader.
| if (isKeyFromAndroidKeyStore) { | ||
| int ivLength = is.read() & 0xFF; | ||
| iv = new byte[ivLength]; | ||
| is.read(iv, 0, ivLength); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "EncryptionIO.java" | head -5Repository: dimagi/commcare-android
Length of output: 125
🏁 Script executed:
find . -type f -name "checkstyle.xml" | head -5Repository: dimagi/commcare-android
Length of output: 99
🏁 Script executed:
if [ -f "app/src/org/commcare/models/encryption/EncryptionIO.java" ]; then
wc -l "app/src/org/commcare/models/encryption/EncryptionIO.java"
fiRepository: dimagi/commcare-android
Length of output: 127
🏁 Script executed:
cat -n app/src/org/commcare/models/encryption/EncryptionIO.java | sed -n '100,125p'Repository: dimagi/commcare-android
Length of output: 1559
🏁 Script executed:
cat -n ./.github/linters/checkstyle.xmlRepository: dimagi/commcare-android
Length of output: 22009
Add robust short-read handling to IV parsing in keystore decrypt path.
The current implementation assumes is.read(iv, 0, ivLength) reads all requested bytes in a single call. InputStream.read() may return fewer bytes without throwing an exception, leaving the IV partially initialized. Add a loop to read the full IV buffer.
However, the proposed fix removes the & 0xFF mask from the length byte, which changes EOF handling: a legitimate 0x00 byte length (though cryptographically invalid) would now be rejected. Consider either preserving & 0xFF or explicitly documenting that 0-length IVs are intentionally rejected.
💡 Revised approach
Preserve the & 0xFF mask and add the partial-read loop:
if (isKeyFromAndroidKeyStore) {
int ivLength = is.read() & 0xFF;
iv = new byte[ivLength];
+ int offset = 0;
+ while (offset < ivLength) {
+ int read = is.read(iv, offset, ivLength - offset);
+ if (read < 0) {
+ throw new IOException("Unexpected EOF while reading IV");
+ }
+ offset += read;
+ }
- is.read(iv, 0, ivLength);
}📝 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 (isKeyFromAndroidKeyStore) { | |
| int ivLength = is.read() & 0xFF; | |
| iv = new byte[ivLength]; | |
| is.read(iv, 0, ivLength); | |
| } | |
| if (isKeyFromAndroidKeyStore) { | |
| int ivLength = is.read() & 0xFF; | |
| iv = new byte[ivLength]; | |
| int offset = 0; | |
| while (offset < ivLength) { | |
| int read = is.read(iv, offset, ivLength - offset); | |
| if (read < 0) { | |
| throw new IOException("Unexpected EOF while reading IV"); | |
| } | |
| offset += read; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/models/encryption/EncryptionIO.java` around lines 109 -
113, When reading the IV in the keystore decrypt path (the
isKeyFromAndroidKeyStore branch), preserve the unsigned byte interpretation
(keep the "& 0xFF" mask when assigning ivLength) and replace the single
is.read(iv, 0, ivLength) call with a loop that repeatedly reads into iv until
ivLength bytes have been filled or EOF occurs; if EOF is reached before the
buffer is full, throw/propagate an IOException so the partially-initialized iv
is never used. Ensure the variables referenced are ivLength, iv, is, and the
isKeyFromAndroidKeyStore branch so the change is localized to that parsing
logic.
| final Cipher decrypter = | ||
| FormUploadUtil.getDecryptCipher((new SecretKeySpec(record.getAesKey(), "AES"))); | ||
| FormUploadUtil.getDecryptCipher((new SecretKeySpec(record.getAesKey(), "AES")), null, null); | ||
| InputStream is = new CipherInputStream(new FileInputStream(f), decrypter); |
There was a problem hiding this comment.
getDecryptCipher can return null, causing a silent NPE in CipherInputStream.
FormUploadUtil.getDecryptCipher swallows NoSuchAlgorithmException, InvalidKeyException, and related crypto exceptions and returns null. All three call sites pass the returned value directly into new CipherInputStream(...) without a null check. A null cipher causes CipherInputStream to throw NPE on first read, with no contextual error message.
While this null-return pattern predates this PR, these lines are being touched. Consider at least adding a null guard:
🛡️ Proposed null guard (example for process() at line 73)
- final Cipher decrypter =
- FormUploadUtil.getDecryptCipher((new SecretKeySpec(record.getAesKey(), "AES")), null, null);
+ final Cipher decrypter =
+ FormUploadUtil.getDecryptCipher((new SecretKeySpec(record.getAesKey(), "AES")), null, null);
+ if (decrypter == null) {
+ throw new IOException("Failed to initialize decryption cipher for form record " + record.getInstanceID());
+ }The same guard is needed in performLinearFileScan (Line 227) and attemptXmlScan (Line 262).
Also applies to: 227-232, 262-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/models/FormRecordProcessor.java` around lines 73 - 75,
The call to FormUploadUtil.getDecryptCipher(...) can return null and feeding it
directly into new CipherInputStream(...) leads to a silent NPE; update each call
site in FormRecordProcessor (the process(...) method around the decrypter
creation, performLinearFileScan, and attemptXmlScan) to check the returned
Cipher for null after calling FormUploadUtil.getDecryptCipher(new
SecretKeySpec(record.getAesKey(), "AES"), ...) and handle it explicitly—either
throw a clear IOException/IllegalStateException with context (including record
identifier and the underlying exception message if available) or log an error
and abort processing—so you never pass a null Cipher into CipherInputStream and
the failure surface includes useful diagnostic info.
| int ivLength = fis.read() & 0xFF; | ||
| iv = new byte[ivLength]; | ||
| fis.read(iv, 0, ivLength); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "EncryptedFileBody.java" -type fRepository: dimagi/commcare-android
Length of output: 120
🏁 Script executed:
cat -n ./app/src/org/commcare/network/EncryptedFileBody.java | head -70Repository: dimagi/commcare-android
Length of output: 3059
🏁 Script executed:
find . -name "checkstyle.xml" -type fRepository: dimagi/commcare-android
Length of output: 99
🏁 Script executed:
cat ./.github/linters/checkstyle.xmlRepository: dimagi/commcare-android
Length of output: 19447
Validate and fully read the IV header before decrypting.
The IV read path does not handle incomplete reads or EOF. FileInputStream.read() can return -1 on EOF (which becomes 255 after & 0xFF), and read(byte[], int, int) may return fewer bytes than requested. Either case leaves the IV corrupt or partially uninitialized, breaking decryption.
Proposed fix
if (isKeyFromAndroidKeyStore) {
- int ivLength = fis.read() & 0xFF;
- iv = new byte[ivLength];
- fis.read(iv, 0, ivLength);
+ int ivLength = fis.read();
+ if (ivLength <= 0) {
+ throw new IOException("Invalid IV length while reading encrypted file: " + file.getAbsolutePath());
+ }
+ iv = new byte[ivLength];
+ int offset = 0;
+ while (offset < ivLength) {
+ int count = fis.read(iv, offset, ivLength - offset);
+ if (count < 0) {
+ throw new IOException("Unexpected EOF while reading IV");
+ }
+ offset += count;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/network/EncryptedFileBody.java` around lines 55 - 58,
The IV-read code in EncryptedFileBody is not validating reads and can produce a
corrupted IV; update the block that reads from fis (the read of ivLength and the
fis.read(iv, 0, ivLength) into iv) to: check the initial fis.read() return for
-1 and throw an IOException on EOF, validate ivLength is within expected bounds
(e.g., >0 and <= a reasonable max like 256), allocate iv accordingly, then loop
calling fis.read(...) until you have received exactly ivLength bytes
(accumulating bytesRead) and throw an IOException if a read returns -1 before
filling the buffer—this ensures the IV is fully and safely read before
decryption.
| @RequiresApi(Build.VERSION_CODES.M) | ||
| class SessionManager { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all Java call sites invoking SessionManager
rg -n "SessionManager\." --type java -A 2 -B 5Repository: dimagi/commcare-android
Length of output: 10909
🏁 Script executed:
# Also check if there are any Kotlin files using SessionManager
rg -n "SessionManager\." --type kotlin -A 2 -B 5Repository: dimagi/commcare-android
Length of output: 12202
🏁 Script executed:
# First, let me find the SessionManager.kt file to verify it exists and check its content
fd "SessionManager.kt" -t fRepository: dimagi/commcare-android
Length of output: 114
🏁 Script executed:
# Check for ktlint tool availability and run it on SessionManager.kt if found
if command -v ktlint &> /dev/null; then
ktlint "app/src/org/commcare/services/SessionManager.kt" 2>&1
else
echo "ktlint not found in environment"
fiRepository: dimagi/commcare-android
Length of output: 98
🏁 Script executed:
# Check LogSubmissionTask.java around line 300
sed -n '280,310p' app/src/org/commcare/tasks/LogSubmissionTask.javaRepository: dimagi/commcare-android
Length of output: 1089
🏁 Script executed:
# Check if LogSubmissionTask.java class has `@RequiresApi`
head -n 30 app/src/org/commcare/tasks/LogSubmissionTask.java | grep -A 5 -B 5 "@RequiresApi\|class LogSubmissionTask"Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check UserDatabaseUpgrader.java around line 869
sed -n '850,885p' app/src/org/commcare/models/database/user/UserDatabaseUpgrader.javaRepository: dimagi/commcare-android
Length of output: 1491
🏁 Script executed:
# Check if UserDatabaseUpgrader.java method has guards
sed -n '800,880p' app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java | grep -B 80 "SessionManager" | tail -n 40Repository: dimagi/commcare-android
Length of output: 1782
🏁 Script executed:
# Check DeviceReportRecord.java around line 71
sed -n '1,100p' app/src/org/commcare/android/javarosa/DeviceReportRecord.javaRepository: dimagi/commcare-android
Length of output: 2490
🏁 Script executed:
# Check LogSubmissionTask.java class declaration and method signature
grep -n "class LogSubmissionTask\|@RequiresApi\|public.*doInBackground\|protected.*doInBackground" app/src/org/commcare/tasks/LogSubmissionTask.java | head -20Repository: dimagi/commcare-android
Length of output: 255
🏁 Script executed:
# Check UserDatabaseUpgrader.java class and method context
grep -n "class UserDatabaseUpgrader\|@RequiresApi\|rekeyDeviceReportFileToKeystoreKey\|SDK_INT" app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java | head -20Repository: dimagi/commcare-android
Length of output: 283
🏁 Script executed:
# Check DeviceReportRecord.java for `@RequiresApi` and SDK_INT
grep -n "@RequiresApi\|class DeviceReportRecord\|SDK_INT\|Build.VERSION" app/src/org/commcare/android/javarosa/DeviceReportRecord.javaRepository: dimagi/commcare-android
Length of output: 147
🏁 Script executed:
# Look at the full method containing SessionManager calls in UserDatabaseUpgrader
sed -n '840,895p' app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java | cat -nRepository: dimagi/commcare-android
Length of output: 2931
🏁 Script executed:
# Check around the doInBackground method in LogSubmissionTask to see if there's any SDK_INT guard
sed -n '100,330p' app/src/org/commcare/tasks/LogSubmissionTask.java | grep -A 25 "protected LogSubmitOutcomes doInBackground"Repository: dimagi/commcare-android
Length of output: 1349
🏁 Script executed:
# Check if LogSubmissionTask has any `@RequiresApi` at all
head -n 60 app/src/org/commcare/tasks/LogSubmissionTask.java | grep "@RequiresApi"Repository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check the full method signature and context of openOutputStream in DeviceReportRecord
sed -n '65,80p' app/src/org/commcare/android/javarosa/DeviceReportRecord.javaRepository: dimagi/commcare-android
Length of output: 418
🏁 Script executed:
# Check if there's any API level check in the parent class Persisted or EncryptedModel
fd "Persisted.java" -t fRepository: dimagi/commcare-android
Length of output: 128
🏁 Script executed:
# Check for any Build.VERSION or SDK_INT checks in UserDatabaseUpgrader
grep -n "Build.VERSION\|SDK_INT" app/src/org/commcare/models/database/user/UserDatabaseUpgrader.javaRepository: dimagi/commcare-android
Length of output: 49
🏁 Script executed:
# Check if Persisted.java or any parent provides API guards
head -n 50 app/src/org/commcare/android/storage/framework/Persisted.javaRepository: dimagi/commcare-android
Length of output: 1914
🏁 Script executed:
# Check if DeviceReportRecord.openOutputStream is only called in safe contexts
grep -rn "openOutputStream\|DeviceReportRecord" app/src/org/commcare/tasks/LogSubmissionTask.javaRepository: dimagi/commcare-android
Length of output: 1649
🏁 Script executed:
# Let's see the full content of SessionManager.kt to understand the `@RequiresApi` scope
cat app/src/org/commcare/services/SessionManager.ktRepository: dimagi/commcare-android
Length of output: 850
🏁 Script executed:
# Check if there are any callers that wrap the openOutputStream call
rg -n "openOutputStream" --type java -B 3 -A 3Repository: dimagi/commcare-android
Length of output: 1452
Java call sites invoking SessionManager methods must add @RequiresApi(Build.VERSION_CODES.M) annotations or SDK_INT runtime guards.
The class-level @RequiresApi(M) annotation on SessionManager.kt provides lint enforcement in Kotlin but does not guard Java callers. The following Java methods invoke SessionManager.getEncryptionKey() or SessionManager.getKeyTransformation() without protection and will crash on API < 23:
LogSubmissionTask.doInBackground()(lines 300–301)UserDatabaseUpgrader.rekeyDeviceReportFileToKeystoreKey()(lines 869–870)DeviceReportRecord.openOutputStream()(lines 71–72)
Each must either be annotated with @RequiresApi(Build.VERSION_CODES.M) or guarded with an explicit if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/services/SessionManager.kt` around lines 11 - 12,
SessionManager is annotated `@RequiresApi`(M) only for Kotlin, so Java callers can
still call getEncryptionKey()/getKeyTransformation() on older SDKs and crash;
update each Java call site (LogSubmissionTask.doInBackground,
UserDatabaseUpgrader.rekeyDeviceReportFileToKeystoreKey, and
DeviceReportRecord.openOutputStream) to either add
`@RequiresApi`(Build.VERSION_CODES.M) to the method declaration or wrap the calls
in an explicit runtime guard if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M)
before invoking SessionManager.getEncryptionKey() or
SessionManager.getKeyTransformation(); pick the approach consistent with each
method's API contract (annotation if the whole method requires M, runtime check
if only a code path needs guarding).
| private val sessionKeyAndTransformation by lazy { | ||
| AesKeyStoreHandler(SESSION_KEY_ALIAS, needsUserAuth = false).getKeyOrGenerate() | ||
| } |
There was a problem hiding this comment.
Kotlin by lazy caches the first exception — keystore initialization failure is unrecoverable for the process lifetime.
If AesKeyStoreHandler(...).getKeyOrGenerate() throws (e.g., keystore unavailable, hardware missing, first-boot race), Kotlin's lazy caches the Throwable and rethrows it on every subsequent call to getEncryptionKey() or getKeyTransformation() — with no opportunity for retry or fallback. This makes any transient hardware/keystore issue a permanent failure for the app session.
Consider wrapping with a runCatching or using lazy(LazyThreadSafetyMode.NONE) combined with explicit retry logic, or at minimum document the permanent-failure behavior clearly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/services/SessionManager.kt` around lines 16 - 18,
sessionKeyAndTransformation currently uses Kotlin's default by lazy which caches
thrown exceptions from AesKeyStoreHandler(SESSION_KEY_ALIAS, needsUserAuth =
false).getKeyOrGenerate(), making keystore-init failures permanent; change to
avoid caching exceptions by wrapping initialization in a safe accessor: replace
the lazy usage with a non-caching approach (e.g.,
lazy(LazyThreadSafetyMode.NONE) combined with explicit retry/timeout or use
runCatching inside a short-lived initialization function) and ensure callers
getEncryptionKey() and getKeyTransformation() handle retryable failures and
surface clear errors or fallbacks instead of letting a cached Throwable be
rethrown for the process lifetime.
avazirna
changed the title
avazirna
changed the base branch from
master
to
vaz/encrypt-device-reports-with-android-keystore
avazirna
force-pushed
the
vaz/migrate-device-reports-to-android-keystore
branch
2 times, most recently
from
625f2e6 to
6d37530
Compare
avazirna
force-pushed
the
vaz/migrate-device-reports-to-android-keystore
branch
from
6d37530 to
f9ace7a
Compare
1 participant
Product Description
No user-facing changes.
Technical Summary
Device report files (log submissions) were previously encrypted with a randomly generated AES key stored as a
@Persistingfield on eachDeviceReportRecord. This PR replaces that per-record key with a single Android Keystore-backed AES key managed by a newSessionManagerclass, improving key management and removing the need to persist encryption keys in the database.Key changes:
SessionManager(new): lazily initialises and caches a Keystore-backed AES-CBC key viaAesKeyStoreHandlerDeviceReportRecord: removedaesKeyfield;openOutputStream()now delegates toSessionManagerEncryptionIO:createFileOutputStreamandgetFileInputStreamupdated to support IV-aware encryption (IV length + IV bytes prepended to ciphertext) when using a Keystore keyEncryptedFileBody: storesKey/transformation/isKeyFromAndroidKeyStoreinstead of a pre-builtCipher; reads IV from stream onwriteTo()FormUploadUtil.getDecryptCipher: updated signature to acceptKey,transformation, andivDeviceReportRecordblobs to strip the staleaesKeybytes, and re-keys all existing device report files from the old per-record AES key to the Keystore key using a write-to-temp-then-rename strategy to avoid data loss on failureDeviceReportRecordPreV30(new): legacy model used only during migration to deserialise the old blob formatFeature Flag
None.
Safety Assurance
Safety story
EncryptionIO,FormUploadUtil, andHybridFileBackedSqlStoragethat use per-record AES keys passtransformation=nullandisKeyFromAndroidKeyStore=false, preserving existing behaviour unchanged.Automated test coverage
No existing unit tests cover
DeviceReportRecorddirectly. The migration path andEncryptionIOchanges are covered by manual testing.QA Plan
Labels and Review