Strip UTF-8 BOMs from each file when concatenating

[suyogyashukla]

Fixes Automattic#49. Reported again in a support ticket this week.

The problem

A BOM gives the encoding only at offset 0 of a file. The concatenator reads each file and appends the raw bytes, so the BOM of any file after the first becomes a U+FEFF character in the middle of the response. CSS accepts that character in a selector. The browser reads it as part of the next selector and ignores that rule, then carries on with the rest of the file.

The same rule renders with ?concat_css=false. The site that reported it ships a BOM on frontend.css from a Dart Sass build, and lost *{margin:0;padding:0}.

A BOM also breaks the @charset hoist, because that test reads offset 0.

The fix

Remove the BOM after the read and before the code that uses the buffer. relative_path_replace, the @charset hoist, the @import hoist and cssmin all read that buffer, so the position of this code matters. The JS branch gets the same change.

If a BOM was present, add charset=UTF-8 to the Content-Type header. Issue Automattic#49 does not ask for this, but the BOM was the encoding declaration for that file, and a strip on its own drops it. A variant that strips and declares nothing, served to a windows-1252 page:

Variant	CSS content char codes	Result
Strip only	[34, 226, 8224, 8217, 34]	"→", wrong
Strip and declare	[34, 8594, 34]	"→" (U+2192), correct

The Content-Type charset has a higher priority than a BOM and than an @charset rule, so the bundle stays UTF-8 for any file position.

Two things about that header, both worth recording. The charset is set only after a BOM, so nothing is claimed about the encoding of bundles that had none. But it also means the header now depends on a byte this code deletes: if a build stops emitting a BOM, the bundle loses charset=UTF-8 and the encoding interpretation changes, with no change to this plugin. The alternatives are to always declare UTF-8 for CSS and JS, which is a larger change than Automattic#49 asks for, or to re-emit one BOM at offset 0 and leave the header alone.

