Skip to content
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

fix: webhook validation when float weight field is present (closes #467) #471

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

Justintime50
Copy link
Member

@Justintime50 Justintime50 commented Aug 13, 2024

Description

When the weight field is present on a webhook (eg: tracking update), it gets converted but JS drops the decimal if .0 which often is the case. This breaks webhook validation because HMAC signatures won't match if it's altered at all. This properly puts back the .0 if necessary and missing to ensure that validation will continue to work. Tests validate that we properly compare the signatures

Testing

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • 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 not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

@Justintime50 Justintime50 requested a review from a team August 13, 2024 22:23
nwithan8
nwithan8 previously approved these changes Aug 13, 2024
@footcarts
Copy link

Hi,

I just noticed that this regex is missing an edge case. When I initially wrote it, I only saw examples of weight with values of null and whole numbers formatted with .0. After running the regex for a while, I discovered that weight can also have other decimal values, like 614.4. In this case, the old regex does not ignore these values and incorrectly inserts .0, resulting in 614.4 being transformed into 61.04.4. I have now updated the regex to support these cases.

.replace(/("weight":\s*)(\d+)(\s*)(?=,|\})/g,"$1$2.0")

We have been running this updated version and have had 0 webhook validation errors.

jchen293
jchen293 previously approved these changes Aug 14, 2024
@Justintime50
Copy link
Member Author

Fantastic, thanks for the updated info! We've incorporated that.

@Justintime50 Justintime50 merged commit 21d5f90 into master Aug 16, 2024
14 checks passed
@Justintime50 Justintime50 deleted the fix_webhook_validation_floats branch August 16, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants