From 10055c9358bb9df189175fb1bc38be826aab677e Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Wed, 30 Sep 2026 03:46:34 +0200 Subject: [PATCH] Only treat comment markers outside literals as comments removeComments() used regexes that also matched "--" and "/*" inside string literals. A literal like '--' or '/*' then removed the code after it from the checks while PostgreSQL still ran it, e.g. COPY (SELECT '--') TO PROGRAM 'sh' passed the forbidden-construct check, and a DELETE in a CTE between '/*' and '*/' literals skipped the confirmation. Comments and literals are now found by one scanner that walks the SQL the way the database does ('...', E'...', "...", $tag$...$tag$, -- and block comments), shared by removeComments(), removeLiterals() and splitStatements(). --- lib/Service/SqlGuard.php | 140 ++++++++++++++++++---------- tests/Unit/Service/SqlGuardTest.php | 9 ++ 2 files changed, 102 insertions(+), 47 deletions(-) diff --git a/lib/Service/SqlGuard.php b/lib/Service/SqlGuard.php index a03515d..6863798 100644 --- a/lib/Service/SqlGuard.php +++ b/lib/Service/SqlGuard.php @@ -64,50 +64,17 @@ class SqlGuard $i = 0; while ($i < $len) { - $ch = $sql[$i]; - $end = null; - - if ($ch === "'" || $ch === '"') { - $backslashEscapes = $ch === "'" && $i > 0 && ($sql[$i - 1] === 'E' || $sql[$i - 1] === 'e') - && ($i === 1 || !preg_match('/[A-Za-z0-9_]/', $sql[$i - 2])); - $end = $i + 1; - while ($end < $len) { - if ($backslashEscapes && $sql[$end] === '\\') { - $end += 2; - continue; - } - if ($sql[$end] === $ch) { - if ($end + 1 < $len && $sql[$end + 1] === $ch) { - $end += 2; - continue; - } - break; - } - $end++; - } - $end = min($end + 1, $len); - } elseif ($ch === '$' && preg_match('/\G\$([A-Za-z_][A-Za-z0-9_]*)?\$/', $sql, $m, 0, $i)) { - $close = strpos($sql, $m[0], $i + strlen($m[0])); - $end = $close === false ? $len : $close + strlen($m[0]); - } elseif ($ch === '-' && substr($sql, $i, 2) === '--') { - $newline = strpos($sql, "\n", $i); - $end = $newline === false ? $len : $newline; - } elseif ($ch === '/' && substr($sql, $i, 2) === '/*') { - $close = strpos($sql, '*/', $i + 2); - $end = $close === false ? $len : $close + 2; - } - + $end = self::literalEnd($sql, $i) ?? self::commentEnd($sql, $i); if ($end !== null) { $current .= substr($sql, $i, $end - $i); $i = $end; continue; } - - if ($ch === ';') { + if ($sql[$i] === ';') { $statements[] = trim($current); $current = ''; } else { - $current .= $ch; + $current .= $sql[$i]; } $i++; } @@ -120,26 +87,105 @@ class SqlGuard } /** - * Replaces comments with a space. A comment separates tokens, so - * dropping it without a space would glue "TO/ ** /PROGRAM" into one - * word and hide it from the patterns above. + * Replaces comments with a space, outside of literals only. A comment + * separates tokens, so dropping it without a space would glue + * "TO/ ** /PROGRAM" into one word; and a comment marker inside a + * literal ('/*') must not swallow the code after it. */ public static function removeComments(string $query): string { - $query = preg_replace('/--[^\n]*/', ' ', $query); - return preg_replace('/\/\*[\s\S]*?\*\//', ' ', $query); + return self::scan($query, false); } /** - * Replaces string literals and quoted identifiers with a placeholder, - * so words inside them ('deleted', "update") don't count as keywords. + * Also replaces string literals and quoted identifiers with a + * placeholder, so words inside them ('deleted', "update") don't count + * as keywords. */ public static function removeLiterals(string $query): string { - $query = preg_replace('/\$([A-Za-z_][A-Za-z0-9_]*|)\$[\s\S]*?\$\1\$/', "''", $query); - $query = preg_replace("/[Ee]'(?:[^'\\\\]|\\\\.|'')*'/", "''", $query); - $query = preg_replace("/'(?:[^']|'')*'/", "''", $query); - return preg_replace('/"(?:[^"]|"")*"/', '""', $query); + return self::scan($query, true); + } + + private static function scan(string $sql, bool $replaceLiterals): string + { + $result = ''; + $len = strlen($sql); + $i = 0; + + while ($i < $len) { + $end = self::literalEnd($sql, $i); + if ($end !== null) { + $result .= $replaceLiterals ? ($sql[$i] === '"' ? '""' : "''") : substr($sql, $i, $end - $i); + $i = $end; + continue; + } + $end = self::commentEnd($sql, $i); + if ($end !== null) { + $result .= ' '; + $i = $end; + continue; + } + $result .= $sql[$i]; + $i++; + } + + return $result; + } + + /** + * End offset of the 'string', E'string', "identifier" or $tag$body$tag$ + * starting at $i, or null if none starts there. An unclosed literal + * runs to the end. + */ + private static function literalEnd(string $sql, int $i): ?int + { + $len = strlen($sql); + $ch = $sql[$i]; + + if ($ch === "'" || $ch === '"') { + $backslashEscapes = $ch === "'" && $i > 0 && ($sql[$i - 1] === 'E' || $sql[$i - 1] === 'e') + && ($i === 1 || !preg_match('/[A-Za-z0-9_]/', $sql[$i - 2])); + $end = $i + 1; + while ($end < $len) { + if ($backslashEscapes && $sql[$end] === '\\') { + $end += 2; + continue; + } + if ($sql[$end] === $ch) { + if ($end + 1 < $len && $sql[$end + 1] === $ch) { + $end += 2; + continue; + } + return $end + 1; + } + $end++; + } + return $len; + } + + if ($ch === '$' && preg_match('/\G\$([A-Za-z_][A-Za-z0-9_]*)?\$/', $sql, $m, 0, $i)) { + $close = strpos($sql, $m[0], $i + strlen($m[0])); + return $close === false ? $len : $close + strlen($m[0]); + } + + return null; + } + + /** + * End offset of the -- or block comment starting at $i, or null. + */ + private static function commentEnd(string $sql, int $i): ?int + { + if (substr($sql, $i, 2) === '--') { + $newline = strpos($sql, "\n", $i); + return $newline === false ? strlen($sql) : $newline; + } + if (substr($sql, $i, 2) === '/*') { + $close = strpos($sql, '*/', $i + 2); + return $close === false ? strlen($sql) : $close + 2; + } + return null; } /** diff --git a/tests/Unit/Service/SqlGuardTest.php b/tests/Unit/Service/SqlGuardTest.php index cb228a8..27ad54a 100644 --- a/tests/Unit/Service/SqlGuardTest.php +++ b/tests/Unit/Service/SqlGuardTest.php @@ -24,6 +24,11 @@ class SqlGuardTest extends TestCase ["SELECT lo_import('/etc/passwd')"], ["SELECT dblink_connect('host=x')"], ["SELECT * FROM t INTO OUTFILE '/tmp/x'"], + ["COPY (SELECT '/*') TO PROGRAM 'sh' --*/"], + ["COPY (SELECT '--') TO PROGRAM 'sh'"], + ["COPY (SELECT \$\$/*\$\$) TO PROGRAM 'sh' --*/"], + ["COPY (SELECT \"/*\") TO PROGRAM 'sh' --*/"], + ["COPY (SELECT E'\\'/*') TO PROGRAM 'sh' --*/"], ]; } @@ -69,6 +74,7 @@ class SqlGuardTest extends TestCase ["SELECT \$\$DELETE FROM x\$\$"], ["SELECT E'it\\'s delete' FROM t"], ['VALUES (1)'], + ["SELECT '/* not a comment */' AS a, '-- nor this' AS b"], ]; } @@ -106,6 +112,9 @@ class SqlGuardTest extends TestCase ["SELECT setval('seq', 1)"], ['MERGE INTO t USING s ON true WHEN MATCHED THEN DELETE'], ['VACUUM'], + ["WITH a AS (SELECT '/*'), b AS (DELETE FROM t RETURNING 1) SELECT '*/'"], + ["SELECT '--' AS x FROM t WHERE a IN (SELECT 1) FOR UPDATE"], + ["WITH a AS (SELECT \$\$/*\$\$), b AS (DELETE FROM t RETURNING 1) SELECT '*/'"], ['', ], ]; }