Email Template Escaping Followed The Data - edmozley/freeitsm GitHub Wiki
Every automated email FreeITSM sends is a template with [merge_codes] in it. Whether that template is treated as HTML โ and therefore whether its contents are escaped โ was decided by looking at the finished email. Since merge codes are substituted first, a piece of data could make that decision.
Found while adding the Note shared with requester trigger for issue #103. It affected all four triggers that already existed.
Fixed in 66885702, released as update #1203.
An administrator writes a plain-text template:
Hi [requester_first_name],
[note_text]
Thanks.
No HTML. Plain words. FreeITSM's job is to escape it and turn the line breaks into <br> before sending.
Now an analyst writes a note reading:
use the
<table>in room 2
The email that goes out is not escaped at all. Not the note, and not the rest of the template either.
Two steps, in this order:
// includes/template_email.php โ sendTemplateEmail()
$body = resolveMergeCodes($template['body_template'], $mergeData); // 1. substitute
โฆ
$fullBody = buildTemplateEmailBody($body, $ticketNumber); // 2. assembleAnd step two asked the question by looking at what it was handed:
// before
if (strip_tags($bodyContent) === $bodyContent) {
$bodyContent = nl2br(htmlspecialchars($bodyContent, ENT_QUOTES, 'UTF-8'));
}"If this body contains no tags, it is plain text, so escape it."
Perfectly reasonable โ except that by the time it runs, the merge codes are already in. So:
template : "Hi,\n[note_text]" โ genuinely plain text
data : "use the <table> in room 2"
merged : "Hi,\nuse the <table> in room 2" โ now contains a tag
verdict : "this is HTML"
result : nothing is escaped
A value decided the rule that was supposed to govern it.
The existing merge codes make it very unlikely. [ticket_reference], [requester_name], [created_date], [ticket_url] โ short, structured, drawn from database columns that do not normally contain angle brackets.
[note_text] is different in kind: free-form, multi-line prose typed by an analyst who has no idea it is about to be merged into an email template. It is the first merge code likely to contain a < at all, which is how the flaw surfaced the moment one was added.
Whether a template is HTML is a property of what the administrator wrote, not of today's data. So the question is asked of the template, before anything is merged:
$bodyIsHtml = strip_tags($template['body_template']) !== $template['body_template'];and the answer is carried forward rather than re-derived:
$fullBody = buildTemplateEmailBody($body, $ticketNumber, $bodyIsHtml);buildTemplateEmailBody() keeps the old sniff behind a null default, because one caller โ the workflow engine โ has no template to inspect.
Caller-supplied merge data is free-form text, so it is escaped when it is about to land inside HTML:
if ($bodyIsHtml) {
foreach ($extraMergeData as $k => $v) {
$mergeData[$k] = nl2br(htmlspecialchars((string)$v, ENT_QUOTES, 'UTF-8'));
}
}Only when the template is HTML. For a plain-text template the whole body is escaped afterwards, and escaping first as well would show the customer literal <br> and &lt; โ a double-escaping bug in place of the original one.
Values built from the ticket are left as they are. They render correctly today, and widening the change to them was not this fix's job.
| File | |
|---|---|
includes/template_email.php |
the verdict moved to the template; buildTemplateEmailBody() takes it as an argument |
The failing case is demonstrated directly rather than described โ the same merged body, judged both ways:
sniffing the merged body escapes it : NO - unescaped
telling it the template was plain : yes - PASS
And a value chosen to fight back โ Use the <table> in room 2\nLine two & "quoted", carrying a tag, an ampersand, a quote and a newline โ checked in both directions:
| HTML template โ the tag is neutralised | PASS |
HTML template โ the newline survives as <br>
|
PASS |
HTML template โ no live <table> reaches the mail |
PASS |
| plain template โ escaped exactly once, not twice | PASS |
That last row is the one worth keeping. It is the check that catches an over-eager fix.
Nothing to do. Templates behave the way they always appeared to.
If you write plain-text templates, they now stay plain text regardless of what turns up in a ticket. If you write HTML templates, a merge value can no longer break your layout โ or arrive as markup you did not write.
- Email template sender rules
- Ticket notes โ developer guide โ ยง5 covers this rule for anyone adding a trigger
- "Share with Requester" did less than it said
- Bugs resolved