SCAL-338938: allowlist runtimeFilters and runtimeParameters before they reach Mixpanel - #677
sastaachar wants to merge 2 commits into
Conversation
… Mixpanel
SCAL-338938
TsEmbed's constructor spread the entire viewConfig into the
visual-sdk-embed-create Mixpanel event, so runtimeFilters and
runtimeParameters went up verbatim on every embed construction --
columnName (a customer worksheet column), values (the operands being
filtered on), and a parameter's name and value.
Both arrays are now replaced by an allowlisted summary before upload:
runtimeFilters -> { count, operators }
runtimeParameters -> { count, applicabilityLevels }
operators and applicabilityLevels are checked for membership in
RuntimeFilterOp / ApplicabilityLevel before being kept, so an arbitrary
string sitting under an enum-valued key is dropped rather than passed
through. A non-array value under either key is dropped entirely. Every
other viewConfig property is untouched.
The helper is internal and is not exported from index.ts, so there is no
public API change.
Verified by reverting only the call site: the two privacy assertions fail
without the fix.
There was a problem hiding this comment.
Code Review
This pull request introduces telemetry filtering to prevent sensitive customer data (such as runtime filters and parameters) from being uploaded to Mixpanel. It adds utility functions to replace these arrays with safe, allowlisted summaries (counts, operators, and applicability levels) and integrates them into the embed creation event. Comprehensive unit tests are also added to verify that no sensitive data is leaked. The review feedback highlights style guide violations in the JSDoc comments, specifically the use of deprecated terminology ('worksheet' instead of 'Model') and British English spelling ('Summarises' instead of 'Summarizes').
|
|
||
| /** | ||
| * `runtimeFilters` and `runtimeParameters` carry customer data: the column and | ||
| * parameter names come from the customer's worksheet, and the operands are the |
There was a problem hiding this comment.
According to the repository style guide (Rule 13: Deprecated Terminology Detection and Flagging), the term worksheet is deprecated. Please replace it with Model or LogicalModel.
| * parameter names come from the customer's worksheet, and the operands are the | |
| * parameter names come from the customer's Model, and the operands are the |
References
- Deprecated terms in documentation must be flagged for the developer to update to the current equivalent. Worksheet should be replaced with Model or LogicalModel. (link)
| ): string[] => Array.from(new Set(values.filter((v) => isEnumMember(enumObject, v)))) as string[]; | ||
|
|
||
| /** | ||
| * Summarises the runtime filters on a view config for telemetry. |
There was a problem hiding this comment.
According to the repository style guide (Rule 9: Language and Locale), all written work must be in American English (en-US) format. Please use Summarizes instead of Summarises.
| * Summarises the runtime filters on a view config for telemetry. | |
| * Summarizes the runtime filters on a view config for telemetry. |
References
- All written work must be in American English (en-US) format. Use American English spelling (e.g., 'summarizes' instead of 'summarises'). (link)
| ), | ||
| }); | ||
|
|
||
| /** |
There was a problem hiding this comment.
According to the repository style guide (Rule 9: Language and Locale), all written work must be in American English (en-US) format. Please use Summarizes instead of Summarises.
* Summarizes the runtime parameters on a view config for telemetry.References
- All written work must be in American English (en-US) format. Use American English spelling (e.g., 'summarizes' instead of 'summarises'). (link)
commit: |
The problem
TsEmbed's constructor spreads the entireviewConfiginto a Mixpanel event on every embed construction —src/embed/ts-embed.ts:254onmain:runtimeFiltersandruntimeParametersride along in that spread, and both carry customer data:RuntimeFilter.columnName— a column name from the customer's worksheetRuntimeFilter.values— the actual operands being filtered onRuntimeParameter.name/RuntimeParameter.value— likewiseCaptured from a real
LiveboardEmbedconstruction, with this PR's fix reverted:{"liveboardId":"lb-guid", "runtimeFilters":[{"columnName":"Patient SSN","operator":"EQ","values":["123-45-6789"]}], "runtimeParameters":[{"name":"Sales Region","value":"EMEA"}], "embedComponentType":"LiveboardEmbed"}Every value in there reaches Mixpanel today.
This is the only telemetry path on
maincarrying runtime filters or parameters: it is the sole...viewConfigspread intouploadMixpanelEvent(the other spreads insrc/are functional), andvisual-sdk-trigger-<HostEvent>uploads no properties at all onmain.The fix
Both arrays are replaced by an allowlisted summary before upload. Every other
viewConfigproperty is untouched.runtimeFilters{ count, operators }runtimeParameters{ count, applicabilityLevels }So the payload above becomes:
{"liveboardId":"lb-guid", "runtimeFilters":{"count":1,"operators":["EQ"]}, "runtimeParameters":{"count":1,"applicabilityLevels":[]}, "embedComponentType":"LiveboardEmbed"}Two things worth calling out in the implementation:
operatorsandapplicabilityLevelsare membership-checked, not key-allowlisted. A value is kept only if it is actually a member ofRuntimeFilterOp/ApplicabilityLevel. A bare key allowlist would pass an arbitrary string sitting underoperatorstraight through to Mixpanel — which is the exact leak this PR exists to close.This follows the privacy rule already agreed on SCAL-333657: store the shape, not the value, except enum members — and only once membership is verified.
Scope
The helper lives in
src/utils/runtimeTelemetry.tsand is not exported fromindex.ts, so there is no public API change and no@versionannotation is needed. No behaviour changes outside the one telemetry call.src/embed/ts-embed.tssrc/utils/runtimeTelemetry.tssrc/utils/runtimeTelemetry.spec.tssrc/embed/ts-embed.spec.tsVerification
ts-embed.tscall site reverted, the two privacy assertions fail —Expected substring: not "Patient SSN"— and the third still passes. The tests have real diagnostic power.ts-embed.spec.ts's'should trigger Navigate only after UpdateEmbedParams has settled'is order-dependent and fails intermittently with a 5003 ms timeout. It is pre-existing and unrelated to this PR — I reverted to pristineorigin/mainsources in the same worktree and reproduced the identical failure there, and it passes in 480 ms in isolation either way.tsc --noEmitcleants-embed.tsare pre-existingcomment-lengthon lines this PR doesn't touch)check-size31.97 kB against the 34 kB budgetOne test-hygiene note: the new spec in
ts-embed.spec.tscreates its Mixpanel spy inbeforeEachrather than in the describe body, because an earlier suite in that file callsjest.restoreAllMocks()and would otherwise unwire it.Deliberately out of scope
The same spread also uploads
searchQueryandsearchOptions.searchTokenString(raw customer search strings) and content GUIDs (liveboardId,vizId,answerId). I've left those alone and flagged them on the ticket rather than widening this PR — whether search text and GUIDs are acceptable in Mixpanel is a product call, not a mechanical one.Background
Found and empirically confirmed during SCAL-333657 (a browser harness observed it live against a recording Mixpanel stub), deliberately left out of scope there, and never filed until now.
Jira: https://thoughtspot.atlassian.net/browse/SCAL-338938