Repository navigation
fix(sqlite): reject SQLite URLs with user, password, or port #998
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ use async_trait::async_trait; | |
| use {sqlx::sqlite::SqlitePool, sqlx::sqlite::SqlitePoolOptions}; | ||
|
|
||
| /// SQLite embedded database connector | ||
| #[derive(Debug)] | ||
| pub struct SqliteConnector { | ||
| #[allow(dead_code)] | ||
| config: DatabaseConfig, | ||
|
|
@@ -42,6 +43,30 @@ impl SqliteConnector { | |
| }); | ||
| } | ||
|
|
||
| // SQLite has no network endpoint or credentials. Reject user, password, or port. | ||
| // A host by itself is allowed (e.g. sqlite://relative/path.db). | ||
| if connection_info.username.is_some() | ||
| || connection_info.password.is_some() | ||
| || connection_info.port.is_some() | ||
| { | ||
| let mut found = Vec::new(); | ||
| if connection_info.username.is_some() { | ||
| found.push("user"); | ||
| } | ||
| if connection_info.password.is_some() { | ||
| found.push("password"); | ||
| } | ||
| if connection_info.port.is_some() { | ||
| found.push("port"); | ||
| } | ||
| let found_str = found.join(", "); | ||
| return Err(DataProfilerError::DatabaseConfigError { | ||
| message: format!( | ||
| "SQLite URLs do not support {found_str}. Supported forms: sqlite:///absolute/path.db, sqlite://relative/path.db, sqlite::memory:, or a bare file path" | ||
| ), | ||
| }); | ||
| } | ||
|
|
||
| Ok(Self { | ||
| config, | ||
| connection_info, | ||
|
|
@@ -159,8 +184,7 @@ impl DatabaseConnector for SqliteConnector { | |
| use sqlx::Row; | ||
|
|
||
| let pool = self.pool.as_ref().ok_or_else(not_connected_error)?; | ||
|
|
||
| let query = format!("PRAGMA table_info({})", table_name); | ||
| let query = format!("PRAGMA table_info({table_name})"); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to the fix, please revert this, the |
||
|
|
||
| let rows = sqlx::query(&query).fetch_all(pool).await.map_err(|e| { | ||
| DataProfilerError::database_query(&format!("Failed to get table schema: {}", e)) | ||
|
|
@@ -188,14 +212,13 @@ impl DatabaseConnector for SqliteConnector { | |
| let pool = self.pool.as_ref().ok_or_else(not_connected_error)?; | ||
|
|
||
| validate_sql_identifier(table_name)?; | ||
| let query = format!("SELECT COUNT(*) FROM {}", table_name); | ||
| let query = format!("SELECT COUNT(*) FROM {table_name}"); | ||
| let count: i64 = sqlx::query_scalar(&query) | ||
| .fetch_one(pool) | ||
| .await | ||
| .map_err(|e| { | ||
| DataProfilerError::database_query(&format!("Failed to count rows: {}", e)) | ||
| })?; | ||
|
|
||
| Ok(count as u64) | ||
| } | ||
|
|
||
|
|
@@ -207,7 +230,6 @@ impl DatabaseConnector for SqliteConnector { | |
| #[cfg(feature = "sqlite")] | ||
| { | ||
| let pool = self.pool.as_ref().ok_or_else(not_connected_error)?; | ||
|
|
||
| let result: i32 = sqlx::query_scalar("SELECT 1") | ||
| .fetch_one(pool) | ||
| .await | ||
|
|
@@ -231,3 +253,62 @@ impl crate::DecodeStructured for sqlx::sqlite::SqliteRow { | |
| None | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::DatabaseConfig; | ||
|
|
||
| fn config(url: &str) -> DatabaseConfig { | ||
| DatabaseConfig { | ||
| connection_string: url.to_string(), | ||
| ..Default::default() | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_user() { | ||
| let err = SqliteConnector::new(config("sqlite://user@host/data.db")).unwrap_err(); | ||
| let msg = err.to_string(); | ||
| assert!(msg.contains("user"), "{msg}"); | ||
| assert!(msg.contains("Supported forms"), "{msg}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_password() { | ||
| let err = SqliteConnector::new(config("sqlite://:pw@host/data.db")).unwrap_err(); | ||
| let msg = err.to_string(); | ||
| assert!(msg.contains("password"), "{msg}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_port() { | ||
| let err = SqliteConnector::new(config("sqlite://host:5432/data.db")).unwrap_err(); | ||
| let msg = err.to_string(); | ||
| assert!(msg.contains("port"), "{msg}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn rejects_user_password_and_port() { | ||
| let err = SqliteConnector::new(config("sqlite://user:pw@host:5432/data.db")).unwrap_err(); | ||
| let msg = err.to_string(); | ||
| assert!(msg.contains("user"), "{msg}"); | ||
| assert!(msg.contains("password"), "{msg}"); | ||
| assert!(msg.contains("port"), "{msg}"); | ||
| // Must not include the actual password. | ||
| assert!(!msg.contains("pw"), "{msg}"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn accepts_supported_forms() { | ||
| for url in [ | ||
| "sqlite:///absolute/path.db", | ||
| "sqlite://relative/path.db", | ||
| "sqlite::memory:", | ||
| "/absolute/path.db", | ||
| "relative.db", | ||
| ] { | ||
| SqliteConnector::new(config(url)).unwrap_or_else(|e| panic!("should accept {url}: {e}")); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rustfmt splits this over two lines, which is what fails Code Quality. |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a public
Debugimpl only so the tests can callunwrap_err(). The Postgres and MySQL connectors don't derive it either. The tests can do without: