PR Review: DRO-708 - Network Mode with WebSocket

This is a substantial PR implementing network mode with WebSocket support, reverse connections, and gesture controls. Overall, the implementation is solid with good architectural decisions.

✅ Strengths

Architecture & Design

• Excellent separation of concerns: ActionDispatcher centralizes command routing for both HTTP and WebSocket
• Clean abstraction: GestureController provides a well-designed singleton for gesture operations
• ViewBinding adoption: Modernizes UI code and improves type safety
• String resources: Good internationalization practices
• Test coverage: Added unit tests for critical components

Code Quality

• Consistent use of constants: Magic numbers replaced with named constants
• Kotlin idioms: Good use of KTX extensions and idiomatic patterns
• Error handling: Comprehensive try-catch blocks with logging
• Documentation: Clear KDoc comments on key classes

🔒 Security Concerns

Critical Issues

1. Hardcoded User ID (ReverseConnectionService.kt:80)

   • ⚠️ CRITICAL: Hardcoded UUID 7785b089-b9aa-458d-a32e-baec315e5e16 appears to be a test value
   • Should be removed, loaded from configuration, or generated per-device

2. Auth Token on Clipboard (MainActivity.kt:138)

   • Copying sensitive tokens to clipboard exposes them to monitoring apps
   • Consider adding a warning toast or confirmation dialog

3. Ping Endpoint Without Auth (SocketServer.kt:140)

   • Unauthenticated /ping enables reconnaissance and potential DoS attacks
   • Recommendation: Require auth or implement rate limiting

4. WebSocket Auth Token in Query String (PortalWebSocketServer.kt:46)

   • Tokens in query params can be logged and leaked via Referer headers
   • Recommendation: Prefer header-based auth only

5. CORS Wildcard (SocketServer.kt:255, 319)

   • Access-Control-Allow-Origin: * allows any origin
   • Consider limiting to specific origins

Medium Priority

6. No Request Size Limits: POST_BODY_BUFFER_SIZE = 1024 doesn't enforce upper limit - memory exhaustion risk

7. Auth Token Generation (ConfigManager.kt:42): UUIDs are less secure than SecureRandom tokens

🐛 Potential Bugs

High Priority

1. Race Condition in ReverseConnectionService (ReverseConnectionService.kt:145-152)

   • actionDispatcher initialization check happens on every message but only initializes once
   • If accessibility service isn't ready on first message, all subsequent messages fail
   • Should initialize in onCreate() with retry logic

2. Screenshot Encode/Decode Cycle (ApiHandler.kt:232-234)

   • Inefficient double Base64 encoding - Service should return bytes directly
   • Adds ~33% overhead

3. Resource Leak (SocketServer.kt:94)

   • Socket not closed if executorService?.submit() throws
   • Fix: executorService?.submit { handleClient(clientSocket) } ?: clientSocket.close()

Medium Priority

4. Silent Failures in getPackages (ApiHandler.kt:94-97)

   • Package failures silently skipped - no visibility to users

5. Misleading Log (ReverseConnectionService.kt:106-107)

   • Says "connection established" immediately after async connect() call
   • Should say "connection initiated"

6. No Backoff in Reconnect (ReverseConnectionService.kt:23)

   • Fixed 3-second delay could cause excessive load and drain battery
   • Recommendation: Exponential backoff (3s → 6s → 12s → max 60s)

⚡ Performance Considerations

1. Inefficient Package Enumeration (ApiHandler.kt:73-140)

   • getPackages() queries PackageManager synchronously on API thread
   • Can take 100s of ms for 100+ apps
   • Recommendation: Cache results with BroadcastReceiver invalidation on package changes

2. Screenshot Encoding Overhead: Double Base64 encode/decode adds ~33% overhead

3. Handler on Main Looper (ReverseConnectionService.kt:32)

   • Reconnect scheduling doesn't need main thread
   • Consider background handler or coroutines

4. Synchronous Gesture Dispatch (GestureController.kt:29)

   • No timeout enforced - could block API thread if system is slow
   • Consider adding timeout or making API explicitly async

5. String Concatenation: HTTP header building in loops (SocketServer.kt:252-257, 316-321)

6. JSON Parsing: For high-throughput scenarios, consider more efficient library (Moshi, kotlinx.serialization)

🧪 Test Coverage

Good Coverage ✅

• ActionDispatcherTest: Parameter normalization, defaults, error cases
• EventHubTest: Filtering and subscription logic
• PortalEventTest: JSON serialization

Missing Coverage ❌

• No tests for: GestureController, ReverseConnectionService, PortalWebSocketServer, ApiHandler, ConfigManager
• No integration tests for WebSocket → ActionDispatcher → ApiHandler → AccessibilityService pipeline
• No instrumented tests despite heavy Android system service interaction

Recommendation: Aim for 60%+ code coverage. Network mode is a critical path that needs thorough testing.

📝 Code Quality Issues

1. Typo (PortalWebSocketServer.kt:28): UNAUTORIZED → UNAUTHORIZED (missing H)

2. Magic Number (PortalWebSocketServer.kt:27): EXPECTED_REQUEST_ID_BYTES = 36 - add comment explaining UUID string format

3. TODO Comments: Multiple TODOs should be tracked as issues or resolved before merge:

   • ReverseConnectionService.kt:69
   • SocketServer.kt:167, 245
   • PortalWebSocketServer.kt:31

4. Inconsistent Error Messages: Consider standardizing format across codebase

🎯 Recommendations

Before Merge (Critical)

1. 🔴 MUST FIX: Remove hardcoded User ID in ReverseConnectionService.kt:80
2. 🔴 Fix race condition in ReverseConnectionService actionDispatcher initialization
3. 🟡 Add exponential backoff to reconnect logic
4. 🟡 Fix resource leak in SocketServer socket handling
5. 🟡 Correct misleading "connection established" log message
6. 🟡 Fix typo: UNAUTORIZED → UNAUTHORIZED

Post-Merge Improvements

1. Add integration tests for WebSocket command pipeline
2. Implement caching for getPackages() to improve performance
3. Refactor screenshot handling to avoid double encoding
4. Implement rate limiting for unauthenticated endpoints
5. Add request size limits to prevent DoS
6. Review auth token generation to use SecureRandom
7. Resolve all TODO comments or create tracking issues

📊 Summary

Overall: Well-architected feature that significantly enhances portal capabilities. The hardcoded User ID is critical and must be fixed before merge. Other issues are manageable as post-merge improvements. Great job on refactoring and test additions! 🎉

Code Review: PR #33 - Network Mode with WebSocket Implementation

Overview

This PR introduces WebSocket and HTTP network communication as an alternative to ADB, adding significant new functionality including gesture control, reverse connections, and improved API response handling. Overall, the implementation is solid with good test coverage for new components.

✅ Strengths

Code Quality

• Well-structured architecture: The ActionDispatcher provides excellent separation of concerns, eliminating duplication between HTTP and WebSocket handlers
• Good test coverage: New classes have unit tests (ActionDispatcherTest, EventHubTest, PortalEventTest)
• Clean abstractions: GestureController as a singleton provides a clean API for gesture operations
• Error handling: Comprehensive error handling in ApiHandler.getPackages() and startApp()

Design Decisions

• View Binding: Good modernization to enable view binding
• Response types: Expanded ApiResponse sealed class with Binary, RawArray, RawObject provides type-safe handling
• Proper lifecycle management: ReverseConnectionService handles WebSocket lifecycle with reconnection logic

🔴 Critical Issues

1. SECURITY: Hardcoded User ID (ConfigManager.kt:80, ReverseConnectionService.kt:80)

headers["X-User-ID"] = "7785b089-b9aa-458d-a32e-baec315e5e16"

Impact: Critical security vulnerability
Issue: Hardcoded UUID appears to be test/development data
Fix: Remove or make this configurable. If this is legitimate, add a comment explaining why it's hardcoded

