Fix opaque AttributeError on malformed CloudFormation templates (#2307)

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,18 +665,40 @@ 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:
             return self.template_dict
 
         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)
 

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).")

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):