fix(security): patch two XSS sinks, and stabilise the document types E2E test

[erseco]

Two independent fixes, one commit each.

1. Cross-site scripting

The reported one — documentate-admin.js

getRichEditorText() assigned the editor's raw value to innerHTML on a detached div, purely to strip tags and read the text back:

var tmp = document.createElement('div');
tmp.innerHTML = textarea.value;
return (tmp.textContent || tmp.innerText || '').trim();

Being detached does not make this inert — the node still belongs to the active document, with scripting enabled, so the onerror handler of <img src=x onerror=...> saved in a document fires the moment an editor triggers form validation. That is a stored XSS reachable by any contributor who can save content, and it is the CodeQL js/xss-through-dom alert.

Replaced with DOMParser, extracted into extractPlainText():

var parsed = new DOMParser().parseFromString(String(html), 'text/html');
return (parsed.body.textContent || '').trim();

A document from parseFromString has no browsing context, so scripting is disabled: neither scripts nor inline event handlers run. That is precisely the difference from innerHTML on a detached node, which still belongs to the active document and did fire the onerror handler. The parsed tree is never inserted anywhere; only body.textContent is read.

(Note the guarantee is scoped deliberately: the spec disables scripting, it does not promise that no subresource is ever fetched. The fix rests on handlers not executing.)

CodeQL flags this line, and that flag is a false positive — dismissed as such on alert #521. The rule fires on any HTML parse of untrusted text without distinguishing the inert case. Two alternatives were tried and rejected:

• A tag-stripping regex. It removed the sink but broke correctness. <p>&#160;</p>, <p>&#xA0;</p> and <p title="1 > 0"></p> all came back non-empty, so a visually empty field would pass a required check. Real parsing is the only thing that decodes entities and understands > inside an attribute value.
• editor.getContent({ format: 'text' }). Asking TinyMCE for its own text looked cleanest and broke saving: getContent() fires TinyMCE's SaveContent handlers, and WordPress's integration writes that result back into the textarea, overwriting the HTML save() had just put there. Documents saved empty. Caught by two rich-content E2E specs.

Correctness wins over silencing the linter.

A second one, not reported — documentate-collaborative-editor.js

Found while auditing the other innerHTML sites. CodeQL does not flag it, and it is the more serious of the two:

avatarEl.innerHTML = `<img src="${user.avatar}" alt="${user.name}" />`;

user comes from awareness.getStates() — Yjs state that every peer in the room publishes about itself via the signaling server. The default signaling server is wss://signaling.yjs.dev, which is public, so the room is not private either. A peer whose display name is "><img src=x onerror=...> breaks out of the quoted attribute and runs code in every other editor's browser.

Fixed by building the element and assigning src and alt as properties.

Checked and left alone

• escapeHtml() in documentate-revisions.js and documentate-actions.js — the safe idiom (textContent in, innerHTML out).
• createToolbarHTML() — interpolates only an internal DOM id.

Regression test

tests/js/documentate-admin-rich-text.test.js pins the emptiness contract this validation depends on — 19 cases covering exactly what the regex attempt got wrong:

Input	Expected
<p>&nbsp;</p>, <p>&#160;</p>, <p>&#xA0;</p>	"" — empty
<p title="1 > 0"></p>	"" — empty
<!-- nota interna -->	"" — empty
<img src=x onerror=alert(1)>	"", and never executed
<p>&amp;</p>	"&" — decoded
<p title="1 > 0">Texto</p>	"Texto"

The tests were verified to catch the bug: swapping the regex implementation back in makes 6 of them fail.

extractPlainText is exported under CommonJS so the tests can reach it, and the IIFE argument tolerates a missing jQuery. WordPress serves this file as a plain script where module is undefined, so neither branch is taken in the browser and runtime behaviour is unchanged.

2. The flaky document-types E2E test

document-types.spec.js "can create new document type with name" failed intermittently, and reliably once enough test data existed.

Root cause. The test creates Test Type <timestamp> and never removes it, while the assertion looked for the new row on the unfiltered first page of a list WordPress paginates at 20. Every run leaks one term; after enough runs the new term lands on page 2. The environment where this was investigated had 35 document types, 18 of them left over from earlier runs. That also explains why CI, which starts from a fresh site, mostly passed.

Fixed by reloading the list filtered by name, so the assertion no longer depends on where the term falls or how many exist.

A second, genuine race in the same page object, the other failure mode observed:

await this.submitButton.click();
await this.page.waitForResponse( ... );   // registered after the click

waitForResponse only observes responses arriving after it is called, so a fast server answer was missed and the call timed out. The listener is now registered first, then the click, then the await — plus a wait for the row the AJAX handler appends.

Verification

The jest suite previously ran only locally: lint_and_test installed npm dependencies but went straight to PHPUnit. npm run test:unit-js now runs in CI, before wp-env starts so a JS regression fails in seconds instead of after the container boot.

npm run test:unit-js   ✓ 28 tests (19 new), now enforced in CI
make test-e2e          ✓ 72 passed, 21 skipped, 1 pre-existing failure
affected test, --repeat-each=5, 26 terms present   ✓ 5 passed (previously 5 failed)

Two E2E failures seen, neither from this PR

Both were attributed by measurement, not assumption:

• document-rich-formatting.spec.js fails ~2 of 3 runs. Swapping in main's version of both changed JS files reproduces the same 2-of-3 rate, so it is pre-existing. Cause: the spec clicks the Text tab and fills the textarea without waiting for TinyMCE to complete the asynchronous switch, so fill() races a hidden element. A fix was attempted here and reverted — driving the switch through switchEditors made it worse (5 of 5 failing), and hardening an unrelated spec does not belong in this change. Worth its own PR.
• document-revisions.spec.js:74 failed once under full-suite load, passed on a second full run, and passes 12/12 in isolation. Non-deterministic under load; those tests take 38–60s each.

Expect the rich-formatting spec to go red intermittently in CI regardless of this PR.

Coverage gap

The required-field validation this function guards has no direct E2E coverage — no spec exercises it end to end. The new unit tests cover the extraction contract, and the rich-content save specs caught the getContent() attempt indirectly, but an E2E test for the validation flow itself would be worth adding.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!