The #2946: Login fails and no warning is issued if cookies are not enabled causes TrustedRedirectResponse::isSafe() to return false when formerly it always returned true for the same configuration.

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: It is not safe to redirect to https://site.com?check_logged_in=1 in Drupal\Component\HttpFoundation\SecuredRedirectResponse->setTargetUrl() (line 58 of core/lib/Drupal/Component/HttpFoundation/SecuredRedirectResponse.php).
Drupal\user\Authentication\Provider\Cookie->addCheckToUrl(Object, 'kernel.response', Object)
call_user_func(Array, Object, 'kernel.response', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.response') (Line: 191)
Symfony\Component\HttpKernel\HttpKernel->filterResponse(Object, Object, 1) (Line: 179)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 49)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Configuration:

  1. session.storage.options.cookie_domain = .site.com
  2. trusted_host_patterns = ['^api\\.site\\.com$', '^site\\.com$']

The login process (SSO):

  1. https://site.com - visited by a user.
  2. https://REDACTED.auth.eu-west-1.amazoncognito.com/oauth2/authorize?state... - visited by a user by hitting a login link.
  3. https://api.site.com/auth/cognito/frontend?code=CODE&state=STATE - redirected automatically after signing in.
  4. https://site.com - redirected by the TrustedRedirectResponse within a custom module that handles Cognito authorization.

On the final step, the TrustedRedirectResponse::$trustedUrls has ['https://site.com'] while the redirect URL is modified to https://site.com?check_logged_in=1. This causes it to be considered unsafe and not local because the Drupal instance lives on https://api.site.com.

Comments

BR0kEN created an issue. See original summary.

br0ken’s picture

Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new1.16 KB
br0ken’s picture

Issue tags: -Needs tests
StatusFileSize
new2.61 KB
new3.76 KB
catch’s picture

Priority: Normal » Major
Issue tags: +Bug Smash Initiative

Patch looks good to me, bumping to major.

murilohp’s picture

StatusFileSize
new3.76 KB
new1.22 KB

The patch looks good! It applied without any problems, I just have one minor suggestion, inside the UserAuthTest, you've used static calls to valid the assert and the expect for the mocks, I think for this scenario, it's a good idea to use "$this" instead of "static::", this way we keep the consistency of the code, since all the methods are using "$this->{method}".

The following patch is the suggestion that I've mentioned, I'll keep to needs to review to see if anyone has any other ideas.

Thanks!

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This also looks good to me - nice fix, test is good too. Agree with #5 that we use $this-> for PHPUnit even though the methods are static.

  • catch committed 6033fd0 on 10.0.x
    Issue #3253889 by BR0kEN, murilohp, longwave: `?check_logged_in=1`...

  • catch committed f83161f on 9.4.x
    Issue #3253889 by BR0kEN, murilohp, longwave: `?check_logged_in=1`...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good here too. Committed/pushed to 10.0.x and cherry-picked to 9.4.x and 9.3.x, thanks!

  • catch committed 51d7a6c on 9.3.x
    Issue #3253889 by BR0kEN, murilohp, longwave: `?check_logged_in=1`...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.