diff --git a/trailbase-core/src/admin/list_logs.rs b/trailbase-core/src/admin/list_logs.rs index e8af3a8b..5ac567a3 100644 --- a/trailbase-core/src/admin/list_logs.rs +++ b/trailbase-core/src/admin/list_logs.rs @@ -15,8 +15,8 @@ use crate::admin::AdminError as Error; use crate::app_state::AppState; use crate::constants::{LOGS_RETENTION_DEFAULT, LOGS_TABLE_ID_COLUMN}; use crate::listing::{ - build_filter_where_clause, limit_or_default, parse_query, Cursor, Order, QueryParseResult, - WhereClause, + build_filter_where_clause, limit_or_default, parse_and_sanitize_query, Cursor, Order, + QueryParseResult, WhereClause, }; use crate::table_metadata::{lookup_and_parse_table_schema, TableMetadata}; @@ -115,7 +115,7 @@ pub async fn list_logs_handler( limit, order, .. - } = parse_query(raw_url_query.as_deref()) + } = parse_and_sanitize_query(raw_url_query.as_deref()) .map_err(|err| Error::Precondition(format!("Invalid query '{err}': {raw_url_query:?}")))?; // NOTE: We cannot use state.table_metadata() here, since we're working on the logs database. diff --git a/trailbase-core/src/admin/rows/list_rows.rs b/trailbase-core/src/admin/rows/list_rows.rs index fbc20ec0..215263c7 100644 --- a/trailbase-core/src/admin/rows/list_rows.rs +++ b/trailbase-core/src/admin/rows/list_rows.rs @@ -8,8 +8,8 @@ use ts_rs::TS; use crate::admin::AdminError as Error; use crate::app_state::AppState; use crate::listing::{ - build_filter_where_clause, limit_or_default, parse_query, Cursor, Order, QueryParseResult, - WhereClause, + build_filter_where_clause, limit_or_default, parse_and_sanitize_query, Cursor, Order, + QueryParseResult, WhereClause, }; use crate::records::sql_to_json::rows_to_json_arrays; use crate::schema::Column; @@ -41,7 +41,7 @@ pub async fn list_rows_handler( order, offset, .. - } = parse_query(raw_url_query.as_deref()) + } = parse_and_sanitize_query(raw_url_query.as_deref()) .map_err(|err| Error::Precondition(format!("Invalid query '{err}': {raw_url_query:?}")))?; let (table_metadata, table_or_view_metadata): ( diff --git a/trailbase-core/src/admin/user/list_users.rs b/trailbase-core/src/admin/user/list_users.rs index 538fc435..b384d4bd 100644 --- a/trailbase-core/src/admin/user/list_users.rs +++ b/trailbase-core/src/admin/user/list_users.rs @@ -14,8 +14,8 @@ use crate::app_state::AppState; use crate::auth::user::DbUser; use crate::constants::{USER_TABLE, USER_TABLE_ID_COLUMN}; use crate::listing::{ - build_filter_where_clause, limit_or_default, parse_query, Cursor, Order, QueryParseResult, - WhereClause, + build_filter_where_clause, limit_or_default, parse_and_sanitize_query, Cursor, Order, + QueryParseResult, WhereClause, }; use crate::util::id_to_b64; @@ -70,7 +70,7 @@ pub async fn list_users_handler( limit, order, .. - } = parse_query(raw_url_query.as_deref()) + } = parse_and_sanitize_query(raw_url_query.as_deref()) .map_err(|err| Error::Precondition(format!("Invalid query '{err}': {raw_url_query:?}")))?; let Some(table_metadata) = state.table_metadata().get(USER_TABLE) else { diff --git a/trailbase-core/src/listing.rs b/trailbase-core/src/listing.rs index 7fe9f2ab..f233cabe 100644 --- a/trailbase-core/src/listing.rs +++ b/trailbase-core/src/listing.rs @@ -139,11 +139,20 @@ fn parse_bool(s: &str) -> Option { }; } +#[inline] +fn sanitize_column_name(name: &str) -> bool { + // Assuming that all uses are quoted correctly, it should be enough to discard names containing + // (", ', `, [, ]), however we're conservative here. + return name + .chars() + .all(|c| c.is_alphanumeric() || c == '.' || c == '-' || c == '_'); +} + /// Parses out list-related query params including pagination (limit, cursort), order, and filters. /// /// An example query may look like: /// ?cursor=[0:16]&limit=50&order=price,-date&price[lte]=100&date[gte]=. -pub fn parse_query(query: Option<&str>) -> Result { +pub fn parse_and_sanitize_query(query: Option<&str>) -> Result { let mut result: QueryParseResult = Default::default(); let Some(query) = query else { return Ok(result); @@ -159,33 +168,51 @@ pub fn parse_query(query: Option<&str>) -> Result { "cursor" => result.cursor = Cursor::parse(value.as_ref()), "offset" => result.offset = value.parse::().ok(), "count" => result.count = parse_bool(&value), - "expand" => result.expand = Some(value.split(",").map(|s| s.to_owned()).collect()), - "order" => { - let order: Vec<(String, Order)> = value - .split(",") - .map(|v| { - return match v { - x if x.starts_with("-") => (v[1..].to_string(), Order::Descending), - x if x.starts_with("+") => (v[1..].to_string(), Order::Ascending), - x => (x.to_string(), Order::Ascending), - }; - }) - .collect(); + "expand" => { + result.expand = Some( + value + .split(",") + .map(|s| { + let column_name = s.to_owned(); - result.order = Some(order); + if !sanitize_column_name(&column_name) { + return Err(column_name); + } + + return Ok(column_name); + }) + .collect::, _>>()?, + ) + } + "order" => { + result.order = Some( + value + .split(",") + .map(|v| { + let col_order = match v.trim() { + x if x.starts_with("-") => (v[1..].to_string(), Order::Descending), + x if x.starts_with("+") => (v[1..].to_string(), Order::Ascending), + x => (x.to_string(), Order::Ascending), + }; + + if !sanitize_column_name(&col_order.0) { + return Err(col_order.0); + } + + return Ok(col_order); + }) + .collect::, _>>()?, + ) } key => { - // Key didn't match any of the predefined list operations (limit, cursor, order), we thus - // assume it's a column filter. We try to split any qualifier/operation, e.g. + // Key didn't match any of the predefined list operations (limit, cursor, order, ...), we + // thus assume it's a column filter. We try to split any qualifier/operation, e.g. // column[op]=value. let Some((k, maybe_op)) = split_key_into_col_and_op(key) else { return Err(key.to_string()); }; - if !k - .chars() - .all(|c| c.is_alphanumeric() || c == '.' || c == '-' || c == '_') - { + if !sanitize_column_name(k) { return Err(key.to_string()); } @@ -289,6 +316,7 @@ lazy_static! { mod tests { use super::*; use crate::util::id_to_b64; + use crate::util::urlencode; #[test] fn test_op_splitting_regex() { @@ -309,8 +337,8 @@ mod tests { #[test] fn test_query_parsing() { - assert!(parse_query(None).is_ok()); - assert!(parse_query(Some("")).is_ok()); + assert!(parse_and_sanitize_query(None).is_ok()); + assert!(parse_and_sanitize_query(Some("")).is_ok()); { let cursor: [u8; 16] = [5; 16]; @@ -321,7 +349,7 @@ mod tests { "limit=10&cursor={cursor}&order=%2bcol0,-col1,col2", cursor = id_to_b64(&cursor) ); - let result = parse_query(Some(&query)).unwrap(); + let result = parse_and_sanitize_query(Some(&query)).unwrap(); assert_eq!(result.limit, Some(10)); assert_eq!(result.cursor, Some(Cursor::Blob(cursor.to_vec()))); @@ -337,7 +365,7 @@ mod tests { { let query = Some("baz=23&bar[like]=foo"); - let result = parse_query(query).unwrap(); + let result = parse_and_sanitize_query(query).unwrap(); assert_eq!( result.params.as_ref().unwrap().get("baz").unwrap(), @@ -358,16 +386,22 @@ mod tests { { // foo,bar is an invalid key. let query = Some("baz=23&foo,bar&foo_bar"); - assert_eq!(parse_query(query).err(), Some("foo,bar".to_string())); + assert_eq!( + parse_and_sanitize_query(query).err(), + Some("foo,bar".to_string()) + ); let query = Some("baz=23&foo_bar"); - assert_eq!(parse_query(query).err(), Some("foo_bar".to_string())); + assert_eq!( + parse_and_sanitize_query(query).err(), + Some("foo_bar".to_string()) + ); } { // Check whitespaces let query = Some("foo=a+b&bar=a%20b"); - let result = parse_query(query).unwrap(); + let result = parse_and_sanitize_query(query).unwrap(); assert_eq!( result.params.as_ref().unwrap().get("foo").unwrap(), @@ -387,7 +421,7 @@ mod tests { { let query = Some("col_0[gte]=10&col_0[lte]=100"); - let result = parse_query(query).unwrap(); + let result = parse_and_sanitize_query(query).unwrap(); assert_eq!( result.params.as_ref().unwrap().get("col_0"), @@ -411,7 +445,7 @@ mod tests { // Test both encodings: "+" and %20 for " ". let value = "with+white%20spaces"; let query = Some(format!("text={value}")); - let result = parse_query(query.as_deref()).unwrap(); + let result = parse_and_sanitize_query(query.as_deref()).unwrap(); assert_eq!( result.params.as_ref().unwrap().get("text"), @@ -424,5 +458,16 @@ mod tests { result.params ); } + + { + // Sanitizing + assert!( + parse_and_sanitize_query(Some(&format!("order={}", urlencode("col'; inject")))).is_err() + ); + assert!( + parse_and_sanitize_query(Some(&format!("expand={}", urlencode("col'; inject")))).is_err() + ); + assert!(parse_and_sanitize_query(Some(&urlencode("col'; inject"))).is_err()); + } } } diff --git a/trailbase-core/src/records/list_records.rs b/trailbase-core/src/records/list_records.rs index 8558c689..bf0ba41e 100644 --- a/trailbase-core/src/records/list_records.rs +++ b/trailbase-core/src/records/list_records.rs @@ -11,7 +11,8 @@ use trailbase_sqlite::Value; use crate::app_state::AppState; use crate::auth::user::User; use crate::listing::{ - build_filter_where_clause, limit_or_default, parse_query, Order, QueryParseResult, WhereClause, + build_filter_where_clause, limit_or_default, parse_and_sanitize_query, Order, QueryParseResult, + WhereClause, }; use crate::records::query_builder::Expansions; use crate::records::sql_to_json::{row_to_json, row_to_json_expand, rows_to_json_expand}; @@ -62,14 +63,14 @@ pub async fn list_records_handler( }; let QueryParseResult { - params: filter_params, - cursor, limit, - order, + cursor, count, expand: query_expand, + order, + params: filter_params, .. - } = parse_query(raw_url_query.as_deref()).map_err(|_err| { + } = parse_and_sanitize_query(raw_url_query.as_deref()).map_err(|_err| { return RecordError::BadRequest("Invalid query"); })?;