♻️ [PANA-5105] Serialize all DOM attribute values as strings (#3999)

Author: sethfowler-datadog

packages/rum/src/domain/record/serialization/htmlAst.specHelper.ts

@@ -1438,7 +1438,7 @@ export const AST_ALLOW = {
                   type: 2,
                   tagName: 'option',
                   attributes: {
-                    selected: true,
+                    selected: '',
                     'aria-label': 'A',
                     value: 'private option A',
                   },
@@ -1526,7 +1526,7 @@ export const AST_ALLOW = {
                 type: 'checkbox',
                 name: 'inputFoo',
                 value: 'on',
-                checked: true,
+                checked: '',
               },
               childNodes: [],
             },
@@ -1558,7 +1558,6 @@ export const AST_ALLOW = {
                 type: 'radio',
                 name: 'radioGroup',
                 value: 'bar-private',
-                checked: false,
               },
               childNodes: [],
             },

packages/rum/src/domain/record/serialization/serializeAttributes.spec.ts

@@ -61,19 +61,9 @@ describe('serializeDOMAttributes', () => {
       // serialized at all. Used for boolean form element attributes like `<option selected>`.
       // - 'maskable-image': Like 'maskable', except that we expect the masked
       // representation to be a data URL image.
-      // - 'maskable-option-selected': Like 'maskable-boolean', in that when unmasked the
-      // representation is a boolean value. However, when the value is masked, instead of
-      // not being serialized, its underlying string value is used. The effect is a weird
-      // hybrid of 'always-unmasked' and 'maskable-boolean'.
-      // TODO: Reduce the complexity here by aligning 'maskable-form',
-      // 'maskable-form-boolean', and 'maskable-option-selected'.
-      expectedBehavior:
-        | 'always-unmasked'
-        | 'maskable'
-        | 'maskable-form'
-        | 'maskable-form-boolean'
-        | 'maskable-image'
-        | 'maskable-option-selected'
+      // TODO: Reduce the complexity here by aligning 'maskable-form' and
+      // 'maskable-form-boolean'.
+      expectedBehavior: 'always-unmasked' | 'maskable' | 'maskable-form' | 'maskable-form-boolean' | 'maskable-image'
 
       // How to treat the IGNORE privacy level. The default is 'unmasked'.
       // TODO: Eliminate this inconsistency by always masking for IGNORE.
@@ -225,10 +215,7 @@ describe('serializeDOMAttributes', () => {
       // when masked, the entire attribute should not be serialized.
       {
         html: '<option selected>',
-        // TODO: This is a bug! If the <option> is masked, we don't set the value of
-        // 'selected' based on the property, but we still allow the DOM attribute to be
-        // recorded. We should fix this; this should be 'maskable-boolean'.
-        expectedBehavior: 'maskable-option-selected',
+        expectedBehavior: 'maskable-form-boolean',
         ignoreBehavior: 'masked',
       },
       {
@@ -285,13 +272,7 @@ describe('serializeDOMAttributes', () => {
       attribute: { name: string; value: string | undefined },
       privacyLevel: NodePrivacyLevel
     ): boolean | string | undefined {
-      let unmaskedValue: boolean | string | undefined = testCase.attrValue ?? attribute.value
-      if (
-        testCase.expectedBehavior === 'maskable-form-boolean' ||
-        testCase.expectedBehavior === 'maskable-option-selected'
-      ) {
-        unmaskedValue = attribute.value === ''
-      }
+      const unmaskedValue: boolean | string | undefined = testCase.attrValue ?? attribute.value
 
       let maskedValue: boolean | string | undefined
       if (testCase.expectedBehavior === 'maskable-form') {
@@ -300,8 +281,6 @@ describe('serializeDOMAttributes', () => {
         maskedValue = undefined
       } else if (testCase.expectedBehavior === 'maskable-image') {
         maskedValue = CENSORED_IMG_MARK
-      } else if (testCase.expectedBehavior === 'maskable-option-selected') {
-        maskedValue = attribute.value
       } else {
         maskedValue = CENSORED_STRING_MARK
       }
@@ -346,7 +325,7 @@ describe('serializeDOMAttributes', () => {
     // behavior; it shouldn't behave differently because Element#tagName is lowercase.
     ;(element as any).selected = true
     const attributes = serializeDOMAttributes(element, NodePrivacyLevel.ALLOW, transaction)
-    expect(attributes['selected']).toBe(true)
+    expect(attributes['selected']).toBe('')
   })
 })
 

packages/rum/src/domain/record/serialization/serializeAttributes.ts

@@ -10,7 +10,7 @@ export function serializeAttributes(
   element: Element,
   nodePrivacyLevel: NodePrivacyLevel,
   transaction: SerializationTransaction
-): Record<string, boolean | number | string> {
+): Record<string, number | string> {
   return {
     ...serializeDOMAttributes(element, nodePrivacyLevel, transaction),
     ...serializeVirtualAttributes(element, nodePrivacyLevel, transaction),
@@ -21,12 +21,12 @@ export function serializeDOMAttributes(
   element: Element,
   nodePrivacyLevel: NodePrivacyLevel,
   transaction: SerializationTransaction
-): Record<string, boolean | string> {
+): Record<string, string> {
   if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) {
     return {}
   }
 
-  const attrs: Record<string, string | boolean> = {}
+  const attrs: Record<string, string> = {}
   const tagName = normalizedTagName(element)
 
   for (let i = 0; i < element.attributes.length; i += 1) {
@@ -51,11 +51,12 @@ export function serializeDOMAttributes(
   /**
    * <Option> can be selected, which occurs if its `value` matches ancestor `<Select>.value`
    */
-  if (tagName === 'option' && nodePrivacyLevel === NodePrivacyLevel.ALLOW) {
-    // For privacy=`MASK`, all the values would be the same, so skip.
+  if (tagName === 'option') {
     const optionElement = element as HTMLOptionElement
-    if (optionElement.selected) {
-      attrs.selected = optionElement.selected
+    if (optionElement.selected && !shouldMaskNode(optionElement, nodePrivacyLevel)) {
+      attrs.selected = ''
+    } else {
+      delete attrs.selected
     }
   }
 
@@ -69,9 +70,9 @@ export function serializeDOMAttributes(
    */
   const inputElement = element as HTMLInputElement
   if (tagName === 'input' && (inputElement.type === 'radio' || inputElement.type === 'checkbox')) {
-    if (nodePrivacyLevel === NodePrivacyLevel.ALLOW) {
-      attrs.checked = !!inputElement.checked
-    } else if (shouldMaskNode(inputElement, nodePrivacyLevel)) {
+    if (inputElement.checked && !shouldMaskNode(inputElement, nodePrivacyLevel)) {
+      attrs.checked = ''
+    } else {
       delete attrs.checked
     }
   }

packages/rum/src/domain/record/serialization/serializeNode.spec.ts

@@ -239,7 +239,7 @@ describe('serializeNodeWithId', () => {
             jasmine.objectContaining({
               attributes: {
                 value: 'bar',
-                selected: true,
+                selected: '',
               },
             }),
           ],
@@ -271,7 +271,6 @@ describe('serializeNodeWithId', () => {
           attributes: {
             type: 'checkbox',
             value: 'on',
-            checked: false,
           },
         })
       )
@@ -283,7 +282,7 @@ describe('serializeNodeWithId', () => {
           attributes: {
             type: 'checkbox',
             value: 'on',
-            checked: true,
+            checked: '',
           },
         })
       )
@@ -295,15 +294,14 @@ describe('serializeNodeWithId', () => {
       expect(serializeNodeWithId(radio, NodePrivacyLevel.ALLOW, transaction)?.attributes).toEqual({
         type: 'radio',
         value: 'on',
-        checked: false,
       })
 
       radio.checked = true
 
       expect(serializeNodeWithId(radio, NodePrivacyLevel.ALLOW, transaction)?.attributes).toEqual({
         type: 'radio',
         value: 'on',
-        checked: true,
+        checked: '',
       })
     })