Strip UTF-8 BOMs from each file when concatenating

concat-utils.php

@@ -3,6 +3,9 @@ class WPCOM_Concat_Utils {
 	// Maximum group size, anything over that will be split into multiple groups
 	protected static int $concat_max = 150;
 
+	// UTF-8 byte order mark.
+	const UTF8_BOM = "\xEF\xBB\xBF";
+
 	public static function get_concat_max() {
 		return self::$concat_max;
 	}
@@ -44,6 +47,49 @@ public static function realpath( $url, $site_url ) {
 		return realpath( ABSPATH . $url_path );
 	}
 
+	/**
+	 * Tells if a buffer starts with a UTF-8 byte order mark.
+	 *
+	 * @param string $buf File contents.
+	 * @return bool
+	 */
+	public static function has_utf8_bom( $buf ) {
+		return 0 === strncmp( $buf, self::UTF8_BOM, strlen( self::UTF8_BOM ) );
+	}
+
+	/**
+	 * Removes UTF-8 byte order marks from the start of a buffer.
+	 *
+	 * A BOM gives the encoding only at offset 0. In a bundle, the BOM of a later
+	 * file becomes a U+FEFF character in the middle of the stream. CSS accepts
+	 * that character in a selector. The browser then adds it to the next
+	 * selector, and ignores that rule.
+	 *
+	 * The function removes every BOM at the start, not only the first one, so the
+	 * buffer it returns has none. A second BOM is not an encoding signal. In any
+	 * file but the first it becomes the same stray character.
+	 *
+	 * It keeps a UTF-16 or UTF-32 BOM. Those bytes are not valid in a UTF-8 bundle.
+	 *
+	 * @see https://github.com/Automattic/nginx-http-concat/issues/49
+	 *
+	 * @param string $buf File contents.
+	 * @return string The buffer without a BOM at the start.
+	 */
+	public static function strip_utf8_bom( $buf ) {
+		$bom_len = strlen( self::UTF8_BOM );
+		$len     = strlen( $buf );
+		$offset  = 0;
+
+		// Count the BOMs in one pass. A file of only BOMs then needs one copy.
+		while ( $offset + $bom_len <= $len
+			&& 0 === substr_compare( $buf, self::UTF8_BOM, $offset, $bom_len ) ) {
+			$offset += $bom_len;
+		}
+
+		return 0 === $offset ? $buf : substr( $buf, $offset );
+	}
+
 	public static function relative_path_replace( $buf, $dirpath ) {
 		// url(relative/path/to/file) -> url(/absolute/and/not/relative/path/to/file)
 		$buf = preg_replace(

ngx-http-concat.php

@@ -143,6 +143,7 @@ function concat_get_path( $uri ) {
 $last_modified = 0;
 $pre_output = '';
 $output = '';
+$had_utf8_bom = false;
 
 $css_minify = new tubalmartin\CssMin\Minifier;
 
@@ -175,6 +176,13 @@ function concat_get_path( $uri ) {
 	if ( false === $buf )
 		concat_http_status_exit( 500 );
 
+	// Remove the BOM before the code below reads $buf. The @charset test is at
+	// offset 0, and a BOM also breaks that test.
+	if ( WPCOM_Concat_Utils::has_utf8_bom( $buf ) ) {
+		$buf = WPCOM_Concat_Utils::strip_utf8_bom( $buf );
+		$had_utf8_bom = true;
+	}
+
 	if ( 'text/css' == $mime_type ) {
 		$dirpath = $subdir_path_prefix . dirname( $uri );
 
@@ -240,8 +248,17 @@ function ( $match ) use ( $dirpath ) {
 		$output .= "$buf";
 }
 
+// A BOM showed that a file is UTF-8, and the loop above removed it. Give that
+// encoding in the header to keep the declaration. The Content-Type charset has a
+// higher priority than a BOM and than an @charset rule. Set the charset only
+// after a BOM, because the encoding of other bundles is not known.
+$content_type = $mime_type;
+if ( $had_utf8_bom ) {
+	$content_type .= '; charset=UTF-8';
+}
+
 header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s', $last_modified ) . ' GMT' );
 header( 'Content-Length: ' . ( strlen( $pre_output ) + strlen( $output ) ) );
-header( "Content-Type: $mime_type" );
+header( "Content-Type: $content_type" );
 
 echo $pre_output . $output;

tests/test-concat-utils.php

@@ -206,3 +206,144 @@ function test__function( $test_string, $expected, $dirpath = '/' ) {
 		$this->assertSame( $expected, $actual );
 	}
 }
+
+class WPCOM_Concat_Utils__Strip_Utf8_Bom__TestCase extends WP_UnitTestCase {
+	const BOM = "\xEF\xBB\xBF";
+
+	function get_test_data() {
+		return array(
+			// Nothing to strip.
+			'empty_string' => array(
+				'',
+				'',
+				false,
+			),
+			'no_bom' => array(
+				'*{margin:0;padding:0}',
+				'*{margin:0;padding:0}',
+				false,
+			),
+			// The buffer is shorter than a BOM. The code must not read too far.
+			'single_byte' => array(
+				'a',
+				'a',
+				false,
+			),
+			'two_bytes' => array(
+				'ab',
+				'ab',
+				false,
+			),
+			// An incomplete BOM is not a BOM.
+			'partial_bom' => array(
+				"\xEF\xBB",
+				"\xEF\xBB",
+				false,
+			),
+			'partial_bom_then_text' => array(
+				"\xEF\xBBz",
+				"\xEF\xBBz",
+				false,
+			),
+			// The reported bug. A BOM is before the first rule of a file.
+			// https://github.com/Automattic/nginx-http-concat/issues/49
+			'bom_then_rule' => array(
+				self::BOM . '*{margin:0;padding:0}',
+				'*{margin:0;padding:0}',
+				true,
+			),
+			// The @charset test is at offset 0. A BOM breaks that test.
+			'bom_then_charset' => array(
+				self::BOM . '@charset "UTF-8";',
+				'@charset "UTF-8";',
+				true,
+			),
+			'bom_only' => array(
+				self::BOM,
+				'',
+				true,
+			),
+			// Only a BOM at offset 0 is an encoding signal. The rest must go too.
+			'repeated_boms' => array(
+				self::BOM . self::BOM . self::BOM . '.a{color:red}',
+				'.a{color:red}',
+				true,
+			),
+			// A BOM gives the encoding only at offset 0. In other positions it is content.
+			'bom_mid_stream_is_left_alone' => array(
+				'.a{color:red}' . self::BOM . '.b{color:blue}',
+				'.a{color:red}' . self::BOM . '.b{color:blue}',
+				false,
+			),
+			'bom_at_end_is_left_alone' => array(
+				'.a{color:red}' . self::BOM,
+				'.a{color:red}' . self::BOM,
+				false,
+			),
+			// UTF-16 and UTF-32 bytes are not valid in the bundle. Keep the BOM,
+			// because UTF-8 is the wrong encoding for that content.
+			'utf16_le_bom_untouched' => array(
+				"\xFF\xFE.a{color:red}",
+				"\xFF\xFE.a{color:red}",
+				false,
+			),
+			'utf16_be_bom_untouched' => array(
+				"\xFE\xFF.a{color:red}",
+				"\xFE\xFF.a{color:red}",
+				false,
+			),
+			// The first character has more than one byte, but it is not a BOM.
+			'leading_non_bom_multibyte_char' => array(
+				"\xE2\x86\x92.a{color:red}",
+				"\xE2\x86\x92.a{color:red}",
+				false,
+			),
+		);
+	}
+
+	/**
+	 * @dataProvider get_test_data
+	 */
+	function test__strip_utf8_bom( $test_string, $expected, $expected_has_bom ) {
+		$this->assertSame( $expected, WPCOM_Concat_Utils::strip_utf8_bom( $test_string ) );
+	}
+
+	/**
+	 * @dataProvider get_test_data
+	 */
+	function test__has_utf8_bom( $test_string, $expected, $expected_has_bom ) {
+		$this->assertSame( $expected_has_bom, WPCOM_Concat_Utils::has_utf8_bom( $test_string ) );
+	}
+
+	/**
+	 * A safe buffer has no BOM. Therefore a second call must make no change.
+	 *
+	 * @dataProvider get_test_data
+	 */
+	function test__strip_utf8_bom_is_idempotent( $test_string, $expected, $expected_has_bom ) {
+		$once = WPCOM_Concat_Utils::strip_utf8_bom( $test_string );
+
+		$this->assertSame( $once, WPCOM_Concat_Utils::strip_utf8_bom( $once ) );
+		$this->assertFalse( WPCOM_Concat_Utils::has_utf8_bom( $once ) );
+	}
+
+	/**
+	 * The stream must have no BOM after offset 0. The position of the file
+	 * with the BOM must not change this result.
+	 */
+	function test__concatenating_stripped_buffers_leaves_no_stray_bom() {
+		$files = array(
+			'body{color:red}',
+			self::BOM . '*{margin:0;padding:0}',
+			'.c{color:blue}',
+		);
+
+		$output = '';
+		foreach ( $files as $buf ) {
+			$output .= WPCOM_Concat_Utils::strip_utf8_bom( $buf );
+		}
+
+		$this->assertSame( 'body{color:red}*{margin:0;padding:0}.c{color:blue}', $output );
+		$this->assertStringNotContainsString( self::BOM, $output );
+	}
+}