Encryption over transit. #747
Conversation
- makes code more verbose - done to handle encryption in upcoming commits.
- uses internal keyword where needed - fixes issues mentioned in info panel of android studio with suggested quick fix
- makes arrangement to collect public api key from the client in manifest - adds request body as string so we can encrypt and append it in send queue request
…st body code - adds method where we will create request body - adds provision where we will create encoded request body - wip - we only encode /a1 for now - adds methods in fixture to create config without encryption key
- adds key version which will be input as a meta by the client
- splits keygenerator and encryption class so we can generate a key.
…nager changes, ct api changes for header - adds header in ct api which sends info about encryption - exposes keygenerator generate secret key function - models encryption result during http call via EncryptionResult.kt - methods in crypt factory to provide encryption objects - adds singleton rsa encryption processor
- bubbles up secret key as encryption method param - uses correct key in NetworkEncryptionManager.kt to encrypt request body - renames request body val, folds unnecessary val declarations
CTLalit
requested review from
darshanclevertap,
piyush-kukadiya,
Anush-Shand and
vasct
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces extensive enhancements to encryption and network functionality in the CleverTap SDK. New classes for key management (e.g. CTKeyGenerator) and encryption (e.g. NetworkEncryptionManager, RSAEncryption) have been added, while existing factories and configuration classes are updated with public encryption key properties and methods. Cryptographic modules are refactored to decouple context from key generation. The network layer is restructured with a new Kotlin-based NetworkManager and improved response handling. Corresponding test cases and sample manifest configurations have been added or updated to validate and support the new encryption features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CtApi
participant NetworkEncryptionManager
participant AESGCMCrypt
participant CTKeyGenerator
Client->>CtApi: sendQueue(isEncrypted=true, body)
CtApi->>CtApi: Add encryption header if enabled
CtApi->>NetworkEncryptionManager: encryptResponse(response)
NetworkEncryptionManager->>CTKeyGenerator: Retrieve/generate session key
NetworkEncryptionManager->>AESGCMCrypt: performCryptOperation(response, sessionKey)
AESGCMCrypt-->>NetworkEncryptionManager: Return EncryptionResult
NetworkEncryptionManager-->>CtApi: Encrypted response payload
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (22)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiTest.kt (2)
43-70: Consider enhancing test coverage with edge cases.The test effectively verifies the basic encryption header functionality. Consider adding test cases for:
- Multiple consecutive requests with encryption toggled
- Edge cases where encryption is disabled mid-flow
- Invalid encryption states
Example test cases to add:
@Test fun test_sendRequests_handlesEncryptionStateTransitions() { // Test encryption enabled -> disabled transition val response1 = ctApi.sendQueue(false, getEmptyQueueBodyString(), isEncrypted = true) assertContains(response1.request.headers, "X-CleverTap-Encryption-Enabled" to "true") val response2 = ctApi.sendQueue(false, getEmptyQueueBodyString(), isEncrypted = false) assertFalse(response2.request.headers.containsKey("X-CleverTap-Encryption-Enabled")) }
346-348: Consider caching the empty queue string.Since the empty queue string is constant and used across multiple tests, consider caching it as a companion object property to avoid repeated object creation and string conversion.
class CtApiTest { + companion object { + private val EMPTY_QUEUE_STRING = SendQueueRequestBody(null, JSONArray()).toString() + } private fun getEmptyQueueBodyString(): String { - return SendQueueRequestBody(null, JSONArray()).toString() + return EMPTY_QUEUE_STRING } }clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt (1)
44-44: Consider moving the encryption header to companion object.The encryption header constant could be moved to the companion object to maintain consistency with other header constants like
HEADER_CUSTOM_HANDSHAKE.companion object { const val DEFAULT_CONTENT_TYPE = "application/json; charset=utf-8" const val DEFAULT_QUERY_PARAM_OS = "Android" const val HEADER_CUSTOM_HANDSHAKE = "X-CleverTap-Handshake-Domain" + const val HEADER_ENCRYPTION_ENABLED = "X-CleverTap-Encryption-Enabled" + const val HEADER_ENCRYPTION_ENABLED_VALUE = "true" } - private val encryptionHeader = "X-CleverTap-Encryption-Enabled" to "true" + private val encryptionHeader = HEADER_ENCRYPTION_ENABLED to HEADER_ENCRYPTION_ENABLED_VALUEclevertap-core/src/main/java/com/clevertap/android/sdk/network/RSAEncryption.kt (2)
20-24: Consider making cryptographic parameters configurable.The RSA parameters (key size, transformation) are hardcoded. Consider:
- Making KEY_SIZE configurable for future-proofing
- Moving crypto constants to a dedicated configuration class
98-158: Improve key conversion methods for consistency and clarity.Enhance the key conversion methods:
- Use Base64.NO_WRAP consistently
- Add format information in documentation (X.509/PKCS#8)
- Consider throwing exceptions instead of returning null
Example for one method:
/** * Converts a Base64 encoded string to a PublicKey object. + * The string should be a Base64-encoded X.509 formatted public key. * * @param publicKeyString The Base64 encoded public key string. * @return The PublicKey object, or null if an error occurs. + * @throws CryptoException if the key string is invalid */ - fun getPublicKeyFromString(publicKeyString: String): PublicKey? { - return try { - val publicKeyBytes = Base64.decode(publicKeyString, Base64.DEFAULT) + @Throws(CryptoException::class) + fun getPublicKeyFromString(publicKeyString: String): PublicKey { + try { + val publicKeyBytes = Base64.decode(publicKeyString, Base64.NO_WRAP) val keySpec = X509EncodedKeySpec(publicKeyBytes) val keyFactory = KeyFactory.getInstance(RSA_ALGORITHM) - keyFactory.generatePublic(keySpec) + return keyFactory.generatePublic(keySpec) } catch (e: Exception) { Logger.v("Error converting string to PublicKey", e) - null + throw CryptoException("Invalid public key format", e) } }clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CTKeyGenerator.kt (2)
13-23: Consider using an encrypted local storage mechanism.
Storing cryptographic keys in SharedPreferences, even Base64-encoded, may pose security risks on compromised or rooted devices. If backward compatibility allows, consider encrypting these stored keys using Android’s EncryptedSharedPreferences or similar solutions to better protect sensitive data.
54-77: Improve error handling around the Android Keystore operations.
WhenfromAndroidKeystore()encounters exceptions, the function logs them and returnsnull. In critical encryption flows, returningnullwithout retry or fallback might degrade security if the consumer proceeds with insecure methods or fails silently. Consider prompting a controlled fallback or rethrowing specific exceptions where appropriate.clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/AESGCMCrypt.kt (2)
15-17: Revisit visibility level on AESGCMCrypt.
Marking the classinternaland also exposing crypt-related methods publicly can be confusing if used across modules. Double-check that the usage context indeed requires aninternalclass rather than public or package-private.
124-145: Consider key or IV rotation strategy.
While AES-GCM automatically generates an IV for encryption, reusing the same key indefinitely may lead to cryptographic weaknesses if an attacker obtains multiple ciphertext/plaintext pairs. Define a rotation strategy or key re-generation interval to improve long-term security.clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (3)
63-77: Clarify “internal open” usage.
Declaring the class as bothinternalandopensuggests that you intend to extend it only within the same module. Confirm that you do not need external extension. Otherwise, consider adjusting visibility accordingly.
911-911: SwallowedJSONExceptionat line 911.
A comment says//skip, but fully swallowing JSON parse errors can hide potential data issues. Consider surfacing or handling the exception more explicitly.🧰 Tools
🪛 detekt (1.23.7)
[warning] 911-911: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
606-641: Consolidate domain/mute operations.
processIncomingHeaders()updates both mute state and domain. If multiple headers need processing, a single method with multiple side effects can become unwieldy. Consider splitting the domain logic from the mute logic or using separate private helpers for clarity and easier maintenance.clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/EncryptionResult.kt (1)
3-10: Consider adding error details to EncryptionFailure.The sealed class hierarchy is well-designed for representing encryption outcomes. However, the
EncryptionFailurecase could be enhanced by including error details to aid in debugging and logging.Consider modifying the failure case to include error details:
-internal data object EncryptionFailure : EncryptionResult() +internal data class EncryptionFailure( + val error: String, + val cause: Throwable? = null +) : EncryptionResult()clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptFactory.kt (1)
45-51: Consider adding null checks and error handling.The convenience methods could fail with ClassCastException or return null RSA instance.
Add proper error handling:
fun getAesGcmCrypt() : AESGCMCrypt { - return cryptInstances.getOrPut(EncryptionAlgorithm.AES_GCM) { getCrypt(EncryptionAlgorithm.AES_GCM, accountId, ctKeyGenerator) } as AESGCMCrypt + return (cryptInstances.getOrPut(EncryptionAlgorithm.AES_GCM) { + getCrypt(EncryptionAlgorithm.AES_GCM, accountId, ctKeyGenerator) + } as? AESGCMCrypt) ?: throw IllegalStateException("Failed to create AESGCMCrypt instance") } fun getRsaCrypt(): RSAEncryption { - return rsaCrypt ?: RSAEncryption().also { rsaCrypt = it } + return rsaCrypt ?: synchronized(this) { + rsaCrypt ?: RSAEncryption().also { rsaCrypt = it } + } }clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt (1)
9-10: Consider using more realistic test key values.While the current values work for testing, using realistic key formats would better validate format handling.
Use base64 encoded test keys that match production format:
- const val PUBLIC_ENCRYPTION_KEY = "public-encryption-key" + const val PUBLIC_ENCRYPTION_KEY = "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA..."clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapInstanceConfigTest.kt (1)
15-19: Add test cases for edge scenarios.While the basic encryption test is good, consider adding tests for:
- Invalid/malformed encryption keys
- Key version mismatches
- Empty/blank keys
Example additional test:
@Test fun `should not encrypt with invalid key format`() { val configWithInvalidKey = CleverTapFixtures.provideCleverTapInstanceConfig().apply { publicEncryptionKey = "invalid-key-format" } assertFalse(configWithInvalidKey.shouldEncryptResponse()) }clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkEncryptionManager.kt (1)
57-61: Use NO_WRAP flag for Base64 encoding.The DEFAULT flag adds newline characters every 76 characters which may cause issues with certain systems.
- return Base64.encodeToString(arr, Base64.DEFAULT) + return Base64.encodeToString(arr, Base64.NO_WRAP)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt (1)
11-11: Use a more descriptive constant name.The constant name should indicate its purpose and type of key being stored.
-const val ENCRYPTION_KEY = "EncryptionKey" +const val LOCAL_ENCRYPTION_KEY_STORAGE_KEY = "LocalEncryptionKey"clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt (2)
284-287: Add error handling for encryption initialization.While the encryption setup is correct, consider adding error handling to gracefully handle initialization failures.
Consider wrapping the initialization in a try-catch block:
-val encryptionManager = NetworkEncryptionManager( - keyGenerator = ctKeyGenerator, - aesgcm = cryptFactory.getAesGcmCrypt() -) +val encryptionManager = try { + NetworkEncryptionManager( + keyGenerator = ctKeyGenerator, + aesgcm = cryptFactory.getAesGcmCrypt() + ) +} catch (e: Exception) { + config.logger.debug(config.accountId, "Failed to initialize encryption: ${e.message}") + null +}
284-287: Consider adding error handling for encryption initialization.The initialization of
NetworkEncryptionManagershould handle potential errors fromkeyGeneratororaesgcminitialization to prevent runtime failures.Consider wrapping the initialization in a try-catch block:
-val encryptionManager = NetworkEncryptionManager( - keyGenerator = ctKeyGenerator, - aesgcm = cryptFactory.getAesGcmCrypt() -) +val encryptionManager = try { + NetworkEncryptionManager( + keyGenerator = ctKeyGenerator, + aesgcm = cryptFactory.getAesGcmCrypt() + ) +} catch (e: Exception) { + config.logger.debug(config.accountId, "Failed to initialize encryption manager: ${e.message}") + null +}clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java (2)
375-377: Enhance null safety and validation inshouldEncryptResponse.The current implementation only checks for null values. Consider adding validation for empty strings and version format.
Consider this enhanced implementation:
-public Boolean shouldEncryptResponse() { - return publicEncryptionKey != null && publicEncryptionKeyVersion != null; +public Boolean shouldEncryptResponse() { + return !TextUtils.isEmpty(publicEncryptionKey) && + !TextUtils.isEmpty(publicEncryptionKeyVersion) && + isValidVersion(publicEncryptionKeyVersion); +} + +private boolean isValidVersion(String version) { + try { + return !TextUtils.isEmpty(version) && version.matches("^\\d+(\\.\\d+)*$"); + } catch (Exception e) { + return false; + } +}
363-365: Add validation in setters for encryption key and version.The setters should validate the input values to prevent invalid data.
Consider adding validation:
public void setPublicEncryptionKey(String publicEncryptionKey) { + if (TextUtils.isEmpty(publicEncryptionKey)) { + logger.debug(getAccountId(), "Invalid public encryption key"); + return; + } this.publicEncryptionKey = publicEncryptionKey; } public void setPublicEncryptionKeyVersion(String publicEncryptionKeyVersion) { + if (TextUtils.isEmpty(publicEncryptionKeyVersion) || !isValidVersion(publicEncryptionKeyVersion)) { + logger.debug(getAccountId(), "Invalid public encryption key version"); + return; + } this.publicEncryptionKeyVersion = publicEncryptionKeyVersion; }Also applies to: 371-373
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapFactory.kt(4 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/CleverTapInstanceConfig.java(8 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/ManifestInfo.java(6 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/AESGCMCrypt.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CTKeyGenerator.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptExtensions.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptFactory.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptRepository.kt(2 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkEncryptionManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.java(0 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/RSAEncryption.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/EncryptedSendQueueRequestBody.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/EncryptionResult.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/Response.kt(1 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/UrlConnectionHttpClient.kt(3 hunks)clevertap-core/src/main/java/com/clevertap/android/sdk/response/CleverTapResponseDecorator.java(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt(2 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapInstanceConfigTest.kt(1 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt(3 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiTest.kt(5 hunks)clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/MockHttpClient.kt(1 hunks)sample/src/main/AndroidManifest.xml(2 hunks)
💤 Files with no reviewable changes (1)
- clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.java
🧰 Additional context used
🪛 detekt (1.23.7)
clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt
[warning] 328-328: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 911-911: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 939-939: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (17)
clevertap-core/src/test/java/com/clevertap/android/sdk/network/api/CtApiTest.kt (1)
33-33: LGTM! Consistent conversion to string representation.The change from
getEmptyQueueBody()togetEmptyQueueBodyString()is consistently applied across all test methods, aligning with the encryption requirements where data needs to be transmitted as strings.Also applies to: 116-116, 125-125
clevertap-core/src/main/java/com/clevertap/android/sdk/response/CleverTapResponseDecorator.java (1)
10-10: Verify the necessity of making the class public.The change from
abstracttopublic abstractaligns with the decorator pattern and could be necessary for implementing encryption-related response decorators. However, please verify:
- Are there any new decorators in other packages that require this visibility change?
- Could this expose any sensitive implementation details?
Let's verify the usage of this class:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for classes that extend CleverTapResponseDecorator ast-grep --pattern 'class $_ extends CleverTapResponseDecorator' # Search for references to CleverTapResponseDecorator rg "CleverTapResponseDecorator" -A 3Length of output: 6352
Review Verification: Public Modifier Change is Valid
The search results confirm that several response classes (e.g.,
InAppResponse,GeofenceResponse,ARPResponse, etc.) extendCleverTapResponseDecoratorand are declared as public. This indicates that making the decorator classpublic abstractis necessary to support these external implementations, aligning with the encryption over transit changes, and does not expose sensitive details.
- Verified extensions: In files like
InAppResponse.java,GeofenceResponse.java,ARPResponse.java, etc.clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/CtApi.kt (1)
50-66: 🛠️ Refactor suggestionAdd validation and documentation for encrypted payload handling.
The changes introduce encryption support but have several areas for improvement:
- The
bodyparameter type has been changed fromSendQueueRequestBodytoString, losing type safety.- There's no validation of the encrypted payload format.
- Missing documentation about the encryption protocol and expected format.
Consider these improvements:
- Add KDoc documentation describing the encryption protocol and format
- Add validation for encrypted payload
- Create a sealed type hierarchy for the body parameter:
sealed interface QueueRequestBody { fun serialize(): String } data class PlainQueueRequestBody( val body: SendQueueRequestBody ) : QueueRequestBody { override fun serialize() = body.toString() } data class EncryptedQueueRequestBody( val encryptedBody: String ) : QueueRequestBody { override fun serialize(): String { require(isValidEncryptedFormat(encryptedBody)) { "Invalid encrypted body format" } return encryptedBody } private fun isValidEncryptedFormat(body: String): Boolean { // Add validation logic here return true } }Then update the method signature:
fun sendQueue( isViewedEvent: Boolean, body: QueueRequestBody ): ResponseThis approach:
- Maintains type safety
- Enforces validation
- Makes the encryption handling explicit in the type system
clevertap-core/src/main/java/com/clevertap/android/sdk/network/RSAEncryption.kt (1)
17-18: Address the TODO comment and consider class structure.The TODO suggests trimming the class, but all current methods appear to be essential for RSA operations. Consider:
- Documenting why the class needs trimming or remove the TODO if no longer applicable
- Moving methods out of companion object if instance-level encryption state management is needed in the future
clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CTKeyGenerator.kt (2)
24-44: Handlenullsecret keys gracefully.
IngenerateOrGetKey(), iffromAndroidKeystore()or local key retrieval fails, the method can returnnull. Ensure that the downstream usage of this key checks fornullto avoid unexpected behavior or crashes.
46-52: Ensure key length availability across devices.
Using a 256-bit AES key is ideal, but some older devices or specific regions may lack the relevant security providers. Confirm that all target devices support this key size to avoid runtime failures.clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/AESGCMCrypt.kt (1)
84-89: ValidatesecretKeybefore initialization.
performCryptOperation()callsgenerateOrGetKey()by default, but it may still produce anullkey. Ensure that the calling code checks for a non-nullsecretKeyto prevent potentialNullPointerExceptionor cryptographic failures.clevertap-core/src/main/java/com/clevertap/android/sdk/network/NetworkManager.kt (1)
643-702: Optimize retry logic to avoid potential infinite loops.
WhensendQueue()repeatedly encounters exceptions (e.g., issues with domain resolution or encryption), it incrementsnetworkRetryCountandresponseFailureCount, but final exit criteria may become complicated. Confirm that you have an upper bound on retries to prevent indefinite loops when the server is unreachable.clevertap-core/src/main/java/com/clevertap/android/sdk/network/http/Response.kt (1)
5-10: Consider memory implications of storing response body as String.Storing the entire response body in memory could lead to OutOfMemoryError for large responses. Consider using a streaming approach for large payloads.
clevertap-core/src/main/java/com/clevertap/android/sdk/cryption/CryptFactory.kt (1)
23-32: LGTM: Factory method properly handles encryption algorithm selection.The removal of Context dependency and injection of CTKeyGenerator improves testability while maintaining secure key generation.
clevertap-core/src/test/java/com/clevertap/android/sdk/CleverTapFixtures.kt (1)
36-58: LGTM: Config creation methods properly handle encryption settings.The methods provide good test coverage for both encrypted and unencrypted scenarios.
clevertap-core/src/test/java/com/clevertap/android/sdk/network/http/MockHttpClient.kt (1)
21-22: LGTM: Simplified response handling for testing.The change from stream to direct string response makes it easier to test encrypted responses.
clevertap-core/src/test/java/com/clevertap/android/sdk/network/NetworkManagerTest.kt (1)
58-61: LGTM!The teardown is properly implemented using the AutoCloseable pattern.
clevertap-core/src/main/java/com/clevertap/android/sdk/Constants.java (2)
177-178: LGTM!The new encryption-related constants follow the existing naming convention and are appropriately placed in the constants file.
177-178: LGTM!The new constants follow the existing pattern and naming conventions.
sample/src/main/AndroidManifest.xml (2)
19-19: Verify Theme Attribute ConsistencyThe
android:themeattribute on line 19 is marked with a change indicator. If this change is solely due to formatting, then it’s acceptable. However, please verify that this update is intentional and that the theme remains consistent with the design requirements.
65-67:⚠️ Potential issueReview Public Encryption Key Meta-Data
The new
<meta-data>element forCLEVERTAP_PUB_KEYhas been added to support encryption over transit. Ensure that the placeholder value"YOUR PUBLIC KEY FOR ENCRYPTION"is replaced with the actual public key before production release. This update is critical for enabling proper encryption functionality and securing data in transit.
| fun sendQueue( | ||
| isViewedEvent: Boolean, | ||
| body: SendQueueRequestBody | ||
| body: String, | ||
| isEncrypted: Boolean = false | ||
| ): Response = | ||
| httpClient.execute( | ||
| createRequest( | ||
| baseUrl = getActualDomain(isViewedEvent = isViewedEvent) ?: defaultDomain, | ||
| relativeUrl = "a1", | ||
| body = body.toString() | ||
| body = body, | ||
| headers = if (isEncrypted) { | ||
| defaultHeaders.plus(encryptionHeader) | ||
| } else { | ||
| defaultHeaders | ||
| } | ||
| ) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify encryption header usage across the codebase.
The encryption header is only added to the sendQueue method. Other methods like defineVars and defineTemplates might also need encryption support.
Let's verify the usage:
🏁 Script executed:
#!/bin/bash
# Search for methods that might need encryption support
rg -A 5 "fun.*body.*Response" clevertap-core/src/main/java/com/clevertap/android/sdk/network/api/Length of output: 1590
Encryption header support is inconsistent:
- The
sendQueuemethod correctly appends the encryption header whenisEncryptedis true. - The
defineVarsanddefineTemplatesmethods, however, do not include any support for encryption headers.
This confirms the review comment’s concern that encryption handling is only implemented in sendQueue and may be missing in other similar methods.
| fun generateKeyPair(): KeyPair? { | ||
| return try { | ||
| val keyPairGenerator = KeyPairGenerator.getInstance(RSA_ALGORITHM) | ||
| keyPairGenerator.initialize(KEY_SIZE) | ||
| val generateKeyPair = keyPairGenerator.generateKeyPair() | ||
| generateKeyPair | ||
| } catch (e: Exception) { | ||
| Logger.v("Error generating RSA key pair", e) | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in key generation.
The current implementation silently logs errors and returns null. Consider:
- Throwing custom exceptions for different failure scenarios
- Adding detailed logging about the cause of failure
- fun generateKeyPair(): KeyPair? {
- return try {
+ @Throws(CryptoException::class)
+ fun generateKeyPair(): KeyPair {
+ try {
val keyPairGenerator = KeyPairGenerator.getInstance(RSA_ALGORITHM)
keyPairGenerator.initialize(KEY_SIZE)
- val generateKeyPair = keyPairGenerator.generateKeyPair()
- generateKeyPair
+ return keyPairGenerator.generateKeyPair()
} catch (e: Exception) {
- Logger.v("Error generating RSA key pair", e)
- null
+ Logger.v("Failed to generate RSA key pair: ${e.message}", e)
+ throw CryptoException("Failed to generate RSA key pair", e)
}
}📝 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.
| fun generateKeyPair(): KeyPair? { | |
| return try { | |
| val keyPairGenerator = KeyPairGenerator.getInstance(RSA_ALGORITHM) | |
| keyPairGenerator.initialize(KEY_SIZE) | |
| val generateKeyPair = keyPairGenerator.generateKeyPair() | |
| generateKeyPair | |
| } catch (e: Exception) { | |
| Logger.v("Error generating RSA key pair", e) | |
| null | |
| } | |
| } | |
| @Throws(CryptoException::class) | |
| fun generateKeyPair(): KeyPair { | |
| try { | |
| val keyPairGenerator = KeyPairGenerator.getInstance(RSA_ALGORITHM) | |
| keyPairGenerator.initialize(KEY_SIZE) | |
| return keyPairGenerator.generateKeyPair() | |
| } catch (e: Exception) { | |
| Logger.v("Failed to generate RSA key pair: ${e.message}", e) | |
| throw CryptoException("Failed to generate RSA key pair", e) | |
| } | |
| } |
| fun decrypt(encryptedData: String, privateKey: PrivateKey): ByteArray? { | ||
| return try { | ||
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | ||
| val oaepParameterSpec = OAEPParameterSpec( | ||
| "SHA-256", | ||
| "MGF1", | ||
| MGF1ParameterSpec.SHA256, | ||
| PSource.PSpecified.DEFAULT | ||
| ) | ||
| cipher.init(Cipher.DECRYPT_MODE, privateKey, oaepParameterSpec) | ||
| val encryptedBytes = Base64.decode(encryptedData, Base64.DEFAULT) | ||
| cipher.doFinal(encryptedBytes) | ||
| } catch (e: Exception) { | ||
| Logger.v("Error decrypting data with RSA", e) | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance security and consistency in decryption.
Similar improvements needed as encryption:
- Use the extracted OAEP parameters method
- Use Base64.NO_WRAP for consistency
- Add input validation
- Make error messages less specific to prevent oracle attacks
fun decrypt(encryptedData: String, privateKey: PrivateKey): ByteArray? {
return try {
val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION)
- val oaepParameterSpec = OAEPParameterSpec(
- "SHA-256",
- "MGF1",
- MGF1ParameterSpec.SHA256,
- PSource.PSpecified.DEFAULT
- )
- cipher.init(Cipher.DECRYPT_MODE, privateKey, oaepParameterSpec)
- val encryptedBytes = Base64.decode(encryptedData, Base64.DEFAULT)
+ cipher.init(Cipher.DECRYPT_MODE, privateKey, createOAEPParams())
+ val encryptedBytes = Base64.decode(encryptedData, Base64.NO_WRAP)
cipher.doFinal(encryptedBytes)
} catch (e: Exception) {
- Logger.v("Error decrypting data with RSA", e)
+ Logger.v("Decryption failed") // Generic error message
null
}
}📝 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.
| fun decrypt(encryptedData: String, privateKey: PrivateKey): ByteArray? { | |
| return try { | |
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | |
| val oaepParameterSpec = OAEPParameterSpec( | |
| "SHA-256", | |
| "MGF1", | |
| MGF1ParameterSpec.SHA256, | |
| PSource.PSpecified.DEFAULT | |
| ) | |
| cipher.init(Cipher.DECRYPT_MODE, privateKey, oaepParameterSpec) | |
| val encryptedBytes = Base64.decode(encryptedData, Base64.DEFAULT) | |
| cipher.doFinal(encryptedBytes) | |
| } catch (e: Exception) { | |
| Logger.v("Error decrypting data with RSA", e) | |
| null | |
| } | |
| } | |
| fun decrypt(encryptedData: String, privateKey: PrivateKey): ByteArray? { | |
| return try { | |
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | |
| cipher.init(Cipher.DECRYPT_MODE, privateKey, createOAEPParams()) | |
| val encryptedBytes = Base64.decode(encryptedData, Base64.NO_WRAP) | |
| cipher.doFinal(encryptedBytes) | |
| } catch (e: Exception) { | |
| Logger.v("Decryption failed") // Generic error message | |
| null | |
| } | |
| } |
| fun encrypt(data: ByteArray, publicKey: PublicKey): String? { | ||
| return try { | ||
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | ||
| val oaepParameterSpec = OAEPParameterSpec( | ||
| "SHA-256", | ||
| "MGF1", | ||
| MGF1ParameterSpec.SHA256, | ||
| PSource.PSpecified.DEFAULT | ||
| ) | ||
| cipher.init(Cipher.ENCRYPT_MODE, publicKey, oaepParameterSpec) | ||
| val encryptedBytes = cipher.doFinal(data) | ||
| Base64.encodeToString(encryptedBytes, Base64.DEFAULT) | ||
| } catch (e: Exception) { | ||
| Logger.v("Error encrypting data with RSA", e) | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate OAEP parameters and enhance input validation.
Several improvements needed:
- Extract duplicate OAEP parameter creation to a private method
- Use Base64.NO_WRAP instead of DEFAULT to avoid line breaks
- Add input validation for data size relative to key size
+ private fun createOAEPParams() = OAEPParameterSpec(
+ "SHA-256",
+ "MGF1",
+ MGF1ParameterSpec.SHA256,
+ PSource.PSpecified.DEFAULT
+ )
fun encrypt(data: ByteArray, publicKey: PublicKey): String? {
+ if (data.size > KEY_SIZE / 8 - 42) { // OAEP overhead is 42 bytes
+ Logger.v("Data too large for RSA encryption with key size $KEY_SIZE")
+ return null
+ }
return try {
val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION)
- val oaepParameterSpec = OAEPParameterSpec(
- "SHA-256",
- "MGF1",
- MGF1ParameterSpec.SHA256,
- PSource.PSpecified.DEFAULT
- )
- cipher.init(Cipher.ENCRYPT_MODE, publicKey, oaepParameterSpec)
+ cipher.init(Cipher.ENCRYPT_MODE, publicKey, createOAEPParams())
val encryptedBytes = cipher.doFinal(data)
- Base64.encodeToString(encryptedBytes, Base64.DEFAULT)
+ Base64.encodeToString(encryptedBytes, Base64.NO_WRAP)
} catch (e: Exception) {
Logger.v("Error encrypting data with RSA", e)
null
}
}📝 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.
| fun encrypt(data: ByteArray, publicKey: PublicKey): String? { | |
| return try { | |
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | |
| val oaepParameterSpec = OAEPParameterSpec( | |
| "SHA-256", | |
| "MGF1", | |
| MGF1ParameterSpec.SHA256, | |
| PSource.PSpecified.DEFAULT | |
| ) | |
| cipher.init(Cipher.ENCRYPT_MODE, publicKey, oaepParameterSpec) | |
| val encryptedBytes = cipher.doFinal(data) | |
| Base64.encodeToString(encryptedBytes, Base64.DEFAULT) | |
| } catch (e: Exception) { | |
| Logger.v("Error encrypting data with RSA", e) | |
| null | |
| } | |
| } | |
| private fun createOAEPParams() = OAEPParameterSpec( | |
| "SHA-256", | |
| "MGF1", | |
| MGF1ParameterSpec.SHA256, | |
| PSource.PSpecified.DEFAULT | |
| ) | |
| fun encrypt(data: ByteArray, publicKey: PublicKey): String? { | |
| if (data.size > KEY_SIZE / 8 - 42) { // OAEP overhead is 42 bytes | |
| Logger.v("Data too large for RSA encryption with key size $KEY_SIZE") | |
| return null | |
| } | |
| return try { | |
| val cipher = Cipher.getInstance(RSA_CIPHER_TRANSFORMATION) | |
| cipher.init(Cipher.ENCRYPT_MODE, publicKey, createOAEPParams()) | |
| val encryptedBytes = cipher.doFinal(data) | |
| Base64.encodeToString(encryptedBytes, Base64.NO_WRAP) | |
| } catch (e: Exception) { | |
| Logger.v("Error encrypting data with RSA", e) | |
| null | |
| } | |
| } |
| ) | ||
| ) | ||
| } | ||
| } catch (e: JSONException) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Swallowed exception at line 939.
Here, e.printStackTrace() is called, but no further handling or rethrow is done. This can obscure the severity of the error. Provide a structured logging statement or consider rethrowing if needed.
🧰 Tools
🪛 detekt (1.23.7)
[warning] 939-939: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| @@ -37,6 +38,7 @@ class UrlConnectionHttpClient( | |||
|
|
|||
| override fun execute(request: Request): Response { | |||
| var connection: HttpsURLConnection? = null | |||
| var reader: BufferedReader? = null | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Kotlin's use for resource management.
The BufferedReader and HttpsURLConnection should be closed using Kotlin's use block for better resource management.
- var connection: HttpsURLConnection? = null
- var reader: BufferedReader? = null
+ return openHttpsURLConnection(request).use { connection ->
+ val responseCode = connection.responseCode
+ val headers = connection.headerFields
- try {
- connection = openHttpsURLConnection(request)
- // ...
- } finally {
- reader?.close()
- connection?.disconnect()
- }
+ if (responseCode == HttpURLConnection.HTTP_OK) {
+ connection.inputStream
+ } else {
+ connection.errorStream
+ }.bufferedReader(Charsets.UTF_8).use { reader ->
+ Response(
+ request = request,
+ code = responseCode,
+ headers = headers,
+ body = reader.readText()
+ )
+ }
+ }Also applies to: 73-74
| } catch (e: Exception) { | ||
| connection?.disconnect() | ||
| // todo why should we throw and not model this into error response? | ||
| throw e | ||
| } finally { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model errors into response instead of throwing exceptions.
As noted in the TODO comment, errors should be modeled into the response rather than thrown.
- } catch (e: Exception) {
- // todo why should we throw and not model this into error response?
- throw e
+ } catch (e: Exception) {
+ logger.debug("HTTP request failed", e)
+ return Response(
+ request = request,
+ code = -1,
+ headers = emptyMap(),
+ body = null
+ )
}📝 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.
| } catch (e: Exception) { | |
| connection?.disconnect() | |
| // todo why should we throw and not model this into error response? | |
| throw e | |
| } finally { | |
| } catch (e: Exception) { | |
| logger.debug("HTTP request failed", e) | |
| return Response( | |
| request = request, | |
| code = -1, | |
| headers = emptyMap(), | |
| body = null | |
| ) | |
| } finally { |
| private var closeable: AutoCloseable? = null | ||
| private lateinit var networkManager: NetworkManager | ||
| private lateinit var ctApi: CtApi | ||
| private lateinit var mockHttpClient: MockHttpClient | ||
| @Mock private lateinit var ctApiWrapper : CtApiWrapper | ||
| @Mock private lateinit var networkEncryptionManager: NetworkEncryptionManager | ||
|
|
||
| @Before | ||
| fun setUpNetworkManager() { | ||
| MockitoAnnotations.openMocks(this) | ||
| fun setup() { | ||
| closeable = MockitoAnnotations.openMocks(this) | ||
| mockHttpClient = MockHttpClient() | ||
| ctApi = CtApiTestProvider.provideTestCtApiForConfig(cleverTapInstanceConfig, mockHttpClient) | ||
| networkManager = provideNetworkManager() | ||
| `when`(ctApiWrapper.ctApi).thenReturn(ctApi) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for encryption functionality.
The test class lacks coverage for the newly added encryption functionality.
Would you like me to generate test cases for:
- Session key management
- Response encryption
- Error handling during encryption
| notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY); | ||
| publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON); |
There was a problem hiding this comment.
Fix swapped assignments of notification icon and public encryption key.
The assignments appear to be swapped:
notificationIconis incorrectly assigned the value ofLABEL_CLEVERTAP_PUB_KEYpublicEncryptionKeyis incorrectly assigned the value ofLABEL_NOTIFICATION_ICON
This will cause the notification icon to be missing and the encryption key to be incorrect.
Apply this diff to fix the assignments:
-notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY);
-publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON);
+notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON);
+publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY);📝 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.
| notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY); | |
| publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON); | |
| notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON); | |
| publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY); |
Fix swapped values in constructor.
The values for notificationIcon and publicEncryptionKey are incorrectly swapped:
notificationIconis assigned the value fromLABEL_CLEVERTAP_PUB_KEYpublicEncryptionKeyis assigned the value fromLABEL_NOTIFICATION_ICON
Apply this diff to fix the swapped values:
-notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY);
-publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON);
+notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON);
+publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY);📝 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.
| notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY); | |
| publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON); | |
| notificationIcon = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_NOTIFICATION_ICON); | |
| publicEncryptionKey = _getManifestStringValueForKey(metaData, ManifestInfo.LABEL_CLEVERTAP_PUB_KEY); |
| public Boolean shouldEncryptResponse() { | ||
| return publicEncryptionKey != null && publicEncryptionKeyVersion != null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for encryption configuration.
The shouldEncryptResponse method checks for non-null values but should also validate the format and length of the encryption key and version.
Consider adding validation:
public Boolean shouldEncryptResponse() {
- return publicEncryptionKey != null && publicEncryptionKeyVersion != null;
+ if (publicEncryptionKey == null || publicEncryptionKeyVersion == null) {
+ return false;
+ }
+ // Validate key format (e.g., base64 encoded)
+ if (!isValidBase64(publicEncryptionKey)) {
+ logger.debug(getAccountId(), "Invalid public encryption key format");
+ return false;
+ }
+ // Validate version format (e.g., semantic version)
+ if (!isValidVersion(publicEncryptionKeyVersion)) {
+ logger.debug(getAccountId(), "Invalid public encryption key version format");
+ return false;
+ }
+ return true;
}
+
+private boolean isValidBase64(String key) {
+ try {
+ android.util.Base64.decode(key, android.util.Base64.DEFAULT);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+}
+
+private boolean isValidVersion(String version) {
+ return version.matches("^\\d+(\\.\\d+)*$");
+}📝 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.
| public Boolean shouldEncryptResponse() { | |
| return publicEncryptionKey != null && publicEncryptionKeyVersion != null; | |
| } | |
| public Boolean shouldEncryptResponse() { | |
| if (publicEncryptionKey == null || publicEncryptionKeyVersion == null) { | |
| return false; | |
| } | |
| // Validate key format (e.g., base64 encoded) | |
| if (!isValidBase64(publicEncryptionKey)) { | |
| logger.debug(getAccountId(), "Invalid public encryption key format"); | |
| return false; | |
| } | |
| // Validate version format (e.g., semantic version) | |
| if (!isValidVersion(publicEncryptionKeyVersion)) { | |
| logger.debug(getAccountId(), "Invalid public encryption key version format"); | |
| return false; | |
| } | |
| return true; | |
| } | |
| private boolean isValidBase64(String key) { | |
| try { | |
| android.util.Base64.decode(key, android.util.Base64.DEFAULT); | |
| return true; | |
| } catch (IllegalArgumentException e) { | |
| return false; | |
| } | |
| } | |
| private boolean isValidVersion(String version) { | |
| return version.matches("^\\d+(\\.\\d+)*$"); | |
| } |
- encrypts key used in symmetric AES algo with RSA - this is passed along in request body - fallback scenario includes making a normal api request - moves static methods outside at class level
- moves header constants together - extracts out request body from response handling method
- fixes NetworkEncryptionManager.kt creation in core state creation, passes rsa key lambda - adds constant for encryption needed header - fixes method encryptedSessionKey() to use correct public rsa key from client - adds method on NetworkEncryptionManager.kt to decrypt response from LC - changes handle a1 response method code to decrypt response before passing
https://wizrocket.atlassian.net/browse/SDK-4341
Summary by CodeRabbit
These updates further strengthen data security, ensure reliable encrypted communications, and provide a smoother, more robust configuration experience for end-users.