Skip to content

Commit

Permalink
fix: Explicitly handle null attribute values (rrweb-io#1157)
Browse files Browse the repository at this point in the history
* fix: Explicitly handle removed attributes

The attribute `value` can be null when a mutation observer triggers due to a removed attribute. This is currently not reflected by types and code.

* Apply formatting changes

* fix

* add changeset
  • Loading branch information
mydea committed Feb 27, 2023
1 parent e65465e commit 8e47ca1
Show file tree
Hide file tree
Showing 7 changed files with 256 additions and 15 deletions.
6 changes: 6 additions & 0 deletions .changeset/swift-peas-film.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'rrweb-snapshot': patch
'rrweb': patch
---

fix: Explicitly handle `null` attribute values
7 changes: 6 additions & 1 deletion packages/rrweb-snapshot/src/rebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ function buildNode(
* They often overwrite other attributes on the element.
* We need to parse them last so they can overwrite conflicting attributes.
*/
const specialAttributes: attributes = {};
const specialAttributes: { [key: string]: string | number } = {};
for (const name in n.attributes) {
if (!Object.prototype.hasOwnProperty.call(n.attributes, name)) {
continue;
Expand All @@ -165,6 +165,11 @@ function buildNode(
continue;
}

// null values mean the attribute was removed
if (value === null) {
continue;
}

/**
* Boolean attributes are considered to be true if they're present on the element at all.
* We should set value to the empty string ("") or the attribute's name, with no leading or trailing whitespace.
Expand Down
29 changes: 17 additions & 12 deletions packages/rrweb-snapshot/src/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,33 +223,36 @@ export function transformAttribute(
doc: Document,
tagName: string,
name: string,
value: string,
): string {
value: string | null,
): string | null {
if (!value) {
return value;
}

// relative path in attribute
if (
name === 'src' ||
(name === 'href' && value && !(tagName === 'use' && value[0] === '#'))
(name === 'href' && !(tagName === 'use' && value[0] === '#'))
) {
// href starts with a # is an id pointer for svg
return absoluteToDoc(doc, value);
} else if (name === 'xlink:href' && value && value[0] !== '#') {
} else if (name === 'xlink:href' && value[0] !== '#') {
// xlink:href starts with # is an id pointer
return absoluteToDoc(doc, value);
} else if (
name === 'background' &&
value &&
(tagName === 'table' || tagName === 'td' || tagName === 'th')
) {
return absoluteToDoc(doc, value);
} else if (name === 'srcset' && value) {
} else if (name === 'srcset') {
return getAbsoluteSrcsetString(doc, value);
} else if (name === 'style' && value) {
} else if (name === 'style') {
return absoluteToStylesheet(value, getHref());
} else if (tagName === 'object' && name === 'data' && value) {
} else if (tagName === 'object' && name === 'data') {
return absoluteToDoc(doc, value);
} else {
return value;
}

return value;
}

export function _isBlockedElement(
Expand Down Expand Up @@ -794,8 +797,10 @@ function serializeElementNode(
};
}

function lowerIfExists(maybeAttr: string | number | boolean): string {
if (maybeAttr === undefined) {
function lowerIfExists(
maybeAttr: string | number | boolean | undefined | null,
): string {
if (maybeAttr === undefined || maybeAttr === null) {
return '';
} else {
return (maybeAttr as string).toLowerCase();
Expand Down
2 changes: 1 addition & 1 deletion packages/rrweb-snapshot/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export type documentTypeNode = {
};

export type attributes = {
[key: string]: string | number | true;
[key: string]: string | number | true | null;
};
export type legacyAttributes = {
/**
Expand Down
2 changes: 1 addition & 1 deletion packages/rrweb/src/record/mutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ export default class MutationBuffer {
this.doc,
target.tagName,
m.attributeName!,
value!,
value,
);
}
break;
Expand Down
199 changes: 199 additions & 0 deletions packages/rrweb/test/__snapshots__/integration.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -3960,6 +3960,205 @@ exports[`record integration tests can use maskInputOptions to configure which ty
]"
`;

exports[`record integration tests handles null attribute values 1`] = `
"[
{
\\"type\\": 0,
\\"data\\": {}
},
{
\\"type\\": 1,
\\"data\\": {}
},
{
\\"type\\": 4,
\\"data\\": {
\\"href\\": \\"about:blank\\",
\\"width\\": 1920,
\\"height\\": 1080
}
},
{
\\"type\\": 2,
\\"data\\": {
\\"node\\": {
\\"type\\": 0,
\\"childNodes\\": [
{
\\"type\\": 1,
\\"name\\": \\"html\\",
\\"publicId\\": \\"\\",
\\"systemId\\": \\"\\",
\\"id\\": 2
},
{
\\"type\\": 2,
\\"tagName\\": \\"html\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 2,
\\"tagName\\": \\"head\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 4
},
{
\\"type\\": 2,
\\"tagName\\": \\"body\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 6
},
{
\\"type\\": 2,
\\"tagName\\": \\"p\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"mutation observer\\",
\\"id\\": 8
}
],
\\"id\\": 7
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 9
},
{
\\"type\\": 2,
\\"tagName\\": \\"ul\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 11
},
{
\\"type\\": 2,
\\"tagName\\": \\"li\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 12
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 13
}
],
\\"id\\": 10
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\",
\\"id\\": 14
},
{
\\"type\\": 2,
\\"tagName\\": \\"canvas\\",
\\"attributes\\": {},
\\"childNodes\\": [],
\\"id\\": 15
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n\\\\n \\",
\\"id\\": 16
},
{
\\"type\\": 2,
\\"tagName\\": \\"script\\",
\\"attributes\\": {},
\\"childNodes\\": [
{
\\"type\\": 3,
\\"textContent\\": \\"SCRIPT_PLACEHOLDER\\",
\\"id\\": 18
}
],
\\"id\\": 17
},
{
\\"type\\": 3,
\\"textContent\\": \\"\\\\n \\\\n \\\\n\\",
\\"id\\": 19
}
],
\\"id\\": 5
}
],
\\"id\\": 3
}
],
\\"id\\": 1
},
\\"initialOffset\\": {
\\"left\\": 0,
\\"top\\": 0
}
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 20,
\\"attributes\\": {
\\"aria-label\\": \\"label\\",
\\"id\\": \\"test-li\\"
}
}
],
\\"removes\\": [],
\\"adds\\": [
{
\\"parentId\\": 10,
\\"nextId\\": null,
\\"node\\": {
\\"type\\": 2,
\\"tagName\\": \\"li\\",
\\"attributes\\": {
\\"aria-label\\": \\"label\\",
\\"id\\": \\"test-li\\"
},
\\"childNodes\\": [],
\\"id\\": 20
}
}
]
}
},
{
\\"type\\": 3,
\\"data\\": {
\\"source\\": 0,
\\"texts\\": [],
\\"attributes\\": [
{
\\"id\\": 20,
\\"attributes\\": {
\\"aria-label\\": null
}
}
],
\\"removes\\": [],
\\"adds\\": []
}
}
]"
`;

exports[`record integration tests mutations should work when blocked class is unblocked 1`] = `
"[
{
Expand Down
26 changes: 26 additions & 0 deletions packages/rrweb/test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,32 @@ describe('record integration tests', function (this: ISuite) {
assertSnapshot(snapshots);
});

it('handles null attribute values', async () => {
const page: puppeteer.Page = await browser.newPage();
await page.goto('about:blank');
await page.setContent(getHtml.call(this, 'mutation-observer.html', {}));

await page.evaluate(() => {
const li = document.createElement('li');
const ul = document.querySelector('ul') as HTMLUListElement;
ul.appendChild(li);

li.setAttribute('aria-label', 'label');
li.setAttribute('id', 'test-li');
});

await new Promise((resolve) => setTimeout(resolve, 100));

await page.evaluate(() => {
const li = document.querySelector('#test-li') as HTMLLIElement;
// This triggers the mutation observer with a `null` attribute value
li.removeAttribute('aria-label');
});

const snapshots = await page.evaluate('window.snapshots');
assertSnapshot(snapshots);
});

it('can record node mutations', async () => {
const page: puppeteer.Page = await browser.newPage();
await page.goto('about:blank');
Expand Down

0 comments on commit 8e47ca1

Please sign in to comment.