FinishResponseSubscriber: Need warning/error when headers exceed 16k

mr !143 apply and solve my problem on D9.5
Many thanks!!!

mr! 1943 Worked for me too on 9.3.22. Thanks!

Moving to NW for the test cases before reviewing.

Hello
I'm facing off this on D9.5.3, do I need to patch ? if yes, which files ?

Thanks

@aiphes : you must look at the mr !1943 (see #57 for the link). From there, you can download the patch (in the upper right part of the screen see Code > download email patches)

@Dunebl, so this is the patch https://www.drupal.org/files/issues/2021-12-09/2844620x93.patch → , and in which folder do I need to apply it ?

@aiphes this is not the last/full patch. Download it from the explanations I provide in #66 then apply it from within your Drupal root.

Here's a link to the MR:

https://git.drupalcode.org/project/drupal/-/merge_requests/1943

And here's a direct link to the patch:

https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff

I believe, security-wise, it's best practice to download the diff and use it a local patch file than reference this link, because attackers could update this MR to introduce vulnerabilities, and then you'd just automatically pull them in.

You can unpatch with this command:

cd "${drupal_root}"
patch -p0 -R < "${path_to_patch}"

@DuneBL & @charginghawk
Thanks for your help, patch seem to fix my random 500 error on editing certain content types. Hope this is durable. I will do the same on my others D9 websites.
Will I need to repatch after core updating in future ?

@aiphes: yes, you will need to repatch after each core update. (you can use composer and add the patch in composer.json)

@DuneBL for sure, but how can I do that ?

A quick search leads me to this article: https://duvien.com/blog/how-apply-patch-file-composer-based-drupal-89

ok, but I get errors of syntax:

 "extra": {
        "drupal-scaffold": {
            "locations": {
                "web-root": "web/"
            },
		"file-mapping": {
             "[web-root]/robots.txt": false,
			 "[web-root]/sites/default/default.*": false
			}
        },
		"patches": {
			"drupal/core": {
				"Patch pour Premature end of script headers - 2844620": "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff"
			}
		}
    },

 "./composer.json" does not contain valid JSON
  Parse error on line 290:
  ...   ]        }    },    "require-dev":
  --------------------^
  Expected: 'EOF'

Um, sorry to interrupt the party, but I'd like to note that usually the patch is not needed for live websites. Make sure the development mode is not active (i.e. the line "include $app_root . '/' . $site_path . '/settings.local.php';" is commented/not enabled in sites/default/settings.php). Development mode should not be enabled for live websites! If you run into this problem with development mode enabled, make sure "http.response.debug_cacheability_headers" is set to false in sites/development.services.yml. Make sure you clear the cache after any changes in these files. These things should usually avoid this error - no need to patch anything.

@rgpublic So you tell that we doesn't need to use settings.local.php on production websites ?
Because I do this every time, whithout issues except this one after migrating from D6 or D8 to D9.

@aiphes: Exactly. You can open and read that file if you don't believe me. You will find that contains only things that are needed for *development*. It will significantly slow down your live website. It disables caches, enables debugging headers and so on. It will also leave your site vulnerable to some attacks. You *can* run with this enabled. But it's not recommended.

@rgpublic ok, I will plan to use the settings.php and see after the future update ( that will drop the patch) , my 500 errors still goes out. Finger cross.

@DuneBL Trying to rollback and it doesn't work
/sited9$ patch -p0 -R < "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff" or

/sited9/web$ patch -p0 -R < "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff"

give
-bash: https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff: No such file or directory

@aiphes I don't think that the patch command works with https. You should use the real patch file in your local hard drive

@DuneBL But I don't know where are stored patches. I've nothing in the ~/patchs folder :/ I go to ask to my web host.

There is a misunderstanding: you must download by yourself the patch file and put it in the HD of your web host. After that you can run the command to apply or un-apply this patch
I am a little bit annoyed because I think we are polluting this thread with other concerns

I am a little bit annoyed because I think we are polluting this thread with other concerns

Totally right :) ;)

@rgpublic after done an update - Upgrading drupal/core (9.5.3 => 9.5.7) and disable settings.local before, it seem to work without 500 errors on known pages. Your tips seem to be the real fix :)

You should have used it like so https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs.patch

Preferably download it and use it as a local patch in your composer.json since, if the code in the pr changes, the patch will change as well with your next composer install/update whatever, and that might be something you don't want to happen uncontrollably.

I uploaded a patch you can use, and it will also be useful (probably) for allowing drupal.org test suite to run.

Thanks Marios, but I don't understand anymore. Some says patch isn't a good way, prefer using settings.php only, others say patch is needed. :/

Great discussion about patches, but let's please, out of respect for the 87 people following this issue, stop talking about patches here, and do not even reply to say "I agree" or "I understand" or something. We're done talking about patches! Thank you! Direct any further patch discussion here:

https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →
https://www.drupal.org/node/1399218/discuss →

Regarding settings.local.php and development.services.yml (which, rgpublic is correct, should not be enabled on production environments!), please refer to the files themselves and you can discuss it on the "Disable caching" page:

https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/example.se...
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/developmen...
https://www.drupal.org/docs/develop/development-tools/disable-caching →
https://www.drupal.org/node/3340861/discuss →

You can also always go to Forums, drupal.stackexchange.com, Slack to get answers to your questions:

https://www.drupal.org/community/contributor-guide/reference-information... →

My next comment here is going to summarize the state of this ticket and hopefully get us back on track.

The most recent MR/patch is still mxr576's, !1943, from 8 March 2022.

https://git.drupalcode.org/project/drupal/-/merge_requests/1943

It is reviewed by the community (#62, #63) but it needs tests (#64).

Hi, I recently upgraded my site to D9.5 and I'm getting this error on some of my pages. So far I tried the following:
1) Created a patch from 1943 MR and applied as a local patch
2) Updated the `http.response.debug_cacheability_headers` to `FALSE`

None of these above works for me. The errors I see apart from the WSOD can be found below:

web_1  | [Wed Apr 12 07:08:19.863755 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] [client 172.23.0.2:34096] Premature end of script headers: index.php, referer: http://mysite.docksal.site/admin/content
web_1  | [Wed Apr 12 07:08:19.864173 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] [client 172.23.0.2:34096] AH01070: Error parsing script headers, referer: http://mysite.docksal.site/admin/content
web_1  | [Wed Apr 12 07:08:19.864194 2023] [proxy_fcgi:error] [pid 9:tid 281473321831168] (22)Invalid argument: [client 172.23.0.2:34096] AH01075: Error dispatching request to : , referer: http://mysite.docksal.site/admin/content
web_1  | 172.23.0.2 - - [12/Apr/2023:07:08:00 +0000] "GET /node/1901/edit?destination=/admin/content HTTP/1.1" 500 -

Any idea what else I can do ? Thanks in advance.

@dynamdilshan This ticket deals with limiting the X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers. With http.response.debug_cacheability_headers set to false, these should not be an issue anyway. You can use xdebug or other debugging tools to step through the code in the patch and confirm whether or not those headers are limited or even need limiting.

I will note that other headers (including from the fastly and pantheon_advanced_page_cache modules) can get out of control and the patch doesn't account for them (though you could maybe argue it should). That's based on recent comments on this Docksal issue:

https://github.com/docksal/docksal/issues/1110#issuecomment-1216488250

Thanks @charginghawk for your reply. Due to some reason this patch and the http.response.debug_cacheability_headers set to false not working for me. I put this below code on index.php to debug and get the size of my headers

if ($config['config_split.config_split.local']['status']) {
  $response->headers->set('x-drupal-cache-tags', '');
  $response->headers->set('cache-tags', '');
}

And currently nuking all the cache tags on local is the only way for me to get the page working. I found found out some of my pages giving more than 8k size headers.

I was having a similar issue but it turned out not to be an issue with core. My enormous header was edge-cache-tags from the akamai module. Disabling the purge and akamai modules on local took care of it.

Here's a patch version of MR 1943 (latest commit bbcb4ac7), if anyone else needs it.
PS: our setup enforces static patches and MR diffs are not allowed (as they can change unexpectedly).

I was able to reduce the number of tags dramatically by updating the cache tag blacklist with all the options listed here: https://www.drupal.org/docs/contributed-modules/akamai/cache-tag-purging →

On some pages it reduced the number of tags from ~600 to ~100. Still a lot, but definitely an improvement.

Thank you, this fixed a problem in a project that was recently updated to Drupal 10 where I was suddenly unable to edit the homepage.
I could revert the revision of the homepage and then edit it. But after a while editing would again not work and again the /node/xxxx/edit page would return an error 500. This patch (the diff from the issue fork) fixes it in my D10 project.

Same here after update of project to 10.1.7
Updated the `http.response.debug_cacheability_headers` to `FALSE`
+ patch #87 fixed it

I tried to clarify current state in the issue body: we need test to move this to RTBC.

The MR should be used, ignore patches.

The patch is working for me in a real project.

#87 works on D9.5.11 core .

Ran into this recently loading the fields settings page for a content type after http.response.debug_cacheability_headers was set to "true" via the development.services.yml file; the site runs on ddev, Safari showed the site giving a 500 error and nothing I could do would give me an explanation of what was going on.

Considering this is still an issue in D11, I figured we should move it forward. Added tests and new MR for 11.x, back to review

smustgrave → changed the visibility of the branch 11.x to hidden.

Added some nitpicky change for the tests.

Leaving in review for additional eyes.

Changes loosk good to me. However, it would be great if we can run the tests only without the patch to confirm the test works ok (fails when no fix has been applied, passes when it is applied). I tried to run the "Test-only changes" stage at https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1342592 but I says:

This job does not run automatically and must be started manually, but you do not have access to it.

@tunic this was previously run, but I guess retriggered on rebase. For access I think you just need to request access to the fork and then sign in to gitlab.

Either way, here's the pass on main, and fail on test-only run: https://git.drupalcode.org/issue/drupal-2844620/-/pipelines/148275

Ah, of course, I forgot to ask for access rights.

Issue is addressed, code issues have been solved, test-only patch fails as it should and tests passes with patch. I think this can be moved to RTBC but I'll wait for more reviewers.

Updating solution.

BTW, do we need a change record

Given this is a debug-specific change, I don't think a change record here is necessary; however, updating any docs referring to debugging headers is definitely a good idea.

Other doc page need also to be update https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags-varnish → , https://www.drupal.org/project/purge → , https://www.drupal.org/project/varnish_purge →

#114, I think those are related but different headers. In this issue we are handling the headers X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts that are sent by Drupal core when the cacheability debug functionality is on (http.response.debug_cacheability_headers: true). However, the links you provide refer to the Cache-Tags and Purge-Cache-Tags headers (each purger module uses one or the other).

While the information they carry is similar (the cache tag information) and all are affected by the same problem (header too large), the purger headers are not affected by the fix proposed here because that later headers are added by each purger module itself.

It can be interesting to propose a similar fix on the purger modules; however, it will probably be harder for Varnish to process a single header or multiple headers with an incremental sequence (Cache-Tags-1, Cache-Tags-2, etc) so I guess this is something that must be discussed.

ok, sorry for the confusion (TL; DR 😬).
I think purge should just split tags by 16k chunks and make multiple calls to Varnish/CDN.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Ran the test-only feature https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1943211

And see there are 2 tests but testDebugHeaders isn't failing without the change. So does it need to be tweaked or removed?

@smustgrave Sorry I should've been more clear in my description. I added two tests here:

1. testDebugHeaders()
   • This test was to test the general functionality of debug headers since a test for this didn't exist already, it will pass under current conditions
2. testLargeTagsAndContexts()
   • This is a test for the added functionality of this issue, and should fail without the patch

I was going to split this to two issues at first for the initial test, but figured given the longevity of this issue it made sense to keep it here. Back to NR.

@b_sharpe thanks for the explanation

Left 1 nitipicky comment but overall changes look good. +1 from me.

Going to leave in review for additional eyes.

Self applied 1 nitpicky item.

The title no longer matches the issue summary and MR, can it be updated? Also one comment on the MR.

scott_euser → changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to hidden.

1. Resolved feedback
2. Updated issue summary
3. Updated title

Updated to standard issue summary template

Ha thanks! Clearly my morning coffee hadn't kicked in yet!

Appears feedback has been addressed.

I read the IS, comments and the MR. I don't see any unanswered questions. Thank you to @charginghawk for working on summarizing and keeping this issue on track.

There is a large number of documentation links in the comment for places this needs to be updated. Does this need to be documented in so many places? I have not looked at all the pages but this should be documented in one place and the others refer to that one place.

Here are the links I gathered from the comments

• https://www.drupal.org/docs/develop/development-tools/disabling-and-debu... →
• https://www.drupal.org/docs/drupal-apis/cache-api/cache-contexts →
• https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags →
• https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags#reverse-pro... →
• https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags-varnish →
• https://www.drupal.org/docs/drupal-apis/render-api/cacheability-of-rende... →

I left some suggestions and comments in the MR which need to be looked at so setting to NW.

I also updated credit.

I just stumbled upon an issue with Cloudflare Purger and created https://www.drupal.org/project/cloudflare/issues/3482279 🐛 Cache tags / hash collision Active for that. I then found this issue and I think both are related to some extent. I was hoping the solution of this issue would apply for the Cloudflare Purger Cache-Tags case, but I don't think it would work if we're adding numerical suffixes such as "-1", "-2" etc when we have multiple "split" response headers.

For Cloudflare specifically their documentation seems to indicate the solution is to have multiple response headers with the same name but different values.

That seems to be applicable to that header as it might for X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts as those adhere to the comma-separated specification for response header values (although the "X-" ones are purely for debugging purposes to my knowledge, while the Cloudflare Purger Cache-Tags is functional). See:

1. https://stackoverflow.com/q/3241326
2. https://www.rfc-editor.org/rfc/rfc9110.html#name-field-order (above answer points to a deprecated link, this should be the updated info)

So the question is, if this solution is applicable for any headers, should we allow some to be added multiple times without adding numerical suffixes?

Just created a patch from the MR so I can apply it in my composer.json.

dubois → changed the visibility of the branch 2844620-finishresponsesubscriber-need-warningerror to active.

As @erikbj also mentions in #131, the official proper way to handle headers with too many values is indeed to split them up into separate lines but with the same header name (see https://www.rfc-editor.org/rfc/rfc9110.html#section-5.2). This would also ensure it remains easy to implement downstream logic basic on the header ("if header contains xyz" is a lot easier than "if header-1 contains xyz or header-2 contains xyz or header-3 contains xyz ...")

So I think splitting the headers but keeping the name is preferable over the current solution appending '-{delta}'. One caveat however: the different header line values are combined with ",", which means the current implementation would have to be converted to generate a comma-separated list instead of a space-separated list (which in turn is not backwards-compatible). On the other hand, this is meant to be a debugging tool, so I don't think this is that big a deal.

Splitting the header into separate lines with the same header name is going to be difficult, given that the Symfony Response object's headers object doesn't allow for multiple headers with the same key.

Also, if the goal here is only to fix headers for the debug cache tags, having the -1, -2 suffixes probably works just fine. If this issue also needs to address headers generally that are too long (such as the ones used by Purge), the current MR does not address that, and the question of whether and how to split headers into multiple lines with the same name is relevant.

Hi, everyone

I've tried to apply patch #132 to 11.x version but it does not apply. I updated it and created a new patch for version 10.2.x

Please, check if it's working.

#132 is working for me on 10.4.3. Was getting a WSOD on pages with larger views with images results, facets, and having admin toolbar extras installed. Pages load successfully now, without having to limit views result size.