fix: surface user-facing errors for malformed Fn::If on Role and CodeUri#3919
Conversation
573c535 to
a32bc5d
Compare
a32bc5d to
6ea76e3
Compare
aws-sam-cli-bot
left a comment
There was a problem hiding this comment.
Code Review Results
Reviewed: b4aabe1..6ea76e3
Files: 9 (2 schema JSON files omitted due to size — generated files)
Comments: 1
Comments on lines outside the diff:
[samtranslator/model/sam_resources.py:629] [BUG] The same unguarded Fn::If unpack pattern that this PR fixes in _make_lambda_role also exists in _get_or_make_condition. A malformed Fn::If on a destination property (e.g. OnSuccess / OnFailure for event invoke config) with fewer than 3 elements will crash with an IndexError on dest_list[1] / dest_list[2], producing the same opaque "Internal transform failure" message this PR aims to eliminate.
Since the PR already imports and uses validate_intrinsic_if_items, applying the same guard here would be a minimal addition:
if is_intrinsic_if(destination):
dest_list = destination.get("Fn::If")
try:
validate_intrinsic_if_items(dest_list)
except ValueError as e:
raise InvalidResourceException(
logical_id,
f"Malformed destination 'Fn::If': {e!s}. "
"Expected form: Fn::If: [Condition, TrueValue, FalseValue].",
) from e
if is_intrinsic_no_value(dest_list[1]) and is_intrinsic_no_value(dest_list[2]):This is the same class of bug being fixed — consider addressing it in this PR to avoid a separate follow-up.
Note: The following files were omitted due to size: samtranslator/schema/schema.json, schema_source/cloudformation.schema.json (generated schema files).
6ea76e3 to
d75aaaf
Compare
60a3d02 to
9fead93
Compare
…deUri
Three customer templates were crashing the SAM transform with unhandled
exceptions that CloudFormation surfaced as the opaque message
"Transform AWS::Serverless-2016-10-31 failed with: Internal transform
failure.", leaving the customer with no indication of which resource or
property was wrong.
1) AWS::Serverless::Function.Role supports Fn::If so a user can switch
between an existing IAM role and a SAM-generated one. _make_lambda_role
went straight from `is_intrinsic_if(role)` to a 3-tuple unpack:
role_condition, role_if, role_else = role_resolved_value.get("Fn::If")
A malformed 2- or 4-element list therefore raised
ValueError: not enough values to unpack (expected 3, got 2)
Wrap with validate_intrinsic_if_items (already used in
resource_policies.py and preferences/deployment_preference_collection.py)
and raise InvalidResourceException(self.logical_id, ...).
2) EventInvokeConfig.DestinationConfig.{OnSuccess,OnFailure}.Destination
also supports Fn::If. _get_or_make_condition indexed dest_list[1] /
dest_list[2] without arity validation, so a 2-element list raised
IndexError: list index out of range
Same guard using validate_intrinsic_if_items is applied here.
3) parse_s3_uri calls urllib.parse.urlparse, which has validated
bracketed-host segments against the IPv6 grammar since the
CVE-2024-11168 fix (Python 3.9.17+/3.10.12+/3.11.4+/3.12/3.13/3.14).
Unresolved CDK tokens like "s3://[TOKEN.25]/key" now raise:
ValueError: "TOKEN.25" does not appear to be an IPv4 or IPv6 address
Catch ValueError and return None so construct_s3_location_object raises
the existing friendly "CodeUri is not a valid S3 Uri..."
InvalidResourceException with the logical id.
Tests:
- tests/translator/input/error_function_role_fn_if_malformed.{yaml,json}
- tests/translator/input/error_function_codeuri_unresolved_token.{yaml,json}
- tests/translator/input/error_function_event_destination_fn_if_malformed.{yaml,json}
(auto-discovered by test_transform_invalid_document)
- tests/model/s3_utils/test_uri_parser.py: parse_s3_uri returns None on
malformed bracketed netloc; construct_s3_location_object raises
InvalidResourceException with the resource logical id
- tests/model/test_sam_resources.py: _make_lambda_role raises
InvalidResourceException on too-few and too-many Fn::If items
Full suite: 4503 passed, 0 failed.
9fead93 to
698aefd
Compare
Issue #, if available
N/A
Description of changes
Three malformed-template inputs crashed the SAM transform with unhandled
exceptions, which CloudFormation surfaced as:
The customer had no way to know which resource or property was wrong.
1. Malformed
Fn::IfonAWS::Serverless::Function.Role.Fn::Ifmust have exactly 3 elements, but
_make_lambda_roleunpacked directlywithout arity validation, so a 2- or 4-element list raised
ValueError: not enough values to unpack (expected 3, got 2).Now validates via the existing
validate_intrinsic_if_itemshelper andraises
InvalidResourceExceptionwith the logical id.2. Malformed
Fn::Ifon event-invoke destinations.EventInvokeConfig.DestinationConfig.{OnSuccess,OnFailure}.Destinationalso supports
Fn::If, and_get_or_make_conditionindexeddest_list[1]/dest_list[2]without arity validation. A 2-elementlist raised
IndexError: list index out of range. Same guard is nowapplied here.
3. Malformed
CodeUriURIs. Since the CVE-2024-11168 fix (Python3.9.17+/3.10.12+/3.11.4+/3.12/3.13/3.14),
urllib.parse.urlparsevalidates bracketed-host segments against the IPv6/IPv4 grammars. CDK
sometimes synthesizes unresolved
IResolvabletokens likes3://[TOKEN.25]/keyinto template strings, which now raisesValueErrorfrom insideurlparse.parse_s3_urinow catches that andreturns
None, soconstruct_s3_location_objectraises the existingfriendly
InvalidResourceException.After the fix, users see the same class of message SAM emits for other
malformed properties:
Description of how you validated changes
test_transform_invalid_document):tests/translator/input/error_function_role_fn_if_malformed.{yaml,json},tests/translator/input/error_function_event_destination_fn_if_malformed.{yaml,json},tests/translator/input/error_function_codeuri_unresolved_token.{yaml,json}.parse_s3_uri/construct_s3_location_objectin
tests/model/s3_utils/test_uri_parser.py.Fn::Ifargs) totests/model/test_sam_resources.py::TestSamFunctionRoleResolver.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.