Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sqld: reject ATTACH statements #1841

Merged
merged 2 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion libsql-server/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand All @@ -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),
}
}
}
Expand Down
28 changes: 26 additions & 2 deletions libsql-server/src/namespace/configurator/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
sivukhin marked this conversation as resolved.
Show resolved Hide resolved
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") {
LucioFranco marked this conversation as resolved.
Show resolved Hide resolved
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)
}
Expand Down
146 changes: 146 additions & 0 deletions libsql-server/tests/namespaces/dumps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Original file line number Diff line number Diff line change
@@ -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"}
Original file line number Diff line number Diff line change
@@ -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"}
Loading