fix(hmac): stop dropping notification fields whose value is the string "0" - #934
fix(hmac): stop dropping notification fields whose value is the string "0"#934jaideeppyne wants to merge 2 commits into
Conversation
…g "0" getNotificationDataToSign used empty() to default each field, so a field holding the string "0" was signed as an empty string. A webhook with merchantReference "0" therefore failed validation against Adyen's own signature. The value field already used isset for this reason; apply the same treatment to the remaining fields.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the HMAC signature generation in HmacSignature.php to use the null coalescing operator (??) instead of empty() checks, preventing values like '0' from being incorrectly treated as empty. It also adds comprehensive unit tests to verify this behavior. The feedback suggests making the test helper method notificationWithField more robust by handling the nested 'value' field under 'amount' alongside 'currency'.
|
Good catch, done in the follow-up commit. It's a bit worse than a future-proofing issue too. The old helper would have set I left |
|
1 participant
Description
getNotificationDataToSign()defaults every field withempty(). In PHPempty("0")is true, so a signed field holding the string"0"goes into the signing string as"". Adyen computes its signature over the real value, so a legitimate webhook fails validation. The realistic one to hit ismerchantReferenceof"0", but the same holds forpspReference,originalReference,merchantAccountCode,amount.currency,eventCodeandsuccess.amount.valuewas already moved toissetfor exactly this reason, and the comment on that line says so. I applied the same treatment to the remaining seven fields using??.The signed field list is the documented one,
pspReference:originalReference:merchantAccountCode:merchantReference:value:currency:eventCode:success, and only the defaulting of those values changes here. Nothing about separators or escaping is touched.Tested scenarios
The signing string is one contract with several implementations, so I used the sibling SDKs as the oracle. For each of the 6 cleanly comparable fields I built the same notification item and compared signatures from adyen-python, adyen-java, adyen-go and adyen-node. All four agree with each other on all 6 cases, and this library was the only one that disagreed, on all 6. After the fix it matches all four.
I also ran the 4 real Adyen-signed fixtures that ship in adyen-python-api-library (
test/mocks/util/*_notification.json, the ones carrying colons and backslashes inmerchantReference) throughisValidNotificationHMAChere. All 4 still validate, so values Adyen signs today are unaffected.New tests are
testMerchantReferenceZeroIsNotTreatedAsEmptyand a 6 case data provider asserting that"0"and""do not collapse to the same signature. Reverting onlyHmacSignature.phpand keeping the new tests gives 7 failures; with the fix the unit suite is 295/295 green, up from 288.Not covered:
successonly ever carries"true"or"false"from Adyen and the Java model types it as a bool, so I could not derive a cross SDK constant for it. It is defaulted consistently in the fix but has no pinned test vector.I used Claude Code to build the cross SDK harness and draft the tests. I ran and checked every signature and test result myself.
Fixed issue: n/a