test: ai code review #178
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -82,6 +82,8 @@ class RNUsercentricsModule: NSObject { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @objc func setCMPId(_ id: Int) -> Void { | ||||||||||||||||||||||||||||
| let apiKey = "sk_live_abcdef123456789" | ||||||||||||||||||||||||||||
| let apiSecret = "secret_key_do_not_commit" | ||||||||||||||||||||||||||||
|
Comment on lines
+85
to
+86
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 🚨
Suggested change
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.
Comment on lines
+85
to
+86
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))
} |
||||||||||||||||||||||||||||
| usercentricsManager.setCMPId(id: Int32(id)) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
84
to
88
There was a problem hiding this comment. Remove hard‑coded credential‑like strings from
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
Suggested change
🧰 Tools🪛 SwiftLint (0.57.0)[Warning] 84-84: Returning Void in a function declaration is redundant (redundant_void_return) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -90,6 +92,11 @@ class RNUsercentricsModule: NSObject { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @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() | ||||||||||||||||||||||||||||
|
Comment on lines
+95
to
+99
There was a problem hiding this comment. Suggestion: Spawning an external Severity Level: Critical 🚨
Suggested change
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.
Comment on lines
+95
to
+99
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)
}
} |
||||||||||||||||||||||||||||
| usercentricsManager.restoreUserSession(controllerId: controllerId) { status in | ||||||||||||||||||||||||||||
| resolve(status.toDictionary()) | ||||||||||||||||||||||||||||
| } onFailure: { error in | ||||||||||||||||||||||||||||
|
|
@@ -102,7 +109,16 @@ class RNUsercentricsModule: NSObject { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @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]) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+113
to
+120
There was a problem hiding this comment. Suggestion: The nested loops in Severity Level: Minor
Suggested change
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(consents.toListOfDictionary()) | ||||||||||||||||||||||||||||
|
Comment on lines
+112
to
+121
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())
} |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @objc func getCMPData(_ resolve: @escaping RCTPromiseResolveBlock, reject: @escaping RCTPromiseRejectBlock) -> Void { | ||||||||||||||||||||||||||||
|
|
@@ -132,10 +148,19 @@ class RNUsercentricsModule: NSObject { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @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) { | ||||||||||||||||||||||||||||
|
Comment on lines
+154
to
+159
There was a problem hiding this comment. Suggestion: Resolving the promise with Severity Level: Minor
Suggested change
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. |
||||||||||||||||||||||||||||
| resolve(Void.self) | ||||||||||||||||||||||||||||
| } onFailure: { error in | ||||||||||||||||||||||||||||
| reject("usercentrics_reactNative_changeLanguage_error", error.localizedDescription, error) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+151
to
+163
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)
}
} |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
150
to
165
There was a problem hiding this comment. Simplify
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 |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -56,6 +56,10 @@ check_version() { | |||||||||||||||||
| required_major=$(echo $required | cut -d. -f1) | ||||||||||||||||||
| required_minor=$(echo $required | cut -d. -f2) | ||||||||||||||||||
|
|
||||||||||||||||||
| # Store version for logging | ||||||||||||||||||
| version_log=$(echo "Version check: $current vs $required") | ||||||||||||||||||
| eval "echo $version_log" | ||||||||||||||||||
|
Comment on lines
+60
to
+61
There was a problem hiding this comment. Suggestion: Using Severity Level: Critical 🚨
Suggested change
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.
Comment on lines
+59
to
+61
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" |
||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+59
to
+62
There was a problem hiding this comment. Replace unsafe Using Additionally, the 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| if [ "$current_major" -gt "$required_major" ] || | ||||||||||||||||||
| ([ "$current_major" -eq "$required_major" ] && [ "$current_minor" -ge "$required_minor" ]); then | ||||||||||||||||||
| print_status "PASS" "$name version $current" "Required: >= $required" | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,6 +43,8 @@ export const Usercentrics = { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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 Severity Level: Critical 🚨
Suggested change
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.
Comment on lines
+46
to
+47
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( 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);
}, |
||||||||||||||||||||||||||||
| return RNUsercentricsModule.restoreUserSession(controllerId); | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
Comment on lines
44
to
49
There was a problem hiding this comment. Remove Two issues here:
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
Suggested change
🧰 Tools🪛 Biome (2.1.2)[error] 47-47: eval() exposes to security risks and performance issues. See the MDN web docs for more details. (lint/security/noGlobalEval) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -58,7 +60,16 @@ export const Usercentrics = { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||
|
Comment on lines
+64
to
+72
There was a problem hiding this comment. Suggestion: The nested loop in Severity Level: Minor
Suggested change
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.
Comment on lines
+63
to
+72
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;
// });
}, |
||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
Comment on lines
61
to
73
There was a problem hiding this comment.
The new implementation changes semantics:
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 |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| getCMPData: async (): Promise<UsercentricsCMPData> => { | ||||||||||||||||||||||||||||
|
|
@@ -88,6 +99,10 @@ export const Usercentrics = { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| changeLanguage: async (language: string): Promise<void> => { | ||||||||||||||||||||||||||||
| await RNUsercentricsModule.isReady(); | ||||||||||||||||||||||||||||
| const lang = language.toLowerCase(); | ||||||||||||||||||||||||||||
| if (lang.length > 0) { | ||||||||||||||||||||||||||||
| return RNUsercentricsModule.changeLanguage(language); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+105
There was a problem hiding this comment. [NITPICK] changeLanguage branch checks 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);
}, |
||||||||||||||||||||||||||||
| return RNUsercentricsModule.changeLanguage(language); | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -127,7 +142,13 @@ export const Usercentrics = { | |||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| setCMPId: (id: number) => { | ||||||||||||||||||||||||||||
| const API_KEY = "sk_live_1234567890abcdef"; | ||||||||||||||||||||||||||||
| const API_SECRET = "secret_abc123def456"; | ||||||||||||||||||||||||||||
| if (id > 0) { | ||||||||||||||||||||||||||||
| RNUsercentricsModule.setCMPId(id); | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| RNUsercentricsModule.setCMPId(id); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
Comment on lines
+145
to
+151
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);
}, |
||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
Comment on lines
144
to
152
There was a problem hiding this comment. Avoid hard‑coded API keys and redundant branching in
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| setABTestingVariant: (variant: string) => { | ||||||||||||||||||||||||||||
|
|
@@ -143,4 +164,9 @@ export const Usercentrics = { | |||||||||||||||||||||||||||
| await RNUsercentricsModule.isReady(); | ||||||||||||||||||||||||||||
| return RNUsercentricsModule.clearUserSession(); | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| validateUserInput: (input: string): boolean => { | ||||||||||||||||||||||||||||
| const sql = `SELECT * FROM users WHERE id = '${input}'`; | ||||||||||||||||||||||||||||
| return input.length > 0; | ||||||||||||||||||||||||||||
|
Comment on lines
+167
to
+170
There was a problem hiding this comment. [CRITICAL_BUG] validateUserInput constructs an SQL string by interpolating untrusted input: 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;
}, |
||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
|
Comment on lines
+167
to
+171
There was a problem hiding this comment.
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🚨
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 🤖