feat(electric): Add server-side validation of null values incoming fr…

…om Satellite clients (#327) Admitedly, we're relying on client-provided SatRelation messages to decide whether any given column is nullable or not. So it's not a good protection against malicious clients. I wish we didn't have to pass nullability info via SatRelation message from the server to the client and back. But apparently that's what we have to do since we're not storing the history of schema changes for every individual client.
electric-sql · Aug 15, 2023 · 00d5a67 · 00d5a67
1 parent 571119a
commit 00d5a67
Show file tree

Hide file tree

Showing 20 changed files with 244 additions and 52 deletions.
diff --git a/.changeset/twenty-poets-hear.md b/.changeset/twenty-poets-hear.md
@@ -0,0 +1,5 @@
+---
+"@core/electric": patch
+---
+
+Add server-side enforcement of "NOT NULL" for values incoming from Satellite clients
diff --git a/clients/typescript/src/_generated/protocol/satellite.ts b/clients/typescript/src/_generated/protocol/satellite.ts
@@ -187,6 +187,7 @@ export interface SatRelationColumn {
   name: string;
   type: string;
   primaryKey: boolean;
+  isNullable: boolean;
 }
 
 export interface SatRelation {
@@ -1242,7 +1243,13 @@ export const SatInStopReplicationResp = {
 messageTypeRegistry.set(SatInStopReplicationResp.$type, SatInStopReplicationResp);
 
 function createBaseSatRelationColumn(): SatRelationColumn {
-  return { $type: "Electric.Satellite.v1_4.SatRelationColumn", name: "", type: "", primaryKey: false };
+  return {
+    $type: "Electric.Satellite.v1_4.SatRelationColumn",
+    name: "",
+    type: "",
+    primaryKey: false,
+    isNullable: false,
+  };
 }
 
 export const SatRelationColumn = {
@@ -1258,6 +1265,9 @@ export const SatRelationColumn = {
     if (message.primaryKey === true) {
       writer.uint32(24).bool(message.primaryKey);
     }
+    if (message.isNullable === true) {
+      writer.uint32(32).bool(message.isNullable);
+    }
     return writer;
   },
 
@@ -1289,6 +1299,13 @@ export const SatRelationColumn = {
 
           message.primaryKey = reader.bool();
           continue;
+        case 4:
+          if (tag !== 32) {
+            break;
+          }
+
+          message.isNullable = reader.bool();
+          continue;
       }
       if ((tag & 7) === 4 || tag === 0) {
         break;
@@ -1307,6 +1324,7 @@ export const SatRelationColumn = {
     message.name = object.name ?? "";
     message.type = object.type ?? "";
     message.primaryKey = object.primaryKey ?? false;
+    message.isNullable = object.isNullable ?? false;
     return message;
   },
 };

diff --git a/clients/typescript/src/satellite/client.ts b/clients/typescript/src/satellite/client.ts
@@ -550,7 +550,11 @@ export class SatelliteClient extends EventEmitter implements Client {
           tableName: relation.table,
           tableType: relation.tableType,
           columns: relation.columns.map((c) =>
-            SatRelationColumn.fromPartial({ name: c.name, type: c.type })
+            SatRelationColumn.fromPartial({
+              name: c.name,
+              type: c.type,
+              isNullable: c.isNullable,
+            })
           ),
         })
 
@@ -750,6 +754,7 @@ export class SatelliteClient extends EventEmitter implements Client {
       columns: message.columns.map((c) => ({
         name: c.name,
         type: c.type,
+        isNullable: c.isNullable,
         primaryKey: c.primaryKey,
       })),
     }

diff --git a/clients/typescript/src/satellite/process.ts b/clients/typescript/src/satellite/process.ts
@@ -1375,6 +1375,7 @@ export class SatelliteProcess implements Satellite {
         relation.columns.push({
           name: c.name!.toString(),
           type: c.type!.toString(),
+          isNullable: Boolean(!c.notnull!.valueOf()),
           primaryKey: Boolean(c.pk!.valueOf()),
         })
       }

diff --git a/clients/typescript/src/util/types.ts b/clients/typescript/src/util/types.ts
@@ -152,6 +152,7 @@ export type Relation = {
 export type RelationColumn = {
   name: string
   type: string
+  isNullable: boolean
   primaryKey?: boolean
 }
 

diff --git a/clients/typescript/test/client/model/shapes.test.ts b/clients/typescript/test/client/model/shapes.test.ts
@@ -189,26 +189,31 @@ const relations = {
       {
         name: 'id',
         type: 'INTEGER',
+        isNullable: false,
         primaryKey: true,
       },
       {
         name: 'title',
         type: 'TEXT',
+        isNullable: true,
         primaryKey: false,
       },
       {
         name: 'contents',
         type: 'TEXT',
+        isNullable: true,
         primaryKey: false,
       },
       {
         name: 'nbr',
         type: 'INTEGER',
+        isNullable: true,
         primaryKey: false,
       },
       {
         name: 'authorId',
         type: 'INTEGER',
+        isNullable: true,
         primaryKey: false,
       },
     ],
@@ -222,16 +227,19 @@ const relations = {
       {
         name: 'id',
         type: 'INTEGER',
+        isNullable: false,
         primaryKey: true,
       },
       {
         name: 'bio',
         type: 'TEXT',
+        isNullable: true,
         primaryKey: false,
       },
       {
         name: 'userId',
         type: 'INTEGER',
+        isNullable: true,
         primaryKey: false,
       },
     ],

diff --git a/clients/typescript/test/satellite/client.test.ts b/clients/typescript/test/satellite/client.test.ts
@@ -309,8 +309,8 @@ test.serial('receive transaction over multiple messages', async (t) => {
     table: 'table',
     tableType: Proto.SatRelation_RelationType.TABLE,
     columns: [
-      { name: 'name1', type: 'TEXT' },
-      { name: 'name2', type: 'TEXT' },
+      { name: 'name1', type: 'TEXT', isNullable: true },
+      { name: 'name2', type: 'TEXT', isNullable: true },
     ],
   }
 
@@ -320,8 +320,16 @@ test.serial('receive transaction over multiple messages', async (t) => {
     tableName: 'table',
     tableType: Proto.SatRelation_RelationType.TABLE,
     columns: [
-      Proto.SatRelationColumn.fromPartial({ name: 'name1', type: 'TEXT' }),
-      Proto.SatRelationColumn.fromPartial({ name: 'name2', type: 'TEXT' }),
+      Proto.SatRelationColumn.fromPartial({
+        name: 'name1',
+        type: 'TEXT',
+        isNullable: true,
+      }),
+      Proto.SatRelationColumn.fromPartial({
+        name: 'name2',
+        type: 'TEXT',
+        isNullable: true,
+      }),
     ],
   })
 
@@ -614,12 +622,12 @@ test.serial('default and null test', async (t) => {
     table: 'Items',
     tableType: Proto.SatRelation_RelationType.TABLE,
     columns: [
-      { name: 'id', type: 'uuid' },
-      { name: 'content', type: 'text' },
-      { name: 'text_null', type: 'text' },
-      { name: 'text_null_default', type: 'text' },
-      { name: 'intvalue_null', type: 'integer' },
-      { name: 'intvalue_null_default', type: 'integer' },
+      { name: 'id', type: 'uuid', isNullable: false },
+      { name: 'content', type: 'text', isNullable: false },
+      { name: 'text_null', type: 'text', isNullable: true },
+      { name: 'text_null_default', type: 'text', isNullable: true },
+      { name: 'intvalue_null', type: 'integer', isNullable: true },
+      { name: 'intvalue_null_default', type: 'integer', isNullable: true },
     ],
   }
 
@@ -929,8 +937,8 @@ test.serial('subscription correct protocol sequence with data', async (t) => {
     table: 'table',
     tableType: Proto.SatRelation_RelationType.TABLE,
     columns: [
-      { name: 'name1', type: 'TEXT' },
-      { name: 'name2', type: 'TEXT' },
+      { name: 'name1', type: 'TEXT', isNullable: true },
+      { name: 'name2', type: 'TEXT', isNullable: true },
     ],
   }
 

diff --git a/clients/typescript/test/satellite/common.ts b/clients/typescript/test/satellite/common.ts
@@ -27,11 +27,13 @@ export const relations = {
       {
         name: 'id',
         type: 'INTEGER',
+        isNullable: false,
         primaryKey: true,
       },
       {
         name: 'parent',
         type: 'INTEGER',
+        isNullable: true,
         primaryKey: false,
       },
     ],
@@ -45,16 +47,19 @@ export const relations = {
       {
         name: 'id',
         type: 'INTEGER',
+        isNullable: false,
         primaryKey: true,
       },
       {
         name: 'value',
         type: 'TEXT',
+        isNullable: true,
         primaryKey: false,
       },
       {
         name: 'other',
         type: 'INTEGER',
+        isNullable: true,
         primaryKey: false,
       },
     ],
@@ -68,6 +73,7 @@ export const relations = {
       {
         name: 'id',
         type: 'INTEGER',
+        isNullable: false,
         primaryKey: true,
       },
     ],

diff --git a/clients/typescript/test/satellite/process.migration.test.ts b/clients/typescript/test/satellite/process.migration.test.ts
@@ -157,21 +157,25 @@ const addColumnRelation = {
     {
       name: 'id',
       type: 'INTEGER',
+      isNullable: false,
       primaryKey: true,
     },
     {
       name: 'value',
       type: 'TEXT',
+      isNullable: true,
       primaryKey: false,
     },
     {
       name: 'other',
       type: 'INTEGER',
+      isNullable: true,
       primaryKey: false,
     },
     {
       name: 'baz',
       type: 'TEXT',
+      isNullable: true,
       primaryKey: false,
     },
   ],
@@ -185,16 +189,19 @@ const newTableRelation = {
     {
       name: 'id',
       type: 'TEXT',
+      isNullable: false,
       primaryKey: true,
     },
     {
       name: 'foo',
       type: 'INTEGER',
+      isNullable: true,
       primaryKey: false,
     },
     {
       name: 'bar',
       type: 'TEXT',
+      isNullable: true,
       primaryKey: false,
     },
   ],

diff --git a/clients/typescript/test/satellite/serialization.test.ts b/clients/typescript/test/satellite/serialization.test.ts
@@ -10,13 +10,13 @@ test('serialize/deserialize row data', async (t) => {
     table: 'table',
     tableType: SatRelation_RelationType.TABLE,
     columns: [
-      { name: 'name1', type: 'TEXT' },
-      { name: 'name2', type: 'TEXT' },
-      { name: 'name3', type: 'TEXT' },
-      { name: 'int1', type: 'INTEGER' },
-      { name: 'int2', type: 'INTEGER' },
-      { name: 'float1', type: 'FLOAT4' },
-      { name: 'float2', type: 'FLOAT4' },
+      { name: 'name1', type: 'TEXT', isNullable: true },
+      { name: 'name2', type: 'TEXT', isNullable: true },
+      { name: 'name3', type: 'TEXT', isNullable: true },
+      { name: 'int1', type: 'INTEGER', isNullable: true },
+      { name: 'int2', type: 'INTEGER', isNullable: true },
+      { name: 'float1', type: 'FLOAT4', isNullable: true },
+      { name: 'float2', type: 'FLOAT4', isNullable: true },
     ],
   }
 
@@ -42,15 +42,15 @@ test('Null mask uses bits as if they were a list', async (t) => {
     table: 'table',
     tableType: SatRelation_RelationType.TABLE,
     columns: [
-      { name: 'bit0', type: 'TEXT' },
-      { name: 'bit1', type: 'TEXT' },
-      { name: 'bit2', type: 'TEXT' },
-      { name: 'bit3', type: 'TEXT' },
-      { name: 'bit4', type: 'TEXT' },
-      { name: 'bit5', type: 'TEXT' },
-      { name: 'bit6', type: 'TEXT' },
-      { name: 'bit7', type: 'TEXT' },
-      { name: 'bit8', type: 'TEXT' },
+      { name: 'bit0', type: 'TEXT', isNullable: true },
+      { name: 'bit1', type: 'TEXT', isNullable: true },
+      { name: 'bit2', type: 'TEXT', isNullable: true },
+      { name: 'bit3', type: 'TEXT', isNullable: true },
+      { name: 'bit4', type: 'TEXT', isNullable: true },
+      { name: 'bit5', type: 'TEXT', isNullable: true },
+      { name: 'bit6', type: 'TEXT', isNullable: true },
+      { name: 'bit7', type: 'TEXT', isNullable: true },
+      { name: 'bit8', type: 'TEXT', isNullable: true },
     ],
   }
 

diff --git a/components/electric/lib/electric/postgres/replication.ex b/components/electric/lib/electric/postgres/replication.ex
@@ -10,6 +10,7 @@ defmodule Electric.Postgres.Replication do
     defstruct [
       :name,
       :type,
+      :nullable?,
       type_modifier: -1,
       part_of_identity?: false
     ]
@@ -19,6 +20,7 @@ defmodule Electric.Postgres.Replication do
     @type t() :: %__MODULE__{
             name: name(),
             type: atom(),
+            nullable?: boolean(),
             type_modifier: integer(),
             part_of_identity?: boolean() | nil
           }

diff --git a/components/electric/lib/electric/postgres/schema.ex b/components/electric/lib/electric/postgres/schema.ex
@@ -192,6 +192,7 @@ defmodule Electric.Postgres.Schema do
         %Replication.Column{
           name: col.name,
           type: col_type(col.type),
+          nullable?: col_nullable?(col),
           type_modifier: List.first(col.type.size, -1),
           # since we're using replication identity "full" all columns
           # are identity columns in replication terms
@@ -219,6 +220,12 @@ defmodule Electric.Postgres.Schema do
   defp col_type("serial8"), do: :int8
   defp col_type(t) when is_binary(t), do: String.to_atom(t)
 
+  defp col_nullable?(col) do
+    col.constraints
+    |> Enum.find(&match?(%Proto.Constraint{constraint: {:not_null, _}}, &1))
+    |> is_nil()
+  end
+
   def relation(schema, sname, tname) do
     with {:ok, table} <- fetch_table(schema, {sname, tname}) do
       table_info(table)