From 15758f2e8324b359e5b34ec8a7a2207fb870461e Mon Sep 17 00:00:00 2001 From: Przemek Szalko Date: Wed, 30 Sep 2026 12:37:11 +0200 Subject: [PATCH] Added parser SQL syntax check for invalid VARCHAR column definition. #issue-671 --- src/Parsers/AlterOperations.php | 7 + src/Parsers/DataTypes.php | 31 ++ tests/Components/DataTypeTest.php | 176 +++++++++ tests/Parser/AlterStatementTest.php | 1 + ...AlterTableAddColumnInvalidVarcharLength.in | 1 + ...lterTableAddColumnInvalidVarcharLength.out | 337 ++++++++++++++++++ 6 files changed, 553 insertions(+) create mode 100644 tests/Components/DataTypeTest.php create mode 100644 tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.in create mode 100644 tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.out diff --git a/src/Parsers/AlterOperations.php b/src/Parsers/AlterOperations.php index 7083b175..2cb54347 100644 --- a/src/Parsers/AlterOperations.php +++ b/src/Parsers/AlterOperations.php @@ -262,6 +262,7 @@ public static function parse(Parser $parser, TokensList $list, array $options = * Counts brackets. */ $brackets = 0; + $isFirstUnknownToken = true; /** * The state of the parser. @@ -355,6 +356,12 @@ public static function parse(Parser $parser, TokensList $list, array $options = $state = 2; } elseif ($state === 2) { + if ($isFirstUnknownToken && $ret->options?->has('ADD') === true && $token->keyword === 'VARCHAR') { + // Validate the column type while preserving the original tokens used to build ALTER. + DataTypes::parse($parser, clone $list); + } + + $isFirstUnknownToken = false; if (is_string($token->value) || is_int($token->value)) { $arrayKey = $token->value; } else { diff --git a/src/Parsers/DataTypes.php b/src/Parsers/DataTypes.php index 8b961bc0..e22bfcc2 100644 --- a/src/Parsers/DataTypes.php +++ b/src/Parsers/DataTypes.php @@ -11,6 +11,7 @@ use PhpMyAdmin\SqlParser\TokensList; use PhpMyAdmin\SqlParser\TokenType; +use function preg_match; use function strtoupper; /** @@ -79,7 +80,37 @@ public static function parse(Parser $parser, TokensList $list, array $options = $state = 1; } elseif ($state === 1) { if (($token->type === TokenType::Operator) && ($token->value === '(')) { + $parametersStart = $list->idx + 1; $parameters = ArrayObjs::parse($parser, $list); + if ($ret->name === 'VARCHAR') { + $hasLength = false; + for ($idx = $parametersStart; $idx < $list->idx; ++$idx) { + $parameter = $list->tokens[$idx]; + if ($parameter->type === TokenType::Whitespace || $parameter->type === TokenType::Comment) { + continue; + } + + // Check the original lexeme: conversion can turn 20.0 or '20' into 20. + if ( + $hasLength || $parameter->type !== TokenType::Number + || preg_match('/^[0-9]+$/D', $parameter->token) !== 1 + ) { + $parser->error('VARCHAR length must be a single nonnegative integer.', $parameter); + $hasLength = true; + break; + } + + $hasLength = true; + } + + if (! $hasLength) { + $parser->error( + 'VARCHAR length must be a single nonnegative integer.', + $list->tokens[$list->idx] ?? null, + ); + } + } + ++$list->idx; $ret->parameters = ($ret->name === 'ENUM') || ($ret->name === 'SET') ? $parameters->raw : $parameters->values; diff --git a/tests/Components/DataTypeTest.php b/tests/Components/DataTypeTest.php new file mode 100644 index 00000000..142c416a --- /dev/null +++ b/tests/Components/DataTypeTest.php @@ -0,0 +1,176 @@ +getTokensList($declaration . ' COLLATE utf8mb4_bin NULL')); + self::assertSame([], $parser->errors); + self::assertNotNull($type); + self::assertSame('VARCHAR', $type->name); + self::assertSame([$length], $type->parameters); + self::assertNotNull($type->options); + self::assertSame('utf8mb4_bin', $type->options->get('COLLATE')); + + $parser = new Parser('CREATE TABLE temp_users (phone ' . $declaration . ' COLLATE utf8mb4_bin NULL);'); + self::assertSame([], $parser->errors); + self::assertInstanceOf(CreateStatement::class, $parser->statements[0]); + self::assertIsArray($parser->statements[0]->fields); + $field = $parser->statements[0]->fields[0]; + self::assertNotNull($field->type); + self::assertSame([$length], $field->type->parameters); + self::assertNotNull($field->options); + self::assertTrue($field->options->has('NULL')); + + $parser = new Parser('ALTER TABLE temp_users ADD COLUMN phone ' . $declaration . ' NULL;'); + self::assertSame([], $parser->errors); + self::assertInstanceOf(AlterStatement::class, $parser->statements[0]); + self::assertStringContainsString($length, $parser->statements[0]->build()); + self::assertStringContainsString('NULL', $parser->statements[0]->build()); + } + + /** @return array */ + public static function validLengthProvider(): array + { + return [ + 'ordinary' => ['VARCHAR(20)', '20'], + 'zero' => ['VARCHAR(0)', '0'], + 'leading zeros' => ['VARCHAR(020)', '20'], + 'mixed case and whitespace' => ["vArChAr \n ( 20\t )", '20'], + 'comments' => ['varchar /* type */ ( /* before */ 20 /* after */ )', '20'], + 'line comments' => ["varchar( -- before\n 20 # after\n )", '20'], + ]; + } + + #[DataProvider('invalidLengthProvider')] + public function testInvalidLength(string $length, string $invalidToken): void + { + $parser = new Parser(); + DataTypes::parse($parser, $this->getTokensList('VARCHAR(' . $length . ') NULL')); + self::assertCount(1, $parser->errors); + self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage()); + self::assertInstanceOf(ParserException::class, $parser->errors[0]); + self::assertNotNull($parser->errors[0]->token); + self::assertSame($invalidToken, $parser->errors[0]->token->token); + + foreach (self::columnQueries('VARCHAR(' . $length . ')') as $query) { + $parser = new Parser($query . ' SELECT 1;'); + self::assertCount(1, $parser->errors); + self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage()); + self::assertInstanceOf(ParserException::class, $parser->errors[0]); + self::assertNotNull($parser->errors[0]->token); + self::assertSame($invalidToken, $parser->errors[0]->token->token); + self::assertCount(2, $parser->statements); + self::assertInstanceOf(SelectStatement::class, $parser->statements[1]); + + try { + new Parser($query, true); + self::fail('Strict mode must reject an invalid VARCHAR length.'); + } catch (ParserException $exception) { + self::assertSame(self::LENGTH_ERROR, $exception->getMessage()); + } + } + } + + /** @return array */ + public static function invalidLengthProvider(): array + { + return [ + 'identifier' => ['X', 'X'], + 'quoted string' => ["'20'", "'20'"], + 'double quoted string' => ['"20"', '"20"'], + 'quoted identifier' => ['`20`', '`20`'], + 'fraction' => ['20.0', '20.0'], + 'negative' => ['-20', '-20'], + 'positive sign' => ['+20', '+20'], + 'scientific' => ['2e1', '2e1'], + 'hexadecimal' => ['0x14', '0x14'], + 'binary' => ['0b10100', '0b10100'], + 'expression' => ['1+19', '+19'], + 'nested parentheses' => ['(20)', '('], + 'empty' => ['', ')'], + 'only comments' => [' /* empty */ ', ')'], + 'extra parameter' => ['20,30', ','], + 'trailing comma' => ['20,', ','], + 'separate numbers' => ['20 /* gap */ 30', '30'], + ]; + } + + /** @return list */ + private static function columnQueries(string $declaration): array + { + return [ + 'CREATE TABLE temp_users (phone ' . $declaration . ' NULL);', + 'ALTER TABLE temp_users ADD COLUMN phone ' . $declaration . ' NULL;', + 'ALTER TABLE temp_users ADD phone ' . $declaration . ' NULL;', + ]; + } + + /** @param list $parameters */ + #[DataProvider('otherTypeProvider')] + public function testOtherTypes(string $declaration, array $parameters): void + { + $parser = new Parser(); + $type = DataTypes::parse($parser, $this->getTokensList($declaration)); + self::assertSame([], $parser->errors); + self::assertNotNull($type); + self::assertSame($parameters, $type->parameters); + } + + /** @return array}> */ + public static function otherTypeProvider(): array + { + return [ + 'enum' => ["ENUM('a','b')", ["'a'", "'b'"]], + 'set' => ["SET('a','b')", ["'a'", "'b'"]], + 'decimal' => ['DECIMAL(10,2)', ['10', '2']], + 'lengthless varchar' => ['VARCHAR', []], + ]; + } + + public function testLengthlessRoutine(): void + { + $parser = new Parser('CREATE PROCEDURE p(IN phone VARCHAR) SELECT phone;'); + self::assertSame([], $parser->errors); + } + + public function testAlterContinuation(): void + { + $parser = new Parser( + 'ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL, ADD COLUMN name VARCHAR(20) NOT NULL;', + ); + self::assertCount(1, $parser->errors); + self::assertSame(self::LENGTH_ERROR, $parser->errors[0]->getMessage()); + self::assertInstanceOf(AlterStatement::class, $parser->statements[0]); + self::assertNotNull($parser->statements[0]->altered); + self::assertCount(2, $parser->statements[0]->altered); + self::assertStringContainsString('VARCHAR(20) NOT NULL', $parser->statements[0]->build()); + } + + public function testAlterStringParameters(): void + { + $parser = new Parser("ALTER TABLE temp_users ADD COLUMN e ENUM('a','b'), ADD COLUMN s SET('a','b');"); + self::assertSame([], $parser->errors); + self::assertInstanceOf(AlterStatement::class, $parser->statements[0]); + self::assertNotNull($parser->statements[0]->altered); + self::assertCount(2, $parser->statements[0]->altered); + self::assertStringContainsString("ENUM('a','b')", $parser->statements[0]->build()); + } +} diff --git a/tests/Parser/AlterStatementTest.php b/tests/Parser/AlterStatementTest.php index c2f019de..743643a8 100644 --- a/tests/Parser/AlterStatementTest.php +++ b/tests/Parser/AlterStatementTest.php @@ -45,6 +45,7 @@ public static function alterProvider(): array ['parser/parseAlterTablePartitionByRange2'], ['parser/parseAlterTableCoalescePartition'], ['parser/parseAlterTableAddColumnWithCheck'], + ['parser/parseAlterTableAddColumnInvalidVarcharLength'], ['parser/parseAlterTableAddSpatialIndex1'], ['parser/parseAlterTableAddUniqueKey1'], ['parser/parseAlterTableAddUniqueKey2'], diff --git a/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.in b/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.in new file mode 100644 index 00000000..217c77ae --- /dev/null +++ b/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.in @@ -0,0 +1 @@ +ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL; \ No newline at end of file diff --git a/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.out b/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.out new file mode 100644 index 00000000..51cf069f --- /dev/null +++ b/tests/data/parser/parseAlterTableAddColumnInvalidVarcharLength.out @@ -0,0 +1,337 @@ +{ + "query": "ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL;", + "lexer": { + "@type": "PhpMyAdmin\\SqlParser\\Lexer", + "strict": false, + "errors": [], + "str": "ALTER TABLE temp_users ADD COLUMN phone VARCHAR(X) NULL;", + "len": 56, + "last": 56, + "list": { + "@type": "PhpMyAdmin\\SqlParser\\TokensList", + "count": 20, + "idx": 20, + "tokens": [ + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "ALTER", + "value": "ALTER", + "keyword": "ALTER", + "type": { + "@type": "PhpMyAdmin\\SqlParser\\TokenType", + "name": "Keyword", + "value": 1 + }, + "flags": 3, + "position": 0 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "PhpMyAdmin\\SqlParser\\TokenType", + "name": "Whitespace", + "value": 3 + }, + "flags": 0, + "position": 5 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "TABLE", + "value": "TABLE", + "keyword": "TABLE", + "type": { + "@type": "@3" + }, + "flags": 3, + "position": 6 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 11 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "temp_users", + "value": "temp_users", + "keyword": null, + "type": { + "@type": "PhpMyAdmin\\SqlParser\\TokenType", + "name": "None", + "value": 0 + }, + "flags": 0, + "position": 12 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 22 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "ADD", + "value": "ADD", + "keyword": "ADD", + "type": { + "@type": "@3" + }, + "flags": 3, + "position": 23 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 26 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "COLUMN", + "value": "COLUMN", + "keyword": "COLUMN", + "type": { + "@type": "@3" + }, + "flags": 3, + "position": 27 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 33 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "phone", + "value": "phone", + "keyword": null, + "type": { + "@type": "@9" + }, + "flags": 0, + "position": 34 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 39 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "VARCHAR", + "value": "VARCHAR", + "keyword": "VARCHAR", + "type": { + "@type": "@3" + }, + "flags": 11, + "position": 40 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "(", + "value": "(", + "keyword": null, + "type": { + "@type": "PhpMyAdmin\\SqlParser\\TokenType", + "name": "Operator", + "value": 2 + }, + "flags": 16, + "position": 47 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "X", + "value": "X", + "keyword": "X", + "type": { + "@type": "@3" + }, + "flags": 33, + "position": 48 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": ")", + "value": ")", + "keyword": null, + "type": { + "@type": "@19" + }, + "flags": 16, + "position": 49 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": " ", + "value": " ", + "keyword": null, + "type": { + "@type": "@5" + }, + "flags": 0, + "position": 50 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "NULL", + "value": "NULL", + "keyword": "NULL", + "type": { + "@type": "@3" + }, + "flags": 3, + "position": 51 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": ";", + "value": ";", + "keyword": null, + "type": { + "@type": "PhpMyAdmin\\SqlParser\\TokenType", + "name": "Delimiter", + "value": 9 + }, + "flags": 0, + "position": 55 + }, + { + "@type": "PhpMyAdmin\\SqlParser\\Token", + "token": "", + "value": "", + "keyword": null, + "type": { + "@type": "@25" + }, + "flags": 0, + "position": null + } + ] + }, + "delimiter": ";", + "delimiterLen": 1 + }, + "parser": { + "@type": "PhpMyAdmin\\SqlParser\\Parser", + "strict": false, + "errors": [], + "list": { + "@type": "@1" + }, + "statements": [ + { + "@type": "PhpMyAdmin\\SqlParser\\Statements\\AlterStatement", + "table": { + "@type": "PhpMyAdmin\\SqlParser\\Components\\Expression", + "database": null, + "table": "temp_users", + "column": null, + "expr": "temp_users", + "alias": null, + "function": null, + "subquery": null + }, + "altered": [ + { + "@type": "PhpMyAdmin\\SqlParser\\Components\\AlterOperation", + "options": { + "@type": "PhpMyAdmin\\SqlParser\\Components\\OptionsArray", + "options": { + "1": "ADD", + "2": "COLUMN" + } + }, + "field": { + "@type": "PhpMyAdmin\\SqlParser\\Components\\Expression", + "database": null, + "table": null, + "column": "phone", + "expr": "phone", + "alias": null, + "function": null, + "subquery": null + }, + "partitions": null, + "unknown": [ + { + "@type": "@17" + }, + { + "@type": "@18" + }, + { + "@type": "@20" + }, + { + "@type": "@21" + }, + { + "@type": "@22" + }, + { + "@type": "@23" + } + ] + } + ], + "options": { + "@type": "PhpMyAdmin\\SqlParser\\Components\\OptionsArray", + "options": { + "3": "TABLE" + } + }, + "first": 0, + "last": 18 + } + ], + "brackets": 0 + }, + "errors": { + "lexer": [], + "parser": [ + [ + "VARCHAR length must be a single nonnegative integer.", + { + "@type": "@20" + }, + 0 + ] + ] + } +} \ No newline at end of file