Problem/Motivation

If the database connection is interrupted momentarily, and a database exception is thrown, there is a set of circumstances where a redirect to the installer page could be stored into the page cache if it uses a non-database storage. This will persist and redirect all users for a specific page to the installer page until the cache is cleared.

Steps to reproduce

Difficult to reproduce but with a specific forced failure point the problematic code paths can be demonstrated. Albeit, this forced failure is a specific exception that is unlikely to occur and the real cause is slightly more involved and explained after the steps to reproduce quickly.

1. Install page_cache and configure it to cache for 24 hours
2. Edit user module src/Form/UserLoginForm.php and throw an instance of \Drupal\Core\Database\DatabaseNotFoundException
3. Access /user/login using a CLI tool that shows headers
4. Note that X-Drupal-Cache: MISS and you receive a redirect
5. Access /user/login again
6. Note that X-Drupal-Cache: HIT and you receive the same redirect

Main contributor to the issue

The subscriber at core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php will see a redirect to the installer and see that it is not a SecuredRedirectResponse and will subsequently convert it to a LocalRedirectResponse. Unfortunately, this is cacheable, whereas the original redirect was not.

The redirect to the installer is generated by the subscriber in core/lib/Drupal/Core/EventSubscriber/ExceptionDetectNeedsInstallSubscriber.php. This generates a regular RedirectResponse. The fix may be for it to return a LocalRedirectResponse with max-age set to 0, or a TrustedRedirectResponse with cacheability also disabled.

Specific on the database failure, and a minor contributor

Looking at code it is attempting to avoid a redirect to the installer, as you can see in core/lib/Drupal/Core/Installer/InstallerRedirectTrait.php in the method shouldRedirectToInstaller. This works in general except for the final check for tableExists on the sessions table, which will occur in the case of the database connection being lost and a PDOException being raised (MySQL in my case). However, this tableExists does not work.

The MySQL schema driver at core/lib/Drupal/Core/Database/Driver/mysql/Schema.php implements tableExists in a way that will never throw an exception. It will always assume any failure to be the table not existing. This causes the lost connection to generate the original installer redirect.

This issue won't occur for anyone running PgSQL or SQLite as those don't capture exceptions from the tableExists check.

Proposed resolution

1. Return a LocalRedirectResponse with no cacheability from the exception listener so that the page_cache module on the stack can be told not to cache it. *OR* Implement a new response that does not implement cacheability but does implement SecuredRedirectResponse - currently there are no implementations that do not implement CacheableResponseInterface.
(TBF I have realised the max-age 0 is ignored by page_cache currently still... so the latter may be better option, or a new response policy to deny this specific redirect.)

2. (Will raise in a follow up) Improve exception handling in MySQL schema so that it can correctly throw if there are connectivity issues, allowing the shouldRedirectToInstaller to behave in a better way and avoid showing installer pages to visitors when a MySQL database connection is interrupted

Remaining tasks

1. Fix caching issue of the installer redirect
2. Raise followup for proposed resolution point 2 https://www.drupal.org/project/drupal/issues/3267207 (Related and alternate fix that may be blocked on MySQL issues: https://www.drupal.org/project/drupal/issues/2797141)
3. Write test coverage
4. update for trigger_error

User interface changes

None

API changes

None, unless a new redirect response is added that implements SecuredRedirectResponse but not CacheableResponseInterface

Data model changes

None

Release notes snippet

N/A

CommentFileSizeAuthor
#25 3267203-10.3.x-25.patch2.46 KBd.stoyanov
#12 1911.diff2.5 KBjcnventura

Issue fork drupal-3267203

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Driskell created an issue. See original summary.

driskell’s picture

Issue summary: View changes
driskell’s picture

Issue summary: View changes
driskell’s picture

Issue summary: View changes

driskell’s picture

Status: Active » Needs review
alexpott’s picture

driskell’s picture

@alexpott That issue resolves the biggest problem. However, I do still think that this shouldn't necessarily be cacheable. It seems to be the caching of this redirect would not benefit a new installation (as it's unlikely to even have an external cache) - and if there is ANY reason at any time that somehow triggers this redirect on a production site (due to a bug for example) this shouldn't be caching.

So I think it would be useful to consider the MR here to ensure there's never a cached redirect to the installer.

shenzhuxi’s picture

This is an easy way to reproduce, if $another_db_name doesn't exist.

  $default_connection = Database::getConnection('default', 'default');
  $options = $default_connection->getConnectionOptions();
  $options['database'] = $another_db_name;
  Database::addConnectionInfo('external', 'external', $options);
  $connection = Database::getConnection('external', 'external');

I think for any database exception like this, Drupal should just show an error rather than redirect to the installer page.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jcnventura’s picture

StatusFileSize
new2.5 KB

Adding current state of the MR so that I can use it from composer patches securely.

bradjones1’s picture

Status: Needs review » Needs work
Related issues: +#1537198: Add a Production/Development toggle

The installer redirect shouldn't run at all in production - there have been some high-profile examples of this happening (for reasons explained here), cache issue aside, on very large government sites in recent years. Much like the toggle for error verbosity, a site should be able to have a flag set to say, "we're in production, just fail hard."

Adding related, even though it's stale... could provide a way to make this not show up when we don't want it, to begin with.

driskell’s picture

Status: Needs work » Needs review

The installer redirect shouldn't run at all in production - there have been some high-profile examples of this happening (for reasons explained here), cache issue aside, on very large government sites in recent years. Much like the toggle for error verbosity, a site should be able to have a flag set to say, "we're in production, just fail hard."

Even with a toggle this issue would affect the mode where the redirect was allowed. So introduction of any toggle would be a separate task and I think as you note in related now there’s already an issue setup to do that. It shouldn’t impact this MR, which is still necessary fix.

Also keen not to delay a bug fix with feature changes that necessarily need discussion. So switching to review.

bradjones1’s picture

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs tests, +Needs change record

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As a bug this will need a test case showing the issue.

Also think the new parameter should default to NULL for BC and trigger_error that it will be required in D11. With a test case also.

New parameter could use a change record also.

Thanks.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

d.stoyanov made their first commit to this issue’s fork.

d.stoyanov changed the visibility of the branch 3267203-patch-update to active.

d.stoyanov changed the visibility of the branch 3267203-patch-update to hidden.

d.stoyanov’s picture

StatusFileSize
new2.46 KB

Reroll for 10.3.x

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.