Knowledge Articles Could Run Scripts

Security: Knowledge articles could run scripts

Fixed in 3.15.1 · Reported privately by a security researcher, who asked not to be named · Present in every release up to and including 3.15.0 · Modules: Knowledge

---

The short version

A Knowledge article is HTML, written in the editor. The editor cleans what you type — but only the editor does. FreeITSM stored an article's body exactly as it was sent, and the Knowledge page put it straight into the page.

So an analyst who sent an article without using the editor — straight to the save endpoint, or through a REST API key — could save an article containing script, and that script ran in the session of every analyst who opened the article, administrators included. It ran again when the article was previewed in the recycle bin, and when somebody exported it to PDF.

Who could do it: an analyst with the Knowledge module, or the holder of a REST API key that can write articles. Nobody outside the organisation — a requester, a stranger — could reach it. What they gained was the reach of whoever opened the article: anything that analyst could do in FreeITSM, the script could do as them.

Upgrading is the fix. Articles already saved are cleaned as they are shown, so nothing needs editing or re-saving.

What was wrong

Stored as sent

includes/services/knowledge.php says it plainly in its header:

Bodies are TinyMCE HTML stored verbatim (no sanitisation exists anywhere).

and the save does exactly that:

$newBody = array_key_exists('body_html', $in) ? (string)$in['body_html'] : (string)$current['body'];

That was a reasonable-looking choice. TinyMCE — the editor on the Knowledge page — strips event handlers and scripts as you edit, so every article written in the editor arrives clean. But the cleaning happens in the browser of the person writing, which is the one place a writer controls. api/knowledge/knowledge_save.php needs nothing more than the Knowledge module, and it accepts whatever body arrives. A body posted to it directly, or sent through POST /api/v1/knowledge/articles, never met the editor at all.

Shown as stored

assets/js/knowledge.js then put the body into the page, unchanged, in three places:

// the article view (renderArticleDetail)
        <div class="article-content-body">
            ${currentArticle.body}
        </div>

// the recycle bin's preview (viewArchivedArticle)
document.getElementById('archivedArticleBody').innerHTML = article.body;

// the PDF export (buildArticlePdf)
temp.innerHTML = currentArticle.body || '';

Assigning HTML through innerHTML does not run a <script> tag — which is why this kind of code looks safe. But it does run inline event handlers. <img src=x onerror="..."> fires the moment it is inserted, in every browser.

The third one is the one worth remembering. The PDF export builds a <div> that is never added to the page — it is only read back for its text. It looks harmless, but a detached element still loads its images, so an onerror in it still fires. "It is not on the screen" is not "it does not run".

The fix

One cleaner, the shared one

FreeITSM already has a cleaner for HTML it did not write: safeHtmlFragment() in assets/js/safe-html.js. The inbox uses it for email bodies, and the portal Help Centre already used it for these same articles. It parses the HTML in an inert document, removes the tags that execute or navigate, every attribute beginning on, srcdoc and formaction, and javascript: and data:text/html URLs.

knowledge.js gains one function, and all three places call it:

function kbSafeBody(html) {
    return typeof safeHtmlFragment === 'function'
        ? safeHtmlFragment(html || '', { allowEmbeds: true })
        : escapeHtml(html || '');
}

            ${kbSafeBody(currentArticle.body)}
document.getElementById('archivedArticleBody').innerHTML = kbSafeBody(article.body);
temp.innerHTML = kbSafeBody(currentArticle.body);   // a detached element still fires onerror

🔑 It fails closed. If safe-html.js did not load, the article is shown as escaped text — ugly, readable, inert — never as raw HTML. A missing security control must not mean "render it as it is".

knowledge/index.php did not load safe-html.js before, and now does, ahead of knowledge.js.

Videos: the one thing the cleaner had to learn

The shared cleaner removes every <iframe>, which is right for email. But the Knowledge editor's Media button embeds a video as an <iframe>, and articles have them. Removing them would have broken real articles to fix a hole in fake ones.

So safeHtmlFragment() gains an opt-in, allowEmbeds, which only Knowledge passes. With it on, an <iframe> is not kept — it is rebuilt from nothing:

