Skip to content
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(surveys): Validate HTML for all questions and descriptions #824

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"@rrweb/types": "^2.0.0-alpha.11",
"@sentry/types": "7.37.2",
"@testing-library/dom": "^9.3.0",
"@types/dompurify": "^3.0.3",
"@types/jest": "^29.5.1",
"@types/react-dom": "^18.0.10",
"@types/uuid": "^9.0.1",
Expand All @@ -51,6 +52,7 @@
"babel-eslint": "10.1.0",
"babel-jest": "^26.6.3",
"cypress": "10.3.1",
"dompurify": "^3.0.6",
"eslint": "8.20.0",
"eslint-config-prettier": "^8.5.0",
"eslint-plugin-compat": "^4.1.4",
Expand Down
4 changes: 3 additions & 1 deletion rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ export default [
name: 'posthog',
},
],
plugins: [...plugins],
// We need to make sure DOMPurify is bundled together with the surveys code
// hence the no-module option resolution
plugins: [resolve(), ...plugins],
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@benjackwhite as the rollup expert, do you know of a better way I can bundle an external module dependency into surveys.js ? I don't think it makes much sense to download this from elsewhere, as versions can be different, it can change under us, etc. etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, the resolve() configuration options for other things doesn't appear to make a difference, do you remember what it was for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No idea 🙈

},
{
input: 'src/loader-globals.ts',
Expand Down
84 changes: 83 additions & 1 deletion src/__tests__/extensions/surveys.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createShadow, callSurveys, generateSurveys } from '../../extensions/surveys'
import { createShadow, callSurveys, generateSurveys, validateAndFixHTML } from '../../extensions/surveys'

describe('survey display logic', () => {
beforeEach(() => {
Expand Down Expand Up @@ -152,4 +152,86 @@ describe('survey display logic', () => {
expect(mockPostHog.getActiveMatchingSurveys).toBeCalledTimes(1)
expect(setInterval).toHaveBeenLastCalledWith(expect.any(Function), 1500)
})

test('survey fixes broken HTML', () => {
mockSurveys = [
{
id: 'testSurvey1',
name: 'Test survey 1',
appearance: null,
questions: [
{
question: 'How satisfied are you with our newest product?<li>test',
description: 'This is a question description<li>break</br><br/>',
type: 'rating',
display: 'number',
scale: 10,
lower_bound_label: 'Not Satisfied',
upper_bound_label: 'Very Satisfied',
},
],
},
]

expect(localStorage.getItem(`seenSurvey_${mockSurveys[0].id}`)).toBe(null)
callSurveys(mockPostHog, false)
expect(mockPostHog.capture).toBeCalledTimes(1)
expect(mockPostHog.capture).toBeCalledWith('survey shown', {
$survey_id: 'testSurvey1',
$survey_name: 'Test survey 1',
sessionRecordingUrl: undefined,
})

expect(
document
.getElementsByClassName(`PostHogSurvey${mockSurveys[0].id}`)[0]
.shadowRoot.querySelectorAll('.survey-question')[0].innerHTML
).toEqual('How satisfied are you with our newest product?<li>test</li>')
expect(
document
.getElementsByClassName(`PostHogSurvey${mockSurveys[0].id}`)[0]
.shadowRoot.querySelectorAll('.description')[0].innerHTML
).toEqual('This is a question description<li>break<br><br></li>')
})
})

describe('validateAndFixHTML', () => {
it('should do nothing to valid HTML', () => {
const html = '<div><h1>Test</h1><p>Test</p></div>'
expect(validateAndFixHTML(html)).toEqual('<div><h1>Test</h1><p>Test</p></div>')
})

it('should fix broken HTML', () => {
const html = '<div><h1>Test</h1><p>Test</p></div'
expect(validateAndFixHTML(html)).toEqual('<div><h1>Test</h1><p>Test</p></div>')
})

it('should do nothing to regular strings', () => {
const html = 'Test'
expect(validateAndFixHTML(html)).toEqual('Test')
})

it('should do nothing to empty strings', () => {
const html = ''
expect(validateAndFixHTML(html)).toEqual('')
})

it('returns undefined when passed undefined', () => {
expect(validateAndFixHTML(undefined)).toEqual(undefined)
})

it('completes missing closing tags', () => {
const html = '<div><h1>Test</h1><p>Test</p>'
expect(validateAndFixHTML(html)).toEqual('<div><h1>Test</h1><p>Test</p></div>')
})

it('should not execute script tags', () => {
const html = '<hr><script>alert("test?")</script>'
expect(validateAndFixHTML(html)).toEqual('<hr>')
})

it('should sanitize image tags', () => {
const html = '<img src="x" onerror="alert(\'test\')">'
expect(validateAndFixHTML(html)).toEqual('<img src="x">')
})
})
15 changes: 15 additions & 0 deletions src/extensions/surveys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
SurveyAppearance,
SurveyQuestion,
} from '../posthog-surveys-types'
import DOMPurify from 'dompurify'

const satisfiedEmoji =
'<svg class="emoji-svg" xmlns="http://www.w3.org/2000/svg" height="48" viewBox="0 -960 960 960" width="48"><path d="M626-533q22.5 0 38.25-15.75T680-587q0-22.5-15.75-38.25T626-641q-22.5 0-38.25 15.75T572-587q0 22.5 15.75 38.25T626-533Zm-292 0q22.5 0 38.25-15.75T388-587q0-22.5-15.75-38.25T334-641q-22.5 0-38.25 15.75T280-587q0 22.5 15.75 38.25T334-533Zm146 272q66 0 121.5-35.5T682-393h-52q-23 40-63 61.5T480.5-310q-46.5 0-87-21T331-393h-53q26 61 81 96.5T480-261Zm0 181q-83 0-156-31.5T197-197q-54-54-85.5-127T80-480q0-83 31.5-156T197-763q54-54 127-85.5T480-880q83 0 156 31.5T763-763q54 54 85.5 127T880-480q0 83-31.5 156T763-197q-54 54-127 85.5T480-80Zm0-400Zm0 340q142.375 0 241.188-98.812Q820-337.625 820-480t-98.812-241.188Q622.375-820 480-820t-241.188 98.812Q140-622.375 140-480t98.812 241.188Q337.625-140 480-140Z"/></svg>'
Expand Down Expand Up @@ -661,6 +662,12 @@ export const callSurveys = (posthog: PostHog, forceReload: boolean = false) => {
}
}

// safe-validate all the questions & descriptions
survey.questions.forEach((question) => {
question.question = validateAndFixHTML(question.question) || ''
question.description = validateAndFixHTML(question.description || undefined)
})

if (!localStorage.getItem(`seenSurvey_${survey.id}`)) {
const shadow = createShadow(style(survey.id, survey?.appearance), survey.id)
let surveyPopup
Expand Down Expand Up @@ -875,3 +882,11 @@ export function generateSurveys(posthog: PostHog) {
}, 1500)
}
}

export function validateAndFixHTML(html?: string): string | undefined {
if (html === undefined || html === null) {
return undefined
}

return DOMPurify(window).sanitize(html, { USE_PROFILES: { html: true } })
}
17 changes: 17 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3609,6 +3609,13 @@
dependencies:
"@types/ms" "*"

"@types/dompurify@^3.0.3":
version "3.0.3"
resolved "https://registry.yarnpkg.com/@types/dompurify/-/dompurify-3.0.3.tgz#d34ba1cf4f8b8f2cbfe5d3118dc3b7d81858fa42"
integrity sha512-odiGr/9/qMqjcBOe5UhcNLOFHSYmKFOyr+bJ/Xu3Qp4k1pNPAlNLUVNNLcLfjQI7+W7ObX58EdD3H+3p3voOvA==
dependencies:
"@types/trusted-types" "*"

"@types/error-stack-parser@^1.3.18":
version "1.3.18"
resolved "https://registry.yarnpkg.com/@types/error-stack-parser/-/error-stack-parser-1.3.18.tgz#e01c9f8c85ca83b610320c62258b0c9026ade0f7"
Expand Down Expand Up @@ -3781,6 +3788,11 @@
resolved "https://registry.yarnpkg.com/@types/stack-utils/-/stack-utils-2.0.1.tgz#20f18294f797f2209b5f65c8e3b5c8e8261d127c"
integrity sha512-Hl219/BT5fLAaz6NDkSuhzasy49dwQS/DSdu4MdggFB8zcXv7vflBI3xp7FEmkmdDkBUI2bPUNeMttp2knYdxw==

"@types/trusted-types@*":
version "2.0.4"
resolved "https://registry.yarnpkg.com/@types/trusted-types/-/trusted-types-2.0.4.tgz#2b38784cd16957d3782e8e2b31c03bc1d13b4d65"
integrity sha512-IDaobHimLQhjwsQ/NMwRVfa/yL7L/wriQPMhw1ZJall0KX6E1oxk29XMDeilW5qTIg5aoiqf5Udy8U/51aNoQQ==

"@types/uuid@^9.0.1":
version "9.0.1"
resolved "https://registry.yarnpkg.com/@types/uuid/-/uuid-9.0.1.tgz#98586dc36aee8dacc98cc396dbca8d0429647aa6"
Expand Down Expand Up @@ -5584,6 +5596,11 @@ domexception@^2.0.1:
dependencies:
webidl-conversions "^5.0.0"

dompurify@^3.0.6:
version "3.0.6"
resolved "https://registry.yarnpkg.com/dompurify/-/dompurify-3.0.6.tgz#925ebd576d54a9531b5d76f0a5bef32548351dae"
integrity sha512-ilkD8YEnnGh1zJ240uJsW7AzE+2qpbOUYjacomn3AvJ6J4JhKGSZ2nh4wUIXPZrEPppaCLx5jFe8T89Rk8tQ7w==

duplexer@~0.1.1:
version "0.1.2"
resolved "https://registry.yarnpkg.com/duplexer/-/duplexer-0.1.2.tgz#3abe43aef3835f8ae077d136ddce0f276b0400e6"
Expand Down
Loading