diff --git a/appinfo/info.xml b/appinfo/info.xml index f3a0c6f..6b65490 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -5,7 +5,7 @@ Extended OCC Web OCC Commands in a web terminal - 0.4.4 + 0.4.5 agpl Dillard Blom OCCWeb diff --git a/js/index.js b/js/index.js index 739aa6f..236d619 100644 --- a/js/index.js +++ b/js/index.js @@ -19,10 +19,23 @@ // text (e.g. Symfony's "[alias]" in `occ list`) and leave the ANSI // sequences alone, so from_ansi can still turn them into colors. This // only works together with unixFormattingEscapeBrackets (see below). - function escapeOutputBrackets(output) { - return String(output).replace(/(\x1B\[[0-9;]*[A-Za-z])|[\[\]]/g, function (match, ansi) { - return ansi ? ansi : (match === '[' ? '[' : ']'); - }); + // + // Output can contain text from users (display names, file names), so + // keep only color codes (SGR) and drop other escape sequences, + // backspaces and control characters, which could rewrite or hide text + // on screen. A lone CR becomes a newline. + function sanitizeOccOutput(output) { + return String(output) + .replace(/\x1B\][^\x07\x1B]*(\x07|\x1B\\)?/g, '') + .replace(/(\x1B\[[0-9;]*m)|\x1B\[[0-?]*[ -\/]*[@-~]|\x1B[@-_]?/g, function (match, sgr) { + return sgr || ''; + }) + .replace(/\r\n/g, '\n') + .replace(/\r/g, '\n') + .replace(/[\x00-\x08\x0B\x0C\x0E-\x1A\x1C-\x1F\x7F]/g, '') + .replace(/(\x1B\[[0-9;]*m)|[\[\]]/g, function (match, sgr) { + return sgr ? sgr : (match === '[' ? '[' : ']'); + }); } // Current terminal mode: 'occ' (regular occ commands) or 'sql' @@ -74,7 +87,8 @@ // Quick client-side check for DELETE/UPDATE — purely for UX (to avoid // an extra round trip to the server). The backend always makes the - // final call (requiresConfirmation); this is just a hint. + // final call (requiresConfirmation) and also covers every other + // statement that changes something; this is just a hint. function scriptNeedsConfirmation(sql) { return splitStatements(sql).some(function (part) { var normalized = part.replace(/^(\s*--[^\n]*\n)*\s*/, ''); @@ -326,7 +340,8 @@ } function askDeleteConfirmation(term, sql, message) { - var prompt = '[[;#ff5555;]' + (message || 'This script contains DELETE/UPDATE statement(s).') + ' Type "yes" to run it: ]'; + var text = message || 'This script contains statement(s) that change data or the schema.'; + var prompt = '[[;#ff5555;]' + $.terminal.escape_brackets(text) + ' Type "yes" to run it: ]'; term.read(prompt).then(function (answer) { if ((answer || '').trim().toLowerCase() === 'yes') { sendSqlQuery(term, sql, true); @@ -397,7 +412,7 @@ headers: { requesttoken: OC.requestToken }, data: JSON.stringify(occCommand) }).done(function (response) { - term.echo('\n' + escapeOutputBrackets(response)).resume(); + term.echo('\n' + sanitizeOccOutput(response)).resume(); }).fail(function (xhr, status) { term.echo('\n[[;#ff5555;]Request failed: ]' + $.terminal.escape_formatting(xhr.status + ' ' + xhr.statusText)).resume(); }); @@ -412,6 +427,9 @@ // The overtyping and from_ansi formatters unescape brackets before // parsing; this makes them escape the text again afterwards. unixFormattingEscapeBrackets: true, + // Output can contain user-controlled text; don't turn URLs in it + // into links an admin might click by mistake. + convertLinks: false, keydown: function (e) { // Shift+Enter inserts a newline instead of running the command, // letting you type multi-line SQL scripts in sql mode. diff --git a/lib/Controller/DbController.php b/lib/Controller/DbController.php index 994d645..fe7f36c 100644 --- a/lib/Controller/DbController.php +++ b/lib/Controller/DbController.php @@ -3,6 +3,7 @@ namespace OCA\OCCWeb\Controller; use OC; +use OCA\OCCWeb\Service\SqlGuard; use OCP\AppFramework\Controller; use OCP\IRequest; use OCP\IDBConnection; @@ -19,26 +20,6 @@ class DbController extends Controller /** Maximum size of submitted SQL text in bytes (protection against OOM/DoS). */ private const MAX_SQL_BYTES = 1048576; // 1 MB - /** - * Constructs that give access to the server's filesystem or allow - * launching external programs via SQL. These are blocked entirely, - * with no way to confirm past them — unlike DELETE, this isn't - * about data loss, it's about potential server takeover. - */ - private const FORBIDDEN_PATTERNS = [ - '/\bCOPY\b[\s\S]*\bPROGRAM\b/i' => 'COPY ... PROGRAM (runs external commands)', - '/\bCOPY\b[\s\S]*\b(FROM|TO)\b\s*\'/i' => "COPY ... FROM/TO 'file' (access to the server's filesystem)", - '/\bpg_read_binary_file\s*\(/i' => 'pg_read_binary_file()', - '/\bpg_read_file\s*\(/i' => 'pg_read_file()', - '/\bpg_ls_dir\s*\(/i' => 'pg_ls_dir()', - '/\bpg_stat_file\s*\(/i' => 'pg_stat_file()', - '/\blo_import\s*\(/i' => 'lo_import()', - '/\blo_export\s*\(/i' => 'lo_export()', - '/\bdblink(_connect)?\s*\(/i' => 'dblink() (connects to arbitrary databases)', - '/\bLOAD_FILE\s*\(/i' => 'LOAD_FILE()', - '/\bINTO\s+(OUTFILE|DUMPFILE)\b/i' => 'INTO OUTFILE/DUMPFILE', - ]; - private $db; private $groupManager; private $userSession; @@ -60,35 +41,6 @@ class DbController extends Controller $this->logger = OC::$server->get(LoggerInterface::class); } - /** - * Strips leading single-line comments ("-- ...") before a query. - * After splitting a batch, such a comment can end up stuck to the - * next query and interfere with detecting its type (SELECT/SET/DELETE). - */ - private function stripLeadingComments($query) - { - $query = ltrim($query); - while (preg_match('/^--[^\n]*\n/', $query)) { - $query = ltrim(preg_replace('/^--[^\n]*\n/', '', $query, 1)); - } - return $query; - } - - /** - * Strips ALL single-line (-- ...) and block-style C comments from a - * query, including ones sitting inside an expression (e.g. between a - * function name and its opening parenthesis — otherwise a forbidden - * construct could be hidden from findForbiddenConstruct that way). - * Used only for checking against forbidden constructs — the query - * that actually gets executed is left unmodified. - */ - private function removeAllComments($query) - { - $query = preg_replace('/--[^\n]*/', '', $query); - $query = preg_replace('/\/\*[\s\S]*?\*\//', '', $query); - return $query; - } - /** * Splits a batch of queries on ";" while respecting single and double * quotes (strings and Postgres-escaped identifiers), so that a @@ -142,24 +94,6 @@ class DbController extends Controller })); } - /** - * Returns a description of the forbidden construct found (filesystem - * access, running programs), or null if the query is safe in that - * regard. The comment-stripped version of the query is checked — - * otherwise the construct could be hidden by inserting a comment - * between the function name and "(". - */ - private function findForbiddenConstruct($query) - { - $clean = $this->removeAllComments($query); - foreach (self::FORBIDDEN_PATTERNS as $pattern => $label) { - if (preg_match($pattern, $clean)) { - return $label; - } - } - return null; - } - public function query() { // Deliberate defense-in-depth (see the equivalent check in @@ -210,7 +144,7 @@ class DbController extends Controller // blocked entirely — this isn't about data loss (like DELETE), it's // about potential server takeover, and confirmation doesn't bypass it. foreach ($queries as $query) { - $forbidden = $this->findForbiddenConstruct($query); + $forbidden = SqlGuard::findForbiddenConstruct($query); if ($forbidden !== null) { $this->logger->error('[extended_occweb] Blocked forbidden construct ({construct}) from {user}: {sql}', [ 'app' => 'extended_occweb', @@ -225,34 +159,29 @@ class DbController extends Controller } } - // DELETE and UPDATE are irreversible (or hard to reverse), so we - // require explicit confirmation from the client (confirm=true) before - // executing a single query from the batch. - $deleteCount = 0; - $updateCount = 0; + // Anything that isn't a plain read (DELETE, UPDATE, INSERT, DROP, + // a data-modifying CTE, ...) needs explicit confirmation from the + // client (confirm=true) before a single query of the batch runs. + $writeTypes = []; foreach ($queries as $query) { - $normalized = $this->stripLeadingComments($query); - if (stripos($normalized, 'DELETE') === 0) { - $deleteCount++; - } elseif (stripos($normalized, 'UPDATE') === 0) { - $updateCount++; + if (!SqlGuard::isReadOnly($query)) { + $type = SqlGuard::statementType($query) ?: 'UNKNOWN'; + $writeTypes[$type] = ($writeTypes[$type] ?? 0) + 1; } } - if (($deleteCount > 0 || $updateCount > 0) && !$confirmed) { + if ($writeTypes !== [] && !$confirmed) { $parts = []; - if ($deleteCount > 0) { - $parts[] = "{$deleteCount} DELETE"; - } - if ($updateCount > 0) { - $parts[] = "{$updateCount} UPDATE"; + foreach ($writeTypes as $type => $count) { + $parts[] = "{$count} {$type}"; } return new JSONResponse([ 'success' => false, 'requiresConfirmation' => true, - 'deleteCount' => $deleteCount, - 'updateCount' => $updateCount, - 'error' => 'Batch contains ' . implode(' and ', $parts) . ' statement(s) and was not executed. Resend with confirm=true to proceed.' + 'deleteCount' => $writeTypes['DELETE'] ?? 0, + 'updateCount' => $writeTypes['UPDATE'] ?? 0, + 'writeCount' => array_sum($writeTypes), + 'error' => 'Batch contains statement(s) that change data or the schema (' . implode(', ', $parts) . ') and was not executed. Resend with confirm=true to proceed.' ]); } @@ -289,17 +218,16 @@ class DbController extends Controller } foreach ($queries as $query) { - $normalized = $this->stripLeadingComments($query); - $isSelect = stripos($normalized, 'SELECT') === 0; - $isSet = stripos($normalized, 'SET ') === 0; - $isDelete = stripos($normalized, 'DELETE') === 0; - $isUpdate = stripos($normalized, 'UPDATE') === 0; + $statementType = SqlGuard::statementType($query); + $isSet = $statementType === 'SET'; + $isDelete = $statementType === 'DELETE'; + $isUpdate = $statementType === 'UPDATE'; try { $stmt = $this->db->prepare($query); $stmt->execute(); - if ($isSelect) { + if ($statementType === 'SELECT' || ($statementType === 'WITH' && SqlGuard::isReadOnly($query))) { // Read row by row and stop at MAX_ROWS, instead of // fetchAll() + array_slice — otherwise a SELECT without // LIMIT on a huge table would still pull everything into diff --git a/lib/Service/SqlGuard.php b/lib/Service/SqlGuard.php new file mode 100644 index 0000000..551f78c --- /dev/null +++ b/lib/Service/SqlGuard.php @@ -0,0 +1,115 @@ + 'COPY ... PROGRAM (runs external commands)', + // A COPY file name is a string literal: 'x', E'x', U&'x' or $$x$$ + '/\bCOPY\b[\s\S]*\b(FROM|TO)\s+(E|U&)?(\'|\$)/i' => "COPY ... FROM/TO 'file' (access to the server's filesystem)", + '/\bpg_read_binary_file\s*\(/i' => 'pg_read_binary_file()', + '/\bpg_read_file\s*\(/i' => 'pg_read_file()', + '/\bpg_ls_dir\s*\(/i' => 'pg_ls_dir()', + '/\bpg_stat_file\s*\(/i' => 'pg_stat_file()', + '/\blo_import\s*\(/i' => 'lo_import()', + '/\blo_export\s*\(/i' => 'lo_export()', + '/\bdblink(_connect)?\s*\(/i' => 'dblink() (connects to arbitrary databases)', + '/\bLOAD_FILE\s*\(/i' => 'LOAD_FILE()', + '/\bINTO\s+(OUTFILE|DUMPFILE)\b/i' => 'INTO OUTFILE/DUMPFILE', + ]; + + /** Statements that start with one of these only read, unless WRITES_INSIDE_READ matches. */ + private const READ_ONLY_START = '/^(SELECT|WITH|SHOW|EXPLAIN|SET|VALUES|TABLE)\b/i'; + + /** + * What makes a statement that starts as a read change something: + * a data-modifying CTE (WITH x AS (DELETE ...)), SELECT ... INTO, + * SELECT ... FOR UPDATE, EXPLAIN ANALYZE (runs the statement), + * SET ROLE / SET SESSION AUTHORIZATION, and functions with side + * effects. Statements that don't start as a read always need + * confirmation, so INSERT, DROP, CREATE etc. don't need to be here. + */ + private const WRITES_INSIDE_READ = [ + '/\b(INSERT|UPDATE|DELETE|MERGE|INTO|ANALYZE)\b/i', + '/^SET\s+(SESSION\s+|LOCAL\s+)?(ROLE|SESSION\s+AUTHORIZATION)\b/i', + '/\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', + ]; + + /** + * 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. + */ + public static function removeComments(string $query): string + { + $query = preg_replace('/--[^\n]*/', ' ', $query); + return preg_replace('/\/\*[\s\S]*?\*\//', ' ', $query); + } + + /** + * 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); + } + + /** + * Returns a description of the forbidden construct found, or null. + * Literals are kept here on purpose: a file name is a literal. + */ + public static function findForbiddenConstruct(string $query): ?string + { + $clean = self::removeComments($query); + foreach (self::FORBIDDEN_PATTERNS as $pattern => $label) { + if (preg_match($pattern, $clean)) { + return $label; + } + } + return null; + } + + /** + * The first keyword of a statement, upper case, e.g. SELECT or DELETE. + */ + public static function statementType(string $query): string + { + $clean = ltrim(self::removeComments($query), " \t\n\r\0\x0B("); + return preg_match('/^([A-Za-z]+)/', $clean, $m) ? strtoupper($m[1]) : ''; + } + + /** + * Whether a statement only reads. Anything else needs confirmation. + */ + public static function isReadOnly(string $query): bool + { + $clean = ltrim(self::removeLiterals(self::removeComments($query)), " \t\n\r\0\x0B("); + if (preg_match(self::READ_ONLY_START, $clean) !== 1) { + return false; + } + foreach (self::WRITES_INSIDE_READ as $pattern) { + if (preg_match($pattern, $clean) === 1) { + return false; + } + } + return true; + } +} diff --git a/tests/Unit/Service/SqlGuardTest.php b/tests/Unit/Service/SqlGuardTest.php new file mode 100644 index 0000000..6dde084 --- /dev/null +++ b/tests/Unit/Service/SqlGuardTest.php @@ -0,0 +1,128 @@ +assertNotNull(SqlGuard::findForbiddenConstruct($sql)); + } + + public static function allowedProvider(): array + { + return [ + ['SELECT * FROM oc_users'], + ['COPY (SELECT 1) TO STDOUT'], + ]; + } + + /** + * @dataProvider allowedProvider + */ + public function testAllowed(string $sql): void + { + $this->assertNull(SqlGuard::findForbiddenConstruct($sql)); + } + + public static function readOnlyProvider(): array + { + return [ + ['SELECT * FROM oc_users'], + [' -- comment' . "\n" . 'SELECT 1'], + ['/* comment */ SELECT 1'], + ['(SELECT 1) UNION (SELECT 2)'], + ['WITH x AS (SELECT 1) SELECT * FROM x'], + ["SELECT * FROM oc_jobs WHERE class = 'OCA\\\\Mail\\\\DeleteJob'"], + ["SELECT * FROM t WHERE status = 'deleted' OR note = 'update into'"], + ['SELECT "update" FROM t'], + ["SELECT replace(name, 'a', 'b') FROM t"], + ['SHOW search_path'], + ['EXPLAIN SELECT 1'], + ["SET myvar.uid = 'alice'"], + ["SELECT \$\$DELETE FROM x\$\$"], + ["SELECT E'it\\'s delete' FROM t"], + ['VALUES (1)'], + ]; + } + + /** + * @dataProvider readOnlyProvider + */ + public function testReadOnly(string $sql): void + { + $this->assertTrue(SqlGuard::isReadOnly($sql), $sql); + } + + public static function writeProvider(): array + { + return [ + ['DELETE FROM oc_preferences'], + ['/* not a leading -- comment */ DELETE FROM oc_preferences'], + ["-- x\nUPDATE t SET a = 1"], + ['WITH removed AS (DELETE FROM oc_preferences RETURNING *) SELECT count(*) FROM removed'], + ['WITH x AS (UPDATE t SET a = 1 RETURNING *) SELECT 1'], + ['INSERT INTO t VALUES (1)'], + ['TRUNCATE oc_preferences'], + ['DROP TABLE t'], + ['ALTER TABLE t ADD c int'], + ['CREATE TABLE t (a int)'], + ['GRANT ALL ON t TO x'], + ["DO \$\$BEGIN PERFORM 1; END\$\$"], + ['CALL p()'], + ['SELECT * INTO newtable FROM t'], + ['SELECT * FROM t FOR UPDATE'], + ['EXPLAIN ANALYZE DELETE FROM t'], + ['SET ROLE postgres'], + ['SET SESSION AUTHORIZATION postgres'], + ['SELECT pg_terminate_backend(123)'], + ["SELECT set_config('x', 'y', false)"], + ["SELECT setval('seq', 1)"], + ['MERGE INTO t USING s ON true WHEN MATCHED THEN DELETE'], + ['VACUUM'], + ['', ], + ]; + } + + /** + * @dataProvider writeProvider + */ + public function testNeedsConfirmation(string $sql): void + { + $this->assertFalse(SqlGuard::isReadOnly($sql), $sql); + } + + public function testStatementType(): void + { + $this->assertSame('DELETE', SqlGuard::statementType('/* x */ delete from t')); + $this->assertSame('WITH', SqlGuard::statementType("-- x\nWITH a AS (SELECT 1) SELECT 1")); + $this->assertSame('SELECT', SqlGuard::statementType('(SELECT 1)')); + $this->assertSame('', SqlGuard::statementType('')); + } +}