chore: refactor sensitive data capture

.changeset/young-clocks-call.md

@@ -0,0 +1,5 @@
+---
+'posthog-js': patch
+---
+
+Fix autocapture incorrectly blocking capture from elements with IDs/names containing sensitive looking substrings (e.g., `<a id="password">`). Sensitive data protection is maintained through element type checks and value pattern matching, as well as `ph-no-capture` element attributes.

packages/browser/src/__tests__/autocapture-utils.test.ts

@@ -1,11 +1,8 @@
 /// <reference lib="dom" />
 
-import sinon from 'sinon'
-
 import {
     getSafeText,
-    shouldCaptureDomEvent,
-    shouldCaptureElement,
+    shouldAutocaptureEvent,
     isSensitiveElement,
     shouldCaptureValue,
     isAngularStyleAttr,
@@ -14,6 +11,8 @@ import {
     getElementsChainString,
     getClassNames,
     makeSafeText,
+    isExplicitNoCapture,
+    isExplicitCapture,
 } from '../autocapture-utils'
 import { document } from '../utils/globals'
 import { makeMouseEvent } from './autocapture.test'
@@ -121,6 +120,30 @@ describe(`Autocapture utility functions`, () => {
             el.innerHTML = `Mixed "double" and 'single' quotes`
             expect(getSafeText(el)).toBe(`Mixed "double" and 'single' quotes`)
         })
+
+        it(`should collect text from sensitive elements with ph-include class`, () => {
+            const input = document!.createElement(`input`)
+            input.className = `ph-include`
+            input.appendChild(document.createTextNode(`Hello world`))
+            document!.body.appendChild(input)
+            expect(getSafeText(input)).toBe(`Hello world`)
+        })
+
+        it(`shouldn't collect text from elements with ph-no-capture class`, () => {
+            const el = document!.createElement(`div`)
+            el.className = `ph-no-capture`
+            el.innerHTML = `Hello world`
+            document!.body.appendChild(el)
+            expect(getSafeText(el)).toBe(``)
+        })
+
+        it(`shouldn't collect text from elements with ph-sensitive class`, () => {
+            const el = document!.createElement(`div`)
+            el.className = `ph-sensitive`
+            el.innerHTML = `Hello world`
+            document!.body.appendChild(el)
+            expect(getSafeText(el)).toBe(``)
+        })
     })
 
     describe(`makeSafeText`, () => {
@@ -175,22 +198,22 @@ describe(`Autocapture utility functions`, () => {
     describe(`shouldCaptureDomEvent`, () => {
         it(`should capture "submit" events on <form> elements`, () => {
             expect(
-                shouldCaptureDomEvent(document!.createElement(`form`), {
+                shouldAutocaptureEvent(document!.createElement(`form`), {
                     type: `submit`,
                 } as unknown as Event)
             ).toBe(true)
         })
 
         it.each([`input`, `SELECT`, `textarea`])(`should capture "change" events on <%s> elements`, (tagName) => {
             expect(
-                shouldCaptureDomEvent(document!.createElement(tagName), {
+                shouldAutocaptureEvent(document!.createElement(tagName), {
                     type: `change`,
                 } as unknown as Event)
             ).toBe(true)
         })
 
         it.each([`A`, `a`])(`should capture "click" events on <%s> elements`, (tagName) => {
-            expect(shouldCaptureDomEvent(document!.createElement(tagName), makeMouseEvent({}))).toBe(true)
+            expect(shouldAutocaptureEvent(document!.createElement(tagName), makeMouseEvent({}))).toBe(true)
         })
 
         it(`should capture "click" events on <button> elements`, () => {
@@ -200,22 +223,22 @@ describe(`Autocapture utility functions`, () => {
             const button3 = document!.createElement(`input`)
             button3.setAttribute(`type`, `submit`)
             ;[button1, button2, button3].forEach((button) => {
-                expect(shouldCaptureDomEvent(button, makeMouseEvent({}))).toBe(true)
+                expect(shouldAutocaptureEvent(button, makeMouseEvent({}))).toBe(true)
             })
         })
 
         it(`should protect against bad inputs`, () => {
-            expect(shouldCaptureDomEvent(null as unknown as Element, makeMouseEvent({}))).toBe(false)
-            expect(shouldCaptureDomEvent(undefined as unknown as Element, makeMouseEvent({}))).toBe(false)
-            expect(shouldCaptureDomEvent(`div` as unknown as Element, makeMouseEvent({}))).toBe(false)
+            expect(shouldAutocaptureEvent(null as unknown as Element, makeMouseEvent({}))).toBe(false)
+            expect(shouldAutocaptureEvent(undefined as unknown as Element, makeMouseEvent({}))).toBe(false)
+            expect(shouldAutocaptureEvent(`div` as unknown as Element, makeMouseEvent({}))).toBe(false)
         })
 
         it(`should NOT capture "click" events on <form> elements`, () => {
-            expect(shouldCaptureDomEvent(document!.createElement(`form`), makeMouseEvent({}))).toBe(false)
+            expect(shouldAutocaptureEvent(document!.createElement(`form`), makeMouseEvent({}))).toBe(false)
         })
 
         it.each([`html`, 'body'])(`should NOT capture "click" events on <%s> elements`, (tagName) => {
-            expect(shouldCaptureDomEvent(document!.createElement(tagName), makeMouseEvent({}))).toBe(false)
+            expect(shouldAutocaptureEvent(document!.createElement(tagName), makeMouseEvent({}))).toBe(false)
         })
 
         describe('css selector allowlist', () => {
@@ -317,7 +340,7 @@ describe(`Autocapture utility functions`, () => {
                 ],
             ])('correctly respects the allow list: %s', (_, clickTarget, autoCaptureConfig, shouldCapture) => {
                 expect(
-                    shouldCaptureDomEvent(clickTarget, makeMouseEvent({}), autoCaptureConfig as AutocaptureConfig)
+                    shouldAutocaptureEvent(clickTarget, makeMouseEvent({}), autoCaptureConfig as AutocaptureConfig)
                 ).toBe(shouldCapture)
             })
         })
@@ -348,110 +371,64 @@ describe(`Autocapture utility functions`, () => {
         })
     })
 
-    describe(`shouldCaptureElement`, () => {
+    describe(`isExplicitCapture`, () => {
+        it(`should return true for elements with class "ph-include"`, () => {
+            const el = document!.createElement(`div`)
+            el.className = `test1 ph-include test2`
+            expect(isExplicitCapture(el)).toBe(true)
+        })
+
+        it(`should return false for elements without class "ph-include"`, () => {
+            const el = document!.createElement(`div`)
+            el.className = `test1 test2`
+            expect(isExplicitCapture(el)).toBe(false)
+        })
+    })
+
+    describe(`isExplicitNoCapture`, () => {
         let el: HTMLDivElement
-        let input: HTMLInputElement
         let parent1: HTMLDivElement
         let parent2: HTMLDivElement
 
         beforeEach(() => {
             el = document!.createElement(`div`)
-            input = document!.createElement(`input`)
             parent1 = document!.createElement(`div`)
             parent2 = document!.createElement(`div`)
             parent1.appendChild(el)
-            parent1.appendChild(input)
             parent2.appendChild(parent1)
             document!.body.appendChild(parent2)
         })
 
-        it(`should include sensitive elements with class "ph-include"`, () => {
-            el.className = `test1 ph-include test2`
-            expect(shouldCaptureElement(el)).toBe(true)
-        })
-
-        it(`should never include inputs with class "ph-sensitive"`, () => {
+        it(`should return true for elements with class "ph-sensitive"`, () => {
             el.className = `test1 ph-include ph-sensitive test2`
-            expect(shouldCaptureElement(el)).toBe(false)
+            expect(isExplicitNoCapture(el)).toBe(true)
         })
 
-        it(`should not include elements with class "ph-no-capture" as properties`, () => {
+        it(`should return true for elements with class "ph-no-capture"`, () => {
             el.className = `test1 ph-no-capture test2`
-            expect(shouldCaptureElement(el)).toBe(false)
+            expect(isExplicitNoCapture(el)).toBe(true)
         })
 
-        it(`should not include elements with a parent that have class "ph-no-capture" as properties`, () => {
-            expect(shouldCaptureElement(el)).toBe(true)
+        it(`should return true for elements with a parent that has class "ph-no-capture"`, () => {
+            expect(isExplicitNoCapture(el)).toBe(false)
 
             parent2.className = `ph-no-capture`
 
-            expect(shouldCaptureElement(el)).toBe(false)
+            expect(isExplicitNoCapture(el)).toBe(true)
         })
+    })
 
-        it(`should not include hidden fields`, () => {
+    describe(`isSensitiveElement - hidden and password inputs`, () => {
+        it(`should return true for hidden fields`, () => {
+            const input = document!.createElement(`input`)
             input.type = `hidden`
-            expect(shouldCaptureElement(input)).toBe(false)
+            expect(isSensitiveElement(input)).toBe(true)
         })
 
-        it(`should not include password fields`, () => {
+        it(`should return true for password fields`, () => {
+            const input = document!.createElement(`input`)
             input.type = `password`
-            expect(shouldCaptureElement(input)).toBe(false)
-        })
-
-        it(`should not include fields with sensitive names`, () => {
-            const sensitiveNames = [
-                `cc_name`,
-                `card-num`,
-                `ccnum`,
-                `credit-card_number`,
-                `credit_card[number]`,
-                `csc num`,
-                `CVC`,
-                `Expiration`,
-                `password`,
-                `pwd`,
-                `routing`,
-                `routing-number`,
-                `security code`,
-                `seccode`,
-                `security number`,
-                `social sec`,
-                `SsN`,
-            ]
-            sensitiveNames.forEach((name) => {
-                input.name = ''
-                expect(shouldCaptureElement(input)).toBe(true)
-
-                input.name = name
-                expect(shouldCaptureElement(input)).toBe(false)
-            })
-        })
-
-        // See https://github.com/posthog/posthog-js/issues/165
-        // Under specific circumstances a bug caused .replace to be called on a DOM element
-        // instead of a string, removing the element from the page. Ensure this issue is mitigated.
-        it(`shouldn't inadvertently replace DOM nodes`, () => {
-            // setup
-            ;(el as any).replace = sinon.spy()
-
-            // test
-            input.name = el as any
-            shouldCaptureElement(parent1) // previously this would cause el.replace to be called
-            expect((el as any).replace.called).toBe(false)
-            input.name = ''
-
-            parent1.id = el as any
-            shouldCaptureElement(parent2) // previously this would cause el.replace to be called
-            expect((el as any).replace.called).toBe(false)
-            parent1.id = ''
-
-            input.type = el as any
-            shouldCaptureElement(parent2) // previously this would cause el.replace to be called
-            expect((el as any).replace.called).toBe(false)
-            input.type = ''
-
-            // cleanup
-            ;(el as any).replace = undefined
+            expect(isSensitiveElement(input)).toBe(true)
         })
     })