diff --git a/lib/Controller/DbController.php b/lib/Controller/DbController.php index fe7f36c..2885e24 100644 --- a/lib/Controller/DbController.php +++ b/lib/Controller/DbController.php @@ -41,59 +41,6 @@ class DbController extends Controller $this->logger = OC::$server->get(LoggerInterface::class); } - /** - * Splits a batch of queries on ";" while respecting single and double - * quotes (strings and Postgres-escaped identifiers), so that a - * semicolon inside a 'string' or "identifier" (including '' / "" as - * an escaped quote) doesn't break up the split. - */ - private function splitStatements($sql) - { - $statements = []; - $current = ''; - $len = strlen($sql); - $quoteChar = null; - - for ($i = 0; $i < $len; $i++) { - $ch = $sql[$i]; - - if ($quoteChar !== null) { - if ($ch === $quoteChar) { - if ($i + 1 < $len && $sql[$i + 1] === $quoteChar) { - $current .= $quoteChar . $quoteChar; - $i++; - continue; - } - $quoteChar = null; - } - $current .= $ch; - continue; - } - - if ($ch === "'" || $ch === '"') { - $quoteChar = $ch; - $current .= $ch; - continue; - } - - if ($ch === ';') { - $statements[] = trim($current); - $current = ''; - continue; - } - - $current .= $ch; - } - - if (trim($current) !== '') { - $statements[] = trim($current); - } - - return array_values(array_filter($statements, function ($s) { - return $s !== ''; - })); - } - public function query() { // Deliberate defense-in-depth (see the equivalent check in @@ -138,7 +85,7 @@ class DbController extends Controller // All queries run sequentially on the same DB connection (within a // single HTTP request), so SET retains its value for // current_setting() in the following queries of the same batch. - $queries = $this->splitStatements($sql); + $queries = SqlGuard::splitStatements($sql); // Access to the server's filesystem / running programs via SQL is // blocked entirely — this isn't about data loss (like DELETE), it's diff --git a/lib/Controller/OccController.php b/lib/Controller/OccController.php index 0828646..df331a7 100644 --- a/lib/Controller/OccController.php +++ b/lib/Controller/OccController.php @@ -91,7 +91,7 @@ class OccController extends Controller try { $this->application->run($input, $this->output); return $this->output->fetch(); - } catch (Exception $ex) { + } catch (\Throwable $ex) { $this->logger->error($ex->getMessage(), ['exception' => $ex]); return "error: " . $ex->getMessage(); } diff --git a/lib/Service/SqlGuard.php b/lib/Service/SqlGuard.php index 551f78c..a03515d 100644 --- a/lib/Service/SqlGuard.php +++ b/lib/Service/SqlGuard.php @@ -49,6 +49,76 @@ class SqlGuard '/\b(pg_terminate_backend|pg_cancel_backend|pg_reload_conf|set_config|setval|lo_create|lo_put|lo_unlink|pg_switch_wal|pg_create_restore_point|pg_advisory_lock)\s*\(/i', ]; + /** + * Splits a batch on ";" outside of 'strings', "identifiers", + * $tag$dollar-quoted bodies$tag$ and comments, so a semicolon in a + * DO block or a quote in a comment doesn't break up the split. + * + * @return string[] + */ + public static function splitStatements(string $sql): array + { + $statements = []; + $current = ''; + $len = strlen($sql); + $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; + } + + if ($end !== null) { + $current .= substr($sql, $i, $end - $i); + $i = $end; + continue; + } + + if ($ch === ';') { + $statements[] = trim($current); + $current = ''; + } else { + $current .= $ch; + } + $i++; + } + + $statements[] = trim($current); + + return array_values(array_filter($statements, function ($s) { + return $s !== ''; + })); + } + /** * Replaces comments with a space. A comment separates tokens, so * dropping it without a space would glue "TO/ ** /PROGRAM" into one diff --git a/tests/Unit/Service/SqlGuardTest.php b/tests/Unit/Service/SqlGuardTest.php index 6dde084..cb228a8 100644 --- a/tests/Unit/Service/SqlGuardTest.php +++ b/tests/Unit/Service/SqlGuardTest.php @@ -125,4 +125,29 @@ class SqlGuardTest extends TestCase $this->assertSame('SELECT', SqlGuard::statementType('(SELECT 1)')); $this->assertSame('', SqlGuard::statementType('')); } + + public static function splitProvider(): array + { + return [ + ['SELECT 1; SELECT 2', ['SELECT 1', 'SELECT 2']], + ["SELECT ';'; SELECT 2", ["SELECT ';'", 'SELECT 2']], + ["SELECT 'it''s; x'", ["SELECT 'it''s; x'"]], + ['SELECT "a;b" FROM t', ['SELECT "a;b" FROM t']], + ["SELECT E'it\\'s; x'; SELECT 2", ["SELECT E'it\\'s; x'", 'SELECT 2']], + ["DO \$\$BEGIN PERFORM 1; PERFORM 2; END\$\$; SELECT 3", ["DO \$\$BEGIN PERFORM 1; PERFORM 2; END\$\$", 'SELECT 3']], + ["SELECT \$f\$a;\$\$;b\$f\$; SELECT 2", ["SELECT \$f\$a;\$\$;b\$f\$", 'SELECT 2']], + ["-- don't; split\nSELECT 1; SELECT 2", ["-- don't; split\nSELECT 1", 'SELECT 2']], + ["/* it's; here */ SELECT 1; SELECT 2", ["/* it's; here */ SELECT 1", 'SELECT 2']], + ['SELECT \$1; SELECT 2', ['SELECT \$1', 'SELECT 2']], + [' ; ;SELECT 1;', ['SELECT 1']], + ]; + } + + /** + * @dataProvider splitProvider + */ + public function testSplitStatements(string $sql, array $expected): void + { + $this->assertSame($expected, SqlGuard::splitStatements($sql)); + } }