Deprecate allow_insecure_derivatives and suppress_itok_output in image system

+1 to removing them. Do we need a hook_update_N() to remove them from active config as well?

https://security.drupal.org/node/70884#comment-45713 (needs sec. team access) seems to be the first place this variable was talked about, as a way to help with the intrusiveness of the hardening. But if that's the only driving reason, stricter image derivatives is probably the least intrusive thing about moving from D7 to D8. ;)

The issue summary does not mention the disruption of this to D8 sites that already utilize these settings. I have 1 live client site already that does this.

Fixed the MD5 hash in the Variable.php dump file, at @alexpott's request.

Re #15:

So one argument that I have heard repeatedly from clients is that the added query parameter is bad for SEO. I can't say that I understand why.

In this particular case, however, we were not utilizing the on-demand creation of image derivatives anyway. We were importing images from an external system and creating all derivatives during import. So the added security of the query parameter, was irrelevant in that case. I.e. even if it didn't do any harm, it was unnecessary at best.

Yes, people in #1934498: Allow the image style 'itok' token to be suppressed in image derivative URLs were very adamant about leaving 'allow_insecure_derivatives' and 'suppress_itok_output' on indefinitely (which is how the latter got committed to both Drupal 7 and Drupal 8 in that issue) so as to remove the query strings from image derivatives entirely. I never fully understood the reasons either - maybe the best is #1934498-62: Allow the image style 'itok' token to be suppressed in image derivative URLs which suggests that Google PageSpeed flags query parameters on image URLs as a problem.

There was also an IMCE feature broken by this that has never been restored and doesn't seem like it will be - see #1930698-6: Drupal 7.20 update broke IMCE's feature for generating previews via image styles.

I'm not sure either of those are that compelling, but people are using this for more than backwards compatibility in Drupal 7, and it feels like it might be a little late to remove from Drupal 8 at this point.

We should add proper deprecation notices in 8.x if we're going to remove these in 9.x per #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases.

Is this still something that is going to be deprecated on Drupal 9?

initial re-roll to check current state, surely it needs to deprecate config but no idea how to keep BC same time

#34 failed in the test file:

patching file core/modules/image/config/install/image.settings.yml
patching file core/modules/image/config/schema/image.schema.yml
patching file core/modules/image/image.install
Hunk #1 succeeded at 76 with fuzz 2 (offset -6 lines).
patching file core/modules/image/migrations/d7_image_settings.yml
patching file core/modules/image/src/Controller/ImageStyleDownloadController.php
Hunk #1 succeeded at 115 (offset 18 lines).
patching file core/modules/image/src/Entity/ImageStyle.php
Hunk #1 succeeded at 210 (offset 3 lines).
patching file core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php
Reversed (or previously applied) patch detected!  Skipping patch.
3 out of 3 hunks ignored -- saving rejects to file core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php.rej
patching file core/modules/image/tests/src/Kernel/Migrate/d7/MigrateImageSettingsTest.php
Hunk #1 FAILED at 26.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/image/tests/src/Kernel/Migrate/d7/MigrateImageSettingsTest.php.rej

Rerolled the patch in #34, thanks!

Tried to fix custom command failure

This will resolve test failures.

Well i have read a lot on that SEO argument and also checked a lot of pages. I don´t see other pages that drupal adding these tokens.
All my pages also have been kind of hurt after upgrading from D7->8->9
Removing that feature i guess will result in a lot of custom hacks again. I will now disable the itok on my sites an check the results.
And yes, i know it is there because of a possible denial of service attack requesting hundreds of derivates using some script. That kind of fast hammering in any way should be banned very fast anayway by like fail2ban on bigger sites.

Since folks are asking for a case study... my current reason for (at least considering) using this parameter is due to an unresolved issue with image style generation:
https://www.drupal.org/project/drupal/issues/3225202

We also had a need for suppress_itok_output, when using the flysystem_s3 module, with patch from #2772847: Add support for private uploads / presigned URLs. The itok param would be put onto s3 urls, and cause the signature to be invalid, resulting in a broken image url.

I've since updated the implementation to strip out the itok param (so we are no longer reliant on suppress_itok_output). I had to use hook_preprocess_image_style.
The main reason I did this is because suppress_itok_output is a global setting, and as a site can run multiple filesystems, which could combine local with S3. So if anything, it would have been useful to have more granular control over suppress_itok_output, i.e. only apply it on certain urls.

Actually, using hook_preprocess_image_style to strip out the itok param is not a great solution. As image style urls don't always run through this function.

We're therefore reverting back to using suppress_itok_output.
If this setting was to go, it would be nice if there was a way to modify the image style url. Currently the only way I could see was to override the ImageStyle entity definition, which is not really ideal. Maybe if something like #2924796: Create a generic way to decorate Entity classes landed, it would be a lot easier.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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