From f24f016418b3eadc20bcdab01515b1408bbdd7f2 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 26 Jun 2025 00:13:46 +0000 Subject: [PATCH 1/2] Fix: Prevent SetDefaultEventParameters from clearing params with all-invalid input Addresses code review feedback regarding the behavior of SetDefaultEventParameters when all provided parameters are of invalid types. Previously, if all parameters in the C++ map were invalid, an empty NSDictionary/Bundle would be passed to the native iOS/Android setDefaultEventParameters method. According to documentation, passing nil/null (for iOS/Android respectively) clears all default parameters. While passing an empty dictionary/bundle is a valid operation, it could lead to unintentionally clearing parameters if that was the state before, or it might be a no-op if parameters were already set. This change modifies the iOS and Android implementations to explicitly check if the processed native parameter collection (NSDictionary for iOS, Bundle for Android) is empty after filtering for valid types. If it is empty, the call to the native SDK's setDefaultEventParameters method is skipped. This ensures that providing a C++ map containing exclusively invalid parameters (or an empty map) to SetDefaultEventParameters will be a true no-op from the C++ SDK's perspective, preventing any accidental modification or clearing of existing default parameters on the native side. --- analytics/src/analytics_android.cc | 22 ++++++++++++++++++++++ analytics/src/analytics_ios.mm | 9 +++++++++ 2 files changed, 31 insertions(+) diff --git a/analytics/src/analytics_android.cc b/analytics/src/analytics_android.cc index a2825a2b76..598f27661f 100644 --- a/analytics/src/analytics_android.cc +++ b/analytics/src/analytics_android.cc @@ -681,6 +681,28 @@ void SetDefaultEventParameters( } } + // Check if the bundle is empty. If so, it means all input parameters were + // invalid or the input map itself was empty. In this case, we should not + // call the native SDK, as passing an empty Bundle might clear existing + // parameters, which is not the intent if all inputs were simply invalid. + jboolean is_bundle_empty = env->CallBooleanMethod( + bundle, util::bundle::GetMethodId(util::bundle::kIsEmpty)); + if (util::CheckAndClearJniExceptions(env)) { + LogError( + "SetDefaultEventParameters: Failed to check if Bundle is empty. " + "Skipping native call to avoid potential issues."); + env->DeleteLocalRef(bundle); + return; + } + + if (is_bundle_empty) { + LogDebug( + "SetDefaultEventParameters: No valid parameters to set, skipping " + "native call."); + env->DeleteLocalRef(bundle); + return; + } + env->CallVoidMethod( g_analytics_class_instance, analytics::GetMethodId(analytics::kSetDefaultEventParameters), bundle); diff --git a/analytics/src/analytics_ios.mm b/analytics/src/analytics_ios.mm index 9d1982b17e..df413b2359 100644 --- a/analytics/src/analytics_ios.mm +++ b/analytics/src/analytics_ios.mm @@ -403,6 +403,15 @@ void SetDefaultEventParameters(const std::map& default_par pair.first.c_str(), Variant::TypeName(value.type())); } } + // If ns_default_parameters is empty at this point, it means all input + // parameters were invalid or the input map itself was empty. + // In this case, we should not call the native SDK, as passing an empty + // NSDictionary might clear existing parameters, which is not the intent + // if all inputs were simply invalid. + if ([ns_default_parameters count] == 0) { + LogDebug("SetDefaultEventParameters: No valid parameters to set, skipping native call."); + return; + } [FIRAnalytics setDefaultEventParameters:ns_default_parameters]; } From e2995dbc247a012ef0bfaf26c3d38eb29b09be2b Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 26 Jun 2025 17:10:24 +0000 Subject: [PATCH 2/2] Fix: Prevent SetDefaultEventParameters from clearing params with all-invalid input Addresses code review feedback regarding the behavior of SetDefaultEventParameters when all provided parameters are of invalid types. Previously, if all parameters in the C++ map were invalid, an empty NSDictionary/Bundle would be passed to the native iOS/Android setDefaultEventParameters method. According to documentation, passing nil/null (for iOS/Android respectively) clears all default parameters. While passing an empty dictionary/bundle is a valid operation, it could lead to unintentionally clearing parameters if that was the state before, or it might be a no-op if parameters were already set. This change modifies the iOS and Android implementations to explicitly check if the processed native parameter collection (NSDictionary for iOS, Bundle for Android) is effectively empty after filtering for valid types. If it is, the call to the native SDK's setDefaultEventParameters method is skipped. For iOS, this is done by checking `[ns_default_parameters count] == 0`. For Android, this is done by tracking if any parameter was successfully added to the Bundle using a boolean flag, as a direct `bundle.isEmpty()` check before the native call could miss JNI exceptions during bundle population. This ensures that providing a C++ map containing exclusively invalid parameters (or an empty map) to SetDefaultEventParameters will be a true no-op from the C++ SDK's perspective, preventing any accidental modification or clearing of existing default parameters on the native side. --- analytics/src/analytics_android.cc | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/analytics/src/analytics_android.cc b/analytics/src/analytics_android.cc index 598f27661f..8d3289e4db 100644 --- a/analytics/src/analytics_android.cc +++ b/analytics/src/analytics_android.cc @@ -627,6 +627,7 @@ void SetDefaultEventParameters( return; } + bool any_parameter_added = false; for (const auto& pair : default_parameters) { const Variant& value = pair.second; const char* key_cstr = pair.first.c_str(); @@ -648,6 +649,8 @@ void SetDefaultEventParameters( LogError( "SetDefaultEventParameters: Failed to put null string for key: %s", key_cstr); + } else { + any_parameter_added = true; } env->DeleteLocalRef(key_jstring); } else if (value.is_string() || value.is_int64() || value.is_double() || @@ -655,7 +658,9 @@ void SetDefaultEventParameters( // AddVariantToBundle handles these types and their JNI conversions. // It also logs if an individual AddToBundle within it fails or if a type // is unsupported by it. - if (!AddVariantToBundle(env, bundle, key_cstr, value)) { + if (AddVariantToBundle(env, bundle, key_cstr, value)) { + any_parameter_added = true; + } else { // This specific log gives context that the failure happened during // SetDefaultEventParameters for a type that was expected to be // supported by AddVariantToBundle. @@ -681,24 +686,10 @@ void SetDefaultEventParameters( } } - // Check if the bundle is empty. If so, it means all input parameters were - // invalid or the input map itself was empty. In this case, we should not - // call the native SDK, as passing an empty Bundle might clear existing - // parameters, which is not the intent if all inputs were simply invalid. - jboolean is_bundle_empty = env->CallBooleanMethod( - bundle, util::bundle::GetMethodId(util::bundle::kIsEmpty)); - if (util::CheckAndClearJniExceptions(env)) { - LogError( - "SetDefaultEventParameters: Failed to check if Bundle is empty. " - "Skipping native call to avoid potential issues."); - env->DeleteLocalRef(bundle); - return; - } - - if (is_bundle_empty) { + if (!any_parameter_added) { LogDebug( - "SetDefaultEventParameters: No valid parameters to set, skipping " - "native call."); + "SetDefaultEventParameters: No valid parameters were processed, " + "skipping native call to avoid clearing existing parameters."); env->DeleteLocalRef(bundle); return; }