diff --git a/src/components/marker-table/index.js b/src/components/marker-table/index.js index 6928167fd2..1f85812064 100644 --- a/src/components/marker-table/index.js +++ b/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); } diff --git a/src/components/tooltip/Marker.js b/src/components/tooltip/Marker.js index 672a5cb391..acd35d5778 100644 --- a/src/components/tooltip/Marker.js +++ b/src/components/tooltip/Marker.js @@ -230,7 +230,11 @@ class MarkerTooltipContents extends React.PureComponent { 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 diff --git a/src/profile-logic/marker-data.js b/src/profile-logic/marker-data.js index 3d31ed39aa..4ea8a869f6 100644 --- a/src/profile-logic/marker-data.js +++ b/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) + ); }); } diff --git a/src/profile-logic/marker-schema.js b/src/profile-logic/marker-schema.js index f8c8d7c029..01c8d499f4 100644 --- a/src/profile-logic/marker-schema.js +++ b/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 diff --git a/src/profile-logic/sanitize.js b/src/profile-logic/sanitize.js index 7b0f93e42a..e19f85b770 100644 --- a/src/profile-logic/sanitize.js +++ b/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, - windowIdFromActiveTab: Set + windowIdFromActiveTab: Set, + 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 ( diff --git a/src/selectors/publish.js b/src/selectors/publish.js index 23d9ad0dd5..914b222679 100644 --- a/src/selectors/publish.js +++ b/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 = getProfile, getDerivedMarkerInfoForAllThreads, getRemoveProfileInformation, + getMarkerSchemaByName, sanitizePII ); diff --git a/src/test/unit/merge-compare.test.js b/src/test/unit/merge-compare.test.js index 57fd303484..a35983e0d3 100644 --- a/src/test/unit/merge-compare.test.js +++ b/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 ); diff --git a/src/test/unit/sanitize.test.js b/src/test/unit/sanitize.test.js index 87e6337769..8265d53d18 100644 --- a/src/test/unit/sanitize.test.js +++ b/src/test/unit/sanitize.test.js @@ -65,10 +65,55 @@ describe('sanitizePII', function () { } ); + const markerSchemaByName = { + FileIO: { + name: 'FileIO', + display: ['marker-chart', 'marker-table', 'timeline-fileio'], + data: [ + { + key: 'operation', + label: 'Operation', + format: 'string', + searchable: true, + }, + { + key: 'source', + label: 'Source', + format: 'string', + searchable: true, + }, + { + key: 'filename', + label: 'Filename', + format: 'file-path', + searchable: true, + }, + { + key: 'threadId', + label: 'Thread ID', + format: 'string', + searchable: true, + }, + ], + }, + Url: { + name: 'Url', + tableLabel: '{marker.name} - {marker.data.url}', + display: ['marker-chart', 'marker-table'], + data: [ + { + key: 'url', + format: 'url', + }, + ], + }, + }; + const sanitizedProfile = sanitizePII( originalProfile, derivedMarkerInfoForAllThreads, - PIIToRemove + PIIToRemove, + markerSchemaByName ).profile; return { @@ -548,6 +593,38 @@ describe('sanitizePII', function () { expect(marker2.filename).toBe('\\' + marker2File); }); + it('should sanitize the URL properties in markers', function () { + const { sanitizedProfile } = setup( + { + shouldRemoveUrls: true, + }, + getProfileWithMarkers([ + [ + 'SpeculativeConnect', + 1, + 2, + { + type: 'Url', + url: 'https://img-getpocket.cdn.mozilla.net', + }, + ], + ]) + ); + expect(sanitizedProfile.threads.length).toEqual(1); + const thread = sanitizedProfile.threads[0]; + expect(thread.markers.length).toEqual(1); + + const marker = thread.markers.data[0]; + + // The url fields should still be there + if (!marker || !marker.url) { + throw new Error('Failed to find url property in the payload'); + } + + // Now check the url fields and make sure they are sanitized. + expect(marker.url).toBe('https://'); + }); + it('should sanitize the eTLD+1 field if urls are supposed to be sanitized', function () { // Create a simple profile with eTLD+1 field in its thread. const { profile } = getProfileFromTextSamples('A'); diff --git a/src/types/markers.js b/src/types/markers.js index e8c2921343..2d9ee679a7 100644 --- a/src/types/markers.js +++ b/src/types/markers.js @@ -20,9 +20,7 @@ export type MarkerFormatType = // String types. // Show the URL, and handle PII sanitization - // TODO Handle PII sanitization. Issue #2757 | 'url' - // TODO Handle PII sanitization. Issue #2757 // Show the file path, and handle PII sanitization. | 'file-path' // Important, do not put URL or file path information here, as it will not be @@ -734,6 +732,11 @@ export type NoPayloadUserData = {| innerWindowID?: number, |}; +export type UrlMarkerPayload = {| + type: 'Url', + url: string, +|}; + /** * The union of all the different marker payloads that profiler.firefox.com knows about, * this is not guaranteed to be all the payloads that we actually get from the Gecko @@ -766,7 +769,8 @@ export type MarkerPayload = | MediaSampleMarkerPayload | JankPayload | BrowsertimeMarkerPayload - | NoPayloadUserData; + | NoPayloadUserData + | UrlMarkerPayload; export type MarkerPayload_Gecko = | GPUMarkerPayload @@ -788,6 +792,7 @@ export type MarkerPayload_Gecko = | IPCMarkerPayload_Gecko | MediaSampleMarkerPayload | NoPayloadUserData + | UrlMarkerPayload // The following payloads come in with a stack property. During the profile processing // the "stack" property is are converted into a "cause". See the CauseBacktrace type // for more information.