2. Memory Leak Risk (ReverseConnectionService.kt:32)

private val handler = Handler(Looper.getMainLooper())

Impact: Handler holds implicit reference to Service
Issue: Handler created with main looper without proper cleanup
Fix:

• Clear pending callbacks in onDestroy(): handler.removeCallbacksAndMessages(null)
• Consider using WeakReference or a static Handler

3. ConfigManager State Management (ConfigManager.kt:150)

var reverseConnectionEnabled: Boolean = false

Impact: State inconsistency bug
Issue: Non-persisted property in a persistence manager. Every instance will reset to false, ignoring the init block check
Fix: Either persist this property or move it outside ConfigManager

4. Screenshot Double Encoding (ApiHandler.kt:227-229)

// Result is Base64 string from Service. 
// decode it back to bytes to pass as Binary response.
val bytes = android.util.Base64.decode(result, android.util.Base64.DEFAULT)

Impact: Performance inefficiency
Issue: Encode → decode → re-encode cycle wastes CPU/memory
Fix: As the TODO suggests, have Service return bytes directly

⚠️ Major Issues

5. Deprecated API Usage (ApiHandler.kt:126)

val versionCode = pkgInfo.longVersionCode

Impact: Will fail on API < 28
Issue: Removed version check for longVersionCode which is only available on API 28+
Fix: Restore the API check:

val versionCode = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
    pkgInfo.longVersionCode
} else {
    @Suppress("DEPRECATION")
    pkgInfo.versionCode.toLong()
}

6. Inconsistent Error Handling (ReverseConnectionService.kt:66-72)

if (hostUrl.isBlank()) {
    Log.w(TAG, "No host URL configured")
    // Don't stop self, maybe user will config later? 
    // Or stop and let UI restart it.
    return
}

Impact: Service runs indefinitely doing nothing
Issue: Unclear behavior when misconfigured
Fix: Decide on behavior - either stop service or notify user

7. Race Condition (ReverseConnectionService.kt:145-152)

if (!::actionDispatcher.isInitialized) {
    val service = DroidrunAccessibilityService.getInstance()
    // ...
    actionDispatcher = service.getActionDispatcher()
}

Impact: Potential crashes if accessibility service isn't ready
Issue: Late initialization on first message could fail
Fix: Initialize in onCreate() or return error response if not ready

8. Inconsistent SharedPreferences Usage (ConfigManager.kt)

Impact: Code inconsistency
Issue: Mixes edit { } Kotlin extension (lines 52, 64, etc.) with manual edit().apply() (line 133)
Fix: Consistently use edit { } throughout

💡 Minor Issues & Suggestions

9. Missing Resource Cleanup

• ActionDispatcherTest: Uses MockK but doesn't call unmockkAll() in teardown
• Consider adding @After method to clean up mocks

10. Magic Numbers

• GestureController.kt:13: TAP_DURATION_MS = 50L - consider if this should be configurable
• ReverseConnectionService.kt:23: RECONNECT_DELAY_MS = 3000L - consider exponential backoff for production

11. Code Formatting

Some inconsistent formatting (extra blank lines, inconsistent spacing). Consider running ktlint/detekt.

12. Logging in Production

ApiHandler.getPackages() adds extensive logging. Consider using a logging level that can be disabled in release builds.

13. Singleton Anti-pattern (ConfigManager.kt:39)

// TODO replace
@Volatile
private var INSTANCE: ConfigManager? = null

The TODO acknowledges this. Consider dependency injection (Hilt/Koin) for better testability.

14. Incomplete API Version Check (AccessibilityTreeBuilder.kt:70)

put("hint", node.hintText?.toString() ?: "")
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CO

Impact: Syntax error, won't compile
Issue: Truncated version check in the diff
Fix: Verify this compiles correctly

📊 Test Coverage

Good:

• New ActionDispatcher, EventHub, PortalEvent have tests
• Tests cover edge cases (null handling, empty strings, aliases)

Gaps:

• No tests for GestureController (difficult to test without instrumentation)
• No tests for ReverseConnectionService
• No tests for new ApiHandler methods (performTap, performSwipe, etc.)
• No tests for ApiResponse.Binary equality/hashCode

Recommendation: Add instrumented tests for gesture operations and integration tests for the WebSocket flow.

🔒 Security Assessment

Concerns:

1. Hardcoded credentials (User ID) - MUST be addressed
2. Authentication tokens: Auto-generated UUIDs in ConfigManager are good, but ensure these are validated server-side
3. WebSocket security: No TLS validation visible - ensure WSS is used in production
4. No rate limiting: Consider adding rate limiting for API endpoints to prevent abuse
5. Intent creation: startApp() properly validates and sanitizes package/activity names ✅

Recommendations:

• Add authentication middleware for WebSocket connections
• Validate all user inputs (coordinates, durations, package names)
• Consider adding intent verification before launching activities

🎯 Performance Considerations

1. Screenshot base64 round-trip: Address as noted in Issue Added semantic text to interactive elements and adjust rectangle appearance  #4
2. Main thread operations: Handler uses main looper for network callbacks - consider background thread
3. Package enumeration: getPackages() could be slow with many apps - consider caching
4. WebSocket reconnection: Fixed delay instead of exponential backoff could hammer server

📝 Summary

This is a solid implementation that adds important functionality. The code is generally well-structured with good separation of concerns. However, there are critical issues that must be addressed before merging:

Must Fix Before Merge:

1. Remove hardcoded User ID (security vulnerability)
2. Fix reverseConnectionEnabled state management bug
3. Restore API version check for longVersionCode
4. Add handler cleanup in ReverseConnectionService.onDestroy()

Should Fix:

5. Decide on error handling strategy when reverse connection URL is blank
6. Fix race condition in actionDispatcher initialization
7. Make SharedPreferences usage consistent

Nice to Have:

8. Add more test coverage
9. Implement exponential backoff for reconnection
10. Address screenshot double-encoding TODO

Recommendation: Request changes for critical issues, then approve after fixes.

Great work overall! The architecture choices are sound and this significantly expands the app's capabilities. 🚀

Pull Request Review: DRO-708 - Network Mode with WebSocket

Overview

This PR introduces significant new functionality for WebSocket communication, reverse connections, and gesture-based actions. The changes are well-structured with good separation of concerns.

✅ Strengths

1. Architecture & Design

• ActionDispatcher pattern: Excellent abstraction for handling commands from both HTTP and WebSocket layers
• GestureController as singleton: Clean separation of gesture logic from the accessibility service
• Response type hierarchy: Enhanced ApiResponse sealed class with Binary, RawObject, and RawArray variants is well-designed

2. Test Coverage

• Good unit tests for ActionDispatcher, EventHub, and event models
• Tests cover edge cases like null activity handling, method aliases, and default parameters
• Proper use of MockK for clean, maintainable tests

3. Code Quality

• Consistent Kotlin idioms (data classes, sealed classes, companion objects)
• Good use of extension functions and lambda expressions
• Proper logging throughout for debugging

🔴 Critical Issues

1. Security: Hardcoded Credentials (ConfigManager.kt:223)

val userID: String
    get() {
        return "7785b089-b9aa-458d-a32e-baec315e5e16"
    }

Issue: Hardcoded user ID is a security vulnerability.
Recommendation: Generate dynamically like deviceID or load from secure configuration.

2. Security: WebSocket Authentication (PortalWebSocketServer.kt:58)

Issues:

• Auth token is auto-generated UUID (ConfigManager.kt:42), which is weak for production
• Typo in constant name: UNAUTORIZED → UNAUTHORIZED (line 28)
  Recommendation: Add TODO about implementing proper token management for production

3. Thread Safety: Service Lifecycle (ReverseConnectionService.kt:153)

if (!::actionDispatcher.isInitialized) {
    val service = DroidrunAccessibilityService.getInstance()
    actionDispatcher = service.getActionDispatcher()
}

