BOM_1.5.2

[jakubuid]

No description provided.

[claude]

Claude finished @jakubuid's task in 2m 27s —— View job

---

Code Review Complete

Reviewed PR #242: BOM_1.5.2 (67 files, 3143 additions, 42 deletions)

---

Issues Found

Issue 1: Potential device serial exposure in logs

ID: posapplicati-device-serial-exposure-a7c3
File: sample/pos/src/main/kotlin/com/walletconnect/sample/pos/POSApplication.kt:19
Severity: MEDIUM
Category: security

Using Build.SERIAL in device ID construction exposes hardware serial numbers which may be considered PII. On Android 8.0+ (API 26+), accessing Build.SERIAL requires READ_PHONE_STATE permission and returns "unknown" without it. This could cause collision issues with multiple devices.

Recommendation: Use a more privacy-friendly approach:

// Replace Build.SERIAL with a randomly generated UUID stored in SharedPreferences
val deviceId = context.getSharedPreferences("pos_prefs", Context.MODE_PRIVATE)
    .getString("device_id", null) ?: UUID.randomUUID().toString()
    .also { context.getSharedPreferences("pos_prefs", Context.MODE_PRIVATE)
        .edit().putString("device_id", it).apply() }

---

Issue 2: Hardcoded test merchant ID in production code

ID: posapplicati-test-merchant-id-9f82
File: sample/pos/src/main/kotlin/com/walletconnect/sample/pos/POSApplication.kt:23
Severity: LOW
Category: code_quality

The merchant ID "wc_merchant_id_test_1" is hardcoded and appears to be a test value in the sample application initialization code.

Recommendation: Move to BuildConfig or configuration:

merchantId = BuildConfig.MERCHANT_ID

And add to build.gradle.kts:

buildConfigField("String", "MERCHANT_ID", "\"${System.getenv("MERCHANT_ID") ?: "wc_merchant_id_test_1"}\"")

---

Issue 3: Infinite polling loop without exponential backoff

ID: apiclient-infinite-poll-no-backoff-c291
File: product/pos/src/main/kotlin/com/walletconnect/pos/api/ApiClient.kt:96-125
Severity: MEDIUM
Category: performance

The startPolling function uses server-provided pollInMs without any exponential backoff or maximum delay cap. If the server sends very short polling intervals or polling continues indefinitely due to a bug, this could cause excessive API calls and battery drain.

Recommendation: Add exponential backoff with max delay:

private suspend fun startPolling(
    paymentId: String,
    onEvent: (Pos.PaymentEvent) -> Unit
) {
    var lastEmittedStatus: String? = null
    val minDelay = 1000L // 1 second
    val maxDelay = 30000L // 30 seconds
    
    while (true) {
        when (val result = getPaymentStatus(paymentId)) {
            is ApiResult.Success -> {
                val data = result.data
                if (data.status != lastEmittedStatus) {
                    lastEmittedStatus = data.status
                    onEvent(mapStatusToPaymentEvent(data.status, paymentId))
                }
                if (data.isFinal || data.pollInMs == null) break
                
                // Clamp polling interval between min and max
                val pollDelay = data.pollInMs.coerceIn(minDelay, maxDelay)
                delay(pollDelay)
            }
            is ApiResult.Error -> {
                onEvent(mapErrorCodeToPaymentError(result.code, result.message))
                break
            }
        }
    }
}

---

Issue 4: No input validation on payment amount

ID: pos-amount-no-validation-8d41
File: product/pos/src/main/kotlin/com/walletconnect/pos/Pos.kt:14
Severity: MEDIUM
Category: code_quality

The Amount.format() function throws an IllegalStateException when value is not a valid long, but this validation only occurs during formatting. Invalid amounts could be sent to the API before validation occurs, potentially causing server errors or security issues.

Recommendation: Add validation in Amount constructor or factory:

data class Amount(
    val unit: String,
    val value: String
) {
    init {
        require(unit.isNotBlank()) { "Currency unit cannot be blank" }
        require(value.toLongOrNull() != null) { "Amount value must be a valid long integer" }
        require(value.toLong() >= 0) { "Amount cannot be negative" }
    }
    
    fun format(): String {
        val currency = unit.substringAfter("/", "")
        val amount = value.toLong() // Safe now due to init validation
        val majorUnits = amount / 100.0
        return String.format("%.2f %s", majorUnits, currency)
    }
}

---

Issue 5: Missing timeout for payment status check

ID: posclient-checkpayment-no-timeout-5b19
File: product/pos/src/main/kotlin/com/walletconnect/pos/PosClient.kt:76-83
Severity: LOW
Category: code_quality

The checkPaymentStatus suspend function has no timeout configured. If the API call hangs, the caller will wait indefinitely.

Recommendation: Add timeout using withTimeout:

suspend fun checkPaymentStatus(paymentId: String): Pos.PaymentEvent {
    checkInitialized()
    
    return withTimeout(30_000) { // 30 second timeout
        when (val result = apiClient!!.getPaymentStatus(paymentId)) {
            is ApiResult.Success -> mapStatusToPaymentEvent(result.data.status, paymentId)
            is ApiResult.Error -> mapErrorCodeToPaymentError(result.code, result.message)
        }
    }
}

---

Issue 6: TODO comment for missing error message functionality

ID: mapping-todo-error-message-7f93
File: product/pos/src/main/kotlin/com/walletconnect/pos/api/Mapping.kt:27
Severity: LOW
Category: code_quality

TODO comment indicates incomplete implementation: //TODO: add error message? for payment failed status. This means users receive a generic "Payment failed" message without specific failure reasons.

Recommendation: Complete the implementation by extracting error details from the API response or remove the TODO if the API doesn't provide detailed error messages.

---

Issue 7: Null-safety issue with non-null assertion operator

ID: posclient-force-unwrap-apiClient-4e21
File: product/pos/src/main/kotlin/com/walletconnect/pos/PosClient.kt:62,79
Severity: LOW
Category: code_quality

Using apiClient!! (non-null assertion) after checkInitialized() is fragile. While checkInitialized() throws if null, using !! can mask issues if initialization logic changes.

Recommendation: Use safe calls or local non-null variables:

fun createPaymentIntent(amount: Pos.Amount, referenceId: String) {
    checkInitialized()
    val client = apiClient ?: error("ApiClient not initialized")
    currentPollingJob?.cancel()
    currentPollingJob = scope?.launch {
        client.createPayment(referenceId, amount.unit, amount.value) { event ->
            emitEvent(event)
        }
    }
}

---

Automated Checks

✅ No external domain URLs detected (all URLs point to approved domains: reown.com, walletconnect.com, walletconnect.org, or are Android XML namespaces)

✅ No static resource cache-control issues detected

✅ No GitHub Actions workflow security issues (no workflow files changed)

---

Summary

Found 7 issues in the new POS SDK module:

• 0 HIGH severity issues
• 3 MEDIUM severity issues (device serial exposure, infinite polling without backoff, no amount validation)
• 4 LOW severity issues (hardcoded test merchant, missing timeout, incomplete TODO, null assertion usage)

The new POS SDK implementation is generally well-structured with good separation of concerns, proper error handling patterns, and comprehensive test coverage for initialization and edge cases. The main concerns are around input validation, polling behavior, and privacy considerations for device identification.

Core protocol changes in foundation/ and core/android/ are minor and don't introduce security issues.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud