site-kit-wp: Error establishing database connection message rendering with "" tag
Bug Description
It looks like the description https://github.com/google/site-kit-wp/blob/01e077bfd5a56039942cc010408e257cd006bbc0/assets/js/components/legacy-notifications/cta.js#L51 string comes with an <h1> tag. This is probably taken as a literal string, but even if it wasn’t It probably does not make sense to have an <h1> inside a <p> tag
Steps to reproduce
- Go to SiteKit dashboard
- Trigger a database connection error (I triggered mine by having a slow connection)
- See error
Screenshots
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The text in error messages that we use to show errors (eg https://github.com/google/site-kit-wp/blob/eed009c134ae533e307d65390e3af84d5551a9e7/assets/js/components/ReportError.js#L51) should be stripped of any HTML tags. We’ll never want to render arbitrary HTML from errors, so this location (or someplace else near where we render the error, rather than store it) seems appropriate.
- Error messages or responses that include HTML should render as plain strings, with all of their HTML stripped, in the Site Kit UI.
Implementation Brief
In assets/js/components/ReportError.js:
- import the DOMPurify instance that is exported as
purify from assets/js/util/purify.js.
- before defining the constant
description, sanitize the message variable only when there is no reconnectURL, using the sanitize method of the imported DOMPurify instance and pass the following as options:
ALLOWED_TAGS: Empty array.
- Make sure that the sanitized version of the message is used in the
description.
Test Coverage
- Not needed.
QA Brief
- Ensure tests for ReportError pass.
- Check new stories (ReportError with HTML tags) for ReportError and ensure no HTML tags are displayed.
Changelog entry
- Remove HTML tags from report errors.
Bug Description
It looks like the description https://github.com/google/site-kit-wp/blob/01e077bfd5a56039942cc010408e257cd006bbc0/assets/js/components/legacy-notifications/cta.js#L51 string comes with an <h1> tag. This is probably taken as a literal string, but even if it wasn’t It probably does not make sense to have an <h1> inside a <p> tag
Steps to reproduce
- Go to SiteKit dashboard
- Trigger a database connection error (I triggered mine by having a slow connection)
- See error
Screenshots
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The text in error messages that we use to show errors (eg https://github.com/google/site-kit-wp/blob/eed009c134ae533e307d65390e3af84d5551a9e7/assets/js/components/ReportError.js#L51) should be stripped of any HTML tags. We’ll never want to render arbitrary HTML from errors, so this location (or someplace else near where we render the error, rather than store it) seems appropriate.
- Error messages or responses that include HTML should render as plain strings, with all of their HTML stripped, in the Site Kit UI.
Implementation Brief
In assets/js/components/ReportError.js:
- import the DOMPurify instance that is exported as
purifyfromassets/js/util/purify.js. - before defining the constant
description, sanitize themessagevariable only when there is noreconnectURL, using thesanitizemethod of the importedDOMPurifyinstance and pass the following as options:ALLOWED_TAGS: Empty array.
- Make sure that the sanitized version of the message is used in the
description.
Test Coverage
- Not needed.
QA Brief
- Ensure tests for ReportError pass.
- Check new stories (ReportError with HTML tags) for ReportError and ensure no HTML tags are displayed.
Changelog entry
- Remove HTML tags from report errors.
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 20
Thanks for jumping in @eugene-manuilov . That’s correct, I intentionally added the tests even though it was not mentioned in the IB just to make sure that HTML tags are stripped. So the tests passing only should be fine. And you’re correct @wpdarren , that’s the only story to look at.
Actually, I think I can help too 😄. @wpdarren it means that all js tests should pass which has already been confirmed during code review.
@asvinb could you please, help, Darren with his question?
@eugene-manuilov The ErrorText component already have html sanitization. I have updated the IB to only clean the message when there is no
reconnectURL. Should be good now.More specifically, it’s likely that the message in https://github.com/google/site-kit-wp/blob/eed009c134ae533e307d65390e3af84d5551a9e7/assets/js/components/ReportError.js#L51 is what’s causing this issue.
In summation: the error message sent from WordPress (specifically the WP REST API) here contains an error page with
<h1>Some database error</h1>. That’s the message our code is receiving and displaying—the literal“<h1>”string appears because React (helpfully) escapes HTML in strings by default. But we should strip HTML tags entirely from error message strings to avoid things like this happening in the future.I’ve added as much to the ACs. We should be able to use DOMPurify (our instance is in https://github.com/google/site-kit-wp/blob/eed009c134ae533e307d65390e3af84d5551a9e7/assets/js/util/purify.js#L6) to sanitise HTML strings to regular strings.