fix: Prevent infinite recursion when resolving policy parameter when the parameter refs a CFN parameter with the same name

samtranslator/intrinsics/resolver.py

@@ -93,12 +93,12 @@ def resolve_sam_resource_id_refs(self, input, supported_resource_id_refs):  # ty
         """
         return self._traverse(input, supported_resource_id_refs, self._try_resolve_sam_resource_id_refs)  # type: ignore[no-untyped-call]
 
-    def _traverse(self, input, resolution_data, resolver_method):  # type: ignore[no-untyped-def]
+    def _traverse(self, input_value, resolution_data, resolver_method):  # type: ignore[no-untyped-def]
         """
         Driver method that performs the actual traversal of input and calls the appropriate `resolver_method` when
         to perform the resolution.
 
-        :param input: Any primitive type  (dict, array, string etc) whose value might contain an intrinsic function
+        :param input_value: Any primitive type  (dict, array, string etc) whose value might contain an intrinsic function
         :param resolution_data: Data that will help with resolution. For example, when resolving parameter references,
             this object will contain a dictionary of parameter names and their values.
         :param resolver_method: Method that will be called to actually resolve an intrinsic function. This method
@@ -108,7 +108,7 @@ def _traverse(self, input, resolution_data, resolver_method):  # type: ignore[no
 
         # There is data to help with resolution. Skip the traversal altogether
         if len(resolution_data) == 0:
-            return input
+            return input_value
 
         #
         # Traversal Algorithm:
@@ -126,15 +126,14 @@ def _traverse(self, input, resolution_data, resolver_method):  # type: ignore[no
         # to handle nested intrinsics. All of these cases lend well towards a Pre-Order traversal where we try and
         # process the intrinsic, which results in a modified sub-tree to traverse.
         #
-
-        input = resolver_method(input, resolution_data)
-
-        if isinstance(input, dict):
-            return self._traverse_dict(input, resolution_data, resolver_method)  # type: ignore[no-untyped-call]
-        if isinstance(input, list):
-            return self._traverse_list(input, resolution_data, resolver_method)  # type: ignore[no-untyped-call]
+        input_value = resolver_method(input_value, resolution_data)
+        if isinstance(input_value, dict):
+            return self._traverse_dict(input_value, resolution_data, resolver_method)  # type: ignore[no-untyped-call]
+        if isinstance(input_value, list):
+            return self._traverse_list(input_value, resolution_data, resolver_method)  # type: ignore[no-untyped-call]
         # We can iterate only over dict or list types. Primitive types are terminals
-        return input
+
+        return input_value
 
     def _traverse_dict(self, input_dict, resolution_data, resolver_method):  # type: ignore[no-untyped-def]
         """

samtranslator/policy_template_processor/template.py

@@ -1,10 +1,13 @@
-import copy
+from typing import Any
 
 from samtranslator.intrinsics.resolver import IntrinsicsResolver
 from samtranslator.intrinsics.actions import RefAction
 from samtranslator.policy_template_processor.exceptions import InsufficientParameterValues, InvalidParameterValues
 
 
+POLICY_PARAMETER_DISAMBIGUATE_PREFIX = "___SAM_POLICY_PARAMETER_"
+
+
 class Template(object):
     """
     Class representing a single policy template. It includes the name, parameters and template dictionary.
@@ -50,17 +53,53 @@ def to_statement(self, parameter_values):  # type: ignore[no-untyped-def]
         # injection of values for parameters not intended in the template. This is important because "Ref" resolution
         # will substitute any references for which a value is provided.
         necessary_parameter_values = {
-            name: value for name, value in parameter_values.items() if name in self.parameters
+            POLICY_PARAMETER_DISAMBIGUATE_PREFIX + name: value
+            for name, value in parameter_values.items()
+            if name in self.parameters
         }
 
         # Only "Ref" is supported
         supported_intrinsics = {RefAction.intrinsic_name: RefAction()}
 
         resolver = IntrinsicsResolver(necessary_parameter_values, supported_intrinsics)  # type: ignore[no-untyped-call]
-        definition_copy = copy.deepcopy(self.definition)
+        definition_copy = self._disambiguate_policy_parameter(self.definition)
 
         return resolver.resolve_parameter_refs(definition_copy)
 
