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
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3267203-10.3.x-25.patch | 2.46 KB | d.stoyanov |
| #12 | 1911.diff | 2.5 KB | jcnventura |
Issue fork drupal-3267203
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
Comment #2
driskell commentedComment #3
driskell commentedComment #4
driskell commentedComment #5
driskell commentedComment #7
driskell commentedComment #8
alexpott@Driskell now that #2797141: Remove the methods tableExists() and fieldExists() from Drupal\Core\Database\Driver\mysql\Schema has landed can we close this issue as outdated?
Comment #9
driskell commented@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.
Comment #10
shenzhuxi commentedThis is an easy way to reproduce, if $another_db_name doesn't exist.
I think for any database exception like this, Drupal should just show an error rather than redirect to the installer page.
Comment #12
jcnventuraAdding current state of the MR so that I can use it from composer patches securely.
Comment #13
bradjones1The 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.
Comment #14
driskell commentedEven 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.
Comment #15
bradjones1Comment #17
smustgrave commentedThis 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.
Comment #25
d.stoyanov commentedReroll for 10.3.x