Repository: firebase/firebase-ios-sdk (language: cpp)

Issue:
AuthAPNSTokenManager.getToken() fatal error unwrapping nil
### Description

When performing `PhoneAuthProvider.provider().verifyPhoneNumber` and getting a timeout, the app crashes since `AuthAPNSTokenManager` runs into a nil unwrapping fatal error.

To my understanding, there is an issue in the commit **[auth-swift] Finish Transition to async-await for RPC calls (#11557)** from 19 Jul 2023.

Indeed the design of the callback in `getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void)` lacks consistency in my opinion.
It takes a tuple of two optional `(AuthAPNSToken?, Error?)` instead of using more appropriate structures such as `Result<AuthAPNSToken, Error>`.

It is probably envisioned by the developer that "it will never happen that both are nil at the same time", and thus we come with the implementation of `getToken() async throws AuthAPNSToken` which force unwraps `error` in the case of `token` being `nil`. See `AuthAPNSTokenManager` lines 76 to 86.

What strikes me is that in the case of a timeout, this callback (through the call of `self.callback` which gets over all the pending callbacks in the array which was set line 61) is called with `(nil, nil)` !

So it comes with not so much surprise that line 82 leads to a fatal error by force unwrapping a nil value.

<img width="1627" alt="Screenshot 2024-08-08 at 15 39 31" src="https://github.com/user-attachments/assets/172048d3-4969-4fcd-b0e7-7c256a4028bb">


### Reproducing the issue

1. configure Firebase
2. call PhoneAuthProvider.provider().verifyPhoneNumber
3. get a timeout after 5 sec
4. app crashes

### Firebase SDK Version

11.0.0

### Xcode Version

15.4

### Installation Method

Swift Package Manager

### Firebase Product(s)

Authentication

### Targeted Platforms

iOS

### Relevant Log Output

_No response_

### If using Swift Package Manager, the project's Package.resolved

<!--- Look below for instructions on how to share your Package.resolved. --->

<details>
<summary>Expand <code>Package.resolved</code> snippet</summary>
<br>

```json

Not applicable.

```

</details>


### If using CocoaPods, the project's Podfile.lock

<!--- Look below for instructions on how to share your Podfile.lock. --->

<details>
<summary>Expand <code>Podfile.lock</code> snippet</summary>
<br>

```yml

Not applicable.

```

</details>



Discussion / hints:
I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
@snapxo Thanks for the report and sharing the investigation.

We'll work on a fix for the next release.

Fix patch:
diff --git a/FirebaseAuth/CHANGELOG.md b/FirebaseAuth/CHANGELOG.md
index 66a0b4101eb..0f6d994d704 100644
--- a/FirebaseAuth/CHANGELOG.md
+++ b/FirebaseAuth/CHANGELOG.md
@@ -3,6 +3,8 @@
 - [added] Added custom provider support to `AuthProviderID`. Note that this change will be breaking
   to any code that implemented an exhaustive `switch` on `AuthProviderID` in 11.0.0 - the `switch`
   will need expansion. (#13429)
+- [fixed] Fix crash introduced in 11.0.0 in phone authentication flow from
+  implicitly unwrapping `nil` error after a token timeout. (#13470)
 
 # 11.0.0
 - [fixed] Fixed auth domain matching code to prioritize matching `firebaseapp.com` over `web.app`
diff --git a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift
index fdbe5c6bb74..49521cae813 100644
--- a/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift
+++ b/FirebaseAuth/Sources/Swift/SystemService/AuthAPNSTokenManager.swift
@@ -49,9 +49,9 @@
     /// token becomes available, or when timeout occurs, whichever happens earlier.
     ///
     /// This function is internal to make visible for tests.
-    func getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void) {
+    func getTokenInternal(callback: @escaping (Result<AuthAPNSToken, Error>) -> Void) {
       if let token = tokenStore {
-        callback(token, nil)
+        callback(.success(token))
         return
       }
       if pendingCallbacks.count > 0 {
@@ -68,18 +68,19 @@
       kAuthGlobalWorkQueue.asyncAfter(deadline: deadline) {
         // Only cancel if the pending callbacks remain the same, i.e., not triggered yet.
         if applicableCallbacks.count == self.pendingCallbacks.count {
-          self.callback(withToken: nil, error: nil)
+          self.callback(.failure(AuthErrorUtils.missingAppTokenError(underlyingError: nil)))
         }
       }
     }
 
     func getToken() async throws -> AuthAPNSToken {
       return try await withCheckedThrowingContinuation { continuation in
-        self.getTokenInternal { token, error in
-          if let token {
+        self.getTokenInternal { result in
+          switch result {
+          case let .success(token):
             continuation.resume(returning: token)
-          } else {
-            continuation.resume(throwing: error!)
+          case let .failure(error):
+            continuation.resume(throwing: error)
           }
         }
       }
@@ -105,7 +106,7 @@
           newToken = AuthAPNSToken(withData: setToken.data, type: detectedTokenType)
         }
         tokenStore = newToken
-        callback(withToken: newToken, error: nil)
+        callback(.success(newToken))
       }
     }
 
@@ -115,18 +116,18 @@
     /// Cancels any pending `getTokenWithCallback:` request.
     /// - Parameter error: The error to return .
     func cancel(withError error: Error) {
-      callback(withToken: nil, error: error)
+      callback(.failure(error))
     }
 
     /// Enable unit test faking.
     var application: AuthAPNSTokenApplication
-    private var pendingCallbacks: [(AuthAPNSToken?, Error?) -> Void] = []
+    private var pendingCallbacks: [(Result<AuthAPNSToken, Error>) -> Void] = []
 
-    private func callback(withToken token: AuthAPNSToken?, error: Error?) {
+    private func callback(_ result: Result<AuthAPNSToken, Error>) {
       let pendingCallbacks = self.pendingCallbacks
       self.pendingCallbacks = []
       for callback in pendingCallbacks {
-        callback(token, error)
+        callback(result)
       }
     }
 
diff --git a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift
index 78b0295edb8..fdde2e0757d 100644
--- a/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift
+++ b/FirebaseAuth/Tests/SampleSwift/AuthenticationExample/ViewControllers/AuthViewController.swift
@@ -468,14 +468,14 @@ class AuthViewController: UIViewController, DataSourceProviderDelegate {
   }
 
   private func verifyClient() {
-    AppManager.shared.auth().tokenManager.getTokenInternal { token, error in
-      if token == nil {
+    AppManager.shared.auth().tokenManager.getTokenInternal { result in
+      guard case let .success(token) = result else {
         print("Verify iOS Client failed.")
         return
       }
       let request = VerifyClientRequest(
-        withAppToken: token?.string,
-        isSandbox: token?.type == .sandbox,
+        withAppToken: token.string,
+        isSandbox: token.type == .sandbox,
         requestConfiguration: AppManager.shared.auth().requestConfiguration
       )
 
diff --git a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift
index 585cd217b98..3406149ee90 100644
--- a/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift
+++ b/FirebaseAuth/Tests/Unit/AuthAPNSTokenManagerTests.swift
@@ -63,21 +63,29 @@
       XCTAssertFalse(fakeApplication!.registerCalled)
       var firstCallbackCalled = false
       let manager = try XCTUnwrap(manager)
-      manager.getTokenInternal { token, error in
+      manager.getTokenInternal { result in
         firstCallbackCalled = true
-        XCTAssertEqual(token?.data, self.data)
-        XCTAssertEqual(token?.type, .sandbox)
-        XCTAssertNil(error)
+        switch result {
+        case let .success(token):
+          XCTAssertEqual(token.data, self.data)
+          XCTAssertEqual(token.type, .sandbox)
+        case let .failure(error):
+          XCTFail("Unexpected error: \(error)")
+        }
       }
       XCTAssertFalse(firstCallbackCalled)
 
       // Add second callback, which is yet to be called either.
       var secondCallbackCalled = false
-      manager.getTokenInternal { token, error in
+      manager.getTokenInternal { result in
         secondCallbackCalled = true
-        XCTAssertEqual(token?.data, self.data)
-        XCTAssertEqual(token?.type, .sandbox)
-        XCTAssertNil(error)
+        switch result {
+        case let .success(token):
+          XCTAssertEqual(token.data, self.data)
+          XCTAssertEqual(token.type, .sandbox)
+        case let .failure(error):
+          XCTFail("Unexpected error: \(error)")
+        }
       }
       XCTAssertFalse(secondCallbackCalled)
 
@@ -96,11 +104,15 @@
 
       // Add third callback, which should be called back immediately.
       var thirdCallbackCalled = false
-      manager.getTokenInternal { token, error in
+      manager.getTokenInternal { result in
         thirdCallbackCalled = true
-        XCTAssertEqual(token?.data, self.data)
-        XCTAssertEqual(token?.type, .sandbox)
-        XCTAssertNil(error)
+        switch result {
+        case let .success(token):
+          XCTAssertEqual(token.data, self.data)
+          XCTAssertEqual(token.type, .sandbox)
+        case let .failure(error):
+          XCTFail("Unexpected error: \(error)")
+        }
       }
       XCTAssertTrue(thirdCallbackCalled)
 
@@ -123,9 +135,16 @@
 
       // Add callback to time out.
       let expectation = self.expectation(description: #function)
-      manager.getTokenInternal { token, error in
-        XCTAssertNil(token)
-        XCTAssertNil(error)
+      manager.getTokenInternal { result in
+        switch result {
+        case let .success(token):
+          XCTFail("Unexpected success: \(token)")
+        case let .failure(error):
+          XCTAssertEqual(
+            error as NSError,
+            AuthErrorUtils.missingAppTokenError(underlyingError: nil) as NSError
+          )
+        }
         expectation.fulfill()
       }
       // Time out.
@@ -153,9 +172,13 @@
 
       // Add callback to cancel.
       var callbackCalled = false
-      manager.getTokenInternal { token, error in
-        XCTAssertNil(token)
-        XCTAssertEqual(error as? NSError, self.error as NSError)
+      manager.getTokenInternal { result in
+        switch result {
+        case let .success(token):
+          XCTFail("Unexpected success: \(token)")
+        case let .failure(error):
+          XCTAssertEqual(error as NSError, self.error as NSError)
+        }
         XCTAssertFalse(callbackCalled) // verify callback is not called twice
         callbackCalled = true
       }
diff --git a/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift b/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift
index 9b1e1caaf30..0030c52776d 100644
--- a/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift
+++ b/FirebaseAuth/Tests/Unit/PhoneAuthProviderTests.swift
@@ -690,9 +690,9 @@
     }
 
     class FakeTokenManager: AuthAPNSTokenManager {
-      override func getTokenInternal(callback: @escaping (AuthAPNSToken?, Error?) -> Void) {
+      override func getTokenInternal(callback: @escaping (Result<AuthAPNSToken, Error>) -> Void) {
         let error = NSError(domain: "dummy domain", code: AuthErrorCode.missingAppToken.rawValue)
-        callback(nil, error)
+        callback(.failure(error))
       }
     }
 


Test patch: