Delete outdated fb cookie

[pomartel]

When a signed_request parameter is present, the fb cookie should be removed. This way, if a user logs out of Facebook or unauthorize the app, the session will no longer be vaild.

[pomartel]

In fact, wait if you want to pull this change. I'm going to take a few days to test it good.

[pomartel]

To help test this change and get my pull request approved, I've setup two test environments both deployed on Heroku. They are minimal test apps with no database backend.

The first app uses Facebooker2 latest release (from git) and you can access it here : http://apps.facebook.com/facebooker-test/

The code is public on github : https://github.com/pomartel/facebooker2-test

The second app is identical except it uses my Facebooker2 fork : http://apps.facebook.com/facebooker-pomartel

The code is also public on github : https://github.com/pomartel/facebooker2-pomartel-test

Now here are the steps you can do to reproduce the bug :

1- Go to the canvas URL mentioned above 2- Authorize the app 3- You should now see the test page telling you you are authorized. This page uses a before_filter to make sure there is a logged in user 4- Now either disconnect from Facebook or remove the app from the Privacy Settings menu. 5- Navigate back to the canvas URL

With the current Facebooker2 release, you will still be able to see the page even though you are logged out or you have deauthorized the app. This is because the cookie has not been invalidated.

With my fork, Facebook should redirect you to a login page or the authorization page.

[rjlee]

So, I did a little looking into this and from this thread it appears to be "expected behaviour" :

http://forum.developers.facebook.net/viewtopic.php?pid=229062

The proposed solution only partially fixes the problem as it's still possible to use an app, as long as you don't refresh it in the browser (i.e. you remove the app in a separate browser window/tab). So it's doesn't completely ensure that the user won't be able to access the app unfortunately.

I'd suggest implementing the javascript SDK check suggested in the facebook thread above if it's a must have and you need to ensure that an app isn't accessible for users who have removed the app.

[pomartel]

Well don't you agree that if a valid signed_request is passed to the server, this should invalidate the cookie?

Right now if the signed_request says the app has not been authorized, the cookie will override it. That's a major bug if you ask me! If you use a facebook app from a public computer and log off facebook, the next guy sitting at that computer can still access the app with your Facebook credentials.

As you've mentionned, there is still the case where you navigate the iframe without reloading the parent facebook frame. In this case, no signed_request will be passed to the server and the cookie won't be invalidated. Maybe something should be done with JS but we should patch the server too.

I just don't see how that's not a major security issue right now.

[rjlee]

I think what I was saying was that this is the intended/advertised behaviour as described by facebook (as far as I can make out from the forum thread above). That being the case, there may be unintended effects to your change, or there may be people who have built against the expected behavour that this change could break. So whilst this may correct a problem that affects your codebase, it could adversely affect others who may be expecting it or have worked around it in some other way.

The original cookie setting code was basically taken straight from the PHP facebook library (it has had a few modifications since admittedly), but it aims to replicate the nearest thing to a reference implementation as published by Facebook (https://github.com/facebook/php-sdk/blob/master/src/facebook.php).

I think the important thing to understand is if this happens with the PHP facebook implementation, if it doesn't then we've clearly got something wrong and it needs fixing using the same method that the PHP implementation uses, If it it does happen with the PHP implementation, then that should be reported to the maintainers of the library and your fix may be a good way to work around the problem in facebooker2.

I'll try and setup the simple PHP app outlined here (https://github.com/facebook/php-sdk/blob/master/examples/example.php) to test it with (probably next week though) to see how that behaves. If you have the time to do that or can spot any errors in the facebooker2 implementation based on the PHP one then that would be handy.

[pomartel]

Ok, I think I'm approaching this problem the wrong way. Invalidating the cookie fixes it but that's not really the source of the problem. Here's what I think is the source of the problem on line 30 and 31 of controller.rb :

sig = fetch_client_and_user_from_signed_request
sig = fetch_client_and_user_from_cookie unless @_current_facebook_client

We first try to fetch the client and user from the signed_request. If this didn't set the @_current_facebook_client then we try to fetch the client and user from the cookie.

If a valid signed_request is passed to the server, how come we don't create a client? Here's what a typical uncrypted signed_request of an unlogged user looks like :

{"algorithm"=>"HMAC-SHA256", "issued_at"=>1300976861, "user"=>{"country"=>"ca", "locale"=>"fr_CA", "age"=>{"min"=>21}}}

There is no oauth_token passed in the signed_request. Couldn't we then just create a generic client with create_and_authenticate_as_application ? I might be getting this wrong but we shouldn't use the cookie if we have a fresh signed_request.

Mike, any thoughts on this?

[mmangino]

We create a client from the signed request only if it has an oauth token. If it doesn't, we don't create a client because we dont have a valid client from the user. If the app wants to have one, it can create a client.

My gut feeling is that this is an inherent problem in the way facebook handles logout and is their design decision. I think the JS solution is the right one. That said, I can see removing the existing cookie if we get a logged out request. I don't like the implementation however. I think it needs to be more explicit and needs to see if the signed request is for a not logged in user.

I would instead do something like

    sig = fetch_client_and_user_from_signed_request
    sig = fetch_client_and_user_from_cookie if @_current_facebook_client.nil? and !signed_request_from_logged_out_user

Mike

[pomartel]

Mike, I'm not sure you and Rob completely understand the side effect of this cookie implementation. Rob pointed to a forum post about the user still being logged in after unauthorizing the app. To me that's only a minor annoyance I can live with.

From my tests, and correct me if I'm wrong, there is no way to log yourself out of a canvas app when using Facebooker2. That means if I log off Facebook, I am still logged in my Facebook canvas app. The only way I can log off my app is by clearing my browser's cookies. I would be very surprised if the PHP SDK worked this way since this is in my opinion a major security flaw that I didn't have before the cookie implementation. I can't find anyone in the Facebook developer forum complaining about this. And I tried a few facebook canvas apps using the PHP SDK (farmville, cityville, etc.) and can't reproduce this behavior.

There's probably something to be done on the JS side but relying only on the client for security is not a good idea since JS can easily be disabled or altered.

[mmangino]

I agree, hence my psuedocode

Deleting a cookie in the facebook_params method is unexpected. Nothin about that method says that a cookie will be removed. Instead, if you add the check and don't fetch the info from the cookie if we have a logged out user, the existing code will handle the case and remove the cookie.

[pomartel]

Alright, I can code it tomorrow morning and I'll update my pull request. Thanks for the feedback!

[pomartel]

I have updated the pull request and the tests. I also redeployed my test app on facebook : http://apps.facebook.com/facebooker-pomartel/

Mike, Rob, can you have a look at it and tell me if that works for you?

[mmangino]

Applied! Thanks!

[emiltin]

nice! i was just struggling with this exact issue.