Sanitize URL properties in markers

src/components/marker-table/index.js

@@ -123,7 +123,11 @@ class MarkerTree {
         start: _formatStart(marker.start, this._zeroAt),
         duration,
         name,
-        type: getMarkerSchemaName(this._markerSchemaByName, marker),
+        type: getMarkerSchemaName(
+          this._markerSchemaByName,
+          marker.name,
+          marker.data
+        ),
       };
       this._displayDataByIndex.set(markerIndex, displayData);
     }

src/components/tooltip/Marker.js

@@ -230,7 +230,11 @@ class MarkerTooltipContents extends React.PureComponent<Props> {
 
     if (data) {
       // Add the details for the markers based on their Marker schema.
-      const schema = getSchemaFromMarker(markerSchemaByName, marker);
+      const schema = getSchemaFromMarker(
+        markerSchemaByName,
+        marker.name,
+        marker.data
+      );
       if (schema) {
         for (const schemaData of schema.data) {
           // Check for a schema that is looking up and formatting a value from

src/profile-logic/marker-data.js

@@ -35,7 +35,6 @@ import type {
   IPCMarkerPayload,
   NetworkPayload,
   PrefMarkerPayload,
-  FileIoPayload,
   TextMarkerPayload,
   StartEndRange,
   IndexedArray,
@@ -161,7 +160,11 @@ export function getSearchFilteredMarkerIndexes(
       }
 
       // Now check the schema for the marker payload for searchable
-      const markerSchema = getSchemaFromMarker(markerSchemaByName, marker);
+      const markerSchema = getSchemaFromMarker(
+        markerSchemaByName,
+        marker.name,
+        marker.data
+      );
       if (
         markerSchema &&
         markerPayloadMatchesSearch(markerSchema, marker, regExp)
@@ -1264,22 +1267,6 @@ export function removePrefMarkerPreferenceValues(
   return { ...payload, prefValue: '' };
 }
 
-/**
- * Sanitize FileIO marker's filename property if it's non-empty.
- */
-export function sanitizeFileIOMarkerFilenamePath(
-  payload: FileIoPayload
-): FileIoPayload {
-  if (!payload.filename) {
-    return payload;
-  }
-
-  return {
-    ...payload,
-    filename: removeFilePath(payload.filename),
-  };
-}
-
 /**
  * Sanitize Text marker's name property for potential URLs.
  */
@@ -1316,6 +1303,41 @@ export function sanitizeExtensionTextMarker(
   return payload;
 }
 
+export function sanitizeFromMarkerSchema(
+  markerSchema: MarkerSchema,
+  markerPayload: MarkerPayload
+): MarkerPayload {
+  for (const propertyDescription of markerSchema.data) {
+    if (
+      propertyDescription.key !== undefined &&
+      propertyDescription.format !== undefined
+    ) {
+      const key = propertyDescription.key;
+      const format = propertyDescription.format;
+      if (!(key in markerPayload)) {
+        continue;
+      }
+
+      // We're typing the result of the sanitization with `any` because Flow
+      // doesn't like much our enormous enum of non-exact objects that's used as
+      // MarkerPayload type, and this code is too generic for Flow in this context.
+      if (format === 'url') {
+        markerPayload = ({
+          ...markerPayload,
+          [key]: removeURLs(markerPayload[key]),
+        }: any);
+      } else if (format === 'file-path') {
+        markerPayload = ({
+          ...markerPayload,
+          [key]: removeFilePath(markerPayload[key]),
+        }: any);
+      }
+    }
+  }
+
+  return markerPayload;
+}
+
 /**
  * Markers can be filtered by display area using the marker schema. Get a list of
  * marker "types" (the type field in the Payload) for a specific location.
@@ -1362,7 +1384,9 @@ export function filterMarkerByDisplayLocation(
       return additionalResult;
     }
 
-    return markerTypes.has(getMarkerSchemaName(markerSchemaByName, marker));
+    return markerTypes.has(
+      getMarkerSchemaName(markerSchemaByName, marker.name, marker.data)
+    );
   });
 }
 

src/profile-logic/marker-schema.js

@@ -22,6 +22,7 @@ import type {
   MarkerSchemaByName,
   Marker,
   MarkerIndex,
+  MarkerPayload,
 } from 'firefox-profiler/types';
 
 /**
@@ -86,34 +87,33 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [
  */
 export function getMarkerSchemaName(
   markerSchemaByName: MarkerSchemaByName,
-  marker: Marker
+  markerName: string,
+  markerData: MarkerPayload | null
 ): string {
-  const { data, name } = marker;
-  // Fall back to using the name if no payload exists.
-
-  if (data) {
-    const { type } = data;
-    if (type === 'tracing' && data.category) {
+  if (markerData) {
+    const { type } = markerData;
+    if (type === 'tracing' && markerData.category) {
       // TODO - Tracing markers have a duplicate "category" field.
       // See issue #2749
 
       // Does a marker schema for the "category" exist?
-      return markerSchemaByName[data.category] === undefined
+      return markerSchemaByName[markerData.category] === undefined
         ? // If not, default back to tracing
           'tracing'
         : // If so, use the category as the schema name.
-          data.category;
+          markerData.category;
     }
     if (type === 'Text') {
       // Text markers are a cheap and easy way to create markers with
       // a category. Check for schema if it exists, if not, fallback to
       // a Text type marker.
-      return markerSchemaByName[name] === undefined ? 'Text' : name;
+      return markerSchemaByName[markerName] === undefined ? 'Text' : markerName;
     }
-    return data.type;
+    return markerData.type;
   }
 
-  return name;
+  // Fall back to using the name if no payload exists.
+  return markerName;
 }
 
 /**
@@ -122,10 +122,13 @@ export function getMarkerSchemaName(
  */
 export function getSchemaFromMarker(
   markerSchemaByName: MarkerSchemaByName,
-  marker: Marker
+  markerName: string,
+  markerData: MarkerPayload | null
 ): MarkerSchema | null {
   return (
-    markerSchemaByName[getMarkerSchemaName(markerSchemaByName, marker)] || null
+    markerSchemaByName[
+      getMarkerSchemaName(markerSchemaByName, markerName, markerData)
+    ] || null
   );
 }
 
@@ -349,7 +352,11 @@ export function getLabelGetter(
     // No label exists, it will have to be generated for the first time.
     if (label === undefined) {
       const marker = getMarker(markerIndex);
-      const schemaName = getMarkerSchemaName(markerSchemaByName, marker);
+      const schemaName = getMarkerSchemaName(
+        markerSchemaByName,
+        marker.name,
+        marker.data
+      );
       const applyLabel = labelFns.get(schemaName);
 
       label = applyLabel

src/profile-logic/sanitize.js

@@ -14,11 +14,12 @@ import { removeURLs } from '../utils/string';
 import {
   removeNetworkMarkerURLs,
   removePrefMarkerPreferenceValues,
-  sanitizeFileIOMarkerFilenamePath,
   filterRawMarkerTableToRangeWithMarkersToDelete,
   sanitizeExtensionTextMarker,
   sanitizeTextMarker,
+  sanitizeFromMarkerSchema,
 } from './marker-data';
+import { getSchemaFromMarker } from './marker-schema';
 import { filterThreadSamplesToRange } from './profile-data';
 import type {
   Profile,
@@ -30,6 +31,7 @@ import type {
   IndexIntoFrameTable,
   IndexIntoFuncTable,
   InnerWindowID,
+  MarkerSchemaByName,
 } from 'firefox-profiler/types';
 
 export type SanitizeProfileResult = {|
@@ -47,7 +49,8 @@ export type SanitizeProfileResult = {|
 export function sanitizePII(
   profile: Profile,
   derivedMarkerInfoForAllThreads: DerivedMarkerInfo[],
-  maybePIIToBeRemoved: RemoveProfileInformation | null
+  maybePIIToBeRemoved: RemoveProfileInformation | null,
+  markerSchemaByName: MarkerSchemaByName
 ): SanitizeProfileResult {
   if (maybePIIToBeRemoved === null) {
     // Nothing is sanitized.
@@ -133,7 +136,8 @@ export function sanitizePII(
         threadIndex,
         PIIToBeRemoved,
         windowIdFromPrivateBrowsing,
-        windowIdFromActiveTab
+        windowIdFromActiveTab,
+        markerSchemaByName
       );
 
       // Filtering out the current thread if it's null.
@@ -227,7 +231,8 @@ function sanitizeThreadPII(
   threadIndex: number,
   PIIToBeRemoved: RemoveProfileInformation,
   windowIdFromPrivateBrowsing: Set<InnerWindowID>,
-  windowIdFromActiveTab: Set<InnerWindowID>
+  windowIdFromActiveTab: Set<InnerWindowID>,
+  markerSchemaByName: MarkerSchemaByName
 ): Thread | null {
   if (PIIToBeRemoved.shouldRemoveThreads.has(threadIndex)) {
     // If this is a hidden thread, remove the thread immediately.
@@ -274,40 +279,42 @@ function sanitizeThreadPII(
         markerTable.data[i] = removePrefMarkerPreferenceValues(currentMarker);
       }
 
-      // Remove the all network URLs if user wants to remove them.
-      if (
-        PIIToBeRemoved.shouldRemoveUrls &&
-        currentMarker &&
-        currentMarker.type === 'Network'
-      ) {
-        // Remove the URI fields from marker payload.
-        markerTable.data[i] = removeNetworkMarkerURLs(currentMarker);
+      if (currentMarker && PIIToBeRemoved.shouldRemoveUrls) {
+        // Use the schema to find some properties that need to be sanitized.
+        const markerNameIndex = markerTable.name[i];
+        const markerName = thread.stringTable.getString(markerNameIndex);
+        const markerSchema = getSchemaFromMarker(
+          markerSchemaByName,
+          markerName,
+          currentMarker
+        );
+        if (markerSchema) {
+          currentMarker = markerTable.data[i] = sanitizeFromMarkerSchema(
+            markerSchema,
+            currentMarker
+          );
+        }
 
-        // Strip the URL from the marker name
-        const stringIndex = markerTable.name[i];
-        stringArray[stringIndex] = stringArray[stringIndex].replace(/:.*/, '');
-      }
+        // Remove the network URLs if user wants to remove them.
+        if (currentMarker.type === 'Network') {
+          // Remove the URI fields from marker payload.
+          markerTable.data[i] = removeNetworkMarkerURLs(currentMarker);
 
-      // Remove the all OS paths from FileIO markers if user wants to remove them.
-      if (
-        PIIToBeRemoved.shouldRemoveUrls &&
-        currentMarker &&
-        currentMarker.type === 'FileIO'
-      ) {
-        // Remove the filename path from marker payload.
-        markerTable.data[i] = sanitizeFileIOMarkerFilenamePath(currentMarker);
-      }
+          // Strip the URL from the marker name
+          const stringIndex = markerTable.name[i];
+          stringArray[stringIndex] = stringArray[stringIndex].replace(
+            /:.*/,
+            ''
+          );
+        }
 
-      if (
-        PIIToBeRemoved.shouldRemoveUrls &&
-        currentMarker &&
-        currentMarker.type === 'Text'
-      ) {
-        // Sanitize all the name fields of text markers in case they contain URLs.
-        markerTable.data[i] = sanitizeTextMarker(currentMarker);
-        // Re-assign the value of currentMarker as the marker may be
-        // sanitized again to remove extension ids.
-        currentMarker = markerTable.data[i];
+        if (currentMarker.type === 'Text') {
+          // Sanitize all the name fields of text markers in case they contain URLs.
+          markerTable.data[i] = sanitizeTextMarker(currentMarker);
+          // Re-assign the value of currentMarker as the marker may be
+          // sanitized again to remove extension ids.
+          currentMarker = markerTable.data[i];
+        }
       }
 
       if (

src/selectors/publish.js

@@ -16,6 +16,7 @@ import {
   getContainsPrivateBrowsingInformation,
   getThreads,
   getActiveTabID,
+  getMarkerSchemaByName,
 } from './profile';
 import { compress } from '../utils/gz';
 import { serializeProfile } from '../profile-logic/process-profile';
@@ -222,6 +223,7 @@ export const getSanitizedProfile: Selector<SanitizeProfileResult> =
     getProfile,
     getDerivedMarkerInfoForAllThreads,
     getRemoveProfileInformation,
+    getMarkerSchemaByName,
     sanitizePII
   );
 

src/test/unit/merge-compare.test.js

@@ -481,7 +481,7 @@ describe('mergeThreads function', function () {
 
     // Check if we properly merged the string tables and have the correct url fields.
     const markerUrlsAfterMerge = mergedMarkers.data.map((markerData) =>
-      markerData && 'url' in markerData && markerData.url
+      markerData && 'url' in markerData && typeof markerData.url === 'number'
         ? markerData.url
         : null
     );