From ae430913b9c5c34f2d564fd7006ab4b182df50eb Mon Sep 17 00:00:00 2001 From: fatadel Date: Mon, 31 Aug 2026 14:58:02 +0200 Subject: [PATCH] Drive PII sanitization from marker schemas Markers requiring PII sanitization were identified through hardcoded `data.type` checks. Describing PII categories in marker schema fields removes type-specific branching while leaving sanitization behavior to the consumer. --- docs-developer/CHANGELOG-formats.md | 4 + src/app-logic/constants.ts | 2 +- src/profile-logic/marker-data.ts | 238 ++++++++++-------- src/profile-logic/marker-schema.ts | 56 ++++- src/profile-logic/process-profile.ts | 9 +- .../processed-profile-versioning.ts | 42 ++++ src/profile-logic/sanitize.ts | 98 +------- src/test/fixtures/profiles/marker-schema.ts | 7 +- .../__snapshots__/profiler-edit.test.ts.snap | 8 +- .../__snapshots__/profile-view.test.ts.snap | 9 +- .../profile-conversion.test.ts.snap | 36 +-- .../profile-upgrading.test.ts.snap | 110 +++++++- src/test/unit/process-profile.test.ts | 56 +++++ src/test/unit/profile-upgrading.test.ts | 69 +++++ src/test/unit/sanitize.test.ts | 154 +++++++----- src/types/markers.ts | 8 + 16 files changed, 612 insertions(+), 294 deletions(-) diff --git a/docs-developer/CHANGELOG-formats.md b/docs-developer/CHANGELOG-formats.md index dcbcb8f484..ab14d10fd0 100644 --- a/docs-developer/CHANGELOG-formats.md +++ b/docs-developer/CHANGELOG-formats.md @@ -6,6 +6,10 @@ Note that this is not an exhaustive list. Processed profile format upgraders can ## Processed profile format +### Version 72 + +Marker schema fields can now include a `containsPII` array describing the categories of privacy-sensitive data they contain. Profile sanitization uses these categories instead of identifying privacy-sensitive fields from the marker type. + ### Version 71 The frame table (`profile.shared.frameTable`) representation changed in such a way that all its columns can now be typed arrays when using [JsonSlabs](https://github.com/mstange/json-slabs/) profiles. diff --git a/src/app-logic/constants.ts b/src/app-logic/constants.ts index e0f67bd2ff..034f51bdc5 100644 --- a/src/app-logic/constants.ts +++ b/src/app-logic/constants.ts @@ -12,7 +12,7 @@ export const GECKO_PROFILE_VERSION = 36; // The current version of the "processed" profile format. // Please don't forget to update the processed profile format changelog in // `docs-developer/CHANGELOG-formats.md`. -export const PROCESSED_PROFILE_VERSION = 71; +export const PROCESSED_PROFILE_VERSION = 72; // The following are the margin sizes for the left and right of the timeline. Independent // components need to share these values. diff --git a/src/profile-logic/marker-data.ts b/src/profile-logic/marker-data.ts index 5c196abd14..cd3c8ffb17 100644 --- a/src/profile-logic/marker-data.ts +++ b/src/profile-logic/marker-data.ts @@ -23,6 +23,7 @@ import { } from 'firefox-profiler/app-logic/constants'; import { getSchemaFromMarker, + isStringIndexFormat, isStringIndexMarkerField, markerPayloadMatchesSearch, markerSchemaFrontEndOnly, @@ -43,18 +44,18 @@ import type { IPCSharedData, IPCMarkerPayload, NetworkPayload, - PrefMarkerPayload, - TextMarkerPayload, StartEndRange, IndexedArray, DerivedMarkerInfo, MarkerSchema, + MarkerSchemaPIICategory, MarkerSchemaByName, MarkerDisplayLocation, Tid, LogMarkerPayload, ThreadIndex, Profile, + RemoveProfileInformation, } from 'firefox-profiler/types'; /** @@ -1456,132 +1457,153 @@ export function groupScreenshotsById( return idToScreenshotMarkers; } -export function removeNetworkMarkerURLs( - payload: NetworkPayload -): NetworkPayload { - return { ...payload, URI: '', RedirectURI: '' }; +function _removeExtensionId(markerName: string, text: string): string { + if (['ExtensionParent', 'ExtensionChild'].includes(markerName)) { + return text.replace(/^.*, (api_(call|event): )/, '$1'); + } + + if (markerName === 'Extension Suspend') { + return text.replace(/ by .*$/, ''); + } + + return text; } -export function removePrefMarkerPreferenceValues( - payload: PrefMarkerPayload -): PrefMarkerPayload { - return { ...payload, prefValue: '' }; +function _shouldSanitizePIICategory( + category: MarkerSchemaPIICategory, + PIIToBeRemoved: RemoveProfileInformation +): boolean { + switch (category) { + case 'url': + return PIIToBeRemoved.shouldRemoveUrls; + case 'extension-id': + return PIIToBeRemoved.shouldRemoveExtensions; + case 'preference-value': + return PIIToBeRemoved.shouldRemovePreferenceValues; + case 'private-browsing': + return PIIToBeRemoved.shouldRemovePrivateBrowsingData; + default: + assertExhaustiveCheck(category); + return false; + } } -/** - * Apply a transformation to a Text marker's text. The schema tells us whether the - * payload holds the text inline or as a string table index. In the latter case the - * result is interned as a new string, as other markers and frames may share that - * entry. - */ -function _updateTextMarkerText( - payload: TextMarkerPayload, - stringIndexMarkerFieldsByDataType: Map, +function _updateMarkerPayloadField( + markerPayload: MarkerPayload, + key: string, + isStringIndex: boolean, stringTable: StringTable, transform: (text: string) => string -): TextMarkerPayload { - // The casts below follow the storage layout the schema declares, which - // TypeScript can't verify from the payload type alone. - if ( - !isStringIndexMarkerField( - stringIndexMarkerFieldsByDataType, - payload.type, - 'name' - ) - ) { - return { ...payload, name: transform(payload.name as string) }; +): MarkerPayload { + const value = (markerPayload as any)[key]; + if (!isStringIndex) { + return { ...markerPayload, [key]: transform(value) } as any; } - const nameIndex = payload.name as IndexIntoStringTable; - if (!stringTable.hasIndex(nameIndex)) { - return payload; + const stringIndex = value as IndexIntoStringTable; + if (!stringTable.hasIndex(stringIndex)) { + return markerPayload; } - const text = stringTable.getString(nameIndex); + const text = stringTable.getString(stringIndex); const newText = transform(text); if (newText === text) { - return payload; - } - return { ...payload, name: stringTable.indexForString(newText) }; -} - -/** - * Sanitize Text marker's name property for potential URLs. Only for payloads - * holding their text inline, as the string table is sanitized as a whole. - */ -export function sanitizeTextMarker( - payload: TextMarkerPayload, - stringIndexMarkerFieldsByDataType: Map, - stringTable: StringTable -): TextMarkerPayload { - return _updateTextMarkerText( - payload, - stringIndexMarkerFieldsByDataType, - stringTable, - removeURLs - ); -} - -/** - * Sanitize Extension Text marker's name property for potential add-on ids. - */ -export function sanitizeExtensionTextMarker( - markerName: string, - payload: TextMarkerPayload, - stringIndexMarkerFieldsByDataType: Map, - stringTable: StringTable -): TextMarkerPayload { - if (['ExtensionParent', 'ExtensionChild'].includes(markerName)) { - return _updateTextMarkerText( - payload, - stringIndexMarkerFieldsByDataType, - stringTable, - (text) => text.replace(/^.*, (api_(call|event): )/, '$1') - ); + return markerPayload; } - - if (markerName === 'Extension Suspend') { - return _updateTextMarkerText( - payload, - stringIndexMarkerFieldsByDataType, - stringTable, - (text) => text.replace(/ by .*$/, '') - ); - } - - return payload; + return { + ...markerPayload, + [key]: stringTable.indexForString(newText), + } as any; } -export function sanitizeFromMarkerSchema( +/** Apply a marker schema's PII rules to its payload. */ +export function sanitizeMarkerFromSchema( markerSchema: MarkerSchema, - markerPayload: MarkerPayload -): MarkerPayload { - for (const { key, format } of markerSchema.fields) { - if (!(key in markerPayload)) { - continue; + markerName: string, + markerPayload: MarkerPayload, + stringTable: StringTable, + PIIToBeRemoved: RemoveProfileInformation +): { + markerPayload: MarkerPayload; + shouldRemoveMarker: boolean; +} { + let shouldRemoveMarker = false; + + for (const { key, format, containsPII = [] } of markerSchema.fields) { + const hasField = key in markerPayload; + + if (hasField) { + // The casts are needed because TypeScript cannot refine the payload union + // using a schema field that is only known at runtime. + if (PIIToBeRemoved.shouldRemoveUrls && format === 'url') { + markerPayload = { + ...markerPayload, + [key]: removeURLs((markerPayload as any)[key]), + } as any; + } else if (PIIToBeRemoved.shouldRemoveUrls && format === 'file-path') { + markerPayload = { + ...markerPayload, + [key]: removeFilePath((markerPayload as any)[key]), + } as any; + } else if ( + PIIToBeRemoved.shouldRemoveUrls && + format === 'sanitized-string' + ) { + markerPayload = { + ...markerPayload, + [key]: '', + } as any; + } } - // 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 as any)[key]), - } as any; - } else if (format === 'file-path') { - markerPayload = { - ...markerPayload, - [key]: removeFilePath((markerPayload as any)[key]), - } as any; - } else if (format === 'sanitized-string') { - markerPayload = { - ...markerPayload, - [key]: '', - } as any; + const isStringIndex = isStringIndexFormat(format); + for (const category of containsPII) { + if (!_shouldSanitizePIICategory(category, PIIToBeRemoved)) { + continue; + } + + switch (category) { + case 'url': + if (hasField) { + markerPayload = _updateMarkerPayloadField( + markerPayload, + key, + isStringIndex, + stringTable, + removeURLs + ); + } + break; + case 'extension-id': + if (hasField) { + markerPayload = _updateMarkerPayloadField( + markerPayload, + key, + isStringIndex, + stringTable, + (text) => _removeExtensionId(markerName, text) + ); + } + break; + case 'preference-value': + markerPayload = _updateMarkerPayloadField( + markerPayload, + key, + isStringIndex, + stringTable, + () => '' + ); + break; + case 'private-browsing': + shouldRemoveMarker ||= + hasField && Boolean((markerPayload as any)[key]); + break; + default: + assertExhaustiveCheck(category); + } } } - return markerPayload; + return { markerPayload, shouldRemoveMarker }; } /** diff --git a/src/profile-logic/marker-schema.ts b/src/profile-logic/marker-schema.ts index 90a81d4333..b5dd5ff467 100644 --- a/src/profile-logic/marker-schema.ts +++ b/src/profile-logic/marker-schema.ts @@ -21,6 +21,7 @@ import type { MarkerSchema, MarkerSchemaByName, MarkerSchemaField, + MarkerSchemaPIICategory, Marker, MarkerIndex, MarkerPayload, @@ -29,12 +30,61 @@ import type { } from 'firefox-profiler/types'; import type { StringTable } from '../utils/string-table'; +// Profiles recorded by Gecko versions without PII annotations use these defaults. +const markerSchemaPIICategoriesBySchemaName = new Map< + string, + Map +>([ + [ + 'Network', + new Map([ + ['URI', ['url']], + ['RedirectURI', ['url']], + ['isPrivateBrowsing', ['private-browsing']], + ]), + ], + ['Text', new Map([['name', ['url', 'extension-id']]])], + ['PreferenceRead', new Map([['prefValue', ['preference-value']]])], +]); + +export function addPIICategoriesToMarkerSchema( + markerSchema: MarkerSchema +): MarkerSchema { + const piiCategoriesByField = markerSchemaPIICategoriesBySchemaName.get( + markerSchema.name + ); + if (!piiCategoriesByField) { + return markerSchema; + } + + const fields = markerSchema.fields.map((field) => { + const containsPII = piiCategoriesByField.get(field.key); + return containsPII && !field.containsPII + ? { ...field, containsPII } + : field; + }); + const existingFieldKeys = new Set(fields.map(({ key }) => key)); + for (const [key, containsPII] of piiCategoriesByField) { + if (!existingFieldKeys.has(key)) { + fields.push({ key, format: 'string', hidden: true, containsPII }); + } + } + + return { ...markerSchema, fields }; +} + +export function addPIICategoriesToMarkerSchemas( + markerSchemas: MarkerSchema[] +): MarkerSchema[] { + return markerSchemas.map(addPIICategoriesToMarkerSchema); +} + /** * The marker schema comes from Gecko, and is embedded in the profile. However, * we may want to define schemas that are front-end only. This is the location * to do that. The schema will get merged in with the Gecko schema. */ -export const markerSchemaFrontEndOnly: MarkerSchema[] = [ +const markerSchemaFrontEndOnlyWithoutPII: MarkerSchema[] = [ { name: 'Jank', display: ['marker-table', 'marker-chart'], @@ -99,6 +149,10 @@ export const markerSchemaFrontEndOnly: MarkerSchema[] = [ }, ]; +export const markerSchemaFrontEndOnly = addPIICategoriesToMarkerSchemas( + markerSchemaFrontEndOnlyWithoutPII +); + /** * This function takes the intended marker schema for a marker field, and applies * the appropriate formatting function. diff --git a/src/profile-logic/process-profile.ts b/src/profile-logic/process-profile.ts index 8be48440cb..0b2b620ad3 100644 --- a/src/profile-logic/process-profile.ts +++ b/src/profile-logic/process-profile.ts @@ -58,7 +58,10 @@ import { toFloat64Array, toFloat64ArraySetNullToZero, } from '../utils/typed-arrays'; -import { computeStringIndexMarkerFieldsByDataType } from '../profile-logic/marker-schema'; +import { + addPIICategoriesToMarkerSchema, + computeStringIndexMarkerFieldsByDataType, +} from '../profile-logic/marker-schema'; import { convertJsTracerToThread } from '../profile-logic/js-tracer'; import type { StringTable } from '../utils/string-table'; @@ -1722,7 +1725,7 @@ function _convertGeckoMarkerSchema( description = staticFields[staticDescriptionFieldIndex].value; } - return { + return addPIICategoriesToMarkerSchema({ name, tooltipLabel, tableLabel, @@ -1733,7 +1736,7 @@ function _convertGeckoMarkerSchema( graphs, colorField, isStackBased, - }; + }); } /** diff --git a/src/profile-logic/processed-profile-versioning.ts b/src/profile-logic/processed-profile-versioning.ts index c009bd052d..9e1693257e 100644 --- a/src/profile-logic/processed-profile-versioning.ts +++ b/src/profile-logic/processed-profile-versioning.ts @@ -3433,6 +3433,48 @@ const _upgraders: { frameTable.address = new Uint32Array(frameTable.address); } }, + [72]: (profile: any) => { + const piiCategoriesBySchemaName = new Map>([ + [ + 'Network', + new Map([ + ['URI', ['url']], + ['RedirectURI', ['url']], + ['isPrivateBrowsing', ['private-browsing']], + ]), + ], + ['Text', new Map([['name', ['url', 'extension-id']]])], + ['PreferenceRead', new Map([['prefValue', ['preference-value']]])], + ]); + + for (const schema of profile.meta.markerSchema) { + const piiCategoriesByField = piiCategoriesBySchemaName.get(schema.name); + if (!piiCategoriesByField) { + continue; + } + + for (const field of schema.fields) { + const containsPII = piiCategoriesByField.get(field.key); + if (containsPII && !field.containsPII) { + field.containsPII = containsPII; + } + } + + const existingFieldKeys = new Set( + schema.fields.map((field: any) => field.key) + ); + for (const [key, containsPII] of piiCategoriesByField) { + if (!existingFieldKeys.has(key)) { + schema.fields.push({ + key, + format: 'string', + hidden: true, + containsPII, + }); + } + } + } + }, // If you add a new upgrader here, please document the change in // `docs-developer/CHANGELOG-formats.md`. }; diff --git a/src/profile-logic/sanitize.ts b/src/profile-logic/sanitize.ts index b296b647b5..8024e7ae9c 100644 --- a/src/profile-logic/sanitize.ts +++ b/src/profile-logic/sanitize.ts @@ -11,18 +11,10 @@ import { computeCompactedProfile } from './profile-compacting'; import { StringTable } from '../utils/string-table'; import { removeURLs } from '../utils/string'; import { - removeNetworkMarkerURLs, - removePrefMarkerPreferenceValues, filterRawMarkerTableToRangeWithMarkersToDelete, - sanitizeExtensionTextMarker, - sanitizeTextMarker, - sanitizeFromMarkerSchema, + sanitizeMarkerFromSchema, } from './marker-data'; -import { - computeStringIndexMarkerFieldsByDataType, - getSchemaFromMarker, - isStringIndexMarkerField, -} from './marker-schema'; +import { getSchemaFromMarker } from './marker-schema'; import { filterRawThreadSamplesToRange, filterCounterSamplesToRange, @@ -146,11 +138,6 @@ export function sanitizePII( stringArray, }; - // Precompute the payload fields that hold string table indexes, so that the - // marker loop below doesn't have to walk the schema fields for every marker. - const stringIndexMarkerFieldsByDataType = - computeStringIndexMarkerFieldsByDataType(Object.values(markerSchemaByName)); - let stackFlags: Uint8Array | null = null; if (windowIdFromPrivateBrowsing.size > 0) { @@ -338,7 +325,6 @@ export function sanitizePII( PIIToBeRemoved, windowIdFromPrivateBrowsing, markerSchemaByName, - stringIndexMarkerFieldsByDataType, stackFlags ); @@ -454,7 +440,6 @@ function sanitizeThreadPII( PIIToBeRemoved: RemoveProfileInformation, windowIdFromPrivateBrowsing: Set, markerSchemaByName: MarkerSchemaByName, - stringIndexMarkerFieldsByDataType: Map, stackFlags: Uint8Array | null ): RawThread | null { if (PIIToBeRemoved.shouldRemoveThreads.has(threadIndex)) { @@ -489,77 +474,27 @@ function sanitizeThreadPII( for (let i = 0; i < markerTable.length; i++) { let currentMarker = markerTable.data[i]; - // Remove the all the preference values, if the user wants that. - if ( - PIIToBeRemoved.shouldRemovePreferenceValues && - currentMarker && - currentMarker.type === 'PreferenceRead' - ) { - // Remove the preference value field from the marker payload. - markerTable.data[i] = removePrefMarkerPreferenceValues(currentMarker); - } - - if (currentMarker && PIIToBeRemoved.shouldRemoveUrls) { - // Use the schema to find some properties that need to be sanitized. + if (currentMarker) { const markerSchema = getSchemaFromMarker( markerSchemaByName, currentMarker ); if (markerSchema) { - currentMarker = markerTable.data[i] = sanitizeFromMarkerSchema( + const markerName = stringTable.getString(markerTable.name[i]); + const sanitizedMarker = sanitizeMarkerFromSchema( markerSchema, - currentMarker - ); - } - - // 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); - - // Strip the URL from the marker name - const requestStr = stringTable.getString(markerTable.name[i]); - const sanitizedRequestStr = requestStr.replace(/:.*/, ''); - markerTable.name[i] = stringTable.indexForString(sanitizedRequestStr); - } - - if ( - currentMarker.type === 'Text' && - !isStringIndexMarkerField( - stringIndexMarkerFieldsByDataType, - 'Text', - 'name' - ) - ) { - // Sanitize all the name fields of text markers in case they contain URLs. - // Newer profiles hold the text in the string table, sanitized above. - markerTable.data[i] = sanitizeTextMarker( + markerName, currentMarker, - stringIndexMarkerFieldsByDataType, - stringTable + stringTable, + PIIToBeRemoved ); - // Re-assign the value of currentMarker as the marker may be - // sanitized again to remove extension ids. - currentMarker = markerTable.data[i]; + currentMarker = markerTable.data[i] = sanitizedMarker.markerPayload; + if (sanitizedMarker.shouldRemoveMarker) { + markersToDelete.add(i); + } } } - if ( - PIIToBeRemoved.shouldRemoveExtensions && - currentMarker && - currentMarker.type === 'Text' - ) { - const markerName = stringTable.getString(markerTable.name[i]); - // Sanitize extension ids out of known extension markers. Unlike URLs, - // these aren't removed from the string table as a whole. - markerTable.data[i] = sanitizeExtensionTextMarker( - markerName, - currentMarker, - stringIndexMarkerFieldsByDataType, - stringTable - ); - } - // Remove the screenshots if the current thread index is in the // threadsWithScreenshots array if ( @@ -571,15 +506,6 @@ function sanitizeThreadPII( } if (PIIToBeRemoved.shouldRemovePrivateBrowsingData) { - if ( - currentMarker && - currentMarker.type === 'Network' && - currentMarker.isPrivateBrowsing - ) { - // Remove network requests coming from private browsing sessions - markersToDelete.add(i); - } - if ( currentMarker && 'innerWindowID' in currentMarker && diff --git a/src/test/fixtures/profiles/marker-schema.ts b/src/test/fixtures/profiles/marker-schema.ts index 00dc94843b..9556a5384f 100644 --- a/src/test/fixtures/profiles/marker-schema.ts +++ b/src/test/fixtures/profiles/marker-schema.ts @@ -2,8 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import type { MarkerSchema } from 'firefox-profiler/types'; +import { addPIICategoriesToMarkerSchemas } from 'firefox-profiler/profile-logic/marker-schema'; -export const markerSchemaForTests: MarkerSchema[] = [ +const markerSchemaForTestsWithoutPII: MarkerSchema[] = [ { name: 'GCMajor', display: ['marker-chart', 'marker-table', 'timeline-memory'], @@ -193,3 +194,7 @@ export const markerSchemaForTests: MarkerSchema[] = [ ], }, ]; + +export const markerSchemaForTests = addPIICategoriesToMarkerSchemas( + markerSchemaForTestsWithoutPII +); diff --git a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap index 27af272e13..cfec2157bc 100644 --- a/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap +++ b/src/test/integration/profiler-edit/__snapshots__/profiler-edit.test.ts.snap @@ -87,7 +87,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -1488,7 +1488,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -2889,7 +2889,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "a.out", "sampleUnits": Object { @@ -4290,7 +4290,7 @@ Object { "markerSchema": Array [], "oscpu": "macOS 14.6.1", "pausedRanges": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "a.out", "sampleUnits": Object { diff --git a/src/test/store/__snapshots__/profile-view.test.ts.snap b/src/test/store/__snapshots__/profile-view.test.ts.snap index 92fd2e39d8..5f559363ef 100644 --- a/src/test/store/__snapshots__/profile-view.test.ts.snap +++ b/src/test/store/__snapshots__/profile-view.test.ts.snap @@ -217,6 +217,9 @@ Object { "label": "Type", }, Object { + "containsPII": Array [ + "preference-value", + ], "format": "string", "key": "prefValue", "label": "Value", @@ -255,6 +258,10 @@ Object { ], "fields": Array [ Object { + "containsPII": Array [ + "url", + "extension-id", + ], "format": "string", "key": "name", "label": "Details", @@ -428,7 +435,7 @@ Object { "oscpu": "", "physicalCPUs": 0, "platform": "", - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "Firefox", "sourceURL": "", diff --git a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap index 46f5bf9b8e..3bffbb2edf 100644 --- a/src/test/unit/__snapshots__/profile-conversion.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-conversion.test.ts.snap @@ -42,7 +42,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -1022,7 +1022,7 @@ Object { "RefreshDriverTick", "Network", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "ART Trace (Android)", "symbolicated": true, "version": 36, @@ -2305,7 +2305,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -2697,7 +2697,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3086,7 +3086,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3187,7 +3187,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3540,7 +3540,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3605,7 +3605,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3759,7 +3759,7 @@ Object { "markerSchemaNames": Array [ "EventDispatch", ], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Chrome Trace", "symbolicated": true, "version": 36, @@ -3817,7 +3817,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4207,7 +4207,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4265,7 +4265,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4323,7 +4323,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Firefox", "symbolicated": true, "version": 36, @@ -4643,7 +4643,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5019,7 +5019,7 @@ Object { "importedFrom": "Simpleperf", "interval": 0, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "com.example.sampleapplication", "symbolicated": undefined, "version": 30, @@ -5319,7 +5319,7 @@ Object { "importedFrom": "dhat", "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "target/debug/examples/work_log (dhat)", "symbolicated": true, "version": 36, @@ -5452,7 +5452,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Flamegraph", "symbolicated": true, "version": 36, @@ -5510,7 +5510,7 @@ Object { "importedFrom": undefined, "interval": 1, "markerSchemaNames": Array [], - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "product": "Flamegraph", "symbolicated": true, "version": 36, diff --git a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap index 4d039c9117..2689100221 100644 --- a/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap +++ b/src/test/unit/__snapshots__/profile-upgrading.test.ts.snap @@ -40,7 +40,7 @@ Object { "oscpu": undefined, "physicalCPUs": undefined, "platform": undefined, - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "Firefox", "sampleUnits": undefined, @@ -7631,6 +7631,9 @@ Object { "label": "Type", }, Object { + "containsPII": Array [ + "preference-value", + ], "format": "string", "key": "prefValue", "label": "Value", @@ -7664,6 +7667,10 @@ Object { ], "fields": Array [ Object { + "containsPII": Array [ + "url", + "extension-id", + ], "format": "string", "key": "name", "label": "Details", @@ -7812,7 +7819,32 @@ Object { "marker-table", "timeline-network", ], - "fields": Array [], + "fields": Array [ + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "URI", + }, + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "RedirectURI", + }, + Object { + "containsPII": Array [ + "private-browsing", + ], + "format": "string", + "hidden": true, + "key": "isPrivateBrowsing", + }, + ], "name": "Network", }, Object { @@ -7834,7 +7866,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -9026,6 +9058,9 @@ Object { "label": "Type", }, Object { + "containsPII": Array [ + "preference-value", + ], "format": "string", "key": "prefValue", "label": "Value", @@ -9059,6 +9094,10 @@ Object { ], "fields": Array [ Object { + "containsPII": Array [ + "url", + "extension-id", + ], "format": "string", "key": "name", "label": "Details", @@ -9207,7 +9246,32 @@ Object { "marker-table", "timeline-network", ], - "fields": Array [], + "fields": Array [ + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "URI", + }, + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "RedirectURI", + }, + Object { + "containsPII": Array [ + "private-browsing", + ], + "format": "string", + "hidden": true, + "key": "isPrivateBrowsing", + }, + ], "name": "Network", }, Object { @@ -9229,7 +9293,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "Firefox", "stackwalk": 1, @@ -10591,6 +10655,9 @@ Object { "label": "Type", }, Object { + "containsPII": Array [ + "preference-value", + ], "format": "string", "key": "prefValue", "label": "Value", @@ -10624,6 +10691,10 @@ Object { ], "fields": Array [ Object { + "containsPII": Array [ + "url", + "extension-id", + ], "format": "string", "key": "name", "label": "Details", @@ -10772,7 +10843,32 @@ Object { "marker-table", "timeline-network", ], - "fields": Array [], + "fields": Array [ + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "URI", + }, + Object { + "containsPII": Array [ + "url", + ], + "format": "string", + "hidden": true, + "key": "RedirectURI", + }, + Object { + "containsPII": Array [ + "private-browsing", + ], + "format": "string", + "hidden": true, + "key": "isPrivateBrowsing", + }, + ], "name": "Network", }, Object { @@ -10794,7 +10890,7 @@ Object { "misc": "rv:48.0", "oscpu": "Intel Mac OS X 10.11", "platform": "Macintosh", - "preprocessedProfileVersion": 71, + "preprocessedProfileVersion": 72, "processType": 0, "product": "Firefox", "stackwalk": 1, diff --git a/src/test/unit/process-profile.test.ts b/src/test/unit/process-profile.test.ts index 6bd72e94d1..1f40874059 100644 --- a/src/test/unit/process-profile.test.ts +++ b/src/test/unit/process-profile.test.ts @@ -1075,6 +1075,62 @@ describe('source table processing', function () { }); describe('Marker schema conversion', function () { + it('should add PII categories to marker schema fields', function () { + const geckoProfile = createGeckoProfile(); + geckoProfile.meta.markerSchema.push( + { + name: 'Network', + display: [], + data: [], + }, + { + name: 'Text', + display: [], + data: [{ key: 'name', format: 'unique-string' }], + }, + { + name: 'PreferenceRead', + display: [], + data: [{ key: 'prefValue', format: 'string' }], + } + ); + + const processedProfile = processGeckoProfile(geckoProfile); + const schemasByName = Object.fromEntries( + processedProfile.meta.markerSchema.map((schema) => [schema.name, schema]) + ); + + expect(schemasByName.Network.fields).toEqual([ + { key: 'URI', format: 'string', hidden: true, containsPII: ['url'] }, + { + key: 'RedirectURI', + format: 'string', + hidden: true, + containsPII: ['url'], + }, + { + key: 'isPrivateBrowsing', + format: 'string', + hidden: true, + containsPII: ['private-browsing'], + }, + ]); + expect(schemasByName.Text.fields).toEqual([ + { + key: 'name', + format: 'unique-string', + containsPII: ['url', 'extension-id'], + }, + ]); + expect(schemasByName.PreferenceRead.fields).toEqual([ + { + key: 'prefValue', + format: 'string', + containsPII: ['preference-value'], + }, + ]); + }); + it('should preserve optional marker schema properties', function () { const geckoProfile = createGeckoProfile(); diff --git a/src/test/unit/profile-upgrading.test.ts b/src/test/unit/profile-upgrading.test.ts index 50a7d6ceaa..776329b63c 100644 --- a/src/test/unit/profile-upgrading.test.ts +++ b/src/test/unit/profile-upgrading.test.ts @@ -7,6 +7,7 @@ import { serializeProfileToJsonString, } from '../../profile-logic/process-profile'; import { upgradeGeckoProfileToCurrentVersion } from '../../profile-logic/gecko-profile-versioning'; +import { attemptToUpgradeProcessedProfileThroughMutation } from '../../profile-logic/processed-profile-versioning'; import { GECKO_PROFILE_VERSION, PROCESSED_PROFILE_VERSION, @@ -134,6 +135,74 @@ describe('upgrading processed profiles', function () { require('../fixtures/upgrades/processed-3.json') ); }); + + it('adds PII categories to marker schema fields', function () { + const profile: any = { + meta: { + preprocessedProfileVersion: 71, + markerSchema: [ + { name: 'Network', fields: [] }, + { + name: 'Text', + fields: [{ key: 'name', format: 'unique-string' }], + }, + { + name: 'PreferenceRead', + fields: [{ key: 'prefValue', format: 'string' }], + }, + ], + }, + threads: [], + }; + + attemptToUpgradeProcessedProfileThroughMutation(profile, {}); + + expect(profile.meta.markerSchema).toEqual([ + { + name: 'Network', + fields: [ + { + key: 'URI', + format: 'string', + hidden: true, + containsPII: ['url'], + }, + { + key: 'RedirectURI', + format: 'string', + hidden: true, + containsPII: ['url'], + }, + { + key: 'isPrivateBrowsing', + format: 'string', + hidden: true, + containsPII: ['private-browsing'], + }, + ], + }, + { + name: 'Text', + fields: [ + { + key: 'name', + format: 'unique-string', + containsPII: ['url', 'extension-id'], + }, + ], + }, + { + name: 'PreferenceRead', + fields: [ + { + key: 'prefValue', + format: 'string', + containsPII: ['preference-value'], + }, + ], + }, + ]); + }); }); describe('importing perf profile', function () { diff --git a/src/test/unit/sanitize.test.ts b/src/test/unit/sanitize.test.ts index 9af63ef369..eb47aab689 100644 --- a/src/test/unit/sanitize.test.ts +++ b/src/test/unit/sanitize.test.ts @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import { processGeckoProfile } from '../../profile-logic/process-profile'; +import { attemptToUpgradeProcessedProfileThroughMutation } from '../../profile-logic/processed-profile-versioning'; import { sanitizePII } from '../../profile-logic/sanitize'; import { createGeckoProfile } from '../fixtures/profiles/gecko-profile'; import { @@ -15,9 +16,12 @@ import { } from '../fixtures/profiles/processed-profile'; import { ensureExists } from '../../utils/types'; import { + computeCombinedMarkerSchemaList, + computeMarkerSchemaByName, correlateIPCMarkers, deriveMarkersFromRawMarkerTable, } from '../../profile-logic/marker-data'; +import { addPIICategoriesToMarkerSchema } from '../../profile-logic/marker-schema'; import { getTimeRangeForThread, computeTimeColumnForRawSamplesTable, @@ -35,11 +39,27 @@ import { StringTable } from 'firefox-profiler/utils/string-table'; import { FrameFlag } from 'firefox-profiler/types'; import type { MarkerSchemaByName, + Profile, RawThread, RemoveProfileInformation, } from 'firefox-profiler/types'; describe('sanitizePII', function () { + function upgradeProfileWithoutPIIAnnotations(profile: Profile): Profile { + profile.meta.preprocessedProfileVersion = 71; + profile.meta.markerSchema = profile.meta.markerSchema.map((schema) => ({ + ...schema, + fields: schema.fields.map((field) => { + const fieldWithoutPII = { ...field }; + delete fieldWithoutPII.containsPII; + return fieldWithoutPII; + }), + })); + return ensureExists( + attemptToUpgradeProcessedProfileThroughMutation(profile, {}) + ); + } + function setup( piiConfig: Partial, originalProfile = processGeckoProfile(createGeckoProfile()), @@ -82,7 +102,7 @@ describe('sanitizePII', function () { } ); - const markerSchemaByName: MarkerSchemaByName = { + const additionalMarkerSchemas: MarkerSchemaByName = { FileIO: { name: 'FileIO', display: ['marker-chart', 'marker-table', 'timeline-fileio'], @@ -141,6 +161,12 @@ describe('sanitizePII', function () { }, ...extraMarkerSchemas, }; + const markerSchemaByName = computeMarkerSchemaByName( + computeCombinedMarkerSchemaList([ + ...originalProfile.meta.markerSchema, + ...Object.values(additionalMarkerSchemas), + ]) + ); // Mirror what the `getTracedValuesBuffer` selector hands to `sanitizePII` // in the app, instead of pretending that no thread has a buffer. @@ -167,12 +193,12 @@ describe('sanitizePII', function () { // Mirrors what Firefox emits now: the schema declares `name` as a unique // string, so the payload holds a string table index. const uniqueStringTextSchema: MarkerSchemaByName = { - Text: { + Text: addPIICategoriesToMarkerSchema({ name: 'Text', tableLabel: '{marker.name} — {marker.data.name}', display: ['marker-chart', 'marker-table'], fields: [{ key: 'name', label: 'Details', format: 'unique-string' }], - }, + }), }; function setupWithUniqueStringTextMarkers( @@ -627,56 +653,61 @@ describe('sanitizePII', function () { }); it('should sanitize all the URLs inside network markers', function () { - const { sanitizedProfile } = setup({ - shouldRemoveUrls: true, - }); + const originalProfile = getProfileWithMarkers( + getNetworkMarkers({ + uri: 'https://example.com', + payload: { RedirectURI: 'https://redirect.example.com' }, + }) + ); + const originalMarkerData = originalProfile.threads[0].markers.data; + const { sanitizedProfile } = setup( + { shouldRemoveUrls: true }, + originalProfile + ); - const stringArray = sanitizedProfile.shared.stringArray; - for (const thread of sanitizedProfile.threads) { - for (let i = 0; i < thread.markers.length; i++) { - const currentMarker = thread.markers.data[i]; - if ( - currentMarker && - currentMarker.type && - currentMarker.type === 'Network' - ) { - /* eslint-disable jest/no-conditional-expect */ - expect(currentMarker.URI).toBeFalsy(); - expect(currentMarker.RedirectURI).toBeFalsy(); - const stringIndex = thread.markers.name[i]; - expect(stringArray[stringIndex].includes('http')).toBe(false); - /* eslint-enable */ - } - } - } + const markers = sanitizedProfile.threads[0].markers; + expect(markers.data[0]).toEqual({ + ...originalMarkerData[0], + URI: 'https://', + }); + expect(markers.data[1]).toEqual({ + ...originalMarkerData[1], + URI: 'https://', + RedirectURI: 'https://', + }); + expect( + markers.name.map((name) => sanitizedProfile.shared.stringArray[name]) + ).toEqual(['Load 0: https://', 'Load 0: https://']); }); - it('should sanitize the URLs inside text markers', function () { + it('should sanitize URLs inside text markers after upgrading an old processed profile', function () { const unsanitizedNameField = 'onBeforeRequest https://profiler.firefox.com/ by extension'; const sanitizedNameField = 'onBeforeRequest https:// by extension'; + const originalProfile = getProfileWithMarkers([ + [ + 'Extension Suspend', + 0, + 1, + { + type: 'Text', + name: unsanitizedNameField, + }, + ], + ]); + upgradeProfileWithoutPIIAnnotations(originalProfile); const { sanitizedProfile } = setup( { shouldRemoveUrls: true, }, - getProfileWithMarkers([ - [ - 'Extension Suspend', - 0, - 1, - { - type: 'Text', - name: unsanitizedNameField, - }, - ], - ]) + originalProfile ); const marker = sanitizedProfile.threads[0].markers.data[0]; if (!marker || marker.type !== 'Text') { throw new Error('Expected a Text marker'); } - expect(marker.name).toBe(sanitizedNameField); + expect(marker).toEqual({ type: 'Text', name: sanitizedNameField }); }); it('should sanitize all the URLs inside string table', function () { @@ -749,7 +780,7 @@ describe('sanitizePII', function () { if (!marker || marker.type !== 'Text') { throw new Error('Expected a Text marker'); } - expect(marker.name).toBe(sanitizedNameField); + expect(marker).toEqual({ type: 'Text', name: sanitizedNameField }); } }); @@ -779,7 +810,7 @@ describe('sanitizePII', function () { if (!marker || marker.type !== 'Text') { throw new Error('Expected a Text marker'); } - expect(marker.name).toBe(sanitizedNameField); + expect(marker).toEqual({ type: 'Text', name: sanitizedNameField }); }); it('should not sanitize all the preference values inside preference read markers', function () { @@ -810,7 +841,7 @@ describe('sanitizePII', function () { expect(thread.markers.length).toEqual(1); const marker = thread.markers.data[0]; - // All the conditions have to be checked to make Flow happy. + // All the conditions have to be checked to satisfy the type checker. expect( marker && marker.type && @@ -819,26 +850,25 @@ describe('sanitizePII', function () { ).toBeTruthy(); }); - it('should sanitize all the preference values inside preference read markers', function () { + it('should sanitize preference values after upgrading an old processed profile', function () { + const preferenceMarker = { + type: 'PreferenceRead' as const, + prefAccessTime: 0, + prefName: 'preferenceName', + prefKind: 'preferenceKind', + prefType: 'preferenceType', + prefValue: 'preferenceValue', + }; + const originalProfile = getProfileWithMarkers([ + ['PreferenceRead', 0, 1, preferenceMarker], + ]); + upgradeProfileWithoutPIIAnnotations(originalProfile); const { sanitizedProfile } = setup( { shouldRemovePreferenceValues: true, + shouldRemoveUrls: true, }, - getProfileWithMarkers([ - [ - 'PreferenceRead', - 0, - 1, - { - type: 'PreferenceRead', - prefAccessTime: 0, - prefName: 'preferenceName', - prefKind: 'preferenceKind', - prefType: 'preferenceType', - prefValue: 'preferenceValue', - }, - ], - ]) + originalProfile ); expect(sanitizedProfile.threads.length).toEqual(1); @@ -846,14 +876,10 @@ describe('sanitizePII', function () { const thread = sanitizedProfile.threads[0]; expect(thread.markers.length).toEqual(1); - const marker = thread.markers.data[0]; - // All the conditions have to be checked to make Flow happy. - expect( - marker && - marker.type && - marker.type === 'PreferenceRead' && - marker.prefValue === '' - ).toBeTruthy(); + expect(thread.markers.data[0]).toEqual({ + ...preferenceMarker, + prefValue: '', + }); }); it('should not push any null values to marker values by mistake while filtering', function () { diff --git a/src/types/markers.ts b/src/types/markers.ts index 3dfcc65f10..9f71c92517 100644 --- a/src/types/markers.ts +++ b/src/types/markers.ts @@ -128,6 +128,12 @@ export type MarkerGraph = { color?: GraphColor; }; +export type MarkerSchemaPIICategory = + | 'url' + | 'extension-id' + | 'preference-value' + | 'private-browsing'; + export type MarkerSchemaField = { // The property key of the marker data property that carries the field value. key: string; @@ -144,6 +150,8 @@ export type MarkerSchemaField = { // of fields in the tooltip or in the sidebar. Such fields can still be // used inside labels and their values are matched when searching. hidden?: boolean; + + containsPII?: MarkerSchemaPIICategory[]; }; export type MarkerSchema = {