Gateway Auth

[tylerhawkes]

Add Gateway Auth by wiring AuthCallback and AuthHandle through ClientOptions to FFI with contract version check 29

Introduce Kotlin Credential, AuthCallback, and AuthHandle, thread them through ClientOptions to backend connection, and add FFI bindings with async trait calls, foreign futures, and checksum/version validation. Update generated native libraries and register new auth and message deletion callbacks in xmtpv3.kt.

📍Where to Start

Start with FFI load/registration and contract/checksum checks in xmtpv3.kt, then follow ClientOptions usage in Client.kt, and the credential/callback/handle types in Auth.kt.

---

Macroscope summarized fed46b8.

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review Complete ✓

Reviewing PR #499: Gateway Auth implementation

Todo List

• Read and analyze new Auth.kt (main source)
• Review Auth.kt (androidTest)
• Review AuthCredentialTest.kt (unit test)
• Examine Client.kt changes
• Review xmtpv3.kt FFI changes and contract validation
• Check libxmtp version changes
• Analyze for security concerns
• Evaluate test coverage
• Check Kotlin/Android best practices
• Post comprehensive review feedback

---

Review Feedback

This PR successfully introduces gateway authentication through AuthCallback and AuthHandle, threading them through ClientOptions to the FFI layer. The implementation is well-structured and follows Kotlin best practices. Below are my findings:

🟢 Strengths

1. Security-Conscious Design

• Credential values are properly redacted in toString() (library/src/main/java/org/xmtp/android/library/Auth.kt:47)
• Good attempt at security with valueCharArray (Auth.kt:15,18)
• Proper validation of inputs (non-blank name, non-negative expiry)

2. Clean API Design

• AuthCallback interface is clean and well-documented (Auth.kt:55-63)
• AuthHandle properly implements AutoCloseable for resource management (Auth.kt:69-88)
• Good separation of concerns with companion object factory methods

3. Test Coverage

• Unit tests cover credential creation, validation, and FFI round-trip (AuthCredentialTest.kt)
• Integration tests verify both callback and handle flows (Auth.kt androidTest)

4. Proper FFI Integration

• Auth types properly convert to/from FFI types (Auth.kt:41-44, 59-62)
• ClientOptions.Api correctly threads auth parameters (Client.kt:66-67, 214-215)

🟡 Areas for Improvement

Security Concerns

1. Credential Value Not Actually Zeroed ⚠️

// Auth.kt:34-37
val charArray = value.toCharArray()
val credential = Credential(name, charArray, expiresAtSeconds)
// Zero the original string reference if possible, but since it's immutable, just return
return credential

Issue: The comment acknowledges the limitation but doesn't address it. The credential value remains in memory:

• The input String value parameter cannot be zeroed (immutable)
• The valueCharArray field is private but never zeroed
• String interning may keep copies in the string pool

Recommendation: If credential security is critical:

• Accept CharArray directly instead of String in the factory method
• Add a clear() method to zero the valueCharArray after use
• Document that callers must manage the input CharArray lifecycle
• Consider using SecureString or similar patterns if available

Example improvement:

companion object {
    @JvmStatic
    fun create(name: String, value: CharArray, expiresAtSeconds: Long): Credential {
        val copiedArray = value.copyOf()
        return Credential(name, copiedArray, expiresAtSeconds)
    }
}

fun clear() {
    valueCharArray.fill('\u0000')
}

2. Exception Handling in Callbacks (Auth.kt:53-54)

// "If onAuthRequired throws an exception, it will propagate and may crash the FFI layer"

Issue: The documentation warns about crashes but doesn't enforce safe handling.

Recommendation: Consider wrapping the FFI callback in a try-catch:

fun toFfi(): FfiAuthCallback = object : FfiAuthCallback {
    override suspend fun onAuthRequired(): FfiCredential = try {
        Credential.toFfi(this@AuthCallback.onAuthRequired())
    } catch (e: Exception) {
        Log.e("AuthCallback", "Auth callback failed", e)
        throw XMTPException("Authentication failed: ${e.message}", e)
    }
}

3. Cache Key Excludes Auth Details (Client.kt:74-75)

fun toCacheKey(): String =
    "${env.getUrl()}|$isSecure|${appVersion ?: "nil"}|${gatewayHost ?: "nil"}|${authCallback != null}"

Issue: The cache key only checks authCallback != null, not the actual auth handle/callback instance. This means:

• Different auth callbacks will share the same cached connection
• A connection with expired credentials might be reused

Recommendation: Either:

• Don't cache authenticated connections at all (safest)
• Include auth instance identity in cache key: System.identityHashCode(authCallback)
• Add explicit cache invalidation when credentials expire

