Skip to content

Revert #14234 add update error message for DB connection failures#17612

Merged
Zeegaan merged 5 commits intov13/devfrom
v13/fix/14110-revert-14234-add-error-description
Nov 26, 2024
Merged

Revert #14234 add update error message for DB connection failures#17612
Zeegaan merged 5 commits intov13/devfrom
v13/fix/14110-revert-14234-add-error-description

Conversation

@kjac
Copy link
Contributor

@kjac kjac commented Nov 22, 2024

Prerequisites

  • I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #14110

Description

This PR reverts #14234 as we don't really want to force create MSSQL databases automatically.

To fix #14110, the error message for failed DB connections has been updated to include possible causes and also a mention of the InstallMissingDatabase in GlobalSettings:

image

Testing this PR

Configure a MSSQL connection string in app settings. The connection string should point at a non-existing database. Verify that the resulting error message contains a hint about the InstallMissingDatabase setting.

Co-authored-by: Ronald Barendse <ronald@barend.se>
@ronaldbarendse
Copy link
Contributor

Also note that GHSA-h8wc-r4jh-mg7m changed the runtime level from RuntimeLevel.Install to RuntimeLevel.BootFailed when it has a connection failure on startup and either ForceCreateDatabase or InstallMissingDatabase is enabled: this did brake the attended install with a preconfigured connection string (as @JasonElkin mentioned in #14956 (comment)).

This issue was actually the result of changing the SqlServerDatabaseProviderMetadata.ForceCreateDatabase value to true, as that would always put a website using SQL Server (which is used on most, if not all, production sites) into the install state when it had a intermittent connection issue on startup (and doesn't use unattended install). This bypassed the InstallMissingDatabase setting completely, which is mainly there to prevent you from opening up your site to this security issue (even though the conditions are very rare):

if (_globalSettings.Value.InstallMissingDatabase || _databaseProviderMetadata.CanForceCreateDatabase(_databaseFactory))
{
// ok to install on a configured but missing database
Level = RuntimeLevel.BootFailed;
Reason = RuntimeLevelReason.InstallMissingDatabase;
return;
}

This is also the reason why I suggested making the InstallMissingDatabase setting nullable, so a null value falls back to the providers ForceCreateDatabase value, but setting it to false will disable this for all providers: #14956 (comment).

@kjac kjac marked this pull request as draft November 22, 2024 09:37
@kjac kjac marked this pull request as ready for review November 25, 2024 13:01
@kjac
Copy link
Contributor Author

kjac commented Nov 25, 2024

So. We've changed this up a bit. After a few internal discussions, we've decided to obsolete the InstallMissingDatabase configuration option and remove it in V16. No matter what we do, it will always pose a security risk, so it has to go.

Bonus info: The option has not worked since July 2023 😄

@Zeegaan
Copy link
Member

Zeegaan commented Nov 26, 2024

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants