-
Notifications
You must be signed in to change notification settings - Fork 138
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.
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
fix: sanitize class string more #925
Conversation
@tiina303 since this touches an area you're actively working on. I tried to keep a minimal change so hopefully I can fix the issue in PostHog/posthog#19068 without clashing with your work |
Size Change: +460 B (0%) Total Size: 745 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I know very little of this, I was mainly just moving code around. Let's make sure we mirror in plugin-server side & maybe add comments to keep in sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks fine, but we should test for all the acceptable whitespace characters
src/__tests__/autocapture.test.ts
Outdated
it('returns elementsChain correctly with newlines in css', () => { | ||
const elTarget = document.createElement('a') | ||
elTarget.setAttribute('href', 'http://test.com') | ||
elTarget.setAttribute('class', 'test-class\n test-class2 test-class3 test-class4 \r\n test-class5') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add \t
and \f
characters in here, just for completeness sake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We are very intolerant of class strings that contain multiple spaces, tabs, or new lines when processing autocapture.
Technically the class attribute should always be a space separated set of strings, but computers let you generate all kinds of HTML 🙈
Follow-up to #919
This sanitizes the class string in more places so that we generate a working elements chain string even with an invalid css class.