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

DDOS Vulnerability Fix - Secure Facebook Webhook #555

Merged
merged 8 commits into from
Dec 20, 2016

Conversation

jonchurch
Copy link
Contributor

@jonchurch jonchurch commented Dec 14, 2016

Proposed Changes: add express middleware to webhook POST route to validate incoming requests. Validate the X-HUB signature sent in headers against your Facebook App Secret. Throw an error to abort parsing requests that failed validation. The error is handled by an error handler which responds with a safe error to unvalidated requests. Relies on node's crypto lib.

Configurable by passing validate_requests: true, app_secret: your_app_secret when creating your controller object.

When in production, this prevents an attacker from forcing your application to process huge arrays of requests or impersonate FB/users. FB batches its messages, and so a malicious server could make requests stuffed full of data that your bot would then iterate over, potentially locking the process.

@benbrown
Copy link
Contributor

@jonchurch this looks great, would love to merge but for a few small tidbits:

  1. instead of using the process.env object to get at the secret, can you make that a new key in the controller's config object?

  2. if in the example app you want to default it to true -- i think this is fine -- we should also make sure it has the appropriate value for its secret and error and exit the process if not.

@jonchurch
Copy link
Contributor Author

@benbrown Fixed things up!

Facebook_bot.js requires app_secret now.

In addition to your changes, I switched fb_app_secret to app_secret to match convention so far.

@benbrown benbrown merged commit ceaf8eb into howdyai:master Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants