Skip to content

feat(browser): support dom.maxStringLength configuration (#6175) #6311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 6, 2022
Merged

feat(browser): support dom.maxStringLength configuration (#6175) #6311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd44969
feat(browser): support dom.maxStringLength configuration (#6175)
ZachGawlik Nov 27, 2022
2f80141
refactor(utils): move htmlTreeAsString args to object
ZachGawlik Nov 27, 2022
1091715
feat(dom): add debug warning to capping maxStringLength
ZachGawlik Nov 28, 2022
7f7d78d
refactor(browser): rewrite logger call for consistency
ZachGawlik Dec 2, 2022
052be4b
fix(utils): un-break current htmlTreeAsString keyAttrs usages
ZachGawlik Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions packages/browser/src/integrations/breadcrumbs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
addInstrumentationHandler,
getEventDescription,
htmlTreeAsString,
logger,
parseUrl,
safeJoin,
severityLevelFromString,
Expand All @@ -16,13 +17,21 @@ import { WINDOW } from '../helpers';
/** JSDoc */
interface BreadcrumbsOptions {
console: boolean;
dom: boolean | { serializeAttribute: string | string[] };
dom:
| boolean
| {
serializeAttribute?: string | string[];
maxStringLength?: number;
};
fetch: boolean;
history: boolean;
sentry: boolean;
xhr: boolean;
}

/** maxStringLength gets capped to prevent 100 breadcrumbs exceeding 1MB event payload size */
const MAX_ALLOWED_STRING_LENGTH = 1024;

export const BREADCRUMB_INTEGRATION_ID = 'Breadcrumbs';

/**
Expand Down Expand Up @@ -118,15 +127,25 @@ function _domBreadcrumb(dom: BreadcrumbsOptions['dom']): (handlerData: { [key: s
let target;
let keyAttrs = typeof dom === 'object' ? dom.serializeAttribute : undefined;

let maxStringLength =
typeof dom === 'object' && typeof dom.maxStringLength === 'number' ? dom.maxStringLength : undefined;
if (maxStringLength && maxStringLength > MAX_ALLOWED_STRING_LENGTH) {
__DEBUG_BUILD__ &&
logger.warn(
`\`dom.maxStringLength\` cannot exceed ${MAX_ALLOWED_STRING_LENGTH}, but a value of ${maxStringLength} was configured. Sentry will use ${MAX_ALLOWED_STRING_LENGTH} instead.`,
);
maxStringLength = MAX_ALLOWED_STRING_LENGTH;
}

if (typeof keyAttrs === 'string') {
keyAttrs = [keyAttrs];
}

// Accessing event.target can throw (see getsentry/raven-js#838, #768)
try {
target = handlerData.event.target
? htmlTreeAsString(handlerData.event.target as Node, keyAttrs)
: htmlTreeAsString(handlerData.event as unknown as Node, keyAttrs);
? htmlTreeAsString(handlerData.event.target as Node, { keyAttrs, maxStringLength })
: htmlTreeAsString(handlerData.event as unknown as Node, { keyAttrs, maxStringLength });
} catch (e) {
target = '<unknown>';
}
Expand Down
14 changes: 10 additions & 4 deletions packages/utils/src/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@ import { getGlobalObject } from './worldwide';
// eslint-disable-next-line deprecation/deprecation
const WINDOW = getGlobalObject<Window>();

const DEFAULT_MAX_STRING_LENGTH = 80;

/**
* Given a child DOM element, returns a query-selector statement describing that
* and its ancestors
* e.g. [HTMLElement] => body > div > input#foo.btn[name=baz]
* @returns generated DOM path
*/
export function htmlTreeAsString(elem: unknown, keyAttrs?: string[]): string {
export function htmlTreeAsString(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually, this is technically a breaking change (as this method is exported...), so we can't change the signature in that way 😬

Can you make this:

export function htmlTreeAsString(
  elem: unknown, 
  keyAttrs?: string[],
  options?: { maxStringLength?: number }
) {}

It's not as nice, but will have to do for now!

Or, alternatively, make it a union type:

export function htmlTreeAsString(
  elem: unknown, 
  options?: { keyAttrs?: string[], maxStringLength?: number } | string[]
) {}

And handle this in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agh thanks! I didn't think through how this function isn't merely exposed to other packages within this repo, but that it's also exposed anywhere that imports @sentry/utils. I've updated to that latter format, and I've restored the test for the original format.

elem: unknown,
options: string[] | { keyAttrs?: string[]; maxStringLength?: number } = {},
): string {
type SimpleNode = {
parentNode: SimpleNode;
} | null;
Expand All @@ -22,21 +27,22 @@ export function htmlTreeAsString(elem: unknown, keyAttrs?: string[]): string {
try {
let currentElem = elem as SimpleNode;
const MAX_TRAVERSE_HEIGHT = 5;
const MAX_OUTPUT_LEN = 80;
const out = [];
let height = 0;
let len = 0;
const separator = ' > ';
const sepLength = separator.length;
let nextStr;
const keyAttrs = Array.isArray(options) ? options : options.keyAttrs;
const maxStringLength = (!Array.isArray(options) && options.maxStringLength) || DEFAULT_MAX_STRING_LENGTH;

while (currentElem && height++ < MAX_TRAVERSE_HEIGHT) {
nextStr = _htmlElementAsString(currentElem, keyAttrs);
// bail out if
// - nextStr is the 'html' element
// - the length of the string that would be created exceeds MAX_OUTPUT_LEN
// - the length of the string that would be created exceeds maxStringLength
// (ignore this limit if we are on the first iteration)
if (nextStr === 'html' || (height > 1 && len + out.length * sepLength + nextStr.length >= MAX_OUTPUT_LEN)) {
if (nextStr === 'html' || (height > 1 && len + out.length * sepLength + nextStr.length >= maxStringLength)) {
break;
}

Expand Down
25 changes: 25 additions & 0 deletions packages/utils/test/browser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ beforeAll(() => {
});

describe('htmlTreeAsString', () => {
beforeEach(() => {
document.body.innerHTML = '';
});

it('generates html tree for a simple element', () => {
const el = document.createElement('ul');
el.innerHTML = `<li class="container">
Expand Down Expand Up @@ -40,9 +44,30 @@ describe('htmlTreeAsString', () => {
</li>`;
document.body.appendChild(el);

// Two formats for specifying keyAttrs
expect(htmlTreeAsString(document.getElementById('cat-2'), ['test-id'])).toBe(
'body > ul > li.li-class[title="li-title"] > img[test-id="cat-2-test-id"]',
);
expect(htmlTreeAsString(document.getElementById('cat-2'), { keyAttrs: ['test-id'] })).toBe(
'body > ul > li.li-class[title="li-title"] > img[test-id="cat-2-test-id"]',
);
});

it('caps string output according to provided maxStringLength', () => {
const el = document.createElement('div');
el.innerHTML = `<div id="main-cta">
<div class="container">
<button class="bg-blue-500 hover:bg-blue-700 text-white hover:text-blue-100" />
</div>
</div>`;
document.body.appendChild(el);

expect(htmlTreeAsString(document.querySelector('button'))).toBe(
'button.bg-blue-500.hover:bg-blue-700.text-white.hover:text-blue-100',
);
expect(htmlTreeAsString(document.querySelector('button'), { maxStringLength: 100 })).toBe(
'div#main-cta > div.container > button.bg-blue-500.hover:bg-blue-700.text-white.hover:text-blue-100',
);
});
});

Expand Down