From 235f796b300429d81794e61b34cee2d969de8c88 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Fri, 23 Oct 2026 10:47:28 +1100 Subject: [PATCH] Propagate settings parse errors from SettingsCommand SettingsCommand::from_statement discarded SettingChange::parse errors with .ok(), so an invalid CIPHERSTASH.* setting became SettingsCommand::Other. Every caller validates the statement first, so this was unreachable, but a future caller that skipped validation would silently treat an invalid setting as an ordinary statement. from_statement now returns Result, and SettingsProtocol::prepare, SettingsProtocol::simple_query, project_settings_effects and settings_simple_query_requires_fail_closed propagate the error. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/postgresql/context/mod.rs | 35 ++-- .../src/postgresql/context/settings.rs | 34 +++- .../postgresql/context/settings_protocol.rs | 175 ++++++++++-------- .../src/postgresql/middleware/frontend.rs | 2 +- 4 files changed, 149 insertions(+), 97 deletions(-) diff --git a/packages/cipherstash-proxy/src/postgresql/context/mod.rs b/packages/cipherstash-proxy/src/postgresql/context/mod.rs index ceef075e..a418ad83 100644 --- a/packages/cipherstash-proxy/src/postgresql/context/mod.rs +++ b/packages/cipherstash-proxy/src/postgresql/context/mod.rs @@ -1449,8 +1449,7 @@ where let settings = settings_protocol::EffectiveSettings::new(&state.settings); state .settings_protocol - .prepare(operation, name, statement, settings); - Ok(()) + .prepare(operation, name, statement, settings) } fn bind_settings_statement( @@ -1525,26 +1524,26 @@ where settings_protocol, .. } = &mut *state; - Ok(settings_protocol.simple_query(operation, statements, settings)) + settings_protocol.simple_query(operation, statements, settings) } pub fn settings_simple_query_requires_fail_closed( &self, statements: &[sqltk::parser::ast::Statement], - ) -> bool { + ) -> Result { let Ok(state) = self.protocol_state.read() else { - return true; + return Ok(true); }; let mut changed = false; for (statement, _, changes) in - settings_protocol::project_settings_effects(&state.settings, statements) + settings_protocol::project_settings_effects(&state.settings, statements)? { if changed && eql_mapper::requires_type_check(statement) { - return true; + return Ok(true); } changed |= changes; } - false + Ok(false) } pub fn discarding_extended_messages(&self) -> Result { @@ -2559,9 +2558,11 @@ mod tests { "RESET ALL; SELECT 1", ] { assert!( - context.settings_simple_query_requires_fail_closed( - &crate::postgresql::parser::SqlParser::parse_statements(sql).unwrap() - ), + context + .settings_simple_query_requires_fail_closed( + &crate::postgresql::parser::SqlParser::parse_statements(sql).unwrap() + ) + .unwrap(), "{sql}" ); } @@ -2629,9 +2630,11 @@ mod tests { }) .join(); - assert!(context.settings_simple_query_requires_fail_closed( - &crate::postgresql::parser::SqlParser::parse_statements("SELECT 1").unwrap() - )); + assert!(context + .settings_simple_query_requires_fail_closed( + &crate::postgresql::parser::SqlParser::parse_statements("SELECT 1").unwrap() + ) + .unwrap()); } #[test] @@ -2662,7 +2665,9 @@ mod tests { ] { let statements = crate::postgresql::parser::SqlParser::parse_statements(sql).unwrap(); assert!( - !context.settings_simple_query_requires_fail_closed(&statements), + !context + .settings_simple_query_requires_fail_closed(&statements) + .unwrap(), "{sql}" ); } diff --git a/packages/cipherstash-proxy/src/postgresql/context/settings.rs b/packages/cipherstash-proxy/src/postgresql/context/settings.rs index 6ca4b0b9..8f0a3cc8 100644 --- a/packages/cipherstash-proxy/src/postgresql/context/settings.rs +++ b/packages/cipherstash-proxy/src/postgresql/context/settings.rs @@ -167,8 +167,8 @@ pub enum SettingsCommand { } impl SettingsCommand { - pub fn from_statement(statement: &Statement) -> Self { - match statement { + pub fn from_statement(statement: &Statement) -> Result { + Ok(match statement { Statement::StartTransaction { .. } => Self::Begin, Statement::Commit { .. } => Self::Commit, Statement::Rollback { @@ -180,12 +180,10 @@ impl SettingsCommand { } => Self::RollbackTo(postgres_identifier(name)), Statement::Savepoint { name } => Self::Savepoint(postgres_identifier(name)), Statement::ReleaseSavepoint { name } => Self::Release(postgres_identifier(name)), - _ => SettingChange::parse(statement) - .ok() - .flatten() + _ => SettingChange::parse(statement)? .map(Self::Change) .unwrap_or(Self::Other), - } + }) } pub fn may_change_effective(&self, settings: &ConnectionSettings) -> bool { @@ -310,7 +308,10 @@ impl ConnectionSettings { /// Applies only a command confirmed successful by PostgreSQL. #[cfg(test)] pub fn execution_succeeded(&mut self, statement: &Statement, command_tag: &str) { - self.command_succeeded(&SettingsCommand::from_statement(statement), command_tag); + self.command_succeeded( + &SettingsCommand::from_statement(statement).unwrap(), + command_tag, + ); } pub fn command_succeeded(&mut self, command: &SettingsCommand, command_tag: &str) { @@ -875,6 +876,21 @@ mod tests { } } + #[test] + fn settings_command_reports_an_invalid_cipherstash_setting() { + for sql in [ + "SET CIPHERSTASH.KEYSET_ID = 'invalid'", + "SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = 'maybe'", + "SET CIPHERSTASH.UNKNOWN = 'value'", + ] { + let statements = crate::postgresql::parser::SqlParser::parse_statements(sql).unwrap(); + assert!( + SettingsCommand::from_statement(&statements[0]).is_err(), + "{sql}" + ); + } + } + fn parse_one(sql: &str) -> Result, Error> { let statements = crate::postgresql::parser::SqlParser::parse_statements(sql).unwrap(); SettingChange::parse(&statements[0]) @@ -1017,7 +1033,9 @@ mod tests { "SET", ); let commit = crate::postgresql::parser::SqlParser::parse_statement("COMMIT").unwrap(); - assert!(SettingsCommand::from_statement(&commit).may_change_effective(&settings)); + assert!(SettingsCommand::from_statement(&commit) + .unwrap() + .may_change_effective(&settings)); } #[test] diff --git a/packages/cipherstash-proxy/src/postgresql/context/settings_protocol.rs b/packages/cipherstash-proxy/src/postgresql/context/settings_protocol.rs index 669155e1..06f01d8b 100644 --- a/packages/cipherstash-proxy/src/postgresql/context/settings_protocol.rs +++ b/packages/cipherstash-proxy/src/postgresql/context/settings_protocol.rs @@ -3,6 +3,7 @@ use super::{ settings::{ConnectionSettings, SettingChange, SettingsCommand}, KeysetIdentifier, SessionId, }; +use crate::error::Error; use crate::postgresql::{Name, OperationId}; use sqltk::parser::ast::Statement; use std::{ @@ -40,20 +41,23 @@ impl EffectiveSettings { } } -/// Projects a simple-query batch over a copy of the confirmed settings, yielding +/// Projects a simple-query batch over a copy of the confirmed settings, returning /// each statement, its settings command, and whether it may change the /// effective settings after the statements before it succeed. pub(super) fn project_settings_effects<'a>( settings: &ConnectionSettings, statements: &'a [Statement], -) -> impl Iterator + 'a { +) -> Result, Error> { let mut projected = settings.clone(); - statements.iter().map(move |statement| { - let command = SettingsCommand::from_statement(statement); - let changes = command.may_change_effective(&projected); - projected.command_succeeded(&command, command.success_tag()); - (statement, command, changes) - }) + statements + .iter() + .map(|statement| { + let command = SettingsCommand::from_statement(statement)?; + let changes = command.may_change_effective(&projected); + projected.command_succeeded(&command, command.success_tag()); + Ok((statement, command, changes)) + }) + .collect() } #[derive(Clone, Debug)] @@ -159,12 +163,13 @@ impl SettingsProtocol { name: Name, statement: &Statement, settings: EffectiveSettings, - ) { + ) -> Result<(), Error> { + let command = SettingsCommand::from_statement(statement)?; let previous = self.prepared.insert( name.clone(), Prepared { operation, - command: SettingsCommand::from_statement(statement), + command, needs_mapping: eql_mapper::requires_type_check(statement), settings, literal_sensitive: false, @@ -176,6 +181,7 @@ impl SettingsProtocol { metadata: MetadataSnapshot::default(), close: false, }); + Ok(()) } pub fn capture_literals(&mut self, name: &Name, sensitive: bool) { if let Some(prepared) = self.prepared.get_mut(name) { @@ -258,9 +264,10 @@ impl SettingsProtocol { operation: OperationId, statements: &[Statement], settings: &ConnectionSettings, - ) -> bool { + ) -> Result { let mut barrier = false; - let commands: VecDeque<_> = project_settings_effects(settings, statements) + let commands: VecDeque<_> = project_settings_effects(settings, statements)? + .into_iter() .map(|(_, command, changes)| { barrier |= changes; command @@ -275,7 +282,7 @@ impl SettingsProtocol { }, ); self.boundary(operation); - barrier + Ok(barrier) } pub fn succeeded(&mut self, operation: OperationId) -> Option { let pending = self.pending.get_mut(&operation)?; @@ -543,22 +550,26 @@ mod protocol_regressions { use crate::postgresql::{parser::SqlParser, test_operation_id as operation_id}; fn prepare(protocol: &mut SettingsProtocol, name: &Name) { let operation = operation_id(); - protocol.prepare( - operation, - name.clone(), - &SqlParser::parse_statement("SELECT 1").unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + operation, + name.clone(), + &SqlParser::parse_statement("SELECT 1").unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol.prepared_or_bound(operation); } fn named_prepare(protocol: &mut SettingsProtocol, name: &Name, savepoint: &str) -> OperationId { let operation = operation_id(); - protocol.prepare( - operation, - name.clone(), - &SqlParser::parse_statement(&format!("SAVEPOINT {savepoint}")).unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + operation, + name.clone(), + &SqlParser::parse_statement(&format!("SAVEPOINT {savepoint}")).unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); operation } @@ -638,11 +649,13 @@ mod protocol_regressions { let original = named_prepare(&mut protocol, &name, "original"); protocol.prepared_or_bound(original); let discard = operation_id(); - protocol.simple_query( - discard, - &SqlParser::parse_statements("DISCARD ALL").unwrap(), - &ConnectionSettings::default(), - ); + protocol + .simple_query( + discard, + &SqlParser::parse_statements("DISCARD ALL").unwrap(), + &ConnectionSettings::default(), + ) + .unwrap(); let later = named_prepare(&mut protocol, &name, "later"); protocol.succeeded(discard); assert!( @@ -696,12 +709,14 @@ mod protocol_regressions { let name = Name::from_static(b"statement"); prepare(&mut protocol, &name); let rejected = operation_id(); - protocol.prepare( - rejected, - name.clone(), - &SqlParser::parse_statement("SELECT 2").unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + rejected, + name.clone(), + &SqlParser::parse_statement("SELECT 2").unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol.failed(Some(rejected)); protocol.ready(Some(operation_id()), true); protocol.close( @@ -710,12 +725,14 @@ mod protocol_regressions { pg_proto::DescribeTarget::Statement, Default::default(), ); - protocol.prepare( - operation_id(), - name.clone(), - &SqlParser::parse_statement("SELECT 3").unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + operation_id(), + name.clone(), + &SqlParser::parse_statement("SELECT 3").unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol .bind( operation_id(), @@ -780,7 +797,9 @@ mod protocol_regressions { let mut protocol = SettingsProtocol::default(); let statements = SqlParser::parse_statements(sql).unwrap(); assert!( - !protocol.simple_query(operation_id(), &statements, &ConnectionSettings::default()), + !protocol + .simple_query(operation_id(), &statements, &ConnectionSettings::default()) + .unwrap(), "{sql}" ); } @@ -802,12 +821,14 @@ mod protocol_regressions { let mut protocol = SettingsProtocol::default(); let name = Name::from_static(b"insert"); let parse = operation_id(); - protocol.prepare( - parse, - name.clone(), - &SqlParser::parse_statement("INSERT INTO encrypted (secret) VALUES ($1)").unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + parse, + name.clone(), + &SqlParser::parse_statement("INSERT INTO encrypted (secret) VALUES ($1)").unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol.prepared_or_bound(parse); let mut tenant_b = ConnectionSettings::default(); tenant_b.execution_succeeded( @@ -844,13 +865,15 @@ mod protocol_regressions { let mut protocol = SettingsProtocol::default(); let name = Name::from_static(b"insert_literal"); let parse = operation_id(); - protocol.prepare( - parse, - name.clone(), - &SqlParser::parse_statement("INSERT INTO encrypted (secret) VALUES ('literal')") - .unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + parse, + name.clone(), + &SqlParser::parse_statement("INSERT INTO encrypted (secret) VALUES ('literal')") + .unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol.capture_literals(&name, true); protocol.prepared_or_bound(parse); let mut tenant_b = ConnectionSettings::default(); @@ -881,12 +904,14 @@ mod protocol_regressions { let mut protocol = SettingsProtocol::default(); let name = Name::from_static(b"keyset"); let parse = operation_id(); - protocol.prepare( - parse, - name.clone(), - &SqlParser::parse_statement("SET CIPHERSTASH.KEYSET_NAME = 'tenant'").unwrap(), - EffectiveSettings::new(&ConnectionSettings::default()), - ); + protocol + .prepare( + parse, + name.clone(), + &SqlParser::parse_statement("SET CIPHERSTASH.KEYSET_NAME = 'tenant'").unwrap(), + EffectiveSettings::new(&ConnectionSettings::default()), + ) + .unwrap(); protocol.prepared_or_bound(parse); let bind = operation_id(); protocol @@ -918,11 +943,13 @@ mod protocol_regressions { fn failed_simple_query_settings_change_stays_in_flight_until_ready() { let mut protocol = SettingsProtocol::default(); let query = operation_id(); - assert!(protocol.simple_query( - query, - &SqlParser::parse_statements("SET CIPHERSTASH.KEYSET_NAME = 'tenant'").unwrap(), - &ConnectionSettings::default(), - )); + assert!(protocol + .simple_query( + query, + &SqlParser::parse_statements("SET CIPHERSTASH.KEYSET_NAME = 'tenant'").unwrap(), + &ConnectionSettings::default(), + ) + .unwrap()); protocol.failed(Some(query)); assert!(protocol.in_flight()); protocol.ready(Some(query), true); @@ -939,12 +966,14 @@ mod protocol_regressions { let mut protocol = SettingsProtocol::default(); let name = Name::from_static(b"commit"); let parse = operation_id(); - protocol.prepare( - parse, - name.clone(), - &parse_sql("COMMIT"), - EffectiveSettings::new(&connection), - ); + protocol + .prepare( + parse, + name.clone(), + &parse_sql("COMMIT"), + EffectiveSettings::new(&connection), + ) + .unwrap(); protocol.prepared_or_bound(parse); let bind = operation_id(); protocol diff --git a/packages/cipherstash-proxy/src/postgresql/middleware/frontend.rs b/packages/cipherstash-proxy/src/postgresql/middleware/frontend.rs index 02b23959..da2dcf7c 100644 --- a/packages/cipherstash-proxy/src/postgresql/middleware/frontend.rs +++ b/packages/cipherstash-proxy/src/postgresql/middleware/frontend.rs @@ -351,7 +351,7 @@ impl Frontend { } if self .context - .settings_simple_query_requires_fail_closed(&parsed_statements) + .settings_simple_query_requires_fail_closed(&parsed_statements)? { return Err(MappingError::DependentStatementAfterSettings.into()); }