Skip to content

Fix stored HTML injection in comment markdown (GO-39) - #2944

Closed
corsacca wants to merge 2 commits into
developfrom
fix/comment-rendering-xss-go39
Closed

corsacca wants to merge 2 commits into
developfrom
fix/comment-rendering-xss-go39

Conversation

@corsacca

Copy link
Copy Markdown
Member

Markdown in a saved comment or activity field could introduce HTML attributes when formatComment interpolated unescaped image/link captures into the comment and activity templates. This addresses weekly security scan finding GO-39, originally traced at faa6318de8d27126222630335b2a5d893908ce1c (v1.83.6).

Escape plain text, labels, image alt text, and URL attributes inside the shared formatter, preserving one existing level of WordPress HTML entity encoding. Parse image, mention, and link markdown in one pass and protect generated markup from later formatting replacements. Only HTTP/HTTPS destinations are emitted, while record-relative markdown links still resolve through the site's record URL.

Preserve mentions (including RTL names), emphasis, lists, external links, internal record links, images with HTTP/HTTPS URLs, and automatic links. Raw HTML now displays literally; unsupported schemes, protocol-relative destinations, and relative image URLs remain escaped markdown. Automatic links also work beside images. Keep raw activity notes unchanged across renders and let the shared formatter escape grouped activity comments exactly once, after date substitution.

Validation:

  • npm run test:js: 13 passing regression tests, including both activity paths, repeated rendering, quote injection, unsafe schemes, entities, and formatting compatibility. The suite fails on unpatched develop.
  • Offline Chromium DOM parsing: 351 representative/adversarial inputs produced no unexpected tags, attributes, unresolved placeholders, or non-HTTP/HTTPS destinations. This check used an inert template, without a live WordPress session.
  • php -l: all 973 tracked PHP/inc/lib files passed.
  • vendor/bin/phpcs: passed.
  • ESLint, Prettier, Node syntax checks for the changed JavaScript, and git diff --check: passed.
  • Added the JavaScript helper tests to CI. No generated bundle update is needed: these scripts are enqueued directly.

Live verification: after the user supplied the correct local URL, tested the patch through an authenticated browser session on https://multisite.lwp. A disposable contact exercised saved comments, text-field activity, HTTP/HTTPS image rendering, record and external links with query parameters, multilingual mentions, emphasis, lists, ampersands, and repeated rendering through tab toggles. Harmless quote-injection markers in both comments and text-field activity did not execute their page-title change; a JavaScript-scheme markdown link remained literal text. This check caught and corrected double encoding of WordPress's existing & entities. The test contact was renamed as completed and archived, and the three local source files were restored after testing; the PR remains isolated in its worktree.

The original finding was a static trace while the local site was unavailable. A separate Multiplier-to-admin session reproduction, grouped activity log/personal highlights UI verification, and privilege-changing REST actions were not performed. The automated suite covers both activity rendering paths and repeat rendering. Review the intentional raw-HTML and URL-scheme restrictions above before merging.

Based on develop, verified as the repository's default branch with gh repo view.

@corsacca corsacca closed this Oct 23, 2026
@corsacca
corsacca deleted the fix/comment-rendering-xss-go39 branch October 23, 2026 15:44
@github-actions

Copy link
Copy Markdown

Review

High

None found.

Medium

Comment edit flow regresses for <, > and other entities (dt-assets/js/comments.js, the .open-edit-comment handler at ~line 327, plus the removed block at ~line 611)

The PR removes the line that set comment.comment_content to $("<div>").text(comment.comment_content).text(). That line also decoded all HTML entities in the stored comment_content (&lt;, &gt;, &#8217;, and so on) as a side effect. The edit handler reads comment.comment_content and passes it through unescapeHtml(), which only handles &amp;, &quot;, &#39; and &#039;. Its &lt; and &gt; replacements are commented out because the earlier decode covered them.

Now comment_content keeps its raw entities, so editing a comment that contains <, > or numeric entities fills the textarea with literal &lt; or &gt;. Saving re-encodes those to &amp;lt;, so the comment is double-encoded and corrupted.

Trigger: post a comment containing a < b, click edit, save. Rendering is fine. The edit textarea and the saved value are wrong.

Fix: decode the stored value for the textarea, for example via a detached element, or extend unescapeHtml to handle &lt;, &gt; and numeric entities. Add a test for the edit path.

Notes (no action needed)

  • The placeholder scheme in formatComment looks sound. NULs are stripped from the input, and the list-handling early return is unreachable.
  • The activity-log.js and comments.js rendering changes escape exactly once.

Summary: The XSS hardening looks solid and well tested, but the edit-comment regression should be fixed before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant