Ensure BigPipe does not break when HTML document contains CDATA sections or inline scripts matching certain patterns

Other occurrences of brittle parsing:

1. In \Drupal\big_pipe\Render\BigPipe::getPlaceholderOrder():
   

   $fragments = explode('<div data-big-pipe-placeholder-id="', $html);
   …
   $t = explode('"></div>', $fragment, 2);
   

2. In \Drupal\big_pipe\Render\BigPipe::sendPreBody:
   

   list($pre_scripts_bottom, $scripts_bottom, $post_scripts_bottom) = explode('<drupal-big-pipe-scripts-bottom-marker>', $pre_body, 3);
   

IS updated.

Oh and one more, in \Drupal\big_pipe\Render\BigPipe::sendNoJsPlaceholders():

    // Split the HTML on every no-JS placeholder string.
    $prepare_for_preg_split = function ($placeholder_string) {
      return '(' . preg_quote($placeholder_string, '/') . ')';
    };
    $preg_placeholder_strings = array_map($prepare_for_preg_split, array_keys($no_js_placeholders));
    $fragments = preg_split('/' . implode('|', $preg_placeholder_strings) . '/', $html, NULL, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE);

So, working towards a proposed resolution…

• We could theoretically use \DOMDocument-based parsing. I.e. DOM parsing, not string parsing. That'd work for cases 1–3, but not case 4, because no-JS placeholders may be embedded inside attribute values or even text nodes. That could theoretically be done using DOM parsing, but would be extremely slow.
• A much bigger problem is that \DOMDocument doesn't support HTML5. It only supports XHTML. #2429363: Add HTML5-lib to Drupal 8 core for the filter system and for the testing system added a library to support HTML5 parsing. But that means the parsing would be done in PHP, not in C, and hence be even slower.

Given all that, I'm not sure how we can solve this.

Since November 19, when BigPipe 8.x-1.0-beta1 for Drupal 8.0.x was tagged (https://www.drupal.org/node/2619070), not a single person has run into this problem. That's 3.5 months.

Therefore I think it's fair to say this is minor.

Even more so because CDATA is really XML, not HTML5, even though HTML5 still allows it.

#11: also using Kint?

Reproduced the problems with Kint.

Wow, Kint seems very poorly integrated:

1. it dumps its output immediately, even if there is no page output yet. This seems to somehow break the chunked encoding that BigPipe uses. So if you do kint($page) in quickedit_page_attachments() for example, it will completely break Drupal while BigPipe is enabled. You literally won't get any Drupal HTML, just Kint's <script> tag plus The website encountered an unexpected error. Please try again later.<br />.
2. it uses document.write() (gasp!) to load

This makes sense from the POV that Kint wants to be self-contained and robust, regardless of where it is used. But I think we need to more carefully integrate the module into Drupal to prevent these kinds of problems.

Triaged by @Fabianx and I at DrupalCon New Orleans.

We agree this needs to be fixed, but we both think it is in fact minor: it can be fixed at any time, will not require BC breaks. The Kint solution is tangentially related, but not exactly, so we moved that to a separate issue: #2723709: Investigate Kint + BigPipe compatibility.

Discussed with @Fabianx.

14:27:11 <Fabianx-screen> For https://www.drupal.org/node/2678662 I think we need to see how robust text parsing would work.
14:27:19 <Fabianx-screen> Obviously explode is very uhm rudimentary.
14:27:34 <Fabianx-screen> I just lack the necessary reg ex skills to implemented that.
14:27:54 <Fabianx-screen> Some of the sec folks might help as they do these kinda complicated matches and replacements all the time.
14:28:09 <Fabianx-screen> e.g.
14:28:35 <WimLeers> regex would also be brittle
14:28:35 <Fabianx-screen> for the placeholder maybe we should add <!-- --> sections behind and before the placeholder div.
14:28:37 <WimLeers> and slow
14:28:55 <Fabianx-screen> That would solve most problems as it being in accidental things is quite unlikely.
14:29:07 <WimLeers> right, but </body> is the most likely thing anyway
14:29:11 <Fabianx-screen> yeah
14:29:25 <WimLeers> so that's the one we *really* need to solve
14:29:29 <WimLeers> although I'm not convinced we do
14:29:29 <Fabianx-screen> And that we could still solve with strrev(), explode, strrev().
14:29:32 <WimLeers> CDATA is XML
14:29:34 <WimLeers> not HTML5
14:29:35 <Fabianx-screen> quite easily.
14:29:41 <WimLeers> which Drupal 8 site is going to use CDATA?
14:30:49 <WimLeers> and yeah, +1 for strrev, explode, strrev
14:30:59 <WimLeers> I think that's a "good enough" solution
14:31:04 <WimLeers> let's just do that and call it a day?
14:31:23 <WimLeers> it's all theoretical, and then we've addressed the most probable theoretical proble
14:31:27 <WimLeers> that's good enough, I say
14:31:37 <WimLeers> not our fault that PHP's DOM parser is absolute shit
14:55:02 <Fabianx-screen> WimLeers agree 100$
14:55:04 <Fabianx-screen> 100%
14:55:06 <Fabianx-screen> :)
14:55:09 <WimLeers> 100$ :D

So, this issue is going to solve the most-probable (yet still quite improbable) edge case in a simple, sane way. If we encounter these other edge cases in the real world, we'll deal with them then. Hopefully PHP will by then have a working HTML5 parser.

Assigning to me because I'll be working on this in the next few days.

To start, here's regression test coverage showing that a second </body> tag is a problem. This test should fail.

And here's the fix.

That'd work too. It's probably easier to understand.

Fabian, do you agree?

+1

Done.

Great!