Email: Mark HTML comments as "safe" in email templates #64546
Conversation
This changes add an additional "safeHTML" function to the email templates to allow comment blocks to be preserved after compiling the template with Go's html/template package. MJML outputs comments that are specific for MS Outlook and if those are not preserved the email is rendered incorrectly.
gillesdemey
added
type/bug
pr/needs-manual-testing
gillesdemey
requested review from
tskarhed,
JoaoSilvaGrafana,
ashharrison90,
papagian,
zserge and
mildwonkey
and removed request for
a team
emails/grunt/replace.js
Outdated
| * to work with MS Outlook on the Desktop. | ||
| */ | ||
| const HTML_SAFE_FUNC = 'safeHTML'; | ||
| const commentBlock = /(\<\!\-\-(?:.|\n|\r)*?-->)/g; |
There was a problem hiding this comment.
This regex might be a bit too naive as it will try to match <!-- foo --> but comments might still be rendered when there's some other character in the closing HTML comment. One possible character might be !, so <!-- foo --!> might still be a valid comment, but I'm uncertain if that's an issue in this case.
There was a problem hiding this comment.
The only thing that outputs HTML in the compilation pipeline for now is MJML, so as long as it outputs semantically correct HTML comments (which I believe it does) this shoudn't be a concern.
There was a problem hiding this comment.
I'll still update it to a simpler regex, /(<!--[\s\S]*?-->)/g should work too and be less greedy (it doesn't match <!-- foo --!>)
|
This pull request was removed from the 9.4.5 milestone because 9.4.5 is currently being released. |
|
@gillesdemey Following our security assessment with @KristianGrafana, it's OK to ignore the Yet to avoid in the future dangerous usage of |
adds gosec silence
|
Changes made to this PR after meeting with @jmatosgrafana and @KristianGrafana
|
| @@ -1,5 +1,4 @@ | |||
| default: | |||
| - 'clean' | |||
There was a problem hiding this comment.
This is now handled by make clean since we need to clean the folder before grunt is even invoked
| npx mjml \ | ||
| --config.beautify true \ | ||
| --config.minify false \ | ||
| --config.validationLevel=strict \ | ||
| --config.keepComments=false \ | ||
| ./templates/*.mjml --output ../public/emails/ | ||
| ./templates/*.mjml --output ./dist/ |
There was a problem hiding this comment.
Moving the templates here first since we'll need to do some additional processing before we move them to ../public/emails
|
This pull request was removed from the 9.4.6 milestone because 9.4.6 is currently being released. |
| "HiddenSubject": hiddenSubjectTemplateFunc, | ||
| "Subject": subjectTemplateFunc, | ||
| "HiddenSubject": hiddenSubjectTemplateFunc, | ||
| "__dangerouslyInjectHTML": __dangerouslyInjectHTML, |
There was a problem hiding this comment.
I am not sure I like this name. In Prometheus they use safeHtml for the same purpose. However, I think this name is more explicit about the purpose of the function (__ mean that it's internal, we already use this approach in alerting, and dangerously makes it clear that if you use it somewhere outside the intended workaround for HTML comments). So, I am leaning towards keeping it.
|
I tested the PR in windows. The change LGTM. |
This comment was marked as resolved.
This comment was marked as resolved.
grafanabot
added
the
backport-failed
(cherry picked from commit ed82f96)
What is this feature?
This change adds an additional "safeHTML" function to the email templates to allow comment blocks to be preserved after compiling the template with Go's html/template package.
Why do we need this feature?
MJML outputs comments that are specific for MS Outlook and if those are not preserved the email is rendered incorrectly.
Which issue(s) does this PR fix?:
Fixes #63996
Special notes for your reviewer:
There's a security implication here in case the author of the email templates is untrusted.
I still have to figure out how to port this to
grafana-enterpriseI've tested this with an Outlook for Desktop client I had access to from a friend of a friend and they said it looked correct 馃槅