Skip to content

feat(rendering): minify rendered HTML behind FEATURE_FLAG_MINIFY_HTML - #36852

Open
fmontes wants to merge 9 commits into
mainfrom
issue-36851-native-html-minification
Open

feat(rendering): minify rendered HTML behind FEATURE_FLAG_MINIFY_HTML#36852
fmontes wants to merge 9 commits into
mainfrom
issue-36851-native-html-minification

Conversation

@fmontes

@fmontes fmontes commented Jul 31, 2026

Copy link
Copy Markdown
Member

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 the getPageHtml callers (PageResource REST and PageRenderDataFetcher GraphQL). This method already post-processes for CSP, so minification follows the established pattern.
  • FeatureFlagName — adds the FEATURE_FLAG_MINIFY_HTML constant.
  • 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 to response.getOutputStream() and writes into the static page cache — it never returns through getPageHtml. A servlet filter or a hook in VelocityServlet would 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:

  • Copies <pre>, <textarea>, <script>, and <style> content byte-for-byte — protects rendered output and JavaScript automatic semicolon insertion.
  • Copies quoted attribute values byte-for-byte, so <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.
  • Treats < 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.
  • Collapses whitespace between inline elements rather than removing it, so <span>a</span> <span>b</span> keeps its space and words are never joined. Whitespace bordering block elements is removed.
  • Collapses only the five characters HTML treats as collapsible whitespace (space, tab, LF, CR, FF). Unicode spaces that browsers render — the ideographic space U+3000 common in CJK copy, the thin space U+2009 — are left untouched. Character.isWhitespace is deliberately not used, since it matches those too.
  • Strips HTML comments but retains downlevel conditional comments (<!--[if IE]>).
  • Does not otherwise rewrite markup — no <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.
  • Degrades gracefully — any failure logs a warning and serves the original markup, so a bug here cannot take a page down.
  • Is idempotent, which matters because LIVE mode can minify on write and again through eval().

Checklist

  • Tests
  • Translations — n/a, no user-facing strings
  • Security Implications Contemplated — see notes below

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:

  • jsoup (already a dependency at 1.21.1) is a parser, not a minifier. 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.
  • htmlcompressor — the original com.googlecode.htmlcompressor is abandoned (last release 2011). The maintained fork com.github.hazendaz:htmlcompressor:2.0.2 is 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-core currently 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 untouched FileUtilTest fails identically. I verified the suite by compiling and running it directly against the module classpath:

JUnit version 4.13.2
...........
OK (11 tests)

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: 3 before, OK (16 tests) after):

Issue Before Raised by
Whitespace inside quoted attribute values was collapsed, silently rewriting form values, JSON data attributes and accessible text <input value="a b"><input value="a b"> claude[bot]
A literal > 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> Copilot
Character.isWhitespace matched Unicode spaces that HTML renders rather than collapses <p>a b</p><p>a b</p> review

