Broken redirect from RouteNormalizerRequestSubscriber when ajax_page_state present and $request->overrideGlobals() has been called

Created on 10 March 2024, 9 months ago
Updated 15 April 2024, 7 months ago

Problem/Motivation

Originally reported to Cloudflare module in 🐛 AJAX calls break on Drupal 10.2 when using restore client ip + redirect module RTBC but cross posted relevant details here in case it is appropriate to solve here.

RouteNormalizerRequestSubscriber uses $request->server->get('QUERY_STRING') as part of a comparison to determine if a route should be normalized.

I note a comment in the file around why $request->query->all() is not used

// Don't pass in the query here using $request->query->all()
// since that can potentially modify the query parameters.

However $request->server->get('QUERY_STRING') is also not immutable - if anything prior calls $request->overrideGlobals() then any query parameters modified are also present here.

This is an issue in 10.2 as after Libraries item in AJAX page state is compressed the query will contain the uncompressed libraries - so if anything has called $request->overrideGlobals() after the AjaxPageState middleware then the QUERY_STRING param will have the uncompressed libraries here - and redirect module will use this to redirect to a URL with the uncompressed url params - which breaks the page as the libraries can not be parsed in the following request as the URL param is no longer compressed.

Steps to reproduce

  1. Add code that is executed after AjaxPageState middleware and before RouteNormalizerRequestSubscriber that calls overrideGlobals on the request*
  2. Add ajax to any view
  3. Observe ajax is broken as ajax page state can not be parsed

* Currently from a search it looks like cloudflare and reverse_proxy_header call overrideGlobals - it may be present elsewhere and with other combinations of modified parameters.

Proposed resolution

It is possible to get the original query string via parse_url($request->getRequestUri(), PHP_URL_QUERY) - its not immediately clear from the Symfony code if this is intentionally preserving the original query / why overrideGlobals modifies some server params and not others.

My colleague also queried if the RequestChecker service should return FALSE from canRedirect if the redirect contains the ajax_page_state query parameter.

Remaining tasks

  1. Decide if this issue should be resolved by modifying RouteNormalizerRequestSubscriber
  2. Replace usage of $request->server->get('QUERY_STRING') with parse_url($request->getRequestUri(), PHP_URL_QUERY)
  3. Add test coverage for modified query params and overrideGlobals

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Needs review

Version

1.0

Component

Code

Created by

🇳🇿New Zealand ericgsmith

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

Merge Requests

Comments & Activities

  • Issue created by @ericgsmith
  • Merge request !88Issue #3426938: Add test to show bug → (Open) created by ericgsmith
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update 9 months ago
    56 pass, 2 fail
  • Status changed to Needs review 9 months ago
  • 🇳🇿New Zealand ericgsmith

    Added a test to show the error plus the a potential solution so setting to needs review although I note in the issue summary that there may be other ways to resolve this issue that I am keen to here ideas on.

  • 🇳🇿New Zealand ericgsmith

    Test is failing as method only exists from 10.1 onwards

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.x + Environment: PHP 8.1 & MySQL 5.7
    last update 9 months ago
    62 pass, 2 fail
  • Status changed to RTBC 8 months ago
  • 🇦🇺Australia sonnykt Melbourne, Australia

    Using MR88 as a composer patch fixes the issue on our Drupal 10.2.4 site.

                "drupal/redirect": {
                    "Broken redirect from RouteNormalizerRequestSubscriber when ajax_page_state present and $request->overrideGlobals() has been called - https://www.drupal.org/project/redirect/issues/3426938#comment-15483275": "https://git.drupalcode.org/project/redirect/-/merge_requests/88.diff"
                },
    
  • First commit to issue fork.
  • 🇨🇭Switzerland berdir Switzerland

    @sonnykt: You must not use merge request URL's as patches. anyone can commit code to that that will then run on your site. Always tore them as local files.

    I've disabled D9 testing in the meantime, so I guess this will no longer fail. A bit unsure about committing this without raising the Drupal required core version, but it's just a test. Could add a check if that function exists and then skip the test.

  • Status changed to Needs work 8 months ago
  • 🇨🇭Switzerland berdir Switzerland

    Tests fail on 10.1 as well, that's definitely not OK. Fine to skip the test if it's a 10.2+ problem.

  • Pipeline finished with Success
    8 months ago
    Total: 405s
    #144479
  • Status changed to Needs review 8 months ago
  • 🇳🇿New Zealand ericgsmith

    @Berdir Thanks - I have updated the test - it was failing on 10.1 as AjaxPageState was introduced in 10.2 not 10.1 as I previously noted incorrectly in my comment.

    I think the bug is still relevant for 10.1 if other code is modifying request parameters - I've added a check to the test to see if AjaxPageState exists and to modify the request params in the test if it doesn't - so test runs and passes on both current and previous minor.

  • 🇳🇿New Zealand RoSk0 Wellington

    Added couple wording suggestions, but otherwise looks good to me!

    +1 RTBC

  • Pipeline finished with Success
    7 months ago
    Total: 209s
    #147507
Production build 0.71.5 2024