Skip to content
17 changes: 11 additions & 6 deletions samtranslator/swagger/swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ def method_definition_has_integration(self, method_definition):
:param dict method_definition: method definition dictionary
:return: True if an integration exists
"""
if not isinstance(method_definition, dict):
raise InvalidDocumentException(
[InvalidTemplateException("Invalid swagger API operation definition: {}".format(method_definition))]
)
if method_definition.get(self._X_APIGW_INTEGRATION):
return True
return False
Expand Down Expand Up @@ -523,13 +527,14 @@ def set_path_default_authorizer(
:param bool add_default_auth_to_preflight: Bool of whether to add the default
authorizer to OPTIONS preflight requests.
"""

valid_non_http_method_sections = ["parameters", "summary", "description", "$ref", "servers"]
for method_name, method in self.get_path(path).items():
normalized_method_name = self._normalize_method_name(method_name)

# Excluding parameters section
if normalized_method_name == "parameters":
# Excluding non-http-method sections https://swagger.io/specification/
if method_name in valid_non_http_method_sections:
continue

if add_default_auth_to_preflight or normalized_method_name != "options":
normalized_method_name = self._normalize_method_name(method_name)
# It is possible that the method could have two definitions in a Fn::If block.
Expand Down Expand Up @@ -622,14 +627,14 @@ def set_path_default_apikey_required(self, path):
:param string path: Path name
"""

valid_non_http_method_sections = ["parameters", "summary", "description", "$ref", "servers"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to match the OpenAPI 3.0. Looking at references, this class seems to be specific to Rest APIs. Rest APIs support both OpenAPI 2.0 and 3.0: https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-export-api.html

I think we may need to think deeper about this and the recent revert: #2021 as a swagger validation overhaul.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a block-list while #2021 was an allow-list. I mean, we know as a fact that ["parameters", "summary", "description", "$ref", "servers"] are not API operations (HTTP methods)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe asking this a different way. If a customer is using OpenAPI 2.0 and specifies "servers" in the path object, should SAM fail with this being invalid? I know this just ignores the values and moves on, but not sure where we should fail this.

If SAM doesn't fail in the case above, then API Gateway should reject it. Which could be enough, just poking at this to see if we really need or want to do these deeper level of validation or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you asking whether we should validate that the API definition complies with the OpenApi specification or not?
If so, I think this is not the responsibility of SAM. Here, we are trying to do our best to raise 4xx error instead of 5xx when applicable. So, if the API definition is invalid but it doesn't cause SAM to fail, I believe it shouldn't be a concern for SAM. Agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed this was in part validation we do.

Your reasoning makes sense to me.

for method_name, method in self.get_path(path).items():
# Excluding parameters section
if method_name == "parameters":
# Excluding non-http-method sections https://swagger.io/specification/
if method_name in valid_non_http_method_sections:
continue

# It is possible that the method could have two definitions in a Fn::If block.
for method_definition in self.get_method_contents(method):

# If no integration given, then we don't need to process this definition (could be AWS::NoValue)
if not self.method_definition_has_integration(method_definition):
continue
Expand Down
62 changes: 61 additions & 1 deletion tests/swagger/test_swagger.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import copy

from unittest import TestCase
from mock import Mock
from mock import Mock, patch
from parameterized import parameterized, param

from samtranslator.swagger.swagger import SwaggerEditor
Expand Down Expand Up @@ -994,6 +994,66 @@ def test_add_default_apikey_to_all_paths_correctly_handles_method_level_settings
self.assertEqual(api_key_exists, self.editor.swagger["paths"][path]["apikeytrue"]["security"])
self.assertEqual(api_key_exists, self.editor.swagger["paths"][path]["apikeydefault"]["security"])

@patch("samtranslator.swagger.swagger.SwaggerEditor.get_method_contents")
def test_set_path_default_apikey_required_ignores_valid_non_http_method_properties(self, get_method_contents_mock):
get_method_contents_mock.return_value = []
self.original_swagger = {
"swagger": "2.0",
"paths": {
"/foo": {
"parameters": "ANY_VALUE",
"summary": "ANY_VALUE",
"description": "ANY_VALUE",
"$ref": "ANY_VALUE",
"servers": "ANY_VALUE",
"apikeyfalse": {_X_INTEGRATION: {"a": "b"}, "security": [{"api_key_false": []}]},
"apikeytrue": {_X_INTEGRATION: {"a": "b"}, "security": [{"api_key": []}]},
"apikeydefault": {_X_INTEGRATION: {"a": "b"}},
}
},
}

self.editor = SwaggerEditor(self.original_swagger)
self.editor.set_path_default_apikey_required("/foo")
# Assert not called for parameters, summary, description and $ref sections
self.assertEqual(get_method_contents_mock.call_count, 3)
get_method_contents_mock.has_call(self.original_swagger["paths"]["/foo"]["apikeyfalse"])
get_method_contents_mock.has_call(self.original_swagger["paths"]["/foo"]["apikeytrue"])
get_method_contents_mock.has_call(self.original_swagger["paths"]["/foo"]["apikeydefault"])

def test_method_definition_has_integration(self):
self.original_swagger = {
"swagger": "2.0",
"paths": {
"/foo": {
"get": {
"summary": "Valid Swagger API operation with amazon integration",
"requestBody": {},
_X_INTEGRATION: {"a": "b"},
"security": [{"api_key_false": []}],
"responses": {},
"parameters": [],
},
"post": {
"summary": "Valid Swagger API operation without amazon integration",
_X_INTEGRATION: {},
"requestBody": {},
"responses": {},
"parameters": [],
},
"put": "Invalid Value; String instead of swagger API operation",
}
},
}
self.editor = SwaggerEditor(self.original_swagger)
get_method_definition = self.original_swagger["paths"]["/foo"]["get"]
post_method_definition = self.original_swagger["paths"]["/foo"]["post"]
put_method_definition = self.original_swagger["paths"]["/foo"]["put"]
self.assertTrue(self.editor.method_definition_has_integration(get_method_definition))
self.assertFalse(self.editor.method_definition_has_integration(post_method_definition))
with self.assertRaises(InvalidDocumentException):
self.assertTrue(self.editor.method_definition_has_integration(put_method_definition))

def test_set_method_apikey_handling_apikeyrequired_false(self):
expected = [{"api_key_false": []}]
path = "/bar"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
Resources:
ApiWithInvalidPathMethod:
Type: AWS::Serverless::Api
Properties:
Auth:
ApiKeyRequired: true
UsagePlan:
CreateUP: NONE
StageName: Prod
Cors: "'*'"
DefinitionBody:
swagger: 2.0
paths:
/foo:
get: "Any non dict value"
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Structure of the SAM template is invalid. Invalid swagger API operation definition: Any non dict value"
}
1 change: 1 addition & 0 deletions tests/translator/test_translator.py
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,7 @@ def test_transform_success_no_side_effect(self, testcase, partition_with_region)
"error_function_with_event_dest_type",
"error_function_with_api_key_false",
"error_api_with_usage_plan_invalid_parameter",
"error_api_with_usage_plan_invalid_path_method",
"error_http_api_with_cors_def_uri",
"error_http_api_invalid_lambda_auth",
"error_api_mtls_configuration_invalid_field",
Expand Down