Reimplement dangerous request parameter check with visitor

Author: simolus3

.changeset/chilled-lamps-hammer.md

@@ -0,0 +1,5 @@
+---
+'@powersync/service-sync-rules': patch
+---
+
+Remove `usesAuthenticatedRequestParameters` and `usesUnauthenticatedRequestParameters` in favor of `DetectRequestParameters` computing them on-demand.

packages/sync-rules/src/SqlParameterQuery.ts

@@ -30,6 +30,7 @@ import {
   SqliteRow
 } from './types.js';
 import { filterJsonRow, getBucketId, isJsonValue, isSelectStatement, normalizeParameterValue } from './utils.js';
+import { DetectRequestParameters } from './validators.js';
 
 export interface SqlParameterQueryOptions {
   sourceTable: TablePattern;
@@ -496,30 +497,29 @@ export class SqlParameterQuery {
 
   get hasAuthenticatedBucketParameters(): boolean {
     // select request.user_id() as user_id where ...
-    const authenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesAuthenticatedRequestParameters
-      ) != null;
-    return authenticatedExtractor;
+    const visitor = new DetectRequestParameters();
+    visitor.acceptAll(Object.values(this.parameterExtractors));
+
+    return visitor.usesAuthenticatedRequestParameters;
   }
 
   get hasAuthenticatedMatchClause(): boolean {
     // select ... where user_id = request.user_id()
-    const authenticatedInputParameter = this.filter.usesAuthenticatedRequestParameters;
-    return authenticatedInputParameter;
+    const visitor = new DetectRequestParameters();
+    visitor.accept(this.filter);
+    return visitor.usesAuthenticatedRequestParameters;
   }
 
   get usesUnauthenticatedRequestParameters(): boolean {
+    const visitor = new DetectRequestParameters();
+
     // select ... where request.parameters() ->> 'include_comments'
-    const unauthenticatedInputParameter = this.filter.usesUnauthenticatedRequestParameters;
+    visitor.accept(this.filter);
 
     // select request.parameters() ->> 'project_id'
-    const unauthenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesUnauthenticatedRequestParameters
-      ) != null;
+    visitor.acceptAll(Object.values(this.parameterExtractors));
 
-    return unauthenticatedInputParameter || unauthenticatedExtractor;
+    return visitor.usesUnauthenticatedRequestParameters;
   }
 
   /**

packages/sync-rules/src/StaticSqlParameterQuery.ts

@@ -11,6 +11,7 @@ import {
   SqliteJsonValue
 } from './types.js';
 import { getBucketId, isJsonValue } from './utils.js';
+import { DetectRequestParameters } from './validators.js';
 
 export interface StaticSqlParameterQueryOptions {
   sql: string;
@@ -194,26 +195,21 @@ export class StaticSqlParameterQuery {
     // select where request.jwt() ->> 'role' == 'authorized'
     // we do not count this as a sufficient check
     // const authenticatedFilter = this.filter.usesAuthenticatedRequestParameters;
+    const visitor = new DetectRequestParameters();
+    visitor.acceptAll(Object.values(this.parameterExtractors));
 
-    // select request.user_id() as user_id
-    const authenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesAuthenticatedRequestParameters
-      ) != null;
-    return authenticatedExtractor;
+    return visitor.usesAuthenticatedRequestParameters;
   }
 
   get usesUnauthenticatedRequestParameters(): boolean {
+    const visitor = new DetectRequestParameters();
+
     // select where request.parameters() ->> 'include_comments'
-    const unauthenticatedFilter = this.filter?.usesUnauthenticatedRequestParameters;
+    visitor.accept(this.filter);
 
     // select request.parameters() ->> 'project_id'
-    const unauthenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesUnauthenticatedRequestParameters
-      ) != null;
-
-    return unauthenticatedFilter || unauthenticatedExtractor;
+    visitor.acceptAll(Object.values(this.parameterExtractors));
+    return visitor.usesUnauthenticatedRequestParameters;
   }
 
   get usesDangerousRequestParameters() {

packages/sync-rules/src/TableValuedFunctionSqlParameterQuery.ts

@@ -14,6 +14,7 @@ import {
 } from './types.js';
 import { getBucketId, isJsonValue } from './utils.js';
 import { BucketDescription, BucketPriority, DEFAULT_BUCKET_PRIORITY } from './BucketDescription.js';
+import { DetectRequestParameters } from './validators.js';
 
 export interface TableValuedFunctionSqlParameterQueryOptions {
   sql: string;
@@ -260,37 +261,46 @@ export class TableValuedFunctionSqlParameterQuery {
     };
   }
 
+  private visitParameterExtractorsAndCallClause(): DetectRequestParameters {
+    const visitor = new DetectRequestParameters();
+
+    // e.g. select request.user_id() as user_id
+    visitor.acceptAll(Object.values(this.parameterExtractors));
+
+    // e.g. select value from json_each(request.jwt() ->> 'project_ids')
+    visitor.accept(this.callClause);
+
+    return visitor;
+  }
+
   get hasAuthenticatedBucketParameters(): boolean {
     // select where request.jwt() ->> 'role' == 'authorized'
     // we do not count this as a sufficient check
     // const authenticatedFilter = this.filter.usesAuthenticatedRequestParameters;
+    const visitor = new DetectRequestParameters();
 
     // select request.user_id() as user_id
-    const authenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesAuthenticatedRequestParameters
-      ) != null;
+    visitor.acceptAll(Object.values(this.parameterExtractors));
 
     // select value from json_each(request.jwt() ->> 'project_ids')
-    const authenticatedArgument = this.callClause?.usesAuthenticatedRequestParameters ?? false;
+    visitor.accept(this.callClause);
 
-    return authenticatedExtractor || authenticatedArgument;
+    return visitor.usesAuthenticatedRequestParameters;
   }
 
   get usesUnauthenticatedRequestParameters(): boolean {
+    const visitor = new DetectRequestParameters();
+
     // select where request.parameters() ->> 'include_comments'
-    const unauthenticatedFilter = this.filter?.usesUnauthenticatedRequestParameters;
+    visitor.accept(this.filter);
 
     // select request.parameters() ->> 'project_id'
-    const unauthenticatedExtractor =
-      Object.values(this.parameterExtractors).find(
-        (clause) => isParameterValueClause(clause) && clause.usesUnauthenticatedRequestParameters
-      ) != null;
+    visitor.acceptAll(Object.values(this.parameterExtractors));
 
     // select value from json_each(request.parameters() ->> 'project_ids')
-    const unauthenticatedArgument = this.callClause?.usesUnauthenticatedRequestParameters ?? false;
+    visitor.accept(this.callClause);
 
-    return unauthenticatedFilter || unauthenticatedExtractor || unauthenticatedArgument;
+    return visitor.usesUnauthenticatedRequestParameters;
   }
 
   get usesDangerousRequestParameters() {

packages/sync-rules/src/request_functions.ts

@@ -1,7 +1,7 @@
 import { ExpressionType } from './ExpressionType.js';
 import { CompatibilityContext, CompatibilityEdition, CompatibilityOption } from './compatibility.js';
 import { generateSqlFunctions } from './sql_functions.js';
-import { ParameterValueSet, SqliteValue } from './types.js';
+import { CompiledClause, ParameterValueClause, ParameterValueSet, SqliteValue } from './types.js';
 
 export interface SqlParameterFunction {
   readonly debugName: string;
@@ -131,3 +131,14 @@ const REQUEST_FUNCTIONS_NAMED = {
 };
 
 export const REQUEST_FUNCTIONS: Record<string, SqlParameterFunction> = REQUEST_FUNCTIONS_NAMED;
+
+/**
+ * A {@link ParameterValueClause} derived from a call to a {@link SqlParameterFunction}.
+ */
+export interface RequestFunctionCall extends ParameterValueClause {
+  function: SqlParameterFunction;
+}
+
+export function isRequestFunctionCall(clause: CompiledClause): clause is RequestFunctionCall {
+  return (clause as RequestFunctionCall).function != null;
+}

