Fix critical security vulnerability in automigration code

Author: kazimuth

crates/schema/src/auto_migrate.rs

@@ -43,7 +43,7 @@ pub struct AutoMigratePlan<'def> {
     /// There is also an implied check: that the schema in the database is compatible with the old ModuleDef.
     pub prechecks: Vec<AutoMigratePrecheck<'def>>,
     /// The migration steps to perform.
-    /// Order should not matter, as the steps are independent.
+    /// Order matters: `Remove`s of a particular `Def` must be ordered before `Add`s.
     pub steps: Vec<AutoMigrateStep<'def>>,
 }
 
@@ -59,29 +59,46 @@ pub enum AutoMigratePrecheck<'def> {
 /// A step in an automatic migration.
 #[derive(PartialEq, Eq, Debug, PartialOrd, Ord)]
 pub enum AutoMigrateStep<'def> {
+    // It is important FOR CORRECTNESS that `Remove` variants are declared before `Add` variants in this enum!
+    //
+    // The ordering is used to sort the steps of an auto-migration.
+    // If adds go before removes, the following can occur:
+    //
+    // 1. `AddIndex("my_special_boy)`
+    // 2. `RemoveIndex("my_special_boy)`
+    //
+    // You see the problem.
+    //
+    // For now, we just ensure that we declare all `Remove` variants before `Add` variants
+    // and let `#[derive(PartialOrd)]` take care of the rest.
+    // TODO: when this enum is made serializable, a more durable fix will be needed here.
+    // Probably we will want to have separate arrays of add and remove steps.
+    //
+    /// Remove an index.
+    RemoveIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
+    /// Remove a constraint.
+    RemoveConstraint(<ConstraintDef as ModuleDefLookup>::Key<'def>),
+    /// Remove a sequence.
+    RemoveSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
+    /// Remove a schedule annotation from a table.
+    RemoveSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
+    /// Remove a row-level security query.
+    RemoveRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),
+
     /// Add a table, including all indexes, constraints, and sequences.
     /// There will NOT be separate steps in the plan for adding indexes, constraints, and sequences.
     AddTable(<TableDef as ModuleDefLookup>::Key<'def>),
     /// Add an index.
     AddIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
-    /// Remove an index.
-    RemoveIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
-    /// Remove a constraint.
-    RemoveConstraint(<ConstraintDef as ModuleDefLookup>::Key<'def>),
     /// Add a sequence.
     AddSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
-    /// Remove a sequence.
-    RemoveSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
-    /// Change the access of a table.
-    ChangeAccess(<TableDef as ModuleDefLookup>::Key<'def>),
     /// Add a schedule annotation to a table.
     AddSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
-    /// Remove a schedule annotation from a table.
-    RemoveSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
     /// Add a row-level security query.
     AddRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),
-    /// Remove a row-level security query.
-    RemoveRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),
+
+    /// Change the access of a table.
+    ChangeAccess(<TableDef as ModuleDefLookup>::Key<'def>),
 }
 
 /// Something that might prevent an automatic migration.
@@ -412,9 +429,14 @@ fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Id
         .collect_all_errors()
 }
 
-// Because we can refer to many tables and fields on the row level-security query, we need to remove all of them,
-// then add the new ones, instead of trying to track the graph of dependencies.
 fn auto_migrate_row_level_security(plan: &mut AutoMigratePlan) -> Result<()> {
+    // Because we can refer to many tables and fields on the row level-security query, we need to remove all of them,
+    // then add the new ones, instead of trying to track the graph of dependencies.
+    // When pretty-printing, steps to remove and re-add a row-level-security policy are not shown,
+    // since they are an implementation detail that will be surprising to users.
+    // TODO: change this to not add-and-remove unchanged policies, and hand the responsibility for reinitializing
+    // queries to the `core` crate instead.
+    // When you do this, you will need to update `pretty_print.rs`.
     for rls in plan.old.row_level_security() {
         plan.steps.push(AutoMigrateStep::RemoveRowLevelSecurity(rls.key()));
     }

crates/schema/src/auto_migrate/pretty_print.rs

@@ -6,7 +6,7 @@ use colored::{self, Colorize};
 use lazy_static::lazy_static;
 use regex::Regex;
 use spacetimedb_lib::{
-    db::raw_def::v9::{TableAccess, TableType},
+    db::raw_def::v9::{RawRowLevelSecurityDefV9, TableAccess, TableType},
     AlgebraicType,
 };
 use spacetimedb_primitives::{ColId, ColList};
@@ -188,8 +188,9 @@ pub fn pretty_print(plan: &MigratePlan) -> Result<String, fmt::Error> {
 
                 write!(
                     outr,
-                    "- {} auto-increment constraint on column {} of table {}",
+                    "- {} auto-increment constraint {} on column {} of table {}",
                     removed,
+                    constraint_name(*sequence),
                     column_name_from_id(table_def, sequence_def.column),
                     table_name(&*table_def.name),
                 )?;
@@ -232,10 +233,26 @@ pub fn pretty_print(plan: &MigratePlan) -> Result<String, fmt::Error> {
                 )?;
             }
             AutoMigrateStep::AddRowLevelSecurity(rls) => {
+                // Implementation detail: Row-level-security policies are always removed and re-added
+                // because the `core` crate needs to recompile some stuff.
+                // We hide this from the user.
+                if plan.old.lookup::<RawRowLevelSecurityDefV9>(*rls)
+                    == plan.new.lookup::<RawRowLevelSecurityDefV9>(*rls)
+                {
+                    continue;
+                }
                 writeln!(outr, "- {} row level security policy:", created)?;
                 writeln!(outr, "    `{}`", rls.blue())?;
             }
             AutoMigrateStep::RemoveRowLevelSecurity(rls) => {
+                // Implementation detail: Row-level-security policies are always removed and re-added
+                // because the `core` crate needs to recompile some stuff.
+                // We hide this from the user.
+                if plan.old.lookup::<RawRowLevelSecurityDefV9>(*rls)
+                    == plan.new.lookup::<RawRowLevelSecurityDefV9>(*rls)
+                {
+                    continue;
+                }
                 writeln!(outr, "- {} row level security policy:", removed)?;
                 writeln!(outr, "    `{}`", rls.blue())?;
             }

crates/schema/src/def.rs

@@ -122,6 +122,7 @@ pub struct ModuleDef {
     /// The row-level security policies.
     ///
     /// **Note**: Are only validated syntax-wise.
+    // TODO: for consistency, this should be changed to store a `RowLevelSecurityDef` instead.
     row_level_security_raw: HashMap<RawSql, RawRowLevelSecurityDefV9>,
 }