Merge pull request #258 from cipherstash/error-handling-tests

Refactor error handling and expand tests

Author: tobyhede

packages/cipherstash-proxy-integration/src/select/unmappable.rs

@@ -10,7 +10,7 @@ mod tests {
     /// Test ensures that unmappable SQL statements return an error
     ///
     #[tokio::test]
-    async fn unmappable_error() {
+    async fn unmappable_table_not_found() {
         let client = connect_with_tls(PROXY).await;
 
         let sql = "SELECT blah FROM vtha";
@@ -21,4 +21,43 @@ mod tests {
             "Expected unmappble SQL statement to return an error",
         );
     }
+
+    #[tokio::test]
+    async fn unmappable_column_not_found() {
+        let client = connect_with_tls(PROXY).await;
+
+        let sql = "SELECT blah FROM encrypted";
+        let result = client.query(sql, &[]).await;
+
+        assert!(
+            result.is_err(),
+            "Expected unmappble SQL statement to return an error",
+        );
+    }
+
+    #[tokio::test]
+    async fn unmappable_native_cannot_be_unified_with_encrypted() {
+        let client = connect_with_tls(PROXY).await;
+
+        let sql = "SELECT * FROM encrypted WHERE plaintext = encrypted_text";
+        let result = client.query(sql, &[]).await;
+
+        assert!(
+            result.is_err(),
+            "Expected unmappble SQL statement to return an error",
+        );
+    }
+
+    #[tokio::test]
+    async fn unmappable_syntax_error() {
+        let client = connect_with_tls(PROXY).await;
+
+        let sql = "SELECT *, FROM encrypted";
+        let result = client.query(sql, &[]).await;
+
+        assert!(
+            result.is_err(),
+            "Expected unmappble SQL statement to return an error",
+        );
+    }
 }

packages/cipherstash-proxy/src/postgresql/frontend.rs

@@ -116,7 +116,12 @@ where
                     // No mapping needed, don't change the bytes
                     Ok(None) => (),
                     Err(err) => {
-                        bytes = self.to_database_exception(err)?;
+                        warn!(
+                                client_id = self.context.client_id,
+                                msg = "Query Error",
+                                error = ?err.to_string(),
+                        );
+                        self.error_handler(err).await?;
                     }
                 }
             }
@@ -131,39 +136,14 @@ where
                     Ok(Some(mapped)) => bytes = mapped,
                     // No mapping needed, don't change the bytes
                     Ok(None) => (),
-                    Err(err) => match err {
-                        Error::Mapping(MappingError::InvalidSqlStatement(_)) => {
-                            warn!(target: PROTOCOL,
-                                client_id = self.context.client_id,
-                                msg = "MappingError::SqlParse",
-                                error = ?err,
-                            );
-
-                            let error_response =
-                                ErrorResponse::invalid_sql_statement(err.to_string());
-
-                            self.send_error_response(error_response)?;
-                        }
-                        Error::Encrypt(EncryptError::UnknownColumn {
-                            ref table,
-                            ref column,
-                        }) => {
-                            warn!(target: PROTOCOL,
-                                client_id = self.context.client_id,
-                                msg = "EncryptError::UnknownColumn",
-                            );
-                            let error_response =
-                                ErrorResponse::unknown_column(err.to_string(), table, column);
-                            self.send_error_response(error_response)?;
-                        }
-                        _ => {
-                            warn!(target: PROTOCOL,
+                    Err(err) => {
+                        warn!(
                                 client_id = self.context.client_id,
-                                msg = "build_frontend_exception",
-                            );
-                            bytes = self.to_database_exception(err)?;
-                        }
-                    },
+                                msg = "Parse Error",
+                                error = ?err.to_string(),
+                        );
+                        self.error_handler(err).await?;
+                    }
                 }
             }
             Code::Bind => {
@@ -216,6 +196,19 @@ where
         Ok(())
     }
 
+    pub async fn error_handler(&mut self, err: Error) -> Result<(), Error> {
+        let error_response = match err {
+            Error::Mapping(err) => ErrorResponse::invalid_sql_statement(err.to_string()),
+            Error::Encrypt(EncryptError::UnknownColumn {
+                ref table,
+                ref column,
+            }) => ErrorResponse::unknown_column(err.to_string(), table, column),
+            _ => ErrorResponse::system_error(err.to_string()),
+        };
+        self.send_error_response(error_response)?;
+        Ok(())
+    }
+
     pub async fn write_to_server(&mut self, bytes: BytesMut) -> Result<(), Error> {
         let sent: u64 = bytes.len() as u64;
         counter!(SERVER_BYTES_SENT_TOTAL).increment(sent);

packages/cipherstash-proxy/src/postgresql/handler.rs

@@ -138,7 +138,7 @@ pub async fn handler(
             let message = ProtocolError::ClientAuthenticationFailed.to_string();
             error!(msg = message);
 
-            let message = ErrorResponse::invalid_password(&message);
+            let message = ErrorResponse::invalid_password(message);
             let bytes = BytesMut::try_from(message)?;
             client_stream.write_all(&bytes).await?;
         }

packages/cipherstash-proxy/src/postgresql/messages/error_response.rs

@@ -17,6 +17,7 @@ pub const CODE_RAISE_EXCEPTION: &str = "P0001";
 pub const CODE_SYNTAX_ERROR: &str = "42601";
 pub const CODE_INVALID_TEXT_REPRESENTATION: &str = "22P02";
 pub const CODE_IDLE_SESSION_TIMEOUT: &str = "57P05";
+pub const CODE_SYSTEM_ERROR: &str = "58000";
 
 ///
 /// ErrorResponse (B)
@@ -82,7 +83,7 @@ impl ErrorResponse {
         }
     }
 
-    pub fn invalid_password(message: &str) -> Self {
+    pub fn invalid_password(message: String) -> Self {
         Self {
             fields: vec![
                 Field {
@@ -99,7 +100,7 @@ impl ErrorResponse {
                 },
                 Field {
                     code: ErrorResponseCode::Message,
-                    value: message.to_string(),
+                    value: message,
                 },
             ],
         }
@@ -238,6 +239,29 @@ impl ErrorResponse {
         }
     }
 
+    pub fn system_error(message: String) -> Self {
+        Self {
+            fields: vec![
+                Field {
+                    code: ErrorResponseCode::Severity,
+                    value: "FATAL".to_string(),
+                },
+                Field {
+                    code: ErrorResponseCode::SeverityLegacy,
+                    value: "FATAL".to_string(),
+                },
+                Field {
+                    code: ErrorResponseCode::Code,
+                    value: CODE_SYSTEM_ERROR.to_string(),
+                },
+                Field {
+                    code: ErrorResponseCode::Message,
+                    value: message,
+                },
+            ],
+        }
+    }
+
     pub fn tls_required() -> Self {
         Self {
             fields: vec![