"Redirect after login to destination URL" no longer works

Created on 9 February 2024, 5 months ago
Updated 23 February 2024, 4 months ago

Problem/Motivation

I believe πŸ› RuntimeException: Failed to start the session because headers have already been sent Fixed #3246475: Cache Metadata Leak in UserLoginController β†’ introduces a bunch of exit; calls to mitigate an issue relating to metadata leakage.

However, this isn't advisable especially in an OOP context like Symfony, this essentially short-circuits other actions that Drupal or other modules might need to run during the login process such as setting a temporary session to store the destination URL it needs to redirect back to etc.

Steps to reproduce

Enable the "Redirect after login to destination URL" option in /admin/config/system/o365/settings/sso

Then attempt to login to a page using /o365/login/custom_connector?destination=/my-destination, it should redirect to /my-destination.

Proposed resolution

Remove all calls to exit and $response->send(); in the codebase, returning just the response object and instead handle the metadata bubbling context properly.

Remaining tasks

Provide issue fork/patch.

User interface changes

N/A

API changes

N/A

πŸ› Bug report
Status

Needs review

Version

5.0

Component

Code

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @codebymikey
  • Pipeline finished with Success
    5 months ago
    Total: 143s
    #91413
  • Status changed to Needs review 5 months ago
  • Initially thought of adding a wrapper around the code which returns the cacheable responses using a helper function like this:

    /**
     * Returns a trusted response.
     *
     * Taking into account any bubbleable metadata generated.
     *
     * @param callable $callable
     *   The callable to resolve the relevant response.
     *
     * @return \Drupal\Core\Cache\CacheableResponseInterface|mixed
     *   The response.
     */
    protected function wrapResponse(callable $callable) {
      $render_context = new RenderContext();
      $cacheable_metadata = new BubbleableMetadata();
      $response = $this->renderer->executeInRenderContext($render_context, $callable);
      if ($response) {
        if (!$render_context->isEmpty()) {
          $metadata = $render_context->pop();
          $cacheable_metadata->addCacheableDependency($metadata);
        }
        if ($response instanceof CacheableResponseInterface) {
          $response->addCacheableDependency($cacheable_metadata);
        }
        elseif (is_array($response)) {
          BubbleableMetadata::createFromRenderArray($response)
            ->merge($cacheable_metadata)
            ->applyTo($response);
        }
      }
    
      return $response;
    }
    

    But it's probably not necessary, and makes the code a bit more ugly to maintain, so opted for stopping the specific metadata we're aware of from leaking.

    I added a 3420422-on-3295745-redirect-support branch for supporting redirects in a way that's compatible with πŸ“Œ Make it possible to use multiple Office 365 connectors (5.x) RTBC , as well as a static patch that's compatible with #18 πŸ“Œ Make it possible to use multiple Office 365 connectors (5.x) RTBC (3.0.25)

  • πŸ‡³πŸ‡±Netherlands batigolix Utrecht
Production build 0.69.0 2024