fix: validate unknown method for a path in swagger definition for apigw #2061
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wchengru
approved these changes
Jun 18, 2021
sriram-mv
reviewed
Jun 18, 2021
| normalized_method_name = self._normalize_method_name(method_name) | ||
| # It is possible that the method could have two definitions in a Fn::If block. | ||
| for method_definition in self.get_method_contents(self.get_path(path)[normalized_method_name]): | ||
| method_dict = self.get_path(path).get(normalized_method_name) |
There was a problem hiding this comment.
Some additional questions which may have already been considered.
can normalized_method_name be anything other than a string?
can path be anything other than a string?
Could we have intrinsics which resolve each of them to a dict instead of a string?
|
Thanks for the reviews everyone, I find out that Ahmed has an earlier PR which is addressing same issue in a different way. Since it is not polluting our code with extra validation and using what information we have there, I would suggest to move forward with that. You can check it here; #2031 Closing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
N/A
Description of changes:
When setting path for default authorizers, SAM Transform tries to access specific method information from path definition inside swagger in this line; https://github.com/aws/serverless-application-model/blob/develop/samtranslator/swagger/swagger.py#L534
Since it is trying to access this dictionary directly, it might raise exception when a method is defined in function's event definition but not in swagger definition.
Description of how you validated changes:
Before setting up path for default authorizer, we first try to see if that path has the expected method definition, if not we are going to raise an invalid document exception rather than getting item not found exception from dictionary.
Checklist:
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.