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

Added support for webhook authentication #820

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

txrp0x9
Copy link

@txrp0x9 txrp0x9 commented Oct 24, 2024

Adds a verification_token field to the crawl URL, used with the webhook field. It accepts a nonce string for the server to verify upon receiving webhooks, ensuring they originate from a legitimate source.

Adds HMAC-SHA256 verification on webhooks
If provided with a "secretKey" in the request body, the webhook response will now contain a header,
"Webhook-Signature": "sha256=<hash>"
The <hash> being a SHA256 hash with the secretKey on the raw request body string.

Example of verifying the request body with express.js
Make sure to use the body string before it is parsed to JSON (JSON.parse() or equivalent). Ideally in a middleware.

Fixes #813

@txrp0x9
Copy link
Author

txrp0x9 commented Oct 24, 2024

While this solution does mimic the version described in the #813, It can be computationally expensive.
A better solution might just be passing the secret in the response body, instead of hashing it (the secret can be a uuid)
Why? If the secret is leaked, the authentication is broken, so as long as the secret remains safe, that is, the client and server are the only ones who know it, hashing the request body may not be necessary
unless
the request body is susceptible to MITM attacks which can lead to the body being changed, but with HTTPS this should be less of a worry (not non-existent however)

Please let me know if I'm missing something.

We're now going with a simple verification_token method

@nickscamara
Copy link
Member

@rafaelsideguide can you review this when you get a chance? tks!

Copy link
Collaborator

@rafaelsideguide rafaelsideguide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the response still using snake case

@txrp0x9
Copy link
Author

txrp0x9 commented Oct 29, 2024

Renaming seems to have broken something, results have become unpredictable, I'll fix this in a moment.
@rafaelsideguide Could you pllease test this on your system and let me know if its fine

@Nurfen
Copy link

Nurfen commented Dec 9, 2024

Plus one for this update, we can't use webhooks without basic security features

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.

[Feat] Authenticate webhook
4 participants