Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Securing FB Graph API Requests #1170

Merged
merged 3 commits into from
Jan 18, 2018

Conversation

ouadie-lahdioui
Copy link
Collaborator

Hello,

Almost every Graph API call requires an access token. Malicious developers can steal access tokens and use them to send spam from your bot with malicious software on a person's computer or a man in the middle attack.

To prevent that, Facebook recommend sending an app secret proof parameter to every API call.

This PR add a new configuration attribute require_appsecret_proof to enable sending the sha256 proof each call Botkit makes.

var controller = Botkit.facebookbot({
    debug: true,
    log: true,
    access_token: process.env.page_token,
    verify_token: process.env.verify_token,
    app_secret: process.env.app_secret,
    require_appsecret_proof: true
});

Enjoy ✌️

Copy link
Contributor

@HassenIO HassenIO left a comment

Choose a reason for hiding this comment

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

Good job 💪 @ouadie-lahdioui,
I just have minor suggestions, all related to readability.
One more thing: Shouldn't be some tests there? At least for the new getAppSecretProof method, and in case the app secret is required but not provided by the bot.
Keep on the good work 👍


```javascript
var controller = Botkit.facebookbot({
debug: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

debug and log are mandatory options.

Just for clarity purpose, I suggest not mentioning them in your example and just show required options (like tokens + app_secret) + the new app secret proof.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

lib/Facebook.js Outdated
@@ -851,6 +909,11 @@ function Facebookbot(configuration) {
return 'sha1=' + hmac.digest('hex');
}

function getAppSecretProof(dataToHash, key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing the big picture, but is this method intended to be used somewhere else?

If not, I suggest to use more explicit parameter names, like:

function getAppSecretProof(access_token, app_secret) {
    var hmac = crypto.createHmac('sha256', app_secret);
    return hmac.update(access_token).digest('hex');
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

totally agree 👍

Remove package-lock.json generated by npm5+
@ouadie-lahdioui
Copy link
Collaborator Author

Thanks @htaidirt for the review.

  • Yes, it should be a lot of tests there, unfortunately not all functionalities are tested. fork the project and help adding some cool tests.
  • In case the app secret is not provided Botkit will encrypt using an empty key.

@benbrown benbrown merged commit a566325 into howdyai:master Jan 18, 2018
@ouadie-lahdioui ouadie-lahdioui deleted the feature/AppSecretProof branch January 19, 2018 10:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants