Merge pull request #3503 from spinframework/allow-attach-file

sqlite-inproc: Add allow_attach_file runtime config option

Author: lann

crates/sqlite-inproc/Cargo.toml

@@ -8,7 +8,7 @@ edition = { workspace = true }
 anyhow = { workspace = true }
 async-trait = { workspace = true }
 futures = { workspace = true }
-rusqlite = { workspace = true, features = ["bundled"] }
+rusqlite = { workspace = true, features = ["bundled", "hooks"] }
 spin-factor-sqlite = { path = "../factor-sqlite" }
 spin-wasi-async = { path = "../wasi-async" }
 spin-world = { path = "../world" }

crates/sqlite-inproc/src/lib.rs

@@ -46,14 +46,19 @@ impl InProcDatabaseLocation {
 /// A connection to a sqlite database
 pub struct InProcConnection {
     location: InProcDatabaseLocation,
+    allow_attach_file: bool,
     connection: OnceLock<Arc<Mutex<rusqlite::Connection>>>,
 }
 
 impl InProcConnection {
-    pub fn new(location: InProcDatabaseLocation) -> Result<Self, sqlite::Error> {
+    pub fn new(
+        location: InProcDatabaseLocation,
+        allow_attach_file: bool,
+    ) -> Result<Self, sqlite::Error> {
         let connection = OnceLock::new();
         Ok(Self {
             location,
+            allow_attach_file,
             connection,
         })
     }
@@ -74,6 +79,18 @@ impl InProcConnection {
             InProcDatabaseLocation::Path(path) => rusqlite::Connection::open(path),
         }
         .map_err(|e| sqlite::Error::Io(e.to_string()))?;
+        if !self.allow_attach_file {
+            connection.authorizer(Some(|ctx: rusqlite::hooks::AuthContext<'_>| {
+                use rusqlite::hooks::{AuthAction, Authorization};
+                match ctx.action {
+                    // Deny attaching files except tempfile ("") and in-memory (":memory:") databases
+                    AuthAction::Attach { filename } if !matches!(filename, "" | ":memory:") => {
+                        Authorization::Deny
+                    }
+                    _ => Authorization::Allow,
+                }
+            }));
+        }
         Ok(Arc::new(Mutex::new(connection)))
     }
 }

crates/sqlite/src/lib.rs

@@ -126,7 +126,7 @@ impl RuntimeConfigResolver {
             .map(|p| p.join(DEFAULT_SQLITE_DB_FILENAME));
         let factory = move || {
             let location = InProcDatabaseLocation::from_path(path.clone())?;
-            let connection = spin_sqlite_inproc::InProcConnection::new(location)?;
+            let connection = spin_sqlite_inproc::InProcConnection::new(location, false)?;
             Ok(Arc::new(connection) as _)
         };
         Arc::new(factory)
@@ -140,6 +140,13 @@ const DEFAULT_SQLITE_DB_FILENAME: &str = "sqlite_db.db";
 #[serde(deny_unknown_fields)]
 pub struct InProcDatabase {
     pub path: Option<PathBuf>,
+
+    /// If `false` (the default), disallows `ATTACH`ing an existing file to a
+    /// database connection.
+    ///
+    /// Note: Attaching a new tempfile or `:memory:` database is always allowed.
+    #[serde(default)]
+    pub allow_attach_file: bool,
 }
 
 impl InProcDatabase {
@@ -156,7 +163,10 @@ impl InProcDatabase {
             .map(|p| resolve_relative_path(p, base_dir));
         let location = InProcDatabaseLocation::from_path(path)?;
         let factory = move || {
-            let connection = spin_sqlite_inproc::InProcConnection::new(location.clone())?;
+            let connection = spin_sqlite_inproc::InProcConnection::new(
+                location.clone(),
+                self.allow_attach_file,
+            )?;
             Ok(Arc::new(connection) as _)
         };
         Ok(factory)

tests/test-components/components/sqlite/src/lib.rs

@@ -53,9 +53,23 @@ impl Component {
 
         // This should exceed the 128MB query result limit:
         match conn.execute("SELECT * FROM test_data", &[]) {
-            Ok(_) => bail!("large select should not have succeeded",),
+            Ok(_) => bail!("large select should not have succeeded"),
             Err(Error::Io(s)) if s.contains("query result exceeds limit") => {}
-            Err(e) => bail!("unexpected error: {e}",),
+            Err(e) => bail!("unexpected error: {e}"),
+        }
+
+        // ATTACH tempfile and in-memory are allowed
+        for allowed_stmt in ["ATTACH '' AS tempfile", "ATTACH ':memory:' AS inmemory"] {
+            conn.execute(allowed_stmt, &[])
+                .map_err(|e| format!("{allowed_stmt:?} failed: {e:?}"))?;
+        }
+        // ATTACH <file> forbidden by default; VACUUM INTO uses ATTACH under the hood
+        for forbidden_stmt in ["ATTACH 'any_file' AS attach_file", "VACUUM INTO 'any_file'"] {
+            match conn.execute(forbidden_stmt, &[]) {
+                Ok(_) => bail!("{forbidden_stmt:?} should fail"),
+                Err(Error::Io(s)) if s.contains("authoriz") => {}
+                Err(e) => bail!("unexpected error for {forbidden_stmt:?}: {e:?}"),
+            }
         }
 
         Ok(())

tests/test-components/helper/src/lib.rs

@@ -109,7 +109,7 @@ pub fn outgoing_body(body: OutgoingBody, buffer: Vec<u8>) -> Result<(), ErrorCod
                             offset += count;
                         }
                         Err(e) => {
-                            return Err(ErrorCode::InternalError(Some(format!("I/O error: {e}"))))
+                            return Err(ErrorCode::InternalError(Some(format!("I/O error: {e}"))));
                         }
                     }
                 }
@@ -174,12 +174,12 @@ macro_rules! ensure_eq {
 
 #[macro_export]
 macro_rules! bail {
-    ($fmt:expr, $($arg:tt)*) => {{
+    ($fmt:expr $(, $($arg:tt)*)?) => {{
         let krate = module_path!().split("::").next().unwrap();
         let file = file!();
         let line = line!();
         return Err(format!(
-            "{krate}#({file}:{line}) {}", format_args!($fmt, $($arg)*)
+            "{krate}#({file}:{line}) {}", format_args!($fmt $(, $($arg)*)?)
         ));
     }};
 }