Token Pairing Feature: Address Code Review Feedback from bisq2#4321

[coderabbitai]

Context

This issue tracks code review feedback from the token pairing feature backport to 2.1.8:

• Original PR: Feature/for mobile based on 2.1.8 bring token pairing bisq2#4321
• Review: Feature/for mobile based on 2.1.8 bring token pairing bisq2#4321 (review)

The feedback below should be addressed in the mobile-compatible codebase.

---

🔴 Major Issues (9)

1. SecureRandom Performance Issue

📍 common/src/main/java/bisq/common/util/ByteArrayUtils.java (lines 89-92)

Problem: Creating a new SecureRandom instance on every call causes initialization overhead.

Fix: Replace with a shared static final SecureRandom instance and add input validation:

private static final SecureRandom SECURE_RANDOM = new SecureRandom();

public static byte[] getRandomBytes(int size) {
    if (size < 0) {
        throw new IllegalArgumentException("size must be >= 0");
    }
    byte[] bytes = new byte[size];
    SECURE_RANDOM.nextBytes(bytes);
    return bytes;
}

---

2. ClientSecret Stored in Plaintext

📍 http-api/src/main/java/bisq/http_api/access/identity/ClientProfile.java (lines 26-35)

Problem: clientSecret is stored as plaintext and serialized via protobuf, creating exposure risk if persistence store is compromised.

Fix Options:

• Implement encryption/decryption for persistence layer
• Document and enforce strict file-level protections + rotation policies
• Ensure constant-time comparison is maintained for authentication

---

3. Permission Decoding Error Context

📍 http-api/src/main/java/bisq/http_api/access/pairing/PairingCodeDecoder.java (lines 52-55)

Problem: Permission.fromId() throws IllegalArgumentException for unknown IDs without contextual information.

Fix: Either document the fail-fast behavior or wrap exceptions with context:

for (int i = 0; i < numPermissions; i++) {
    try {
        permissions.add(Permission.fromId(BinaryDecodingUtils.readInt(in)));
    } catch (IllegalArgumentException e) {
        throw new IllegalArgumentException(
            "Invalid permission at index " + i + " in pairing code", e);
    }
}

---

4. Race Condition in Pairing Code Consumption

📍 http-api/src/main/java/bisq/http_api/access/pairing/PairingService.java (lines 83-101)

Problem: Non-atomic get() + remove() allows two concurrent requests to use the same pairing code.

Fix: Use atomic remove():

PairingCode pairingCode = pairingCodeByIdMap.remove(pairingCodeId);
if (pairingCode == null) {
    throw new InvalidPairingRequestException("Pairing code not found or already used");
}

if (isExpired(pairingCode)) {
    throw new InvalidPairingRequestException("Pairing code is expired");
}
// Don't call remove again

---

5. Memory Leak in SessionService

📍 http-api/src/main/java/bisq/http_api/access/session/SessionService.java (lines 27-43)

Problem: sessionTokenBySessionIdMap never evicts expired sessions, causing unbounded memory growth.

Fix Strategy:

1. Add lazy cleanup in find():

public Optional<SessionToken> find(String sessionId) {
    SessionToken token = sessionTokenBySessionIdMap.get(sessionId);
    if (token != null && token.isExpired()) {
        sessionTokenBySessionIdMap.remove(sessionId);
        return Optional.empty();
    }
    return Optional.ofNullable(token);
}

2. Add explicit remove(sessionId) method for logout
3. Add background ScheduledExecutorService to periodically clean expired entries

---

6. Memory Leak in Pairing Code Map

📍 http-api/src/main/java/bisq/http_api/HttpApiService.java (lines 251-262)

Problem: Periodic regeneration adds codes without removing old ones, causing pairingCodeByIdMap to grow unbounded.

Fix: Either:

• Purge expired entries before inserting new code
• Track and remove previous pairing ID when generating new one
• Add dedicated cleanup task via Scheduler

---

7. Inconsistent Error Response Formats

📍 http-api/src/main/java/bisq/http_api/rest_api/endpoints/RestApiBase.java (lines 39-43)

Problem: buildNotFoundResponse returns plain string; buildErrorResponse returns JSON {"error": "..."}.

Fix: Unify to JSON structure:

protected Response buildNotFoundResponse(String message) {
    return buildErrorResponse(Response.Status.NOT_FOUND, message);
}

---

8. Null Headers in WebSocketRestApiRequest

📍 http-api/src/main/java/bisq/http_api/web_socket/rest_api_proxy/WebSocketRestApiRequest.java (lines 50-52)

Problem: headers field can be null when omitted from JSON, causing NPEs downstream.

Fix: Initialize to non-null:

private Map<String, String> headers = new HashMap<>();

---

9. Sensitive Data in ToString

📍 http-api/src/main/java/bisq/http_api/web_socket/rest_api_proxy/WebSocketRestApiRequest.java (lines 33-52)

Problem: Lombok @ToString includes sensitive fields (headers, authToken, authTs, authNonce, body).

Fix: Exclude from toString:

@ToString(exclude = {"authToken", "authTs", "authNonce", "headers", "body"})

---

🟡 Minor Issues & Nitpicks (17)

View minor issues and code quality suggestions

Configuration & Validation

1. PairingConfig: Validate sessionTtlInMinutes > 0 in from() method
2. http_api_app.conf: Consider defaulting writePairingQrCodeToDisk to false for production security

Code Quality

3. RestApiBase: buildOkResponse should delegate to buildResponse for DRY
4. HttpApiAuthFilter: Lower log level from info to debug for unauthenticated endpoint checks
5. InvalidPairingRequestException: Add cause-chaining constructor
6. InvalidSessionRequestException: Add cause-chaining constructor
7. PairingQrCodeGenerator: Remove unused @Slf4j annotation
8. ClientProfile: Consider excluding clientSecret from @EqualsAndHashCode to prevent timing attacks

Binary Encoding

9. BinaryDecodingUtils: Default maxLength of Integer.MAX_VALUE misleading (actual limit is 65535)
10. PairingCodeEncoder: Error message says "pairing QR" but encodes "pairing code"
11. PairingCodeDecoder: Same error message inconsistency

Protobuf

12. api.proto: ApiAccessStore skips field index 1; add reserved 1; if intentional

Permissions

13. RestPermissionMapping: String.replace() replaces all occurrences; use startsWith() + substring() instead
14. ApiAccessStore: Deep-copy permission sets in getClone() to prevent snapshot drift
15. ApiAccessStoreService: Copy permissions on write to avoid mutability leaks

Testing

16. PairingServiceTest: Wrap Files.readString() in utility for Android API compatibility
17. Various files: Consider adding more comprehensive unit tests

---

Priority Recommendation

High Priority (fix before mobile release):

• Issues README: getting started improvements based on first new dev joining the project experience #4, Add Bisq 2 library dependencies #5, Integrate full node POC code into Bisq-mobile project #6 (concurrency + memory leaks)
• Issue Initial App Design #2 (security - plaintext secrets)
• Issue Define concept for native mobile notifications #8, Support to build-up reputation for sellers #9 (security - sensitive data exposure)

Medium Priority:

• Issues Adapt a leaf Bisq2 module jar and integrate into the app #1, Framework Setup #3, Node Tor Support #7 (correctness & consistency)

Low Priority:

• All nitpick items (code quality improvements)

---

Related

• Backport PR: Feature/for mobile based on 2.1.8 bring token pairing bisq2#4321
• Original feature: Api pairing session handling and tls bisq2#4263

[rodvar]

@wodoro would you like to take this one?

[rodvar]

@salottodev is doing a full reimpl of the onboarding that is full of bugs after the change from password based + URL impl to pairing token impl

I think I'll wait for him to deliver that before engaging into this one which could potentially bring conflicts.

[rodvar]

this might have been cleared in the PR above, just leaving open to review + bring bugfixing to main/2.1.9 branches

[rodvar]

1. creating a SecureRandom has significnt overhead, I would say the proposed is more of a performance improvement rather than security fix
2. 👍
3. low prio , in prod logs are restrained
4. 👍
5. 👍 (add lazy cleanup to find()
6. 👍 (purge perioidically)
7. no vulnerability here , just API usability
8. 👍 null safety
9. 👍 add exclusions