-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added text editor and implemented it in some cases (#122) #181
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for stuyepsilon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
I am not willing to approve this PR just yet, as there is no back-end sanitization of code (so someone could inject evil scripts in an XSS attack)
Further, we should discuss how to format these HTML contents for our plaintext emails, I see two possibilities
- Keeping our emails plaintext, but extracting the plaintext of the HTML (or using Markdown or some other human-friendly syntax in emails) (I prefer this, because it marginally improves our spam rating, and reduces the odds of email formatting errors)
- Using HTML in our emails (increases complexity and chances of being spam-canned, and would need additional effort to look nice)
@@ -184,7 +184,11 @@ const OrgChat = ({ organization_id }: { organization_id: number }) => { | |||
</Typography> | |||
{/* Add a break or any other separator as needed */} | |||
<br /> | |||
{message.content} | |||
<div | |||
dangerouslySetInnerHTML={{ |
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.
Scary!
> | ||
{announcement.content} | ||
</Typography> | ||
dangerouslySetInnerHTML={{ __html: announcement.content }} |
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.
Concerning!
> | ||
{announcement.content} | ||
</Typography> | ||
dangerouslySetInnerHTML={{ __html: announcement.content }} |
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.
𝓯𝓻𝓮𝓪𝓴𝔂
My thoughts: do not implement the text editor for meetings/posts, which are emailed out in plaintext and should remain as such (to reduce complexity), but keep the editor where it is now (organization messages + announcements) where it may be useful to have. However, before this PR is merged, XSS mitigations must be implemented on the backend for organization messages (announcements are not a likely attack vector because it is only writable by trusted administrators) |
If formatting for meetings/posts are desired, the editor should be adapted to emit Markdown, which will be sent in plaintext verbatim, and parsed to display (with a limited subset of, or no support at all for, embedded HTML) |
This doesn't 100% fix #122 but it implements a basic text editor and uses it in the most simplest of cases.
To do: