fix(sessions): harden parsing of server-provided settings (#16752)

Crashlytics/CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+- [fixed] Improved robustness in Firebase Sessions when parsing malformed
+  settings from the server, including `null` values that could cause a crash
+  when caching settings. (#16728)
+
 # 13.0.0
 - [fixed] Safely validate memory reads when executing `DW_OP_deref_size`
   operations during DWARF stack unwinding (#16550).

FirebaseSessions/Sources/Settings/RemoteSettings.swift

@@ -75,11 +75,28 @@ final class RemoteSettings: SettingsProvider, Sendable {
   }
 
   var samplingRate: Double? {
-    cache.namespacedValue(forKey: RemoteSettings.flagSamplingRate)
+    guard let rate: Double = cache.namespacedValue(forKey: RemoteSettings.flagSamplingRate) else {
+      return nil
+    }
+    // Ignore values outside of [0, 1] (including NaN) so the SDK default is used instead.
+    guard (0.0 ... 1.0).contains(rate) else {
+      Logger.logDebug("[Settings] Ignoring invalid sampling rate: \(rate)")
+      return nil
+    }
+    return rate
   }
 
   var sessionTimeout: TimeInterval? {
-    cache.namespacedValue(forKey: RemoteSettings.flagSessionTimeout)
+    guard let timeout: TimeInterval = cache
+      .namespacedValue(forKey: RemoteSettings.flagSessionTimeout) else {
+      return nil
+    }
+    // Ignore negative or non-finite values so the SDK default is used instead.
+    guard timeout.isFinite, timeout >= 0 else {
+      Logger.logDebug("[Settings] Ignoring invalid session timeout: \(timeout)")
+      return nil
+    }
+    return timeout
   }
 
   func updateSettings(currentTime: Date) {

FirebaseSessions/Sources/Settings/SettingsCacheClient.swift

@@ -145,8 +145,12 @@ final class SettingsCache: SettingsCacheClient {
   }
 
   func updateContents(_ contents: [String: Any]) {
+    // Server-provided JSON may contain values that are not property list objects (e.g. `null`
+    // is parsed as `NSNull`). Writing those to user defaults raises an exception, so drop them.
+    let sanitizedContents = Self.propertyListCompatible(contents)
+
     // Write to in-memory cache directly (no flattening).
-    let container = SendableContainer(data: contents)
+    let container = SendableContainer(data: sanitizedContents)
 
     memoryCache.withLock { cache in
       cache = container.data
@@ -154,7 +158,27 @@ final class SettingsCache: SettingsCacheClient {
 
     // Write to disk cache.
     // We overwrite the entire key to match legacy behavior.
-    diskCache.setObject(contents, forKey: UserDefaultsKeys.forContent)
+    diskCache.setObject(sanitizedContents, forKey: UserDefaultsKeys.forContent)
+  }
+
+  /// Returns a copy of `dictionary` that recursively omits any values that cannot be stored in
+  /// a property list (such as `NSNull`).
+  private static func propertyListCompatible(_ dictionary: [String: Any]) -> [String: Any] {
+    dictionary.compactMapValues(propertyListCompatible(value:))
+  }
+
+  private static func propertyListCompatible(value: Any) -> Any? {
+    switch value {
+    case let dictionary as [String: Any]:
+      return propertyListCompatible(dictionary)
+    case let array as [Any]:
+      return array.compactMap(propertyListCompatible(value:))
+    case is String, is Bool, is NSNumber, is Date, is Data:
+      return value
+    default:
+      Logger.logDebug("[Settings] Dropping invalid settings value: \(value)")
+      return nil
+    }
   }
 
   func updateMetadata(_ metadata: CacheKey) {
@@ -214,6 +238,10 @@ final class SettingsCache: SettingsCacheClient {
     guard let duration = cacheDuration else {
       return Self.cacheDurationSecondsDefault
     }
+    guard duration.isFinite, duration >= 0 else {
+      Logger.logDebug("[Settings] Ignoring invalid cache duration: \(duration)")
+      return Self.cacheDurationSecondsDefault
+    }
     Logger.logDebug("[Settings] Cache duration: \(duration)")
     return duration
   }

FirebaseSessions/Tests/Unit/RemoteSettingsTests.swift

@@ -254,6 +254,184 @@ class RemoteSettingsTests: XCTestCase {
     XCTAssertEqual(settings.sessionTimeout, 20)
   }
 
+  // MARK: - Malformed server payloads
+
+  func test_nullValuesInResponse_areDroppedAndValidValuesPersisted() {
+    // Given
+    // JSONSerialization parses `null` as NSNull, which is not a valid property list value.
+    downloader.successResponse = [
+      "cache_duration": 10,
+      "unknown_root_key": NSNull(),
+      "features": [
+        "list": [1, NSNull(), "a"] as [Any],
+        "nested": ["key": NSNull()] as [String: Any],
+      ] as [String: Any],
+      "app_quality": [
+        "sessions_enabled": true,
+        "sampling_rate": NSNull(),
+        "session_timeout_seconds": 20,
+      ] as [String: Any],
+    ]
+    downloader.shouldSucceed = true
+
+    // When
+    settings.updateSettings(currentTime: date)
+
+    // Then
+    XCTAssertTrue(settings.sessionsEnabled!)
+    XCTAssertNil(settings.samplingRate)
+    XCTAssertEqual(settings.sessionTimeout, 20)
+
+    // Valid values were persisted to disk and are readable after an app restart.
+    cache = SettingsCache(namespace: "app_quality")
+    settings = RemoteSettings(appInfo: appInfo, downloader: downloader, cache: cache)
+    XCTAssertTrue(settings.sessionsEnabled!)
+    XCTAssertNil(settings.samplingRate)
+    XCTAssertEqual(settings.sessionTimeout, 20)
+    XCTAssertFalse(cache.isExpired(for: appInfo, time: date.addingTimeInterval(5)))
+  }
+
+  func test_wrongTypesInResponse_returnNilSettings() {
+    downloader.successResponse = [
+      "cache_duration": "10",
+      "app_quality": [
+        "sessions_enabled": "false",
+        "sampling_rate": "0.5",
+        "session_timeout_seconds": [10],
+      ] as [String: Any],
+    ]
+    downloader.shouldSucceed = true
+
+    settings.updateSettings(currentTime: date)
+
+    XCTAssertNil(settings.sessionsEnabled)
+    XCTAssertNil(settings.samplingRate)
+    XCTAssertNil(settings.sessionTimeout)
+  }
+
+  func test_namespaceWithWrongType_returnsNilSettings() {
+    downloader.successResponse = [
+      "cache_duration": 10,
+      "app_quality": ["sessions_enabled", false, "sampling_rate", 0.5] as [Any],
+    ]
+    downloader.shouldSucceed = true
+
+    settings.updateSettings(currentTime: date)
+
+    XCTAssertNil(settings.sessionsEnabled)
+    XCTAssertNil(settings.samplingRate)
+    XCTAssertNil(settings.sessionTimeout)
+  }
+
+  func test_outOfRangeValues_returnNilSettings() {
+    for (samplingRate, timeout) in [(-0.5, -1.0), (1.5, -Double.greatestFiniteMagnitude),
+                                    (Double.nan, Double.nan),
+                                    (Double.infinity, Double.infinity)] {
+      cache.removeCache()
+      downloader.successResponse = [
+        "cache_duration": 10,
+        "app_quality": [
+          "sampling_rate": samplingRate,
+          "session_timeout_seconds": timeout,
+        ] as [String: Any],
+      ]
+      downloader.shouldSucceed = true
+
+      settings.updateSettings(currentTime: date)
+
+      XCTAssertNil(settings.samplingRate, "sampling_rate: \(samplingRate)")
+      XCTAssertNil(settings.sessionTimeout, "session_timeout_seconds: \(timeout)")
+    }
+  }
+
+  func test_boundaryValues_areReturned() {
+    for (samplingRate, timeout) in [(0, 0), (1, 1_000_000_000)] {
+      cache.removeCache()
+      downloader.successResponse = [
+        "cache_duration": 10,
+        "app_quality": [
+          "sampling_rate": samplingRate,
+          "session_timeout_seconds": timeout,
+        ] as [String: Any],
+      ]
+      downloader.shouldSucceed = true
+
+      settings.updateSettings(currentTime: date)
+
+      XCTAssertEqual(settings.samplingRate, Double(samplingRate))
+      XCTAssertEqual(settings.sessionTimeout, TimeInterval(timeout))
+    }
+  }
+
+  func test_negativeCacheDuration_usesDefaultCacheDuration() {
+    // Given
+    downloader.successResponse = [
+      "cache_duration": -10,
+      "app_quality": validSettings["app_quality"]!,
+    ]
+    downloader.shouldSucceed = true
+    settings.updateSettings(currentTime: date)
+
+    // When
+    // time passed = 5, default TTL = 3600, time passed < TTL
+    downloader.successResponse = validSettings2
+    settings.updateSettings(currentTime: date.addingTimeInterval(5))
+
+    // Then
+    // fetch did not happen, still provide old cached settings
+    XCTAssertFalse(settings.sessionsEnabled!)
+    XCTAssertEqual(settings.samplingRate, 0.5)
+    XCTAssertEqual(settings.sessionTimeout, 10)
+
+    // When
+    // time passed > default TTL
+    settings.updateSettings(currentTime: date.addingTimeInterval(60 * 60 + 1))
+
+    // Then
+    XCTAssertTrue(settings.sessionsEnabled!)
+    XCTAssertEqual(settings.samplingRate, 0.2)
+    XCTAssertEqual(settings.sessionTimeout, 20)
+  }
+
+  func test_cacheContentWithWrongType_returnsNilSettings() {
+    // Given
+    downloader.shouldSucceed = true
+    settings.updateSettings(currentTime: date)
+
+    // When
+    // persisted settings are not a dictionary
+    UserDefaults.standard.set([1, 2, 3], forKey: "firebase-sessions-settings")
+    cache = SettingsCache(namespace: "app_quality")
+    settings = RemoteSettings(appInfo: appInfo, downloader: downloader, cache: cache)
+
+    // Then
+    XCTAssertNil(settings.sessionsEnabled)
+    XCTAssertNil(settings.samplingRate)
+    XCTAssertNil(settings.sessionTimeout)
+    XCTAssertTrue(cache.isExpired(for: appInfo, time: date))
+  }
+
+  func test_cacheKeyWithWrongSchema_fetchNewSettings() {
+    // Given
+    downloader.shouldSucceed = true
+    settings.updateSettings(currentTime: date)
+
+    // When
+    // cache-key is valid JSON data, but does not match the CacheKey schema
+    let data = Data(#"{"createdAt": "yesterday", "googleAppID": null}"#.utf8)
+    UserDefaults.standard.set(data, forKey: "firebase-sessions-cache-key")
+    cache = SettingsCache(namespace: "app_quality")
+    settings = RemoteSettings(appInfo: appInfo, downloader: downloader, cache: cache)
+
+    // Then
+    XCTAssertTrue(cache.isExpired(for: appInfo, time: date))
+    downloader.successResponse = validSettings2
+    settings.updateSettings(currentTime: date)
+    XCTAssertTrue(settings.sessionsEnabled!)
+    XCTAssertEqual(settings.samplingRate, 0.2)
+    XCTAssertEqual(settings.sessionTimeout, 20)
+  }
+
   func write(jsonString: String, isCacheKey: Bool) {
     let name = isCacheKey ? "firebase-sessions-cache-key" : "firebase-sessions-settings"
     UserDefaults.standard.set(jsonString, forKey: name)

FirebaseSessions/Tests/Unit/SessionsSettingsTests.swift

@@ -112,4 +112,32 @@ class SessionsSettingsTests: XCTestCase {
     XCTAssertEqual(sessionSettings.samplingRate, 0.8)
     XCTAssertEqual(sessionSettings.sessionTimeout, 50)
   }
+
+  func test_InvalidRemoteAndDefaultsPresent_DefaultConfigsApply() {
+    let invalidSettings: [String: Any] = [
+      "cache_duration": 10,
+      "app_quality": [
+        "sessions_enabled": NSNull(),
+        "sampling_rate": 2.5,
+        "session_timeout_seconds": -100,
+      ] as [String: Any],
+    ]
+
+    cache.removeCache()
+    downloader = MockSettingsDownloader(successResponse: invalidSettings)
+    remoteSettings = RemoteSettings(appInfo: appInfo, downloader: downloader, cache: cache)
+    remoteSettings.updateSettings(currentTime: Date())
+
+    sessionSettings = SessionsSettings(
+      appInfo: appInfo,
+      installations: MockInstallationsProtocol(),
+      sdkDefaults: sdkDefaultSettings,
+      localOverrides: localOverrideSettings,
+      remoteSettings: remoteSettings
+    )
+
+    XCTAssertTrue(sessionSettings.sessionsEnabled)
+    XCTAssertEqual(sessionSettings.samplingRate, 1.0)
+    XCTAssertEqual(sessionSettings.sessionTimeout, 30 * 60)
+  }
 }