feat: encryption of data #348
Conversation
WalkthroughThe pull request introduces significant updates to the CleverTap SDK, focusing on enhancing encryption capabilities for local storage. Key changes include the removal and update of specific Rollup plugins in Changes
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/util/constants.js (1)
13-13: Consider using a less predictable key name for encryption.Using 'WZRK_E' as the encryption key name is predictable and follows the same pattern as other keys.
Consider using a more random or unique identifier to avoid making it obvious that this key is related to encryption.
rollup.config.js (1)
8-8: Remove extra semicolon.For consistency with the rest of the file, remove the semicolon.
-import commonjs from '@rollup/plugin-commonjs'; +import commonjs from '@rollup/plugin-commonjs'🧰 Tools
🪛 ESLint
[error] 8-8: Extra semicolon.
(semi)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.jspackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(2 hunks)rollup.config.js(2 hunks)src/clevertap.js(6 hunks)src/modules/security/Encryption.js(1 hunks)src/util/constants.js(2 hunks)src/util/storage.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/util/storage.js
[error] 14-14: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 ESLint
rollup.config.js
[error] 8-8: Extra semicolon.
(semi)
🔇 Additional comments (2)
src/util/constants.js (1)
73-77: Review the selection of keys for encryption.Please verify if these are the only sensitive keys that need encryption. For example, consider if META_COOKIE should also be encrypted since it might contain user-specific data.
✅ Verification successful
META_COOKIE encryption is not required
The current selection of keys for encryption (KCOOKIE_NAME, LRU_CACHE, PR_COOKIE) is appropriate. META_COOKIE only stores non-sensitive metadata like timestamps, session counters, and feature flags, which don't require encryption.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of these cookies to understand the type of data they store echo "Analyzing KCOOKIE_NAME usage:" rg -A 5 "KCOOKIE_NAME" echo "\nAnalyzing LRU_CACHE usage:" rg -A 5 "LRU_CACHE" echo "\nAnalyzing PR_COOKIE usage:" rg -A 5 "PR_COOKIE" echo "\nAnalyzing META_COOKIE usage:" rg -A 5 "META_COOKIE"Length of output: 73573
Script:
#!/bin/bash # Search for META_COOKIE usage and data assignments echo "=== META_COOKIE usage ===" rg -A 5 "META_COOKIE.*=|=.*META_COOKIE|\[META_COOKIE\]|META_COOKIE\:" echo -e "\n=== Storage patterns for cookies ===" rg -A 5 "saveToLSorCookie.*META_COOKIE|readFromLSorCookie.*META_COOKIE" echo -e "\n=== Encryption related code ===" rg -A 5 "encrypt.*META_COOKIE|decrypt.*META_COOKIE" echo -e "\n=== META_COOKIE in security contexts ===" rg -p "META_COOKIE" -g "**/*security*/**"Length of output: 67584
Script:
#!/bin/bash # Search for META_COOKIE data assignments echo "=== META_COOKIE data assignments ===" rg -A 5 "setMetaProp|META_COOKIE.*=.*{" echo -e "\n=== Encryption implementation ===" rg -A 10 "encrypt.*META_COOKIE|encryption.*META_COOKIE|security.*META_COOKIE" echo -e "\n=== Security sensitive patterns ===" rg -A 5 "token.*META_COOKIE|key.*META_COOKIE|secret.*META_COOKIE|sensitive.*META_COOKIE|private.*META_COOKIE"Length of output: 65995
Script:
#!/bin/bash # Search for META_COOKIE data assignments and usage echo "=== META_COOKIE data assignments ===" rg "setMetaProp\(" -A 5 rg "META_COOKIE.*=" -A 5 echo -e "\n=== META_COOKIE value patterns ===" rg "wzrkMetaObj\[.*\]" -A 3 rg "metaObj\[.*\]" -A 3 echo -e "\n=== Security related code ===" rg "KEYS_TO_ENCRYPT" -A 5 rg "encrypt" -A 5Length of output: 75476
package.json (1)
34-35: LGTM! Dependencies are up-to-date and secure.The added dependencies look good:
- [email protected] is a recent version with security fixes
- Rollup plugin updates are appropriate for the build system
Also applies to: 50-51
| encrypt (data) { | ||
| return AES.encrypt(data, this.key).toString() | ||
| } |
There was a problem hiding this comment.
Add error handling for encryption/decryption operations.
The encrypt/decrypt methods should handle potential errors that could occur during cryptographic operations.
Apply this diff to add error handling:
encrypt (data) {
- return AES.encrypt(data, this.key).toString()
+ try {
+ if (!this.key) {
+ throw new Error('Encryption key is not set')
+ }
+ return AES.encrypt(data, this.key).toString()
+ } catch (error) {
+ this.#logger?.error('Encryption failed:', error)
+ throw error
+ }
}
decrypt (data) {
- return AES.decrypt(data, this.key).toString(enc.Utf8)
+ try {
+ if (!this.key) {
+ throw new Error('Encryption key is not set')
+ }
+ return AES.decrypt(data, this.key).toString(enc.Utf8)
+ } catch (error) {
+ this.#logger?.error('Decryption failed:', error)
+ throw error
+ }
}Also applies to: 51-53
| set key (value) { | ||
| this.#key = value | ||
| } | ||
|
|
||
| get key () { | ||
| return this.#key | ||
| } |
There was a problem hiding this comment.
Enhance key management security.
The encryption key getter provides direct access to the sensitive key material.
Consider these security improvements:
- Remove the key getter to prevent exposure
- Add key validation in the setter
- Consider using a key derivation function
Apply this diff:
set key (value) {
+ if (!value || typeof value !== 'string' || value.length < 16) {
+ throw new Error('Invalid encryption key')
+ }
this.#key = value
}
-get key () {
- return this.#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.
| set key (value) { | |
| this.#key = value | |
| } | |
| get key () { | |
| return this.#key | |
| } | |
| set key (value) { | |
| if (!value || typeof value !== 'string' || value.length < 16) { | |
| throw new Error('Invalid encryption key') | |
| } | |
| this.#key = value | |
| } |
src/modules/security/Encryption.js
Outdated
| shouldDecrypt (key) { | ||
| // TODO: why not use StorageManager.read()? | ||
| // because it will introduce a circular dependency since we are | ||
| // calling this function within read() as well. | ||
| // Possibly will think of a workaround. | ||
| return (JSON.parse(localStorage.getItem(ENCRYPTION_KEY)) ?? false) && KEYS_TO_ENCRYPT.includes(key) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct localStorage access in shouldDecrypt method.
Direct access to localStorage creates a tight coupling and makes testing difficult. Consider using a storage abstraction.
Apply this diff to improve the implementation:
shouldDecrypt (key) {
- return (JSON.parse(localStorage.getItem(ENCRYPTION_KEY)) ?? false) && KEYS_TO_ENCRYPT.includes(key)
+ const isEncryptionEnabled = this.#encryptLocalStorage
+ return isEncryptionEnabled && KEYS_TO_ENCRYPT.includes(key)
}Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| if (encryption.shouldDecrypt(key)) { | ||
| data = encryption.decrypt(data) | ||
| } | ||
| data = JSON.parse(data) | ||
| } catch (e) {} | ||
| } |
There was a problem hiding this comment.
Improve error handling in read method.
The current implementation silently catches all errors which could hide encryption/decryption failures.
Apply this diff to improve error handling:
try {
if (encryption.shouldDecrypt(key)) {
- data = encryption.decrypt(data)
+ try {
+ data = encryption.decrypt(data)
+ } catch (decryptError) {
+ console.error(`Failed to decrypt data for key ${key}:`, decryptError)
+ return null
+ }
}
data = JSON.parse(data)
-} catch (e) {}
+} catch (parseError) {
+ console.error(`Failed to parse data for key ${key}:`, parseError)
+ return 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.
| try { | |
| if (encryption.shouldDecrypt(key)) { | |
| data = encryption.decrypt(data) | |
| } | |
| data = JSON.parse(data) | |
| } catch (e) {} | |
| } | |
| try { | |
| if (encryption.shouldDecrypt(key)) { | |
| try { | |
| data = encryption.decrypt(data) | |
| } catch (decryptError) { | |
| console.error(`Failed to decrypt data for key ${key}:`, decryptError) | |
| return null | |
| } | |
| } | |
| data = JSON.parse(data) | |
| } catch (parseError) { | |
| console.error(`Failed to parse data for key ${key}:`, parseError) | |
| return null | |
| } | |
| } |
| if (encryption.shouldEncrypt(key)) { | ||
| localStorage.setItem(key, encryption.encrypt(value)) | ||
| return true | ||
| } | ||
| localStorage.setItem(key, typeof value === 'string' ? value : JSON.stringify(value)) | ||
| return true | ||
| } |
There was a problem hiding this comment.
Standardize value serialization before encryption.
The non-encrypted path uses JSON.stringify for objects, but the encryption path doesn't.
Apply this diff to fix the inconsistency:
if (encryption.shouldEncrypt(key)) {
- localStorage.setItem(key, encryption.encrypt(value))
+ const serializedValue = typeof value === 'string' ? value : JSON.stringify(value)
+ localStorage.setItem(key, encryption.encrypt(serializedValue))
return true
}📝 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 (encryption.shouldEncrypt(key)) { | |
| localStorage.setItem(key, encryption.encrypt(value)) | |
| return true | |
| } | |
| localStorage.setItem(key, typeof value === 'string' ? value : JSON.stringify(value)) | |
| return true | |
| } | |
| if (encryption.shouldEncrypt(key)) { | |
| const serializedValue = typeof value === 'string' ? value : JSON.stringify(value) | |
| localStorage.setItem(key, encryption.encrypt(serializedValue)) | |
| return true | |
| } | |
| localStorage.setItem(key, typeof value === 'string' ? value : JSON.stringify(value)) | |
| return true | |
| } |
| console.log(clevertap.account?.[0].id) | ||
| encryption.key = clevertap.account?.[0].id |
There was a problem hiding this comment.
Security concern: Encryption key derivation needs improvement.
Using the account ID directly as an encryption key is not secure as it's predictable and potentially discoverable. Consider implementing a more secure key derivation process.
Recommendations:
- Use a key derivation function (e.g., PBKDF2) to generate a strong encryption key
- Add salt to prevent rainbow table attacks
- Consider using environment-specific factors for additional entropy
Example implementation:
-encryption.key = clevertap.account?.[0].id
+const salt = crypto.getRandomValues(new Uint8Array(16));
+const keyMaterial = clevertap.account?.[0].id + window.location.hostname;
+encryption.key = CryptoJS.PBKDF2(keyMaterial, salt, {
+ keySize: 256/32,
+ iterations: 1000
+});Also applies to: 168-170
| if (accountId) { | ||
| encryption.key = accountId | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential encryption key inconsistency between constructor and init.
The encryption key is set in both the constructor and init method, which could lead to inconsistencies. Consider centralizing the key management.
+private initializeEncryption(accountId) {
+ if (!accountId) return;
+ try {
+ const salt = crypto.getRandomValues(new Uint8Array(16));
+ const keyMaterial = accountId + window.location.hostname;
+ encryption.key = CryptoJS.PBKDF2(keyMaterial, salt, {
+ keySize: 256/32,
+ iterations: 1000
+ });
+ } catch (error) {
+ this.#logger.error('Failed to initialize encryption:', error);
+ }
+}Then use this method in both places:
-encryption.key = accountId
+this.initializeEncryption(accountId)Committable suggestion skipped: line range outside the PR's diff.
| this.encryptLocalStorage = (value) => { | ||
| encryption.encryptLocalStorage = value | ||
| } | ||
|
|
||
| this.isLocalStorageEncrypted = () => { | ||
| return encryption.encryptLocalStorage | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation for encryption methods.
The encryption methods should include proper error handling and input validation to ensure robustness.
this.encryptLocalStorage = (value) => {
+ if (typeof value !== 'boolean') {
+ this.#logger.error('encryptLocalStorage expects a boolean value');
+ return;
+ }
+ try {
encryption.encryptLocalStorage = value;
+ } catch (error) {
+ this.#logger.error('Failed to set encryption:', error);
+ }
}
this.isLocalStorageEncrypted = () => {
+ try {
return encryption.encryptLocalStorage;
+ } catch (error) {
+ this.#logger.error('Failed to get encryption status:', error);
+ return false;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/modules/security/Encryption.js (2)
18-24:⚠️ Potential issueEnhance key management security.
The encryption key management needs security improvements.
As mentioned in the previous review, apply these security improvements:
- Remove the key getter to prevent exposure
- Add key validation in the setter
- Consider using a key derivation function
set key (value) { + if (!value || typeof value !== 'string' || value.length < 16) { + throw new Error('Invalid encryption key') + } this.#key = value } -get key () { - return this.#key -}
44-55:⚠️ Potential issueAdd error handling for encryption/decryption operations.
The cryptographic operations need proper error handling.
As mentioned in the previous review, apply error handling:
encrypt (data) { - return AES.encrypt(data, this.key).toString() + try { + if (!this.key) { + throw new Error('Encryption key is not set') + } + return AES.encrypt(data, this.key).toString() + } catch (error) { + this.#logger?.error('Encryption failed:', error) + throw error + } } decrypt (data) { - const decryptedData = AES.decrypt(data, this.key).toString(enc.Utf8) - if (decryptedData === '') { - return data - } else { - return decryptedData + try { + if (!this.key) { + throw new Error('Encryption key is not set') + } + const decryptedData = AES.decrypt(data, this.key).toString(enc.Utf8) + return decryptedData || data + } catch (error) { + this.#logger?.error('Decryption failed:', error) + throw error } }
🧹 Nitpick comments (4)
src/modules/security/Encryption.js (4)
3-3: Remove or uncomment the StorageManager import.The commented import suggests incomplete refactoring. Either remove it if unused or uncomment if needed.
-// import { StorageManager } from '../../util/storage'
10-16: Add validation for logger instance.Add type checking to ensure the logger instance has required methods.
set logger (classInstance) { + if (!classInstance || typeof classInstance.error !== 'function') { + throw new Error('Invalid logger instance') + } this.#logger = classInstance }
26-32: Persist encryption flag state.The encryption flag state should be persisted to maintain consistency across page reloads.
set encryptLocalStorage (value) { this.#encryptLocalStorage = value + try { + localStorage.setItem('encryptionEnabled', JSON.stringify(value)) + } catch (error) { + this.#logger?.error('Failed to persist encryption state:', error) + } } get encryptLocalStorage () { - return this.#encryptLocalStorage + try { + const persisted = localStorage.getItem('encryptionEnabled') + return persisted ? JSON.parse(persisted) : this.#encryptLocalStorage + } catch (error) { + this.#logger?.error('Failed to read encryption state:', error) + return this.#encryptLocalStorage + } }
34-42: Enhance documentation for backward compatibility.While the comment explains the backward compatibility behavior, it would be helpful to add more detailed documentation about the implications and lifecycle of this behavior.
- // For backwards compatibility, we should decrypt even if encrypt is false. - // This means someone switched it on and then off. + /** + * Determines if a key's value should be decrypted. + * For backwards compatibility, we attempt decryption even if encryption is disabled. + * This handles cases where: + * 1. Data was encrypted when the feature was enabled + * 2. The feature was later disabled + * 3. We still need to read the encrypted data + * + * @param {string} key - The storage key to check + * @returns {boolean} True if the value should be decrypted + */ shouldDecrypt (key) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
clevertap.js.mapis excluded by!**/*.mapclevertap.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (1)
src/modules/security/Encryption.js(1 hunks)
🔇 Additional comments (1)
src/modules/security/Encryption.js (1)
58-60: LGTM! Singleton pattern is appropriate here.The singleton pattern ensures consistent encryption state across the application.
Changes
Describe the key changes in this PR with the Jira Issue reference
Changes to Public Facing API if any
Please list the impact on the public facing API if any
How Has This Been Tested?
Describe the testing approach and any relevant configurations (e.g., environment, platform)
Checklist
Summary by CodeRabbit
Release Notes
New Features
Dependencies
crypto-jslibrary for encryption supportSecurity Improvements
The release introduces advanced security features with local storage encryption, ensuring better protection of critical application data.