ImageStyleDownloadController routes do not limit schemes served

Just been looking at https://www.drupal.org/project/drupal/issues/3058071 - which is similar - at moment it is always public for all streams that aren't the "private" one, so maybe that's a separate issue there as the advisory fixed only the Image style controller.

And it looks like in this patch there becomes a strange behaviour:
* With image derivatives turned on, it properly obeys the route "public" setting and only serves the correct stream
* With image derivatives turned off, it will ignore the route "public" setting, and instead check the additional public streams setting

Should this patch instead use the "public" setting from the route in both cases? Perhaps either deprecating the additional public streams setting in the process, or if that is needed for API compatibility, having the ability to detect "no public setting added" and obey the additional public streams?

Brought this up in slack in the #needs-review-queue-initiative channel and @cmlara talked about rescoping, #8

+1 from me but will this need a change record now? If not think it can be marked RTBC.

Thanks for the quick change record!

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -140,6 +142,10 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
 
+    if ($required_derivative_scheme !== $derivative_scheme) {
+      throw new AccessDeniedHttpException();
+    }

I think we should add an exception message here explaining why access is denied. If someone gets here for non-nefarious reasons it would help them to work out why.

Addressed the suggestion from #18

Think we want a different type of error message.

Brought this up in slack #needs-review-queue-initative channel

From @catch

Was thinking more like the scheme for the image style doesn't match the scheme for the original image

From @cmlara

I’ll note that derivatives can be on a different scheme than the source image (read-only streamWrappers)
Any message we put on there really is for DX IMHO, as image styles links should always be built by buildUrl() this means only developer error or probing from a user should ever trigger it.

Changing exception message as per suggestions from #20.

Think the message is better now.

Though since changing it didn't break anything I wonder if we should add an assertion testing for the message. Will let the committer decide if that's overkill or not.

Crediting @effulgentia

I've pinged security team folks in slack to ask some questions about this

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -106,7 +108,7 @@ public static function create(ContainerInterface $container) {
+  public function deliver(Request $request, $scheme, ImageStyleInterface $image_style, string $required_derivative_scheme) {

Should we make this optional for now, defaulting to e.g. public - is a BC break otherwise

Crediting @benjifisher from the security team for pointing out the above

@cmlara:

The link in #31 is missing a '/' in 'aboutcore'. If I add that and scroll up the page, I see the paragraph,

The Drupal core deprecation policy applies to both public and internal APIs: wherever possible, old APIs should be deprecated for removal in the next major version instead of being removed immediately. For public APIs, the deprecation process is a requirement; for internal APIs, we provide BC and deprecation where possible, but breaking changes may be made in situations where BC is not possible or has negative consequences. (Even for internal APIs, core contributors should always try to follow the deprecation process first, or document in issue discussions why deprecation is not used.)

As you say, the BC layer is not a requirement in this case, but I think the "where possible" part applies.

One thing that bothers me about this issue is that I do not see any discussion of whether this is the simplest/safest approach. For example, could the page controller figure out the expected scheme without being given an extra parameter? Maybe not, but I would like to document that here on the issue.

This has impacts on any module that extends the existing controller or attempts to implement a route based on the controller.

I'm not sure between the various options, but just in principle we don't need to provide backwards compatibility for controllers. https://www.drupal.org/about/core/policies/core-change-policies/bc-polic...

So even if that breaks a contrib or custom module, that'd be fine with a change record and release note.

The same with route definitions too to be honest.

We try not to go out of our way to break things like this, but if it's necessary there is nothing stopping us from doing it.

So taking a look I see there is now a new parameter that could be used in the service, but the CR doesn't have any code example of that and when to use. Think the CR could be tweaked some to be more clear.

Thanks

CR reads well. Lets see what the committers think about the need for BC or not.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Hiding files and converted #36 to an MR.

Please do not credit me if I did not other wise earn it as all I did was convert this.

Restoring status

Fixed a typo and a few readability issues in the IS.

Also verified all changes from the patch are in the MR. So the re-rtbc is double checked 🙂

I read the issue summary and comments. Thanks for the clear issue summary, it made reading the comments much easier. I did not find any unanswered questions.

@BramDriesen, thank you for confirming the change from patch to MR is correct.

The only think I can think of is that the release note snippet should mention the possible BC break.

Committed fd36154 and pushed to 11.x. Thanks!

Not backportable due to 10.2.x due to the minor BC break. Updated the change record to more explicitly note this is a BC break for anyone using the controller as suggested by @quietone, and published it.

Publish the CR

Unfortunately, this is a breaking change in 10.3.x, isn't it?

As noted in #3450244: Provide BC for ImageStyleDownloadController, we need to do a better job of explaining this in its release note (which should also link the CR).

Gave it a shot but not sure best spot to place in

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.

Thanks @smustgrave!

Updated the IS with what we eventually settled on here. Thanks!

I have a situation where images are stored to s3 with flysystem_s3 but thumbnails are stored to local public files folder. After updating from 10.2.7 to 10.3.6 thumbnails start to fail due to this scheme check

if ($required_derivative_scheme !== $derivative_scheme) {
      throw new AccessDeniedHttpException("The scheme for this image doesn't match the scheme for the original image");
    }

Original image is s3 scheme and thumbnail is public. Any ideas how can I keep my thumbnails in public files while actual resources are in s3?

Probably best to create a new ticket and reference this one instead of commenting on a closed/fixed issue.

Amending attribution.