diff --git a/CHANGELOG.md b/CHANGELOG.md index 278281e..c7998b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # CHANGELOG +## 1.0.15 + +### Fixes + +- Fixes a crash where an evaluation error inside a computed/device property call (e.g. an unquoted argument like `daysSince(app_install)`) panicked and aborted the host app. Evaluation errors now propagate and degrade to a non-matching result. (superwall/Superwall-iOS#500) +- Switches the panic strategy to `unwind` and guards all FFI entry points (`evaluateWithContext`, `evaluateAstWithContext`, `evaluateAst`, `parseToAst`) with `catch_unwind`, so on the native iOS/Android builds any future evaluator panic is returned as an `{"Err": ...}` result instead of killing the host process. The WASM/npm build is unaffected by this guard (`wasm32-unknown-unknown` is abort-only); there the protection comes from the error-propagation fix above. +- `parseToAst` now returns a serialized error instead of panicking when the expression does not parse. + +Version 1.0.14 is intentionally skipped: that tag was already used by the `superscript-ios-next` release pipeline. + ## 1.0.13 ### Fixes diff --git a/Cargo.toml b/Cargo.toml index af10e3e..c777fc9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cel-eval" -version = "1.0.13" +version = "1.0.15" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.htmlž @@ -36,7 +36,10 @@ debug=false incremental = false overflow-checks = false codegen-units = 1 -panic = "abort" +# Must stay "unwind": the uniffi FFI entry points rely on catch_unwind so a +# panic (e.g. from a malformed remote expression) can't abort the host app. +# No effect on wasm32-unknown-unknown, whose target spec is abort-only. +panic = "unwind" strip = true [workspace] diff --git a/build_ios.sh b/build_ios.sh index ef93cec..4bb6153 100755 --- a/build_ios.sh +++ b/build_ios.sh @@ -95,19 +95,19 @@ done # For visionOS device SDKROOT="$(xcrun --sdk xros --show-sdk-path)" \ -cargo +nightly build -Zbuild-std=std,core,alloc,panic_abort --target=aarch64-apple-visionos --lib --release +cargo +nightly build -Zbuild-std=std,core,alloc,panic_unwind --target=aarch64-apple-visionos --lib --release # For visionOS simulator SDKROOT="$(xcrun --sdk xrsimulator --show-sdk-path)" \ -cargo +nightly build -Zbuild-std=std,core,alloc,panic_abort --target=aarch64-apple-visionos-sim --lib --release +cargo +nightly build -Zbuild-std=std,core,alloc,panic_unwind --target=aarch64-apple-visionos-sim --lib --release # For watchOS device SDKROOT="$(xcrun --sdk watchos --show-sdk-path)" \ -cargo +nightly build -Zbuild-std=std,core,alloc,panic_abort --target=arm64_32-apple-watchos --lib --release +cargo +nightly build -Zbuild-std=std,core,alloc,panic_unwind --target=arm64_32-apple-watchos --lib --release # For watchOS simulator SDKROOT="$(xcrun --sdk watchsimulator --show-sdk-path)" \ -cargo +nightly build -Zbuild-std=std,core,alloc,panic_abort --target=aarch64-apple-watchos-sim --lib --release +cargo +nightly build -Zbuild-std=std,core,alloc,panic_unwind --target=aarch64-apple-watchos-sim --lib --release SDKROOT="$(xcrun --sdk watchsimulator --show-sdk-path)" \ -cargo +nightly build -Zbuild-std=std,core,alloc,panic_abort --target=x86_64-apple-watchos-sim --lib --release +cargo +nightly build -Zbuild-std=std,core,alloc,panic_unwind --target=x86_64-apple-watchos-sim --lib --release # Rename *.modulemap to module.modulemap diff --git a/src/lib.rs b/src/lib.rs index d6aba85..ddffe58 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,6 +57,31 @@ pub trait ResultCallback: Send + Sync { fn on_result(&self, result: String); } +/** + * Runs the body of an FFI entry point, converting any panic into a serialized + * `Err` result. Expressions arrive from remote config, so a malformed one must + * never abort the host app. Catching requires `panic = "unwind"`; keep + * Cargo.toml and the `-Zbuild-std` flags in build_ios.sh in sync with that. + * On wasm32-unknown-unknown panics still trap (the target is abort-only), so + * the wasm build relies on errors being propagated rather than caught. + */ +fn recovering_from_panics(body: impl FnOnce() -> String) -> String { + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(body)) { + Ok(result) => result, + Err(panic) => { + let message = panic + .downcast_ref::<&str>() + .map(|s| (*s).to_string()) + .or_else(|| panic.downcast_ref::().cloned()) + .unwrap_or_else(|| "unknown panic".to_string()); + let error: Result = + Err(format!("Expression evaluation panicked: {}", message)); + serde_json::to_string(&error) + .unwrap_or_else(|_| "{\"Err\":\"Expression evaluation panicked\"}".to_string()) + } + } +} + /** * Evaluate a CEL expression with the given AST * @param ast The AST Execution Context, serialized as JSON. This defines the AST, the variables, and the platform properties. @@ -64,6 +89,10 @@ pub trait ResultCallback: Send + Sync { * @return The result of the evaluation, either "true" or "false" */ pub fn evaluate_ast_with_context(definition: String, host: Arc) -> String { + recovering_from_panics(move || evaluate_ast_with_context_impl(definition, host)) +} + +fn evaluate_ast_with_context_impl(definition: String, host: Arc) -> String { let data: Result = serde_json::from_str(definition.as_str()); let data = match data { Ok(data) => data, @@ -101,6 +130,10 @@ pub fn evaluate_ast_with_context(definition: String, host: Arc) * @return The result of the evaluation, either "true" or "false" */ pub fn evaluate_ast(ast: String) -> String { + recovering_from_panics(move || evaluate_ast_impl(ast)) +} + +fn evaluate_ast_impl(ast: String) -> String { let data: Result = serde_json::from_str(ast.as_str()); let data: JSONExpression = match data { Ok(data) => data, @@ -126,6 +159,10 @@ pub fn evaluate_ast(ast: String) -> String { */ pub fn evaluate_with_context(definition: String, host: Arc) -> String { + recovering_from_panics(move || evaluate_with_context_impl(definition, host)) +} + +fn evaluate_with_context_impl(definition: String, host: Arc) -> String { let data: Result = serde_json::from_str(definition.as_str()); let data: ExecutionContext = match data { Ok(data) => data, @@ -168,12 +205,22 @@ pub fn evaluate_with_context(definition: String, host: Arc) -> /** * Transforms a given CEL expression into a CEL AST, serialized as JSON. * @param expression The CEL expression to parse - * @return The AST of the expression, serialized as JSON + * @return The AST of the expression, serialized as JSON on success, or a + * serialized `{"Err": ...}` if the expression does not parse. */ pub fn parse_to_ast(expression: String) -> String { - let ast: Result = parse(expression.as_str()).map(|expr| expr.into()); - let ast = ast.map_err(|err| err.to_string()); - serde_json::to_string(&ast.unwrap()).unwrap() + recovering_from_panics(move || match parse(expression.as_str()) { + Ok(expr) => { + let ast: JSONExpression = expr.into(); + serde_json::to_string(&ast) + .unwrap_or_else(|_| "{\"Err\":\"Failed to serialize AST\"}".to_string()) + } + Err(err) => { + let error: Result = Err(err.to_string()); + serde_json::to_string(&error) + .unwrap_or_else(|_| "{\"Err\":\"Failed to parse expression\"}".to_string()) + } + }) } /** @@ -478,6 +525,18 @@ fn execute_with( let host = host_clone.lock(); // Lock the host for safe access match host { Ok(host) => { + // Resolving an argument can fail, e.g. an undeclared reference + // like `daysSince(app_install)`. That must surface as an + // `ExecutionError` (degraded to `Null` by `execute_with`), + // never a panic — a panic here aborts the host app. + let resolved_args = args + .iter() + .map(|expression| { + ftx.ptx + .resolve(expression) + .map(|value| DisplayableValue(value).to_passable()) + }) + .collect::, ExecutionError>>()?; let prop_result = prop_for( if device.contains_key(&it.0) { PropType::Device @@ -485,14 +544,7 @@ fn execute_with( PropType::Computed }, name.clone(), - Some( - args.iter() - .map(|expression| { - DisplayableValue(ftx.ptx.resolve(expression).unwrap()) - .to_passable() - }) - .collect(), - ), + Some(resolved_args), &*host, ); @@ -1543,6 +1595,113 @@ mod tests { assert_eq!(res, "{\"Ok\":{\"type\":\"Null\"}}"); } + /// Builds the execution context from superwall/Superwall-iOS#500: the + /// SDK passes the computed-property functions in both `computed` and + /// `device`, and the expression comes verbatim from dashboard config. + fn dashboard_definition(expression: &str) -> String { + format!( + r#"{{ + "variables": {{ + "map": {{ + "device": {{ + "type": "map", + "value": {{ + "activeEntitlements": {{"type": "list", "value": []}}, + "daysSince_app_install": {{"type": "int", "value": 5}} + }} + }} + }} + }}, + "computed": {{ + "minutesSince": [{{"type": "string", "value": "event_name"}}], + "hoursSince": [{{"type": "string", "value": "event_name"}}], + "daysSince": [{{"type": "string", "value": "event_name"}}] + }}, + "device": {{ + "minutesSince": [{{"type": "string", "value": "event_name"}}], + "hoursSince": [{{"type": "string", "value": "event_name"}}], + "daysSince": [{{"type": "string", "value": "event_name"}}] + }}, + "expression": "{}" + }}"#, + expression + ) + } + + // An unquoted function argument is an undeclared reference. This used to + // panic in the argument-resolution closure and abort the host app + // (superwall/Superwall-iOS#500); it must degrade to a non-matching result. + #[test] + fn test_undeclared_reference_in_function_args_returns_gracefully() { + let ctx = Arc::new(TestContext { + map: HashMap::new(), + }); + let res = evaluate_with_context(dashboard_definition("daysSince(app_install) >= 1"), ctx); + assert_eq!(res, "{\"Ok\":{\"type\":\"Null\"}}"); + } + + // Same as above via the `device.` prefix — the syntax the dashboard + // displays for a stored `device.daysSince_app_install` property. + #[test] + fn test_undeclared_reference_in_device_function_args_returns_gracefully() { + let ctx = Arc::new(TestContext { + map: HashMap::new(), + }); + let res = evaluate_with_context( + dashboard_definition("device.daysSince(app_install) >= 1"), + ctx, + ); + assert_eq!(res, "{\"Ok\":{\"type\":\"Null\"}}"); + } + + // The full filter generated by the dashboard in the incident. + #[test] + fn test_full_dashboard_filter_with_undeclared_reference_does_not_panic() { + let ctx = Arc::new(TestContext { + map: HashMap::new(), + }); + let res = evaluate_with_context( + dashboard_definition( + "(size(device.activeEntitlements) == 0) && (daysSince(app_install) >= 1)", + ), + ctx, + ); + assert_eq!(res, "{\"Ok\":{\"type\":\"Null\"}}"); + } + + // Control: the correctly-quoted argument still resolves through the host. + #[test] + fn test_quoted_function_arg_still_resolves_via_host() { + let days_since = serde_json::to_string(&PassableValue::Int(5)).unwrap(); + let ctx = Arc::new(TestContext { + map: [("daysSince".to_string(), days_since)] + .iter() + .cloned() + .collect(), + }); + let res = evaluate_with_context( + dashboard_definition("daysSince(\\\"app_install\\\") >= 1"), + ctx, + ); + assert_eq!(res, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + } + + #[test] + fn test_panic_guard_converts_panic_to_err_json() { + let res = recovering_from_panics(|| panic!("boom")); + assert_eq!(res, "{\"Err\":\"Expression evaluation panicked: boom\"}"); + } + + #[test] + fn test_parse_to_ast_invalid_expression_returns_err() { + let res = parse_to_ast("daysSince(".to_string()); + assert!( + res.starts_with("{\"Err\":"), + "expected an Err result, got: {}", + res + ); + } + #[test] fn test_list_contains() { let ctx = Arc::new(TestContext { diff --git a/wasm/Cargo.toml b/wasm/Cargo.toml index ed672a2..f2cce0a 100644 --- a/wasm/Cargo.toml +++ b/wasm/Cargo.toml @@ -15,12 +15,8 @@ wasm-bindgen-futures = "0.4.43" futures = "0.3.30" console_error_panic_hook = "0.1.7" -[profile.release] -lto = true -opt-level = "z" # Optimize for size. -codegen-units = 1 -panic = "abort" -strip=true +# No [profile.release] here: cargo ignores profiles in non-root workspace +# members; the workspace root's profile governs this crate too. [package.metadata.wasm-pack.profile.release] wasm-opt = false \ No newline at end of file