Issue: Not thread-safe. Multiple concurrent messages could cause race conditions.
Recommendation: Use lazy delegation or synchronize initialization.

⚠️ High Priority Issues

4. Error Handling: Silent Failures (ApiHandler.kt:67-73)

The getPackages() method logs errors but doesn't include them in response.
Recommendation: Collect errors and return in response metadata so clients know which packages failed.

5. Resource Management: Handler Leak (ReverseConnectionService.kt:32)

private val handler = Handler(Looper.getMainLooper())

Issue: onDestroy() doesn't remove pending callbacks, causing potential memory leak.
Fix:

override fun onDestroy() {
    super.onDestroy()
    isServiceRunning.set(false)
    handler.removeCallbacksAndMessages(null)  // Add this
    disconnect()
    Log.i(TAG, "Service Destroyed")
}

6. Incomplete TODO (ConfigManager.kt:50-52)

val deviceCountryCode: String
    get() {
        // TODO:
        return "de"
    }

Recommendation: Implement proper locale detection or document why "de" is the default.

💡 Medium Priority Suggestions

8. Performance: Base64 Double Encoding (ApiHandler.kt:224-228)

Good job documenting the technical debt! Consider filing an issue to track refactoring screenshot handling to avoid encode/decode cycle.

9. Code Duplication: ApiResponse.toJson() (ApiResponse.kt:25-74)

Two toJson() methods with nearly identical logic. Consider refactoring with optional parameter:

fun toJson(id: Int? = null): String = when (this) {
    is Success -> JSONObject().apply {
        id?.let { put("id", it) }
        put("status", "success")
        put("result", data)
    }.toString()
    // ... rest of cases
}

10. Magic Numbers (GestureController.kt:13-16)

Constants are well-named, but add comments explaining WHY these values were chosen (e.g., "50ms tap duration matches Android system behavior").

11. Inconsistent Error Messages (PortalWebSocketServer.kt:97-101)

UUID size warning is logged but no error response sent to client. Consider rejecting invalid requests.

🐛 Minor Issues

12. Typo (PortalWebSocketServer.kt:28)

UNAUTORIZED should be UNAUTHORIZED (missing 'H')

13. Inconsistent Naming (ActionDispatcher.kt:48-55)

Variable named pkg but parameter is "package". Consider consistent naming.

14. Removed Test (ExampleUnitTest.kt)

Example test deleted (17 lines). Fine for cleanup, but verify it didn't contain useful tests.

15. ViewBinding Enabled (build.gradle.kts:20-21)

✅ Good: Modern Android practice. Ensure all activities/fragments use ViewBinding consistently.

📋 Testing Recommendations

Missing Test Coverage:

1. ReverseConnectionService: No tests for WebSocket reconnection, message handling, lifecycle
2. GestureController: No tests for gesture dispatch (may require instrumented tests)
3. PortalWebSocketServer: Authentication flow untested (TODO on line 31)
4. Binary message handling: Binary response path in PortalWebSocketServer untested

Improvements:

• Add integration tests for full WebSocket request/response cycle
• Test edge cases in ActionDispatcher (malformed JSON, missing params)
• Test thread safety of ReverseConnectionService.handleMessage()

🔒 Security Summary

🎯 Performance Considerations

1. Base64 double encoding: Screenshot data encoded/decoded unnecessarily (documented as tech debt)
2. Handler on main looper: Reconnection logic uses main thread - consider background thread for network ops
3. Synchronous JSON parsing: All parsing on receiving thread - for high-frequency events, consider async parsing

✨ Overall Assessment

Quality Score: 7.5/10

Pros:

• Well-structured architecture with good separation of concerns
• Thoughtful abstraction layers (ActionDispatcher, GestureController)
• Good test coverage for core business logic
• Proper use of Kotlin idioms

Cons:

• Security issues need addressing before production
• Missing tests for new service components
• Incomplete TODOs and hardcoded values
• Thread safety concerns in service lifecycle

Recommendation - Nearly ready for merge with these fixes:

1. ⚠️ Fix hardcoded user ID (critical)
2. ⚠️ Fix Handler memory leak in ReverseConnectionService (high priority)
3. ⚠️ Fix typo: UNAUTORIZED → UNAUTHORIZED
4. 📝 Add TODO comments for auth token improvement
5. 📝 Add thread safety to actionDispatcher initialization

Great work overall! The architecture is solid and the new functionality is well-implemented. 🚀

Pull Request Review: DRO-708 - Network Mode with WebSocket

Overview

This is a significant feature addition that implements network mode with WebSocket support, reverse connection capabilities, gesture control, and binary screenshot handling. The PR adds ~1,941 lines across 29 files with good architectural patterns.

Positive Highlights

Architecture & Design

• Excellent separation of concerns: ActionDispatcher provides a clean abstraction layer for command routing used by both HTTP and WebSocket servers
• Well-structured gesture control: GestureController as a singleton object is appropriate for stateless gesture operations
• Good use of Kotlin idioms: ViewBinding, KTX extensions, and use blocks for resource management
• Proper async handling: Uses AtomicBoolean for thread-safe state management

Code Quality

• Magic numbers eliminated: Constants like SCREENSHOT_TIMEOUT_SECONDS, RECONNECT_DELAY_MS, etc. improve readability
• Resource management: String resources properly externalized from layouts
• Error handling: Comprehensive try-catch blocks with proper logging throughout

Testing

• Good unit test coverage for core components (ActionDispatcherTest, EventHubTest)
• Tests use mockk effectively and cover edge cases (null handling, aliases, defaults)

Issues & Concerns

1. Security Vulnerabilities

Critical: Hardcoded User Credentials

File: ConfigManager.kt:221-223

val userID: String
    get() {
        return "7785b089-b9aa-458d-a32e-baec315e5e16"
    }

This is a critical security issue. A hardcoded UUID means all devices share the same user ID, which:

• Completely breaks user isolation
• Makes it trivial to impersonate any user
• Creates authentication bypass vulnerabilities

Recommendation: Remove hardcoded value and implement proper user ID generation/storage or external configuration.

Weak Authentication Token Generation

File: ConfigManager.kt:54-61

val authToken: String
    get() {
        var token = sharedPrefs.getString(KEY_AUTH_TOKEN, null)
        if (token == null) {
            token = java.util.UUID.randomUUID().toString()
            sharedPrefs.edit { putString(KEY_AUTH_TOKEN, token) }
        }
        return token
    }

Issues:

• UUIDs are not cryptographically secure tokens
• Auto-generation without user awareness creates security risks
• No token rotation mechanism
• Comment says "TODO add external injection from some config file" but not implemented

Recommendation: Use SecureRandom for cryptographic token generation, add expiration/rotation, and require explicit configuration.

Incomplete Authorization in ReverseConnectionService

File: ReverseConnectionService.kt:62-78

The buildHeaders() method only adds auth headers but there's no verification that the remote server validates them. The connection succeeds even if headers are ignored.

2. Resource Leaks & Memory Issues

Missing WebSocket Cleanup

File: ReverseConnectionService.kt:140-148

private fun disconnect() {
    try {
        webSocketClient?.close()
        webSocketClient = null
    } catch (e: Exception) {
        Log.e(TAG, "Error closing connection", e)
    }
}

The reconnect handler in scheduleReconnect() is never cancelled when the service is destroyed. If the service is destroyed while a reconnect is pending, the handler will attempt to reconnect after the service is gone.

Recommendation: Add handler.removeCallbacksAndMessages(null) in onDestroy() (which you do have, but ensure it happens before disconnect()).

Potential Handler Leak

File: ReverseConnectionService.kt:32

private val handler = Handler(Looper.getMainLooper())

Using main looper for network operations can cause UI jank. Consider using a background HandlerThread.

3. Data Handling Issues

Inefficient Screenshot Encoding/Decoding

