From 1241fc78fe5184d2fabef49d9dade20366ebe6fc Mon Sep 17 00:00:00 2001 From: Satyaki Ghosh Date: Thu, 15 Oct 2026 04:45:25 -0400 Subject: [PATCH 1/2] Fix I3013 retention checks for dynamic and conditional properties --- src/cel-engine/src/rules/best_practices.rs | 19 +-- .../tests/resource_rule_parity.rs | 145 ++++++++++++++++++ src/rego-engine/src/builtins.rs | 10 +- src/rego-engine/src/engine.rs | 20 +++ .../expected/validation_reports5.json | 21 +-- src/template-model/src/model.rs | 25 ++- 6 files changed, 193 insertions(+), 47 deletions(-) diff --git a/src/cel-engine/src/rules/best_practices.rs b/src/cel-engine/src/rules/best_practices.rs index bd3370cd..4ef1eb43 100644 --- a/src/cel-engine/src/rules/best_practices.rs +++ b/src/cel-engine/src/rules/best_practices.rs @@ -638,22 +638,9 @@ fn eval_retention_period_rules(ctx: &EvalContext) -> Vec { // anchored on the properties object, so report only the first missing // one - emitting one per missing property would over-report a single // underlying concern. - let first_missing = required_props.iter().find(|prop| { - let key_present = m - .resources - .get(resource_name.as_str()) - .map(|r| r.properties.contains_key(prop.as_str())) - .unwrap_or(false); - // The property is only "set" if it resolves to a real value in - // every satisfiable scenario. A value supplied through - // `Fn::If [cond, X, AWS::NoValue]` is absent in the NoValue - // branch, so the retention period can still lapse - the reference - // linter flags that, and treating mere key presence as "set" would - // miss it. - let path = format!("Properties.{}", prop); - let scenarios = m.resolve_scenarios_json(resource_name.as_str(), &path); - !key_present || scenarios.is_empty() || scenarios.iter().any(|(val, _)| val.is_null()) - }); + let first_missing = required_props + .iter() + .find(|property| !m.property_absence_scenarios(resource_name, property).is_empty()); if let Some(prop) = first_missing { out.push(make_resource_diagnostic("I3013", &format!("'{}' is a required property (The default retention period will delete the data after a pre-defined time. Set an explicit values to avoid data loss on resource)", prop), diff --git a/src/cfn-validate/tests/resource_rule_parity.rs b/src/cfn-validate/tests/resource_rule_parity.rs index e62d9420..82205974 100644 --- a/src/cfn-validate/tests/resource_rule_parity.rs +++ b/src/cfn-validate/tests/resource_rule_parity.rs @@ -797,6 +797,151 @@ Resources: assert!(rego_findings.iter().any(|finding| finding.contains("'UpdateReplacePolicy'"))); } +#[test] +fn dynamic_properties_do_not_imply_missing_retention_period() { + let template = r#" +Parameters: + Stage: + Type: String + RetentionDays: + Type: Number +Conditions: + IsProduction: !Equals [!Ref Stage, production] + IsNotProduction: !Not [!Condition IsProduction] +Resources: + DynamicPropertiesQueue: + Type: AWS::SQS::Queue + Properties: + Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-properties.yaml + SiblingTransformQueue: + Type: AWS::SQS::Queue + Properties: + QueueName: example-queue + Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-properties.yaml + ResourceTransformQueue: + Type: AWS::SQS::Queue + Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-resource.yaml + NoValuePropertiesQueue: + Type: AWS::SQS::Queue + Properties: !Ref AWS::NoValue + NestedTransformQueue: + Type: AWS::SQS::Queue + Properties: + Tags: + Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-tags.yaml + DynamicRetentionQueue: + Type: AWS::SQS::Queue + Properties: + MessageRetentionPeriod: !Ref RetentionDays + ConditionalSetQueue: + Type: AWS::SQS::Queue + Properties: !If + - IsProduction + - MessageRetentionPeriod: 1209600 + - MessageRetentionPeriod: 60 + ConditionalMacroOrSetQueue: + Type: AWS::SQS::Queue + Properties: !If + - IsProduction + - Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/production-queue-properties.yaml + - MessageRetentionPeriod: 60 + ConditionalMacroOnlyQueue: + Type: AWS::SQS::Queue + Properties: !If + - IsProduction + - Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/production-queue-properties.yaml + - Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/development-queue-properties.yaml + ConditionalRetentionQueue: + Type: AWS::SQS::Queue + Properties: + MessageRetentionPeriod: !If [IsProduction, 1209600, !Ref AWS::NoValue] + ResourceConditionAllowsAbsenceQueue: + Type: AWS::SQS::Queue + Condition: IsProduction + Properties: + MessageRetentionPeriod: !If [IsProduction, !Ref AWS::NoValue, 60] + ResourceConditionExcludesAbsenceQueue: + Type: AWS::SQS::Queue + Condition: IsProduction + Properties: + MessageRetentionPeriod: !If [IsProduction, 60, !Ref AWS::NoValue] + CorrelatedConditionsExcludeAbsenceQueue: + Type: AWS::SQS::Queue + Properties: + MessageRetentionPeriod: !If + - IsProduction + - 1209600 + - !If [IsNotProduction, 60, !Ref AWS::NoValue] + ConditionalTransformQueue: + Type: AWS::SQS::Queue + Properties: !If + - IsProduction + - Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-properties.yaml + - VisibilityTimeout: 900 + ConcreteQueue: + Type: AWS::SQS::Queue + Properties: + QueueName: concrete-queue +"#; + let (rego, cel) = engines(); + let rego_findings = selected_findings(®o, template, &["I3013"]); + let cel_findings = selected_findings(&cel, template, &["I3013"]); + + assert_eq!(rego_findings, cel_findings); + assert_eq!( + rego_findings.len(), + 5, + "only known reachable missing retention periods are reported: {rego_findings:?}" + ); + for resource in [ + "ConcreteQueue", + "ConditionalRetentionQueue", + "ConditionalTransformQueue", + "NestedTransformQueue", + "ResourceConditionAllowsAbsenceQueue", + ] { + assert!(rego_findings.iter().any(|finding| finding.contains(&format!("|{resource}|Properties|")))); + } + for resource in [ + "ConditionalMacroOnlyQueue", + "ConditionalMacroOrSetQueue", + "ConditionalSetQueue", + "CorrelatedConditionsExcludeAbsenceQueue", + "DynamicPropertiesQueue", + "DynamicRetentionQueue", + "NoValuePropertiesQueue", + "ResourceConditionExcludesAbsenceQueue", + "ResourceTransformQueue", + "SiblingTransformQueue", + ] { + assert!(rego_findings.iter().all(|finding| !finding.contains(&format!("|{resource}|")))); + } +} + #[test] fn omitted_dynamodb_billing_mode_requires_provisioned_throughput() { let template = r#" diff --git a/src/rego-engine/src/builtins.rs b/src/rego-engine/src/builtins.rs index 58697d0f..a3ab5007 100644 --- a/src/rego-engine/src/builtins.rs +++ b/src/rego-engine/src/builtins.rs @@ -1784,15 +1784,11 @@ fn register_property_can_be_absent(rego: &mut regorus::Engine) { }; let rid = params[0].as_string()?; let path = params[1].as_string()?; - let prop = path.strip_prefix("Properties.").unwrap_or(path.as_ref()); - let key_present = - model.resources.get(rid.as_ref()).map(|r| r.properties.contains_key(prop)).unwrap_or(false); - if !key_present { + let property = path.strip_prefix("Properties.").unwrap_or(path.as_ref()); + if !model.resources.contains_key(rid.as_ref()) { return Ok(Value::from(true)); } - let scenarios = model.resolve_scenarios_json_shared(rid.as_ref(), path.as_ref()); - let absent = scenarios.is_empty() || scenarios.iter().any(|(v, _)| v.is_null()); - Ok(Value::from(absent)) + Ok(Value::from(!model.property_absence_scenarios(rid.as_ref(), property).is_empty())) }), ); } diff --git a/src/rego-engine/src/engine.rs b/src/rego-engine/src/engine.rs index 880e42a9..fe9eb277 100644 --- a/src/rego-engine/src/engine.rs +++ b/src/rego-engine/src/engine.rs @@ -990,6 +990,7 @@ Resources: Type: AWS::SQS::Queue Properties: QueueName: my-queue + MessageRetentionPeriod: !Ref Port Transform: AWS::Serverless-2016-10-31 "#; @@ -1230,6 +1231,25 @@ violation contains v if { assert_eq!(diags.len(), 1, "BucketName is static"); } + #[test] + fn builtin_property_can_be_absent_uses_known_absence() { + let diags = eval_builtin_policy( + r#" +package builtin_test +import rego.v1 +violation contains v if { + property_can_be_absent("MyQueue", "Properties.RedrivePolicy") + not property_can_be_absent("MyBucket", "Properties.BucketName") + not property_can_be_absent("MyQueue", "Properties.MessageRetentionPeriod") + property_can_be_absent("MissingQueue", "Properties.MessageRetentionPeriod") + v := {"rule_id": "B_PROPERTY_ABSENCE", "severity": "error", "message": "ok", "resource_id": "MyQueue"} +} +"#, + "B_PROPERTY_ABSENCE", + ); + assert_eq!(diags.len(), 1, "only known property absence should satisfy the builtin"); + } + #[test] fn builtin_resources_of_type() { let diags = eval_builtin_policy( diff --git a/src/resources/expected/validation_reports5.json b/src/resources/expected/validation_reports5.json index b73c4911..c3e7e8bf 100644 --- a/src/resources/expected/validation_reports5.json +++ b/src/resources/expected/validation_reports5.json @@ -28891,7 +28891,7 @@ "fatal": 0, "errors": 0, "warnings": 0, - "informational": 55, + "informational": 54, "debug": 0 }, "suppressed": 0, @@ -29187,25 +29187,6 @@ "ruleDescription": "Check stateful resources have a set UpdateReplacePolicy/DeletionPolicy", "phase": "LINT" }, - { - "ruleId": "I3013", - "severity": "INFO", - "message": "'MessageRetentionPeriod' is a required property (The default retention period will delete the data after a pre-defined time. Set an explicit values to avoid data loss on resource)", - "source": "CFN_LINT", - "entity": { - "logicalId": "DynamicQueue", - "entityType": "Resource", - "resourceType": "AWS::SQS::Queue" - }, - "propertyPath": "Properties", - "category": "Best Practice", - "startLine": 17, - "startColumn": 5, - "endLine": 17, - "endColumn": 15, - "ruleDescription": "Check resources with auto expiring content have explicit retention period", - "phase": "LINT" - }, { "ruleId": "I3013", "severity": "INFO", diff --git a/src/template-model/src/model.rs b/src/template-model/src/model.rs index 5676cf2c..85416d3a 100644 --- a/src/template-model/src/model.rs +++ b/src/template-model/src/model.rs @@ -2293,10 +2293,12 @@ impl SemanticModel { /// The condition assignments under which the top-level property /// `property_name` is absent at deployment: undeclared, or resolving to - /// `AWS::NoValue`. Only the conditions that decide the property's own - /// presence appear, never conditions nested inside a present value, so the - /// result stays linear in the property's `Fn::If` depth. Empty when the - /// property is always present or the resource's properties are dynamic. + /// `AWS::NoValue`. A macro-owned branch is unknown rather than absent, and + /// an explicitly authored dynamic value remains present. Only the conditions + /// that decide the property's own presence appear, never conditions nested + /// inside a present value, so the result stays linear in the property's + /// `Fn::If` depth. Empty when the property is always present or the resource's + /// properties are otherwise dynamic. #[must_use] pub fn property_absence_scenarios(&self, resource_id: &str, property_name: &str) -> Vec> { let Some(properties) = self.resolved_properties_value(resource_id) else { @@ -2304,6 +2306,20 @@ impl SemanticModel { }; let mut absences = Vec::new(); collect_property_absences(&properties, property_name, &mut Vec::new(), &mut absences); + absences.retain(|conditions| { + let mut assumptions: Vec<(String, bool)> = + conditions.iter().map(|(condition, value)| (condition.clone(), *value)).collect(); + if let Some(resource_condition) = + self.resources.get(resource_id).and_then(|resource| resource.condition.as_ref()) + { + match conditions.get(resource_condition) { + Some(false) => return false, + Some(true) => {} + None => assumptions.push((resource_condition.clone(), true)), + } + } + assumptions.is_empty() || self.conditions.is_satisfiable(&assumptions) + }); absences } @@ -2790,6 +2806,7 @@ fn collect_property_absences( } } } + ResolvedValue::Map { entries } if entries.iter().any(|entry| entry.key == FN_TRANSFORM) => {} ResolvedValue::Map { entries } => match entries.iter().find(|entry| entry.key == property_name) { None => absences.push(assignment(literals)), Some(entry) => { From 2fe634e1f308fa561963ff145e33aca372c8bc3b Mon Sep 17 00:00:00 2001 From: Satyaki Ghosh Date: Fri, 16 Oct 2026 16:46:21 -0400 Subject: [PATCH 2/2] fixes --- src/CUSTOM_RULES.md | 2 +- .../tests/resource_rule_parity.rs | 42 +++++++++++++-- .../expected/validation_reports7.json | 53 ++++++++++++++++++- src/schema-validator/src/validate.rs | 29 ++++++++-- src/template-model/src/model.rs | 24 ++++++--- 5 files changed, 133 insertions(+), 17 deletions(-) diff --git a/src/CUSTOM_RULES.md b/src/CUSTOM_RULES.md index aed26ab3..2e0b35f3 100644 --- a/src/CUSTOM_RULES.md +++ b/src/CUSTOM_RULES.md @@ -300,7 +300,7 @@ condition assignment and should be emitted with `make_diag_conditional`. | `is_satisfiable` | `({condition_name: bool}) -> bool` | Whether the condition assignment is satisfiable; an empty assignment returns `true`. | | `unreachable_if_branches` | `(resource_id) -> [{resourceId, path, message}]` | Finds immediate `Fn::If` branches unreachable under surrounding conditions. `__output__` addresses an output. | | `has_property` | `(resource_id, property_name) -> bool` | Whether the resource authors the named property, whatever its value: a `Ref`, `Fn::GetAtt`, or `Fn::ImportValue` counts as present. Use `"BucketName"`, not `"Properties.BucketName"`. | -| `property_can_be_absent` | `(resource_id, path) -> bool` | Whether the property is missing or can resolve to null/`AWS::NoValue` in a scenario. | +| `property_can_be_absent` | `(resource_id, path) -> bool` | Whether a top-level property is absent in a reachable deployment scenario. Macro-owned property sets are unknown rather than absent. | ### Parameters and Template Metadata diff --git a/src/cfn-validate/tests/resource_rule_parity.rs b/src/cfn-validate/tests/resource_rule_parity.rs index 82205974..b74bd0eb 100644 --- a/src/cfn-validate/tests/resource_rule_parity.rs +++ b/src/cfn-validate/tests/resource_rule_parity.rs @@ -830,9 +830,6 @@ Resources: Name: AWS::Include Parameters: Location: s3://example-bucket/queue-resource.yaml - NoValuePropertiesQueue: - Type: AWS::SQS::Queue - Properties: !Ref AWS::NoValue NestedTransformQueue: Type: AWS::SQS::Queue Properties: @@ -886,6 +883,11 @@ Resources: Condition: IsProduction Properties: MessageRetentionPeriod: !If [IsProduction, 60, !Ref AWS::NoValue] + NegatedResourceConditionExcludesAbsenceQueue: + Type: AWS::SQS::Queue + Condition: IsNotProduction + Properties: + MessageRetentionPeriod: !If [IsProduction, !Ref AWS::NoValue, 60] CorrelatedConditionsExcludeAbsenceQueue: Type: AWS::SQS::Queue Properties: @@ -933,7 +935,7 @@ Resources: "CorrelatedConditionsExcludeAbsenceQueue", "DynamicPropertiesQueue", "DynamicRetentionQueue", - "NoValuePropertiesQueue", + "NegatedResourceConditionExcludesAbsenceQueue", "ResourceConditionExcludesAbsenceQueue", "ResourceTransformQueue", "SiblingTransformQueue", @@ -942,6 +944,38 @@ Resources: } } +#[test] +fn whole_properties_no_value_is_a_schema_type_error() { + let template = r#" +Resources: + Queue: + Type: AWS::SQS::Queue + Properties: !Ref AWS::NoValue + MacroOwnedQueue: + Type: AWS::SQS::Queue + Properties: !Ref AWS::NoValue + Fn::Transform: + Name: AWS::Include + Parameters: + Location: s3://example-bucket/queue-resource.yaml +"#; + let (rego, cel) = engines(); + let rego_findings = selected_findings(®o, template, &["F3012", "I3013"]); + let cel_findings = selected_findings(&cel, template, &["F3012", "I3013"]); + + assert_eq!(rego_findings, cel_findings); + assert_eq!(rego_findings.len(), 1, "the non-object Properties value must produce one finding: {rego_findings:?}"); + assert!( + rego_findings[0].contains("F3012|Fatal|Queue|Properties|{'Ref': 'AWS::NoValue'} is not of type 'object'"), + "the whole Properties value must be rejected without a missing-retention cascade: {rego_findings:?}" + ); + + let rego_anchors = selected_finding_anchors(®o, template, &["F3012"]); + let cel_anchors = selected_finding_anchors(&cel, template, &["F3012"]); + assert_eq!(rego_anchors, cel_anchors); + assert_eq!(rego_anchors, ["Queue|Properties|5:5"]); +} + #[test] fn omitted_dynamodb_billing_mode_requires_provisioned_throughput() { let template = r#" diff --git a/src/resources/expected/validation_reports7.json b/src/resources/expected/validation_reports7.json index 2784c370..34ba6a18 100644 --- a/src/resources/expected/validation_reports7.json +++ b/src/resources/expected/validation_reports7.json @@ -14985,7 +14985,7 @@ "metadata": { "resourcesScanned": 5, "counts": { - "fatal": 4, + "fatal": 6, "errors": 3, "warnings": 0, "informational": 2, @@ -15086,6 +15086,57 @@ "ruleDescription": "Required property missing", "phase": "SCHEMA" }, + { + "ruleId": "F3012", + "severity": "FATAL", + "message": "{'Ref': 'AWS::NoValue'} is not of type 'object'", + "source": "SCHEMA", + "entity": { + "logicalId": "IamRole2", + "entityType": "Resource", + "resourceType": "AWS::IAM::Role" + }, + "propertyPath": "Properties", + "category": "Schema", + "startLine": 26, + "startColumn": 5, + "endLine": 26, + "endColumn": 15, + "documentationUrl": "https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-iam.git", + "ruleDescription": "Property type mismatch", + "phase": "SCHEMA", + "context": { + "actualValue": { + "Ref": "AWS::NoValue" + }, + "expectedConstraint": "object" + } + }, + { + "ruleId": "F3012", + "severity": "FATAL", + "message": "{'Ref': 'AWS::NoValue'} is not of type 'object'", + "source": "SCHEMA", + "entity": { + "logicalId": "CloudFront1", + "entityType": "Resource", + "resourceType": "AWS::CloudFront::Distribution" + }, + "propertyPath": "Properties", + "category": "Schema", + "startLine": 39, + "startColumn": 5, + "endLine": 39, + "endColumn": 15, + "ruleDescription": "Property type mismatch", + "phase": "SCHEMA", + "context": { + "actualValue": { + "Ref": "AWS::NoValue" + }, + "expectedConstraint": "object" + } + }, { "ruleId": "E3024", "severity": "ERROR", diff --git a/src/schema-validator/src/validate.rs b/src/schema-validator/src/validate.rs index fcbd8616..509921cf 100644 --- a/src/schema-validator/src/validate.rs +++ b/src/schema-validator/src/validate.rs @@ -562,6 +562,28 @@ fn validate_resource( let base = KEY_PROPERTIES; let defs = &schema.definitions; + if m.is_authored_ref_at(rid, base, PSEUDO_NO_VALUE) { + let mut actual_value = serde_json::Map::new(); + actual_value.insert(FN_REF.to_string(), serde_json::Value::String(PSEUDO_NO_VALUE.to_string())); + let mut diagnostic = build_diagnostic( + "F3012", + &format!("{{'Ref': '{}'}} is not of type 'object'", PSEUDO_NO_VALUE), + m, + rid, + base, + None, + ); + diagnostic.context = Some(ViolationContext { + actual_value: Some(serde_json::Value::Object(actual_value).into()), + expected_constraint: Some("object".to_string()), + property: None, + lifecycle: None, + resolution_source: None, + extra: None, + }); + out.push(diagnostic); + } + // Lifecycle pointers (deprecated/create-only/write-only) address a specific // property, which for many resources is a nested sub-property (e.g. MediaConnect // Flow deprecates only `Source.Decryption.Url`, never the required `Source` @@ -632,10 +654,9 @@ fn validate_resource( } } - // When the whole Properties block is a deploy-time intrinsic (e.g. - // `Properties: !Ref AWS::NoValue`), the resolved view is empty and every - // required property would look missing. The effective properties are not - // known statically, so skip the key/required-property checks. + // A macro-owned Properties block has no statically complete key set. A + // direct AWS::NoValue reference is already reported above as a type error, + // so neither case should cascade into missing-required diagnostics. let key_scenarios = if res.properties_dynamic { Vec::new() } else { resource_property_key_scenarios(m, rid, res) }; for (actual_keys, conds) in &key_scenarios { let scenario = if conds.is_empty() { None } else { Some(conds) }; diff --git a/src/template-model/src/model.rs b/src/template-model/src/model.rs index 85416d3a..7872a8c1 100644 --- a/src/template-model/src/model.rs +++ b/src/template-model/src/model.rs @@ -124,9 +124,9 @@ pub struct ResolvedResource { pub metadata: Option, #[cfg_attr(feature = "wasm-bindings", tsify(type = "Record"))] pub properties: HashMap, - /// True when a resource-level macro or a non-map `Properties` intrinsic can - /// determine the effective property set only at deploy time, as distinct from - /// a resource that simply declares no properties. + /// True when a resource-level or property-set macro can determine the + /// effective property set only at deploy time, or when a non-map + /// `Properties` intrinsic has no statically inspectable property map. pub properties_dynamic: bool, pub diagnostics: ResourceDiagnostics, } @@ -2298,7 +2298,8 @@ impl SemanticModel { /// that decide the property's own presence appear, never conditions nested /// inside a present value, so the result stays linear in the property's /// `Fn::If` depth. Empty when the property is always present or the resource's - /// properties are otherwise dynamic. + /// properties are otherwise dynamic. Scenarios incompatible with the resource's + /// own condition or the template condition model are excluded. #[must_use] pub fn property_absence_scenarios(&self, resource_id: &str, property_name: &str) -> Vec> { let Some(properties) = self.resolved_properties_value(resource_id) else { @@ -3403,10 +3404,19 @@ fn resolve_resource(arena: &Arena, name: &str, node_ref: NodeRef, resolver: &mut // rather than doubling the `Fn::If` segment. resolver.set_current_path(KEY_PROPERTIES); properties.insert(synthetic_key, resolver.resolve_node(*props_ref)); + } else if matches!( + arena.node(*props_ref), + Node::Intrinsic(IntrinsicFn::Ref(target)) if target == PSEUDO_NO_VALUE + ) && !properties_dynamic + { + // Retain the known non-object value so schema validation can reject + // it without treating every schema property as missing. + resolver.set_current_path(KEY_PROPERTIES); + resolver.resolve_node(*props_ref); + properties_dynamic = true; } else if matches!(arena.node(*props_ref), Node::Intrinsic(_)) { - // The whole Properties block is an intrinsic (e.g. `!Ref AWS::NoValue`) - // that did not collapse into a per-branch synthetic key. Its effective - // properties are only known at deploy time. + // The intrinsic may produce the effective property map only at + // deployment, so static analysis cannot infer missing keys. properties_dynamic = true; } }