Change Text Could Run Script

Text in a change could run script

Fixed in 3.8.0, finished in 3.8.1 (see the places 3.8.0 missed). Present in every release from 1.0.0 to 3.7.0. Found while building the change text history for #180, not reported by anyone.

What you saw (or, more likely, didn't)

Nothing. A change's description, reason, risk evaluation, test plan, rollback plan and post-implementation review are written in the rich-text editor, and they looked exactly as written. But an analyst with Change Management could save script into one of them - not by typing it in the editor, but by sending the text straight to the save request or through an API key - and it ran in the browser of anybody who opened that change, or exported it to PDF, as that person. An administrator opening it would run it with an administrator's access.

What was wrong

The text is stored as the editor sent it. That is normal: the editor's HTML is what makes bold, lists and tables work, so it cannot simply be escaped. The defence has to be at the other end - when the text is drawn on screen, it has to be cleaned: the formatting kept, anything that can run (onerror=, javascript: links, <script>) removed.

FreeITSM has had a cleaner for exactly this since the 2026-08 security review, assets/js/safe-html.js, used by tickets, knowledge and the portal. Change Management never adopted it. Both places that show the text inserted it as stored:

// assets/js/change-management.js - renderDetailSection(), before 3.8.0
<div class="detail-section-body">${content}</div>

// buildPdfContent(), before 3.8.0
<div style="line-height: 1.6;">${section.content}</div>

The status, type, priority and people's names beside them were inserted unescaped too.

The fix

One function, both places	safeChangeHtml() in assets/js/change-management.js passes the text through safeHtmlFragment(), and both the detail view and the PDF export call it.
It fails closed	If safe-html.js did not load, the text is shown as plain, inert text rather than as stored - an ugly page, never a dangerous one.
The names beside it	Status, type, priority and the requester, assignee and approver names are escaped in the detail view and the PDF.
The new screens	Everything 3.8.0 added that shows change text cleans it as well: the text-history viewer (change-revisions.js, through safeHtmlFragment()) and the evidence export (evidence.php, through the server-side sanitiseUserHtml()).

📁 Files: assets/js/change-management.js · change-management/index.php (loads safe-html.js before the module script) · tests/security-findings/run.php

🔑 TRAP: change text is untrusted. Anybody with Change Management, or an API key acting as them, can write it. Every place it reaches innerHTML must go through safeChangeHtml() - the comment on the function says so, and the security suite checks it.

The places 3.8.0 missed

Writing this page meant listing every place Change Management puts a stored value into the page - and that list was longer than the fix. 3.8.0 escaped the status, type and priority in the detail view and the PDF, but the same names went in raw in three more places:

// assets/js/change-management.js - the change list, before 3.8.1
<span class="status-badge ${statusClass}">${c.status}</span>

// the detail view's impact, before 3.8.1
<span class="detail-meta-value">${c.impact}</span>

// assets/js/change-calendar.js - the status filter, before 3.8.1
onchange="toggleStatus('${s.name}')"

These names are set in Change Management → Settings (Statuses, Types, Priorities, Impact), so only somebody allowed to change those settings could use them - a much smaller group than everybody with Change Management, but often not just administrators, now that Roles can hand out a settings tab. A status named with an <img onerror> would have run for everybody who opened the change list.

The calendar one is worth a second look, because escaping alone would not have fixed it. The name sat inside a JavaScript string inside an HTML attribute, two languages deep. HTML-escaping makes it safe for the attribute, and the browser then un-escapes it before the JavaScript runs - so a name containing ' still broke out of the string. (That is also why a status called Won't fix quietly broke the filter.) The fix stops putting data into the code at all:

data-status="${escapeHtml(s.name)}" onchange="toggleStatus(this.dataset.status)"

The handler reads the name back from the attribute as plain data, so nothing in it is ever parsed as code.

🔑 The general rule: never build a JavaScript call out of data inside an on...="" attribute. Put the data in a data- attribute and read it back with this.dataset.

Fixed in 3.8.1. The approvals page and the table view already escaped these names.

How it was proved

• In a browser: a temporary change was saved with an <img onerror> and a javascript: link in its text and opened in headless Chrome. With the fix, the image and link rendered without the handler or the link target. The change was deleted afterwards.
• In the security suite: check F11 in tests/security-findings/run.php fails if either place stops using safeChangeHtml(), if the page stops loading safe-html.js before the module script, or - as its positive control - if the cleaner itself stops dropping on* attributes.

What this means for you

Upgrade to 3.8.0. Nothing else to do: the fix is in how text is shown, so text saved before the upgrade is cleaned too. If you want to check whether anybody tried this, search your changes for onerror, onload or javascript: in their text - with 3.8.0 the text history keeps every version from the upgrade onwards.

Related: Change Management · Security · Bugs resolved