-
Notifications
You must be signed in to change notification settings - Fork 134
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
Conversation
Size Change: +22.4 kB (+3%) Total Size: 748 kB
ℹ️ View Unchanged
|
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], |
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.
@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.
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.
Actually, the resolve()
configuration options for other things doesn't appear to make a difference, do you remember what it was for?
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.
No idea 🙈
src/extensions/surveys.ts
Outdated
@@ -8,6 +8,8 @@ import { | |||
SurveyAppearance, | |||
SurveyQuestion, | |||
} from '../posthog-surveys-types' | |||
import DOMPurify from 'dompurify' | |||
// import { sanitize } from 'dompurify' |
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.
commented out code
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.
nice thanks
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.
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], |
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.
No idea 🙈
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) |
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. |
Changes
Part 1 of 2 of custom HTML support in surveys. This ensures parsed HTML is safe and valid to use
...
Checklist