From 31f697be7e549abab0905cf867ccb0bb2ad270f4 Mon Sep 17 00:00:00 2001
From: Sean Klein <sean@oxide.computer>
Date: Wed, 14 Feb 2024 14:57:06 -0800
Subject: [PATCH] Better string validation visibility

---
 nexus/db-queries/src/db/column_walker.rs     |  2 +-
 nexus/db-queries/src/db/raw_query_builder.rs | 30 +++++++++++---------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/nexus/db-queries/src/db/column_walker.rs b/nexus/db-queries/src/db/column_walker.rs
index 6e21e0d899..cace2ba5fb 100644
--- a/nexus/db-queries/src/db/column_walker.rs
+++ b/nexus/db-queries/src/db/column_walker.rs
@@ -36,7 +36,7 @@ macro_rules! impl_column_walker {
                 // are not controlled by an arbitrary user, and
                 // - The "prefix" type, which is a "&'static str" (AKA,
                 // hopefully known at compile-time, and not leaked).
-                TrustedStr::this_string_wont_cause_sql_injections(
+                TrustedStr::i_take_responsibility_for_validating_this_string(
                     [$([prefix, $column::NAME].join("."),)+].join(", ")
                 )
             }
diff --git a/nexus/db-queries/src/db/raw_query_builder.rs b/nexus/db-queries/src/db/raw_query_builder.rs
index a303378c0c..b0094f09ce 100644
--- a/nexus/db-queries/src/db/raw_query_builder.rs
+++ b/nexus/db-queries/src/db/raw_query_builder.rs
@@ -30,10 +30,7 @@ impl BindParamCounter {
 ///
 /// This is basically a workaround for cases where we haven't yet been
 /// able to construct a query at compile-time.
-pub enum TrustedStr {
-    Static(&'static str),
-    ValidatedExplicitly(String),
-}
+pub struct TrustedStr(TrustedStrVariants);
 
 impl TrustedStr {
     /// Explicitly constructs a string, with a name that hopefully
@@ -42,25 +39,32 @@ impl TrustedStr {
     /// If arbitrary user input is provided here, this string COULD
     /// cause SQL injection attacks, so each call-site should have a
     /// justification for "why it's safe".
-    pub fn this_string_wont_cause_sql_injections(s: String) -> Self {
-        Self::ValidatedExplicitly(s)
+    pub fn i_take_responsibility_for_validating_this_string(s: String) -> Self {
+        Self(TrustedStrVariants::ValidatedExplicitly(s))
     }
 
     #[cfg(test)]
     pub fn as_str(&self) -> &str {
-        match self {
-            Self::Static(s) => s,
-            Self::ValidatedExplicitly(s) => s.as_str(),
+        match &self.0 {
+            TrustedStrVariants::Static(s) => s,
+            TrustedStrVariants::ValidatedExplicitly(s) => s.as_str(),
         }
     }
 }
 
 impl From<&'static str> for TrustedStr {
     fn from(s: &'static str) -> Self {
-        Self::Static(s)
+        Self(TrustedStrVariants::Static(s))
     }
 }
 
+// This enum should be kept non-pub to make it harder to accidentally
+// construct a "ValidatedExplicitly" variant.
+enum TrustedStrVariants {
+    Static(&'static str),
+    ValidatedExplicitly(String),
+}
+
 trait SqlQueryBinds {
     fn add_bind(self, bind_counter: &BindParamCounter) -> Self;
 }
@@ -127,9 +131,9 @@ impl QueryBuilder {
     ///
     /// See the documentation of [TrustedStr] for more details.
     pub fn sql<S: Into<TrustedStr>>(self, s: S) -> Self {
-        let query = match s.into() {
-            TrustedStr::Static(s) => self.query.sql(s),
-            TrustedStr::ValidatedExplicitly(s) => self.query.sql(s),
+        let query = match s.into().0 {
+            TrustedStrVariants::Static(s) => self.query.sql(s),
+            TrustedStrVariants::ValidatedExplicitly(s) => self.query.sql(s),
         };
         Self { query, bind_counter: self.bind_counter }
     }