File: ApiHandler.kt:224-229

// Result is Base64 string from Service. 
// decode it back to bytes to pass as Binary response.
// In future, Service should return bytes directly to avoid this encode/decode cycle.
val bytes = android.util.Base64.decode(result, android.util.Base64.DEFAULT)
ApiResponse.Binary(bytes)

This double-encode/decode cycle wastes CPU and memory. The TODO comment acknowledges this but it should be fixed now.

Recommendation: Refactor StateRepository.takeScreenshot() to return bytes directly instead of Base64 string.

Missing Input Validation

File: ActionDispatcher.kt:28-31

"tap" -> {
    val x = params.optInt("x", 0)
    val y = params.optInt("y", 0)
    apiHandler.performTap(x, y)
}

No validation that coordinates are within screen bounds. Malicious or buggy clients could send negative values or values exceeding screen dimensions.

Recommendation: Add coordinate validation against actual screen dimensions.

4. Error Handling & Logging

Silent Failures in Package Enumeration

File: ApiHandler.kt:96-112

for (resolveInfo in resolvedApps) {
    try {
        // ... package processing
    } catch (e: Exception) {
        Log.w("ApiHandler", "Skipping package ${resolveInfo.activityInfo.packageName}: ${e.message}")
    }
}

Individual package failures are caught and logged but not reported to the client. The client has no way to know some packages were skipped.

Recommendation: Consider collecting errors and including them in the response metadata.

Incomplete Error Context

File: ReverseConnectionService.kt:178-180

} catch (e: Exception) {
    Log.e(TAG, "Error processing message", e)
}

Errors are logged but no error response is sent back to the server. The server will timeout waiting for a response.

Recommendation: Send error responses even when exceptions occur.

5. Code Smells & Maintainability

Inconsistent API Response Format

The PR mixes different response formats:

• ApiResponse.Success(data)
• ApiResponse.RawObject(json)
• ApiResponse.RawArray(json)
• ApiResponse.Binary(bytes)

While this works, the serialization in toJson() creates inconsistent JSON structures. Some have "result", others have "data".

Recommendation: Standardize on a single response wrapper format.

ConfigManager Initialization Side Effect

File: ConfigManager.kt:32-36

init {
    if (sharedPrefs.contains(KEY_REVERSE_CONNECTION_ENABLED)) {
        sharedPrefs.edit { putBoolean(KEY_REVERSE_CONNECTION_ENABLED, false) }
    }
}

The init block unconditionally resets reverse connection to disabled. This seems like a workaround for a bug rather than intentional behavior. Why force-disable on every app launch?

Dead Code / TODOs

Several TODOs and comments indicate incomplete work:

• Line 31 in PortalWebSocketServer.kt: // TODO test it
• Line 56 in ConfigManager.kt: // TODO add external injection from some config file
• Line 216 in ConfigManager.kt: // TODO replace (for getInstance singleton)
• Line 220 in ConfigManager.kt: // TODO: (for deviceCountryCode)

6. Testing Gaps

While the PR adds good unit tests, there are gaps:

• No tests for GestureController (gesture dispatch is critical functionality)
• No tests for ReverseConnectionService (complex lifecycle and reconnection logic)
• No tests for PortalWebSocketServer (authentication logic should be tested)
• No integration tests for the full WebSocket command flow
• No tests for binary response handling

7. Android-Specific Issues

Missing Permission Checks

The reverse connection service uses network and the gesture controller requires accessibility permissions, but there's no runtime validation that these permissions are granted before attempting operations.

Potential Battery Drain

File: ReverseConnectionService.kt:46-51

override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
    if (!isServiceRunning.getAndSet(true)) {
        Log.i(TAG, "Starting Reverse Connection Service")
        connectToHost()
    }
    return START_STICKY
}

START_STICKY with continuous reconnection attempts can drain battery. Consider using exponential backoff or limiting reconnection attempts.

Performance Considerations

Positive

• Uses thread pools appropriately (SocketServer with ExecutorService)
• Efficient binary data handling for screenshots (once the double-encoding is fixed)
• Proper use of AtomicBoolean for lock-free state checks

Concerns

• Main thread Handler in ReverseConnectionService for background operations
• No caching for package list (called frequently, rebuilds entire list each time)
• String concatenation in loops in ConfigManager.capitalize() (minor)

Recommendations Summary

Must Fix (High Priority)

1. Remove hardcoded userID - critical security vulnerability
2. Implement proper cryptographic token generation for authToken
3. Add error responses in ReverseConnectionService.handleMessage()
4. Fix screenshot double-encoding inefficiency

Should Fix (Medium Priority)

5. Add coordinate validation for gestures
6. Implement battery-friendly reconnection with exponential backoff
7. Add tests for GestureController, ReverseConnectionService, and PortalWebSocketServer
8. Use background thread for ReverseConnectionService handler
9. Fix ConfigManager init side effect (reverse connection auto-disable)

Nice to Have (Low Priority)

10. Standardize API response format
11. Add package enumeration error reporting
12. Resolve all TODO comments
13. Add permission validation before operations
14. Consider caching package list with invalidation strategy

Conclusion

This is a substantial and well-architected feature that demonstrates good Kotlin practices and clean separation of concerns. The core implementation is solid, but there are critical security issues (hardcoded credentials) that must be addressed before merging.

The testing is good but incomplete, and there are several opportunities for performance and maintainability improvements. Overall, this represents significant progress on the network mode functionality, but requires security fixes and additional polish before it's production-ready.

Recommendation: Request changes to address security issues, then approve after fixes.

PR Review: DRO-708 - Network Mode with WebSocket

Overview

This PR implements network communication (WebSocket & HTTP) to replace ADB dependency. This is a significant and well-structured improvement.

✅ Strengths

• Excellent separation of concerns: ActionDispatcher provides clean abstraction
• Good test coverage: New tests for ActionDispatcher, EventHub, PortalEvent
• Proper error handling: Try-catch blocks with logging throughout
• Clean architecture: GestureController singleton, proper constants organization

🔴 Critical Issues (Must Fix Before Merge)

1. Hardcoded User ID (ConfigManager.kt:241-243)

All devices share same UUID - serious security issue
Fix: Generate unique ID per device, store in SharedPreferences

2. Hardcoded Country Code (ConfigManager.kt:237-240)

All devices report Germany
Fix: Use context.resources.configuration.locales[0].country

3. Missing API Check (ApiHandler.kt:85)

longVersionCode needs API 28+ but check removed
Fix: Add Build.VERSION.SDK_INT check with fallback

4. Misleading Log (ReverseConnectionService.kt:116)

Says 'connection established' but connect() is async
Fix: Remove or change to 'connection initiated'

⚠️ Major Issues (Should Fix)

5. Race Condition (ReverseConnectionService.kt:154-161)

actionDispatcher late init can cause races
Fix: Initialize in onCreate() or delay connection

6. Base64 Double-Encoding (ApiHandler.kt:227-232)

Screenshot encoded twice - performance overhead
Fix: Return raw bytes, encode at transport layer only

7. Missing Validation (ActionDispatcher.kt:48-56)

Empty package name not validated
Fix: Return error if package name empty

8. Memory Leak Risk (ConfigManager)

Listeners lack lifecycle management
Fix: Document removeListener() requirement or use WeakReference

🟡 Minor Issues

9. Test Coverage Gaps

Missing tests for: GestureController, ReverseConnectionService, PortalWebSocketServer, new ApiHandler methods

10. No Connection Limits

WebSocket server has no connection limit (DoS risk)

11. Fixed Reconnect Delay

Consider exponential backoff instead of fixed 3s

🔒 Security

Good: Token auth, Bearer + query fallback, 401 responses

Concerns: No token expiration, no rotation, no rate limiting, hardcoded user ID

🎯 Performance

Good: Thread pool, async operations, atomic state, proper timeouts