var embeds = [];
if (opts.allowEmbeds) {
    doc.querySelectorAll('iframe').forEach(function (el) {
        var src = (el.getAttribute('src') || '').trim();
        if (!/^https:\/\//i.test(src)) { el.remove(); return; }
        var safe = doc.createElement('iframe');
        safe.setAttribute('src', src);
        ['width', 'height', 'title'].forEach(function (a) {
            var v = el.getAttribute(a);
            if (v !== null && /^[\w .%-]{0,80}$/.test(v)) safe.setAttribute(a, v);
        });
        safe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-presentation allow-popups');
        safe.setAttribute('referrerpolicy', 'strict-origin-when-cross-origin');
        safe.setAttribute('allowfullscreen', '');
        el.replaceWith(safe);
        embeds.push(safe);
    });
}

Only an https address, a size and a title cross over; nothing else of the original does, so it cannot bring a handler, a srcdoc or a javascript: address with it. The sandbox leaves out top-level navigation and forms, so a framed page cannot take over the one around it.

🔑 But never an embed of FreeITSM's own address. A sandbox that allows both allow-scripts and allow-same-origin only holds for another site: a same-origin frame can simply remove its own sandbox attribute and reach the page around it. A video embed is always somebody else's origin, so the cleaner refuses any frame whose address is this site's:

if (new URL(src).origin === window.location.origin) { el.remove(); return; }

This was caught in review of the first version of the fix, which kept any https frame. Every other caller of safeHtmlFragment() passes no option and is unchanged — no iframe survives for them.

The wrong turn: a marker attribute

The rebuilt iframes then have to get past the next step, which removes every <iframe>. The cleaner has to know which ones it made.

The first draft of this fix did that the obvious way: it put a marker attribute on each rebuilt iframe, and the removal step skipped anything carrying the marker. It reads well, and it is a hole. A marker attribute is something the untrusted HTML can simply include. An article that arrived with <iframe data-safe-embed="1" ...> already in it would have been waved through as one of ours.

The fix that shipped remembers the rebuilt iframes by reference — the embeds array above — and the removal step asks whether an element is one of those exact objects:

doc.querySelectorAll('script, style, link, base, meta, iframe, frame, frameset, object, embed, form')
   .forEach(function (el) { if (embeds.indexOf(el) === -1) el.remove(); });

The HTML being cleaned can write any attribute it likes. It cannot put an element into a JavaScript array. The rule is in the code as a TRAP: comment, and the test below sends a forged marker on purpose.

Not done: cleaning on save, and why

The other half of a full fix would be cleaning the body on the way in, so the database never holds the payload at all. That was considered and not done, for two reasons:

• It would rewrite stored articles — every existing article would change the next time it was saved, which is not something a security patch should do quietly to a knowledge base.
• The server-side cleaner FreeITSM has, includes/html_sanitise.php, was written for HTML from customers, and is deliberately strict: it does not allow <img> or <iframe>. Knowledge articles legitimately need both.

So today the protection is at display. That covers the three places FreeITSM shows a body to an analyst. It does not change what the REST API returns: GET /api/v1/knowledge/articles/{id} still hands back the body as stored, and the API's own documentation already says consumers render it at their own risk.

📁 Files changed

	File	Change
🟦	assets/js/knowledge.js	kbSafeBody(); the article view, the recycle-bin preview and the PDF export use it
🟩	assets/js/safe-html.js	the opt-in allowEmbeds: a video iframe rebuilt with only an https src, a size and a sandbox, tracked by reference
🟨	knowledge/index.php	loads safe-html.js before knowledge.js; knowledge.js?v=64
🟧	tickets/index.php, tasks/index.php, tasks/calendar/index.php, tasks/timeline/index.php, change-management/index.php, knowledge/assistant/index.php, self-service/includes/footer.php	safe-html.js?v=4, so every page picks up the new file
🟪	tests/security-report-2026-10-part2.php	section 9

Tested

tests/security-report-2026-10-part2.php, section 9:

• No raw body reaches the page. The test reads knowledge.js and fails if innerHTML = article.body, innerHTML = currentArticle.body or ${currentArticle.body} appears anywhere in it, and checks kbSafeBody( is used at all three places (four occurrences, counting the definition).
• The cleaner is loaded first. knowledge/index.php loads safe-html.js before knowledge.js — otherwise every article would fall back to escaped text.
• The cleaner itself, in a real browser. With Chrome installed, the test runs safe-html.js in headless Chrome and checks the result:
  • an <img onerror> loses its handler;
  • a YouTube embed survives as an https iframe, rebuilt with the sandbox;
  • a forged marker (data-safe-embed="1") on a frame to another site, a javascript: frame and a srcdoc frame do not get through — of the three bad frames and one good, only the https one survives;
  • without allowEmbeds, no iframe survives at all, so the inbox, the portal and every other caller are unchanged.
  • 🔑 The first assertion is a positive control: that the browser ran the cleaner and returned a result at all. Without it, a browser that failed to start would leave the lines below with nothing to check.

Not tested: saving a real article. Saving can call an AI provider to build the search embedding, so the test proves the display path rather than writing an article.

Do I need to do anything?

Upgrade. Existing articles are cleaned when they are shown, so there is nothing to re-save. If you want to look for an article that was saved with something it should not have, search article bodies in the database for onerror or onload — the editor strips event handlers, so an article written in it should not contain either.

For developers

• The editor is not a security boundary. Anything a browser cleans before sending can be sent uncleaned. If HTML is stored as sent, it must be cleaned where it is shown — every place it is shown.
• innerHTML runs event handlers, and so does an element that is never attached. A scratch <div> used to pull text out of HTML is a page, as far as an onerror is concerned. Clean before you parse, or parse with DOMParser, which is inert.
• Never mark trusted output with an attribute in the same document as untrusted input. The input can write the attribute too. Keep the trusted elements by reference.
• A shared cleaner gets an opt-in, not a fork. Knowledge needed one thing the inbox must not allow; it is an option only Knowledge passes, and a test proves everyone else still gets the old behaviour.
• A fallback fails closed. typeof safeHtmlFragment === 'function' ? clean : escape — never : html.

Related: Knowledge writes ignored folder and article permissions · Escaped text could leave its attribute · A link could change data · Bugs resolved