Also addressed the non-blocking 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.

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 before HTMLPageAssetRenderedBuilder.injectUVEScript runs its lastIndexOf("</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 HtmlMinifierIntegrityTest asserts 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/textarea bodies 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()), and EDIT_MODE/PREVIEW_MODE render with the UVE script correctly placed before </body>.

Page Flag off Flag on Saved
/index 54,290 39,021 28.1%
/members/index 15,144 8,124 46.4%

No 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:

unminified minified saved
raw 54,290 39,021 15,269 B (28.1%)
gzip -9 8,891 7,828 1,063 B (12.0%)

So the wire saving on /index is 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, /index pulls 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:

  • Page cache memory -- LIVE mode stores the minified string, so cached pages hold ~28% less heap. This applies to uncompressed bytes, so the full reduction is real. Probably the strongest argument for the feature.
  • Large HTML documents -- 12% of a small page is 1 KB, but 12% of a 100 KB gzipped listing page can cross round-trip boundaries.
  • Comment stripping -- 93 comments removed from /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 /index output 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.

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

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Claude finished @zJaaal's task in 5m 33s —— View job


Code Review

Reviewed HtmlMinifier and both render seams against origin/main. The design is sound, the safety reasoning holds up, and the three prior findings each now have a test that fails against the old implementation. One small new inconsistency below; everything else is clean.

New Issues

  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/rendering/util/HtmlMinifier.java:112-125 — Whitespace immediately before a retained conditional comment is dropped, contradicting the stated intent. isSignificantAfter() returns true for </p> minifies to `

    a

@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Jul 31, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 HtmlMinifier to 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 into VelocityModeHandler.eval() (post-CSP processing path).
  • Adds FEATURE_FLAG_MINIFY_HTML and 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.

Comment thread dotCMS/src/main/java/com/dotcms/rendering/util/HtmlMinifier.java Outdated
@zJaaal zJaaal added the PR: docker image Build & push a per-PR test image to dotcms/dotcms-test label Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🐳 PR Docker test image

Latest build for commit 5973036 pushed to dotcms/dotcms-test:

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>
Comment thread dotCMS/src/main/java/com/dotcms/rendering/util/HtmlMinifier.java
zJaaal and others added 2 commits August 5, 2026 13:26
…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>
@zJaaal

zJaaal commented Aug 5, 2026

Copy link
Copy Markdown
Member

Both review findings are addressed in 345b9788, plus a third case found while verifying them. Each has a test that fails against the previous implementation (Tests run: 19, Failures: 3 before, OK (19 tests) after).

1. Literal > read as a tag end (@Copilot)

Confirmed: <p>Home > About</p> came out as <p>Home >About</p>, because out.lastIndexOf("<") found the surrounding <p> and judged the space against a block element.

Rather than confirm that the last > closes the last tag, the scan now tracks the tag it emitted last as it goes, so the output buffer is never re-parsed:

// Name of the tag emitted last, or null when text was emitted last.
String lastTag = null;
...
private static boolean isSignificantBefore(final String lastTag) {
    return null == lastTag || isInlineTag(lastTag);
}

A literal > in text leaves lastTag null, so it reads as text and the space survives. A new isMarkupStart() gives the same treatment to a bare <, so <p>3 < 4</p> is also left alone. Side benefit: this removes an O(n) backward scan per whitespace run.

2. Attribute-value whitespace collapsed (claude[bot])

Confirmed: <input value="a b"> became <input value="a b">. Tags are now copied as a unit by a new appendTag() that tracks quoting, so attribute values survive byte-for-byte and a > inside a quoted value no longer ends the tag early. Whitespace between attributes is still collapsed to one space.

3. Unicode whitespace that HTML renders (found while verifying the above)

Character.isWhitespace matches characters HTML renders rather than collapses. Verified against the JDK that U+3000 (ideographic space, ordinary in CJK copy), U+2009, U+200A and U+2028/9 all return true, so they were being replaced by an ASCII space or dropped: <p>a b</p> became <p>a b</p>. Replaced with an isHtmlWhitespace() that matches only the five characters HTML treats as collapsible.

Also

The non-blocking 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.

Left deliberately alone: the space before a self-closing /. Dropping it would append the slash to an unquoted attribute value.

Test note

The ${net.bytebuddy:byte-buddy-agent:jar} surefire failure described above reproduces on a clean main checkout too, and -DargLine does not override it since the pom sets it. It blocks the whole dotcms-core unit suite locally, not just this class. I ran JUnit directly against the module classpath instead. Worth its own issue, but unrelated to this PR. CI is what should confirm the suite.

Investigated and written by Claude, posting under @zJaaal.

@mergify

mergify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

…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>
zJaaal and others added 2 commits August 5, 2026 16:47
…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>
@zJaaal
zJaaal requested a review from a team as a code owner August 5, 2026 20:04
@semgrep-dotcms

semgrep-dotcms Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Semgrep found 1 github-script-injection finding:

Using variable interpolation ${{...}} with github context data in a actions/github-script's script: step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code. github context data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable with env: to store the data and use the environment variable in the run: script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code Area : CI/CD PR changes GitHub Actions/workflows PR: docker image Build & push a per-PR test image to dotcms/dotcms-test

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Native, configurable HTML minification in the core rendering engine

4 participants