From b69dd2d35fdb7232e4b18309a2812abd3646ea8c Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Wed, 4 Dec 2024 13:33:42 -0800 Subject: [PATCH 1/8] Scrub PII from codedError, add unit test --- .../telemetry/src/e2etest/telemetry.test.ts | 34 +++++++++++++++++++ .../telemetry/src/telemetry.ts | 10 ++++++ 2 files changed, 44 insertions(+) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 00df0c63a14..7375a79819f 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -694,3 +694,37 @@ test.each(testTelemetryOptions)( }); }, ); + +test.each(testTelemetryOptions)( + 'Telemetry run test command end to end with CodedError, verifies PII is scrubbed if present in CodedError.', + async options => { + await TelemetryTest.startTest(options); + + const expectedError = new errorUtils.CodedError( + 'MSBuildError', // type + 'test error', // message + { + fieldWithPath: 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the path with "[path]". + fieldWithNoPath: 'Test Error data', // expectation: no changes to this string. + fieldWithNoString: 14, // expectation: no changes to this value. + } // data + ); + + // AI eats errors thrown in telemetry processors + const caughtErrors: Error[] = []; + TelemetryTest.addTelemetryInitializer( + verifyTestCommandTelemetryProcessor( + caughtErrors, + expectedError.type, + expectedError, + ), + ); + + await runTestCommandE2E(() => testCommandBody(expectedError)); + + TelemetryTest.endTest(() => { + // Check if any errors were thrown + expect(caughtErrors).toHaveLength(0); + }); + }, +); \ No newline at end of file diff --git a/packages/@react-native-windows/telemetry/src/telemetry.ts b/packages/@react-native-windows/telemetry/src/telemetry.ts index c14ca9b4793..b15364d678f 100644 --- a/packages/@react-native-windows/telemetry/src/telemetry.ts +++ b/packages/@react-native-windows/telemetry/src/telemetry.ts @@ -442,6 +442,16 @@ export class Telemetry { data: codedError?.data ?? {}, }; + // Scrub any potential PII present in codedError.data array, as long as the data is a string. + if (Object.keys(codedErrorStruct.data).length > 0) { + for (const field in codedErrorStruct.data) { + if (codedErrorStruct.data.hasOwnProperty(field) && typeof codedErrorStruct.data[field] === "string") { + const sanitizedError = errorUtils.sanitizeErrorMessage(codedErrorStruct.data[field]); + codedErrorStruct.data[field] = sanitizedError; + } + } + } + // Copy msBuildErrorMessages into the codedError.data object if ((error as any).msBuildErrorMessages) { // Always grab MSBuild error codes if possible From 250dce86d9e688c762b349cc76f3b7e1d3d453e3 Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Wed, 4 Dec 2024 15:36:00 -0800 Subject: [PATCH 2/8] Nit: comment updates --- .../telemetry/src/e2etest/telemetry.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 7375a79819f..59e20a1b17d 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -391,6 +391,7 @@ function verifyTestCommandTelemetryProcessor( : 'Unknown', ); + // NOTE: At this point, expectedError has been modified by trackException(). expect(codedError.data).toStrictEqual( (expectedError as errorUtils.CodedError).data ?? {}, ); @@ -704,13 +705,12 @@ test.each(testTelemetryOptions)( 'MSBuildError', // type 'test error', // message { - fieldWithPath: 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the path with "[path]". + fieldWithPath: 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". fieldWithNoPath: 'Test Error data', // expectation: no changes to this string. fieldWithNoString: 14, // expectation: no changes to this value. } // data ); - // AI eats errors thrown in telemetry processors const caughtErrors: Error[] = []; TelemetryTest.addTelemetryInitializer( verifyTestCommandTelemetryProcessor( From 0a20a724496135eae2da91e4ea8f69a5e9d5c36e Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Wed, 4 Dec 2024 15:40:39 -0800 Subject: [PATCH 3/8] Lint fixes --- .../telemetry/src/e2etest/telemetry.test.ts | 7 ++++--- .../@react-native-windows/telemetry/src/telemetry.ts | 9 +++++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 59e20a1b17d..74d9fb9c15b 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -705,10 +705,11 @@ test.each(testTelemetryOptions)( 'MSBuildError', // type 'test error', // message { - fieldWithPath: 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". + fieldWithPath: + 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". fieldWithNoPath: 'Test Error data', // expectation: no changes to this string. fieldWithNoString: 14, // expectation: no changes to this value. - } // data + }, // data ); const caughtErrors: Error[] = []; @@ -727,4 +728,4 @@ test.each(testTelemetryOptions)( expect(caughtErrors).toHaveLength(0); }); }, -); \ No newline at end of file +); diff --git a/packages/@react-native-windows/telemetry/src/telemetry.ts b/packages/@react-native-windows/telemetry/src/telemetry.ts index b15364d678f..9f14e53ef6d 100644 --- a/packages/@react-native-windows/telemetry/src/telemetry.ts +++ b/packages/@react-native-windows/telemetry/src/telemetry.ts @@ -445,8 +445,13 @@ export class Telemetry { // Scrub any potential PII present in codedError.data array, as long as the data is a string. if (Object.keys(codedErrorStruct.data).length > 0) { for (const field in codedErrorStruct.data) { - if (codedErrorStruct.data.hasOwnProperty(field) && typeof codedErrorStruct.data[field] === "string") { - const sanitizedError = errorUtils.sanitizeErrorMessage(codedErrorStruct.data[field]); + if ( + codedErrorStruct.data.hasOwnProperty(field) && + typeof codedErrorStruct.data[field] === 'string' + ) { + const sanitizedError = errorUtils.sanitizeErrorMessage( + codedErrorStruct.data[field], + ); codedErrorStruct.data[field] = sanitizedError; } } From a4bc2d59ddab7dc18b1bf4c1b97c5f7e3677edef Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Wed, 4 Dec 2024 15:41:23 -0800 Subject: [PATCH 4/8] Change files --- ...ows-telemetry-d7dc63f1-c900-4229-bcb4-8d7cd683293c.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@react-native-windows-telemetry-d7dc63f1-c900-4229-bcb4-8d7cd683293c.json diff --git a/change/@react-native-windows-telemetry-d7dc63f1-c900-4229-bcb4-8d7cd683293c.json b/change/@react-native-windows-telemetry-d7dc63f1-c900-4229-bcb4-8d7cd683293c.json new file mode 100644 index 00000000000..4505d14de6a --- /dev/null +++ b/change/@react-native-windows-telemetry-d7dc63f1-c900-4229-bcb4-8d7cd683293c.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Expand sanitization checks for error telemetry instances", + "packageName": "@react-native-windows/telemetry", + "email": "14967941+danielayala94@users.noreply.github.com", + "dependentChangeType": "patch" +} From 3d45bf8158a69ca925d74431ce4071e44075e1c8 Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:15:56 -0800 Subject: [PATCH 5/8] Enhanced bug fix using recursion, added test field cases --- .../telemetry/src/e2etest/telemetry.test.ts | 17 +++++++ .../telemetry/src/telemetry.ts | 44 ++++++++++++------- 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 74d9fb9c15b..0113e1797dc 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -709,6 +709,23 @@ test.each(testTelemetryOptions)( 'Test Error occurred at C:\\some\\file\\path\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". fieldWithNoPath: 'Test Error data', // expectation: no changes to this string. fieldWithNoString: 14, // expectation: no changes to this value. + arrayField: [ + 'No path', + 15, + 'Clean this path: C:\\some\\file\\path2\\project.build.appxrecipe', + ], + nestedField: { + fieldWithPath: + 'Test Error occurred at C:\\some\\file\\path3\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". + fieldWithNoPath: 'Test Error data 2', // expectation: no changes to this string. + fieldWithNoString: 16, // expectation: no changes to this value. + anotherNestedField: { + fieldWithPath: + 'Test Error occurred at C:\\some\\file\\path4\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". + fieldWithNoPath: 'Test Error data 3', // expectation: no changes to this string. + fieldWithNoString: 17, // expectation: no changes to this value. + }, + }, }, // data ); diff --git a/packages/@react-native-windows/telemetry/src/telemetry.ts b/packages/@react-native-windows/telemetry/src/telemetry.ts index 9f14e53ef6d..37be4db4d4c 100644 --- a/packages/@react-native-windows/telemetry/src/telemetry.ts +++ b/packages/@react-native-windows/telemetry/src/telemetry.ts @@ -442,21 +442,6 @@ export class Telemetry { data: codedError?.data ?? {}, }; - // Scrub any potential PII present in codedError.data array, as long as the data is a string. - if (Object.keys(codedErrorStruct.data).length > 0) { - for (const field in codedErrorStruct.data) { - if ( - codedErrorStruct.data.hasOwnProperty(field) && - typeof codedErrorStruct.data[field] === 'string' - ) { - const sanitizedError = errorUtils.sanitizeErrorMessage( - codedErrorStruct.data[field], - ); - codedErrorStruct.data[field] = sanitizedError; - } - } - } - // Copy msBuildErrorMessages into the codedError.data object if ((error as any).msBuildErrorMessages) { // Always grab MSBuild error codes if possible @@ -482,6 +467,9 @@ export class Telemetry { } } + // Scrub any potential PII present in codedError.data array, as long as the data is a string. + Telemetry.traverseCodedErrorStruct(codedErrorStruct.data); + // Break down TS Error object into Exception Data const exceptionData = Telemetry.convertErrorIntoExceptionData(error); @@ -546,4 +534,30 @@ export class Telemetry { return exceptionData; } + + static traverseCodedErrorStruct( + data: Record, + path: string[] = [], + ) { + for (const key in data) { + if (Object.prototype.hasOwnProperty.call(data, key)) { + const value = data[key]; + const currentPath = [...path, key]; + + if (Array.isArray(value)) { + // Replace the array by reading all elements in it, + // then check if they are strings. If that's the case, sanitize. + data[key] = value.map(item => + typeof item === 'string' + ? errorUtils.sanitizeErrorMessage(item) + : item, + ); + } else if (typeof value === 'object' && value !== null) { + Telemetry.traverseCodedErrorStruct(value, currentPath); + } else if (typeof value === 'string') { + data[key] = errorUtils.sanitizeErrorMessage(value); + } + } + } + } } From f502d7b29dda955be8cde9a760c5ece17c33e870 Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Wed, 18 Dec 2024 11:28:19 -0800 Subject: [PATCH 6/8] Lint fix --- .../telemetry/src/e2etest/telemetry.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 958e4d6225d..77dcb2eb7cb 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -791,4 +791,4 @@ test.each(testTelemetryOptions)( expect(caughtErrors).toHaveLength(0); }); }, -); \ No newline at end of file +); From 8ef51a8b115f4d0a868a1ddb004a9f6bccc59bd8 Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Fri, 20 Dec 2024 15:49:35 -0800 Subject: [PATCH 7/8] Sanitize any, and test fixes --- .../telemetry/src/e2etest/telemetry.test.ts | 40 ++++++++++++++++--- .../telemetry/src/telemetry.ts | 40 +++++++++---------- 2 files changed, 53 insertions(+), 27 deletions(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 77dcb2eb7cb..6c370818ae9 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -403,7 +403,6 @@ function verifyTestCommandTelemetryProcessor( : 'Unknown', ); - // NOTE: At this point, expectedError has been modified by trackException(). // If the exception type is not CodedError but any data got copied into envelope.CodedError.data, // for instance autolinking error info, build the expected CodedError.data. let expectedCodedErrorData = {}; @@ -747,7 +746,7 @@ test.each(testTelemetryOptions)( async options => { await TelemetryTest.startTest(options); - const expectedError = new errorUtils.CodedError( + const codedErrorInfo = new errorUtils.CodedError( 'MSBuildError', // type 'test error', // message { @@ -759,13 +758,14 @@ test.each(testTelemetryOptions)( 'No path', 15, 'Clean this path: C:\\some\\file\\path2\\project.build.appxrecipe', + ['No path', 150, 'Also clean this: C:\\some\\file\\path2\\project.build.appxrecipe'] ], - nestedField: { + someObject: { fieldWithPath: 'Test Error occurred at C:\\some\\file\\path3\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". fieldWithNoPath: 'Test Error data 2', // expectation: no changes to this string. fieldWithNoString: 16, // expectation: no changes to this value. - anotherNestedField: { + nestedObject: { fieldWithPath: 'Test Error occurred at C:\\some\\file\\path4\\project.build.appxrecipe', // expectation: replace the whole C:\\... thing with "[path]". fieldWithNoPath: 'Test Error data 3', // expectation: no changes to this string. @@ -775,6 +775,36 @@ test.each(testTelemetryOptions)( }, // data ); + const expectedError = new errorUtils.CodedError( + 'MSBuildError', // type + 'test error', // message + { + fieldWithPath: + 'Test Error occurred at [path]', + fieldWithNoPath: 'Test Error data', + fieldWithNoString: 14, + arrayField: [ + 'No path', + 15, + 'Clean this path: [path]', + ['No path', 150, 'Also clean this: [path]'] + ], + someObject: { + fieldWithPath: + 'Test Error occurred at [path]', + fieldWithNoPath: 'Test Error data 2', + fieldWithNoString: 16, + nestedObject: { + fieldWithPath: + 'Test Error occurred at [path]', + fieldWithNoPath: 'Test Error data 3', + fieldWithNoString: 17, + }, + }, + }, // data + ); + + const caughtErrors: Error[] = []; TelemetryTest.addTelemetryInitializer( verifyTestCommandTelemetryProcessor( @@ -784,7 +814,7 @@ test.each(testTelemetryOptions)( ), ); - await runTestCommandE2E(() => testCommandBody(expectedError)); + await runTestCommandE2E(() => testCommandBody(codedErrorInfo)); TelemetryTest.endTest(() => { // Check if any errors were thrown diff --git a/packages/@react-native-windows/telemetry/src/telemetry.ts b/packages/@react-native-windows/telemetry/src/telemetry.ts index 032826a45bd..00303817482 100644 --- a/packages/@react-native-windows/telemetry/src/telemetry.ts +++ b/packages/@react-native-windows/telemetry/src/telemetry.ts @@ -468,7 +468,7 @@ export class Telemetry { } // Scrub any potential PII present in codedError.data array, as long as the data is a string. - Telemetry.traverseCodedErrorStruct(codedErrorStruct.data); + codedErrorStruct.data = Telemetry.sanitizeAny(codedErrorStruct.data); // Break down TS Error object into Exception Data const exceptionData = Telemetry.convertErrorIntoExceptionData(error); @@ -535,29 +535,25 @@ export class Telemetry { return exceptionData; } - static traverseCodedErrorStruct( - data: Record, - path: string[] = [], - ) { - for (const key in data) { - if (Object.prototype.hasOwnProperty.call(data, key)) { - const value = data[key]; - const currentPath = [...path, key]; - - if (Array.isArray(value)) { - // Replace the array by reading all elements in it, - // then check if they are strings. If that's the case, sanitize. - data[key] = value.map(item => - typeof item === 'string' - ? errorUtils.sanitizeErrorMessage(item) - : item, - ); - } else if (typeof value === 'object' && value !== null) { - Telemetry.traverseCodedErrorStruct(value, currentPath); - } else if (typeof value === 'string') { - data[key] = errorUtils.sanitizeErrorMessage(value); + static sanitizeAny(data: any): any { + if (Array.isArray(data)) { + // This is an array, sanitize each element recursively. + return data.map(item => Telemetry.sanitizeAny(item)); + } else if (typeof data === 'object' && data !== null) { + // This is an object, sanitize each field recursively. + const sanitizedObject: Record = {}; + for (const key in data) { + if (Object.prototype.hasOwnProperty.call(data, key)) { + sanitizedObject[key] = Telemetry.sanitizeAny(data[key]); } } + return sanitizedObject; + } else if (typeof data === 'string') { + // The base case: this is a string, sanitize it. + return errorUtils.sanitizeErrorMessage(data); } + + // Not a string, return the data unchanged. + return data; } } From 107da9d28fe126c86bd0ba91063ca866b867bd49 Mon Sep 17 00:00:00 2001 From: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Date: Mon, 6 Jan 2025 11:22:01 -0800 Subject: [PATCH 8/8] Lint fix, again --- .../telemetry/src/e2etest/telemetry.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts index 6c370818ae9..ab238b6b14a 100644 --- a/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts +++ b/packages/@react-native-windows/telemetry/src/e2etest/telemetry.test.ts @@ -758,7 +758,11 @@ test.each(testTelemetryOptions)( 'No path', 15, 'Clean this path: C:\\some\\file\\path2\\project.build.appxrecipe', - ['No path', 150, 'Also clean this: C:\\some\\file\\path2\\project.build.appxrecipe'] + [ + 'No path', + 150, + 'Also clean this: C:\\some\\file\\path2\\project.build.appxrecipe', + ], ], someObject: { fieldWithPath: @@ -779,24 +783,21 @@ test.each(testTelemetryOptions)( 'MSBuildError', // type 'test error', // message { - fieldWithPath: - 'Test Error occurred at [path]', + fieldWithPath: 'Test Error occurred at [path]', fieldWithNoPath: 'Test Error data', fieldWithNoString: 14, arrayField: [ 'No path', 15, 'Clean this path: [path]', - ['No path', 150, 'Also clean this: [path]'] + ['No path', 150, 'Also clean this: [path]'], ], someObject: { - fieldWithPath: - 'Test Error occurred at [path]', + fieldWithPath: 'Test Error occurred at [path]', fieldWithNoPath: 'Test Error data 2', fieldWithNoString: 16, nestedObject: { - fieldWithPath: - 'Test Error occurred at [path]', + fieldWithPath: 'Test Error occurred at [path]', fieldWithNoPath: 'Test Error data 3', fieldWithNoString: 17, }, @@ -804,7 +805,6 @@ test.each(testTelemetryOptions)( }, // data ); - const caughtErrors: Error[] = []; TelemetryTest.addTelemetryInitializer( verifyTestCommandTelemetryProcessor(