AJAX Response Not Sanitized For Length

Adding the following to web/core/lib/Drupal/Core/Ajax/AjaxResponse.php

  /**
   * Safely send headers and content.
   *
   * Empty anything already in the output buffer. Then, use the parent method
   * to send headers and content.
   *
   * @return $this
   */ 
  public function send(): static {
    ob_clean();
    return parent::send(); // TODO: Change the autogenerated stub
  }

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. So, changing this to 11.x.

@quietone Thanks!

Bumping this to major, while it's technically a workaround for extra whitespace in PHP code, it would make everyone's lives a lot easier if it works well.

jasonsafro → closed merge request !10605

Setting priority back to Major per #10

This is neat, but it does hide the real problem. I thought about a different solution when I ran into this before - leading whitespace in a .module file - but never implemented it: when we include .module or .theme files, we could wrap them in ob_start() and ob_end_clean(), and optionally throw a warning to tell developers that there is a problem with their file?

🐛 Uncaught ajax.js error / exception Active is possibly fixed by this as well.

Screenshot with notes to help in testing / debugging.

@longwave You are not wrong. I'm not even sure what is "the real error". Is the real error that someone left some whitespace outside the PHP tags? Or, is the real error that Drupal sets content-length without accurately counting?

If there's a better solution, I'm all for it. But, I'd like to make some change. Debugging this error was not fun or easy.

#16 is worth a try and it would be easier to add a test. Since it could potentially run on every request, we should probably do it in an assert rather than actual logging?

Probably need to support include files too but that can be done in ModuleHandler::loadInclude().

@longwave @catch - Are either of you working on the solution that you discussed?

@longwave proposed that we add some checks to tell programmers if there is extra whitespace in their files. This is an interesting idea. Personally, I think that test belongs in a linter. I'm open to discuss but have not been able to get ahold of @longwave.

I'm going to move forward with the solution that is ready to use. It resolves a difficult to debug issue that is tied to half a dozen tickets. I'd appreciate any feedback.

Still would need test coverage though.

Thanks.

So I agree that we should add linting for this, it could probably be added to phpcs (or maybe there's an existing rule). It is worth an issue against https://www.drupal.org/project/issues/coder → and then we'd need to make sure that runs on the gitlab templates.

Having said that we added the content length header not that long ago, and a surprising number of people have run into this issue, and it's not easy to diagnose, so I think there could be value to the workaround here even if we also open a follow-up to remove it again in Drupal 12 or 13.

On test coverage, I originally though about testing the actual AJAX system, but I think the problem is webserver-specific (e.g. some are permissive with mis-matched content lengths, some or not), and ideally we want the test to fail on all environments.

Something like:

- create a test module (add it to core/modules/system/tests/modules) - it has a .module file with whitespace before the opening <?php tag, doesn't need any more than that apart from .info.yml

- add an extra method to core/tests/Drupal/FunctionalTests/HttpKernel/ContentLengthTest.php

- in that method, enable ajax_test module and the test module added above, request an AJAX url, and check that the content length matches the content length header (or a fixed module that is equal to the content length header).

Or maybe a bit cleaner, would be to add an AJAX controller to http_middleware_test module and hit that URL instead, then the content length can be controlled - e.g. it won't break if someone changes ajax_test module for some other reason then.

@catch You are right. My "fix" doesn't really fix the problem. My fix empties the output buffer before sending the Drupal content. That makes the problem go away.

Would you prefer a patch that checks the size of the output buffer and then does *something*? For example, check line 48 of web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php. I could replace:

// Current code.
$response->headers->set('Content-Length', strlen($content), TRUE);

with

// Proposed option.
// https://www.php.net/manual/en/function.ob-get-length.php.
$total_content_length = (ob_get_length() ?? 0 + strlen($content));
$response->headers->set('Content-Length', $total_content_length, TRUE);

I could also throw an exception if the buffer isn't empty or do something else if you have a good idea.

I could also throw an exception if the buffer isn't empty

I don't think we should throw an exception because it would cause sites that currently work (because their webservers are permissive), to not work. But we could add an assert() so it fails hard on local development environments and tests, and also log a warning (for when assertions are off).

$total_content_length = (ob_get_length() ?? 0 + strlen($content));
$response->headers->set('Content-Length', $total_content_length, TRUE);

This seems... more honest?