Skip to content

Logs all network requests to Analytics #3352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shubham1g5 wants to merge 1 commit into from
Closed

Logs all network requests to Analytics #3352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions app/src/org/commcare/connect/network/ConnectNetworkServiceFactory.kt
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion app/src/org/commcare/connect/network/base/BaseApiClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import okhttp3.Interceptor
import okhttp3.OkHttpClient
import okhttp3.logging.HttpLoggingInterceptor
import org.commcare.dalvik.BuildConfig
import org.commcare.network.NetworkAnalyticsInterceptor
import retrofit2.Retrofit
import retrofit2.converter.gson.GsonConverterFactory
import java.util.concurrent.TimeUnit
Expand All @@ -22,6 +23,7 @@ object BaseApiClient {

val okHttpClient: OkHttpClient = OkHttpClient.Builder()
.addInterceptor(logging)
.addInterceptor(NetworkAnalyticsInterceptor())
.addInterceptor(Interceptor { chain: Interceptor.Chain ->
val originalRequest = chain.request()
val requestWithHeadersBuilder = originalRequest.newBuilder()
Expand All @@ -44,4 +46,4 @@ object BaseApiClient {
.addConverterFactory(GsonConverterFactory.create())
.build()
}
}
}
1 change: 1 addition & 0 deletions app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,6 @@ public class CCAnalyticsEvent {
static final String PERSONAL_ID_CONFIGURATION_FAILURE = "personal_id_configuration_failure";
static final String NAV_DRAWER_OPEN = "nav_drawer_open";
static final String NAV_DRAWER_ITEM_SELECTED = "nav_drawer_item_selected";
static final String NETWORK_REQUEST = "network_request";
Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition


}
4 changes: 4 additions & 0 deletions app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,8 @@ public class CCAnalyticsParam {
static final String PARAM_API_SUCCESS = "ccc_api_success";
static final String PARAM_API_TOTAL_JOBS = "ccc_api_total_jobs";
static final String PARAM_API_NEW_JOBS = "ccc_api_new_jobs";
Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting the overlap with the ccc_api_* events. These can log additional info in some cases though, such as number of new jobs received. I think fine to leave them all in for now, but I wonder if there's a future world we could get to where network_request events could be the single event and optionally include additional analytics info depending on the call? Recognizing this may be difficult with the more-generic NetworkAnalyticsInterceptor...

Copy link
Contributor Author

@shubham1g5 shubham1g5 Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, I think based on the overall feedback , I am going to move away from interceptor approach and instead switch to have this logic as part of generalised network callbacks.


static final String RESPONSE_CODE = "response_code";
static final String REQUEST_DURATION_MS = "request_duration_ms";
static final String URL = "url";
Comment on lines +44 to +46
Copy link

@coderabbitai coderabbitai bot Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Consider URL sanitization for privacy.

The URL parameter will log complete request URLs, which may contain sensitive information such as authentication tokens, API keys, user IDs, or PII in query parameters or path segments. Consider sanitizing URLs before logging them to analytics.

Example sanitization approaches:

  • Strip query parameters entirely
  • Redact path segments that contain IDs or sensitive data
  • Use allowlist of safe parameters/paths
 static final String RESPONSE_CODE = "response_code";
 static final String REQUEST_DURATION_MS = "request_duration_ms";
-static final String URL = "url";
+static final String URL = "url"; // Note: URLs should be sanitized before logging to remove sensitive data

You may want to implement sanitization in NetworkAnalyticsInterceptor.logNetworkAnalytics() or FirebaseAnalyticsUtil.reportNetworkRequest().

🏁 Script executed:

#!/bin/bash
# Locate references to the URL analytics parameter
rg -n "CCAnalyticsParam\.URL" -C3

# Find and inspect the logNetworkAnalytics method
rg -n "logNetworkAnalytics" -C3

# Find and inspect the reportNetworkRequest method
rg -n "reportNetworkRequest" -C3

Length of output: 3419

Sanitize URLs before logging
Both NetworkAnalyticsInterceptor.logNetworkAnalytics(request.url.toString(),…) and FirebaseAnalyticsUtil.reportNetworkRequest(url,…) send raw URLs to analytics, risking exposure of tokens, PII, or sensitive query parameters. Strip or redact sensitive data—e.g., remove query strings entirely, mask path segments containing IDs, or whitelist safe parameters—before logging.

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, might need to remove PII or token

Copy link

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jignesh-dimagi Thank you for confirming! URL sanitization is indeed important to prevent exposing sensitive data in analytics.

If you'd like, I can help design the sanitization logic. Common approaches include:

  1. Strip all query parameters — remove everything after ?
  2. Redact path segments — replace UUIDs, IDs, tokens with placeholders like [REDACTED]
  3. Domain + path pattern only — log just the domain and endpoint structure (e.g., /api/v1/users/[ID]/profile)

Would you like me to suggest specific implementation code for the NetworkAnalyticsInterceptor or FirebaseAnalyticsUtil to handle this sanitization?

}
40 changes: 23 additions & 17 deletions app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,30 @@
package org.commcare.google.services.analytics;

import static org.commcare.google.services.analytics.AnalyticsParamValue.CORRUPT_APP_STATE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.RSA_KEYSTORE_KEY_RETRIEVAL;
import static org.commcare.google.services.analytics.AnalyticsParamValue.STAGE_UPDATE_FAILURE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.UPDATE_RESET;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_FULL;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_IMMEDIATE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_LENGTH_UNKNOWN;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_MOST;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_OTHER;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_PARTIAL;

import android.os.Bundle;
import android.os.Environment;
import android.text.TextUtils;

import androidx.navigation.NavController;
import androidx.navigation.NavDestination;
import androidx.navigation.fragment.FragmentNavigator;

import com.google.firebase.analytics.FirebaseAnalytics;

import org.commcare.CommCareApplication;
import org.commcare.DiskUtils;
import org.commcare.android.database.connect.models.ConnectUserRecord;
import org.commcare.android.logging.ReportingUtils;
import org.commcare.connect.PersonalIdManager;
import org.commcare.connect.database.ConnectUserDatabaseUtil;
import org.commcare.dalvik.BuildConfig;
import org.commcare.preferences.MainConfigurablePreferences;
import org.commcare.suite.model.OfflineUserRestore;
Expand All @@ -21,21 +34,6 @@

import java.util.Date;

import androidx.navigation.NavController;
import androidx.navigation.NavDestination;
import androidx.navigation.fragment.FragmentNavigator;

import static org.commcare.google.services.analytics.AnalyticsParamValue.CORRUPT_APP_STATE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.RSA_KEYSTORE_KEY_RETRIEVAL;
import static org.commcare.google.services.analytics.AnalyticsParamValue.STAGE_UPDATE_FAILURE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.UPDATE_RESET;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_FULL;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_IMMEDIATE;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_LENGTH_UNKNOWN;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_MOST;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_OTHER;
import static org.commcare.google.services.analytics.AnalyticsParamValue.VIDEO_USAGE_PARTIAL;

/**
* Created by amstone326 on 10/13/17.
*/
Expand Down Expand Up @@ -586,4 +584,12 @@ public static void reportNavDrawerItemSelected(String selectedItem) {
new String[]{selectedItem});
}

public static void reportNetworkRequest(String url, int responseCode, long durationMs) {
Bundle bundle = new Bundle();
bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode);
bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs);
bundle.putString(CCAnalyticsParam.URL, url);
reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle);
}
Comment on lines +587 to +593
Copy link

@coderabbitai coderabbitai bot Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: URLs may contain sensitive data (auth tokens, PII, PHI).

Logging full URLs to analytics can expose:

  • Authentication tokens in query parameters or headers
  • User identifiers, session IDs, or personally identifiable information
  • Protected health information (PHI) in paths or parameters
  • Internal server paths or API structure

Additionally, Firebase Analytics truncates parameter values to 100 characters (line 61-62), which may:

  • Still leak partial sensitive data
  • Render URLs incomplete and less useful for debugging
  • Create compliance risks (GDPR, CCPA, HIPAA)

Recommendations:

  1. Sanitize URLs before logging - remove query parameters, hash sensitive path segments, or use URL patterns:
 public static void reportNetworkRequest(String url, int responseCode, long durationMs) {
     Bundle bundle = new Bundle();
     bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode);
     bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs);
-    bundle.putString(CCAnalyticsParam.URL, url);
+    bundle.putString(CCAnalyticsParam.URL, sanitizeUrl(url));
     reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle);
 }
+
+private static String sanitizeUrl(String url) {
+    try {
+        java.net.URL parsed = new java.net.URL(url);
+        // Return only protocol, host, and path (no query params)
+        return parsed.getProtocol() + "://" + parsed.getHost() + parsed.getPath();
+    } catch (Exception e) {
+        return "invalid_url";
+    }
+}

  1. Consider logging only the endpoint pattern (e.g., /api/v1/sync instead of full URL).

  2. Apply rate limiting similar to other high-frequency events (see reportFormNav at line 222).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public static void reportNetworkRequest(String url, int responseCode, long durationMs) {
Bundle bundle = new Bundle();
bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode);
bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs);
bundle.putString(CCAnalyticsParam.URL, url);
reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle);
}
public static void reportNetworkRequest(String url, int responseCode, long durationMs) {
Bundle bundle = new Bundle();
bundle.putInt(CCAnalyticsParam.RESPONSE_CODE, responseCode);
bundle.putLong(CCAnalyticsParam.REQUEST_DURATION_MS, durationMs);
bundle.putString(CCAnalyticsParam.URL, sanitizeUrl(url));
reportEvent(CCAnalyticsEvent.NETWORK_REQUEST, bundle);
}
private static String sanitizeUrl(String url) {
try {
java.net.URL parsed = new java.net.URL(url);
// Return only protocol, host, and path (no query params)
return parsed.getProtocol() + "://" + parsed.getHost() + parsed.getPath();
} catch (Exception e) {
return "invalid_url";
}
}
🤖 Prompt for AI Agents 
In app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java
around lines 587 to 593, do not log full raw URLs (they may contain auth tokens,
PII/PHI and Firebase truncation can still leak data); instead sanitize the URL
before building the Bundle by stripping query parameters and fragments,
replacing sensitive path segments with generic placeholders or a deterministic
hash, and/or mapping the URL to an endpoint pattern (e.g., "/api/v1/sync"); then
log only the sanitized pattern or hashed segment plus the responseCode and
durationMs; also apply the existing rate-limiting approach used by reportFormNav
to limit frequency of NETWORK_REQUEST events.

Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jignesh-dimagi Thank you for acknowledging this security concern!

Given the critical nature of potentially exposing sensitive data (auth tokens, PII, PHI) through URL logging, I wanted to check: are you planning to address the URL sanitization in this PR, or would you like me to create a follow-up issue to track this separately?

If you'd like an issue created, I can include a reference to this discussion and the suggested sanitization approach.


}
40 changes: 40 additions & 0 deletions app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.commcare.network

import okhttp3.Interceptor
import okhttp3.Response
import org.commcare.google.services.analytics.FirebaseAnalyticsUtil
import org.javarosa.core.services.Logger

/**
* OkHttp interceptor that logs analytics events for all HTTP requests/responses
*/
class NetworkAnalyticsInterceptor : Interceptor {
Copy link
Contributor

@Jignesh-dimagi Jignesh-dimagi Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shubham1g5 We are trying to log all network calls, which might be available in AWS EC2 instance with corresponding delays. These might be too much data to handle and might not be useful at the end?

Copy link
Contributor Author

@shubham1g5 shubham1g5 Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are making a shift to log more and more data to analytics (now that we don't have sumologic) , so I would not be worried about this data size untill it starts creating practical issues for us.

I am also not sure how will we filter what requests we want to log and what not.


override fun intercept(chain: Interceptor.Chain): Response {
val request = chain.request()
val startTime = System.currentTimeMillis()
var response: Response?
var responseCode = -1

try {
response = chain.proceed(request)
responseCode = response.code
return response
} finally {
val duration = System.currentTimeMillis() - startTime
logNetworkAnalytics(request.url.toString(), responseCode, duration)
Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that any exceptions sending the request will result in a -1 responseCode with no additional exception info. I wonder if we should attempt to distinguish the various possible exceptions with different negative error codes?

}
}
Comment on lines +13 to +27
Copy link

@coderabbitai coderabbitai bot Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify FirebaseAnalyticsUtil.reportNetworkRequest implementation.

The interceptor will invoke FirebaseAnalyticsUtil.reportNetworkRequest() for every HTTP request in the app. Ensure this method:

  1. Sanitizes URLs to remove sensitive data (tokens, PII, etc.)
  2. Handles high request volumes efficiently (async/batched)
  3. Respects analytics opt-out preferences if applicable

🏁 Script executed:

#!/bin/bash
# Find and examine the FirebaseAnalyticsUtil.reportNetworkRequest implementation

# Search for reportNetworkRequest method definition
ast-grep --pattern $'class FirebaseAnalyticsUtil {
  $$$
  reportNetworkRequest($$$) {
    $$$
  }
  $$$
}'

# Also search with ripgrep as fallback
rg -n -A 10 'fun reportNetworkRequest|void reportNetworkRequest|public.*reportNetworkRequest' --type=java --type=kotlin

Length of output: 1100

Enhance reportNetworkRequest to sanitize URLs, use async/batching, and respect opt-out
FirebaseAnalyticsUtil.java:587 currently logs full URLs, fires each event synchronously, and ignores analytics settings. Update it to:

  • Mask or remove sensitive query parameters before logging
  • Dispatch events off the calling thread or batch them to avoid overhead
  • Check and honor user analytics opt-out before reporting
🤖 Prompt for AI Agents 
In app/src/org/commcare/network/NetworkAnalyticsInterceptor.kt around lines 13
to 27, the interceptor currently logs full URLs synchronously and ignores user
analytics settings; update it to (1) sanitize the request URL before logging by
removing or masking sensitive query parameters (e.g., password, token, auth,
ssn, session, access_token) or stripping the query string entirely when present,
(2) dispatch the analytics call off the network thread—use a background
coroutine, executor, or a batching queue to send events asynchronously or
aggregate events and flush periodically to avoid per-request overhead, and (3)
check the application’s analytics/opt-out setting before sending (call the
existing analytics opt-out API or a central AnalyticsUtil.isEnabled method) and
skip reporting if the user has disabled analytics.


private fun logNetworkAnalytics(
url: String,
responseCode: Int,
duration: Long,
) {
try {
FirebaseAnalyticsUtil.reportNetworkRequest(url, responseCode, duration)
} catch (e: Exception) {
Copy link
Contributor

@OrangeAndGreen OrangeAndGreen Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to adding safety to our analytics calls and silently reporting exceptions, I think we've seen a crash or two come from such calls in the past. But I think we should make it comprehensive, rather than the caller's responsibility as done here.

Looking in FirebaseAnalyticsUtil it seems we may be close to accomplishing this already.

  • reportEvent(String,String[],String[]) is wrapped in a try-catch
  • reportEvent(String,Bundle) is not

The first calls the second, but there are 16 additional usages of the second. Seems a little cleanup here would globally guarantee safety for analytics calls.

Copy link
Contributor Author

@shubham1g5 shubham1g5 Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for flagging, I was under assumption that all calls are fail safe but I can get that in as part of this work.

Logger.exception("Error logging network analytics", e)
}
}
}
5 changes: 3 additions & 2 deletions app/src/org/commcare/network/OkHttpBuilderCustomConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

/**
* The purpose of this class is to offer a single entry point for additional OkHttpClient.Builder
* configs. As it stands, it serves to attach the ISRG Root X1 Certificate and enable/disable
* Certificate Transparency
* configs. As it stands, it serves to attach the ISRG Root X1 Certificate, enable/disable
* Certificate Transparency, and add network analytics logging.
*/
public class OkHttpBuilderCustomConfig implements HttpBuilderConfig {

Expand All @@ -22,6 +22,7 @@ public OkHttpClient.Builder performCustomConfig(OkHttpClient.Builder okHttpBuild
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.N_MR1) {
ISRGCertConfig.attachISRGRootCertificate(okHttpBuilder);
}
okHttpBuilder.addInterceptor(new NetworkAnalyticsInterceptor());

return okHttpBuilder;
}
Expand Down
Loading