From fdf26d5d4d84697917c47f49ff9a3d3d84fb9f48 Mon Sep 17 00:00:00 2001 From: Thibaut Gery Date: Wed, 15 Feb 2023 10:52:15 +0100 Subject: [PATCH 1/5] [REPLAY] Avoid casting & add proper check instead of relying on try/catch --- packages/rum/src/domain/record/serialize.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/rum/src/domain/record/serialize.ts b/packages/rum/src/domain/record/serialize.ts index 7f3cc1d879..4dd6a37f15 100644 --- a/packages/rum/src/domain/record/serialize.ts +++ b/packages/rum/src/domain/record/serialize.ts @@ -356,7 +356,10 @@ function getValidTagName(tagName: string): string { return processedTagName } -function getCssRulesString(s: CSSStyleSheet): string | null { +function getCssRulesString(s: CSSStyleSheet | undefined | null): string | null { + if (!s) { + return null + } try { const rules = s.rules || s.cssRules if (rules) { @@ -428,7 +431,7 @@ function getAttributesForPrivacyLevel( // remote css if (tagName === 'link') { const stylesheet = Array.from(doc.styleSheets).find((s) => s.href === (element as HTMLLinkElement).href) - const cssText = getCssRulesString(stylesheet as CSSStyleSheet) + const cssText = getCssRulesString(stylesheet) if (cssText && stylesheet) { safeAttrs._cssText = cssText } @@ -441,7 +444,7 @@ function getAttributesForPrivacyLevel( // TODO: Currently we only try to get dynamic stylesheet when it is an empty style element !((element as HTMLStyleElement).innerText || element.textContent || '').trim().length ) { - const cssText = getCssRulesString((element as HTMLStyleElement).sheet as CSSStyleSheet) + const cssText = getCssRulesString((element as HTMLStyleElement).sheet) if (cssText) { safeAttrs._cssText = cssText } From e04498fcc470c0d7bb243b03fe7db79fbedc5f24 Mon Sep 17 00:00:00 2001 From: Thibaut Gery Date: Mon, 20 Feb 2023 11:51:47 +0100 Subject: [PATCH 2/5] [REPLAY] Remove try/catch & add monitoring --- packages/rum/src/domain/record/serialize.ts | 26 ++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/rum/src/domain/record/serialize.ts b/packages/rum/src/domain/record/serialize.ts index 4dd6a37f15..b767479be6 100644 --- a/packages/rum/src/domain/record/serialize.ts +++ b/packages/rum/src/domain/record/serialize.ts @@ -1,4 +1,4 @@ -import { assign, startsWith } from '@datadog/browser-core' +import { assign, monitor, startsWith } from '@datadog/browser-core' import type { RumConfiguration } from '@datadog/browser-rum-core' import { isNodeShadowHost, isNodeShadowRoot, STABLE_ATTRIBUTES } from '@datadog/browser-rum-core' import { @@ -356,25 +356,25 @@ function getValidTagName(tagName: string): string { return processedTagName } -function getCssRulesString(s: CSSStyleSheet | undefined | null): string | null { - if (!s) { - return null - } - try { - const rules = s.rules || s.cssRules - if (rules) { - const styleSheetCssText = Array.from(rules, getCssRuleString).join('') - return switchToAbsoluteUrl(styleSheetCssText, s.href) - } +function getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null { + // TODO: remove monitor once we're confident the try/catch was not hiding more error + return monitor(_getCssRulesString)(cssStyleSheet) +} +function _getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null { + if (!cssStyleSheet) { return null - } catch (error) { + } + const rules = cssStyleSheet.rules || cssStyleSheet.cssRules + if (!rules) { return null } + const styleSheetCssText = Array.from(rules, getCssRuleString).join('') + return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href) } function getCssRuleString(rule: CSSRule): string { - return isCSSImportRule(rule) ? getCssRulesString(rule.styleSheet) || '' : rule.cssText + return isCSSImportRule(rule) ? _getCssRulesString(rule.styleSheet) || '' : rule.cssText } function isCSSImportRule(rule: CSSRule): rule is CSSImportRule { From b465fd079290cd25a5f5eb1baf6d241c6b9a6641 Mon Sep 17 00:00:00 2001 From: Thibaut Gery Date: Mon, 20 Feb 2023 15:44:54 +0100 Subject: [PATCH 3/5] [REPLAY] Add check on css behind CORS restriction --- .../rum/src/domain/record/serialize.spec.ts | 25 +++++++++++++++++++ packages/rum/src/domain/record/serialize.ts | 18 +++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/packages/rum/src/domain/record/serialize.spec.ts b/packages/rum/src/domain/record/serialize.spec.ts index 543eb1644c..76403a55d3 100644 --- a/packages/rum/src/domain/record/serialize.spec.ts +++ b/packages/rum/src/domain/record/serialize.spec.ts @@ -558,6 +558,31 @@ describe('serializeNodeWithId', () => { childNodes: [], }) }) + + it('does not serialize style node with dynamic CSS that is behind CORS', () => { + const linkNode = document.createElement('link') + linkNode.setAttribute('rel', 'stylesheet') + linkNode.setAttribute('href', 'https://datadoghq.com/some/style.css') + isolatedDom.document.head.appendChild(linkNode) + const styleSheet = new CSSStyleSheet() + spyOnProperty(styleSheet, 'cssRules', 'get').and.throwError(new DOMException('cors issue', 'SecurityError')) + + Object.defineProperty(isolatedDom.document, 'styleSheets', { + value: [styleSheet], + }) + + expect(serializeNodeWithId(linkNode, DEFAULT_OPTIONS)).toEqual({ + type: NodeType.Element, + tagName: 'link', + id: jasmine.any(Number) as unknown as number, + isSVG: undefined, + attributes: { + rel: 'stylesheet', + href: 'https://datadoghq.com/some/style.css', + }, + childNodes: [], + }) + }) }) describe('text nodes serialization', () => { diff --git a/packages/rum/src/domain/record/serialize.ts b/packages/rum/src/domain/record/serialize.ts index b767479be6..7b3a152033 100644 --- a/packages/rum/src/domain/record/serialize.ts +++ b/packages/rum/src/domain/record/serialize.ts @@ -365,12 +365,20 @@ function _getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): st if (!cssStyleSheet) { return null } - const rules = cssStyleSheet.rules || cssStyleSheet.cssRules - if (!rules) { - return null + try { + const rules = cssStyleSheet.rules || cssStyleSheet.cssRules + if (!rules) { + return null + } + const styleSheetCssText = Array.from(rules, getCssRuleString).join('') + return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href) + } catch (err) { + if (err instanceof DOMException && err.name === 'SecurityError') { + // if css is protected by CORS we cannot access cssRules see: https://www.w3.org/TR/cssom-1/#the-cssstylesheet-interface + return null + } + throw err } - const styleSheetCssText = Array.from(rules, getCssRuleString).join('') - return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href) } function getCssRuleString(rule: CSSRule): string { From 3705df105424eccbb9996d97f519aa7d94f4da18 Mon Sep 17 00:00:00 2001 From: Thibaut Gery Date: Thu, 23 Feb 2023 09:53:56 +0100 Subject: [PATCH 4/5] [REPLAY] Refactor try/catch --- packages/rum/src/domain/record/serialize.ts | 30 ++++++++------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/packages/rum/src/domain/record/serialize.ts b/packages/rum/src/domain/record/serialize.ts index 7b3a152033..0f7eaded7e 100644 --- a/packages/rum/src/domain/record/serialize.ts +++ b/packages/rum/src/domain/record/serialize.ts @@ -1,4 +1,4 @@ -import { assign, monitor, startsWith } from '@datadog/browser-core' +import { assign, startsWith } from '@datadog/browser-core' import type { RumConfiguration } from '@datadog/browser-rum-core' import { isNodeShadowHost, isNodeShadowRoot, STABLE_ATTRIBUTES } from '@datadog/browser-rum-core' import { @@ -357,32 +357,24 @@ function getValidTagName(tagName: string): string { } function getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null { - // TODO: remove monitor once we're confident the try/catch was not hiding more error - return monitor(_getCssRulesString)(cssStyleSheet) -} - -function _getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null { if (!cssStyleSheet) { return null } + let rules: CSSRuleList | undefined try { - const rules = cssStyleSheet.rules || cssStyleSheet.cssRules - if (!rules) { - return null - } - const styleSheetCssText = Array.from(rules, getCssRuleString).join('') - return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href) - } catch (err) { - if (err instanceof DOMException && err.name === 'SecurityError') { - // if css is protected by CORS we cannot access cssRules see: https://www.w3.org/TR/cssom-1/#the-cssstylesheet-interface - return null - } - throw err + rules = cssStyleSheet.rules || cssStyleSheet.cssRules + } catch { + // if css is protected by CORS we cannot access cssRules see: https://www.w3.org/TR/cssom-1/#the-cssstylesheet-interface + } + if (!rules) { + return null } + const styleSheetCssText = Array.from(rules, getCssRuleString).join('') + return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href) } function getCssRuleString(rule: CSSRule): string { - return isCSSImportRule(rule) ? _getCssRulesString(rule.styleSheet) || '' : rule.cssText + return isCSSImportRule(rule) ? getCssRulesString(rule.styleSheet) || '' : rule.cssText } function isCSSImportRule(rule: CSSRule): rule is CSSImportRule { From b1abdd361df1a7b1f3c5b4f57e7c9171e58ed011 Mon Sep 17 00:00:00 2001 From: Thibaut Gery Date: Thu, 23 Feb 2023 10:51:13 +0100 Subject: [PATCH 5/5] [REPLAY] Fix unit test on old browser Old browser do not have a constructor for CssStyleSheet --- packages/rum/src/domain/record/serialize.spec.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/rum/src/domain/record/serialize.spec.ts b/packages/rum/src/domain/record/serialize.spec.ts index 95487fc770..508db0c15a 100644 --- a/packages/rum/src/domain/record/serialize.spec.ts +++ b/packages/rum/src/domain/record/serialize.spec.ts @@ -601,7 +601,12 @@ describe('serializeNodeWithId', () => { linkNode.setAttribute('rel', 'stylesheet') linkNode.setAttribute('href', 'https://datadoghq.com/some/style.css') isolatedDom.document.head.appendChild(linkNode) - const styleSheet = new CSSStyleSheet() + class FakeCSSStyleSheet { + get cssRules() { + return [] + } + } + const styleSheet = new FakeCSSStyleSheet() spyOnProperty(styleSheet, 'cssRules', 'get').and.throwError(new DOMException('cors issue', 'SecurityError')) Object.defineProperty(isolatedDom.document, 'styleSheets', {