Android/Kotlin Best Practices

4. Missing Proguard/R8 Rules
Issue: No proguard rules for FFI interfaces that may be invoked via reflection/JNI.

Recommendation: Add proguard rules to prevent shrinking of FFI types:

-keep class uniffi.xmtpv3.FfiAuthCallback { *; }
-keep class uniffi.xmtpv3.FfiAuthHandle { *; }
-keep class uniffi.xmtpv3.FfiCredential { *; }

5. Test Cleanup Missing (Auth.kt androidTest:46)

handle.close()

Issue: If the test fails before line 46, the handle leaks. The same issue exists if client creation throws.

Recommendation: Use try-finally or use {}:

@Test
fun testAuthHandle() = runBlocking {
    AuthHandle().use { handle ->
        val credential = Credential.create("handleName", "handleValue", 789L)
        handle.set(credential)
        val api = ClientOptions.Api(
            env = XMTPEnvironment.DEV,
            gatewayHost = "test.com",
            authHandle = handle,
        )
        val client = Client.connectToApiBackend(api)
        Assert.assertNotNull(client)
    }
}

6. Potential Memory Leak in Tests (Auth.kt androidTest:28-29, 44-45)
Issue: Client.connectToApiBackend() creates clients that are cached but never closed in tests.

Recommendation: Either:

• Clear the cache after tests: add an @After method
• Document that test clients are for demonstration only
• Use a test-specific non-caching factory

Code Quality

7. Inconsistent Nullable Handling (Client.kt:66-67)

val authCallback: AuthCallback? = null,
val authHandle: AuthHandle? = null,

Issue: Both can be provided simultaneously, but it's unclear which takes precedence or if this is an error.

Recommendation:

• Document which takes precedence if both are provided
• Consider making them mutually exclusive:

init {
    require(authCallback == null || authHandle == null) {
        "Cannot provide both authCallback and authHandle"
    }
}

8. Backing Property Unnecessary (Auth.kt:72-73)

internal val handle: FfiAuthHandle
    get() = ffiHandle

Issue: This getter just returns the same field. Consider exposing ffiHandle directly as internal if needed.

9. Data Class Equals/HashCode Issue (Auth.kt:12-18)
Issue: Credential is a data class with valueCharArray: CharArray, but CharArray.equals() uses reference equality, not content equality.

Recommendation: Override equals/hashCode or make it a regular class:

override fun equals(other: Any?): Boolean {
    if (this === other) return true
    if (other !is Credential) return false
    return name == other.name &&
           valueCharArray.contentEquals(other.valueCharArray) &&
           expiresAtSeconds == other.expiresAtSeconds
}

override fun hashCode(): Int {
    return 31 * (31 * name.hashCode() + valueCharArray.contentHashCode()) + expiresAtSeconds.hashCode()
}

🔵 Minor Issues

10. Typo in Comment (Client.kt:70-72)

/**
 * Cache key excluding auth details, as auth is connection-specific and handled via
 * callbacks/handles. Connections with auth will use the callback/handle rather than cache
 * invalidation.
 */

The wording "rather than cache invalidation" is confusing. Clarify the intent.

11. Missing KDoc for FFI Methods (Auth.kt:41, 44, 59)

• fromFfi(), toFfi() methods lack documentation
• Consider adding @internal or similar to indicate they're not public API

12. Test Assertions Could Be Stronger (AuthCredentialTest.kt:23, 33)

Assert.assertTrue(e.message!!.contains("blank"))

Issue: Force-unwrapping with !! can fail if message is null.

Recommendation:

Assert.assertTrue(e.message?.contains("blank") == true)

📊 Test Coverage Assessment

Good Coverage:

• ✅ Credential creation, validation, toString redaction
• ✅ FFI round-trip conversion
• ✅ AuthCallback integration with client
• ✅ AuthHandle integration with client

Missing Coverage:

• ❌ Expiry handling (what happens when credentials expire?)
• ❌ Concurrent auth requests
• ❌ Auth callback throwing exceptions
• ❌ AuthHandle.set() called multiple times
• ❌ Memory leak tests for AuthHandle
• ❌ Thread safety of AuthHandle

🎯 Summary

This is a solid implementation that introduces authentication support cleanly. The main concerns are:

1. Critical: The credential zeroing doesn't actually work - address this if security is a priority
2. Important: Cache key logic may cause authenticated connections to be shared incorrectly
3. Important: Missing resource cleanup in tests could cause flakiness
4. Nice-to-have: Additional test coverage for edge cases and error handling

Overall, the code is well-structured and follows Kotlin conventions. With the security and caching issues addressed, this will be production-ready.

---