[Security solution] Remove extra data from tracking clicks

[stephmilovic]

Removed a few html properties from the properties tree of analytics tracking click events. These also contained "extra data". Here is an example of elements with a data-provider-id before and after these changes:

Before

After

[elasticmachine]

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[afharo]

The changes to track_clicks LGTM (with a minor nit regarding the naming of the var).
The name of the PR refers to that change only.

Regarding the URL Fix, I have a few thoughts:

1. I'd rather change it in the appropriate context provider rather than the client. This would be more performant because it runs once per URL change instead of for each event. On top of that, there's no tempering of the same data in 2 places, which can be hard to follow and troubleshoot in the future.
2. I wonder if we really need this masking:
   • The URL is automatically reported by the browser in the Referer header: this means that any potential PII is still present in any HTTP request made by the browser. Not only to the Telemetry Web API, but also in requests to the Kibana server, any potential CDN (once we decide to serve assets from CDNs), Security Solution's newsfeed, and any other external requests the browser may make.
   • FullStory collects it. I wonder if it makes sense to tackle this effort in our custom telemetry client if our "other telemetry channel" collects it without a problem.

To be clear, I think it'll be best if the page_url context provider only reported the path without the query strings because it'll make it easier on the analysis end. But not for PII reasons.

WDYT?

[afharo]

🧡

nit: The name HTML_ATTRIBUTES_WITH_EXTRA_DATA doesn't really mean the extra data is necessarily bad or skippable. I wonder if we should use a name that specifies that we want to skip them. If we want to avoid using PII, we could probably use the name HTML_ATTRIBUTES_TO_REMOVE or *_TO_SKIP, or something along those lines?
Naming is hard ™️

[afharo]

I'd prefer to fix it on the source rather than the client.

page_url comes from

kibana/packages/core/application/core-application-browser-internal/src/utils/get_location_observable.ts

Line 34 in 97dc2ec

return `${location.pathname}${location.hash}`;

[stephmilovic]

This only solves for page_url, but we need to mask for the other properties (page, pageName) too. was hoping to find a more central definition but not sure one exists?

[afharo]

page and pageName are populated here:

kibana/packages/core/execution-context/core-execution-context-browser-internal/src/execution_context_service.ts

Lines 124 to 129 in 4c626f1

	map(({ type, name, page, id }) => ({
	pageName: `${compact([type, name, page]).join(':')}`,
	applicationId: name ?? type ?? 'unknown',
	page,
	entityId: id,
	}))

TBH, bearing in mind the source of that information (execution context), it shouldn't expose any PII. page is provided by the app/plugin. After double-checking our telemetry data, we can confirm these usages for the field:

• Some plugins provide human-readable indicators of subsections of the app ("app", "listing", "editor", "console")
  • Special mention to management that shows the name of each section in the Stack Management view "ingestPipelines", "watcher", "indexManagement", etc.
  • visualize also shows nested values to the editor value to indicate the type of viz that's being used ("editor:data_table", "editor:tsvb", "editor:vertical_bar", ...)
• Others like Security Solution, show the URL path. I can see versions prior to 8.8.0 show generic URLs like /rules/id/:detailName/:tabName(rule_exceptions). After 8.8.0, they show the actual values. I wonder if this PR introduced the regression: [Security solutions] Fix execution context page #152816

FWIW, the execution context already carries the pathname (intentionally not used in telemetry, only for logging purposes to make troubleshooting easier). So I wonder if we should fix the page reported by Security Solutions instead of "polluting" the EBT client with this filter.

To be clear: I'm not opposed to running PII filters in the EBT client. FWIW, I think we should traverse the entire object to ensure we don't leak any PII anywhere (things like the one we are looking at may occur) #132259. However, for some reason, patching these fields here instead of fixing the source of the leak seems wrong. I think it's mostly because we are implementing the filter in a reactive way: if a new URL is added nodes/name/:piiSensitiveName, we'll have to remember to add it to the list, or we'll leak those values in the versions released until we fix them.

At the same time, the Referrer header (refer to my previous comment) keeps coming to mind as a reminder of "is it really worth the effort of hiding those here?".

Fixing the source of page (and hence pageName) would improve analytics, as we'll be able to see all events in page /rules/id/:detailName/:tabName(alerts), rather than /rules/id/387817b0-2fb3-11ee-9a91-19aeacd9e5a8/alerts. We could refer to page_url to identify if they spend most of the time looking at the same rule/host or if the cardinality is high.

WDYT?

[afharo]

Could it happen that the _g query param doesn't come the first one? As in https://kibanaHost/my/path?someQuery=value&_g={...}?

Also, would it be safer to remove all the querystrings in the URL? If not, I think it'd be best if we could parse them and maintain a blocklist like the HTML attributes.

WDYT?

[stephmilovic]

sure i will change it to all querystrings

[afharo]

Is it possible that these properties have that info? AFAIK, they come from the executionContext service:

kibana/packages/core/execution-context/core-execution-context-browser-internal/src/execution_context_service.ts

Lines 125 to 128 in 4c626f1

	pageName: `${compact([type, name, page]).join(':')}`,
	applicationId: name ?? type ?? 'unknown',
	page,
	entityId: id,

[stephmilovic]

Yes, for example on the host page:

page: "/hosts/name/Stephs-MBP/authentications"
pageName: "application:securitySolutionUI:/hosts/name/Stephs-MBP/authentications"
page_url: "/app/security/hosts/name/Stephs-MBP/authentications"

This is why I fixed it in the client, fixing it in get_location_observable will only capture the page_url field

[afharo]

Can we generalize the name of the variable so other solutions can add their paths to the list whenever they notice them?

[afharo]

nit: using split + regexp looks kind of odd to me... I wonder if we should replace the regexp with another split by / and drop the first element (AFAIK, that's essentially what the regexp is doing).

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 7eeba80

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
core	365.3KB	365.4KB	+145.0B

History

• 💛 Build #152160 was flaky c88a555

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[afharo]

LGTM! Thank you for iterating! 🧡

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation