Add new SafeInnerHTML component #89
Conversation
cwardzala
commented
Jan 6, 2023
cwardzala
commented
- Add WYSIWYG Editor to Forms page
- update WYSIWYG Editor to sanitize HTML
- Add WYSIWYG Editor to Forms page - update WYSIWYG Editor to sanitize HTML
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
nsmedira
left a comment
nsmedira
left a comment
There was a problem hiding this comment.
reviewed and approved with suggestions 2023-01-06 at 14:35 ET
| SafeInnerHTML.propTypes = { | ||
| as: PropTypes.elementType, | ||
| html: PropTypes.string | ||
| }; |
There was a problem hiding this comment.
if we merged in the work on TS in this PR, we could write this in TS from the start and replace propTypes with Props interface
There was a problem hiding this comment.
Yea, seems like that PR is not quite ready for prime time.
There was a problem hiding this comment.
now that the TS PR has been approved, can we refactor this to typescript
| <WysiwygEditor | ||
| label="WYSIWYG Editor" | ||
| html={`<p>Pellentesque habitant <strong style="color: red">morbi tristique senectus</strong> <img src=x onerror=alert('this should never get shown') /> fames ac turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies mi vitae est. Mauris placerat eleifend leo.</p>`} | ||
| /> | ||
| <WysiwygEditor | ||
| label="Read Only WYSIWYG Editor" | ||
| html="<p>Pellentesque habitant <strong>morbi tristique senectus</strong> <img src=x onerror=alert('this should never get shown') /> et netus et malesuada fames ac turpis egestas. Vestibulum tortor quam, feugiat vitae, ultricies eget, tempor sit amet, ante. Donec eu libero sit amet quam egestas semper. Aenean ultricies mi vitae est. Mauris placerat eleifend leo.</p>" | ||
| readOnly | ||
| /> |
There was a problem hiding this comment.
would the intent of this code (i.e. presumably to demonstrate that alert('this should never get shown') is never executed) be better expressed as a jest test?
e.g. pseudo
describe('WysiwygEditor', () => {
test('html prop is sanitized', () => {
render(<WysiwygEditor html={<img src=x onerror=alert('this should never get shown') />)
expect(wysiwygEditorDiv).toHaveTextContent('');
})
})
nsmedira
left a comment
nsmedira
left a comment
There was a problem hiding this comment.
reviewed and approved 2023-01-17 at 11:04 ET
| Forms should be built with <a href="https://jaredpalmer.com/formik/">Formik</a>. | ||
| Forms should be built with{' '} | ||
| <a href="https://jaredpalmer.com/formik/" target="_blank" rel="noopener noreferrer"> | ||
| Formik <FaExternalLinkAlt style={{ fontSize: '75%' }} /> |
There was a problem hiding this comment.
either add title, aria-label, or aria-hidden. As of now the screen reader has no context for this icon.
| <p> | ||
| The HTML content passed into and generated from <code>WysiwygEditor</code> is sanitized with{' '} | ||
| <a href="https://github.com/cure53/DOMPurify" target="_blank" rel="noopener noreferrer"> | ||
| DOMPurify <FaExternalLinkAlt style={{ fontSize: '75%' }} /> |
There was a problem hiding this comment.
class instead of inline style
There was a problem hiding this comment.
either add title, aria-label, or aria-hidden. As of now the screen reader has no context for this icon.