packages/sync-rules/src/sql_filters.ts

@@ -4,7 +4,7 @@ import { nil } from 'pgsql-ast-parser/src/utils.js';
 import { BucketPriority, isValidPriority } from './BucketDescription.js';
 import { ExpressionType } from './ExpressionType.js';
 import { SqlRuleError } from './errors.js';
-import { REQUEST_FUNCTIONS, SqlParameterFunction } from './request_functions.js';
+import { REQUEST_FUNCTIONS, RequestFunctionCall, SqlParameterFunction } from './request_functions.js';
 import {
   BASIC_OPERATORS,
   OPERATOR_IN,
@@ -35,6 +35,7 @@ import {
   ClauseError,
   CompiledClause,
   InputParameter,
+  LegacyParameterFromTableClause,
   ParameterMatchClause,
   ParameterValueClause,
   QueryParameters,
@@ -419,14 +420,14 @@ export class SqlTools {
 
           const parameterArguments = compiledArguments as ParameterValueClause[];
           return {
+            function: impl,
             key: `${schema}.${fn}(${parameterArguments.map((p) => p.key).join(',')})`,
             lookupParameterValue(parameters) {
               const evaluatedArgs = parameterArguments.map((p) => p.lookupParameterValue(parameters));
               return impl.call(parameters, ...evaluatedArgs);
             },
-            usesAuthenticatedRequestParameters: impl.usesAuthenticatedRequestParameters,
-            usesUnauthenticatedRequestParameters: impl.usesUnauthenticatedRequestParameters
-          } satisfies ParameterValueClause;
+            visitChildren: (v) => parameterArguments.forEach(v)
+          } satisfies RequestFunctionCall;
         }
       }
 