Concerns: Base64 double-encoding, no connection limits, getPackages() loads all at once

📊 Final Verdict

Code Quality: ⭐⭐⭐⭐ (4/5)
Security: ⭐⭐⭐ (3/5)
Test Coverage: ⭐⭐⭐⭐ (4/5)

Recommendation: Approve after fixing 4 critical items. Solid work on WebSocket implementation and ActionDispatcher abstraction! 🚀

Pull Request Review: DRO-708 Network Mode Implementation

Overview

This PR implements a significant architectural enhancement by adding WebSocket and HTTP network communication modes, eliminating the dependency on ADB for device control. The changes introduce 1961 additions across 29 files with new gesture control, action dispatching, and reverse connection capabilities.

✅ Strengths

Architecture & Design

• Excellent separation of concerns with the new ActionDispatcher class that centralizes command handling for both HTTP and WebSocket layers
• Well-structured gesture control via the GestureController object using Android's AccessibilityService APIs
• Good code reuse - both SocketServer and PortalWebSocketServer delegate to the same ActionDispatcher, avoiding duplication
• Clean abstraction in ApiResponse with multiple response types (Success, Error, RawObject, RawArray, Binary, Text)

Code Quality

• Good use of Kotlin idioms (object singletons, data classes, sealed classes)
• Proper error handling with try-catch blocks and logging throughout
• Constants are well-defined and centralized
• View binding enabled for type-safe view access

Testing

• Good test coverage for new components:
  • ActionDispatcherTest.kt with 6 comprehensive tests covering edge cases
  • EventHubTest.kt with proper state isolation
  • PortalEventTest.kt for event model validation
• Tests use MockK properly and cover important scenarios (normalization, defaults, error handling)

🔴 Critical Issues

1. Security: Hardcoded User Credentials ⚠️

Location: ConfigManager.kt:289

The user ID is hardcoded instead of being generated or configured per device. This should either generate a UUID per installation or inject it from configuration.

2. Security: Hardcoded Country Code ⚠️

Location: ConfigManager.kt:283

All devices report as "de" (Germany) regardless of actual location. Should use proper locale detection.

3. Security: Ping Endpoint Bypasses Authentication

Location: SocketServer.kt:140

The ping endpoint bypasses authentication, which can be used for port scanning and service fingerprinting attacks. Either require auth for ping or implement rate limiting.

4. Resource Leak Risk: WebSocket Reconnection

Location: ReverseConnectionService.kt:122-137

No disconnect/cleanup before reconnecting. Old WebSocket client instances may accumulate if connections fail repeatedly. Should call disconnect() before attempting reconnection.

⚠️ Significant Issues

5. Error Handling: Silent Package Enumeration Failures

Location: ApiHandler.kt:131-138

Individual package failures are silently skipped. Users won't know if important apps are missing from the list. Consider collecting errors and including them in response metadata.

6. Race Condition: Lazy ActionDispatcher Initialization

Location: ReverseConnectionService.kt:159-167

The ActionDispatcher initialization check isn't thread-safe. If multiple messages arrive concurrently, multiple threads could initialize it simultaneously. Use synchronized initialization or proper lazy delegation.

7. Code Smell: Inconsistent Response Formatting

Location: PortalWebSocketServer.kt:111-132

The WebSocket response builder has complex conditional logic to handle different response formats, indicating that ApiResponse.toJson() returns inconsistent structures. Should standardize the response format.

8. Performance: Blocking Screenshot Operation

Location: ApiHandler.kt:217-222

Screenshot blocks the request thread for up to 5 seconds. With a limited thread pool, this could lead to thread starvation under load. Consider async/callback-based screenshot handling or increase thread pool size.

💡 Suggestions & Best Practices

9. Code Quality: Magic Numbers

Multiple instances of magic numbers (e.g., substring(14) in SocketServer.kt:133). Should use named constants.

10. Code Quality: Commented-out Code

Location: ApiHandler.kt:224-226

Commented code and TODOs should be tracked in issues, not left in production code.

11. API Design: Inconsistent Parameter Naming

• hideOverlay (camelCase) in screenshot
• base64_text (snake_case) in keyboard input
• package vs packageName inconsistency

Should standardize on camelCase for consistency with Kotlin conventions.

12. Logging: Excessive Debug Logging in Production

Multiple files have Log.d() calls that should be conditional on BuildConfig.DEBUG to avoid leaking sensitive information.

13. Testing: No Tests for New Services

Missing unit tests for:

• GestureController
• ReverseConnectionService
• PortalWebSocketServer authentication logic

14. Documentation: Missing KDoc

Public APIs in ActionDispatcher, GestureController, and ApiHandler lack documentation.

🔒 Security Summary

Authentication & Authorization

• ✅ Token-based authentication implemented for WebSocket and HTTP
• ⚠️ Ping endpoint bypasses authentication (intentional security hole)
• ✅ Bearer token support in headers and query params
• ⚠️ No rate limiting or brute force protection

Data Protection

