Background Information

Some important background information to understand the problem:

  • Drupal now sets a content-length header for most responses (https://www.drupal.org/node/3298551). The length is set in web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php. The length is
    strlen($content) 
  • NGINX strictly follows the content-length header. If content-length=5 and it receives 6 characters, it truncates the last one.
  • It is possible to send data outside of the AJAX response. For example, whitespace before the open PHP tag is sent.

Problem/Motivation

We noticed that some AJAX calls were failing on a Drupal site. The browser was getting a response. But, the last two characters of the response were truncated. This caused an invalid response so the AJAX call failed.

We noticed that there were two characters of whitespace added to the beginning of AJAX responses. For example, Drupal built a response like:
[{"command":"update_build_id","old":"**********","settings":null}]

But the browser received a response like:
[{"command":"update_build_id","old":"**********","settings":null
Note the two whitespace at the beginning and the missing characters at the end. We back-tracked this to whitespace introduced before the open PHP tags on a custom .theme file.

The obvious solution is "don't have whitespace before the PHP tag". Given the number of issues, that happens more than it should. Drupal adds the content-length header. Drupal can do more to make sure Drupal is sending the correct length.

Steps to reproduce

  1. Set up a Drupal site. We are using Lando's Pantheon recipe with NGINX.
  2. Use a default admin theme. Let's say Claro.
  3. Edit a view in the Views admin UI. Click any link that makes an AJAX call. For example, change the title. Confirm that it works.
  4. Edit web/core/themes/claro/claro.theme. Add whitespace before the open PHP tag (" <?php")
  5. Retest the link in the Views admin UI. It should fail. You won't see any change on screen.
  6. Use the browser developer tools, network tab. The request will likely have status 200. But, if you dig into the response, you will see a syntax error. If you read the response, it will be missing characters at the end.

Proposed resolution

Protect AJAX responses so only the content in the response is sent. For example, I added AjaxResponse->send() to override Response->send(). In AjaxResponse->send(), I:

  1. ob_clean() to dump anything in the output buffer (like whitespace that happened to be before the open PHP tag)
  2. execute the parent send() method

For reference:

  • web/core/lib/Drupal/Core/Ajax/AjaxResponse.php extends vendor/symfony/http-foundation/JsonResponse.php
  • JsonResponse extends vendor/symfony/http-foundation/Response.php

Remaining tasks

I don't know the implications of doing a ob_clean() before sending the response. Could someone with more expertise please weigh in on any unintended consequences?

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3494148

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

jasonsafro created an issue. See original summary.

jasonsafro’s picture

Issue summary: View changes
jasonsafro’s picture

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
  }
jasonsafro’s picture

Issue summary: View changes
jasonsafro’s picture

Issue summary: View changes
jasonsafro’s picture

Issue summary: View changes
Related issues: +#3433154: Debug #3411329
jasonsafro’s picture

Version: 10.3.x-dev » 11.0.x-dev
quietone’s picture

Version: 11.0.x-dev » 11.x-dev

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.

jasonsafro’s picture

@quietone Thanks!

catch’s picture

Priority: Normal » Major

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’s picture

Priority: Major » Normal
Status: Active » Needs review
dimitriskr’s picture

Priority: Normal » Major

Setting priority back to Major per #10

longwave’s picture

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?

longwave’s picture

jasonsafro’s picture

StatusFileSize
new820.96 KB

Screenshot with notes to help in testing / debugging.

jasonsafro’s picture

@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 can miscount the content-length?

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.

catch’s picture

#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().

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
jasonsafro’s picture

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

jasonsafro’s picture

Status: Needs work » Needs review

@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.

smustgrave’s picture

Status: Needs review » Needs work

Still would need test coverage though.

Thanks.

catch’s picture

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.

jasonsafro’s picture

@catch I know you primarily responded about testing. But I also want to get the best fix in place. And you are right. My "fix" doesn't really fix the problem. My fix makes the problem go away by dumping anything already in the output buffer. Maybe we can do better.

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.

catch’s picture

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?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.