@@ -494,8 +495,7 @@ export class SqlTools {
             return { [inputParam.key]: value };
           });
         },
-        usesAuthenticatedRequestParameters: leftFilter.usesAuthenticatedRequestParameters,
-        usesUnauthenticatedRequestParameters: leftFilter.usesUnauthenticatedRequestParameters
+        visitChildren: (v) => v(leftFilter)
       } satisfies ParameterMatchClause;
     } else if (
       this.supportsExpandingParameters &&
@@ -530,8 +530,7 @@ export class SqlTools {
           }
           return [{ [inputParam.key]: value }];
         },
-        usesAuthenticatedRequestParameters: rightFilter.usesAuthenticatedRequestParameters,
-        usesUnauthenticatedRequestParameters: rightFilter.usesUnauthenticatedRequestParameters
+        visitChildren: (v) => v(rightFilter)
       } satisfies ParameterMatchClause;
     } else {
       // Not supported, return the error previously computed
@@ -579,8 +578,7 @@ export class SqlTools {
             return { [inputParam.key]: value };
           });
         },
-        usesAuthenticatedRequestParameters: leftFilter.usesAuthenticatedRequestParameters,
-        usesUnauthenticatedRequestParameters: leftFilter.usesUnauthenticatedRequestParameters
+        visitChildren: (v) => v(leftFilter)
       } satisfies ParameterMatchClause;
     } else if (
       this.supportsExpandingParameters &&
@@ -622,8 +620,7 @@ export class SqlTools {
             return { [inputParam.key]: value };
           });
         },
-        usesAuthenticatedRequestParameters: rightFilter.usesAuthenticatedRequestParameters,
-        usesUnauthenticatedRequestParameters: rightFilter.usesUnauthenticatedRequestParameters
+        visitChildren: (v) => v(rightFilter)
       } satisfies ParameterMatchClause;
     } else {
       // Not supported, return the error previously computed
@@ -652,8 +649,7 @@ export class SqlTools {
 
         return [{ [inputParam.key]: value }];
       },
-      usesAuthenticatedRequestParameters: otherFilter.usesAuthenticatedRequestParameters,
-      usesUnauthenticatedRequestParameters: otherFilter.usesUnauthenticatedRequestParameters
+      visitChildren: (v) => v(otherFilter)
     } satisfies ParameterMatchClause;
   }
 
@@ -776,17 +772,16 @@ export class SqlTools {
     }
   }
 
-  private getParameterRefClause(expr: ExprRef): ParameterValueClause {
+  private getParameterRefClause(expr: ExprRef): LegacyParameterFromTableClause {
     const table = AvailableTable.search(expr.table?.name ?? this.defaultTable!, this.parameterTables)!.nameInSchema;
     const column = expr.name;
     return {
+      table,
       key: `${table}.${column}`,
       lookupParameterValue: (parameters) => {
         return parameters.lookup(table, column);
-      },
-      usesAuthenticatedRequestParameters: table == 'token_parameters',
-      usesUnauthenticatedRequestParameters: table == 'user_parameters'
-    } satisfies ParameterValueClause;
+      }
+    } satisfies LegacyParameterFromTableClause;
   }
 
   refHasSchema(ref: ExprRef) {
@@ -861,12 +856,7 @@ export class SqlTools {
     } else if (argsType == 'param') {
       const argStrings = argClauses.map((e) => (e as ParameterValueClause).key);
       const name = `${fnImpl.debugName}(${argStrings.join(',')})`;
-      const usesAuthenticatedRequestParameters =
-        argClauses.find((clause) => isParameterValueClause(clause) && clause.usesAuthenticatedRequestParameters) !=
-        null;
-      const usesUnauthenticatedRequestParameters =
-        argClauses.find((clause) => isParameterValueClause(clause) && clause.usesUnauthenticatedRequestParameters) !=
-        null;
+
       return {
         key: name,
         lookupParameterValue: (parameters) => {
@@ -881,8 +871,7 @@ export class SqlTools {
           });
           return fnImpl.call(...args);
         },
-        usesAuthenticatedRequestParameters,
-        usesUnauthenticatedRequestParameters
+        visitChildren: (v) => argClauses.forEach(v)
       } satisfies ParameterValueClause;
     } else {
       throw new Error('unreachable condition');
@@ -983,9 +972,7 @@ function staticValueClause(value: SqliteValue): StaticValueClause {
     key: JSONBig.stringify(value),
     lookupParameterValue(_parameters) {
       return value;
-    },
-    usesAuthenticatedRequestParameters: false,
-    usesUnauthenticatedRequestParameters: false
+    }
   };
 }