• ✅ Auth tokens are randomly generated UUIDs
• ⚠️ User ID is hardcoded (see Critical Issue Where can I get the DroidRun Portal app? #1)
• ⚠️ Country code is hardcoded to "de" (see Critical Issue Text does not get extracted correctly in some Apps #2)
• ℹ️ No TLS/SSL enforcement (document requirement for secure networks)

Input Validation

• ✅ Parameters are validated with defaults (e.g., swipe duration clamped)
• ⚠️ No validation on package names before startActivity()
• ⚠️ WebSocket message size limits not enforced (DoS risk)

📊 Test Coverage Assessment

What's Tested ✅

• ActionDispatcher: 6 tests covering main scenarios
• EventHub: 3 tests with proper isolation
• PortalEvent: Basic model tests

What's Missing ❌

• GestureController (0% coverage)
• ReverseConnectionService (0% coverage)
• PortalWebSocketServer (0% coverage, especially auth)
• SocketServer (0% coverage)
• ConfigManager new methods (userID, deviceName, etc.)
• ApiHandler new methods (performTap, startApp, etc.)

Target at least 70% coverage for new code. Focus on:

1. WebSocket authentication flow
2. Gesture execution edge cases
3. Error handling in ReverseConnectionService

🎯 Performance Considerations

Identified Concerns

1. Screenshot blocking: 5-second blocking call can exhaust thread pool
2. Package enumeration: Synchronous loading of all packages on each request (consider caching)
3. No connection limits: WebSocket server has no max connection limit
4. Reconnection storms: Fixed 3-second delay could cause thundering herd with many devices

✨ Final Assessment

Summary

This is a substantial and well-architected feature that successfully eliminates the ADB dependency. The code demonstrates good software engineering practices with proper abstraction, error handling, and initial test coverage.

Critical Actions Required Before Merge

1. ❗ Fix hardcoded user ID (Security Issue Where can I get the DroidRun Portal app? #1)
2. ❗ Fix hardcoded country code (Security Issue Text does not get extracted correctly in some Apps #2)
3. ❗ Address WebSocket reconnection resource leak (Issue Added semantic text to interactive elements and adjust rectangle appearance  #4)
4. ❗ Make decision on ping endpoint authentication (Issue Portal APK also shows not visible elements, because the UI tree stil has them marked as visible. #3)

Recommended Follow-ups (Can be separate PRs)

1. Add tests for GestureController, ReverseConnectionService, and WebSocket auth
2. Implement rate limiting for public endpoints
3. Add connection limits and exponential backoff
4. Document network security requirements
5. Standardize API parameter naming conventions
6. Cache package list for performance

Verdict

Approval with required changes: The architectural foundation is solid, but the security issues must be addressed before merging to production. Once the critical issues are resolved, this will be a valuable enhancement to the platform.

Great work on the clean architecture and initial testing! 🚀

Generated with Claude Code - Reviewed 29 files, 1961 additions, 548 deletions

@claude break this PR down, what it does and what it adds. read all relevant files, dig into it and prepare to discuss the ins and outs of it

Claude finished @johnmalek312's task in 3m 19s —— View job

PR #33 Comprehensive Breakdown: Network Mode with WebSocket Implementation

I've done a deep dive into this PR. Here's everything you need to know about what it does, how it works, and the architecture behind it.

🎯 What This PR Does

This PR implements network-based device control as an alternative to ADB (Android Debug Bridge). It adds three primary communication modes:

1. HTTP REST API (port 8080) - Legacy endpoint compatibility
2. WebSocket Server (port 8081) - Real-time bidirectional communication
3. Reverse WebSocket Connection - Device initiates connection to remote server (cloud/host)

The Big Picture

Before this PR, the app relied on ADB for remote control. This PR eliminates that dependency by implementing native network protocols, allowing the Android device to be controlled over WiFi/network without USB debugging.

🏗️ Architecture Overview

The implementation follows a clean layered architecture:

┌─────────────────────────────────────────────────────┐
│         Network Layer (HTTP/WebSocket)              │
│  - SocketServer (HTTP REST)                         │
│  - PortalWebSocketServer (WebSocket)                │
│  - ReverseConnectionService (Outbound WebSocket)    │
└──────────────────┬──────────────────────────────────┘
                   │
                   ▼
┌─────────────────────────────────────────────────────┐
│         Command Routing Layer                       │
│  - ActionDispatcher (Central command router)        │
└──────────────────┬──────────────────────────────────┘
                   │
                   ▼
┌─────────────────────────────────────────────────────┐
│         Business Logic Layer                        │
│  - ApiHandler (API operations)                      │
│  - GestureController (Touch gestures)               │
└──────────────────┬──────────────────────────────────┘
                   │
                   ▼
┌─────────────────────────────────────────────────────┐
│         Android System Layer                        │
│  - DroidrunAccessibilityService                     │
│  - AccessibilityService APIs                        │
└─────────────────────────────────────────────────────┘

📦 New Components Added

1. ActionDispatcher.kt (109 lines) ⭐ Core Innovation

Location: app/src/main/java/com/droidrun/portal/service/ActionDispatcher.kt

Purpose: Central command router that both HTTP and WebSocket servers use to avoid code duplication.

How it works:

• Accepts action name (e.g., "tap", "swipe", "/action/tap") and JSON parameters
• Normalizes action names by stripping prefixes (/action/ or action.)
• Routes to appropriate ApiHandler method
• Returns unified ApiResponse

Supported Actions:

- tap(x, y)                    // Touch at coordinates
- swipe(startX, startY, ...)   // Swipe gesture
- global(action)               // System actions (home, back, recents)
- app(package, activity)       // Launch apps
- keyboard/input(text)         // Type text
- keyboard/clear()             // Clear text
- keyboard/key(keyCode)        // Press key
- screenshot(hideOverlay)      // Capture screen
- packages()                   // List installed apps
- state(filter)                // Get UI state
- version()                    // App version
- time()                       // Timestamp

Key Design Decision: The dispatcher normalizes both REST-style paths (/action/tap) and RPC-style methods (action.tap), making it protocol-agnostic.

2. GestureController.kt (87 lines) ⭐ Gesture Engine

Location: app/src/main/java/com/droidrun/portal/service/GestureController.kt

Purpose: Singleton object that executes touch gestures and global actions via Android's AccessibilityService.

Why it exists: Eliminates the need for ADB shell input commands by using native Android gesture APIs.

Key Methods:

fun tap(x: Int, y: Int): Boolean
  - Creates Path at coordinates
  - 50ms duration tap
  - Dispatches via AccessibilityService.dispatchGesture()

fun swipe(startX, startY, endX, endY, durationMs): Boolean
  - Creates Path from start to end
  - Duration clamped between 10ms - 5000ms
  - Default 300ms if not specified

fun performGlobalAction(action: Int): Boolean
  - GLOBAL_ACTION_HOME, BACK, RECENTS, etc.
  - Direct AccessibilityService call

Technical Detail: Uses GestureDescription.StrokeDescription with Path objects to simulate touch input. This is Android's official API for programmatic gestures (API 24+).

3. ReverseConnectionService.kt (192 lines) ⭐ Cloud Integration

Location: app/src/main/java/com/droidrun/portal/service/ReverseConnectionService.kt

Purpose: Android Service that maintains persistent WebSocket connection to a remote server (cloud/host).

Use Case: Allows devices behind NAT/firewall to be controlled remotely by connecting outbound to a cloud server.

How it works:

1. Initialization (onCreate):

   • Gets ConfigManager instance
   • Reads reverse connection URL and token from config

2. Connection Flow (onStartCommand):

   • Builds headers with auth token, device ID, device name, country code, user ID
   • Creates WebSocketClient with remote URI
   • Connects to remote server

3. Message Handling (handleMessage):

   • Parses incoming JSON: {"id": 123, "method": "tap", "params": {"x": 100, "y": 200}}
   • Lazily initializes ActionDispatcher when first message arrives
   • Dispatches command via ActionDispatcher
   • Sends response back with same ID

4. Reconnection Logic (scheduleReconnect):

   • On disconnect or error, schedules reconnect after 3 seconds
   • Uses AtomicBoolean to prevent multiple concurrent reconnect attempts
   • Runs until service is stopped

Headers Sent:

Authorization: Bearer <token>
X-User-ID: <user_id>
X-Device-ID: <device_id>
X-Device-Name: <manufacturer model>
X-Device-Country: <country_code>

⚠️ Known Issue: userID is hardcoded to "7785b089-b9aa-458d-a32e-baec315e5e16" (ConfigManager.kt:174) - this is a test value that should be replaced.

4. PortalWebSocketServer.kt (183 lines - modified significantly)

Location: app/src/main/java/com/droidrun/portal/events/PortalWebSocketServer.kt

Purpose: WebSocket server running on the Android device (default port 8081).

Changes in this PR:

• ✅ Added authentication via token (header or query param)
• ✅ Added command routing via ActionDispatcher
• ✅ Added binary response support (for screenshots)
• ✅ Maintains event broadcasting for real-time updates

Authentication Flow:

onWebsocketHandshakeReceivedAsServer():
  1. Check Authorization header for "Bearer <token>"
  2. Fallback: check query param "?token=<token>"
  3. Compare with configManager.authToken
  4. Reject with 401 if invalid/missing

Message Handling:

onMessage(String):
  1. Parse JSON: {"id": "uuid", "method": "screenshot", "params": {}}
  2. Dispatch via actionDispatcher.dispatch()
  3. If Binary response → send [UUID bytes][data bytes]
  4. If Text response → send {"id": "uuid", "status": "success", "result": ...}

Binary Protocol (for screenshots):

[36 bytes UUID string][N bytes PNG data]

The client extracts the first 36 bytes as the request ID, then treats the rest as PNG image data.

5. Enhanced ApiHandler.kt (+164 lines, -45 lines modified)

Location: app/src/main/java/com/droidrun/portal/api/ApiHandler.kt

New Methods Added:

performTap(x, y) → calls GestureController.tap()
performSwipe(...) → calls GestureController.swipe()
performGlobalAction(action) → calls GestureController.performGlobalAction()
startApp(packageName, activityName?) → creates Intent and launches
getPackages() → enumerates all launchable apps with metadata
getTime() → returns System.currentTimeMillis()

getPackages() Deep Dive:
This method queries the PackageManager for all apps with CATEGORY_LAUNCHER:

Returns JSON array:
[
  {
    "packageName": "com.example.app",
    "label": "My App",
    "versionName": "1.2.3",
    "versionCode": 123,
    "isSystemApp": false
  },
  ...
]

Error Handling: Individual package failures are caught and logged but not reported to the client (line 96-138). This prevents one broken app from failing the entire enumeration.

⚠️ Issue: Line 126 uses pkgInfo.longVersionCode without API level check. This is only available on API 28+. On older devices, this will crash.

6. Enhanced ConfigManager.kt (+145 lines, -38 lines modified)

Location: app/src/main/java/com/droidrun/portal/config/ConfigManager.kt

New Configuration Properties:

// WebSocket Server
var websocketEnabled: Boolean (default: true)
var websocketPort: Int (default: 8081)

// Reverse Connection
var reverseConnectionUrl: String
var reverseConnectionToken: String
var reverseConnectionEnabled: Boolean (runtime only, not persisted)

// Security
val authToken: String (auto-generated UUID, persisted)
val deviceID: String (auto-generated UUID, persisted)

// Device Info
val deviceName: String (manufacturer + model, e.g., "Samsung Galaxy S21")
val deviceCountryCode: String (hardcoded "de" - TODO)
val userID: String (hardcoded test UUID - ⚠️ SECURITY ISSUE)

// Event Toggles
fun isEventEnabled(type: EventType): Boolean
fun setEventEnabled(type: EventType, enabled: Boolean)

Listener Pattern: ConfigManager notifies listeners when settings change:

interface ConfigChangeListener {
  onOverlayVisibilityChanged(visible: Boolean)
  onOverlayOffsetChanged(offset: Int)
  onSocketServerEnabledChanged(enabled: Boolean)
  onSocketServerPortChanged(port: Int)
  onWebSocketEnabledChanged(enabled: Boolean)
  onWebSocketPortChanged(port: Int)
}

DroidrunAccessibilityService implements this interface to restart servers when ports change.

7. Enhanced SocketServer.kt (+124 lines, -43 lines modified)

Location: app/src/main/java/com/droidrun/portal/service/SocketServer.kt

Changes:

• ✅ Integrated ActionDispatcher for command routing
• ✅ Added authentication (except /ping endpoint)
• ✅ Added CORS headers (Access-Control-Allow-Origin: *)
• ✅ Binary response support for screenshots

HTTP Request Flow:

Client → GET /screenshot?hideOverlay=true
  ↓
SocketServer.handleGetRequest()
  ↓
ActionDispatcher.dispatch("screenshot", {"hideOverlay": true})
  ↓
ApiHandler.getScreenshot(true)
  ↓
Returns ApiResponse.Text (base64 encoded PNG)
  ↓
sendHttpResponse() or sendBinaryResponse()
  ↓
Client receives 200 OK with PNG data

Authentication:

Auth header check (line 132-135):
  Authorization: Bearer <token>
  
Validation (line 140):
  if (path != "/ping" && authToken != configManager.authToken) {
    return 401 Unauthorized
  }

⚠️ Security Note: The /ping endpoint bypasses authentication intentionally for connectivity checks, but this creates a fingerprinting vector.

8. Enhanced ApiResponse.kt (+47 lines, -4 lines modified)

Location: app/src/main/java/com/droidrun/portal/api/ApiResponse.kt

New Response Types:

sealed class ApiResponse {
  data class Success(val data: Any)           // Existing
  data class Error(val message: String)       // Existing
  data class RawObject(val json: JSONObject)  // ✨ NEW
  data class RawArray(val json: JSONArray)    // ✨ NEW
  data class Binary(val data: ByteArray)      // ✨ NEW
  data class Text(val data: String)           // ✨ NEW
}

Why these were added:

• RawObject/RawArray: For endpoints like getPackages() that return complex JSON structures
• Binary: For screenshot data (PNG bytes)
• Text: For base64-encoded data that shouldn't be JSON-escaped

toJson() Method:
All response types can be serialized to JSON (except Binary, which can optionally be base64-encoded for JSON-RPC).

🧪 Testing

New Test Files:

1. ActionDispatcherTest.kt (93 lines)

   • Tests action name normalization (/action/tap → tap)
   • Tests default parameter values (swipe duration = 300ms)
   • Tests activity name handling (null, empty, "null" string all → null)
   • Tests method aliases (input and keyboard/input both work)
   • Tests unknown method error handling

2. EventHubTest.kt (77 lines)

   • Tests event filtering based on ConfigManager settings
   • Tests that PONG and UNKNOWN events bypass filtering
   • Uses reflection to reset EventHub state between tests

3. PortalEventTest.kt (80 lines)

   • Tests event JSON serialization/deserialization
   • Tests event type handling

Test Coverage: ✅ Good coverage for new core logic, ❌ Missing tests for GestureController, ReverseConnectionService, and UI components.

🔧 UI Changes (MainActivity & SettingsBottomSheet)

MainActivity.kt (+209 lines, -215 lines modified)

New Features:

1. Network Info Section:

   • Shows device IP address
   • Shows HTTP port with "Copy" button (copies to clipboard)
   • Shows WebSocket port with "Copy" button
   • Shows auth token with "Copy" button (⚠️ security concern - tokens in clipboard)

2. Reverse Connection Status:

   • Green indicator when connected
   • Red indicator when disconnected
   • Shows connection URL

3. ViewBinding Migration:

   • Replaced findViewById() with type-safe binding.viewName
   • Modern Android development practice

SettingsBottomSheet.kt (+91 lines, -20 lines modified)

New Settings:

1. WebSocket Server Toggle (enable/disable)
2. WebSocket Port Input (default 8081)
3. Reverse Connection Toggle (enable/disable)
4. Reverse Connection URL Input
5. Reverse Connection Token Input

All settings persist to SharedPreferences via ConfigManager.

📊 Key Statistics

Total Changes: 29 files
Additions: +1,961 lines
Deletions: -548 lines
Net Change: +1,413 lines

New Files Created: 3
  - ActionDispatcher.kt
  - GestureController.kt
  - ReverseConnectionService.kt

Test Files Added: 3
  - ActionDispatcherTest.kt
  - EventHubTest.kt  
  - PortalEventTest.kt

Major Refactors: 4
  - DroidrunAccessibilityService.kt
  - SocketServer.kt
  - ConfigManager.kt
  - MainActivity.kt

🔐 Security Considerations

✅ Good Security Practices:

1. Token-based authentication for both HTTP and WebSocket
2. Auto-generated UUIDs for auth tokens and device IDs
3. Auth tokens persisted in SharedPreferences
4. Bearer token support in HTTP headers

⚠️ Security Issues Found:

1. Hardcoded User ID (ConfigManager.kt:174)

   val userID: String
       get() = "7785b089-b9aa-458d-a32e-baec315e5e16"

   All devices share the same user ID - major security vulnerability.

2. Hardcoded Country Code (ConfigManager.kt:169)

   val deviceCountryCode: String
       get() = "de"

   Should use actual device locale.

3. Auth Token in Clipboard (MainActivity.kt:138)
   Copying sensitive tokens to clipboard exposes them to clipboard monitoring apps.

4. Ping Endpoint Without Auth (SocketServer.kt:140)
   Allows unauthenticated port scanning and service fingerprinting.

5. CORS Wildcard (SocketServer.kt:255, 319)
   Access-Control-Allow-Origin: * allows any origin to make requests.

6. Weak Token Generation (ConfigManager.kt:64)
   Using UUID.randomUUID() instead of SecureRandom for cryptographic tokens.

🐛 Bugs & Issues Found

High Priority:

1. API Level Check Missing (ApiHandler.kt:126)

   val versionCode = pkgInfo.longVersionCode  // API 28+ only!

   Will crash on Android versions below API 28.

2. Race Condition (ReverseConnectionService.kt:159-166)

   if (!::actionDispatcher.isInitialized) {
       // Not thread-safe - multiple messages could initialize simultaneously
   }

3. Resource Leak (SocketServer.kt:94)
   Socket not closed if executor is null or submit() throws.

Medium Priority:

4. Misleading Log (ReverseConnectionService.kt:115)
   Says "websocket connection established" immediately after connect() call, but connection is async.

5. Screenshot Double-Encoding (ApiHandler.kt:227-232)
   Service returns Base64 string → decoded to bytes → re-encoded to Base64. Wasteful.

6. No Reconnect Backoff (ReverseConnectionService.kt:23)
   Fixed 3-second delay could cause excessive load. Should use exponential backoff.

🎯 Design Patterns Used

1. Singleton Pattern: ConfigManager, GestureController, DroidrunAccessibilityService
2. Observer Pattern: ConfigChangeListener interface for reactive updates
3. Strategy Pattern: ActionDispatcher routes to different handlers
4. Sealed Class: ApiResponse for type-safe response handling
5. Builder Pattern: GestureDescription.Builder for gesture creation

💡 Why This Architecture is Clever

Problem: HTTP and WebSocket servers would duplicate command handling logic

Solution: ActionDispatcher acts as a central command router

Before (hypothetical):

SocketServer.handlePost("/tap") {
    val x = params.get("x")
    val y = params.get("y")
    GestureController.tap(x, y)
}

WebSocketServer.onMessage() {
    if (method == "tap") {
        val x = params.get("x")
        val y = params.get("y")
        GestureController.tap(x, y)
    }
}

After (actual implementation):

SocketServer.handlePost(path) → actionDispatcher.dispatch(path, params)
WebSocketServer.onMessage() → actionDispatcher.dispatch(method, params)

ActionDispatcher.dispatch("tap", params) {
    val x = params.optInt("x", 0)
    val y = params.optInt("y", 0)
    apiHandler.performTap(x, y)  // Single source of truth!
}

This eliminates code duplication and ensures both protocols behave identically.

🚀 Performance Optimizations

1. Thread Pool (SocketServer.kt:54)

   • 5-thread executor for concurrent HTTP requests
   • Prevents blocking on individual slow requests

2. CompletableFuture (DroidrunAccessibilityService.kt:742)

   • Async screenshot capture with timeout
   • Doesn't block main thread

3. AtomicBoolean (ReverseConnectionService.kt:30)

   • Lock-free thread-safe state management
   • Prevents race conditions in service lifecycle

4. Gesture Duration Clamping (GestureController.kt:56)

   val dur = durationMs.coerceIn(MIN_SWIPE_DURATION_MS, MAX_SWIPE_DURATION_MS)

   Prevents invalid gesture durations that could hang the system

📚 What to Discuss / Ask About

Now that you have the full breakdown, here are some topics we can dive deeper into:

1. Architecture Questions:

   • Why ActionDispatcher instead of inheritance/interfaces?
   • Trade-offs of singleton pattern for GestureController
   • Event system design (EventHub + PortalEvent)

2. Implementation Details:

   • How screenshot capture works (HardwareBuffer → Bitmap → PNG → Base64)
   • WebSocket binary protocol design (UUID prefix)
   • Reverse connection lifecycle management

3. Security:

   • How to fix the hardcoded user ID issue
   • Proper token rotation strategy
   • Rate limiting implementation

4. Testing Strategy:

   • Why no tests for GestureController?
   • How to test WebSocket authentication flow
   • Integration test approach

5. Future Enhancements:

   • Should the double-Base64 encoding be fixed?
   • Exponential backoff for reconnection
   • Connection pooling for HTTP server

Ready to discuss any of these areas in detail! 🎯

@claude SocketServer? does the phone act as a client or server? does it require open ports?

Claude finished @johnmalek312's task in 1m 6s —— View job

SocketServer Architecture Explanation

Great question! Let me clarify the networking architecture in this PR. There are actually three different networking modes, not just one:

🏗️ Three Networking Modes

1. SocketServer (HTTP) - Phone as Server 🔴

File: SocketServer.kt
Role: The Android phone acts as an HTTP server
Default Port: 8080 (configurable)
Requires Open Ports: ✅ YES

ServerSocket(port)  // Line 55 - Creates TCP server socket
serverSocket?.accept()  // Line 94 - Accepts incoming connections

How it works:

• The phone opens port 8080 and listens for incoming HTTP requests
• External clients (laptop, other devices on same network) connect TO the phone
• Clients send HTTP GET/POST requests to control the device
• Example: http://192.168.1.100:8080/screenshot

Port Requirements:

• The phone must be reachable on the network
• If behind a router/NAT, you'd need port forwarding
• Works great on local WiFi (same network as controlling device)

2. PortalWebSocketServer - Phone as Server 🔴

File: PortalWebSocketServer.kt
Role: The Android phone acts as a WebSocket server
Default Port: 8081 (configurable)
Requires Open Ports: ✅ YES

class PortalWebSocketServer(port: Int, ...) : WebSocketServer(InetSocketAddress(port))
// Line 19 - Extends WebSocketServer, making phone a server

How it works:

• The phone opens port 8081 and listens for WebSocket connections
• External clients connect TO the phone and maintain persistent bidirectional connection
• Supports real-time events (UI changes broadcast to all connected clients)
• Example: ws://192.168.1.100:8081?token=abc123

Port Requirements:

• Same as HTTP server - phone must be reachable
• Requires authentication token (via header or query param)

3. ReverseConnectionService - Phone as Client 🔵

File: ReverseConnectionService.kt
Role: The Android phone acts as a WebSocket client
Target: Remote server (cloud/host URL)
Requires Open Ports: ❌ NO (outbound connection only)

webSocketClient = object : WebSocketClient(uri, headers) { ... }  // Line 94
webSocketClient?.connect()  // Line 114 - Initiates OUTBOUND connection

How it works:

• The phone initiates an outbound connection to a remote server (e.g., cloud service)
• The remote server acts as the WebSocket server
• Phone connects OUT, so no inbound ports need to be open
• Solves the NAT/firewall problem - devices behind restrictive networks can still be controlled

Connection Flow:

1. User configures remote server URL in settings (e.g., wss://cloud.example.com/devices)
2. Phone establishes WebSocket connection TO that server
3. Server sends commands down the persistent connection
4. Phone executes commands and sends responses back

Headers Sent (Line 61-76):

Authorization: Bearer <token>
X-User-ID: <user_id>
X-Device-ID: <device_id>
X-Device-Name: Samsung Galaxy S21
X-Device-Country: de

Auto-Reconnection:

• If connection drops, automatically reconnects after 3 seconds
• Runs as Android Service (persistent background process)
• Uses START_STICKY - system restarts service if killed

📊 Comparison Table

🔒 Port Security Considerations

When Ports ARE Open (Modes 1 & 2):

• ⚠️ Anyone on the network can attempt to connect
• ✅ Authentication required (token-based)
• ⚠️ Ping endpoint (/ping) bypasses auth (line 140 in SocketServer.kt)
• 🛡️ CORS enabled with wildcard (Access-Control-Allow-Origin: *)

When Ports Are NOT Open (Mode 3):

• ✅ No inbound connections possible
• ✅ Phone initiates connection (firewall-friendly)
• ✅ Remote server controls authentication
• 🔐 Server can implement rate limiting, IP filtering, etc.

🎯 Which Mode to Use?

Use SocketServer/PortalWebSocketServer (phone as server) when:

• Controlling device from same WiFi network
• Testing/development on local machine
• No cloud infrastructure needed
• Direct device-to-device communication

Use ReverseConnectionService (phone as client) when:

• Device is behind NAT/firewall/mobile network
• Need centralized cloud control of multiple devices
• Cannot open ports on device network
• Want persistent connection managed by remote server

💡 Key Insight

The "SocketServer" name is accurate - the phone does act as a server for that component. However, the PR adds both server and client modes, giving you flexibility:

• Server mode = phone opens ports, clients connect TO it
• Client mode = phone connects OUT to remote server, no open ports needed

The ActionDispatcher (line 23 in SocketServer.kt, line 17 in PortalWebSocketServer.kt) unifies command handling across all three modes, so commands work identically regardless of how they're delivered.

Let me know if you need clarification on any of these modes! 🚀