From f2e280cdcfe6ea09ca9c2144e34f497550610373 Mon Sep 17 00:00:00 2001 From: saiasish Date: Sat, 2 May 2026 22:32:03 -0700 Subject: [PATCH] Fix opaque AttributeError on malformed CloudFormation templates The cloudformation package command crashed with errors like 'NoneType object has no attribute items' or '... has no attribute get' when a template was malformed (typically inconsistent indentation that caused a section or resource body to parse as null), instead of telling the user where the problem was. Issue #2307. Add an InvalidTemplateError that carries a clear message, and guard every site in artifact_exporter.Template that previously dereferenced a section or resource body without checking it. Eight surfaces are now covered: top-level template body, Metadata, Resources, individual resource bodies, the Resources mapping shape, the resource-group shape in export_resources, the Fn::Transform handler, and its Parameters sub-section. Add nine regression tests covering the empty-template, null-Resources, null-resource-body, wrong-shape, and Fn::Transform null cases. Fixes #2307 Signed-off-by: saiasish --- .../cloudformation/artifact_exporter.py | 57 ++++++++- .../cloudformation/exceptions.py | 6 + .../cloudformation/test_artifact_exporter.py | 112 ++++++++++++++++++ 3 files changed, 170 insertions(+), 5 deletions(-) diff --git a/awscli/customizations/cloudformation/artifact_exporter.py b/awscli/customizations/cloudformation/artifact_exporter.py index 7fba8dddbba3..28ec2a9ecb23 100644 --- a/awscli/customizations/cloudformation/artifact_exporter.py +++ b/awscli/customizations/cloudformation/artifact_exporter.py @@ -547,10 +547,14 @@ class CodeCommitRepositoryS3Resource(ResourceWithS3UrlDict): def include_transform_export_handler(template_dict, uploader, parent_dir): - if template_dict.get("Name", None) != "AWS::Include": + if not isinstance(template_dict, dict) \ + or template_dict.get("Name", None) != "AWS::Include": return template_dict - include_location = template_dict.get("Parameters", {}).get("Location", None) + parameters = template_dict.get("Parameters", {}) or {} + if not isinstance(parameters, dict): + return template_dict + include_location = parameters.get("Location", None) if not include_location \ or not is_path_value_valid(include_location) \ or is_s3_url(include_location): @@ -611,6 +615,8 @@ def export_global_artifacts(self, template_dict): here we iterate through the template dict and export params with a handler defined in GLOBAL_EXPORT_DICT """ + if not isinstance(template_dict, dict): + return template_dict for key, val in template_dict.items(): if key in GLOBAL_EXPORT_DICT: template_dict[key] = GLOBAL_EXPORT_DICT[key](val, self.uploader, self.template_dir) @@ -630,10 +636,18 @@ def export_metadata(self, template_dict): :return: The template with references to artifacts that have been exported to s3. """ - if "Metadata" not in template_dict: + if not isinstance(template_dict, dict) \ + or "Metadata" not in template_dict: + return template_dict + + metadata = template_dict["Metadata"] + if metadata is None: return template_dict + if not isinstance(metadata, dict): + raise exceptions.InvalidTemplateError( + message='"Metadata" section must be a mapping') - for metadata_type, metadata_dict in template_dict["Metadata"].items(): + for metadata_type, metadata_dict in metadata.items(): for exporter_class in self.metadata_to_export: if exporter_class.RESOURCE_TYPE != metadata_type: continue @@ -651,6 +665,13 @@ def export(self): :return: The template with references to artifacts that have been exported to s3. """ + if self.template_dict is None: + raise exceptions.InvalidTemplateError( + message="template body is empty") + if not isinstance(self.template_dict, dict): + raise exceptions.InvalidTemplateError( + message="top-level template body must be a mapping") + self.template_dict = self.export_metadata(self.template_dict) if "Resources" not in self.template_dict: @@ -658,11 +679,26 @@ def export(self): self.template_dict = self.export_global_artifacts(self.template_dict) - self.export_resources(self.template_dict["Resources"]) + resources = self.template_dict["Resources"] + if resources is None: + raise exceptions.InvalidTemplateError( + message='"Resources" section is empty; check ' + "that resource entries are indented under " + '"Resources:"') + if not isinstance(resources, dict): + raise exceptions.InvalidTemplateError( + message='"Resources" section must be a mapping of ' + "resource ids to resource definitions") + + self.export_resources(resources) return self.template_dict def export_resources(self, resource_dict): + if not isinstance(resource_dict, dict): + raise exceptions.InvalidTemplateError( + message="resource group must be a mapping of " + "resource ids to resource definitions") for resource_id, resource in resource_dict.items(): if resource_id.startswith("Fn::ForEach::"): @@ -671,6 +707,17 @@ def export_resources(self, resource_dict): self.export_resources(resource[2]) continue + if resource is None: + raise exceptions.InvalidTemplateError( + message='resource "{0}" has no body; check ' + 'indentation under the resource id' + .format(resource_id)) + if not isinstance(resource, dict): + raise exceptions.InvalidTemplateError( + message='resource "{0}" must be a mapping with ' + '"Type" and "Properties" keys' + .format(resource_id)) + resource_type = resource.get("Type", None) resource_dict = resource.get("Properties", None) diff --git a/awscli/customizations/cloudformation/exceptions.py b/awscli/customizations/cloudformation/exceptions.py index b2625cdd27f9..77ddbd576605 100644 --- a/awscli/customizations/cloudformation/exceptions.py +++ b/awscli/customizations/cloudformation/exceptions.py @@ -57,3 +57,9 @@ class DeployBucketRequiredError(CloudFormationCommandError): class InvalidForEachIntrinsicFunctionError(CloudFormationCommandError): fmt = 'The value of {resource_id} has an invalid "Fn::ForEach::" format: Must be a list of three entries' + + +class InvalidTemplateError(CloudFormationCommandError): + fmt = ("Cannot parse CloudFormation template: {message}. " + "The template may be malformed (for example, inconsistent " + "indentation or a missing value).") diff --git a/tests/unit/customizations/cloudformation/test_artifact_exporter.py b/tests/unit/customizations/cloudformation/test_artifact_exporter.py index 9fce853eb78f..9c51d67f7f88 100644 --- a/tests/unit/customizations/cloudformation/test_artifact_exporter.py +++ b/tests/unit/customizations/cloudformation/test_artifact_exporter.py @@ -1170,6 +1170,118 @@ def test_template_export_foreach_invalid(self, yaml_parse_mock): with self.assertRaises(exceptions.InvalidForEachIntrinsicFunctionError): template_exporter.export() + def _build_template_exporter(self, yaml_parse_mock, template_dict, + resources_to_export=None): + parent_dir = os.path.abspath(os.path.sep) + template_dir = os.path.join(parent_dir, 'foo', 'bar') + template_path = os.path.join(template_dir, 'path') + template_str = self.example_yaml_template() + + open_mock = mock.mock_open() + yaml_parse_mock.return_value = template_dict + + kwargs = {} + if resources_to_export is not None: + kwargs["resources_to_export"] = resources_to_export + with mock.patch( + "awscli.customizations.cloudformation.artifact_exporter.open", + open_mock(read_data=template_str)): + return Template( + template_path, parent_dir, self.s3_uploader_mock, **kwargs) + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_template_is_none(self, yaml_parse_mock): + # Regression: empty template file (yaml_parse returns None) used to + # raise an opaque AttributeError in export_metadata / export. + # See https://github.com/aws/aws-cli/issues/2307 + exporter = self._build_template_exporter(yaml_parse_mock, None) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + "template body is empty"): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_resources_is_none(self, yaml_parse_mock): + # Regression for the exact stack trace in issue #2307: misindented + # resource entries cause `Resources:` to parse as None, which then + # raised "AttributeError: 'NoneType' object has no attribute 'items'". + exporter = self._build_template_exporter( + yaml_parse_mock, {"Resources": None}) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + '"Resources" section is empty'): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_resources_is_list(self, yaml_parse_mock): + exporter = self._build_template_exporter( + yaml_parse_mock, {"Resources": ["bad"]}) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + '"Resources" section must be a mapping'): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_resource_body_is_none(self, yaml_parse_mock): + # Regression: a resource declared with an empty body (e.g. `MyFn:` + # with nothing under it) used to raise + # "AttributeError: 'NoneType' object has no attribute 'get'". + exporter = self._build_template_exporter( + yaml_parse_mock, {"Resources": {"MyFn": None}}) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + 'resource "MyFn" has no body'): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_resource_body_is_string(self, yaml_parse_mock): + exporter = self._build_template_exporter( + yaml_parse_mock, {"Resources": {"MyFn": "not-a-mapping"}}) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + 'resource "MyFn" must be a mapping'): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_raises_when_metadata_is_not_mapping(self, yaml_parse_mock): + exporter = self._build_template_exporter( + yaml_parse_mock, + {"Metadata": ["bad"], "Resources": {}}) + with self.assertRaisesRegex(exceptions.InvalidTemplateError, + '"Metadata" section must be a mapping'): + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_allows_null_metadata(self, yaml_parse_mock): + # `Metadata:` with nothing under it should be tolerated, just like + # an absent section, since CFN treats it as no metadata at all. + exporter = self._build_template_exporter( + yaml_parse_mock, {"Metadata": None, "Resources": {}}) + # Should not raise. + exporter.export() + + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") + def test_template_export_allows_empty_resources_mapping(self, yaml_parse_mock): + # An empty `Resources: {}` is structurally valid; nothing to export. + exporter = self._build_template_exporter( + yaml_parse_mock, {"Resources": {}}) + result = exporter.export() + self.assertEqual(result, {"Resources": {}}) + + def test_include_transform_handler_tolerates_non_dict(self): + # The handler is called recursively over arbitrary template fragments; + # passing a non-dict (e.g. a stray scalar) must not crash with + # AttributeError when it tries to call `.get("Name")`. + result = include_transform_export_handler( + None, self.s3_uploader_mock, "/tmp") + self.assertIsNone(result) + result = include_transform_export_handler( + "scalar", self.s3_uploader_mock, "/tmp") + self.assertEqual(result, "scalar") + + def test_include_transform_handler_tolerates_null_parameters(self): + # `Fn::Transform` with `Parameters:` left empty used to raise + # AttributeError on `.get("Location")`. + template_dict = {"Name": "AWS::Include", "Parameters": None} + result = include_transform_export_handler( + template_dict, self.s3_uploader_mock, "/tmp") + self.assertEqual(result, template_dict) + @mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse") def test_template_global_export(self, yaml_parse_mock):