From 071d0aceda5664cff6d4f4c29433a664486f6c94 Mon Sep 17 00:00:00 2001 From: Aaron Eline Date: Fri, 6 Oct 2023 13:52:25 -0400 Subject: [PATCH] Improving our panic audit (#340) (#344) --- .cargo/config.toml | 1 + .github/workflows/ci.yml | 2 +- cedar-policy-core/src/ast/policy.rs | 4 ++++ cedar-policy-core/src/ast/policy_set.rs | 2 ++ cedar-policy-core/src/ast/request.rs | 18 ++++++++++++++ cedar-policy-core/src/ast/value.rs | 2 ++ cedar-policy-core/src/authorizer.rs | 2 ++ cedar-policy-core/src/entities.rs | 10 ++++++++ .../src/entities/json/context.rs | 2 ++ cedar-policy-core/src/est.rs | 2 ++ cedar-policy-core/src/evaluator.rs | 2 ++ cedar-policy-core/src/extensions/decimal.rs | 2 ++ cedar-policy-core/src/extensions/ipaddr.rs | 2 ++ cedar-policy-core/src/parser.rs | 2 ++ cedar-policy-core/src/parser/cst_to_ast.rs | 2 ++ cedar-policy-core/src/parser/text_to_cst.rs | 2 ++ cedar-policy-core/src/transitive_closure.rs | 2 ++ .../src/extensions/decimal.rs | 24 ++++++++++++++++--- .../src/extensions/ipaddr.rs | 24 ++++++++++++++++--- .../src/extensions/partial_evaluation.rs | 22 ++++++++++++++--- cedar-policy-validator/src/rbac.rs | 4 ++++ cedar-policy-validator/src/schema.rs | 4 ++++ .../src/schema_file_format.rs | 2 ++ cedar-policy-validator/src/str_checks.rs | 4 ++++ cedar-policy-validator/src/typecheck.rs | 20 ++++++++++++++++ .../src/typecheck/test_strict.rs | 4 ++++ .../src/typecheck/test_type_annotation.rs | 5 +++- .../src/typecheck/test_utils.rs | 4 ++++ cedar-policy-validator/src/types.rs | 4 ++++ cedar-policy/benches/cedar_benchmarks.rs | 2 ++ cedar-policy/src/api.rs | 22 +++++++++++++++++ cedar-policy/src/frontend/is_authorized.rs | 2 ++ cedar-policy/src/frontend/validate.rs | 2 ++ cedar-policy/src/integration_testing.rs | 2 ++ cedar-policy/tests/corpus_tests.rs | 2 ++ panic_safety.sh | 2 +- 36 files changed, 202 insertions(+), 12 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index e3a4154f8..d802b00ab 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -5,4 +5,5 @@ rustflags = [ "-Dclippy::fallible_impl_from", "-Dclippy::unreachable", "-Dclippy::indexing_slicing", + "-Dclippy::panic", ] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb8be4b20..649e6025a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,7 +22,7 @@ jobs: - run: cargo test --verbose --features "experimental" - run: cargo test --verbose - run: cargo doc --all-features - - run: cargo clippy + - run: cargo clippy --all-features - run: ./panic_safety.sh - run: cargo test --verbose -- --ignored - run: cargo bench --no-run diff --git a/cedar-policy-core/src/ast/policy.rs b/cedar-policy-core/src/ast/policy.rs index 2a84c179e..e5873f085 100644 --- a/cedar-policy-core/src/ast/policy.rs +++ b/cedar-policy-core/src/ast/policy.rs @@ -1527,6 +1527,10 @@ pub mod test_generators { } #[cfg(test)] +// PANIC SAFETY: Unit Test Code +#[allow(clippy::indexing_slicing)] +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] mod test { use std::collections::HashSet; diff --git a/cedar-policy-core/src/ast/policy_set.rs b/cedar-policy-core/src/ast/policy_set.rs index b7aa570f7..ee7ecbeec 100644 --- a/cedar-policy-core/src/ast/policy_set.rs +++ b/cedar-policy-core/src/ast/policy_set.rs @@ -263,6 +263,8 @@ impl std::fmt::Display for PolicySet { } } +// PANIC SAFETY tests +#[allow(clippy::panic)] // PANIC SAFETY tests #[allow(clippy::indexing_slicing)] #[cfg(test)] diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index 1e5bb1421..c4014a090 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -153,6 +153,7 @@ impl std::fmt::Display for Request { #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Context { /// an `Expr::Record` that qualifies as a "restricted expression" + // INVARIANT: `context` must be a `Record` #[serde(flatten)] context: RestrictedExpr, } @@ -164,6 +165,7 @@ impl Context { } /// Create a `Context` from a `RestrictedExpr`, which must be a `Record` + /// INVARIANT: `from_expr` must only be called with an `expr` that is a `Record` pub fn from_expr(expr: RestrictedExpr) -> Self { debug_assert!(matches!(expr.expr_kind(), ExprKind::Record { .. })); Self { context: expr } @@ -171,6 +173,7 @@ impl Context { /// Create a `Context` from a map of key to `RestrictedExpr`, or a Vec of /// `(key, RestrictedExpr)` pairs, or any other iterator of `(key, RestrictedExpr)` pairs + // INVARIANT: always constructs a record pub fn from_pairs(pairs: impl IntoIterator) -> Self { Self { context: RestrictedExpr::record(pairs), @@ -184,6 +187,7 @@ impl Context { /// /// For schema-based parsing, use `ContextJsonParser`. pub fn from_json_str(json: &str) -> Result { + // INVARIANT `.from_json_str` always produces an expression of variant `Record` ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available()) .from_json_str(json) } @@ -195,6 +199,7 @@ impl Context { /// /// For schema-based parsing, use `ContextJsonParser`. pub fn from_json_value(json: serde_json::Value) -> Result { + // INVARIANT `.from_json_value` always produces an expression of variant `Record` ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available()) .from_json_value(json) } @@ -206,12 +211,15 @@ impl Context { /// /// For schema-based parsing, use `ContextJsonParser`. pub fn from_json_file(json: impl std::io::Read) -> Result { + // INVARIANT `.from_json_file` always produces an expression of variant `Record` ContextJsonParser::new(None::<&NullContextSchema>, Extensions::all_available()) .from_json_file(json) } /// Iterate over the (key, value) pairs in the `Context` pub fn iter(&self) -> impl Iterator)> { + // PANIC SAFETY invariant on `self.context` ensures that it is a Record + #[allow(clippy::panic)] match self.context.as_ref().expr_kind() { ExprKind::Record { pairs } => pairs .iter() @@ -238,3 +246,13 @@ impl std::fmt::Display for Context { write!(f, "{}", self.context) } } + +#[cfg(test)] +mod test { + + #[test] + fn test_json_from_str_non_record() { + let src = "1"; + assert!(super::Context::from_json_str(src).is_err()); + } +} diff --git a/cedar-policy-core/src/ast/value.rs b/cedar-policy-core/src/ast/value.rs index 62a50cc1e..6b3004be0 100644 --- a/cedar-policy-core/src/ast/value.rs +++ b/cedar-policy-core/src/ast/value.rs @@ -454,6 +454,8 @@ impl Value { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod test { use super::*; diff --git a/cedar-policy-core/src/authorizer.rs b/cedar-policy-core/src/authorizer.rs index c60b02873..770df62f6 100644 --- a/cedar-policy-core/src/authorizer.rs +++ b/cedar-policy-core/src/authorizer.rs @@ -393,6 +393,8 @@ impl std::fmt::Debug for Authorizer { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod test { use std::collections::BTreeMap; diff --git a/cedar-policy-core/src/entities.rs b/cedar-policy-core/src/entities.rs index 162c522b1..5da17ff96 100644 --- a/cedar-policy-core/src/entities.rs +++ b/cedar-policy-core/src/entities.rs @@ -265,6 +265,8 @@ where /// # Panics /// /// Panics if the self value is not `Data`. + // PANIC SAFETY: This function is intended to panic, and says so in the documentation + #[allow(clippy::panic)] pub fn unwrap(self) -> &'a T { match self { Self::Data(e) => e, @@ -281,6 +283,8 @@ where /// # Panics /// /// Panics if the self value is not `Data`. + // PANIC SAFETY: This function is intended to panic, and says so in the documentation + #[allow(clippy::panic)] pub fn expect(self, msg: &str) -> &'a T { match self { Self::Data(e) => e, @@ -320,6 +324,8 @@ pub enum TCComputation { ComputeNow, } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod json_parsing_tests { use super::*; @@ -822,6 +828,8 @@ mod json_parsing_tests { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod entities_tests { use super::*; @@ -894,6 +902,8 @@ mod entities_tests { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod schema_based_parsing_tests { use super::*; diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 782582a15..5478ddac5 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -80,6 +80,8 @@ impl<'e, 's, S: ContextSchema> ContextJsonParser<'e, 's, S> { let rexpr = vparser.val_into_rexpr(json, expected_ty.as_ref(), || { JsonDeserializationErrorContext::Context })?; + // INVARIANT `Context::from_exprs` requires that `rexpr` is a `Record`. + // This is checked by the `match` expression match rexpr.expr_kind() { ExprKind::Record { .. } => Ok(Context::from_expr(rexpr)), _ => Err(JsonDeserializationError::ExpectedContextToBeRecord { diff --git a/cedar-policy-core/src/est.rs b/cedar-policy-core/src/est.rs index fb343eac5..e74793fe7 100644 --- a/cedar-policy-core/src/est.rs +++ b/cedar-policy-core/src/est.rs @@ -278,6 +278,8 @@ impl From for Clause { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod test { use super::*; diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index d839e5b97..3c2760569 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -857,6 +857,8 @@ fn stack_size_check() -> Result<()> { Ok(()) } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] pub mod test { use std::str::FromStr; diff --git a/cedar-policy-core/src/extensions/decimal.rs b/cedar-policy-core/src/extensions/decimal.rs index 6e12e5753..254afd0c5 100644 --- a/cedar-policy-core/src/extensions/decimal.rs +++ b/cedar-policy-core/src/extensions/decimal.rs @@ -279,6 +279,8 @@ pub fn extension() -> Extension { } #[cfg(test)] +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] mod tests { use super::*; use crate::ast::{Expr, Type, Value}; diff --git a/cedar-policy-core/src/extensions/ipaddr.rs b/cedar-policy-core/src/extensions/ipaddr.rs index c82b191fb..8df3a061a 100644 --- a/cedar-policy-core/src/extensions/ipaddr.rs +++ b/cedar-policy-core/src/extensions/ipaddr.rs @@ -380,6 +380,8 @@ pub fn extension() -> Extension { ) } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod tests { use super::*; diff --git a/cedar-policy-core/src/parser.rs b/cedar-policy-core/src/parser.rs index 55773bf6b..a640dd8c3 100644 --- a/cedar-policy-core/src/parser.rs +++ b/cedar-policy-core/src/parser.rs @@ -336,6 +336,8 @@ pub(crate) fn parse_ident(id: &str) -> Result { } } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod test { use super::*; diff --git a/cedar-policy-core/src/parser/cst_to_ast.rs b/cedar-policy-core/src/parser/cst_to_ast.rs index f9beb7aa3..79f04fb8b 100644 --- a/cedar-policy-core/src/parser/cst_to_ast.rs +++ b/cedar-policy-core/src/parser/cst_to_ast.rs @@ -2152,6 +2152,8 @@ fn construct_expr_record(kvs: Vec<(SmolStr, ast::Expr)>, l: SourceInfo) -> ast:: ast::ExprBuilder::new().with_source_info(l).record(kvs) } +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod tests { use super::*; diff --git a/cedar-policy-core/src/parser/text_to_cst.rs b/cedar-policy-core/src/parser/text_to_cst.rs index 0050f7d7e..3b814a4a0 100644 --- a/cedar-policy-core/src/parser/text_to_cst.rs +++ b/cedar-policy-core/src/parser/text_to_cst.rs @@ -25,6 +25,8 @@ lalrpop_mod!( #[allow(clippy::indexing_slicing)] //PANIC SAFETY: lalrpop uses unreachable, and we are trusting lalrpop to generate correct code #[allow(clippy::unreachable)] + //PANIC SAFETY: lalrpop uses panic, and we are trusting lalrpop to generate correct code + #[allow(clippy::panic)] pub grammar, "/src/parser/grammar.rs" ); diff --git a/cedar-policy-core/src/transitive_closure.rs b/cedar-policy-core/src/transitive_closure.rs index efcd0f73e..b8a50963d 100644 --- a/cedar-policy-core/src/transitive_closure.rs +++ b/cedar-policy-core/src/transitive_closure.rs @@ -185,6 +185,8 @@ where // PANIC SAFETY test cases #[allow(clippy::indexing_slicing)] +// PANIC SAFETY: Unit Test Code +#[allow(clippy::panic)] #[cfg(test)] mod tests { use crate::ast::{Entity, EntityUID}; diff --git a/cedar-policy-validator/src/extensions/decimal.rs b/cedar-policy-validator/src/extensions/decimal.rs index cac7691b0..fa602bfc7 100644 --- a/cedar-policy-validator/src/extensions/decimal.rs +++ b/cedar-policy-validator/src/extensions/decimal.rs @@ -13,6 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +//! Note on panic safety +//! If any of the panics in this file are triggered, that means that this file has become +//! out-of-date with the decimal extension definition in CedarCore. +//! This is tested by the `extension_schema_correctness()` test use crate::extension_schema::{ArgumentCheckFn, ExtensionFunctionType, ExtensionSchema}; use crate::types::{self, Type}; @@ -21,9 +25,8 @@ use cedar_policy_core::evaluator::RestrictedEvaluator; use cedar_policy_core::extensions::{decimal, Extensions}; use std::str::FromStr; -/// If any of the panics in this file are triggered, that means that this file has become -/// out-of-date with the decimal extension definition in CedarCore. - +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_argument_types(fname: &str, decimal_ty: &Type) -> Vec { match fname { "decimal" => vec![Type::primitive_string()], @@ -34,6 +37,8 @@ fn get_argument_types(fname: &str, decimal_ty: &Type) -> Vec { } } +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_return_type(fname: &str, decimal_ty: &Type) -> Type { match fname { "decimal" => decimal_ty.clone(), @@ -44,6 +49,8 @@ fn get_return_type(fname: &str, decimal_ty: &Type) -> Type { } } +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_argument_check(fname: &str) -> Option { match fname { "decimal" => Some(Box::new(validate_decimal_string)), @@ -96,3 +103,14 @@ fn validate_decimal_string(exprs: &[Expr]) -> Result<(), String> { _ => Ok(()), } } + +#[cfg(test)] +mod test { + use super::*; + + // Ensures that `extension_schema()` does not panic + #[test] + fn extension_schema_correctness() { + let _ = extension_schema(); + } +} diff --git a/cedar-policy-validator/src/extensions/ipaddr.rs b/cedar-policy-validator/src/extensions/ipaddr.rs index bd175d869..9b9100fb9 100644 --- a/cedar-policy-validator/src/extensions/ipaddr.rs +++ b/cedar-policy-validator/src/extensions/ipaddr.rs @@ -13,6 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +//! Note on panic safety +//! If any of the panics in this file are triggered, that means that this file has become +//! out-of-date with the decimal extension definition in CedarCore. +//! This is tested by the `extension_schema_correctness()` test use crate::extension_schema::{ArgumentCheckFn, ExtensionFunctionType, ExtensionSchema}; use crate::types::{self, Type}; @@ -21,9 +25,8 @@ use cedar_policy_core::evaluator::RestrictedEvaluator; use cedar_policy_core::extensions::{ipaddr, Extensions}; use std::str::FromStr; -/// If any of the panics in this file are triggered, that means that this file has become -/// out-of-date with the ipaddr extension definition in CedarCore. - +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_argument_types(fname: &str, ipaddr_ty: &Type) -> Vec { match fname { "ip" => vec![Type::primitive_string()], @@ -33,6 +36,8 @@ fn get_argument_types(fname: &str, ipaddr_ty: &Type) -> Vec { } } +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_return_type(fname: &str, ipaddr_ty: &Type) -> Type { match fname { "ip" => ipaddr_ty.clone(), @@ -43,6 +48,8 @@ fn get_return_type(fname: &str, ipaddr_ty: &Type) -> Type { } } +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_argument_check(fname: &str) -> Option { match fname { "ip" => Some(Box::new(validate_ip_string)), @@ -96,3 +103,14 @@ fn validate_ip_string(exprs: &[Expr]) -> Result<(), String> { _ => Ok(()), } } + +#[cfg(test)] +mod test { + use super::*; + + // Ensures that `extension_schema()` does not panic + #[test] + fn extension_schema_correctness() { + let _ = extension_schema(); + } +} diff --git a/cedar-policy-validator/src/extensions/partial_evaluation.rs b/cedar-policy-validator/src/extensions/partial_evaluation.rs index b6c749933..409e425e7 100644 --- a/cedar-policy-validator/src/extensions/partial_evaluation.rs +++ b/cedar-policy-validator/src/extensions/partial_evaluation.rs @@ -13,14 +13,17 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +//! Note on panic safety +//! If any of the panics in this file are triggered, that means that this file has become +//! out-of-date with the decimal extension definition in CedarCore. +//! This is tested by the `extension_schema_correctness()` test use crate::extension_schema::{ExtensionFunctionType, ExtensionSchema}; use crate::types::{self, Type}; use cedar_policy_core::extensions::partial_evaluation; -/// If any of the panics in this file are triggered, that means that this file has become -/// out-of-date with the decimal extension definition in CedarCore. - +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_argument_types(fname: &str) -> Vec { match fname { "error" => vec![Type::primitive_string()], @@ -29,6 +32,8 @@ fn get_argument_types(fname: &str) -> Vec { } } +// PANIC SAFETY see note on panic safety above +#[allow(clippy::panic)] fn get_return_type(fname: &str) -> Type { match fname { "error" => Type::Never, @@ -61,3 +66,14 @@ pub fn extension_schema() -> ExtensionSchema { .collect(); ExtensionSchema::new(pe_ext.name().clone(), fun_tys) } + +#[cfg(test)] +mod test { + use super::*; + + // Ensures that `extension_schema()` does not panic + #[test] + fn extension_schema_correctness() { + let _ = extension_schema(); + } +} diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index dd8177d39..d0288a896 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -422,6 +422,10 @@ impl Validator { } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] +// PANIC SAFETY unit tests +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { use std::collections::{BTreeMap, HashMap, HashSet}; diff --git a/cedar-policy-validator/src/schema.rs b/cedar-policy-validator/src/schema.rs index 15a6a702e..41e2ac9e0 100644 --- a/cedar-policy-validator/src/schema.rs +++ b/cedar-policy-validator/src/schema.rs @@ -1685,6 +1685,10 @@ impl TryInto for NamespaceDefinitionWithActionAttributes { } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] +// PANIC SAFETY unit tests +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { use std::{collections::BTreeMap, str::FromStr}; diff --git a/cedar-policy-validator/src/schema_file_format.rs b/cedar-policy-validator/src/schema_file_format.rs index 4206bf575..3ea479800 100644 --- a/cedar-policy-validator/src/schema_file_format.rs +++ b/cedar-policy-validator/src/schema_file_format.rs @@ -467,6 +467,8 @@ impl SchemaType { } #[cfg(feature = "arbitrary")] +// PANIC SAFETY property testing code +#[allow(clippy::panic)] impl<'a> arbitrary::Arbitrary<'a> for SchemaType { fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { use cedar_policy_core::ast::Name; diff --git a/cedar-policy-validator/src/str_checks.rs b/cedar-policy-validator/src/str_checks.rs index cb92d9023..651fc085a 100644 --- a/cedar-policy-validator/src/str_checks.rs +++ b/cedar-policy-validator/src/str_checks.rs @@ -138,6 +138,10 @@ const BIDI_CHARS: [char; 9] = [ '\u{2069}', ]; +// PANIC SAFETY unit tests +#[allow(clippy::panic)] +// PANIC SAFETY unit tests +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index 5835251a7..7877fba32 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -691,6 +691,7 @@ impl<'a> Typechecker<'a> { } } + // INVARIANT: `e` is a `BinaryApp` ExprKind::BinaryApp { .. } => self.strict_transform_binary(e, type_errors), // The elements of a set must share a single type. We @@ -890,11 +891,14 @@ impl<'a> Typechecker<'a> { } } + // INVARIANT: `bin_expr` must be a `BinaryApp` fn strict_transform_binary<'b>( &self, bin_expr: &Expr>, type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { + // PANIC SAFETY By invariant on this method + #[allow(clippy::panic)] let ExprKind::BinaryApp { op, arg1, arg2 } = bin_expr.expr_kind() else { panic!( "`strict_transform_binary` called with an expression kind other than `BinaryApp`" @@ -1560,15 +1564,19 @@ impl<'a> Typechecker<'a> { } ExprKind::UnaryApp { .. } => { + // INVARIANT `e` is a `UnaryApp`, as required self.typecheck_unary(request_env, prior_eff, e, type_errors) } ExprKind::BinaryApp { .. } => { + // INVARIANT `e` is a `BinaryApp`, as required self.typecheck_binary(request_env, prior_eff, e, type_errors) } ExprKind::MulByConst { .. } => { + // INVARIANT `e` is a `MulByConst`, as required self.typecheck_mul(request_env, prior_eff, e, type_errors) } ExprKind::ExtensionFunctionApp { .. } => { + // INVARIANT `e` is a `ExtensionFunctionApp`, as required self.typecheck_extension(request_env, prior_eff, e, type_errors) } @@ -1845,6 +1853,7 @@ impl<'a> Typechecker<'a> { /// A utility called by the main typecheck method to handle binary operator /// application. + /// INVARIANT `bin_expr` must be a `BinaryApp` fn typecheck_binary<'b>( &self, request_env: &RequestEnv, @@ -1852,6 +1861,8 @@ impl<'a> Typechecker<'a> { bin_expr: &'b Expr, type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { + // PANIC SAFETY by invariant on method + #[allow(clippy::panic)] let ExprKind::BinaryApp { op, arg1, arg2 } = bin_expr.expr_kind() else { panic!("`typecheck_binary` called with an expression kind other than `BinaryApp`"); }; @@ -1971,6 +1982,7 @@ impl<'a> Typechecker<'a> { /// Like `typecheck_binary()`, but for multiplication, which isn't /// technically a `BinaryOp` + /// INVARIANT `mul_expr` must be a `MulByConst` fn typecheck_mul<'b>( &self, request_env: &RequestEnv, @@ -1978,6 +1990,8 @@ impl<'a> Typechecker<'a> { mul_expr: &'b Expr, type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { + // PANIC SAFETY by invariant on method + #[allow(clippy::panic)] let ExprKind::MulByConst { arg, constant } = mul_expr.expr_kind() else { panic!("`typecheck_mul` called with an expression kind other than `MulByConst`"); }; @@ -2410,6 +2424,7 @@ impl<'a> Typechecker<'a> { /// A utility called by the main typecheck method to handle unary operator /// application. + /// INVARIANT `unary_expr` must be a UnaryApp fn typecheck_unary<'b>( &self, request_env: &RequestEnv, @@ -2417,6 +2432,8 @@ impl<'a> Typechecker<'a> { unary_expr: &'b Expr, type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { + // PANIC SAFETY by invariant on method + #[allow(clippy::panic)] let ExprKind::UnaryApp { op, arg } = unary_expr.expr_kind() else { panic!("`typecheck_unary` called with an expression kind other than `UnaryApp`"); }; @@ -2603,6 +2620,7 @@ impl<'a> Typechecker<'a> { /// Utility called by the main typecheck method to handle extension function /// application. + /// INVARIANT `ext_expr` must be a `ExtensionFunctionApp` fn typecheck_extension<'b>( &self, request_env: &RequestEnv, @@ -2610,6 +2628,8 @@ impl<'a> Typechecker<'a> { ext_expr: &'b Expr, type_errors: &mut Vec, ) -> TypecheckAnswer<'b> { + // PANIC SAFETY by invariant on method + #[allow(clippy::panic)] let ExprKind::ExtensionFunctionApp { fn_name, args } = ext_expr.expr_kind() else { panic!("`typecheck_extension` called with an expression kind other than `ExtensionFunctionApp`"); }; diff --git a/cedar-policy-validator/src/typecheck/test_strict.rs b/cedar-policy-validator/src/typecheck/test_strict.rs index d4914e8ea..2ff299eb4 100644 --- a/cedar-policy-validator/src/typecheck/test_strict.rs +++ b/cedar-policy-validator/src/typecheck/test_strict.rs @@ -18,6 +18,10 @@ #![cfg(test)] // GRCOV_STOP_COVERAGE +// PANIC SAFETY unit tests +#![allow(clippy::panic)] +// PANIC SAFETY unit tests +#![allow(clippy::indexing_slicing)] use serde_json::json; use std::str::FromStr; diff --git a/cedar-policy-validator/src/typecheck/test_type_annotation.rs b/cedar-policy-validator/src/typecheck/test_type_annotation.rs index 59ff1119b..6fda6d1c8 100644 --- a/cedar-policy-validator/src/typecheck/test_type_annotation.rs +++ b/cedar-policy-validator/src/typecheck/test_type_annotation.rs @@ -15,7 +15,10 @@ */ #![cfg(test)] - +// PANIC SAFETY unit tests +#![allow(clippy::panic)] +// PANIC SAFETY unit tests +#![allow(clippy::indexing_slicing)] use std::collections::HashSet; use cedar_policy_core::ast::{EntityUID, Expr, ExprBuilder}; diff --git a/cedar-policy-validator/src/typecheck/test_utils.rs b/cedar-policy-validator/src/typecheck/test_utils.rs index 995abf8ea..f5ed05ff7 100644 --- a/cedar-policy-validator/src/typecheck/test_utils.rs +++ b/cedar-policy-validator/src/typecheck/test_utils.rs @@ -19,6 +19,10 @@ #![cfg(test)] // GRCOV_STOP_COVERAGE +// PANIC SAFETY unit tests +#![allow(clippy::panic)] +// PANIC SAFETY unit tests +#![allow(clippy::indexing_slicing)] use std::{collections::HashSet, sync::Arc}; use cedar_policy_core::ast::{EntityType, EntityUID, Expr, ExprShapeOnly, StaticPolicy, Template}; diff --git a/cedar-policy-validator/src/types.rs b/cedar-policy-validator/src/types.rs index 23202a494..4e4a45232 100644 --- a/cedar-policy-validator/src/types.rs +++ b/cedar-policy-validator/src/types.rs @@ -1231,6 +1231,10 @@ impl<'a> Effect<'a> { } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] +// PANIC SAFETY unit tests +#[allow(clippy::indexing_slicing)] #[cfg(test)] mod test { use std::collections::HashMap; diff --git a/cedar-policy/benches/cedar_benchmarks.rs b/cedar-policy/benches/cedar_benchmarks.rs index d7ec647d8..4f041f540 100644 --- a/cedar-policy/benches/cedar_benchmarks.rs +++ b/cedar-policy/benches/cedar_benchmarks.rs @@ -23,6 +23,8 @@ use cedar_policy::{ use criterion::{black_box, criterion_group, criterion_main, Criterion}; +// PANIC SAFETY unit tests +#[allow(clippy::unwrap_used)] pub fn criterion_benchmark(c: &mut Criterion) { let auth = Authorizer::new(); diff --git a/cedar-policy/src/api.rs b/cedar-policy/src/api.rs index fb7a8fea2..4b0584e27 100644 --- a/cedar-policy/src/api.rs +++ b/cedar-policy/src/api.rs @@ -80,6 +80,7 @@ impl From for ast::SlotId { } /// Entity datatype +// INVARIANT The `EntityUid` of an `Entity` cannot be unspecified #[repr(transparent)] #[derive(Debug, Clone, PartialEq, Eq, RefCast)] pub struct Entity(ast::Entity); @@ -113,6 +114,7 @@ impl Entity { ) -> Self { // note that we take a "parents" parameter here; we will compute TC when // the `Entities` object is created + // INVARIANT by invariant on `EntityUid` Self(ast::Entity::new( uid.0, attrs @@ -134,6 +136,7 @@ impl Entity { /// # assert_eq!(alice.attr("age"), None); /// ``` pub fn with_uid(uid: EntityUid) -> Self { + // INVARIANT: by invariant on `EntityUid` Self(ast::Entity::with_uid(uid.0)) } @@ -148,6 +151,7 @@ impl Entity { /// assert_eq!(alice.uid(), euid); /// ``` pub fn uid(&self) -> EntityUid { + // INVARIANT: By invariant on self and `EntityUid`: Our Uid can't be unspecified EntityUid(self.0.uid()) } @@ -366,6 +370,7 @@ impl Entities { Dereference::Residual(_) | Dereference::NoSuchEntity => None, Dereference::Data(e) => Some(e), }?; + // Invariant: No way to write down the unspecified EntityUid, so no way to have ancestors that are unspecified Some(entity.ancestors().map(EntityUid::ref_cast)) } @@ -1338,6 +1343,7 @@ impl std::fmt::Display for EntityNamespace { } /// Unique Id for an entity, such as `User::"alice"` +// INVARIANT: this can never be an `ast::EntityType::Unspecified` #[repr(transparent)] #[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, RefCast)] pub struct EntityUid(ast::EntityUID); @@ -1352,6 +1358,8 @@ impl EntityUid { /// assert_eq!(euid.type_name(), &EntityTypeName::from_str("User").unwrap()); /// ``` pub fn type_name(&self) -> &EntityTypeName { + // PANIC SAFETY by invariant on struct + #[allow(clippy::panic)] match self.0.entity_type() { ast::EntityType::Unspecified => panic!("Impossible to have an unspecified entity"), ast::EntityType::Concrete(name) => EntityTypeName::ref_cast(name), @@ -1382,6 +1390,7 @@ impl EntityUid { /// /// ``` pub fn from_type_name_and_id(name: EntityTypeName, id: EntityId) -> Self { + // INVARIANT: `from_components` always constructs a Concrete id Self(ast::EntityUID::from_components(name.0, id.0)) } @@ -1396,6 +1405,7 @@ impl EntityUid { /// ``` pub fn from_json(json: serde_json::Value) -> Result { let parsed: entities::EntityUidJSON = serde_json::from_value(json)?; + // INVARIANT: There is no way to write down the unspecified entityuid Ok::(Self( parsed.into_euid(|| JsonDeserializationErrorContext::EntityUid)?, )) @@ -1438,6 +1448,7 @@ impl FromStr for EntityUid { /// __DO NOT__ create [`EntityUid`]'s via string concatenation. /// If you have separate components of an [`EntityUid`], use [`EntityUid::from_type_name_and_id`] fn from_str(uid_str: &str) -> Result { + // INVARIANT there is no way to write down the unspecified entity ast::EntityUID::from_normalized_str(uid_str).map(EntityUid) } } @@ -2076,6 +2087,7 @@ impl Policy { /// Get the head constraint on this policy's action pub fn action_constraint(&self) -> ActionConstraint { // Clone the data from Core to be consistant with the other constraints + // INVARIANT: all of the EntityUids come from a policy, which must have Concrete EntityUids match self.ast.template().action_constraint() { ast::ActionConstraint::Any => ActionConstraint::Any, ast::ActionConstraint::In(ids) => ActionConstraint::In( @@ -2114,6 +2126,7 @@ impl Policy { slot: ast::SlotId, ) -> &'a EntityUid { match r { + // INVARIANT: this comes from policy source, so must be concrete ast::EntityReference::EUID(euid) => EntityUid::ref_cast(euid), // PANIC SAFETY: This `unwrap` here is safe due the invariant (values total map) on policies. #[allow(clippy::unwrap_used)] @@ -2585,6 +2598,7 @@ impl Request { pub fn principal(&self) -> Option<&EntityUid> { match self.0.principal() { ast::EntityUIDEntry::Concrete(euid) => match euid.entity_type() { + // INVARIANT: we ensure Concrete-ness here ast::EntityType::Concrete(_) => Some(EntityUid::ref_cast(euid.as_ref())), ast::EntityType::Unspecified => None, }, @@ -2598,6 +2612,7 @@ impl Request { pub fn action(&self) -> Option<&EntityUid> { match self.0.action() { ast::EntityUIDEntry::Concrete(euid) => match euid.entity_type() { + // INVARIANT: we ensure Concrete-ness here ast::EntityType::Concrete(_) => Some(EntityUid::ref_cast(euid.as_ref())), ast::EntityType::Unspecified => None, }, @@ -2611,6 +2626,7 @@ impl Request { pub fn resource(&self) -> Option<&EntityUid> { match self.0.resource() { ast::EntityUIDEntry::Concrete(euid) => match euid.entity_type() { + // INVARIANT: we ensure Concrete-ness here ast::EntityType::Concrete(_) => Some(EntityUid::ref_cast(euid.as_ref())), ast::EntityType::Unspecified => None, }, @@ -3042,6 +3058,8 @@ mod partial_eval_test { } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] #[cfg(test)] mod entity_uid_tests { use super::*; @@ -3434,6 +3452,8 @@ mod head_constraints_tests { } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] /// Tests in this module are adapted from Core's `policy_set.rs` tests #[cfg(test)] mod policy_set_tests { @@ -3786,6 +3806,8 @@ mod ancestors_tests { /// the Validator and Core packages working together. /// /// (Core has similar tests, but using a stubbed implementation of Schema.) +// PANIC SAFETY unit tests +#[allow(clippy::panic)] #[cfg(test)] mod schema_based_parsing_tests { use std::assert_eq; diff --git a/cedar-policy/src/frontend/is_authorized.rs b/cedar-policy/src/frontend/is_authorized.rs index bbc684166..836092c9a 100644 --- a/cedar-policy/src/frontend/is_authorized.rs +++ b/cedar-policy/src/frontend/is_authorized.rs @@ -323,6 +323,8 @@ fn parse_policy_set_from_individual_policies( } } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] #[cfg(test)] mod test { use super::*; diff --git a/cedar-policy/src/frontend/validate.rs b/cedar-policy/src/frontend/validate.rs index 05002a60f..a286a6ae8 100644 --- a/cedar-policy/src/frontend/validate.rs +++ b/cedar-policy/src/frontend/validate.rs @@ -141,6 +141,8 @@ enum ValidateAnswer { Success { notes: Vec }, } +// PANIC SAFETY unit tests +#[allow(clippy::panic)] #[cfg(test)] mod test { use super::*; diff --git a/cedar-policy/src/integration_testing.rs b/cedar-policy/src/integration_testing.rs index 8fbab7d6e..eb354c13a 100644 --- a/cedar-policy/src/integration_testing.rs +++ b/cedar-policy/src/integration_testing.rs @@ -176,6 +176,8 @@ pub trait CustomCedarImpl { /// # Panics /// When integration test data cannot be found #[allow(clippy::too_many_lines)] +// PANIC SAFETY this is testing code +#[allow(clippy::panic)] pub fn perform_integration_test_from_json_custom( jsonfile: impl AsRef, custom_impl_opt: Option<&dyn CustomCedarImpl>, diff --git a/cedar-policy/tests/corpus_tests.rs b/cedar-policy/tests/corpus_tests.rs index c8b1c463e..37610fd9b 100644 --- a/cedar-policy/tests/corpus_tests.rs +++ b/cedar-policy/tests/corpus_tests.rs @@ -35,6 +35,8 @@ fn folder() -> &'static Path { Path::new("corpus_tests") } +// PANIC SAFETY: Corpus Tests +#[allow(clippy::panic)] // for now we have a single #[test] that runs all the corpus tests. // The disadvantage of this is that only one error message will be displayed, // even if many of the corpus tests fail. diff --git a/panic_safety.sh b/panic_safety.sh index b2bc05ccd..04620fb3a 100755 --- a/panic_safety.sh +++ b/panic_safety.sh @@ -23,7 +23,7 @@ total_panics=0 failed=0 crates=($(cargo metadata --no-deps --format-version 1 | jq -r '.packages | map(.name) | join(" ")')) -panic_markers=("unwrap_used expect_used fallible_impl_from unreachable indexing_slicing") +panic_markers=("panic unwrap_used expect_used fallible_impl_from unreachable indexing_slicing") for crate in ${crates[@]}; do crate_panics=0