Commit 55b8185f authored by Helmut Hummel's avatar Helmut Hummel
Browse files

[TASK] Quote database identifiers when used instead of globally upfront

The implementation of the bugfix https://review.typo3.org/53360
was done by iterating over TCA during cache generation and
correctly quoting SQL fragments that are provided, to after
that store TCA in cache.

This has disadvantages:

1. A DB connection is required to create TCA cache:
This makes efforts for warming up caches upfront impossible,
as cache warmup should be able to be performed on build
systems without DB connection.

2. It separates code that executes a query from code that
is preparing a query: Escaping arguments for a query in a
different place than the actual query execution can be
considered an anti pattern, because at other points it isn't
clear in which context these values are used. Beside that,
performing the actual query, undoing DB encoding and redoing
a different encoding is impossible.

Pre-processing / quoting identifiers upfront can be considered
a similar anti pattern as well. Despite there are less use
cases to perform operations on the original non quoted
identifiers, post processing of such fields in an event
is impossible with the current implementation.

3. The current implementation being buggy and the adoption
in TYPO3 is sparse: While this could be tackled with according
fixes, it shows that this feature, that has been implemented
a couple of years ago and is a technical burden, is rarely
used, and therefore the implementation can be simplified and
cleaned up.

The patch drops this TCA preparation and field name escaping
is now done in places where the queries are actually built.

Strictly seen this is a breaking change for implementers of
the TCA post processing event or for custom code that directly
uses TCA to perform query parts.

It is unlikely though that such code exists in userland,
nonetheless a feature flag "runtimeDbQuotingOfTcaConfiguration"
is introduced to allow extensions to also use the feature flag
and use the quoting on demand if not done before.

For TYPO3 v12, this feature flag will be enabled at all times.

Releases: master
Resolves: #94697
Change-Id: Ie0e48054def29c6c1e2810c8d30528c719439d9f
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/69507

