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

Conversation

neilkakkar
Copy link
Collaborator

Changes

Part 1 of 2 of custom HTML support in surveys. This ensures parsed HTML is safe and valid to use
...

Checklist

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Size Change: +22.4 kB (+3%)

Total Size: 748 kB

Filename Size Change
dist/surveys.js 61 kB +22.4 kB (+58%) 🆘
ℹ️ View Unchanged
Filename Size
dist/array.full.js 177 kB
dist/array.js 119 kB
dist/es.js 119 kB
dist/module.js 119 kB
dist/recorder-v2.js 95 kB
dist/recorder.js 58.3 kB

compressed-size-action

@neilkakkar neilkakkar marked this pull request as ready for review October 6, 2023 13:01
@neilkakkar
Copy link
Collaborator Author

the cost of purifying & bundling together is a bigger package 😬

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 🙈

@neilkakkar neilkakkar added the bump minor Bump minor version when this PR gets merged label Oct 6, 2023
@@ -8,6 +8,8 @@ import {
SurveyAppearance,
SurveyQuestion,
} from '../posthog-surveys-types'
import DOMPurify from 'dompurify'
// import { sanitize } from 'dompurify'
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice thanks

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Personally I am not a fan of this. 22kb of extra JS just to make sure the html is valid and pure which could instead be done at write time instead?

Is there a reason why we aren't doing this in app.posthog.com instead?

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

Choose a reason for hiding this comment

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

No idea 🙈

@neilkakkar
Copy link
Collaborator Author

Discussed offline, conclusion is @benjackwhite convinced me that my reluctance to do this validation on the backend is not worth the trade-offs. Bigger package size is annoying to users, plus the security concern of people being able to bypass our API isn't big enough to warrant sanitisation in the frontend. (if that can happen, we have much more serious issues to deal with)

@neilkakkar neilkakkar closed this Oct 9, 2023
@neilkakkar
Copy link
Collaborator Author

And at the same time, the feature to allow scripts / custom HTML rendering via survey API mode is also something we're not going to support. Since it's API mode, they can just add this to the code themselves, rather than parsing these scripts via the survey itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants