diff --git a/Crashlytics/CHANGELOG.md b/Crashlytics/CHANGELOG.md index 28e48f45a6..3b512ddc38 100644 --- a/Crashlytics/CHANGELOG.md +++ b/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). diff --git a/FirebaseSessions/Sources/Settings/RemoteSettings.swift b/FirebaseSessions/Sources/Settings/RemoteSettings.swift index 5c84d5ff9f..0e953601c8 100644 --- a/FirebaseSessions/Sources/Settings/RemoteSettings.swift +++ b/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) { diff --git a/FirebaseSessions/Sources/Settings/SettingsCacheClient.swift b/FirebaseSessions/Sources/Settings/SettingsCacheClient.swift index 4a4875cde8..b30c840127 100644 --- a/FirebaseSessions/Sources/Settings/SettingsCacheClient.swift +++ b/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 } diff --git a/FirebaseSessions/Tests/Unit/RemoteSettingsTests.swift b/FirebaseSessions/Tests/Unit/RemoteSettingsTests.swift index d048c17d8a..fff2827afd 100644 --- a/FirebaseSessions/Tests/Unit/RemoteSettingsTests.swift +++ b/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) diff --git a/FirebaseSessions/Tests/Unit/SessionsSettingsTests.swift b/FirebaseSessions/Tests/Unit/SessionsSettingsTests.swift index f31966d77d..d6367b8d68 100644 --- a/FirebaseSessions/Tests/Unit/SessionsSettingsTests.swift +++ b/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) + } }