Generic timestamp request

kkocel commented 2 years ago

Description

Instead of using a full request just created an essential request class that has fields that are only needed.

Motivation and Context

When an unknown request type comes to the validator it fails because it cannot be deserialized. By introducing GenericTimestampRequest this problem is avoided. For validation, a timestamp is only needed and GenericTimestampRequest provides only that information.

Testing

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] My code follows the code style of this project
[ ] My change requires a change to the documentation
[ ] I have updated the documentation accordingly
[ ] I have read the README document
[ ] I have added tests to cover my changes
[x] All new and existing tests passed

License

[x] By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

doiron commented 2 years ago

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

kkocel commented 2 years ago

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

doiron commented 2 years ago

After syncing with the team we decided not to go ahead with this solution as it does validate that the request is in-fact a RequestEnvelope. This add extra layers of protection and will deny requests that have extra ( potentially malicious ) json parameters.

doiron commented 2 years ago

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

In this case, you would need to have the updated SDK that adds the Beta feature support. Otherwise the skill should not be accepting the request.

kkocel commented 2 years ago

@doiron

After syncing with the team we decided not to go ahead with this solution as it does validate that the request is in-fact a RequestEnvelope. This add extra layers of protection and will deny requests that have extra ( potentially malicious ) json parameters.

There are two types of vaildators in the SDK - one that validates request signature and the one that validates the timestamp. Both are used to validate the request. So there is no possibility to pass a request that is malicious (unless there is a request signed by Amazon that is malicious).

Even if the request has extra json parameters they will be discarded by Jackson deserialization - because Jackson deserializes json into a well defined Java object.

Having this knowledge maybe team can reconsider?

kkocel commented 2 years ago

Could you elaborate more about cases where the incoming Alexa Request cannot be Deserialized?

@doiron We are participating in private beta and we deal with requests that are not present in model package

In this case, you would need to have the updated SDK that adds the Beta feature support. Otherwise the skill should not be accepting the request.

Ok, so I would like to get the thin jar containing model package only for the beta we are participating in. We do not want uber/fat jar since dealing with it is maintainability nightmare. We will pass this request through our solution architect as well.

doiron commented 2 years ago

There are two types of validators in the SDK - one that validates request signature and the one that validates the timestamp. Both are used to validate the request. So there is no possibility to pass a request that is malicious (unless there is a request signed by Amazon that is malicious).

Even if the request has extra json parameters they will be discarded by Jackson deserialization - because Jackson deserializes json into a well defined Java object.

Having this knowledge maybe team can reconsider?

Furthermore, this change would need to:

Be updated so that the changes are backward compatible, or bump up the major version with a breaking change, forcing consumers to upgrade.
1. It's possible we could just introduce a new Constructor instead of replacing the RequestEnvelope object in the ServletRequest class. This will allow Skills that solely rely on the Verifiers to still function as is and pull in the latest bits. Some consumers have their own implementation/equivalent to the SkillServlet class.
2. potentially a new backward compatible implementation of the AlexaHttpRequest interface. This could require some extra conversion logic from RequestEnvelope to GenericTimestampRequestEnvelope. ??
Update the Python SDK as well.
For our node SDK, it doesn't perform any deserialization so most likely no work required for that package.

Since this would take a while for us to prioritize and sort out, I've compiled a jar file to unblock you in the interim, let us know how that works out.

thanks again for working on this.

kkocel commented 2 years ago

I left an issue for this: #326

alexa / alexa-skills-kit-sdk-for-java