-
Notifications
You must be signed in to change notification settings - Fork 47
PW-6762: Fix HMAC verification functions corrupt notification item payload #155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PW-6762: Fix HMAC verification functions corrupt notification item payload #155
Conversation
Thanks for this contribution @alainburindi. I believe this would break usages of those functions that for some reason are expecting the passed payload to change, right? |
Yes, in case there are any. In our use case, we don't want the payload to be changed because our ERP uses one endpoint for different accounts. We can add another parameter on the function that will be used to check whether the user wants the payloads to be changed or not. def is_valid_hmac(dict_object, hmac_key, keep_additional_data=False):
if keep_additional_data:
dict_object = dict_object.copy()
# ...rest @acampos1916 What do you think? with this, we won't break any existing usages that expect the payload to change |
I like that @alainburindi What do you think @AlexandrosMor? |
@alainburindi, Thank you for opening this pull request. yes indeed I prefer also that solution |
93541af
to
f730f63
Compare
@acampos1916 @AlexandrosMor I have done the changes accordingly |
Adyen/util.py
Outdated
def is_valid_hmac(dict_object, hmac_key, keep_additional_data=False): | ||
if keep_additional_data: | ||
dict_object = dict_object.copy() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def is_valid_hmac(dict_object, hmac_key, keep_additional_data=False): | |
if keep_additional_data: | |
dict_object = dict_object.copy() | |
def is_valid_hmac(dict_object, hmac_key): | |
dict_object = dict_object.copy() |
We decided to always keep the origin additional data. Could you please review it and add this suggestion ? @alainburindi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexandrosMor I just modified the changes accordingly. can you check them?
f730f63
to
d871836
Compare
Description
Do a copy of the payload to avoid changing the main notification payload
Tested scenarios
Fixed issue: #148