Whitelist H2 commands for actions (#28583)

* Decouple checking ddl from classifying h2 stmts

- should enable followup for easily blocking more kinds of queries
- check all statements to make sure they aren't "ddl".

* fix classify-query

* linter fixes + get-field refactor

* return the CommandInterface values as ints

Reach into the CommandList when needed

* docstring wording

* catch invalid queries -- they can't be classified

* Remove action subtypes from inlined-models

* Add ddl check for `execute-write-query!`

* Remove ACCESS_MODE_DATA

* Check queries are single select statements

* Add test for sample database privileges

* Fix single-select check

* Add single-select test

* Rename and add more tests for checking read only commands

* commands -> statements

* Fix check-disallow-ddl-commands

* new line

* Add more read-only statements to the tests

* Update error text

* Use are

* Add integration test for executing actions with disallowed commands

* Add test before inserting row

* Run GRANT ALL ON SCHEMA "PUBLIC" TO GUEST

* Restore classify-query

* whitespace

* Whitelist command types for actions

* Add comment

* Rename

* Remove dupes

* Add truncate test

* Add DDL commands

* Rename to query-classification

* Update error message

* Fix test

---------

Co-authored-by: Bryan Maass <bryan.maass@gmail.com>

src/metabase/driver/h2.clj

@@ -164,25 +164,27 @@
       (catch org.h2.message.DbException _
         {:command-types [] :remaining-sql nil}))))
 
-(defn- cmd-type-ddl? [cmd-type]
-  ;; Command types are organized with all DDL commands listed first, so all ddl commands are before ALTER_SEQUENCE.
-  ;; see https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/command/CommandInterface.java#L297
-  (< cmd-type CommandInterface/ALTER_SEQUENCE))
-
-(defn- contains-ddl? [{:keys [command-types remaining-sql]}]
+(defn- every-command-allowed-for-actions? [{:keys [command-types remaining-sql]}]
   (let [cmd-type-nums command-types]
     (boolean
-     (or (some cmd-type-ddl? cmd-type-nums)
-         (some? remaining-sql)))))
-
-;; TODO: black-list RUNSCRIPT, and a bunch more -- but they're not technically ddl. Should be simple to build off of [[classify-query]].
-;; e.g.: similar to contains-ddl? but instead of cmd-type-ddl? use: #(#{CommandInterface/RUNSCRIPT} %)
+     ;; Command types are organized with all DDL commands listed first, so all ddl commands are before ALTER_SEQUENCE.
+     ;; see https://github.com/h2database/h2database/blob/master/h2/src/main/org/h2/command/CommandInterface.java#L297
+     (and (every? #{CommandInterface/INSERT
+                    CommandInterface/MERGE
+                    CommandInterface/TRUNCATE_TABLE
+                    CommandInterface/UPDATE
+                    CommandInterface/DELETE
+                    ;; Read-only commands might not make sense for actions, but they are allowed
+                    CommandInterface/SELECT ; includes SHOW, TABLE, VALUES
+                    CommandInterface/EXPLAIN
+                    CommandInterface/CALL} cmd-type-nums)
+          (nil? remaining-sql)))))
 
-(defn- check-disallow-ddl-commands [{:keys [database] {:keys [query]} :native}]
+(defn- check-action-commands-allowed [{:keys [database] {:keys [query]} :native}]
   (when query
     (when-let [query-classification (classify-query database query)]
-      (when (contains-ddl? query-classification)
-        (throw (ex-info "IllegalArgument: DDL commands are not allowed to be used with h2."
+      (when-not (every-command-allowed-for-actions? query-classification)
+        (throw (ex-info "DDL commands are not allowed to be used with H2."
                         {:classification query-classification}))))))
 
 (defn- read-only-statements? [{:keys [command-types remaining-sql]}]
@@ -209,7 +211,7 @@
 (defmethod driver/execute-write-query! :h2
   [driver query]
   (check-native-query-not-using-default-user query)
-  (check-disallow-ddl-commands query)
+  (check-action-commands-allowed query)
   ((get-method driver/execute-write-query! :sql-jdbc) driver query))
 
 (defmethod sql.qp/add-interval-honeysql-form :h2

test/metabase/driver/h2_test.clj

@@ -168,9 +168,9 @@
                  (some-> (qp/compile query) :query pretty-sql))))))))
 
 
-(deftest classify-ddl-test
+(deftest check-action-commands-test
   (mt/test-driver :h2
-    (are [query] (= false (#'h2/contains-ddl? (#'h2/classify-query (u/the-id (mt/db)) query)))
+    (are [query] (= true (#'h2/every-command-allowed-for-actions? (#'h2/classify-query (u/the-id (mt/db)) query)))
       "select 1"
       "update venues set name = 'bill'"
       "delete venues"
@@ -179,9 +179,16 @@
        delete venues;"
       "update venues set name = 'stomp';"
       "select * from venues; update venues set name = 'stomp';"
-      "update venues set name = 'stomp'; select * from venues;")
+      "update venues set name = 'stomp'; select * from venues;"
+      "truncate table venues"
+      "insert into venues values (1, 'Chicken Chow')"
+      "merge into venues key(1) values (1, 'Chicken Chow')"
+      "merge into venues using (select 1 as id) as source on (venues.id = source.id) when matched then update set name = 'Chicken Chow';")
 
-    (are [query] (= true (#'h2/contains-ddl? (#'h2/classify-query (u/the-id (mt/db)) query)))
+    (are [query] (= false (#'h2/every-command-allowed-for-actions? (#'h2/classify-query (u/the-id (mt/db)) query)))
+      "create table venues (id int, name varchar(255))"
+      "alter table venues add column address varchar(255)"
+      "drop table venues"
       "select * from venues; update venues set name = 'stomp';
        CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';
        EXEC ('open -a Calculator.app')"
@@ -189,9 +196,9 @@
        CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';"
       "CREATE ALIAS EXEC AS 'String shellexec(String cmd) throws java.io.IOException {Runtime.getRuntime().exec(cmd);return \"y4tacker\";}';")
 
-    (is (= nil (#'h2/check-disallow-ddl-commands {:database (u/the-id (mt/db)) :native {:query nil}})))
+    (is (= nil (#'h2/check-action-commands-allowed {:database (u/the-id (mt/db)) :native {:query nil}})))
 
-    (is (= nil (#'h2/check-disallow-ddl-commands
+    (is (= nil (#'h2/check-action-commands-allowed
                 {:database (u/the-id (mt/db))
                  :engine :h2
                  :native {:query (str/join "; "
@@ -203,7 +210,7 @@
                                                    "SELECT * FROM INFORMATION_SCHEMA.Users;"])]
       (is (thrown? clojure.lang.ExceptionInfo
                    #"DDL commands are not allowed to be used with h2."
-                   (#'h2/check-disallow-ddl-commands
+                   (#'h2/check-action-commands-allowed
                     {:database (u/the-id (mt/db))
                      :engine :h2
                      :native {:query trigger-creation-attempt}}))))))
@@ -251,7 +258,7 @@
                                                  :dataset_query {:database (mt/id)
                                                                  :type     "native"
                                                                  :native   {:query sql}}}]
-            (is (=? {:message "Error executing Action: IllegalArgument: DDL commands are not allowed to be used with h2."}
+            (is (=? {:message "Error executing Action: DDL commands are not allowed to be used with H2."}
                     (mt/user-http-request :crowberto
                                           :post 500
                                           (format "action/%s/execute" action-id)))))))