Merge pull request #1841 from tursodatabase/lucio/fix-attach-dump

sqld: reject `ATTACH` statements

Author: LucioFranco

libsql-server/src/error.rs

@@ -297,6 +297,8 @@ pub enum LoadDumpError {
     NoCommit,
     #[error("Path is not a file")]
     NotAFile,
+    #[error("The passed dump sql is invalid: {0}")]
+    InvalidSqlInput(String),
 }
 
 impl ResponseError for LoadDumpError {}
@@ -315,7 +317,8 @@ impl IntoResponse for &LoadDumpError {
             | NoTxn
             | NoCommit
             | NotAFile
-            | DumpFilePathNotAbsolute => self.format_err(StatusCode::BAD_REQUEST),
+            | DumpFilePathNotAbsolute
+            | InvalidSqlInput(_) => self.format_err(StatusCode::BAD_REQUEST),
         }
     }
 }

libsql-server/src/namespace/configurator/helpers.rs

@@ -11,6 +11,7 @@ use futures::Stream;
 use libsql_sys::EncryptionConfig;
 use libsql_wal::io::StdIO;
 use libsql_wal::registry::WalRegistry;
+use rusqlite::hooks::{AuthAction, AuthContext, Authorization};
 use tokio::io::AsyncBufReadExt as _;
 use tokio::task::JoinSet;
 use tokio_util::io::StreamReader;
@@ -328,8 +329,31 @@ where
             line = tokio::task::spawn_blocking({
                 let conn = conn.clone();
                 move || -> crate::Result<String, LoadDumpError> {
-                    conn.with_raw(|conn| conn.execute(&line, ())).map_err(|e| {
-                        LoadDumpError::Internal(format!("line: {}, error: {}", line_id, e))
+                    conn.with_raw(|conn| {
+                        conn.authorizer(Some(|auth: AuthContext<'_>| match auth.action {
+                            AuthAction::Attach { filename: _ } => Authorization::Deny,
+                            _ => Authorization::Allow,
+                        }));
+                        conn.execute(&line, ())
+                    })
+                    .map_err(|e| match e {
+                        rusqlite::Error::SqlInputError {
+                            msg, sql, offset, ..
+                        } => {
+                            let msg = if sql.to_lowercase().contains("attach") {
+                                format!(
+                                    "attach statements are not allowed in dumps, msg: {}, sql: {}, offset: {}",
+                                    msg,
+                                    sql,
+                                    offset
+                                )
+                            } else {
+                                format!("msg: {}, sql: {}, offset: {}", msg, sql, offset)
+                            };
+
+                            LoadDumpError::InvalidSqlInput(msg)
+                        }
+                        e => LoadDumpError::Internal(format!("line: {}, error: {}", line_id, e)),
                     })?;
                     Ok(line)
                 }

libsql-server/tests/namespaces/dumps.rs

@@ -279,3 +279,149 @@ fn export_dump() {
 
     sim.run().unwrap();
 }
+
+#[test]
+fn load_dump_with_attach_rejected() {
+    const DUMP: &str = r#"
+        PRAGMA foreign_keys=OFF;
+    BEGIN TRANSACTION;
+    CREATE TABLE test (x);
+    INSERT INTO test VALUES(42);
+    ATTACH foo/bar.sql
+    COMMIT;"#;
+
+    let mut sim = Builder::new()
+        .simulation_duration(Duration::from_secs(1000))
+        .build();
+    let tmp = tempdir().unwrap();
+    let tmp_path = tmp.path().to_path_buf();
+
+    std::fs::write(tmp_path.join("dump.sql"), DUMP).unwrap();
+
+    make_primary(&mut sim, tmp.path().to_path_buf());
+
+    sim.client("client", async move {
+        let client = Client::new();
+
+        // path is not absolute is an error
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": "file:dump.sql"}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        // path doesn't exist is an error
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": "file:/dump.sql"}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": format!("file:{}", tmp_path.join("dump.sql").display())}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(
+            resp.status(),
+            StatusCode::BAD_REQUEST,
+            "{}",
+            resp.json::<serde_json::Value>().await.unwrap_or_default()
+        );
+
+        assert_snapshot!(resp.body_string().await?);
+
+        let foo =
+            Database::open_remote_with_connector("http://foo.primary:8080", "", TurmoilConnector)?;
+        let foo_conn = foo.connect()?;
+
+        let res = foo_conn.query("select count(*) from test", ()).await;
+        // This should error since the dump should have failed!
+        assert!(res.is_err());
+
+        Ok(())
+    });
+
+    sim.run().unwrap();
+}
+
+#[test]
+fn load_dump_with_invalid_sql() {
+    const DUMP: &str = r#"
+        PRAGMA foreign_keys=OFF;
+    BEGIN TRANSACTION;
+    CREATE TABLE test (x);
+    INSERT INTO test VALUES(42);
+    SELECT abs(-9223372036854775808) 
+    COMMIT;"#;
+
+    let mut sim = Builder::new()
+        .simulation_duration(Duration::from_secs(1000))
+        .build();
+    let tmp = tempdir().unwrap();
+    let tmp_path = tmp.path().to_path_buf();
+
+    std::fs::write(tmp_path.join("dump.sql"), DUMP).unwrap();
+
+    make_primary(&mut sim, tmp.path().to_path_buf());
+
+    sim.client("client", async move {
+        let client = Client::new();
+
+        // path is not absolute is an error
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": "file:dump.sql"}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        // path doesn't exist is an error
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": "file:/dump.sql"}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(resp.status(), StatusCode::BAD_REQUEST);
+
+        let resp = client
+            .post(
+                "http://primary:9090/v1/namespaces/foo/create",
+                json!({ "dump_url": format!("file:{}", tmp_path.join("dump.sql").display())}),
+            )
+            .await
+            .unwrap();
+        assert_eq!(
+            resp.status(),
+            StatusCode::BAD_REQUEST,
+            "{}",
+            resp.json::<serde_json::Value>().await.unwrap_or_default()
+        );
+
+        assert_snapshot!(resp.body_string().await?);
+
+        let foo =
+            Database::open_remote_with_connector("http://foo.primary:8080", "", TurmoilConnector)?;
+        let foo_conn = foo.connect()?;
+
+        let res = foo_conn.query("select count(*) from test", ()).await;
+        // This should error since the dump should have failed!
+        assert!(res.is_err());
+
+        Ok(())
+    });
+
+    sim.run().unwrap();
+}

libsql-server/tests/namespaces/snapshots/tests__namespaces__dumps__load_dump_with_attach_rejected.snap

@@ -0,0 +1,6 @@
+---
+source: libsql-server/tests/namespaces/dumps.rs
+expression: resp.body_string().await?
+snapshot_kind: text
+---
+{"error":"The passed dump sql is invalid: attach statements are not allowed in dumps, msg: near \"COMMIT\": syntax error, sql:     ATTACH foo/bar.sql\n     COMMIT;, offset: 28"}

libsql-server/tests/namespaces/snapshots/tests__namespaces__dumps__load_dump_with_invalid_sql.snap

@@ -0,0 +1,6 @@
+---
+source: libsql-server/tests/namespaces/dumps.rs
+expression: resp.body_string().await?
+snapshot_kind: text
+---
+{"error":"The passed dump sql is invalid: msg: near \"COMMIT\": syntax error, sql:     SELECT abs(-9223372036854775808) \n     COMMIT;, offset: 43"}