feat(rendering): minify rendered HTML behind FEATURE_FLAG_MINIFY_HTML - #36852
feat(rendering): minify rendered HTML behind FEATURE_FLAG_MINIFY_HTML#36852fmontes wants to merge 9 commits into
Conversation
Adds an opt-in HTML minifier that strips insignificant whitespace, line breaks and indentation from rendered pages before they are written to the response. Wired at the two seams that together cover every render path: - VelocityLiveMode.writePage() for LIVE mode, before the page cache write so minification is paid once per cache fill rather than per request - VelocityModeHandler.eval() for preview/edit/admin modes and the REST and GraphQL getPageHtml callers The minifier is conservative by design: pre/textarea/script/style content is copied byte-for-byte, whitespace between inline elements is collapsed rather than removed so words are never joined, and any failure returns the original markup. Refs #36851 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @zJaaal's task in 5m 33s —— View job Code ReviewReviewed New Issues
|
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged, dependency-free HTML minification step to dotCMS’s rendering pipeline so rendered pages can be served (and, in LIVE mode, cached) without indentation/blank lines/comments introduced by Velocity templates—opt-in via FEATURE_FLAG_MINIFY_HTML.
Changes:
- Introduces
HtmlMinifierto conservatively collapse insignificant whitespace and strip HTML comments while preserving<pre>,<textarea>,<script>, and<style>bodies. - Hooks minification into
VelocityLiveMode.writePage()(before page cache write) and intoVelocityModeHandler.eval()(post-CSP processing path). - Adds
FEATURE_FLAG_MINIFY_HTMLand a new unit test suite for the minifier.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dotCMS/src/main/java/com/dotcms/rendering/util/HtmlMinifier.java | New minifier implementation guarded by FEATURE_FLAG_MINIFY_HTML. |
| dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityLiveMode.java | Minifies LIVE mode output before writing/storing into the page cache when enabled. |
| dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityModeHandler.java | Minifies eval() output (after CSP application when configured). |
| dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java | Adds the FEATURE_FLAG_MINIFY_HTML feature flag constant + javadoc. |
| dotCMS/src/test/java/com/dotcms/rendering/util/HtmlMinifierTest.java | Adds unit tests for whitespace significance, preserved regions, comments, and idempotence. |
🐳 PR Docker test imageLatest build for commit docker pull dotcms/dotcms-test:pr-36852-issue-36851-native-html-minification
docker pull dotcms/dotcms-test:pr-36852-issue-36851-native-html-minification_5973036 |
Review of #36852 surfaced three cases where minification changed content rather than just formatting. Each is covered by a test that fails against the previous implementation. * Whitespace inside quoted attribute values was collapsed, because the scan carried no tag or attribute context: `<input value="a b">` became `<input value="a b">`. That silently rewrites submitted form values, JSON data attributes and accessible text. Tags are now copied as a unit by `appendTag()`, which tracks quoting, so attribute values survive byte-for-byte and a `>` inside a quoted value no longer ends the tag early. * A literal `>` in text was read as the end of a tag, so the whitespace after it was judged against whatever tag happened to precede it: `<p>Home > About</p>` became `<p>Home >About</p>`. The tag emitted last is now tracked as the scan proceeds instead of being recovered by scanning the output backwards for `<`, which also removes an O(n) backward scan per whitespace run. A bare `<`, as in `3 < 4`, is likewise treated as text. * `Character.isWhitespace` matches characters HTML renders rather than collapses, including the ideographic space (U+3000) common in CJK copy and the thin space (U+2009), so those were replaced by an ASCII space or dropped. Replaced with `isHtmlWhitespace()`, which matches only the five characters HTML treats as collapsible. Also addressed the non-blocking review notes and a latent crash: `findPreserveTagEnd` no longer wraps a loop that always returned on its first iteration, the unreachable `<![endif]` branch nested under `startsWith("<!--")` is gone, and two `Set.of(...).contains(null)` paths that degenerate markup such as `</>` would have hit are guarded. Behaviour deliberately left alone: the space before a self-closing `/` is kept, since dropping it would append the slash to an unquoted attribute value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tag behaviour #36851 Covers the two review concerns about comments, both of which the current implementation already handles correctly. These tests keep it that way. * Whitespace removal can not forge or destroy a comment boundary. `< !--` is not a comment opener and `-- >` is not a terminator, so joining either would silently delete page content. Whitespace between two pieces of text is always collapsed to a single space rather than removed, which is the structural guarantee behind this. * Tags inside comments are never treated as markup. Comments are resolved before preserved-tag matching, so a commented-out `<pre>` does not open a preserved region. A commented-out `</body>` is removed outright, which means the `lastIndexOf("</body>")` search in `HTMLPageAssetRenderedBuilder.injectUVEScript` can no longer match inside a comment. * A retained downlevel conditional comment keeps its content verbatim, since that content is markup for the browsers that read it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both review findings are addressed in 1. Literal
|
|
Tick the box to add this pull request to the merge queue (same as
|
…real pages #36851 The existing tests assert exact output, so they only cover cases somebody thought to write down. This adds an oracle that asserts an invariant instead: minified markup must be semantically identical to what went in. Anything that changes what a browser would render fails, anticipated or not. `assertIntegrity` parses both sides with jsoup (already a dependency, so no BOM change) and compares: * attribute values, byte-for-byte -- catches whitespace inside a quoted value being collapsed * `script`, `style`, `pre` and `textarea` bodies, byte-for-byte -- catches breaking JavaScript automatic semicolon insertion or rendered output * visible text, whitespace-normalised -- catches words being joined * element structure -- catches markup being restructured or truncated * idempotence -- LIVE mode can minify on write and again through `eval()` Driven by two inputs. Thirty fixtures target specific corruption modes, and a corpus of two real rendered demo pages under `src/test/resources` covers combinations nobody writes by hand: the home page carries an 11KB inline `<style>` block and 630 attribute values, the member page an inline script that depends on ASI for correctness. The oracle has teeth. Against the pre-fix implementation it fails on both the fixtures and the real-page corpus; against the current one all pass. Two further guards: a size floor, so an over-cautious change cannot keep integrity by minifying nothing, and an assertion that the feature flag is off with no configuration present, so the default cannot drift. One deliberate allowance is documented in `visibleText`. A browser renders each `<option>` as a discrete item, so whitespace between options is never painted and removing it is correct, but jsoup has no CSS model and concatenates their text, which reads as joined words. A separator is inserted on both sides of the comparison to restore the boundary. Whitespace within an option's own text is still compared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing #36851 `demo-members.html` was captured while authenticated, so it carried the rendered profile block of the logged-in account: display name, email address and privilege flags. The account was the stock demo admin, so nothing secret was published, but committing authenticated output to a public repository is the wrong pattern -- the next person to refresh the corpus from a real environment would leak a real user. Replaced with `Test User` / `user@example.com`. The fixture is here for its inline script and markup shape, so the identity was never load bearing. Swept both files for the rest: no tokens, API keys, session identifiers, CSP nonces, gravatar hashes (which are hashes of an email address), role or user identifiers, internal hostnames or IP addresses. The one remaining address, `info@dotcms.com` in the home page footer, is the demo starter's public contact. Added a README recording where each file came from, why it earns its place, and the checks to run before adding another. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cation on #36851 The Page collection already asserts a great deal about rendered output. Running it a second time against a server that minifies gives those assertions to the minifier for free, across every render path the collection touches rather than only the paths a bespoke test would think to exercise. It also covers the seams -- CSP ordering, UVE injection, the page cache -- which a unit test on HtmlMinifier cannot reach. * `dotcms-postman/pom.xml` -- new `postman.minify.html` property, defaulted to `false`, wired into the dotCMS container as `DOT_FEATURE_FLAG_MINIFY_HTML`. Every existing suite therefore keeps testing un-minified delivery, unchanged. * `.github/test-matrix.yml` -- one new entry running the same `page` collection with `-Dpostman.minify.html=true`. * `cicd_comp_test-phase.yml` -- the postman branch of the matrix generator now honours `extra_maven_args`, and `stage_name_suffix` so a collection can run twice without the two `build-reports-<stage_name>` artifacts colliding. Verified by replaying the generator over the parsed matrix: 12 postman jobs, 12 unique stage names, and the new entry resolves to `-Dpostman.collections=page -Dpostman.minify.html=true`. The property itself evaluates to `false` by default and `true` when overridden. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Semgrep found 1 Using variable interpolation If this is a critical or high severity finding, please also link this issue in the #security channel in Slack. |
Fixes #36851
Adds an opt-in HTML minifier so rendered pages are served without the indentation, blank lines, and line breaks that VTL templates, containers, and widgets carry for readability.
Off by default — enable with
FEATURE_FLAG_MINIFY_HTML=true.Proposed Changes
HtmlMinifier(new) — dependency-free minifier that strips insignificant whitespace and HTML comments. Deliberately conservative: it does not minify JS/CSS, rewrite attributes, or strip optional end tags.VelocityLiveMode.writePage()— minifies LIVE mode output before the page cache write, so the cost is paid once per cache fill rather than on every request.VelocityModeHandler.eval()— covers preview/edit/admin/navigate modes plus thegetPageHtmlcallers (PageResourceREST andPageRenderDataFetcherGraphQL). This method already post-processes for CSP, so minification follows the established pattern.FeatureFlagName— adds theFEATURE_FLAG_MINIFY_HTMLconstant.HtmlMinifierTest(new) — 19 test methods / 57 assertions covering the whitespace-significance, attribute-value, literal-angle-bracket, Unicode-whitespace, comment-boundary and preserved-region edge cases.Why two seams instead of one filter
There is no single chokepoint for rendered HTML.
VelocityLiveMode.serve()streams directly toresponse.getOutputStream()and writes into the static page cache — it never returns throughgetPageHtml. A servlet filter or a hook inVelocityServletwould therefore have missed the highest-traffic path entirely. The two seams above are the minimum that covers every render path.Safety
The risky part of HTML minification is whitespace that looks removable but is actually rendered. This implementation:
<pre>,<textarea>,<script>, and<style>content byte-for-byte — protects rendered output and JavaScript automatic semicolon insertion.<input value="a b">keeps its spacing. Tags are parsed as a unit with quote tracking, which also means a>inside a quoted value does not end the tag early. Whitespace between attributes is still collapsed to one space.<and>as literal text when they are not part of a tag, so<p>Home > About</p>and<p>3 < 4</p>are left alone.<span>a</span> <span>b</span>keeps its space and words are never joined. Whitespace bordering block elements is removed.U+3000common in CJK copy, the thin spaceU+2009— are left untouched.Character.isWhitespaceis deliberately not used, since it matches those too.<!--[if IE]>).<html>/<body>injection into fragments, no DOCTYPE case changes, no auto-closing of tags, and no dropping of the space before a self-closing/(removing it would append the slash to an unquoted attribute value). This matters for partials, URL-mapped fragments, and non-HTML templates.eval().Checklist
Security notes: minification only removes whitespace and comments; it does not decode, re-encode, or re-escape content, so it cannot introduce XSS by unescaping. Comment stripping removes HTML comments from delivered pages, which slightly reduces incidental information disclosure. Ordering with CSP is preserved — in
eval(),ContentSecurityPolicyUtil.apply()still runs first, so nonce injection is unaffected.Additional Info
Library evaluation. Two candidates were assessed before writing custom code:
prettyPrint(false)preserves whitespace verbatim (no minification at all);prettyPrint(true)re-indents. It also normalizes markup — injecting<html><head></head><body>into every fragment and lowercasing<!DOCTYPE html>— which would break fragment and URL-mapped output.com.googlecode.htmlcompressoris abandoned (last release 2011). The maintained forkcom.github.hazendaz:htmlcompressor:2.0.2is safe and handles preserved regions correctly, but deliberately collapses inter-tag whitespace to a single space rather than removing it, so output still carries a space between every tag. It is also the same library the customer explicitly rejected running as a plugin (see Native, configurable HTML minification in the core rendering engine #36851).Neither delivers full whitespace removal without custom logic layered on top, so a small owned minifier — guarded by tests — was the path chosen. No new dependency, no BOM change.
Scope. HTML whitespace only. Inline JS/CSS minification is intentionally out of scope; it is substantially riskier and should be a separate discussion.
Rollout. Enabling the flag does not retroactively minify already-cached pages — they update as cache entries refill. Flush the page cache to make it immediate.
Testing note.
./mvnw test -pl :dotcms-corecurrently fails in my local environment before reaching any test, on an unresolved${net.bytebuddy:byte-buddy-agent:jar}surefire property. This is pre-existing and unrelated — an untouchedFileUtilTestfails identically. I verified the suite by compiling and running it directly against the module classpath:Worth confirming these run green in CI.
Review round 1
Three cases where minification changed content rather than formatting were found and fixed in
345b9788. Each has a test that fails against the previous implementation (Tests run: 16, Failures: 3before,OK (16 tests)after):<input value="a b">→<input value="a b">claude[bot]>in text was read as the end of a tag, so following whitespace was judged against the preceding tag<p>Home > About</p>→<p>Home >About</p>Character.isWhitespacematched Unicode spaces that HTML renders rather than collapses<p>a b</p>→<p>a b</p>Also addressed the non-blocking notes and a latent crash:
findPreserveTagEndno longer wraps a loop that always returned on its first iteration, the unreachable<![endif]branch nested understartsWith("<!--")is gone, and twoSet.of(...).contains(null)paths that degenerate markup such as</>would have hit are guarded.Tracking the tag emitted last, rather than recovering it by scanning the output backwards for
<, also removes an O(n) backward scan per whitespace run.Comment handling
Two review questions about comments, both checked. The implementation already behaved correctly in each case, so the outcome is three regression tests rather than a code change (
12ec1cab).Can removing whitespace forge or destroy a comment boundary? No.
<!--and-->are whitespace-sensitive tokens, so joining< !--into<!--would turn live markup into a comment and silently delete it, and joining-- >into-->would forge a terminator. Neither can happen: whitespace between two pieces of text is always collapsed to a single space, never removed. Removal only happens where a block-level tag borders the whitespace. A<that is not followed by a tag name,/,!or?counts as text, which is what keeps< !--apart.Are tags inside comments mistaken for real markup? No. Comments are resolved before preserved-tag matching, so a commented-out
<pre>or<script>never opens a preserved region. Relevant to the earlier UVE incident: a commented-out</body>is now removed beforeHTMLPageAssetRenderedBuilder.injectUVEScriptruns itslastIndexOf("</body>"), so that search can no longer match inside a comment. Minification narrows that failure mode rather than widening it.Retained downlevel conditional comments keep their content verbatim, since it is markup for the browsers that read it.
Integrity testing
Exact-output tests only cover cases somebody thought to write down, so
HtmlMinifierIntegrityTestasserts an invariant instead: minified markup must be semantically identical to what went in. It parses both sides with jsoup (already a dependency, no BOM change) and compares attribute values byte-for-byte,script/style/pre/textareabodies byte-for-byte, whitespace-normalised visible text, element structure, and idempotence.Driven by 30 fixtures targeting specific corruption modes plus a corpus of two real rendered demo pages checked in under
src/test/resources. The home page contributes an 11KB inline<style>block and 630 attribute values; the member page an inline script that depends on ASI for correctness.The oracle has teeth: against the pre-fix implementation it fails on both the fixtures and the real-page corpus. Against the current one, all pass. Two further guards: a size floor so an over-cautious change cannot keep integrity by minifying nothing, and an assertion that the flag is off with no configuration present.
Verified against a live environment
Enabled on a dev instance and measured end to end. Both seams fire (LIVE servlet and REST
eval()), andEDIT_MODE/PREVIEW_MODErender with the UVE script correctly placed before</body>./index/members/indexNo content changed: 815 attribute values across both pages, 0 altered, including 20 that contain whitespace runs. The 11,385-char
<style>body and 555-char inline<script>body are byte-identical, and whitespace-normalised visible text matches exactly.On the size claim, in context
Worth stating plainly so nobody expects more than this delivers. The server sends
content-encoding: gzip, and gzip is already very good at exactly what the minifier removes:So the wire saving on
/indexis about 1 KB, not 15 KB. At 8.9 KB the HTML already arrives inside a single TCP initial congestion window (~14.6 KB), so there is no round-trip saved either. For context,/indexpulls 682 KB of external assets, including a 243 KB JS bundle, which makes the saving roughly 0.15% of total page weight.The benefits that do hold up:
/index, including<!-- Container Code: /application/containers/activity.vtl -->entries that leak internal VTL paths.Inline CSS is the larger remaining opportunity: 29.2% of the minified
/indexoutput is untouched inline<style>. It is also the safer of the two, since CSS has no ASI equivalent. Out of scope here and better as its own change.Screenshots
n/a — no UI changes.