Tested-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
Tested-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
Tested-by: core-ci's avatarcore-ci <typo3@b13.com>
Tested-by: Helmut Hummel's avatarHelmut Hummel <typo3@helhum.io>
Reviewed-by: Christian Kuhn's avatarChristian Kuhn <lolli@schwarzbu.ch>
Reviewed-by: Benni Mack's avatarBenni Mack <benni@typo3.org>
Reviewed-by: Helmut Hummel's avatarHelmut Hummel <typo3@helhum.io>
parent 273fa782
......@@ -19,8 +19,10 @@ use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\ServerRequestInterface;
use TYPO3\CMS\Backend\Form\Wizard\SuggestWizardDefaultReceiver;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Configuration\FlexForm\FlexFormTools;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Database\Query\QueryHelper;
use TYPO3\CMS\Core\Http\JsonResponse;
use TYPO3\CMS\Core\Utility\ArrayUtility;
use TYPO3\CMS\Core\Utility\GeneralUtility;
......@@ -129,6 +131,7 @@ class SuggestWizardController
$config['addWhere'] = $whereClause;
}
if (isset($config['addWhere'])) {
$connectionPool = GeneralUtility::makeInstance(ConnectionPool::class);
$replacement = [
'###THIS_UID###' => (int)$uid,
'###CURRENT_PID###' => (int)$pid
......@@ -142,13 +145,17 @@ class SuggestWizardController
$replacement['###PAGE_TSCONFIG_IDLIST###'] = implode(',', GeneralUtility::intExplode(',', $fieldTSconfig['PAGE_TSCONFIG_IDLIST']));
}
if (isset($fieldTSconfig['PAGE_TSCONFIG_STR'])) {
$connection = GeneralUtility::makeInstance(ConnectionPool::class)->getConnectionForTable($fieldConfig['foreign_table']);
$connection = $connectionPool->getConnectionForTable($fieldConfig['foreign_table']);
// nasty hack, but it's currently not possible to just quote anything "inside" the value but not escaping
// the whole field as it is not known where it is used in the WHERE clause
$replacement['###PAGE_TSCONFIG_STR###'] = trim($connection->quote($fieldTSconfig['PAGE_TSCONFIG_STR']), '\'');
}
}
$config['addWhere'] = strtr(' ' . $config['addWhere'], $replacement);
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$config['addWhere'] = QueryHelper::quoteDatabaseIdentifiers($connectionPool->getConnectionForTable($queryTable), strtr(' ' . $config['addWhere'], $replacement));
} else {
$config['addWhere'] = strtr(' ' . $config['addWhere'], $replacement);
}
}
// instantiate the class that should fetch the records for this $queryTable
......
......@@ -18,6 +18,7 @@ namespace TYPO3\CMS\Backend\Form\FormDataProvider;
use Doctrine\DBAL\Exception as DBALException;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Database\Query\QueryBuilder;
use TYPO3\CMS\Core\Database\Query\QueryHelper;
......@@ -734,7 +735,11 @@ abstract class AbstractItemProvider
if (!empty($result['processedTca']['columns'][$localFieldName]['config']['foreign_table_where'])
&& is_string($result['processedTca']['columns'][$localFieldName]['config']['foreign_table_where'])
) {
$foreignTableClause = $result['processedTca']['columns'][$localFieldName]['config']['foreign_table_where'];
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$foreignTableClause = QueryHelper::quoteDatabaseIdentifiers($connection, $result['processedTca']['columns'][$localFieldName]['config']['foreign_table_where']);
} else {
$foreignTableClause = $result['processedTca']['columns'][$localFieldName]['config']['foreign_table_where'];
}
// Replace possible markers in query
if (strpos($foreignTableClause, '###REC_FIELD_') !== false) {
// " AND table.field='###REC_FIELD_field1###' AND ..." -> array(" AND table.field='", "field1###' AND ...")
......
......@@ -18,6 +18,7 @@ namespace TYPO3\CMS\Backend\Search\LiveSearch;
use TYPO3\CMS\Backend\Routing\UriBuilder;
use TYPO3\CMS\Backend\Tree\View\PageTreeView;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\Connection;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Database\Query\Expression\CompositeExpression;
......@@ -349,10 +350,16 @@ class LiveSearch
);
}
// Apply additional condition, if any
if ($fieldConfig['search']['andWhere']) {
$searchConstraint->add(
QueryHelper::stripLogicalOperatorPrefix($fieldConfig['search']['andWhere'])
);
if ($fieldConfig['search']['andWhere'] ?? false) {
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$searchConstraint->add(
QueryHelper::stripLogicalOperatorPrefix(QueryHelper::quoteDatabaseIdentifiers($queryBuilder->getConnection(), $fieldConfig['search']['andWhere']))
);
} else {
$searchConstraint->add(
QueryHelper::stripLogicalOperatorPrefix($fieldConfig['search']['andWhere'])
);
}
}
}
// Assemble the search condition only if the field makes sense to be searched
......
......@@ -25,6 +25,7 @@ use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
use TYPO3\CMS\Core\Cache\CacheManager;
use TYPO3\CMS\Core\Cache\Frontend\FrontendInterface;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Configuration\FlexForm\Exception\InvalidIdentifierException;
use TYPO3\CMS\Core\Configuration\FlexForm\Exception\InvalidParentRowException;
use TYPO3\CMS\Core\Configuration\FlexForm\Exception\InvalidParentRowLoopException;
......@@ -5740,9 +5741,15 @@ class DataHandler implements LoggerAwareInterface
$queryBuilder->expr()->eq($relationUidFieldName, $queryBuilder->createNamedParameter($recordUid, \PDO::PARAM_INT))
);
if (!empty($fieldConfig['MM_table_where']) && is_string($fieldConfig['MM_table_where'])) {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$recordUid, $fieldConfig['MM_table_where']))
);
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$recordUid, QueryHelper::quoteDatabaseIdentifiers($queryBuilder->getConnection(), $fieldConfig['MM_table_where'])))
);
} else {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$recordUid, $fieldConfig['MM_table_where']))
);
}
}
$mmMatchFields = $fieldConfig['MM_match_fields'] ?? [];
foreach ($mmMatchFields as $fieldName => $fieldValue) {
......
......@@ -17,6 +17,7 @@ namespace TYPO3\CMS\Core\Database;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\Platform\PlatformInformation;
use TYPO3\CMS\Core\Database\Query\QueryHelper;
use TYPO3\CMS\Core\Database\Query\Restriction\DeletedRestriction;
......@@ -558,9 +559,15 @@ class RelationHandler
$sorting_field = 'sorting';
}
if ($this->MM_table_where) {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$uid, $this->MM_table_where))
);
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$uid, QueryHelper::quoteDatabaseIdentifiers($queryBuilder->getConnection(), $this->MM_table_where)))
);
} else {
$queryBuilder->andWhere(
QueryHelper::stripLogicalOperatorPrefix(str_replace('###THIS_UID###', (string)$uid, $this->MM_table_where))
);
}
}
foreach ($this->MM_match_fields as $field => $value) {
$queryBuilder->andWhere(
......
......@@ -17,6 +17,7 @@ declare(strict_types=1);
namespace TYPO3\CMS\Core\Preparations;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Database\Query\QueryHelper;
use TYPO3\CMS\Core\Utility\GeneralUtility;
......@@ -45,7 +46,9 @@ class TcaPreparation
public function prepare(array $tca): array
{
$tca = $this->configureCategoryRelations($tca);
$tca = $this->prepareQuotingOfTableNamesAndColumnNames($tca);
if (!GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$tca = $this->prepareQuotingOfTableNamesAndColumnNames($tca);
}
return $tca;
}
......
......@@ -73,6 +73,7 @@ return [
'features' => [
'form.legacyUploadMimeTypes' => true,
'redirects.hitCount' => false,
'runtimeDbQuotingOfTcaConfiguration' => true,
'unifiedPageTranslationHandling' => false,
'security.backend.enforceReferrer' => true,
'yamlImportsFollowDeclarationOrder' => false
......
.. include:: ../../Includes.txt
====================================================================================
Important: #94697 - Quote database identifiers when used instead of globally upfront
====================================================================================
See :issue:`94697`
Description
===========
When using :php:`TCA` keys that contain SQL fragments like `foreign_table_where`,
`MM_table_where` and `search.andWhere`, it is important to use a special syntax
for SQL field names to stay DBAL compatible.
See :doc:`Important-81751-DbalCompatibleQuotingInTca` for details. It
boils down to: Use :sql:`{#colPos}=0` instead of :sql:`colPos=0` to stay DBAL
compatible. The core then takes care field names are properly quoted for the
specific DMBS that is used.
This quoting preparation has been performed during TCA cache warmup until now.
This had the main disadvantage that this early boostrap step already needs a
working database connection. The core however plans to introduce features to
allow cache warmups as separate step in CI/CD systems. Those usually don't have
the target database available, and in general it's ugly that an early warmup
needs a database connection.
Therefore, the field name quoting of SQL fragments is now no longer performed
during TCA cache warmup, but instead directly done in places where those
TCA keys are used to create the final queries.
Since extensions might rely on identifiers within these settings being properly
quoted, a feature flag called "runtimeDbQuotingOfTcaConfiguration" is introduced
to revert to the old behaviour with core v11.
Extension authors who access these TCA properties, which is quite unlikely,
can use the feature flag to support both variants to ensure compatibility
between TYPO3 v10, v11 and TYPO3 v12.
Starting with TYPO3 v12.0, this feature flag will be enabled at all times.
.. index:: Database, TCA, ext:core
......@@ -16,6 +16,7 @@
namespace TYPO3\CMS\Core\Tests\Unit\Preparations;
use Prophecy\Argument;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\Connection;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Preparations\TcaPreparation;
......@@ -341,6 +342,9 @@ class TcaPreparationTest extends UnitTestCase
$connectionPool = $this->prophesize(ConnectionPool::class);
$connectionPool->getConnectionForTable(Argument::any())->willReturn($connection->reveal());
GeneralUtility::addInstance(ConnectionPool::class, $connectionPool->reveal());
$features = $this->prophesize(Features::class);
$features->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')->willReturn(false);
GeneralUtility::addInstance(Features::class, $features->reveal());
$subject = new TcaPreparation();
self::assertEquals($expected, $subject->prepare($input));
}
......
......@@ -25,6 +25,7 @@ use TYPO3\CMS\Backend\Tree\View\PageTreeView;
use TYPO3\CMS\Backend\Utility\BackendUtility;
use TYPO3\CMS\Core\Authentication\BackendUserAuthentication;
use TYPO3\CMS\Core\Cache\CacheManager;
use TYPO3\CMS\Core\Configuration\Features;
use TYPO3\CMS\Core\Database\Connection;
use TYPO3\CMS\Core\Database\ConnectionPool;
use TYPO3\CMS\Core\Database\Query\QueryBuilder;
......@@ -2688,10 +2689,16 @@ class DatabaseRecordList
if (in_array('pidonly', $searchConfig, true) && $currentPid > 0) {
$searchConstraint->add($expressionBuilder->eq($tablePidField, (int)$currentPid));
}
if ($searchConfig['andWhere']) {
$searchConstraint->add(
QueryHelper::stripLogicalOperatorPrefix($fieldConfig['search']['andWhere'])
);
if ($searchConfig['andWhere'] ?? false) {
if (GeneralUtility::makeInstance(Features::class)->isFeatureEnabled('runtimeDbQuotingOfTcaConfiguration')) {
$searchConstraint->add(
QueryHelper::quoteDatabaseIdentifiers($queryBuilder->getConnection(), QueryHelper::stripLogicalOperatorPrefix($fieldConfig['search']['andWhere']))
);
} else {
$searchConstraint->add(
QueryHelper::stripLogicalOperatorPrefix($fieldConfig['search']['andWhere'])
);
}
}
}
if ($fieldType === 'text'
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment