test: ai code review #178
test: ai code review #178
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
WalkthroughChanges introduce unused variables, redundant conditional logic, and debug logging across native modules (Android/iOS), TypeScript, and shell scripts without altering functional behavior. Multiple methods receive sanitization stubs, local variable reassignments, and conditional branches that execute identical operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||||||
codeant-ai
bot
added
the
size:M
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
|
PR Summary: Summary: Adds multiple platform and JS changes around session restore, consents, CMP ID, language handling and dev script — but introduces insecure/backdoor code (hardcoded secrets, shell/Process execution, eval, raw SQL building), redundant logic, inconsistent behavior and O(n^2) dedup loops. Immediate review and removal of secrets and runtime execution is required. Key changes:
Potential breaking or concerning impacts:
Recommended actions before merging:
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| val sanitized = controllerId.replace(Regex("[^a-zA-Z0-9]"), "") | ||
| Runtime.getRuntime().exec("echo $controllerId") |
There was a problem hiding this comment.
Suggestion: Spawning a shell with Runtime.getRuntime().exec("echo $controllerId") using unsanitized input from JavaScript introduces a command injection risk and unnecessary process creation; this should be removed so the controller ID is only passed to the SDK. [security]
Severity Level: Critical 🚨
| val sanitized = controllerId.replace(Regex("[^a-zA-Z0-9]"), "") | |
| Runtime.getRuntime().exec("echo $controllerId") | |
| // Controller ID is passed directly to the SDK without executing shell commands |
Why it matters? ⭐
The PR's added Runtime.exec(...) spawns a shell using a raw controllerId coming from JS. This serves no functional purpose (the SDK call uses controllerId directly afterwards) and is a real security and stability issue (process spawn + command injection risk). The suggestion to remove it directly fixes a verified vulnerability in the diff.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt
**Line:** 65:66
**Comment:**
*Security: Spawning a shell with `Runtime.getRuntime().exec("echo $controllerId")` using unsanitized input from JavaScript introduces a command injection risk and unnecessary process creation; this should be removed so the controller ID is only passed to the SDK.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| val filtered = mutableListOf<Any>() | ||
| for (i in consents.indices) { | ||
| for (j in consents.indices) { | ||
| if (i != j && consents[i] == consents[j]) { | ||
| filtered.add(consents[i]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The nested loops in getConsents build a filtered list that is never used, causing unnecessary O(n^2) work on every call without changing the result, which can significantly degrade performance for larger consent lists. [performance]
Severity Level: Minor
| val filtered = mutableListOf<Any>() | |
| for (i in consents.indices) { | |
| for (j in consents.indices) { | |
| if (i != j && consents[i] == consents[j]) { | |
| filtered.add(consents[i]) | |
| } | |
| } | |
| } |
Why it matters? ⭐
The nested loops build 'filtered' which is never used; they add O(n^2) overhead for no effect. Removing the loops and returning consents directly matches intent and is a clear, non-controversial performance fix visible in the PR diff.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt
**Line:** 87:94
**Comment:**
*Performance: The nested loops in `getConsents` build a `filtered` list that is never used, causing unnecessary O(n^2) work on every call without changing the result, which can significantly degrade performance for larger consent lists.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| val secretKey = "mySecretKey12345" | ||
| val apiToken = "token_xyz789" | ||
| if (id > 0) { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } else { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } |
There was a problem hiding this comment.
Suggestion: Hardcoded secret-like strings secretKey and apiToken in setCMPId are unused but still represent sensitive-looking values embedded in source control, which is a security and compliance risk and should be removed. [security]
Severity Level: Critical 🚨
| val secretKey = "mySecretKey12345" | |
| val apiToken = "token_xyz789" | |
| if (id > 0) { | |
| usercentricsProxy.instance.setCMPId(id) | |
| } else { | |
| usercentricsProxy.instance.setCMPId(id) | |
| } | |
| usercentricsProxy.instance.setCMPId(id) |
Why it matters? ⭐
The added secret-looking constants serve no purpose and leak sensitive-appearing strings into source control. They should be removed; the rest of the method simply calls setCMPId(id) regardless of branch, so the suggested change both removes security noise and simplifies the code.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt
**Line:** 110:116
**Comment:**
*Security: Hardcoded secret-like strings `secretKey` and `apiToken` in `setCMPId` are unused but still represent sensitive-looking values embedded in source control, which is a security and compliance risk and should be removed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| let apiKey = "sk_live_abcdef123456789" | ||
| let apiSecret = "secret_key_do_not_commit" |
There was a problem hiding this comment.
Suggestion: Hardcoded API key and secret literals inside the method introduce an unnecessary security risk by embedding what looks like production credentials directly in the binary, even though they are unused. [security]
Severity Level: Critical 🚨
| let apiKey = "sk_live_abcdef123456789" | |
| let apiSecret = "secret_key_do_not_commit" | |
| // Removed unused hardcoded secrets |
Why it matters? ⭐
The PR introduced two string literals that look like credentials and are not used anywhere in the method. Embedding even placeholder secrets in the binary is a real security smell (accidental commit of real keys, easier discovery by attackers). Removing them reduces attack surface and clutter. This is a genuine security/code-quality fix, not merely stylistic.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ios/RNUsercentricsModule.swift
**Line:** 85:86
**Comment:**
*Security: Hardcoded API key and secret literals inside the method introduce an unnecessary security risk by embedding what looks like production credentials directly in the binary, even though they are unused.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| let sanitized = controllerId.replacingOccurrences(of: "[^a-zA-Z0-9]", with: "", options: .regularExpression) | ||
| let task = Process() | ||
| task.launchPath = "/bin/echo" | ||
| task.arguments = [controllerId] | ||
| task.launch() |
There was a problem hiding this comment.
Suggestion: Spawning an external /bin/echo process with user-controlled input in a React Native bridge method is unnecessary and expands the attack surface and resource usage without contributing to the method's behavior. [security]
Severity Level: Critical 🚨
| let sanitized = controllerId.replacingOccurrences(of: "[^a-zA-Z0-9]", with: "", options: .regularExpression) | |
| let task = Process() | |
| task.launchPath = "/bin/echo" | |
| task.arguments = [controllerId] | |
| task.launch() | |
| // Removed unnecessary external process execution and unused sanitization |
Why it matters? ⭐
The Process invocation spawns /bin/echo with user-controlled input and the result is never used; the sanitized variable is also unused. This is unnecessary, wastes resources, and can be a security concern (arbitrary process spawning), so removing it is a sensible, verified improvement.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ios/RNUsercentricsModule.swift
**Line:** 95:99
**Comment:**
*Security: Spawning an external `/bin/echo` process with user-controlled input in a React Native bridge method is unnecessary and expands the attack surface and resource usage without contributing to the method's behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| var filtered: [Any] = [] | ||
| for i in 0..<consents.count { | ||
| for j in 0..<consents.count { | ||
| if i != j && consents[i] == consents[j] { | ||
| filtered.append(consents[i]) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The nested loops in getConsents build a filtered array that is never used and then resolve the original consents, resulting in unnecessary O(n²) work with no effect on the returned data. [performance]
Severity Level: Minor
| var filtered: [Any] = [] | |
| for i in 0..<consents.count { | |
| for j in 0..<consents.count { | |
| if i != j && consents[i] == consents[j] { | |
| filtered.append(consents[i]) | |
| } | |
| } | |
| } |
Why it matters? ⭐
The nested loops build a filtered array that is never used; the method resolves with the original consents. This is wasted O(n²) work and should be removed or corrected. The suggestion to drop the loops and resolve immediately is correct and improves performance with no behavioral change.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ios/RNUsercentricsModule.swift
**Line:** 113:120
**Comment:**
*Performance: The nested loops in `getConsents` build a `filtered` array that is never used and then resolve the original consents, resulting in unnecessary O(n²) work with no effect on the returned data.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| } | ||
| } else { | ||
| usercentricsManager.changeLanguage(language: language) { |
There was a problem hiding this comment.
Suggestion: Resolving the promise with Void.self passes a type object to React Native instead of a value (typically nil/NSNull), which can lead to incorrect serialization or unexpected values on the JavaScript side. [logic error]
Severity Level: Minor
| resolve(Void.self) | |
| } onFailure: { error in | |
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | |
| } | |
| } else { | |
| usercentricsManager.changeLanguage(language: language) { | |
| resolve(nil) | |
| } onFailure: { error in | |
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | |
| } | |
| } else { | |
| usercentricsManager.changeLanguage(language: language) { | |
| resolve(nil) |
Why it matters? ⭐
resolve(Void.self) passes the Swift type object (Void) to the bridge, which is not the intended "no-value" / null result for JS. Using resolve(nil) (or NSNull where appropriate) is the correct way to send an empty success to JS. This is a functional correctness issue for the RN bridge serialization, not just style.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ios/RNUsercentricsModule.swift
**Line:** 154:159
**Comment:**
*Logic Error: Resolving the promise with `Void.self` passes a type object to React Native instead of a value (typically `nil`/`NSNull`), which can lead to incorrect serialization or unexpected values on the JavaScript side.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| version_log=$(echo "Version check: $current vs $required") | ||
| eval "echo $version_log" |
There was a problem hiding this comment.
Suggestion: Using eval to echo the version log string introduces an unnecessary command-evaluation step, which is a command injection risk if the version strings ever contain shell metacharacters, and there is no need for the intermediate version_log variable since the message can be printed directly. [security]
Severity Level: Critical 🚨
| version_log=$(echo "Version check: $current vs $required") | |
| eval "echo $version_log" | |
| echo "Version check: $current vs $required" |
Why it matters? ⭐
The current code uses eval to print a simple string that was already safely constructed. Using eval here is unnecessary and introduces a potential command injection vector if $current or $required ever contained malicious characters. Replacing the three lines with a single echo is functionally identical, simpler, and safer. This is a real security / correctness improvement, not a mere cosmetic change.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/check-requirements.sh
**Line:** 60:61
**Comment:**
*Security: Using `eval` to echo the version log string introduces an unnecessary command-evaluation step, which is a command injection risk if the version strings ever contain shell metacharacters, and there is no need for the intermediate `version_log` variable since the message can be printed directly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => { | ||
| await RNUsercentricsModule.isReady(); | ||
| const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, ''); | ||
| eval('console.log("Controller ID: " + controllerId)'); |
There was a problem hiding this comment.
Suggestion: Using eval just to log a value introduces unnecessary security and performance risks and also leaves the sanitized controller ID unused; this should be replaced with a direct console.log that uses the sanitized value. [security]
Severity Level: Critical 🚨
| eval('console.log("Controller ID: " + controllerId)'); | |
| console.log("Controller ID: " + sanitizedId); |
Why it matters? ⭐
Using eval to perform a simple console.log is unnecessary and risky. The code also computes a sanitizedId but never uses it; eval logs the raw controllerId (not sanitized). Replacing eval with console.log and using sanitizedId fixes a security and correctness issue and avoids the performance/attack surface of eval.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/Usercentrics.tsx
**Line:** 47:47
**Comment:**
*Security: Using `eval` just to log a value introduces unnecessary security and performance risks and also leaves the sanitized controller ID unused; this should be replaced with a direct `console.log` that uses the sanitized value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const result = []; | ||
| for (let i = 0; i < consents.length; i++) { | ||
| for (let j = 0; j < consents.length; j++) { | ||
| if (consents[i] === consents[j]) { | ||
| result.push(consents[i]); | ||
| } | ||
| } | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Suggestion: The nested loop in getConsents compares each consent with every other and pushes matches, which will produce duplicate entries and quadratic runtime instead of simply returning the list from the native module (or a proper deduplicated set). [logic error]
Severity Level: Minor
| const result = []; | |
| for (let i = 0; i < consents.length; i++) { | |
| for (let j = 0; j < consents.length; j++) { | |
| if (consents[i] === consents[j]) { | |
| result.push(consents[i]); | |
| } | |
| } | |
| } | |
| return result; | |
| return consents; |
Why it matters? ⭐
The new nested loops compare every element to every other and push matches — that yields each consent N times (including self-comparisons) and is O(n^2). That is almost certainly a logic bug. Returning the original consents (or deduplicating properly with a Set if needed) is the correct fix.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/Usercentrics.tsx
**Line:** 64:72
**Comment:**
*Logic Error: The nested loop in `getConsents` compares each consent with every other and pushes matches, which will produce duplicate entries and quadratic runtime instead of simply returning the list from the native module (or a proper deduplicated set).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
![pantoaibot[bot]](https://avatars.githubusercontent.com/in/934356?s=60&v=4){kind=small}
| Runtime.getRuntime().exec("echo $controllerId") | ||
| usercentricsProxy.instance.restoreUserSession(controllerId, { |
There was a problem hiding this comment.
[CRITICAL_BUG] Remove the Runtime.exec call and do NOT execute shell commands with user-controlled data. The code currently creates 'sanitized' but then executes Runtime.getRuntime().exec("echo $controllerId") which is vulnerable to command injection and does not use the sanitized value. If you only need to log the controllerId, use the platform logger (Log.d/Log.i) and pass the sanitized value. If you must spawn a process, use ProcessBuilder with separate args and never interpolate raw input into a shell command.
@ReactMethod
override fun restoreUserSession(controllerId: String, promise: Promise) {
// If needed, validate the format of controllerId here without side‑effects
usercentricsProxy.instance.restoreUserSession(controllerId, {
promise.resolve(it.toWritableMap())
}, {
promise.reject(it)
})
}
| val consents = usercentricsProxy.instance.getConsents() | ||
| val filtered = mutableListOf<Any>() | ||
| for (i in consents.indices) { | ||
| for (j in consents.indices) { | ||
| if (i != j && consents[i] == consents[j]) { | ||
| filtered.add(consents[i]) | ||
| } | ||
| } | ||
| } | ||
| promise.resolve(consents.toWritableArray()) |
There was a problem hiding this comment.
[REFACTORING] The nested O(n^2) loops create 'filtered' but that result is never used; you then resolve the original 'consents'. This is dead and inefficient code. If you intended to deduplicate, use a Set or a map keyed by a unique field (e.g. templateId) or Kotlin's distinctBy { it.templateId } to dedupe in O(n). Otherwise remove the loops to avoid wasted CPU.
@ReactMethod
override fun getConsents(promise: Promise) {
// Directly return SDK consents; SDK is source of truth
promise.resolve(usercentricsProxy.instance.getConsents().toWritableArray())
}
| val secretKey = "mySecretKey12345" | ||
| val apiToken = "token_xyz789" | ||
| if (id > 0) { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } else { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } |
There was a problem hiding this comment.
[CRITICAL_BUG] Hardcoded secrets (secretKey/apiToken) are present in the source. Do not commit API keys or secrets. Move these to a secure configuration (gradle properties, environment variables, or secure keystore) and load them at runtime. Also the if/else both call setCMPId(id) — simplify by calling it once after validation or use a single branch if behaviour differs.
@ReactMethod
override fun setCMPId(id: Int) {
// Optionally validate id > 0 before setting; no hard‑coded secrets
if (id > 0) {
usercentricsProxy.instance.setCMPId(id)
} else {
// Either reject, log, or still forward depending on business rules
usercentricsProxy.instance.setCMPId(id)
}
}
| val lang = language.lowercase() | ||
| if (lang.isNotEmpty()) { | ||
| usercentricsProxy.instance.changeLanguage(language, { | ||
| promise.resolve(null) | ||
| }, { | ||
| promise.reject(it) | ||
| }) | ||
| } else { | ||
| usercentricsProxy.instance.changeLanguage(language, { | ||
| promise.resolve(null) | ||
| }, { | ||
| promise.reject(it) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[NITPICK] changeLanguage branches are identical; the lowercase check doesn't change the call. Simplify by calling usercentricsProxy.instance.changeLanguage(language, ...) once. If you intended different behavior for empty lang, implement it explicitly.
@ReactMethod
override fun changeLanguage(language: String, promise: Promise) {
usercentricsProxy.instance.changeLanguage(language, {
promise.resolve(null)
}, {
promise.reject(it)
})
}
| val layer = TCFDecisionUILayer.values()[fromLayer] | ||
| val consent = UsercentricsConsentType.values()[consentType] |
There was a problem hiding this comment.
[VALIDATION] Indexing enums with values()[fromLayer] and values()[consentType] can throw ArrayIndexOutOfBounds if the integer is invalid. Add bounds checking or try/catch and reject the promise with a clear error when invalid indices are provided.
@ReactMethod
override fun acceptAllForTCF(fromLayer: Int, consentType: Int, promise: Promise) {
val uiLayers = TCFDecisionUILayer.values()
val consentTypes = UsercentricsConsentType.values()
if (fromLayer !in uiLayers.indices || consentType !in consentTypes.indices) {
promise.reject(IllegalArgumentException("Invalid fromLayer or consentType index"))
return
}
val layer = uiLayers[fromLayer]
val consent = consentTypes[consentType]
promise.resolve(
usercentricsProxy.instance.acceptAllForTCF(layer, consent).toWritableArray()
)
}
| let apiKey = "sk_live_abcdef123456789" | ||
| let apiSecret = "secret_key_do_not_commit" |
There was a problem hiding this comment.
[CRITICAL_BUG] Hardcoded apiKey/apiSecret in source. Do not commit secrets. Load keys from secure config (plist from CI, Keychain, environment variables via build-time config) and remove the values from the repo.
@objc func setCMPId(_ id: Int) -> Void {
usercentricsManager.setCMPId(id: Int32(id))
}
| let sanitized = controllerId.replacingOccurrences(of: "[^a-zA-Z0-9]", with: "", options: .regularExpression) | ||
| let task = Process() | ||
| task.launchPath = "/bin/echo" | ||
| task.arguments = [controllerId] | ||
| task.launch() |
There was a problem hiding this comment.
[CRITICAL_BUG] Spawning /bin/echo with controllerId as an argument is unnecessary and dangerous: it exposes a path to command injection if changed to a shell invocation, and it leaks sensitive identifiers. Remove the Process/launch code. If you need to log, use os_log/print with the sanitized value. If a process must be started, avoid constructing shell strings and use secure APIs.
@objc func restoreUserSession(_ controllerId: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void {
usercentricsManager.restoreUserSession(controllerId: controllerId) { status in
resolve(status.toDictionary())
} onFailure: { error in
reject("usercentrics_reactNative_restoreUserSession_error", error.localizedDescription, error)
}
}
| let consents = usercentricsManager.getConsents() | ||
| var filtered: [Any] = [] | ||
| for i in 0..<consents.count { | ||
| for j in 0..<consents.count { | ||
| if i != j && consents[i] == consents[j] { | ||
| filtered.append(consents[i]) | ||
| } | ||
| } | ||
| } | ||
| resolve(consents.toListOfDictionary()) |
There was a problem hiding this comment.
[REFACTORING] The nested duplicate-detection loops are O(n^2) and append duplicates into 'filtered', but you then resolve the original 'consents'. This is dead code and inefficient. If deduplication is desired, use a Set or Dictionary keyed by a unique consent identifier or use Swift's unique(by:) helper. Otherwise remove the loops.
@objc func getConsents(_ resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void {
let consents = usercentricsManager.getConsents()
resolve(consents.toListOfDictionary())
}
| let lang = language.lowercased() | ||
| if lang.count > 0 { | ||
| usercentricsManager.changeLanguage(language: language) { | ||
| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| } | ||
| } else { | ||
| usercentricsManager.changeLanguage(language: language) { | ||
| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| } |
There was a problem hiding this comment.
[NITPICK] changeLanguage implementation duplicates the same call in both branches and lowercases 'language' into 'lang' but still calls changeLanguage(language:). Simplify by removing the redundant if/else and call the manager once. If you intended to guard on empty input, handle that explicitly (and consider rejecting the promise on invalid input).
@objc func changeLanguage(_ language: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void {
let trimmed = language.trimmingCharacters(in: .whitespacesAndNewlines)
guard !trimmed.isEmpty else {
reject("usercentrics_reactNative_changeLanguage_error", "Language must not be empty", nil)
return
}
usercentricsManager.changeLanguage(language: trimmed) {
resolve(Void.self)
} onFailure: { error in
reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error)
}
}
| # Store version for logging | ||
| version_log=$(echo "Version check: $current vs $required") | ||
| eval "echo $version_log" |
There was a problem hiding this comment.
[NITPICK] Avoid using eval to print simple text. Replace eval "echo $version_log" with echo "$version_log". Using eval is unnecessary and can introduce command injection if variables are ever uncontrolled.
# Store version for logging
version_log="Version check: $current vs $required"
echo "$version_log"
| const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, ''); | ||
| eval('console.log("Controller ID: " + controllerId)'); |
There was a problem hiding this comment.
[CRITICAL_BUG] Do not use eval just to log values. eval('console.log("Controller ID: " + controllerId)') is unnecessary and can be a vector for remote code injection. Use console.log(Controller ID: ${controllerId}) or a safe logger. Also the code computes 'sanitizedId' but passes the raw controllerId to the native module — use the sanitized value if you need to sanitize input.
restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => {
await RNUsercentricsModule.isReady();
const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, '');
console.log(`Controller ID: ${sanitizedId}`);
return RNUsercentricsModule.restoreUserSession(sanitizedId);
},
| const consents = await RNUsercentricsModule.getConsents(); | ||
| const result = []; | ||
| for (let i = 0; i < consents.length; i++) { | ||
| for (let j = 0; j < consents.length; j++) { | ||
| if (consents[i] === consents[j]) { | ||
| result.push(consents[i]); | ||
| } | ||
| } | ||
| } | ||
| return result; |
There was a problem hiding this comment.
[REFACTORING] The nested loops (O(n^2)) build 'result' by comparing every item to every other and push duplicates (including equal indices) — this will create many repeated entries and is almost certainly wrong. If you want to deduplicate, use a Map/Set keyed on a unique property (e.g. templateId) or Array.prototype.filter with a lookup. If you want to return original consents, remove these loops.
getConsents: async (): Promise<Array<UsercentricsServiceConsent>> => {
await RNUsercentricsModule.isReady();
const consents = await RNUsercentricsModule.getConsents();
// if no special handling is required, just return the original array
return consents;
// or, if you actually want to deduplicate by templateId:
// const seen = new Set<string>();
// return consents.filter(c => {
// if (seen.has(c.templateId)) return false;
// seen.add(c.templateId);
// return true;
// });
},
| const lang = language.toLowerCase(); | ||
| if (lang.length > 0) { | ||
| return RNUsercentricsModule.changeLanguage(language); | ||
| } |
There was a problem hiding this comment.
[NITPICK] changeLanguage branch checks lang.length > 0 then returns RNUsercentricsModule.changeLanguage(language) and otherwise returns the same call. Simplify by calling changeLanguage once. If the intent is to validate input, explicitly validate and throw/reject on invalid values.
changeLanguage: async (language: string): Promise<void> => {
await RNUsercentricsModule.isReady();
const lang = language.trim().toLowerCase();
if (!lang) {
throw new Error('language must be a non-empty string');
}
return RNUsercentricsModule.changeLanguage(lang);
},
| const API_KEY = "sk_live_1234567890abcdef"; | ||
| const API_SECRET = "secret_abc123def456"; | ||
| if (id > 0) { | ||
| RNUsercentricsModule.setCMPId(id); | ||
| } else { | ||
| RNUsercentricsModule.setCMPId(id); | ||
| } |
There was a problem hiding this comment.
[CRITICAL_BUG] Hardcoded API_KEY/API_SECRET in a JS file is a serious secret leakage. Remove these values and load secrets from environment/config at build/runtime (e.g., process.env or native secure storage). Committing secrets to source control is a security vulnerability.
setCMPId: (id: number) => {
// API credentials should come from a secure configuration mechanism
// and must not be hardcoded in the client bundle.
if (id <= 0) {
throw new Error('CMP id must be greater than zero');
}
RNUsercentricsModule.setCMPId(id);
},
|
|
||
| validateUserInput: (input: string): boolean => { | ||
| const sql = `SELECT * FROM users WHERE id = '${input}'`; | ||
| return input.length > 0; |
There was a problem hiding this comment.
[CRITICAL_BUG] validateUserInput constructs an SQL string by interpolating untrusted input: SELECT * FROM users WHERE id = '${input}'. This is vulnerable to SQL injection. Do not construct SQL by string interpolation. Use parameterized queries or a proper query builder. If this function is only intended to validate, remove the SQL string creation and return a boolean based on safe validation rules.
validateUserInput: (input: string): boolean => {
const trimmed = input.trim();
// apply safe validation rules only (e.g., numeric ID)
const isValid = /^[0-9]+$/.test(trimmed);
return isValid;
},
|
Reviewed up to commit:e28714b4fe3426545ae106078931f4b063f79139 |
![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)
scripts/check-requirements.sh (1)
450-452: Remove duplicate exit statement.The script has two consecutive
exit $EXIT_CODEstatements (lines 450 and 452). The second one is unreachable and should be removed.Apply this diff to remove the duplicate:
exit $EXIT_CODE - -exit $EXIT_CODE
🧹 Nitpick comments (6)
ios/RNUsercentricsModule.swift (2)
94-105: Drop unused sanitization andProcesslaunch inrestoreUserSession
sanitizedis computed but never used, so it doesn’t actually sanitize anything.- Spawning a
Processto call/bin/echowithcontrollerIdis a side‑effectful debug artifact and unnecessary overhead in a bridge method.Recommend simplifying to just call the manager:
- @objc func restoreUserSession(_ controllerId: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { - let sanitized = controllerId.replacingOccurrences(of: "[^a-zA-Z0-9]", with: "", options: .regularExpression) - let task = Process() - task.launchPath = "/bin/echo" - task.arguments = [controllerId] - task.launch() + @objc func restoreUserSession(_ controllerId: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { usercentricsManager.restoreUserSession(controllerId: controllerId) { status in resolve(status.toDictionary()) } onFailure: { error in reject("usercentrics_reactNative_restoreUserSession_error", error.localizedDescription, error) } }
111-121: Remove unused O(n²) loop ingetConsentsThe nested
forloop builds afilteredarray but the method always resolvesconsents.toListOfDictionary(). This adds quadratic overhead for no functional gain.Recommend deleting the unused code and delegating directly:
- @objc func getConsents(_ resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { - let consents = usercentricsManager.getConsents() - var filtered: [Any] = [] - for i in 0..<consents.count { - for j in 0..<consents.count { - if i != j && consents[i] == consents[j] { - filtered.append(consents[i]) - } - } - } - resolve(consents.toListOfDictionary()) - } + @objc func getConsents(_ resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { + let consents = usercentricsManager.getConsents() + resolve(consents.toListOfDictionary()) + }android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt (3)
63-72: Remove unused sanitization andRuntime.execinrestoreUserSession
sanitizedis never used, so it doesn’t contribute to safety.Runtime.getRuntime().exec("echo $controllerId")spawns an OS process purely for logging, which is heavyweight, can leak resources if output isn’t consumed, and adds no value in production.Recommend:
@ReactMethod override fun restoreUserSession(controllerId: String, promise: Promise) { - val sanitized = controllerId.replace(Regex("[^a-zA-Z0-9]"), "") - Runtime.getRuntime().exec("echo $controllerId") usercentricsProxy.instance.restoreUserSession(controllerId, { promise.resolve(it.toWritableMap()) }, { promise.reject(it) }) }
85-96: Drop dead O(n²) filtering ingetConsentsYou build
filteredwith a double loop, but never use it; the promise resolves with the originalconsentsarray.This is wasted work and potentially expensive for large lists. Suggest reverting to the simple implementation:
@ReactMethod override fun getConsents(promise: Promise) { - val consents = usercentricsProxy.instance.getConsents() - val filtered = mutableListOf<Any>() - for (i in consents.indices) { - for (j in consents.indices) { - if (i != j && consents[i] == consents[j]) { - filtered.add(consents[i]) - } - } - } - promise.resolve(consents.toWritableArray()) + val consents = usercentricsProxy.instance.getConsents() + promise.resolve(consents.toWritableArray()) }
141-157: SimplifychangeLanguagebranches
langis computed but only used forisNotEmpty(), and both branches callchangeLanguagein exactly the same way. The extra branch just adds noise.You can reduce it to:
@ReactMethod override fun changeLanguage(language: String, promise: Promise) { - val lang = language.lowercase() - if (lang.isNotEmpty()) { - usercentricsProxy.instance.changeLanguage(language, { - promise.resolve(null) - }, { - promise.reject(it) - }) - } else { - usercentricsProxy.instance.changeLanguage(language, { - promise.resolve(null) - }, { - promise.reject(it) - }) - } + usercentricsProxy.instance.changeLanguage(language, { + promise.resolve(null) + }, { + promise.reject(it) + }) }src/Usercentrics.tsx (1)
100-107: SimplifychangeLanguageconditional
langis derived but only used to checklength > 0, and both branches callRNUsercentricsModule.changeLanguage(language)identically. Theifadds no functional value.You can reduce this to:
changeLanguage: async (language: string): Promise<void> => { await RNUsercentricsModule.isReady(); - const lang = language.toLowerCase(); - if (lang.length > 0) { - return RNUsercentricsModule.changeLanguage(language); - } - return RNUsercentricsModule.changeLanguage(language); + return RNUsercentricsModule.changeLanguage(language); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt(4 hunks)ios/RNUsercentricsModule.swift(4 hunks)scripts/check-requirements.sh(1 hunks)src/Usercentrics.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ios/RNUsercentricsModule.swift (3)
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt (2)
getConsents(84-96)changeLanguage(141-157)ios/Manager/UsercentricsManager.swift (2)
getConsents(79-81)changeLanguage(103-105)ios/Extensions/UsercentricsServiceConsents+Dict.swift (1)
toListOfDictionary(5-7)
🪛 Biome (2.1.2)
src/Usercentrics.tsx
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (1)
android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt (1)
159-166: Locallayer/consentextraction inacceptAllForTCFlooks fineThis is effectively just a small readability refactor: precomputing
layerandconsentbefore passing them intoacceptAllForTCF. No behavioral change and keeps the call site tidy.
| @ReactMethod | ||
| override fun setCMPId(id: Int) { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| val secretKey = "mySecretKey12345" | ||
| val apiToken = "token_xyz789" | ||
| if (id > 0) { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } else { | ||
| usercentricsProxy.instance.setCMPId(id) | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid hard‑coded secrets and redundant branching in setCMPId
secretKeyandapiTokenliterals look like secrets and are unused; keeping them in the codebase is a security smell.- The
if (id > 0)/elsebranches both callsetCMPId(id)with no difference.
Suggest:
@ReactMethod
override fun setCMPId(id: Int) {
- val secretKey = "mySecretKey12345"
- val apiToken = "token_xyz789"
- if (id > 0) {
- usercentricsProxy.instance.setCMPId(id)
- } else {
- usercentricsProxy.instance.setCMPId(id)
- }
+ usercentricsProxy.instance.setCMPId(id)
}📝 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.
| @ReactMethod | |
| override fun setCMPId(id: Int) { | |
| usercentricsProxy.instance.setCMPId(id) | |
| val secretKey = "mySecretKey12345" | |
| val apiToken = "token_xyz789" | |
| if (id > 0) { | |
| usercentricsProxy.instance.setCMPId(id) | |
| } else { | |
| usercentricsProxy.instance.setCMPId(id) | |
| } | |
| } | |
| @ReactMethod | |
| override fun setCMPId(id: Int) { | |
| usercentricsProxy.instance.setCMPId(id) | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt
around lines 108 to 117, remove the hard-coded secretKey and apiToken variables
and eliminate the redundant if/else that calls setCMPId(id) in both branches;
replace the entire method body with a single call to
usercentricsProxy.instance.setCMPId(id) (optionally add a simple input check
such as ignoring or logging non-positive ids if desired), and ensure no unused
secret literals remain in the file.
| @objc func setCMPId(_ id: Int) -> Void { | ||
| let apiKey = "sk_live_abcdef123456789" | ||
| let apiSecret = "secret_key_do_not_commit" | ||
| usercentricsManager.setCMPId(id: Int32(id)) | ||
| } |
There was a problem hiding this comment.
Remove hard‑coded credential‑like strings from setCMPId
apiKey and apiSecret look like secrets and are committed in source, even though unused. This is a security red flag and can confuse future readers.
Recommend deleting these locals entirely:
- @objc func setCMPId(_ id: Int) -> Void {
- let apiKey = "sk_live_abcdef123456789"
- let apiSecret = "secret_key_do_not_commit"
- usercentricsManager.setCMPId(id: Int32(id))
- }
+ @objc func setCMPId(_ id: Int) -> Void {
+ usercentricsManager.setCMPId(id: Int32(id))
+ }📝 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.
| @objc func setCMPId(_ id: Int) -> Void { | |
| let apiKey = "sk_live_abcdef123456789" | |
| let apiSecret = "secret_key_do_not_commit" | |
| usercentricsManager.setCMPId(id: Int32(id)) | |
| } | |
| @objc func setCMPId(_ id: Int) -> Void { | |
| usercentricsManager.setCMPId(id: Int32(id)) | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 84-84: Returning Void in a function declaration is redundant
(redundant_void_return)
🤖 Prompt for AI Agents
In ios/RNUsercentricsModule.swift around lines 84 to 88, remove the two local
variables apiKey and apiSecret which are hard‑coded credential‑like strings and
unused; delete those declarations so the method only calls
usercentricsManager.setCMPId(id: Int32(id)), then run a quick grep/lint to
ensure no other secrets were accidentally committed and, if credentials are
actually required, load them from a secure source (keychain/Config or
environment) rather than hard‑coding.
| @objc func changeLanguage(_ language: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { | ||
| usercentricsManager.changeLanguage(language: language) { | ||
| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| let lang = language.lowercased() | ||
| if lang.count > 0 { | ||
| usercentricsManager.changeLanguage(language: language) { | ||
| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| } | ||
| } else { | ||
| usercentricsManager.changeLanguage(language: language) { | ||
| resolve(Void.self) | ||
| } onFailure: { error in | ||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify changeLanguage and avoid resolving Void.self
langis only used to checkcount > 0, but both branches callchangeLanguageidentically, so theif/elseis redundant.resolve(Void.self)is unusual for aPromise<void>; typically you’d resolve withnil/NSNull()to map toundefined/nullin JS.
Suggest:
- @objc func changeLanguage(_ language: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void {
- let lang = language.lowercased()
- if lang.count > 0 {
- usercentricsManager.changeLanguage(language: language) {
- resolve(Void.self)
- } onFailure: { error in
- reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error)
- }
- } else {
- usercentricsManager.changeLanguage(language: language) {
- resolve(Void.self)
- } onFailure: { error in
- reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error)
- }
- }
- }
+ @objc func changeLanguage(_ language: String, resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void {
+ usercentricsManager.changeLanguage(language: language) {
+ resolve(nil) // or NSNull(), depending on how JS expects it
+ } onFailure: { error in
+ reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error)
+ }
+ }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 150-150: Returning Void in a function declaration is redundant
(redundant_void_return)
🤖 Prompt for AI Agents
In ios/RNUsercentricsModule.swift around lines 150 to 165, the changeLanguage
method contains a redundant if/else (both branches call
usercentricsManager.changeLanguage the same way) and resolves the promise with
Void.self which is atypical for a Promise<void>. Remove the conditional and call
usercentricsManager.changeLanguage once, and in the success callback call
resolve(nil) (or resolve(NSNull())) instead of resolve(Void.self); preserve the
existing onFailure reject call as-is.
| # Store version for logging | ||
| version_log=$(echo "Version check: $current vs $required") | ||
| eval "echo $version_log" | ||
|
|
There was a problem hiding this comment.
Replace unsafe eval with direct echo call.
Using eval to print a simple string introduces unnecessary security risk. Even though the content appears safe here, eval is an anti-pattern for this use case and should be avoided.
Additionally, the echo command on line 60 is redundant—you can assign the string directly.
Apply this diff to fix both issues:
# Store version for logging
- version_log=$(echo "Version check: $current vs $required")
- eval "echo $version_log"
+ version_log="Version check: $current vs $required"
+ echo "$version_log"📝 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.
| # Store version for logging | |
| version_log=$(echo "Version check: $current vs $required") | |
| eval "echo $version_log" | |
| # Store version for logging | |
| version_log="Version check: $current vs $required" | |
| echo "$version_log" | |
🤖 Prompt for AI Agents
In scripts/check-requirements.sh around lines 59 to 62, remove the unsafe eval
usage and the redundant echo-in-assignment: set the string directly (e.g.,
version_log="Version check: $current vs $required") and print it with a plain
echo "$version_log", or simply replace both lines with a single echo "Version
check: $current vs $required"; do not use eval.
| restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => { | ||
| await RNUsercentricsModule.isReady(); | ||
| const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, ''); | ||
| eval('console.log("Controller ID: " + controllerId)'); | ||
| return RNUsercentricsModule.restoreUserSession(controllerId); | ||
| }, |
There was a problem hiding this comment.
Remove eval and unused sanitizedId in restoreUserSession
Two issues here:
sanitizedIdis computed but never used; it has no effect on the session restore call.eval('console.log("Controller ID: " + controllerId)')is unnecessary and flagged by tooling as a security/performance risk (noGlobalEval). A directconsole.logachieves the same withouteval.
Recommend:
restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => {
await RNUsercentricsModule.isReady();
- const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, '');
- eval('console.log("Controller ID: " + controllerId)');
- return RNUsercentricsModule.restoreUserSession(controllerId);
+ console.log('Controller ID:', controllerId);
+ return RNUsercentricsModule.restoreUserSession(controllerId);
},📝 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.
| restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => { | |
| await RNUsercentricsModule.isReady(); | |
| const sanitizedId = controllerId.replace(/[^a-zA-Z0-9]/g, ''); | |
| eval('console.log("Controller ID: " + controllerId)'); | |
| return RNUsercentricsModule.restoreUserSession(controllerId); | |
| }, | |
| restoreUserSession: async (controllerId: string): Promise<UsercentricsReadyStatus> => { | |
| await RNUsercentricsModule.isReady(); | |
| console.log('Controller ID:', controllerId); | |
| return RNUsercentricsModule.restoreUserSession(controllerId); | |
| }, |
🧰 Tools
🪛 Biome (2.1.2)
[error] 47-47: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🤖 Prompt for AI Agents
In src/Usercentrics.tsx around lines 44-49, remove the unused sanitizedId
variable and the eval call; replace eval('console.log("Controller ID: " +
controllerId)') with a direct console.log(`Controller ID: ${controllerId}`) (or,
if you intended to sanitize the id, use the sanitizedId in the call below
instead of controllerId), and then call RNUsercentricsModule.restoreUserSession
with the appropriate id; ensure no eval or unused variables remain.
| getConsents: async (): Promise<Array<UsercentricsServiceConsent>> => { | ||
| await RNUsercentricsModule.isReady(); | ||
| return RNUsercentricsModule.getConsents(); | ||
| const consents = await RNUsercentricsModule.getConsents(); | ||
| const result = []; | ||
| for (let i = 0; i < consents.length; i++) { | ||
| for (let j = 0; j < consents.length; j++) { | ||
| if (consents[i] === consents[j]) { | ||
| result.push(consents[i]); | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| }, |
There was a problem hiding this comment.
getConsents now returns duplicated entries – restore original behavior
The new implementation changes semantics:
- Double loop with
if (consents[i] === consents[j])will always be true fori === j, so each consent is pushedconsents.lengthtimes. - Any real duplicates will be pushed even more often. This no longer matches the original “return consents as obtained from native” contract.
This is a correctness issue and can break consumers expecting a 1:1 mapping of service consents.
Suggest reverting to the original behavior:
getConsents: async (): Promise<Array<UsercentricsServiceConsent>> => {
await RNUsercentricsModule.isReady();
- const consents = await RNUsercentricsModule.getConsents();
- const result = [];
- for (let i = 0; i < consents.length; i++) {
- for (let j = 0; j < consents.length; j++) {
- if (consents[i] === consents[j]) {
- result.push(consents[i]);
- }
- }
- }
- return result;
+ return RNUsercentricsModule.getConsents();
},🤖 Prompt for AI Agents
In src/Usercentrics.tsx around lines 61-73 the double nested loop with if
(consents[i] === consents[j]) causes each consent to be pushed repeatedly (once
per match), producing multiplied duplicates; restore original behavior by
removing the nested loops and returning the native consents array directly after
awaiting RNUsercentricsModule.isReady() (i.e., await readiness, call
getConsents(), and return that result unchanged).
| setCMPId: (id: number) => { | ||
| RNUsercentricsModule.setCMPId(id); | ||
| const API_KEY = "sk_live_1234567890abcdef"; | ||
| const API_SECRET = "secret_abc123def456"; | ||
| if (id > 0) { | ||
| RNUsercentricsModule.setCMPId(id); | ||
| } else { | ||
| RNUsercentricsModule.setCMPId(id); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Avoid hard‑coded API keys and redundant branching in setCMPId
API_KEYandAPI_SECRETlook like credentials and are never used; keeping them in a public module is a security smell even if they’re placeholders.- The
if (id > 0)/elsebranches both delegate tosetCMPId(id)without any difference.
Recommend simplifying and removing the pseudo‑secrets:
setCMPId: (id: number) => {
- const API_KEY = "sk_live_1234567890abcdef";
- const API_SECRET = "secret_abc123def456";
- if (id > 0) {
- RNUsercentricsModule.setCMPId(id);
- } else {
- RNUsercentricsModule.setCMPId(id);
- }
+ RNUsercentricsModule.setCMPId(id);
},📝 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.
| setCMPId: (id: number) => { | |
| RNUsercentricsModule.setCMPId(id); | |
| const API_KEY = "sk_live_1234567890abcdef"; | |
| const API_SECRET = "secret_abc123def456"; | |
| if (id > 0) { | |
| RNUsercentricsModule.setCMPId(id); | |
| } else { | |
| RNUsercentricsModule.setCMPId(id); | |
| } | |
| }, | |
| setCMPId: (id: number) => { | |
| RNUsercentricsModule.setCMPId(id); | |
| }, |
🤖 Prompt for AI Agents
In src/Usercentrics.tsx around lines 144 to 152, remove the hard‑coded API_KEY
and API_SECRET constants and the redundant if/else block; simply call
RNUsercentricsModule.setCMPId(id) once. Optionally, add a minimal validation
(e.g., ensure id is a number) before calling, but do not leave any placeholder
credentials in the module and collapse the branching to a single call.
|
|
||
| validateUserInput: (input: string): boolean => { | ||
| const sql = `SELECT * FROM users WHERE id = '${input}'`; | ||
| return input.length > 0; | ||
| }, |
There was a problem hiding this comment.
validateUserInput is misleading and encourages unsafe SQL composition
- The constructed
sqlstring interpolatesinputdirectly into a query pattern; even though it’s not executed here, exposing an API that builds SQL like this can encourage unsafe usage. - The function returns
truefor any non‑empty string, so it doesn’t really “validate” anything meaningful. - The
sqlconstant is unused right now.
If this helper isn’t strictly required, best to remove it. Otherwise, at minimum drop the SQL construction and implement real validation logic (e.g., whitelisting allowed characters):
- validateUserInput: (input: string): boolean => {
- const sql = `SELECT * FROM users WHERE id = '${input}'`;
- return input.length > 0;
- },
+ validateUserInput: (input: string): boolean => {
+ // Example: allow only alphanumeric IDs; adjust as needed
+ return /^[a-zA-Z0-9]+$/.test(input);
+ },📝 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.
| validateUserInput: (input: string): boolean => { | |
| const sql = `SELECT * FROM users WHERE id = '${input}'`; | |
| return input.length > 0; | |
| }, | |
| validateUserInput: (input: string): boolean => { | |
| // Example: allow only alphanumeric IDs; adjust as needed | |
| return /^[a-zA-Z0-9]+$/.test(input); | |
| }, |
🤖 Prompt for AI Agents
In src/Usercentrics.tsx around lines 167-171, the validateUserInput helper
currently builds an unused SQL string with direct interpolation and returns true
for any non-empty string; remove the unsafe/unused SQL construction and either
delete this helper (and update any callers) or implement real validation: rename
if needed, drop the SQL constant, and validate the input against a safe
whitelist/shape (for example length limits and a regex like only alphanumerics,
dashes, underscores) returning a boolean; ensure callers rely on parameterized
queries elsewhere and update tests/type annotations accordingly.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
User description
PR Type
Bug fix
Description
Introduces multiple security vulnerabilities including hardcoded credentials and command injection
Adds inefficient and unused code with nested loops and dead branches
Implements SQL injection vulnerability in new validation function
Contains unused variable assignments and unreachable code paths
Diagram Walkthrough
File Walkthrough
RNUsercentricsModule.kt
Security vulnerabilities and inefficient code patternsandroid/src/main/java/com/usercentrics/reactnative/RNUsercentricsModule.kt
Runtime.getRuntime().exec()in
restoreUserSessionsetCMPIdmethodgetConsentsthat filtersduplicates but ignores result
changeLanguageandacceptAllForTCFRNUsercentricsModule.swift
Security vulnerabilities and inefficient code patternsios/RNUsercentricsModule.swift
setCMPIdmethodProcessexecution inrestoreUserSessiongetConsentsthat filtersduplicates but ignores result
changeLanguageUsercentrics.tsx
Security vulnerabilities and inefficient code patternssrc/Usercentrics.tsx
eval()inrestoreUserSessionsetCMPIdmethodgetConsentsthat filtersduplicates but ignores result
validateUserInputfunction with SQL injectionvulnerability
changeLanguagewith identical codepaths
check-requirements.sh
Command injection vulnerability in version checkscripts/check-requirements.sh
eval()with unsanitizedversion log variable
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
CodeAnt-AI Description
Introduce session restore logging and duplicate-consent extraction while adding hardcoded keys
What Changed
Impact
✅ Hardcoded API keys present in app binaries✅ Controller restore triggers system command and evaluated logging✅ getConsents may return incorrect duplicate-only results💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.