From 0f1854bb2155b88f36ac24ff35f96a95bcf9a6dc Mon Sep 17 00:00:00 2001 From: Tom Ballinger Date: Mon, 11 Mar 2024 12:50:34 -0700 Subject: [PATCH] Validate args for system UDFs (#23235) Validate arguments for system UDFs GitOrigin-RevId: 1c482648ad68cbf544f1eb256f059c35a8283bcb --- crates/isolate/src/environment/action/mod.rs | 10 ++- crates/isolate/src/environment/analyze.rs | 11 ++- crates/isolate/src/environment/auth_config.rs | 15 +++- crates/isolate/src/environment/mod.rs | 11 ++- crates/isolate/src/environment/schema.rs | 15 +++- crates/isolate/src/environment/udf/mod.rs | 12 +++- crates/isolate/src/execution_scope.rs | 5 +- crates/isolate/src/ops/database.rs | 13 ++-- crates/isolate/src/ops/mod.rs | 1 + crates/isolate/src/ops/validate_args.rs | 72 +++++++++++++++++++ crates/isolate/src/tests/mod.rs | 1 + crates/isolate/src/tests/system_udfs.rs | 51 +++++++++++++ .../src/node_action_callbacks.rs | 4 +- .../_system/frontend/getTableMapping.ts | 2 +- .../convex/_system/secretSystemTables.ts | 13 +++- npm-packages/udf-tests/convex/idStrings.ts | 6 +- 16 files changed, 219 insertions(+), 23 deletions(-) create mode 100644 crates/isolate/src/ops/validate_args.rs create mode 100644 crates/isolate/src/tests/system_udfs.rs diff --git a/crates/isolate/src/environment/action/mod.rs b/crates/isolate/src/environment/action/mod.rs index 9333dc94..5aacd256 100644 --- a/crates/isolate/src/environment/action/mod.rs +++ b/crates/isolate/src/environment/action/mod.rs @@ -81,7 +81,9 @@ use value::{ heap_size::HeapSize, ConvexArray, Size, + TableMapping, TableMappingValue, + VirtualTableMapping, }; use self::{ @@ -1081,8 +1083,12 @@ impl IsolateEnvironment for ActionEnvironment { self.phase.get_environment_variable(name) } - fn get_table_mapping(&mut self) -> anyhow::Result { - anyhow::bail!("get_table_mapping unsupported in actions") + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result { + anyhow::bail!("get_table_mapping_without_system_tables unsupported in actions") + } + + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)> { + anyhow::bail!("get_all_table_mappings unsupported in actions") } // We lookup all modules' sources upfront when initializing the action diff --git a/crates/isolate/src/environment/analyze.rs b/crates/isolate/src/environment/analyze.rs index ebf4fd48..45101a9b 100644 --- a/crates/isolate/src/environment/analyze.rs +++ b/crates/isolate/src/environment/analyze.rs @@ -72,7 +72,9 @@ use sync_types::{ }; use value::{ heap_size::WithHeapSize, + TableMapping, TableMappingValue, + VirtualTableMapping, }; use crate::{ @@ -151,7 +153,14 @@ impl IsolateEnvironment for AnalyzeEnvironment { Ok(value) } - fn get_table_mapping(&mut self) -> anyhow::Result { + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result { + anyhow::bail!(ErrorMetadata::bad_request( + "NoTableMappingFetchDuringImport", + "Getting the table mapping unsupported at import time" + )) + } + + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)> { anyhow::bail!(ErrorMetadata::bad_request( "NoTableMappingFetchDuringImport", "Getting the table mapping unsupported at import time" diff --git a/crates/isolate/src/environment/auth_config.rs b/crates/isolate/src/environment/auth_config.rs index 10d71216..43273c8a 100644 --- a/crates/isolate/src/environment/auth_config.rs +++ b/crates/isolate/src/environment/auth_config.rs @@ -33,7 +33,11 @@ use rand_chacha::ChaCha12Rng; use regex::Regex; use serde::Deserialize; use serde_json::Value as JsonValue; -use value::TableMappingValue; +use value::{ + TableMapping, + TableMappingValue, + VirtualTableMapping, +}; use crate::{ concurrency_limiter::ConcurrencyPermit, @@ -118,7 +122,14 @@ impl IsolateEnvironment for AuthConfigEnvironment { .map(Some) } - fn get_table_mapping(&mut self) -> anyhow::Result { + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result { + anyhow::bail!(ErrorMetadata::bad_request( + "NoTableMappingFetchDuringAuthConfig", + "Getting the table mapping unsupported when evaluating auth config file" + )) + } + + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)> { anyhow::bail!(ErrorMetadata::bad_request( "NoTableMappingFetchDuringAuthConfig", "Getting the table mapping unsupported when evaluating auth config file" diff --git a/crates/isolate/src/environment/mod.rs b/crates/isolate/src/environment/mod.rs index f70afd2c..7bf46ff8 100644 --- a/crates/isolate/src/environment/mod.rs +++ b/crates/isolate/src/environment/mod.rs @@ -28,7 +28,11 @@ use model::modules::module_versions::{ }; use rand::Rng; use serde_json::Value as JsonValue; -use value::TableMappingValue; +use value::{ + TableMapping, + TableMappingValue, + VirtualTableMapping, +}; pub use self::async_op::AsyncOpRequest; use crate::{ @@ -80,7 +84,10 @@ pub trait IsolateEnvironment: 'static { fn get_environment_variable(&mut self, name: EnvVarName) -> anyhow::Result>; - fn get_table_mapping(&mut self) -> anyhow::Result; + + /// The table mapping omitting system tables, intended for the dashboard. + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result; + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)>; fn start_async_op( &mut self, diff --git a/crates/isolate/src/environment/schema.rs b/crates/isolate/src/environment/schema.rs index 70efcafd..d42aed38 100644 --- a/crates/isolate/src/environment/schema.rs +++ b/crates/isolate/src/environment/schema.rs @@ -36,7 +36,11 @@ use model::{ use rand::SeedableRng; use rand_chacha::ChaCha12Rng; use serde_json::Value as JsonValue; -use value::TableMappingValue; +use value::{ + TableMapping, + TableMappingValue, + VirtualTableMapping, +}; use crate::{ concurrency_limiter::ConcurrencyPermit, @@ -101,7 +105,14 @@ impl IsolateEnvironment for SchemaEnvironment { )) } - fn get_table_mapping(&mut self) -> anyhow::Result { + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result { + anyhow::bail!(ErrorMetadata::bad_request( + "NoTableMappingFetchInSchema", + "Getting the table mapping unsupported when evaluating schema" + )) + } + + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)> { anyhow::bail!(ErrorMetadata::bad_request( "NoTableMappingFetchInSchema", "Getting the table mapping unsupported when evaluating schema" diff --git a/crates/isolate/src/environment/udf/mod.rs b/crates/isolate/src/environment/udf/mod.rs index 045fd34a..dabd0898 100644 --- a/crates/isolate/src/environment/udf/mod.rs +++ b/crates/isolate/src/environment/udf/mod.rs @@ -89,7 +89,9 @@ use value::{ WithHeapSize, }, Size, + TableMapping, TableMappingValue, + VirtualTableMapping, MAX_DOCUMENT_NESTING, MAX_USER_SIZE, VALUE_TOO_LARGE_SHORT_MSG, @@ -254,10 +256,18 @@ impl IsolateEnvironment for DatabaseUdfEnvironment { self.phase.get_environment_variable(name) } - fn get_table_mapping(&mut self) -> anyhow::Result { + fn get_table_mapping_without_system_tables(&mut self) -> anyhow::Result { Ok(self.phase.tx()?.table_mapping().clone().into()) } + fn get_all_table_mappings(&mut self) -> anyhow::Result<(TableMapping, VirtualTableMapping)> { + let tx = self.phase.tx()?; + Ok(( + tx.table_mapping().clone(), + tx.virtual_table_mapping().clone(), + )) + } + async fn lookup_source( &mut self, path: &str, diff --git a/crates/isolate/src/execution_scope.rs b/crates/isolate/src/execution_scope.rs index f89cae46..8c41786f 100644 --- a/crates/isolate/src/execution_scope.rs +++ b/crates/isolate/src/execution_scope.rs @@ -568,7 +568,10 @@ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment> ExecutionScope<'a, 'b, "atob" => self.op_atob(args, rv)?, "btoa" => self.op_btoa(args, rv)?, "environmentVariables/get" => self.op_environmentVariables_get(args, rv)?, - "getTableMapping" => self.op_getTableMapping(args, rv)?, + "getTableMappingWithoutSystemTables" => { + self.op_getTableMappingWithoutSystemTables(args, rv)? + }, + "validateArgs" => self.op_validate_args(args, rv)?, "crypto/randomUUID" => self.op_crypto_randomUUID(args, rv)?, "crypto/getRandomValues" => self.op_crypto_getRandomValues(args, rv)?, "crypto/sign" => self.op_crypto_sign(args, rv)?, diff --git a/crates/isolate/src/ops/database.rs b/crates/isolate/src/ops/database.rs index 748e9220..d6dc7ab2 100644 --- a/crates/isolate/src/ops/database.rs +++ b/crates/isolate/src/ops/database.rs @@ -9,11 +9,14 @@ use crate::{ impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment> ExecutionScope<'a, 'b, RT, E> { #[convex_macro::v8_op] - pub fn op_getTableMapping(&mut self) -> anyhow::Result { + pub fn op_getTableMappingWithoutSystemTables(&mut self) -> anyhow::Result { let state = self.state_mut()?; - state.environment.get_table_mapping().and_then(|mapping| { - serde_json::to_value(mapping) - .map_err(|_| anyhow!("Couldn’t serialize the table mapping")) - }) + state + .environment + .get_table_mapping_without_system_tables() + .and_then(|mapping| { + serde_json::to_value(mapping) + .map_err(|_| anyhow!("Couldn’t serialize the table mapping")) + }) } } diff --git a/crates/isolate/src/ops/mod.rs b/crates/isolate/src/ops/mod.rs index c96ade12..fbd9f8ff 100644 --- a/crates/isolate/src/ops/mod.rs +++ b/crates/isolate/src/ops/mod.rs @@ -16,5 +16,6 @@ mod storage; mod stream; mod text; mod time; +mod validate_args; pub use self::crypto::CryptoOps; diff --git a/crates/isolate/src/ops/validate_args.rs b/crates/isolate/src/ops/validate_args.rs new file mode 100644 index 00000000..a793c3f5 --- /dev/null +++ b/crates/isolate/src/ops/validate_args.rs @@ -0,0 +1,72 @@ +use std::convert::{ + TryFrom, + TryInto, +}; + +use anyhow::Context; +use common::{ + self, + runtime::Runtime, +}; +use model::{ + self, + modules::args_validator::ArgsValidator, +}; +use serde_json::{ + json, + Value as JsonValue, +}; +use value::ConvexArray; + +use crate::{ + environment::IsolateEnvironment, + execution_scope::ExecutionScope, + helpers::UdfArgsJson, +}; + +impl<'a, 'b: 'a, RT: Runtime, E: IsolateEnvironment> ExecutionScope<'a, 'b, RT, E> { + #[convex_macro::v8_op] + + pub fn op_validate_args( + &mut self, + validator: JsonValue, + args: UdfArgsJson, + ) -> anyhow::Result { + let JsonValue::String(validator_string) = validator.clone() else { + return Err(anyhow::anyhow!("export_args result not a string")); + }; + + let args_validator: ArgsValidator = + match serde_json::from_str::(&validator_string) { + Ok(args_json) => match ArgsValidator::try_from(args_json) { + Ok(validator) => validator, + Err(err) => return Err(err), + }, + Err(json_error) => { + let message = + format!("Unable to parse JSON returned from `exportArgs`: {json_error}"); + return Err(anyhow::anyhow!(message)); + }, + }; + + let args_array = args + .into_arg_vec() + .into_iter() + .map(|arg| arg.try_into()) + .collect::>>() + .and_then(ConvexArray::try_from) + .map_err(|err| anyhow::anyhow!(format!("{}", err)))?; + + let state = self.state_mut()?; + let (table_mapping, virtual_table_mapping) = state.environment.get_all_table_mappings()?; + match args_validator.check_args(&args_array, &table_mapping, &virtual_table_mapping)? { + Some(js_error) => Ok(json!({ + "valid": false, + "message": format!("{}", js_error) + })), + None => Ok(json!({ + "valid": true, + })), + } + } +} diff --git a/crates/isolate/src/tests/mod.rs b/crates/isolate/src/tests/mod.rs index b281825f..52d51821 100644 --- a/crates/isolate/src/tests/mod.rs +++ b/crates/isolate/src/tests/mod.rs @@ -28,6 +28,7 @@ mod search; mod shapes; mod size_errors; mod source_maps; +mod system_udfs; mod unicode; mod user_error; mod vector_search; diff --git a/crates/isolate/src/tests/system_udfs.rs b/crates/isolate/src/tests/system_udfs.rs new file mode 100644 index 00000000..74995c7d --- /dev/null +++ b/crates/isolate/src/tests/system_udfs.rs @@ -0,0 +1,51 @@ +use std::collections::HashSet; + +use common::{ + assert_obj, + types::EnvironmentVariable, + value::ConvexValue, +}; +use keybroker::Identity; +use model::environment_variables::EnvironmentVariablesModel; +use must_let::must_let; +use runtime::testing::TestRuntime; +use value::ConvexObject; + +use crate::test_helpers::UdfTest; + +#[convex_macro::test_runtime] +async fn test_system_udf(rt: TestRuntime) -> anyhow::Result<()> { + let t = UdfTest::default(rt).await?; + let mut tx = t.database.begin(Identity::system()).await?; + let environment_variable = EnvironmentVariable::new("A".parse()?, "B".parse()?); + EnvironmentVariablesModel::new(&mut tx) + .create(environment_variable, &HashSet::new()) + .await?; + t.database.commit(tx).await?; + + // nonexistent returns Null + must_let!(let ConvexValue::Null = t.query( + "_system/cli/queryEnvironmentVariables:get", + assert_obj!("name" => "nonexistent".to_string()), + ) + .await?); + + // return environment variable + must_let!(let ConvexValue::Object(obj) = t.query( + "_system/cli/queryEnvironmentVariables:get", + assert_obj!("name" => "A".to_string()), + ) + .await?); + must_let!(let Some(ConvexValue::String(name)) = ConvexObject::get(&obj, "name")); + assert_eq!(name.to_string(), "A"); + must_let!(let Some(ConvexValue::String(value)) = ConvexObject::get(&obj, "value")); + assert_eq!(value.to_string(), "B"); + + // calling with empty argument fails + let error = t + .query_js_error("_system/cli/queryEnvironmentVariables:get", assert_obj!()) + .await?; + + assert!(error.message.contains("missing the required field `name`")); + Ok(()) +} diff --git a/crates/local_backend/src/node_action_callbacks.rs b/crates/local_backend/src/node_action_callbacks.rs index ed97f202..428dccaa 100644 --- a/crates/local_backend/src/node_action_callbacks.rs +++ b/crates/local_backend/src/node_action_callbacks.rs @@ -508,7 +508,7 @@ mod tests { let json_body = json!({ "path": "_system/frontend/paginatedScheduledJobs.js", - "args":json!({"paginationOpts": {"numItems": 10}}), + "args":json!({"paginationOpts": {"numItems": 10, "cursor": null}}), "format": "json", }); let body = Body::from(serde_json::to_vec(&json_body)?); @@ -571,7 +571,7 @@ mod tests { let json_body = json!({ "path": "_system/frontend/paginatedScheduledJobs.js", - "args":json!({"paginationOpts": {"numItems": 10}}), + "args":json!({"paginationOpts": {"numItems": 10, "cursor": null}}), "format": "json", }); let body = Body::from(serde_json::to_vec(&json_body)?); diff --git a/npm-packages/system-udfs/convex/_system/frontend/getTableMapping.ts b/npm-packages/system-udfs/convex/_system/frontend/getTableMapping.ts index 88a5474e..d2cdea3e 100644 --- a/npm-packages/system-udfs/convex/_system/frontend/getTableMapping.ts +++ b/npm-packages/system-udfs/convex/_system/frontend/getTableMapping.ts @@ -8,6 +8,6 @@ import { queryPrivateSystem } from "../secretSystemTables"; export default queryPrivateSystem({ args: {}, handler: async () => { - return performOp("getTableMapping"); + return performOp("getTableMappingWithoutSystemTables"); }, }); diff --git a/npm-packages/system-udfs/convex/_system/secretSystemTables.ts b/npm-packages/system-udfs/convex/_system/secretSystemTables.ts index 1e0ad8d5..35438cab 100644 --- a/npm-packages/system-udfs/convex/_system/secretSystemTables.ts +++ b/npm-packages/system-udfs/convex/_system/secretSystemTables.ts @@ -9,6 +9,7 @@ import { import { Validator } from "convex/values"; import { query as baseQuery } from "../_generated/server"; import { Id } from "../_generated/dataModel"; +import { performOp } from "../syscall"; // This set must be kept up-to-date to prevent accidental access to secret // system tables in system UDFs. @@ -95,6 +96,7 @@ function maskPublicSystem( type FunctionDefinition = { args: Record>; handler: (ctx: any, args: DefaultFunctionArgs) => any; + exportArgs(): string; }; /// `queryPrivateSystem` is for querying private system tables. @@ -106,9 +108,18 @@ export const queryPrivateSystem = ((functionDefinition: FunctionDefinition) => { if (!("args" in functionDefinition)) { throw new Error("args validator required for system udf"); } + const query = baseQuery({ + args: functionDefinition.args, + handler: () => {}, + }); + const argsValidatorJson = query.exportArgs(); return baseQuery({ args: functionDefinition.args, - handler: (ctx: any, args: any) => { + handler: async (ctx: any, args: any) => { + const result = await performOp("validateArgs", argsValidatorJson, args); + if (!result.valid) { + throw new Error(result.message); + } return functionDefinition.handler( { ...ctx, db: maskPrivateSystem(ctx.db) }, args, diff --git a/npm-packages/udf-tests/convex/idStrings.ts b/npm-packages/udf-tests/convex/idStrings.ts index 9b249666..c91da89a 100644 --- a/npm-packages/udf-tests/convex/idStrings.ts +++ b/npm-packages/udf-tests/convex/idStrings.ts @@ -3,7 +3,7 @@ import { query } from "./_generated/server"; /** * Copied and pasted from syscall.ts - * We don't normally allow UDFs to call ops, but this is for testing the system UDF `getTableMapping` + * We don't normally allow UDFs to call ops, but this is for testing the system UDF `getTableMappingWithoutSystemTables` **/ declare const Convex: { syscall: (op: string, jsonArgs: string) => string; @@ -14,7 +14,7 @@ declare const Convex: { /** * Copied and pasted from syscall.ts - * We don't normally allow UDFs to call ops, but this is for testing the system UDF `getTableMapping` + * We don't normally allow UDFs to call ops, but this is for testing the system UDF`getTableMappingWithoutSystemTables` **/ function performOp(op: string, ...args: any[]): any { if (typeof Convex === "undefined" || Convex.op === undefined) { @@ -30,7 +30,7 @@ function performOp(op: string, ...args: any[]): any { * so that it can be tested in UDF tests. */ export const getTableMapping = query(async () => { - return performOp("getTableMapping"); + return performOp("getTableMappingWithoutSystemTables"); }); export const normalizeId = query({