-
Notifications
You must be signed in to change notification settings - Fork 247
Redacting urls before sending to telemetry data. #2549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces URL field redaction before sending telemetry data to prevent the exposure of sensitive information. Key changes include the addition and export of a new function (fieldRedaction) to sanitize URLs, updates to configuration and tests to control and validate the redaction behavior, and integration of redaction in multiple telemetry modules.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
shared/AppInsightsCore/src/applicationinsights-core-js.ts | Exporting the new redaction function from EnvUtils. |
shared/AppInsightsCore/src/JavaScriptSDK/EnvUtils.ts | Introducing constants and the fieldRedaction function to sanitize URLs. |
shared/AppInsightsCore/src/JavaScriptSDK.Interfaces/IConfiguration.ts | Adding a new redactionEnabled configuration property. |
shared/AppInsightsCore/Tests/Unit/src/ApplicationInsightsCore.Tests.ts | Adding various test cases to validate the redaction function. |
shared/AppInsightsCommon/src/Telemetry/Common/DataSanitizer.ts | Integrating fieldRedaction in the URL sanitization logic. |
shared/AppInsightsCommon/Tests/Unit/src/AppInsightsCommon.tests.ts | Adding tests to verify dataSanitizeUrl behavior with redaction. |
shared/1ds-core-js/test/Unit/src/FileSizeCheckTest.ts | Adjusting file size limits possibly related to size changes from redaction. |
shared/1ds-core-js/src/Index.ts | Exporting fieldRedaction for use in other modules. |
extensions/applicationinsights-dependencies-js/src/ajax.ts | Redacting URLs in AJAX calls when configured. |
extensions/applicationinsights-clickanalytics-js/src/DataCollector.ts | Applying fieldRedaction in URL sanitization for click analytics. |
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/Telemetry/PageViewManager.ts | Updating page view tracking to include URL redaction. |
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/AnalyticsPlugin.ts | Incorporating fieldRedaction across telemetry events to ensure sensitive data is redacted. |
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/AnalyticsPlugin.ts
Outdated
Show resolved
Hide resolved
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/AnalyticsPlugin.ts
Outdated
Show resolved
Hide resolved
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/AnalyticsPlugin.ts
Outdated
Show resolved
Hide resolved
777ec80
to
0e7a8e5
Compare
Please note, this is not the complete set of changes, I am still working on adding fieldRedaction in places where the url needs to be redacted. |
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/AnalyticsPlugin.ts
Outdated
Show resolved
Hide resolved
extensions/applicationinsights-analytics-js/src/JavaScriptSDK/Telemetry/PageViewManager.ts
Show resolved
Hide resolved
extensions/applicationinsights-clickanalytics-js/src/DataCollector.ts
Outdated
Show resolved
Hide resolved
@@ -35,6 +36,12 @@ function addDependencyInitializerOnClick() { | |||
// Change properties of telemetry event "before" it's been processed | |||
details.item.name = "dependency-name"; | |||
details.item.properties.url = details.item?.target; | |||
if (isString(details.item.properties.url)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add this code in example apps we have here, we should be redacting the URL internally so this is more confusing than helpful
@@ -141,7 +143,9 @@ export function sanitizeUrl(config: IClickAnalyticsConfiguration, location: Loca | |||
if (!!config.urlCollectQuery) { // false by default | |||
url += (isValueAssigned(location.search)? location.search : ""); | |||
} | |||
|
|||
if (rootConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking config everywhere you call fieldRedaction, add a config validation inside that method if is not already there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the config validation is inside the fieldRedaction method. I will remove this extra check.
@@ -126,6 +126,9 @@ export class PageAction extends WebEvent { | |||
|
|||
pageActionEvent.timeToAction = _getTimeToClick(); | |||
pageActionEvent.refUri = isValueAssigned(overrideValues.refUri) ? overrideValues.refUri : _self._config.coreData.referrerUri; | |||
if (_self._clickAnalyticsPlugin.core.config) { | |||
pageActionEvent.refUri = fieldRedaction(pageActionEvent.refUri, _self._clickAnalyticsPlugin.core.config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to redact URLs that are overridden and not automatically tracked?, in this case customer is explicitly adding this value right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a good point! Does the spec mention about customer defined URLs? or should we add configs to allow disabling auto-redact urls?
return input; | ||
} | ||
if (input.indexOf(" ") !== -1) { | ||
return input; // Checking for URLs with spaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the redaction is cancelled if there is a space?
// Check if any parameters need redaction | ||
let anyParamRedacted = false; | ||
if (config && config.redactQueryParams) { | ||
sensitiveParams = [...DEFAULT_SENSITIVE_PARAMS, ...config.redactQueryParams]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit use concat
instead of ...
for minification purpose
b62bd9d
to
65a77aa
Compare
No description provided.