From f54070d0591b770a4f5b85dda060a1612624a8d0 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Sun, 9 Aug 2026 00:23:19 +1200 Subject: [PATCH] fix: refuse a non-string select value instead of fatalling on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A nested-array value on `select` reached str_contains() and raised a TypeError. A TypeError is an Error, not an Exception, so it escaped the QueryException catch in every caller and surfaced as a 500 — where the same malformation on `equal`, `limit` and every other method returns a typed refusal, because their payload either lives on Query's typed string $attribute or never meets a string function. select was the only method whose values array is consumed as a string without a type check; a sweep of all 49 methods confirms it was the only fatal. The check runs before the duplicate check on purpose: array_unique() casts every array to "Array", so two nested values collapsed into one and reported a duplicate that was not there, masking the real error. The two downstream sites in Database.php are guarded too. They are unreachable while the validator refuses first, but live whenever validation is skipped. Co-Authored-By: Claude Fable 5 --- src/Database/Database.php | 5 +- src/Database/Validator/Query/Select.php | 10 ++ tests/unit/SelectProjectionTest.php | 120 ++++++++++++++++++++++ tests/unit/Validator/Query/SelectTest.php | 56 ++++++++++ 4 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 tests/unit/SelectProjectionTest.php diff --git a/src/Database/Database.php b/src/Database/Database.php index 69cabf1006..674969b0b2 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -9520,6 +9520,9 @@ private function validateSelections(Document $collection, array $queries): array foreach ($queries as $query) { if ($query->getMethod() == Query::TYPE_SELECT) { foreach ($query->getValues() as $value) { + if (!\is_string($value)) { + throw new QueryException('Attribute selection must be a string, got ' . \get_debug_type($value)); + } if (\str_contains($value, '.')) { $relationshipSelections[] = $value; continue; @@ -10011,7 +10014,7 @@ private function processRelationshipQueries( $values = $query->getValues(); foreach ($values as $valueIndex => $value) { - if (!\str_contains($value, '.')) { + if (!\is_string($value) || !\str_contains($value, '.')) { continue; } diff --git a/src/Database/Validator/Query/Select.php b/src/Database/Validator/Query/Select.php index b0ed9e564b..2df21c9a3c 100644 --- a/src/Database/Validator/Query/Select.php +++ b/src/Database/Validator/Query/Select.php @@ -68,6 +68,16 @@ public function isValid($value): bool return false; } + // Before the duplicate check: array_unique() stringifies every array element + // to "Array", so two nested values collapse into one and report a misleading + // duplicate instead of the type error that is actually there. + foreach ($value->getValues() as $attribute) { + if (!\is_string($attribute)) { + $this->message = 'Attribute selection must be a string, got ' . \get_debug_type($attribute); + return false; + } + } + if (\count($value->getValues()) !== \count(\array_unique($value->getValues()))) { $this->message = 'Duplicate attributes selected'; return false; diff --git a/tests/unit/SelectProjectionTest.php b/tests/unit/SelectProjectionTest.php new file mode 100644 index 0000000000..262e609e1b --- /dev/null +++ b/tests/unit/SelectProjectionTest.php @@ -0,0 +1,120 @@ +database = new Database(new DatabaseMemory(), new Cache(new CacheMemory())); + $this->database + ->setDatabase('utopiaTests') + ->setNamespace('select_' . \uniqid()); + + $this->database->create(); + $this->database->createCollection('widgets'); + $this->database->createAttribute('widgets', 'sku', Database::VAR_STRING, 255, false); + $this->database->createDocument('widgets', new Document([ + '$id' => 'widget', + '$permissions' => [Permission::read(Role::any())], + 'sku' => 'abc', + ])); + } + + /** + * @param array $values + * + * @dataProvider malformedSelections + */ + public function testAMalformedSelectionIsRefusedRatherThanFatal(array $values): void + { + $this->expectException(QueryException::class); + $this->expectExceptionMessage('Attribute selection must be a string, got'); + + $this->database->find('widgets', [Query::select($values)]); + } + + /** + * @return array}> + */ + public static function malformedSelections(): array + { + return [ + 'nested array' => [[['sku']]], + 'nested wildcard' => [[['*']]], + 'mixed flat and nested' => [['sku', ['x']]], + 'two nested values' => [[['a'], ['b']]], + ]; + } + + /** + * The refusal must be a catchable Exception. A TypeError is an Error, so a caller + * catching Exception — as the HTTP layer does — never sees it and returns a 500. + * + * @param array $values + * + * @dataProvider malformedSelections + */ + public function testTheRefusalIsCatchableAsAnException(array $values): void + { + $caught = null; + + try { + $this->database->find('widgets', [Query::select($values)]); + } catch (\Exception $exception) { + $caught = $exception; + } + + $this->assertInstanceOf( + QueryException::class, + $caught, + 'a malformed selection must be catchable as an Exception, otherwise it escapes as a 500', + ); + } + + public function testTheLegitimateFlatFormStillProjects(): void + { + $rows = $this->database->find('widgets', [Query::select(['sku'])]); + + $this->assertCount(1, $rows); + $this->assertSame('abc', $rows[0]->getAttribute('sku')); + } + + public function testTheWildcardStillProjects(): void + { + $this->assertCount(1, $this->database->find('widgets', [Query::select(['*'])])); + } + + /** + * The type check must not swallow the schema check that already worked. + */ + public function testAnUnknownAttributeIsStillRefusedBySchema(): void + { + $this->expectException(QueryException::class); + $this->expectExceptionMessage('Attribute not found in schema: nope'); + + $this->database->find('widgets', [Query::select(['nope'])]); + } +} diff --git a/tests/unit/Validator/Query/SelectTest.php b/tests/unit/Validator/Query/SelectTest.php index 2dafdb94c0..86b8d2495c 100644 --- a/tests/unit/Validator/Query/SelectTest.php +++ b/tests/unit/Validator/Query/SelectTest.php @@ -49,4 +49,60 @@ public function testValueFailure(): void $this->assertEquals('Invalid query', $this->validator->getDescription()); $this->assertFalse($this->validator->isValid(Query::select(['name.artist']))); } + + /** + * A non-string selection used to reach str_contains() and raise a TypeError, + * which is an Error rather than an Exception, so it escaped every catch on the + * way out and surfaced as a 500. Select was the only query method that answered + * a malformed value that way. + * + * @param array $values + * + * @dataProvider nonStringSelections + */ + public function testANonStringSelectionIsRefusedByType(array $values, string $expected): void + { + $this->assertFalse($this->validator->isValid(Query::select($values))); + $this->assertSame($expected, $this->validator->getDescription()); + } + + /** + * @return array, string}> + */ + public static function nonStringSelections(): array + { + return [ + 'nested array' => [[['attr']], 'Attribute selection must be a string, got array'], + 'nested wildcard' => [[['*']], 'Attribute selection must be a string, got array'], + 'mixed flat and nested' => [['attr', ['x']], 'Attribute selection must be a string, got array'], + 'assoc array' => [[['a' => 1]], 'Attribute selection must be a string, got array'], + 'integer' => [[1], 'Attribute selection must be a string, got int'], + 'null' => [[null], 'Attribute selection must be a string, got null'], + ]; + } + + /** + * Two nested values used to collapse to one under array_unique(), which casts + * every array to the string "Array", so the duplicate check tripped first and + * reported a duplicate that was not there. The type check has to run before it. + * + * Parsed from JSON rather than built with Query::select(), because that is the + * path a hand-written HTTP client takes and the only one that can carry a value + * the constructor's array type would reject. + */ + public function testTwoNestedSelectionsReportTheTypeNotAFalseDuplicate(): void + { + $query = Query::parse('{"method":"select","values":[["a"],["b"]]}'); + + $this->assertFalse($this->validator->isValid($query)); + $this->assertSame('Attribute selection must be a string, got array', $this->validator->getDescription()); + } + + public function testTheLegitimateFlatFormStillPasses(): void + { + $this->assertTrue($this->validator->isValid(Query::select(['attr']))); + $this->assertTrue($this->validator->isValid(Query::select(['*']))); + $this->assertTrue($this->validator->isValid(Query::select(['$id', '$createdAt']))); + $this->assertTrue($this->validator->isValid(Query::select(['artist.name']))); + } }