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

feat(surveys): Add question html support #17847

Merged
merged 4 commits into from
Oct 10, 2023
Merged

feat(surveys): Add question html support #17847

merged 4 commits into from
Oct 10, 2023

Conversation

neilkakkar
Copy link
Collaborator

Problem

Fixes #17506

Depends on PostHog/posthog-js#824

image image

We sanitize inputs

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar neilkakkar marked this pull request as ready for review October 6, 2023 13:51
@@ -496,6 +496,7 @@ def hashed_identifier(self, feature_flag: FeatureFlag) -> Optional[str]:
return self.hash_key_overrides[feature_flag.key]
return self.distinct_id
else:
# TODO: Don't use the cache if self.groups is empty, since that means no groups provided anyway
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a part of the PR? :o

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops, no, but was meant to go in anyway 😅 - notes for fixing a few decide issues, whenever I get to them.

@liyiy
Copy link
Contributor

liyiy commented Oct 9, 2023

I'm going to test this locally and then merge if it's all good

@liyiy
Copy link
Contributor

liyiy commented Oct 10, 2023

I feel like we should actually prevent the user from creating the survey if they have script in their description HTML. Removing them on save works too but since we're going to great lengths to prevent it anyway, maybe we should be more direct by just form erroring them entirely?

Anyway not really a blocker and everything else is working, so I'll leave it to you to decide!

@neilkakkar
Copy link
Collaborator Author

The tricky bit is that there are several ways to get a script inside:

DOMPurify.sanitize('<img src=x onerror=alert(1)//>'); // becomes <img src="x">
DOMPurify.sanitize('<svg><g/onload=alert(2)//<p>'); // becomes <svg><g></g></svg>
DOMPurify.sanitize('<p>abc<iframe//src=jAva&Tab;script:alert(3)>def</p>'); // becomes <p>abc</p>
DOMPurify.sanitize('<math><mi//xlink:href="data:x,<script>alert(4)</script>">'); // becomes <math><mi></mi></math>
DOMPurify.sanitize('<TABLE><tr><td>HELLO</tr></TABL>'); // becomes <table><tbody><tr><td>HELLO</td></tr></tbody></table>
DOMPurify.sanitize('<UL><li><A HREF=//google.com>click</UL>'); // becomes <ul><li><a href="//google.com">click</a></li></ul>

and there's a second benefit of sanitising which we seriously need: To fix malformed html, per user input error, or close unclosed tags. This doesn't affect the preview much, but would completely break the actual survey if there are malformed tags.

So, we're always doing some adjustments on save. And at this stage instead of making exceptions for a small class of script tags, I felt it better to just go the route of "we will clean up your HTML no matter what it is"

@neilkakkar neilkakkar merged commit b02d800 into master Oct 10, 2023
73 checks passed
@neilkakkar neilkakkar deleted the survey-html branch October 10, 2023 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support markdown in surveys
4 participants