diff --git a/Cargo.lock b/Cargo.lock index 47d176a..941213a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -596,7 +596,7 @@ dependencies = [ "addr2line", "cfg-if", "libc", - "miniz_oxide 0.8.0", + "miniz_oxide 0.8.1", "object", "rustc-demangle", "windows-targets 0.52.6", @@ -2019,7 +2019,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c936bfdafb507ebbf50b8074c54fa31c5be9a1e7e5f467dd659697041407d07c" dependencies = [ "crc32fast", - "miniz_oxide 0.8.0", + "miniz_oxide 0.8.1", ] [[package]] @@ -3256,9 +3256,9 @@ dependencies = [ [[package]] name = "miniz_oxide" -version = "0.8.0" +version = "0.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2d80299ef12ff69b16a84bb182e3b9df68b5a91574d3d4fa6e41b65deec4df1" +checksum = "a2ef2593ffb6958c941575cee70c8e257438749971869c4ae5acf6f91a168a61" dependencies = [ "adler2", ] diff --git a/trailbase-core/src/listing.rs b/trailbase-core/src/listing.rs index efe78f3..15355e2 100644 --- a/trailbase-core/src/listing.rs +++ b/trailbase-core/src/listing.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; use std::collections::HashMap; use thiserror::Error; -use crate::records::json_to_sql::simple_json_value_to_param; +use crate::records::json_to_sql::json_string_to_value; use crate::table_metadata::TableOrViewMetadata; use crate::util::b64_to_id; @@ -141,6 +141,16 @@ pub fn parse_query(query: Option) -> Option { continue; }; + if !k + .chars() + .all(|c| c.is_alphanumeric() || c == '.' || c == '-' || c == '_') + { + #[cfg(debug_assertions)] + debug!("skipping non-trivial query param: {key}={value}"); + + continue; + } + if value.is_empty() { continue; } @@ -183,6 +193,8 @@ pub fn build_filter_where_clause( ))); } + // IMPORTANT: We only include parameters with known columns to avoid building an invalid + // query early and forbid injections. let Some((col, _col_meta)) = table_metadata.column_by_name(&column_name) else { return Err(WhereClauseError::UnrecognizedParam(format!( "Unrecognized parameter: {column_name}" @@ -195,10 +207,7 @@ pub fn build_filter_where_clause( continue; }; - match simple_json_value_to_param( - col.data_type, - serde_json::Value::String(query_param.value.clone()), - ) { + match json_string_to_value(col.data_type, query_param.value) { Ok(value) => { where_clauses.push(format!("{column_name} {op} :{column_name}")); params.push((format!(":{column_name}").into(), value)); diff --git a/trailbase-core/src/records/json_to_sql.rs b/trailbase-core/src/records/json_to_sql.rs index e210eb0..f54888c 100644 --- a/trailbase-core/src/records/json_to_sql.rs +++ b/trailbase-core/src/records/json_to_sql.rs @@ -691,7 +691,10 @@ fn try_json_array_to_blob(arr: &Vec) -> Result Result { +pub(crate) fn json_string_to_value( + data_type: ColumnDataType, + value: String, +) -> Result { return Ok(match data_type { ColumnDataType::Null => Value::Null, // Strict/storage types diff --git a/trailbase-core/src/records/list_records.rs b/trailbase-core/src/records/list_records.rs index b9d7bbc..72b075f 100644 --- a/trailbase-core/src/records/list_records.rs +++ b/trailbase-core/src/records/list_records.rs @@ -2,7 +2,9 @@ use axum::{ extract::{Path, RawQuery, State}, Json, }; +use itertools::Itertools; use std::borrow::Cow; +use trailbase_sqlite::Value; use crate::app_state::AppState; use crate::auth::user::User; @@ -48,10 +50,7 @@ pub async fn list_records_handler( .map_err(|_err| RecordError::BadRequest("Invalid filter params"))?; if let Some(cursor) = cursor { - params.push(( - Cow::Borrowed(":cursor"), - trailbase_sqlite::Value::Blob(cursor.to_vec()), - )); + params.push((Cow::Borrowed(":cursor"), Value::Blob(cursor.to_vec()))); clause = format!("{clause} AND _ROW_.id < :cursor"); } @@ -59,13 +58,11 @@ pub async fn list_records_handler( params.extend_from_slice(&[ ( Cow::Borrowed(":limit"), - trailbase_sqlite::Value::Integer(limit_or_default(limit) as i64), + Value::Integer(limit_or_default(limit) as i64), ), ( Cow::Borrowed(":__user_id"), - user.map_or(trailbase_sqlite::Value::Null, |u| { - trailbase_sqlite::Value::Blob(u.uuid.into()) - }), + user.map_or(Value::Null, |u| Value::Blob(u.uuid.into())), ), ]); @@ -75,7 +72,7 @@ pub async fn list_records_handler( // TODO: Should this be a separate access rule? Maybe one wants users to access a specific // record but not list all the records. if let Some(read_access) = api.access_rule(Permission::Read) { - clause = format!("({clause}) AND {read_access}"); + clause = format!("({read_access}) AND ({clause})"); } let default_ordering = || { @@ -94,7 +91,6 @@ pub async fn list_records_handler( } ) }) - .collect::>() .join(", "); let query = format!( diff --git a/trailbase-core/src/records/record_api.rs b/trailbase-core/src/records/record_api.rs index f1e363f..d20a66b 100644 --- a/trailbase-core/src/records/record_api.rs +++ b/trailbase-core/src/records/record_api.rs @@ -12,6 +12,29 @@ use crate::schema::{Column, ColumnDataType}; use crate::table_metadata::{TableMetadata, TableOrViewMetadata, ViewMetadata}; use crate::util::{assert_uuidv7, b64_to_id}; +enum RecordApiMetadata { + Table(TableMetadata), + View(ViewMetadata), +} + +impl RecordApiMetadata { + #[inline] + fn table_name(&self) -> &str { + match &self { + RecordApiMetadata::Table(ref table) => &table.schema.name, + RecordApiMetadata::View(ref view) => &view.schema.name, + } + } + + #[inline] + fn metadata(&self) -> &(dyn TableOrViewMetadata + Send + Sync) { + match &self { + RecordApiMetadata::Table(ref table) => table, + RecordApiMetadata::View(ref view) => view, + } + } +} + /// FILTER CONTROL. /// /// Open question: right now we use the read_access rule also for listing. It could be nice to @@ -27,11 +50,6 @@ pub struct RecordApi { state: Arc, } -enum RecordApiMetadata { - Table(TableMetadata), - View(ViewMetadata), -} - struct RecordApiState { conn: trailbase_sqlite::Connection, metadata: RecordApiMetadata, @@ -44,10 +62,19 @@ struct RecordApiState { insert_autofill_missing_user_id_columns: bool, create_access_rule: Option, + create_access_query: Option, + read_access_rule: Option, + read_access_query: Option, + update_access_rule: Option, + update_access_query: Option, + delete_access_rule: Option, + delete_access_query: Option, + schema_access_rule: Option, + schema_access_query: Option, } impl RecordApi { @@ -129,6 +156,32 @@ impl RecordApi { return Err(format!("RecordApi misses name: {config:?}")); }; + let read_access_query = config.read_access_rule.as_ref().map(|rule| { + build_read_delete_schema_query(metadata.table_name(), &record_pk_column.name, rule) + }); + + let delete_access_query = config.delete_access_rule.as_ref().map(|rule| { + build_read_delete_schema_query(metadata.table_name(), &record_pk_column.name, rule) + }); + + let schema_access_query = config.schema_access_rule.as_ref().map(|rule| { + build_read_delete_schema_query(metadata.table_name(), &record_pk_column.name, rule) + }); + + let create_access_query = config.create_access_rule.as_ref().and_then(|rule| { + if let RecordApiMetadata::Table(ref m) = metadata { + return Some(build_create_access_query(m, rule)); + } + return None; + }); + + let update_access_query = config.update_access_rule.as_ref().and_then(|rule| { + if let RecordApiMetadata::Table(ref m) = metadata { + return Some(build_update_access_query(m, &record_pk_column.name, rule)); + } + return None; + }); + return Ok(RecordApi { state: Arc::new(RecordApiState { conn, @@ -151,11 +204,22 @@ impl RecordApi { convert_acl(&config.acl_authenticated), ], // Access rules. + // + // Create: create_access_rule: config.create_access_rule, + create_access_query, + read_access_rule: config.read_access_rule, + read_access_query, + update_access_rule: config.update_access_rule, + update_access_query, + delete_access_rule: config.delete_access_rule, + delete_access_query, + schema_access_rule: config.schema_access_rule, + schema_access_query, }), }); } @@ -167,18 +231,12 @@ impl RecordApi { #[inline] pub fn table_name(&self) -> &str { - match &self.state.metadata { - RecordApiMetadata::Table(ref table) => &table.schema.name, - RecordApiMetadata::View(ref view) => &view.schema.name, - } + return self.state.metadata.table_name(); } #[inline] pub fn metadata(&self) -> &(dyn TableOrViewMetadata + Send + Sync) { - match &self.state.metadata { - RecordApiMetadata::Table(ref table) => table, - RecordApiMetadata::View(ref view) => view, - } + return self.state.metadata.metadata(); } pub fn table_metadata(&self) -> Option<&TableMetadata> { @@ -219,6 +277,17 @@ impl RecordApi { }; } + #[inline] + fn access_query(&self, p: Permission) -> &Option { + return match p { + Permission::Create => &self.state.create_access_query, + Permission::Read => &self.state.read_access_query, + Permission::Update => &self.state.update_access_query, + Permission::Delete => &self.state.delete_access_query, + Permission::Schema => &self.state.schema_access_query, + }; + } + #[inline] pub fn insert_autofill_missing_user_id_columns(&self) -> bool { return self.state.insert_autofill_missing_user_id_columns; @@ -240,40 +309,30 @@ impl RecordApi { // First check table level access and if present check row-level access based on access rule. self.check_table_level_access(p, user)?; - 'acl: { - let Some(ref access_rule) = self.access_rule(p) else { - return Ok(()); - }; - - let (access_query, params) = self.build_access_query_and_params( - p, - access_rule, - self.table_name(), - record_id, - request_params, - user, - )?; - - let allowed = match self - .state - .conn - .call(move |conn| Self::query_access(conn, &access_query, params)) - .await - { - Ok(allowed) => allowed, - Err(err) => { - if cfg!(test) { - panic!("RLA query failed: {err}"); - } - warn!("RLA query failed: {err}"); - break 'acl; + let Some(access_query) = self.access_query(p) else { + return Ok(()); + }; + let access_query = access_query.clone(); + let params = self.build_named_params(p, record_id, request_params, user)?; + + match self + .state + .conn + .call(move |conn| Self::query_access(conn, &access_query, params)) + .await + { + Ok(allowed) => { + if allowed { + return Ok(()); } - }; + } + Err(err) => { + warn!("RLA query failed: {err}"); - if allowed { - return Ok(()); + #[cfg(test)] + panic!("RLA query failed: {err}"); } - } + }; return Err(RecordError::Forbidden); } @@ -291,35 +350,24 @@ impl RecordApi { // First check table level access and if present check row-level access based on access rule. self.check_table_level_access(p, user)?; - 'acl: { - let Some(ref access_rule) = self.access_rule(p) else { - return Ok(()); - }; - - let (access_query, params) = self.build_access_query_and_params( - p, - access_rule, - self.table_name(), - record_id, - request_params, - user, - )?; - - let allowed = match Self::query_access(conn, &access_query, params) { - Ok(allowed) => allowed, - Err(err) => { - if cfg!(test) { - panic!("RLA query failed: {err}"); - } - warn!("RLA query failed: {err}"); - break 'acl; + let Some(access_query) = self.access_query(p) else { + return Ok(()); + }; + let params = self.build_named_params(p, record_id, request_params, user)?; + + match Self::query_access(conn, access_query, params) { + Ok(allowed) => { + if allowed { + return Ok(()); } - }; + } + Err(err) => { + warn!("RLA query failed: {err}"); - if allowed { - return Ok(()); + #[cfg(test)] + panic!("RLA query failed: {err}"); } - } + }; return Err(RecordError::Forbidden); } @@ -364,18 +412,31 @@ impl RecordApi { // TODO: We should probably break this up into separate functions for CRUD, to only do and inject // what's actually needed. Maybe even break up the entire check_access_and_rls_then. It's pretty // winding right now. - fn build_access_query_and_params( + fn build_named_params( &self, p: Permission, - access_rule: &str, - table_name: &str, record_id: Option<&Value>, request_params: Option<&mut LazyParams<'_>>, user: Option<&User>, - ) -> Result<(String, NamedParams), RecordError> { + ) -> Result { // We need to inject context like: record id, user, request, and row into the access // check. Below we're building the query and binding the context as params accordingly. - let mut params = NamedParams::with_capacity(16); + let mut params = match p { + Permission::Create | Permission::Update => { + let Some(table_metadata) = self.table_metadata() else { + return Err(RecordError::ApiRequiresTable); + }; + + build_request_params( + table_metadata, + request_params + .expect("params for update & create") + .params() + .map_err(|err| RecordError::Internal(err.into()))?, + ) + } + Permission::Read | Permission::Delete | Permission::Schema => NamedParams::with_capacity(2), + }; params.extend_from_slice(&[ ( @@ -388,94 +449,95 @@ impl RecordApi { ), ]); - // Assumes access_rule is an expression: https://www.sqlite.org/syntax/expr.html - // - // Create has no "row" - // Read and delete have no "request" - // And only update has "row" and "request". - let query = match p { - Permission::Create => { - let Some(table_metadata) = self.table_metadata() else { - return Err(RecordError::ApiRequiresTable); - }; + return Ok(params); + } +} - let (request_sub_select, mut request_params) = build_request_sub_select( - table_metadata, - request_params - .unwrap() - .params() - .map_err(|err| RecordError::Internal(err.into()))?, - ); - params.append(&mut request_params); - - indoc::formatdoc!( - r#" - SELECT - ({access_rule}) - FROM - (SELECT :__user_id AS id) AS _USER_, - ({request_sub_select}) AS _REQ_ - "#, - ) - } - Permission::Update => { - let pk_column_name = &self.state.record_pk_column.name; - let Some(table_metadata) = self.table_metadata() else { - return Err(RecordError::ApiRequiresTable); - }; +/// Build access query for record reads, deletes and query access. +/// +/// Assumes access_rule is an expression: https://www.sqlite.org/syntax/expr.html +fn build_read_delete_schema_query( + table_name: &str, + pk_column_name: &str, + access_rule: &str, +) -> String { + return indoc::formatdoc!( + r#" + SELECT + ({access_rule}) + FROM + (SELECT :__user_id AS id) AS _USER_, + (SELECT * FROM "{table_name}" WHERE "{pk_column_name}" = :__record_id) AS _ROW_ + "# + ); +} - let (request_sub_select, mut request_params) = build_request_sub_select( - table_metadata, - request_params - .unwrap() - .params() - .map_err(|err| RecordError::Internal(err.into()))?, - ); - params.append(&mut request_params); - - indoc::formatdoc!( - r#" - SELECT - ({access_rule}) - FROM - (SELECT :__user_id AS id) AS _USER_, - ({request_sub_select}) AS _REQ_, - (SELECT * FROM "{table_name}" WHERE "{pk_column_name}" = :__record_id) AS _ROW_ - "#, - ) - } - Permission::Read | Permission::Delete | Permission::Schema => { - let pk_column_name = &self.state.record_pk_column.name; - - indoc::formatdoc!( - r#" - SELECT - ({access_rule}) - FROM - (SELECT :__user_id AS id) AS _USER_, - (SELECT * FROM "{table_name}" WHERE "{pk_column_name}" = :__record_id) AS _ROW_ - "# - ) - } - }; +/// Build access query for record creation. +/// +/// Assumes access_rule is an expression: https://www.sqlite.org/syntax/expr.html +fn build_create_access_query(table_metadata: &TableMetadata, create_access_rule: &str) -> String { + let column_sub_select = format!( + "SELECT {placeholders}", + placeholders = table_metadata + .schema + .columns + .iter() + .map(|col| format!(r#":{name} AS '{name}'"#, name = col.name)) + .join(", ") + ); - return Ok((query, params)); - } + return indoc::formatdoc!( + r#" + SELECT + ({create_access_rule}) + FROM + (SELECT :__user_id AS id) AS _USER_, + ({column_sub_select}) AS _REQ_ + "#, + ); } -/// Builds the sub-query for _REQ_. -fn build_request_sub_select( +/// Build access query for record updates. +/// +/// Assumes access_rule is an expression: https://www.sqlite.org/syntax/expr.html +fn build_update_access_query( table_metadata: &TableMetadata, - request_params: &Params, -) -> (String, NamedParams) { + pk_column_name: &str, + update_access_rule: &str, +) -> String { + let table_name = table_metadata.name(); + + let column_sub_select = format!( + "SELECT {placeholders}", + placeholders = table_metadata + .schema + .columns + .iter() + .map(|col| format!(r#":{name} AS '{name}'"#, name = col.name)) + .join(", ") + ); + + return indoc::formatdoc!( + r#" + SELECT + ({update_access_rule}) + FROM + (SELECT :__user_id AS id) AS _USER_, + ({column_sub_select}) AS _REQ_, + (SELECT * FROM "{table_name}" WHERE "{pk_column_name}" = :__record_id) AS _ROW_ + "#, + ); +} + +/// Build SQL named parameters from request fields. +fn build_request_params(table_metadata: &TableMetadata, request_params: &Params) -> NamedParams { // NOTE: This has gotten pretty wild. We cannot have access queries access missing _REQ_.props. // So we need to inject an explicit NULL value for all missing fields on the request. // Can we make this cheaper, either by pre-processing the access query or improving construction? // For example, could we build a transaction-scoped temp view with positional placeholders to // save some string ops? - let schema = &table_metadata.schema; - - let mut named_params: NamedParams = schema + let mut named_params: NamedParams = table_metadata + .schema .columns .iter() .map(|c| (Cow::Owned(format!(r#":{}"#, c.name)), Value::Null)) @@ -491,17 +553,7 @@ fn build_request_sub_select( named_params[col_index].1 = request_params.named_params()[param_index].1.clone(); } - return ( - format!( - "SELECT {placeholders}", - placeholders = schema - .columns - .iter() - .map(|col| format!(r#":{name} AS '{name}'"#, name = col.name)) - .join(", ") - ), - named_params, - ); + return named_params; } fn convert_acl(acl: &Vec) -> u8 {