+    @staticmethod
+    def _disambiguate_policy_parameter(policy_definition: Any) -> Any:
+        """
+        Return a deepcopy of policy definition where all parameters are
+        renamed to avoid naming collision of normal CFN parameters.
+        This is to avoid IntrinsicResolver.resolve_parameter_refs()
+        will make infinitely recursion on this:
+        ```
+        - DynamoDBCrudPolicy:
+          TableName:  <- this is the policy parameter
+            Fn::ImportValue:
+              Fn::Join:
+              - '-'
+              - - Ref: TableName <- this is the CFN parameter
+                - hello
+                - Ref: EnvironmentType
+        ```
+        Once IntrinsicResolver.resolve_parameter_refs() replace the "Ref: TableName"
+        with "TableName: .... Ref: TableName - hello ---"
+        There are "Ref: TableName" in it again (indefinitely).
+        """
+
+        def _traverse(node: Any) -> Any:
+            if isinstance(node, dict):
+                copy = {key: _traverse(value) for key, value in node.items()}
+                if "Ref" in copy and isinstance(copy["Ref"], str):
+                    copy["Ref"] = POLICY_PARAMETER_DISAMBIGUATE_PREFIX + copy["Ref"]
+                return copy
+            if isinstance(node, list):
+                return [_traverse(item) for item in node]
+            return node
+
+        return _traverse(policy_definition)
+
     def missing_parameter_values(self, parameter_values):  # type: ignore[no-untyped-def]
         """
         Checks if the given input contains values for all parameters used by this template

tests/policy_template_processor/test_template.py

@@ -128,7 +128,7 @@ def test_to_statement_must_work_with_valid_inputs(self, intrinsics_resolver_mock
         result = template.to_statement(parameter_values)
 
         self.assertEqual(expected, result)
-        intrinsics_resolver_mock.assert_called_once_with(parameter_values, {"Ref": ANY})
+        intrinsics_resolver_mock.assert_called_once_with({"___SAM_POLICY_PARAMETER_param1": "b"}, {"Ref": ANY})
         resolver_instance_mock.resolve_parameter_refs.assert_called_once_with({"Statement": {"key": "value"}})
 
     @patch("samtranslator.policy_template_processor.template.IntrinsicsResolver")
@@ -145,7 +145,7 @@ def test_to_statement_must_exclude_extra_parameter_values(self, intrinsics_resol
         template.to_statement(parameter_values)
 
         # Intrinsics resolver must be called only with the parameters declared in the template
-        expected_parameter_values = {"param1": "b"}
+        expected_parameter_values = {"___SAM_POLICY_PARAMETER_param1": "b"}
         intrinsics_resolver_mock.assert_called_once_with(expected_parameter_values, ANY)
 
     @patch("samtranslator.policy_template_processor.template.IntrinsicsResolver")

tests/translator/input/policy_template_import_vaule_refs_parameters_with_same_name_as_policy_parameter.yaml

@@ -0,0 +1,19 @@
+Parameters:
+  TableName:
+    Type: String
+
+Resources:
+  MapFunction:
+    Type: AWS::Serverless::Function
+    Properties:
+      Handler: index.handler
+      Runtime: nodejs16.x
+      CodeUri: s3://bucket/key
+      Policies:
+      - DynamoDBCrudPolicy:
+          TableName:
+            Fn::ImportValue:
+              Fn::Join:
+              - '-'
+              - - Ref: TableName # this is the same as DynamoDBCrudPolicy's parameter name
+                - hello

tests/translator/output/aws-cn/policy_template_import_vaule_refs_parameters_with_same_name_as_policy_parameter.json

@@ -0,0 +1,128 @@
+{
+  "Parameters": {
+    "TableName": {
+      "Type": "String"
+    }
+  },
+  "Resources": {
+    "MapFunction": {
+      "Properties": {
+        "Code": {
+          "S3Bucket": "bucket",
+          "S3Key": "key"
+        },
+        "Handler": "index.handler",
+        "Role": {
+          "Fn::GetAtt": [
+            "MapFunctionRole",
+            "Arn"
+          ]
+        },
+        "Runtime": "nodejs16.x",
+        "Tags": [
+          {
+            "Key": "lambda:createdBy",
+            "Value": "SAM"
+          }
+        ]
+      },
+      "Type": "AWS::Lambda::Function"
+    },
+    "MapFunctionRole": {
+      "Properties": {
+        "AssumeRolePolicyDocument": {
+          "Statement": [
+            {
+              "Action": [
+                "sts:AssumeRole"
+              ],
+              "Effect": "Allow",
+              "Principal": {
+                "Service": [
+                  "lambda.amazonaws.com"
+                ]
+              }
+            }
+          ],
+          "Version": "2012-10-17"
+        },
+        "ManagedPolicyArns": [
+          "arn:aws-cn:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
+        ],
+        "Policies": [
+          {
+            "PolicyDocument": {
+              "Statement": [
+                {
+                  "Action": [
+                    "dynamodb:GetItem",
+                    "dynamodb:DeleteItem",
+                    "dynamodb:PutItem",
+                    "dynamodb:Scan",
+                    "dynamodb:Query",
+                    "dynamodb:UpdateItem",
+                    "dynamodb:BatchWriteItem",
+                    "dynamodb:BatchGetItem",
+                    "dynamodb:DescribeTable",
+                    "dynamodb:ConditionCheckItem"
+                  ],
+                  "Effect": "Allow",
+                  "Resource": [
+                    {
+                      "Fn::Sub": [
+                        "arn:${AWS::Partition}:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${tableName}",
+                        {
+                          "tableName": {
+                            "Fn::ImportValue": {
+                              "Fn::Join": [
+                                "-",
+                                [
+                                  {
+                                    "Ref": "TableName"
+                                  },
+                                  "hello"
+                                ]
+                              ]
+                            }
+                          }
+                        }
+                      ]
+                    },
+                    {
+                      "Fn::Sub": [
+                        "arn:${AWS::Partition}:dynamodb:${AWS::Region}:${AWS::AccountId}:table/${tableName}/index/*",
+                        {
+                          "tableName": {
+                            "Fn::ImportValue": {
+                              "Fn::Join": [
+                                "-",
+                                [
+                                  {
+                                    "Ref": "TableName"
+                                  },
+                                  "hello"
+                                ]
+                              ]
+                            }
+                          }
+                        }
+                      ]
+                    }
+                  ]
+                }
+              ]
+            },
+            "PolicyName": "MapFunctionRolePolicy0"
+          }
+        ],
+        "Tags": [
+          {
+            "Key": "lambda:createdBy",
+            "Value": "SAM"
+          }
+        ]
+      },
+      "Type": "AWS::IAM::Role"
+    }
+  }
+}