The practical scope is smaller than it looks. On VIP, CSS bundles already come back as text/css;charset=utf-8, and JS bundles as application/javascript with no charset, because PHP appends default_charset only to a text/* header. So for CSS on VIP this changes the spelling of a charset that was already there, to text/css; charset=UTF-8, which is the same value under RFC 7231. It carries real weight for JS, and for any deployment with an empty default_charset. PHP does not add a second charset when one is already set.

Testing

This file is the byte path for every concatenated asset request on VIP and WP.com.

I built a document root, drove ngx-http-concat.php over HTTP, and compared this branch against master for 17 requests. Every request without a BOM returns the same bytes, status and headers. Every request with a BOM loses the BOM, and Content-Length matches the body in all 17. The 404 and 400 paths do not change.

tests/test-concat-utils.php gains 46 cases. The suite is 78 tests and passes on PHPUnit 9.6, the version in composer.lock.

For the reported CSS in a browser, master leaves body at the 8px user agent margin and this branch gives 0px.

phpcs --standard=WordPress-VIP-Go reports the same violations on this branch as on master for both changed files, and none on the new test file.

Cases covered: a BOM in the first file, a later file and the only file; more than one BOM; a file of only a BOM; a file shorter than a BOM; an incomplete BOM; a BOM with @charset; a BOM with @import; a UTF-16 BOM; JS in both positions; the gzip and base64 URL form; HEAD; and a subdirectory install.

The cost on the common path is one 3-byte strncmp for each file, because has_utf8_bom returns before the loop starts. A file of only BOMs stays linear: at 50k, 100k and 200k BOMs, each doubling of the count doubles the time. Absolute timings are hardware-specific, so the ratio is the part to read.

Notes

Bundles already in the edge cache do not change. They hold for a year and clear on a version change, so a site with this problem needs a version change or a purge after this deploys.

The @charset hoist has a separate bug, and it predates this change. Files that start with @charset already get the rule copied to $pre_output and left in the body, because the preg_replace_callback result is never assigned. This change makes BOM'd files behave the same as those files, and does not alter the bug. Automattic#70 already proposes a fix for it.

The $had_utf8_bom to Content-Type wiring is the one piece of new logic with no automated coverage, because it lives in the procedural part of the script. The HTTP comparison above covers it, but nothing will catch a later regression.

[GaryJones]

Reviewed the diff, read the surrounding code, and ran the new functions standalone on PHP 8.3 to check the claims.

Verdict

The fix is correct, in the right place, and the diff is about as small as this problem allows. The strip happens after file_get_contents and before every consumer of $buf — relative_path_replace, the @charset test, the @import hoist, cssmin — which is the only position that works. Content-Length still sums $pre_output + $output, so it stays truthful. Only css and js are in $concat_types, so there is no binary-corruption path. No other byte path exists in the repo (cssconcat.php / jsconcat.php build URLs, they do not read files), so this is the root cause rather than a symptom patch.

Verified independently

I ran the 15 provider cases as plain PHP: all pass, idempotency holds, no residual BOM, and substr_compare never reads past the end because of the $offset + $bom_len <= $len guard. The loop is linear (50k/100k/200k/400k BOMs gave 12/25/49/98 ms on my machine — roughly 10x the figures in the description, so those absolute numbers are hardware-specific, but the shape is right). The no-BOM path is about 1 microsecond per MB, because has_utf8_bom short-circuits before the loop is entered.

Push-back

1. The conditional charset couples the header to a byte you have just deleted.

This is the part worth recording. The bundle's Content-Type now depends on whether some file in the group happened to ship a BOM. Fix the Sass build at source and the header silently loses charset=UTF-8 — the encoding interpretation changes again, for a change nobody made to this plugin.

It also creates a new failure shape: a bundle of file A (UTF-8, BOM) plus file B (@charset "windows-1252", high bytes) previously let B's @charset win; now the header overrides it and B renders as mojibake. Vanishingly rare, and mixing encodings in one bundle is already broken, but it is a way this change can make something worse.

The predictable alternatives are to always declare UTF-8 for CSS and JS (bigger blast radius, out of scope for Automattic#49), or to re-emit one BOM at offset 0 of the output (keeps today's declaration mechanism, no header change at all). I would still keep what you have — the header genuinely outranks a BOM and an @charset — but the hidden-coupling trade-off deserves a sentence in the description, because the current note only argues the other bundles case.

2. The multi-BOM loop's stated rationale does not hold.

"A file can be the result of an earlier concatenation" — but after this change this concatenator's output never carries a BOM at offset 0 at all, so the case cannot originate here. The loop is defensible on other grounds (some build tools really do emit doubled BOMs, and a second BOM lands as a stray character in the first selector), it is eight lines, and it is tested, so keep it if you like. Just correct the reasoning in the docblock, or drop to a single substr and lose one test case.

3. The @charset hoist.

Worth being precise here: this is not a new bug introduced by the PR. Non-BOM files starting with @charset already get the rule copied to $pre_output and left in the body, because the preg_replace_callback result is never assigned. This change simply makes BOM'd files behave the same as everything else. Leaving it alone is the right call for this PR.

Nits

• has_utf8_bom exists solely to set the flag, and strip_utf8_bom already handles the no-BOM case. $stripped = strip( ... ); if ( $stripped !== $buf ) { ... } would drop a public method. It reads fine as it is and both are tested, so this is preference, not a change request.
• The only new logic with no CI coverage is the riskiest part — the $had_utf8_bom to Content-Type wiring in the procedural script. The manual HTTP comparison covered it, but nothing will catch a regression later. Not cheaply fixable without restructuring the script; just worth knowing it is the uncovered edge.

[suyogyashukla]

Agreed on all three. The two code-side items are in a3c47b1 and 1605230, both comment-only, and the description carries the parts that were argument rather than code.

The multi-BOM loop

The reasoning was wrong in the way you describe. After this change the concatenator never emits a BOM at offset 0, so its own output cannot produce a repeated BOM.

Two commits because I first took the build-tool half of your parenthetical, then took it back out: neither of us has checked it, and it was the same kind of unverified claim as the line it replaced. The docblock now gives the half the tests demonstrate, that a second BOM is not an encoding signal, and in any file but the first it reaches the output as the same stray character.

Checking that turned up one detail. A single substr is not equivalent, but not for the reason I would have guessed: with one strip, a double-BOM file in first position leaves a BOM at offset 0, which is valid and harmless. It only becomes a stray character from second position on. So the loop is what makes "the output carries no BOM" true rather than usually true, which is the invariant your point above rests on. Loop and test kept.

The conditional charset

In the description now, with both alternatives you name.

One correction on the failure shape. On VIP that bundle is already mojibake today, because CSS bundles come back as text/css;charset=utf-8, so the header already outranked file B's @charset before this change.

I tested the mechanism rather than assuming it. On the same page, CSS bundles carry the charset and JS bundles come back as bare application/javascript. That split rules out nginx, whose default charset_types includes application/javascript and would have charset both. header('Content-Type: text/css') under default_charset=utf-8 reproduces the production header exactly, lowercase and unspaced.

So the coupling is narrower than it looks. For CSS on VIP this only respells a charset that was already there, as text/css; charset=UTF-8, the same value under RFC 7231, and PHP does not append a second. It earns its place on JS, and on any deployment with an empty default_charset.

Your "always declare UTF-8" alternative is stronger than my first pass allowed, because on VIP CSS already always carries the charset, so the conditional is what makes two bundles from one site differ. JS still rules it out: always declaring changes every JS bundle on every install, which is the blast radius you name.

Your scenario does hold where default_charset is empty. I checked VIP, not WP.com, so that part is worth your eyes.

The @charset hoist

The description now says the bug predates this change rather than leaving it to inference. Automattic#70 already proposes the assignment fix, open since 2022 on the same six lines. It does not conflict: this branch inserts above the block and changes the header, Automattic#70 rewrites the block.

Nits

Timings: the description now gives the doubling ratio and drops the absolutes, which is the right fix whatever explains the 10x. I put it down to hardware earlier; an active Xdebug would account for it more neatly.

has_utf8_bom: left as it is. The uncovered $had_utf8_bom to Content-Type wiring is recorded in the description as the known gap.

phpcs --standard=WordPress-VIP-Go reports the same violations on this branch as on master for both changed files, and none on the new test file.