Harden redirect responses to make external URIs opt in (was SA-CORE-2015-002 foward-port)

For reference, the Drupal 7 patch and tests can be found here:
http://cgit.drupalcode.org/drupal/commit/?id=5cb79b4b217e9aa315d61284398...

(the relevant changes are to the field_ui module)

Credit for that Drupal 7 fix (from https://www.drupal.org/SA-CORE-2015-002) is: yched, DamienMcKenna, Pere Orga, David_Rothstein, klausi

Does that mean that \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl does not effectively protected against redirects to external URLs?

I think this should be the proper place to fix it.

@Tim, yes but Field UI sets $form_state->setRedirectUrl(), which eventually then becomes a RedirectResponse.

Even if we don't do the check in RedirectResponseSubscriber, I'd expect us to do it in FormState no?

Right so we do set it as part of core/modules/field_ui/src/Form/FieldStorageAddForm.php:422 for example.

Somewhere, like in FinishResponseSubscriber or somewhere else we could explicitly check and then maybe allow to opt IN for redirects across domains, but by default we should really not allow it. Maybe opt in could be done using a custom subclass of the RedirectResponse class, which we could set a new property on.

What about this more deep in protected outside of FAPI? This even protects against custom code setting RedirectResponse objects ...

This time with the entire patch ..

Expanded and fixed the test coverage as well as merged in https://www.drupal.org/files/issues/2507831-field_ui-9.patch

Let's fix things ...

I like the opt-in at the response level a lot.

Adapted the IS

Reading the new test coverage, that brings up another issue.

It's a bit obscure, but I've seen the following before on a high traffic Drupal 7 site.

- Had several web servers behind a load balancer
- The load balancer was configured to take a web server out of rotation if it kept returning 500s
- image derivatives for various reasons including #927138: Fail image generation with 404 instead of 500, when source file is missing would return 500s to crawlers and/or real users
- all of the webservers would be taken out then put back into rotation, bringing the site down.

This would require a form submit, but for example a site with anonymous commenting you could script something to trigger enough 500s to do that.

So given all of that, I think we should do something other than throw the exception, like a trigger_error() and maybe ignoring the redirect altogether for the end user.

Mh, so you should never be able to trigger 500s if possible at least not in an easy way?

Well I think that load balancing configuration should probably not be used for this reason - something like varnish health checking is much better. But that particular problem made me paranoid about allowing any kind of regular user to trigger 500s easily, yes.

Also in this case the site isn't broken at all, just someone's visited a naughtily crafted link, so 500 semantically isn't the correct error code anyway.

Reading through the list on https://developer.mozilla.org/en-US/docs/Web/HTTP/Response_codes I think 400 is the most appropriate one.

Anything else left here then?

Not much from me:

1. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
   @@ -617,6 +617,19 @@ function testDuplicateFieldName() {
   +
   

   400. Assuming tests will fail but making note just in case.

2. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -101,6 +100,96 @@ public static function providerTestDestinationRedirect() {
   +  }
   +
   +  public function testRedirectWithOptInExternalUrl() {
   +    $dispatcher = new EventDispatcher();
   +    $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
   +    $response = new RedirectResponseToExternalUrl('http://external-url.com');
   +    $url_generator = $this->getMockBuilder('Drupal\Core\Routing\UrlGenerator')
   +      ->disableOriginalConstructor()
   

   Missing docblock.

400. Assuming tests will fail but making note just in case.

Assuming you are right here.

I'm not really happy about the solution to try catch the entire thing, but well, this at least catches the problems.

Interesting approach. But I think RedirectResponseToExternalUrl is a confusing name, because you would actually use it whenever you have trusted input (that might be external), not just when you know it is external.

It would be easier to name and more backwards-compatible (although a little less comprehensive security-wise) if it were the other way around, e.g. use RedirectResponseInternal when you know that an external URL should not be allowed. This approach would be worth considering for Drupal 7 backport.

+        if (UrlHelper::isExternal($url) && !UrlHelper::externalIsLocal($url, $this->requestContext->getCompleteBaseUrl()) && !$response instanceof RedirectResponseToExternalUrl) {

This would be better if the "instanceof" check were first in the list.

It would be easier to name and more backwards-compatible (although a little less comprehensive security-wise) if it were the other way around, e.g. use RedirectResponseInternal when you know that an external URL should not be allowed.

I think we explicitly want to break redirects to external URLs, and require existing and new code to opt-in here, at least for 8.x.

What about RedirectReponseAllowExternalUrl for the name?

Mh, so the problem here is that we are dealing with code which is both triggered as part of the normal flow as well as exception handling.
During exception handling, in that case even at the end, it is kinda a bad idea to throw another exception.

Let's go with the approach of catch earlier, throw an error ...

This is the patch uploaded so far.

Comment to #33

It would be easier to name and more backwards-compatible (although a little less comprehensive security-wise) if it were the other way around, e.g. use RedirectResponseInternal when you know that an external URL should not be allowed. This approach would be worth considering for Drupal 7 backport.

I see, so this basically means, in every spot we create a response object coming from the destination(s) query string, we use that object. That seems to be good enough to cover at least core issues. I guess its still okay to be potentially less secure for some custom usecases, but well, its not less secure as HEAD.

You are concerned about the backward-compatible aspect. Do you feel strong about it? I agree for Drupal 7 be BC compliant is the way to go, but for Drupal 8 being safe first, seems to be the way to move forward. I think we could find a better name?
On the other hand, opt IN to protection would have a potential better support for 3rd party code dealing with redirect responses, not sure whether it matters a lot though.

I guess the backport would mean that we take the same approach. For query strings, we opt into a protection mechansim

RedirectReponseAllowExternalUrl

That seems indeed better!

"RedirectResponseAllowExternalUrl" is a mouthful, but works :)

I just thought Drupal 8 was aiming for backwards-compatibility at this point, otherwise don't really care about the opt-in vs opt-out myself.

On the other hand, opt IN to protection would have a potential better support for 3rd party code dealing with redirect responses, not sure whether it matters a lot though.

That's a good question, though, given that RedirectResponse is a Symfony thing, not a Drupal thing...

I just thought Drupal 8 was aiming for backwards-compatibility at this point, otherwise don't really care about the opt-in vs opt-out myself.

Fair point, I honestly prefer security first and a BC break for it, but I just call out to the framework maintainer hat of catch.

This fixes the failing test. We need to ensure that the specific error is thrown and then remove it from the list of active assertions.

Fair point, I honestly prefer security first and a BC break for it, but I just call out to the framework maintainer hat of catch.

Right I feel like here the current API is the root cause of the security issue, so changing the API to make it impossible to have open redirect by accident is the right choice.

On the other hand, opt IN to protection would have a potential better support for 3rd party code dealing with redirect responses, not sure whether it matters a lot though.

If third party code is providing a RedirectResponse and that's being handled by our RedirectResponseSubscriber, then I think opt-in is better here too. Some Drupal code must be integrating with that code to get the RedirectResponse back, and that can then convert it to RedirectResponseAllowExternalUrl if it needs to.

Well my point was that you can't write code which is agnostic of Drupal and redirect, because you'd need a dependency to Drupal.
I think for now its okay to live with that kind of limitation.

Given that external code needs some form of integration anyway I think its a small price to pay for better security by default.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -8,9 +8,13 @@
   +use InvalidArgumentException;
   

   Unused.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +65,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +        // In case someone setup a custom RedirectResponse object ensure that the
   +        // URL is not external. In case this is wanted, check for a custom flag
   +        // on RedirectResponse.
   +        $url = $response->getTargetUrl();
   +        if (!$response instanceof RedirectResponseAllowExternalUrl && UrlHelper::isExternal($url) && !UrlHelper::externalIsLocal($url, $this->requestContext->getCompleteBaseUrl())) {
   +          $this->setBadRequestException($event, 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\RedirectResponseAllowExternalUrl for it');
            }
   

   Can we make this a separate subscriber? Maybe rename the existing one to DestinationSubscriber and the new logic to either PreventOpenRedirectSubscriber or SafeRedirectSubscriber? That would allow projects concerned with #42 to swap out this new piece more easily.

3. +++ b/core/lib/Drupal/Core/Routing/RedirectResponseAllowExternalUrl.php
   @@ -0,0 +1,22 @@
   + * Provides a redirect response which allows to redirect to an external URl.
   

   s/URl/URL/

4. +++ b/core/lib/Drupal/Core/Routing/RedirectResponseAllowExternalUrl.php
   @@ -0,0 +1,22 @@
   +class RedirectResponseAllowExternalUrl extends RedirectResponse {
   

   Maybe rename to TrustedRedirectResponse?

Thinking more about RedirectResponseAllowExternalUrl/TrustedRedirectResponse, something else I'm uneasy with about this class is that it doesn't override setTargetUrl(). Which means one piece of code can create an instance with a truly trusted URL, and then another piece of code can call setTargetUrl() on it with user input, and then the protection added by this patch is bypassed.

For example, it's kind of an implementation detail that Field UI applies destinations at the point of instantiating a redirect response. It could just as easily have implemented that logic in a response event subscriber or similar, just like RedirectResponseSubscriber does for destination. To tell people "don't worry about validating user entered redirection URLs when you create a RedirectResponse, but do worry about it when you call ->setTargetUrl() on an already created one" seems worse than either saying "always validate yourself" or "never validate yourself".

In other words, if we want to fix this issue with the code that's in #44.2 (which I think is pretty compelling), then I think we should also remove the existing validation of destination and make sure #44.2 automatically covers that as well. Not sure if that means we should harden RedirectResponseAllowExternalUrl or if we want to ditch that as a special class and instead have a separate service for registering trusted external hosts and/or URLs.

Thinking more about RedirectResponseAllowExternalUrl/TrustedRedirectResponse, something else I'm uneasy with about this class is that it doesn't override setTargetUrl(). Which means one piece of code can create an instance with a truly trusted URL, and then another piece of code can call setTargetUrl() on it with user input, and then the protection added by this patch is bypassed.

In general I think we should not disallow the usage of setTargetUrl at all, on the other hand I get your point, we don't want to make it explicit that we require opting out from security. What about the following idea.

Can we make this a separate subscriber? Maybe rename the existing one to DestinationSubscriber and the new logic to either PreventOpenRedirectSubscriber or SafeRedirectSubscriber? That would allow projects concerned with #42 to swap out this new piece more easily.

We could actually get rid of the sanitation but personally I don't have a problem with having multiple level of security.

I agree with #45. While the proposed patch is great already, it also is encapsulating things in a (for me) difficult to understand manner.

However I don't think a white list of domains is correct either, because an attacker might have control over a specific path of that domain, but not on the originally set path.

I think making RedirectResponseAllowExternalUrl() immutable is a good idea (I am kinda getting fancy about immutable objects since PSR-7 ...).

That would prevent someone else from changing a trusted url into an untrusted one.

Train of thought

Not sure, if that is still possible, but the cleanest API change would be to always return a clone of the object in all set* methods (hopefully not performance critical), where RedirectResponseAllowExternalUrl would always use the default unsafe factory to create the new objects ... Just some thoughts.

Another possibility would be to encapsulate for example PSR-7 request objects and allow creating a redirect with that request object (possibly sub-class and add the SafeInterface to them), because a redirect in reality is kinda a new request ...

Supporting encapsulation of an immutable object also would be a way to solve this.

Not sure, if that is still possible, but the cleanest API change would be to always return a clone of the object in all set* methods (hopefully not performance critical), where RedirectResponseAllowExternalUrl would always use the default unsafe factory to create the new objects ... Just some thoughts.

Well, effecitly this is what the patch is doing. It disallows you to set untrusted URLs. But we need to keep in mind that there has to be an explicit method to opt out, in case you really want to.

I also don't think that doing that would increase the security, because well, if you want, you can already just use $event->setResponse() and do whatever you want.

It is just sad, how unit test failures look like.

It is just sad, how unit test failures look like.

+++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
@@ -0,0 +1,93 @@
+    if (!$trusted && UrlHelper::isExternal($url) && !UrlHelper::externalIsLocal($url, $this->getRequestContext()->getCompleteBaseUrl())) {
+      throw new \InvalidArgumentException('It is not possible to set an external URL using ::setTargetUrl, use setTrustedTargetUrl instead.');
+    }

I like this, except I'm not sure about throwing an exception. Would it be ok to simply return without setting the URL? Reason being: I think code (such as the subscriber that deals with destination) should be able to call ->setTargetUrl() without worrying about whether the object is a RedirectResponse or TrustedRedirectResponse. Or, if we want to throw an exception, is there some global code that can catch it, in order to allow such callers to not need to themselves?

Ignore #53. RedirectResponse::setTargetUrl() already has a defined API of throwing \InvalidArgumentException, so I think it's ok for callers to need to deal with that. I'm going to take a stab at improving this a bit more.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +66,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +            // The destination query parameter can be a relative URL in the sense
   

   80 cols.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +66,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +            // of not including the scheme and host, but its path is expected to
   +            // be absolute (start with a '/'). For such a case, prepend the
   

   Don't we call this "root-relative URLs"?

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +66,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +        // In case someone setup a custom RedirectResponse object ensure that the
   

   80 cols.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +66,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +   * We are not throwing a BadRequestHttpException because filterResponse might
   

   I think this is intended to be s/filterResponse/FilterResponseEvent/ ?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -61,39 +66,71 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
   +    trigger_error($message, E_USER_ERROR);
   

   I'm assuming this is here for debugging purposes?

6. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,93 @@
   + * Contains \Drupal\Core\Routing\RedirectResponse.
   

   Copy/paste error.

7. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,93 @@
   +use \Symfony\Component\HttpFoundation\RedirectResponse;
   

   Nit: remove the leading backslash.

8. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,93 @@
   + * Provides a redirect response which allows to redirect to an external URl.
   

   s/URl/URL/

9. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,93 @@
   +    $this->headers = new ResponseHeaderBag($headers);
   +    $this->setContent('');
   +    $this->setStatusCode($status);
   +    $this->setProtocolVersion('1.0');
   +    if (!$this->headers->has('Date')) {
   +      $this->setDate(new \DateTime(null, new \DateTimeZone('UTC')));
   +    }
   

   This feels quite weird. Is this copy/pasted from the base class? Or…?

10. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
    @@ -0,0 +1,93 @@
    +  }
    +
    +
    +  /**
    

    Nit: two newlines.

11. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
    @@ -0,0 +1,93 @@
    +   * The request context.
    +   *
    +   * @var \Drupal\Core\Routing\RequestContext
    +   */
    +  protected $requestContext;
    

    Should be moved to before the constructor.

12. +++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
    @@ -617,6 +617,31 @@ function testDuplicateFieldName() {
    +   * Tests that external URLs in the 'destinations' query parameter are blocked.
    ...
    +  public function testExternalDestinations() {
    ...
    +      'query' => ['destinations' => ['http://example.com']],
    

    Should this not be singular, i.e. destination, instead of plural?

#55.9: Yes, this is verbatim from Response::__construct(), little bad that we can't call the parents constructor here due to getting an Exception on the setTargetUrl() it would throw ...

#55.9: Yes, this is verbatim from Response::__construct(), little bad that we can't call the parents constructor here due to getting an Exception on the setTargetUrl() it would throw ...

Well, we could catch it.

My local work in progress addresses #55.9 (and I think some of the other points in #55). Will post what I have later today.

Here's what I have so far. Has some docs todos. And tests might fail. But curious what you all think about the changes.

Can we make this a separate subscriber? Maybe rename the existing one to DestinationSubscriber and the new logic to either PreventOpenRedirectSubscriber or SafeRedirectSubscriber?

I think we should also remove the existing validation of destination and make sure #44.2 automatically covers that as well.

This patch does neither of above, but paves the way to do so in a non-critical followup if we want to.

I really like the separation of logic into proper objects.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -56,78 +53,74 @@
   +        // The 'Location' HTTP header must always be absolute.
   +        $destination = $this->getDestinationAsAbsoluteUrl($destination, $request->getSchemeAndHttpHost());
   +        try {
   +          $response->setTargetUrl($destination);
   ...
   +        catch (\InvalidArgumentException $e) {
            }
   

   I would have expected that this code just always uses a LocalRedirectResponse

2. +++ b/core/lib/Drupal/Component/HttpFoundation/SafeRedirectResponse.php
   @@ -0,0 +1,46 @@
   +/**
   + * @todo Document.
   + */
   +abstract class SafeRedirectResponse extends RedirectResponse {
   

   Here would be perfect place to also explain what is unsafe in regard to external redirects

3. +++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
   @@ -0,0 +1,44 @@
   +class LocalRedirectResponse extends SafeRedirectResponse {
   ...
   +  protected function isSafe($url) {
   +    return UrlHelper::isExternal($url) && UrlHelper::externalIsLocal($url, $this->getRequestContext()->getCompleteBaseUrl());
   +  }
   

   That is a bit odd, so an actual relative URL would be not considered as safe? It seems to work simply because of the conversion logic in the subscriber. I think a local redirect response should be able to specify a relative path

Some changes. I also renamed SafeRedirectResponse to SecuredRedirectResponse, because its not necessarily "safe" yet.

Yeah not sure about the hierarchy of classes, but here is my thought:

• SecuredRedirectResponse ... the general framework to ensure that redirects are safe
• TrustedUrlResponse ... Its a secured redirect response which is already trusted # I'm curious whether it really makes sense to not have a TrustedExteranalUrlResponse and let custom code
  figure out whether a local or external is wanted.
• LocalUrlResponse ... This is a secured redirect because it just allows you to redirect to a local URL.

Ups.

Mh, so we have a test which ensure that you can't call out to Drupal\Core, this is nice, but it also ensures that it cannot be used in documentation :(

This failure is really nice. #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash fixed the Url object itself ... so we now have a proper validation error.

This is pretty close in my book

1. +++ b/core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php
   @@ -0,0 +1,60 @@
   +   * @param string $url
   

   Nitpick, missing a doc according to our standard, not sure it needs it though

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,75 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +   * @todo Document.
   

   :)

3. +++ b/core/lib/Drupal/Core/Routing/SecuredRedirectResponseTrait.php
   @@ -0,0 +1,58 @@
   + * Class SecuredRedirectResponseTrait
   + * @package Drupal\Core\Routing
   

   This is the first time I've seen @package - can you point me to some docs on it's use - or is it just a default docblock added by an IDE?

4. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,57 @@
   +   * @todo Document.
   

   :)

5. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -87,12 +108,9 @@ public function testDestinationRedirect(Request $request, $expected) {
   -      array(new Request(array('destination' => '/example.com')), 'http://example.com/example.com'),
   

   Any reason to rework this test case?

This is the first time I've seen @package - can you point me to some docs on it's use - or is it just a default docblock added by an IDE?

Well, this is afaik the way how phpstorm autofills stuff.

Any reason to rework this test case?

Well, I think it makes sense to split up the case of an external redirect ...

This failure is caused by #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash being reverted ...

I also renamed SafeRedirectResponse to SecuredRedirectResponse, because its not necessarily "safe" yet.

We throw an exception in setTargetUrl(), so I don't get what you mean by "not necessarily safe yet", but regardless, +1 to the rename.

I think a local redirect response should be able to specify a relative path

Doing so violates the HTTP spec, but I think all browsers can handle it and Symfony's RedirectResponse allows it, so I'm fine with your relaxing of the isSafe() check.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,84 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +        catch (\InvalidArgumentException $e) {
   +          // If the above failed, it's because the redirect target wasn't
   +          // local. Do not follow that redirect. Display an error message
   +          // instead. We're already catching one exception, so trigger_error()
   +          // rather than throw another one.
   +          $message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.';
   +          trigger_error($message, E_USER_ERROR);
   +          $safe_response = new Response($message, 400);
   +        }
   

   I wrote this comment, but I think it might be wrong. It's ok to rethrow exceptions, so why exactly are we using trigger_error() here?

2. +++ b/core/lib/Drupal/Core/Routing/SecuredRedirectResponseTrait.php
   @@ -0,0 +1,57 @@
   +  protected function isSafe($url) {
   +    return !UrlHelper::isExternal($url) || UrlHelper::externalIsLocal($url, $this->getRequestContext()->getCompleteBaseUrl());
   +  }
   

   What about renaming the trait to LocalRedirectResponseTrait or LocalAwareRedirectResponseTrait and the method to isLocal()? Then classes using this trait can implement isSafe() as calling isLocal() or doing other logic as they see fit.

@effulgentsia see #21 for the trigger_error().

Doing so violates the HTTP spec, but I think all browsers can handle it and Symfony's RedirectResponse allows it, so I'm fine with your relaxing of the isSafe() check.

Let's open up a follow up for that: #2514318: Ensure that all redirect responses comply to http spec

What about renaming the trait to LocalRedirectResponseTrait or LocalAwareRedirectResponseTrait and the method to isLocal()? Then classes using this trait can implement isSafe() as calling isLocal() or doing other logic as they see fit.

Let's do that.

Ups

Let's fix that as well.

Leaving at needs-review:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,84 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +          // instead. We're already catching one exception, so trigger_error()
   +          // rather than throw another one.
   

   Nit - I think we should write we avoid throwing an exception, to avoid making legitimate pages being cached as 500 errors.

2. +++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
   @@ -0,0 +1,22 @@
   +  use LocalAwareRedirectResponseTrait {
   +    LocalAwareRedirectResponseTrait::isLocal as isSafe;
   +  }
   

   I wasn't aware of that syntax, nice! :)

3. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
   @@ -87,12 +108,9 @@ public function testDestinationRedirect(Request $request, $expected) {
   -      array(new Request(array('destination' => 'http://example.com')), FALSE),
   -      array(new Request(array('destination' => 'http://example.com/foobar')), FALSE),
   -      array(new Request(array('destination' => 'http://example.ca/drupal')), FALSE),
          array(new Request(array('destination' => 'test')), 'http://example.com/drupal/test'),
   -      array(new Request(array('destination' => '/test')), 'http://example.com/test'),
   -      array(new Request(array('destination' => '/example.com')), 'http://example.com/example.com'),
   

   My feeling is we are loosing test coverage here.

   While later we test legitimate external URLs we never check for the wrong case.

   At least I was not able to find them ...

Thank you for your review!

Nit - I think we should write we avoid throwing an exception, to avoid making legitimate pages being cached as 500 errors.

Good point.

At least I was not able to find them ...

Fair point, added the once from providerTestDestinationRedirectToExternalUrl() as well.

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
@@ -87,12 +108,9 @@ public function testDestinationRedirect(Request $request, $expected) {
-      array(new Request(array('destination' => 'http://example.com')), FALSE),
-      array(new Request(array('destination' => 'http://example.com/foobar')), FALSE),
-      array(new Request(array('destination' => 'http://example.ca/drupal')), FALSE),

@@ -101,7 +119,94 @@ public static function providerTestDestinationRedirect() {
+      'absolute external url' => [new Request(['destination' => 'http://example.com']), 'http://example.com'],
+      'absolute external url with folder' => [new Request(['destination' => 'http://example.com/foobar']), 'http://example.com/foobar'],
+      'absolute external url with folder2' => [new Request(['destination' => 'http://example.ca/drupal']), 'http://example.ca/drupal'],

@@ -128,6 +235,11 @@ public function providerTestDestinationRedirectWithInvalidUrl() {
+    $data['absolute external url'] = [new Request(['destination' => 'http://example.com'])];
+    $data['absolute external url with folder'] = [new Request(['destination' => 'http://example.ca/drupal'])];

So don't we need any tests with:

http://attacker.com?

I just wonder ...

What is the difference between http://attack.com and 'http://example.ca/drupal' given that the complete base URl is 'http://example.com/drupal' ?

I'm unable to reproduce the vulnerability using 8.0.x HEAD:

When I go to /admin/structure/types/manage/page/fields/node.page.body?destinations[0]=http://google.com, change something and click "Save settings" the page breaks with the following error:

Error
Saved Body configuration.
The website encountered an unexpected error. Please try again later.

When I go to /admin/structure/types/manage/page/fields/node.page.body/storage?destinations[0]=http://google.com, change something and click "Save field settings" the page does not break but gives the following messages:

Updated field Body field settings.
Attempt to update field Body failed: The internal path component "http://google.com" is external. You are not allowed to specify an external URL together with internal:/..

I get exactly the same behaviour when applying patch in #83.

Is the "destinations" functionality (note the plural) worth keeping it?

When I go to /admin/structure/types/manage/page/fields/node.page.body?destinations[0]=http://google.com, change something and click "Save settings" the page breaks with the following error:

The reason for that is #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash which ensured that you our url generation machanisms are safe.
On top of that this issue ensures that even our redirect system disallows you to redirect in the first place.

Is the "destinations" functionality (note the plural) worth keeping it?

I totally think its worth keeping, given that otherwise adding a field would be a major pain, as there are more than 2 steps involved in that process.

Just to be clear, yes the issue linked above #2501655: ConfirmFormHelper::buildCancelLink() incorrectly handles ?destination URLs with a leading slash
causes this exception.

InvalidArgumentException: The internal path component "http://google.com" is external. You are not allowed to specify an external URL together with internal:/. in Drupal\Core\Url::fromInternalUri() (line 431 of core/lib/Drupal/Core/Url.php).

But, well, this issue fixes all kind of possible redirect issues, but you could argue with me to move this to major.

Just discussed this at some length with @dawehner in irc.

The link in #88 was a different issue, the issue that added the exception was #2512452: Confirm form cancel button can lead to external domain.

After that issue, attempting to exploit the original Field UI bug, you get the exception instead of the open redirect. So we've already fixed the security issue here as a by-product of that patch with Url:Helper:fromInternalUri() hardening. That's good.

However, showing exceptions to users only due to user input (i.e. nothing is wrong with the site, so it should not be any kind of 500 error) is not good, so I opened #2521836: Ensure all callers of UrlHelper::fromInternalUri() either operate on validated user input, or catch InvalidArgumentException as a follow-up to handle that exception properly.

That leaves the hardening added here, which would additionally prevent open redirects. I think that is definitely worth doing, and per the earlier discussion I think the small API break (external URI opt-in) is worth doing any time up until release candidate (although obviously we can still discuss that - but this will be even less disruptive if we don't introduce the break).

However as hardening, and given UrlHelper:fromInternalUri() hardening already caught this case independently, it's really 'just' defense in depth at this point and not fixing any specific vulnerability, including this one.

So.. re-titling this issue to focus on the hardening rather than the original SA, and downgrading to major.

To be clear though, the patch still exactly looks like how it should look like and is totally fine IMHO.

So reviews are still welcomed!

I still think the Exception case should be added to test-coverage as well (i.e. what had been FALSE before - or maybe I just don't see it), but I am also fine for it to go in without.

For me it is RTBC already - except for that thing.

I still think the Exception case should be added to test-coverage as well (i.e. what had been FALSE before - or maybe I just don't see it), but I am also fine for it to go in without.

Well, we have dedicated test coverage farther below in the test file .... we just split it up now.

Looking good to me. Only able to find nits.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,86 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +          // We don't throw an exception, because we dont' want to cache 500
   

   Nit: s/dont'/don't/

   And "cache 500 pages"? Not sure what that means.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,86 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +      // The destination query parameter can be a relative URL in the sense
   +      // of not including the scheme and host, but its path is expected to
   +      // be absolute (start with a '/'). For such a case, prepend the
   +      // scheme and host, because the 'Location' header must be absolute.
   

   Nit: 80 cols.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
   @@ -51,46 +53,86 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
   +        // Legacy destination query parameters can be relative paths that
   +        // have not yet been converted to URLs (outbound path processors
   +        // and other URL handling still needs to be performed).
   

   Nit: 80 cols.

4. +++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
   @@ -0,0 +1,57 @@
   + * Use this class in case you know that you want to redirect to an external
   + * URL.
   

   Nit: 80 cols.

#93: RTBC then :)

I think this issue needs a CR. If you want to redirect to external url with this patch you have to do extra work.

diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
index d250723..b9f121a 100644
--- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
@@ -55,9 +55,9 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
     if ($response instanceOf RedirectResponse) {
       $request = $event->getRequest();
 
-      // Let the 'destination' query parameter override the redirect target.
-      // If $response is already a SecuredRedirectResponse, it might reject the
-      // new target as invalid, in which case proceed with the old target.
+      // Let the 'destination' query parameter override the redirect target. If
+      // $response is already a SecuredRedirectResponse, it might reject the new
+      // target as invalid, in which case proceed with the old target.
       $destination = $request->query->get('destination');
       if ($destination) {
         // The 'Location' HTTP header must always be absolute.
@@ -80,12 +80,11 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
           $safe_response->setRequestContext($this->requestContext);
         }
         catch (\InvalidArgumentException $e) {
-          // If the above failed, it's because the redirect target wasn't
-          // local. Do not follow that redirect. Display an error message
-          // instead. We're already catching one exception, so trigger_error()
-          // rather than throw another one.
-          // We don't throw an exception, because we dont' want to cache 500
-          // pages.
+          // If the above failed, it's because the redirect target wasn't local.
+          // Do not follow that redirect. Display an error message instead.
+          // We're already catching one exception, so trigger_error() rather
+          // than throw another one. We don't throw an exception, because we
+          // don't want to cache 500 pages.
           $message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.';
           trigger_error($message, E_USER_ERROR);
           $safe_response = new Response($message, 400);
@@ -109,17 +108,17 @@ public function checkRedirectUrl(FilterResponseEvent $event) {
    */
   protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) {
     if (!UrlHelper::isExternal($destination)) {
-      // The destination query parameter can be a relative URL in the sense
-      // of not including the scheme and host, but its path is expected to
-      // be absolute (start with a '/'). For such a case, prepend the
-      // scheme and host, because the 'Location' header must be absolute.
+      // The destination query parameter can be a relative URL in the sense of
+      // not including the scheme and host, but its path is expected to be
+      // absolute (start with a '/'). For such a case, prepend the scheme and
+      // host, because the 'Location' header must be absolute.
       if (strpos($destination, '/') === 0) {
         $destination = $scheme_and_host . $destination;
       }
       else {
-        // Legacy destination query parameters can be relative paths that
-        // have not yet been converted to URLs (outbound path processors
-        // and other URL handling still needs to be performed).
+        // Legacy destination query parameters can be relative paths that have
+        // not yet been converted to URLs (outbound path processors and other
+        // URL handling still needs to be performed).
         // @todo As generateFromPath() is deprecated, remove this in
         //   https://www.drupal.org/node/2418219.
         $destination = UrlHelper::parse($destination);
diff --git a/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
index d2b8690..d8ad719 100644
--- a/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
+++ b/core/lib/Drupal/Core/Routing/LocalRedirectResponse.php
@@ -8,7 +8,6 @@
 namespace Drupal\Core\Routing;
 
 use Drupal\Component\HttpFoundation\SecuredRedirectResponse;
-use Drupal\Component\Utility\UrlHelper;
 
 /**
  * Provides a redirect response which cannot redirect to an external URL.
diff --git a/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
index c7a1e49..c7a43a2 100644
--- a/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
+++ b/core/lib/Drupal/Core/Routing/TrustedRedirectResponse.php
@@ -12,8 +12,7 @@
 /**
  * Provides a redirect response which contains trusted URLs.
  *
- * Use this class in case you know that you want to redirect to an external
- * URL.
+ * Use this class in case you know that you want to redirect to an external URL.
  */
 class TrustedRedirectResponse extends SecuredRedirectResponse {
 

Above are all the fixes I would make on commit...

Fixed the points + added the change record: https://www.drupal.org/node/2532212

Tweaked CR a little.

+++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php
@@ -51,46 +53,86 @@ public function __construct(UrlGeneratorInterface $url_generator, RequestContext
+          // rather than throw another one.
+          // We don't throw an exception, because we don't want to cache 500
+          // pages.

I updated this on commit because I don't think it's about caching - just changed to 'because it's a client error rather than a server error'.

Committed/pushed to 8.0.x, thanks!

I went ahead and published the change notice.

So we had tagged this for possible Drupal 7 backport - do we want to add something like drupal_goto_trusted() or drupal_goto_internal() in Drupal 7?