From bb11b08d02e64a4ac180878769fd4751e30403f6 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Fri, 12 Apr 2024 21:03:04 +0100
Subject: [PATCH] Issue #3419690 by longwave, mondrake, immaculatexavier,
alexpott: Find an alternative to trigger_error in
Drupal\Core\Database\Query\Condition::compile
---
core/.phpstan-baseline.php | 6 -----
.../Drupal/Core/Database/Query/Condition.php | 9 +-------
.../KernelTests/Core/Database/QueryTest.php | 22 +++++++------------
.../Tests/Core/Database/ConditionTest.php | 4 +++-
4 files changed, 12 insertions(+), 29 deletions(-)
diff --git a/core/.phpstan-baseline.php b/core/.phpstan-baseline.php
index c4283234b867..c0a427eec173 100644
--- a/core/.phpstan-baseline.php
+++ b/core/.phpstan-baseline.php
@@ -2164,12 +2164,6 @@
'count' => 1,
'path' => __DIR__ . '/tests/Drupal/Tests/Core/Config/ConfigTest.php',
];
-$ignoreErrors[] = [
- 'message' => '#^Call to deprecated method expectError\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#',
- 'count' => 1,
- 'path' => __DIR__ . '/tests/Drupal/Tests/Core/Database/ConditionTest.php',
-];
$ignoreErrors[] = [
'message' => '#^Trying to mock an undefined method getRevisionId\\(\\) on class Drupal\\\\Tests\\\\Core\\\\Entity\\\\UrlTestEntity\\.$#',
'count' => 1,
diff --git a/core/lib/Drupal/Core/Database/Query/Condition.php b/core/lib/Drupal/Core/Database/Query/Condition.php
index 377d45cb59d5..2e738bf916bc 100644
--- a/core/lib/Drupal/Core/Database/Query/Condition.php
+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -255,14 +255,7 @@ public function compile(Connection $connection, PlaceholderInterface $queryPlace
// Provide a string which will result into an empty query result.
$this->stringVersion = '( AND 1 = 0 )';
- // Conceptually throwing an exception caused by user input is bad
- // as you result into a 'white screen of death', which depending on
- // your webserver configuration can result into the assumption that
- // your site is broken.
- // On top of that the database API relies on __toString() which
- // does not allow to throw exceptions.
- trigger_error('Invalid characters in query operator: ' . $condition['operator'], E_USER_ERROR);
- return;
+ throw new InvalidQueryException('Invalid characters in query operator: ' . $condition['operator']);
}
// For simplicity, we convert all operators to a data structure to
diff --git a/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php b/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php
index 112c101cd2b2..5893197ef74f 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/QueryTest.php
@@ -4,6 +4,8 @@
namespace Drupal\KernelTests\Core\Database;
+use Drupal\Core\Database\InvalidQueryException;
+
/**
* Tests Drupal's extended prepared statement syntax.
*
@@ -70,16 +72,6 @@ public function testArrayArgumentsSQLInjection() {
public function testConditionOperatorArgumentsSQLInjection() {
$injection = "IS NOT NULL) ;INSERT INTO {test} (name) VALUES ('test12345678'); -- ";
- $previous_error_handler = set_error_handler(function ($severity, $message, $filename, $lineno) use (&$previous_error_handler) {
- // Normalize the filename to use UNIX directory separators.
- if (preg_match('@core/lib/Drupal/Core/Database/Query/Condition.php$@', str_replace(DIRECTORY_SEPARATOR, '/', $filename))) {
- // Convert errors to exceptions for testing purposes below.
- throw new \ErrorException($message, 0, $severity, $filename, $lineno);
- }
- if ($previous_error_handler) {
- return $previous_error_handler($severity, $message, $filename, $lineno);
- }
- });
try {
$result = $this->connection->select('test', 't')
->fields('t')
@@ -87,7 +79,8 @@ public function testConditionOperatorArgumentsSQLInjection() {
->execute();
$this->fail('Should not be able to attempt SQL injection via condition operator.');
}
- catch (\ErrorException $e) {
+ catch (InvalidQueryException $e) {
+ $this->assertSame("Invalid characters in query operator: $injection", $e->getMessage());
// Expected exception; just continue testing.
}
@@ -115,7 +108,8 @@ public function testConditionOperatorArgumentsSQLInjection() {
->execute();
$this->fail('Should not be able to attempt SQL injection via operator.');
}
- catch (\ErrorException $e) {
+ catch (InvalidQueryException $e) {
+ $this->assertSame("Invalid characters in query operator: $injection", $e->getMessage());
// Expected exception; just continue testing.
}
@@ -132,10 +126,10 @@ public function testConditionOperatorArgumentsSQLInjection() {
->execute();
$this->fail('Should not be able to attempt SQL injection via operator.');
}
- catch (\ErrorException $e) {
+ catch (InvalidQueryException $e) {
+ $this->assertSame("Invalid characters in query operator: $injection", $e->getMessage());
// Expected exception; just continue testing.
}
- restore_error_handler();
}
/**
diff --git a/core/tests/Drupal/Tests/Core/Database/ConditionTest.php b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
index 0cd5b4fcd3c3..908b62650283 100644
--- a/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
+++ b/core/tests/Drupal/Tests/Core/Database/ConditionTest.php
@@ -5,6 +5,7 @@
namespace Drupal\Tests\Core\Database;
use Drupal\Core\Database\Connection;
+use Drupal\Core\Database\InvalidQueryException;
use Drupal\Core\Database\Query\Condition;
use Drupal\Core\Database\Query\PlaceholderInterface;
use Drupal\Tests\Core\Database\Stub\StubCondition;
@@ -169,7 +170,8 @@ public function testCompileWithSqlInjectionForOperator($operator) {
$condition = $connection->condition('AND');
$condition->condition('name', 'value', $operator);
- $this->expectError();
+ $this->expectException(InvalidQueryException::class);
+ $this->expectExceptionMessage('Invalid characters in query operator:');
$condition->compile($connection, $query_placeholder);
}
--
GitLab