Bug 1244295 - Validate client IDs before sending them in a Telemtry report. r=mfinkle

Additionally, we log some of the Exceptions thrown while retrieving the client
ID to make it clearer what is happening. The underlying GeckoProfile methods
ensure the profile path is not printed so we don't have to worry about leaking
that.

MozReview-Commit-ID: 3o0rvXDRZzM
This commit is contained in:
Michael Comella 2016-02-17 12:22:18 -08:00
parent 71c7ea29f1
commit 5f3968b2e9
2 changed files with 34 additions and 21 deletions

View File

@ -619,31 +619,22 @@ public final class GeckoProfile {
*
* @throws IOException if the client ID could not be retrieved.
*/
// Mimics ClientID.jsm _doLoadClientID.
@WorkerThread
public String getClientId() throws IOException {
final JSONObject obj = getClientIdJSONObject();
try {
return obj.getString(CLIENT_ID_JSON_ATTR);
} catch (final JSONException e) {
// Don't log to avoid leaking data in JSONObject.
throw new IOException("Client ID does not exist in JSONObject");
}
return getValidClientIdFromDisk(CLIENT_ID_FILE_PATH);
} catch (final IOException e) {
// Avoid log spam: don't log the full Exception w/ the stack trace.
Log.d(LOGTAG, "Could not get client ID - attempting to migrate ID from FHR: " + e.getLocalizedMessage());
}
// Mimics ClientID.jsm _doLoadClientID. One exception is that we don't validate client IDs like it does.
@WorkerThread
private JSONObject getClientIdJSONObject() throws IOException {
try {
return readJSONObjectFromFile(CLIENT_ID_FILE_PATH);
} catch (final IOException e) { /* No contemporary client ID: fallthrough. */ }
Log.d(LOGTAG, "Could not get client ID attempting to migrate ID from FHR");
String clientIdToWrite;
try {
final JSONObject fhrClientIdObj = readJSONObjectFromFile(FHR_CLIENT_ID_FILE_PATH);
clientIdToWrite = fhrClientIdObj.getString(CLIENT_ID_JSON_ATTR);
} catch (final IOException|JSONException e) {
Log.d(LOGTAG, "Could not migrate client ID from FHR creating a new one");
clientIdToWrite = getValidClientIdFromDisk(FHR_CLIENT_ID_FILE_PATH);
} catch (final IOException e) {
// Avoid log spam: don't log the full Exception w/ the stack trace.
Log.d(LOGTAG, "Could not migrate client ID from FHR creating a new one: " + e.getLocalizedMessage());
clientIdToWrite = UUID.randomUUID().toString();
}
@ -657,7 +648,21 @@ public final class GeckoProfile {
//
// In any case, if we get an exception, intentionally throw - there's nothing more to do here.
persistClientId(clientIdToWrite);
return readJSONObjectFromFile(CLIENT_ID_FILE_PATH);
return getValidClientIdFromDisk(CLIENT_ID_FILE_PATH);
}
/**
* @return a valid client ID
* @throws IOException if a valid client ID could not be retrieved
*/
@WorkerThread
private String getValidClientIdFromDisk(final String filePath) throws IOException {
final JSONObject obj = readJSONObjectFromFile(filePath);
final String clientId = obj.optString(CLIENT_ID_JSON_ATTR);
if (isClientIdValid(clientId)) {
return clientId;
}
throw new IOException("Received client ID is invalid: " + clientId);
}
@WorkerThread
@ -678,6 +683,15 @@ public final class GeckoProfile {
writeFile(CLIENT_ID_FILE_PATH, obj.toString()); // Logs errors within function: ideally we'd throw.
}
// From ClientID.jsm - isValidClientID.
public static boolean isClientIdValid(final String clientId) {
// We could use UUID.fromString but, for consistency, we take the implementation from ClientID.jsm.
if (TextUtils.isEmpty(clientId)) {
return false;
}
return clientId.matches("(?i:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})");
}
/**
* @return the profile creation date in the format returned by {@link System#currentTimeMillis()} or -1 if the value
* was not found.

View File

@ -172,8 +172,7 @@ public class TelemetryUploadService extends BackgroundService {
try {
clientId = profile.getClientId();
} catch (final IOException e) {
// Don't log the exception to avoid leaking the profile path.
Log.w(LOGTAG, "Unable to get client ID to generate core ping: returning.");
Log.w(LOGTAG, "Unable to get client ID to generate core ping: returning.", e);
return;
}