Merge pull request #14878 from Microsoft/fix-callstacks

Fixes #12694 regex replace strips filepath when approot is PIIPath
microsoft · Nov 7, 2016 · 8812422 · 8812422
2 parents 6fc75bf + e81d70a
commit 8812422
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 3 deletions.
diff --git a/src/vs/platform/telemetry/common/telemetryService.ts b/src/vs/platform/telemetry/common/telemetryService.ts
@@ -54,15 +54,16 @@ export class TelemetryService implements ITelemetryService {
 		// #1 `file:///DANGEROUS/PATH/resources/app/Useful/Information`
 		// #2 // Any other file path that doesn't match the approved form above should be cleaned.
 		// #3 "Error: ENOENT; no such file or directory" is often followed with PII, clean it
-		for (let piiPath of this._piiPaths) {
-			this._cleanupPatterns.push([new RegExp(escapeRegExpCharacters(piiPath), 'gi'), '']);
-		}
 		this._cleanupPatterns.push(
 			[/file:\/\/\/.*?\/resources\/app\//gi, ''],
 			[/file:\/\/\/.*/gi, ''],
 			[/ENOENT: no such file or directory.*?\'([^\']+)\'/gi, 'ENOENT: no such file or directory']
 		);
 
+		for (let piiPath of this._piiPaths) {
+			this._cleanupPatterns.push([new RegExp(escapeRegExpCharacters(piiPath), 'gi'), '']);
+		}
+
 		if (this._configurationService) {
 			this._updateUserOptIn();
 			this._configurationService.onDidUpdateConfiguration(this._updateUserOptIn, this, this._disposables);

diff --git a/src/vs/platform/telemetry/test/node/telemetryService.test.ts b/src/vs/platform/telemetry/test/node/telemetryService.test.ts
@@ -433,6 +433,68 @@ suite('TelemetryService', () => {
 		service.dispose();
 	}));
 
+	test('Unexpected Error Telemetry removes PII but preserves Code file path when PIIPath is configured', sinon.test(function () {
+
+		let origErrorHandler = Errors.errorHandler.getUnexpectedErrorHandler();
+		Errors.setUnexpectedErrorHandler(() => { });
+
+		try {
+			let settings = new ErrorTestingSettings();
+			let testAppender = new TestTelemetryAppender();
+			let service = new TelemetryService({ appender: testAppender, piiPaths: [ settings.personalInfo + '/resources/app/' ] }, undefined);
+			const errorTelemetry = new ErrorTelemetry(service);
+
+			let dangerousPathWithImportantInfoError: any = new Error(settings.dangerousPathWithImportantInfo);
+			dangerousPathWithImportantInfoError.stack = settings.stack;
+
+			// Test that important information remains but personal info does not
+			Errors.onUnexpectedError(dangerousPathWithImportantInfoError);
+			this.clock.tick(ErrorTelemetry.ERROR_FLUSH_TIMEOUT);
+
+			assert.notEqual(testAppender.events[0].data.message.indexOf(settings.importantInfo), -1);
+			assert.equal(testAppender.events[0].data.message.indexOf(settings.personalInfo), -1);
+			assert.equal(testAppender.events[0].data.message.indexOf(settings.filePrefix), -1);
+			assert.notEqual(testAppender.events[0].data.stack.indexOf(settings.importantInfo), -1);
+			assert.equal(testAppender.events[0].data.stack.indexOf(settings.personalInfo), -1);
+			assert.equal(testAppender.events[0].data.stack.indexOf(settings.filePrefix), -1);
+			assert.notEqual(testAppender.events[0].data.stack.indexOf(settings.stack[4]), -1);
+			assert.equal(testAppender.events[0].data.stack.split('\n').length, settings.stack.length);
+
+			errorTelemetry.dispose();
+			service.dispose();
+		}
+		finally {
+			Errors.setUnexpectedErrorHandler(origErrorHandler);
+		}
+	}));
+
+	test('Uncaught Error Telemetry removes PII but preserves Code file path when PIIPath is configured', sinon.test(function () {
+		let errorStub = this.stub(window, 'onerror');
+		let settings = new ErrorTestingSettings();
+		let testAppender = new TestTelemetryAppender();
+		let service = new TelemetryService({ appender: testAppender, piiPaths: [ settings.personalInfo + '/resources/app/' ] }, undefined);
+		const errorTelemetry = new ErrorTelemetry(service);
+
+		let dangerousPathWithImportantInfoError: any = new Error('dangerousPathWithImportantInfo');
+		dangerousPathWithImportantInfoError.stack = settings.stack;
+		(<any>window.onerror)(settings.dangerousPathWithImportantInfo, 'test.js', 2, 42, dangerousPathWithImportantInfoError);
+		this.clock.tick(ErrorTelemetry.ERROR_FLUSH_TIMEOUT);
+
+		assert.equal(errorStub.callCount, 1);
+		// Test that important information remains but personal info does not
+		assert.notEqual(testAppender.events[0].data.message.indexOf(settings.importantInfo), -1);
+		assert.equal(testAppender.events[0].data.message.indexOf(settings.personalInfo), -1);
+		assert.equal(testAppender.events[0].data.message.indexOf(settings.filePrefix), -1);
+		assert.notEqual(testAppender.events[0].data.stack.indexOf(settings.importantInfo), -1);
+		assert.equal(testAppender.events[0].data.stack.indexOf(settings.personalInfo), -1);
+		assert.equal(testAppender.events[0].data.stack.indexOf(settings.filePrefix), -1);
+		assert.notEqual(testAppender.events[0].data.stack.indexOf(settings.stack[4]), -1);
+		assert.equal(testAppender.events[0].data.stack.split('\n').length, settings.stack.length);
+
+		errorTelemetry.dispose();
+		service.dispose();
+	}));
+
 	test('Unexpected Error Telemetry removes PII but preserves Missing Model error message', sinon.test(function () {
 
 		let origErrorHandler = Errors.errorHandler.getUnexpectedErrorHandler();