Skip to content

Commit ab2d46d

Browse files
author
jcain
committed
Upgraded to version 3.0.1.
Unit tests can be run from the command line using: phpunit --colors --coverage-html coverage --verbose --stderr --bootstrap tests/bootstrap.php tests/tests.php Changes: + Added a new bootstrap file (as bootstrap.php) that helps the unit tests run more smoothly. + Allow for the possibility that session_start has already been called prior to construction of a Facebook class. + Updated the app-secret unit test to confirm that Desktop applications require a user access token to get insights. + Make sure that current URLs like /example.php?a=b&c=&d retain their structure (don't strip or introduce an equals sign for valueless GET params), and added unit tests to exercise this. + CSRF state is now managed using the persistent store instead of cookies.
1 parent 56fe8b7 commit ab2d46d

File tree

4 files changed

+134
-50
lines changed

4 files changed

+134
-50
lines changed

‎src/base_facebook.php‎

Lines changed: 39 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ abstract class BaseFacebook
120120
/**
121121
* Version.
122122
*/
123-
const VERSION = '3.0.0';
123+
const VERSION = '3.0.1';
124124

125125
/**
126126
* Default options for curl.
@@ -206,8 +206,10 @@ public function __construct($config) {
206206
if (isset($config['fileUpload'])) {
207207
$this->setFileUploadSupport($config['fileUpload']);
208208
}
209-
if (isset($_COOKIE[$this->getCSRFTokenCookieName()])) {
210-
$this->state = $_COOKIE[$this->getCSRFTokenCookieName()];
209+
210+
$state = $this->getPersistentData('state');
211+
if (!empty($state)) {
212+
$this->state = $this->getPersistentData('state');
211213
}
212214
}
213215

@@ -533,7 +535,7 @@ protected function getCode() {
533535

534536
// CSRF state has done its job, so clear it
535537
$this->state = null;
536-
unset($_COOKIE[$this->getCSRFTokenCookieName()]);
538+
$this->clearPersistentData('state');
537539
return $_REQUEST['code'];
538540
} else {
539541
self::errorLog('CSRF state token does not match one provided.');
@@ -575,18 +577,14 @@ protected function getApplicationAccessToken() {
575577
}
576578

577579
/**
578-
* Lays down a CSRF state token for this process. We
579-
* only generate a new CSRF token if one isn't currently
580-
* circulating in the domain's cookie jar.
580+
* Lays down a CSRF state token for this process.
581581
*
582582
* @return void
583583
*/
584584
protected function establishCSRFTokenState() {
585585
if ($this->state === null) {
586586
$this->state = md5(uniqid(mt_rand(), true));
587-
setcookie($name = $this->getCSRFTokenCookieName(),
588-
$value = $this->state,
589-
$expires = time() + 3600); // sticks for an hour
587+
$this->setPersistentData('state', $this->state);
590588
}
591589
}
592590

@@ -772,15 +770,6 @@ protected function makeRequest($url, $params, $ch=null) {
772770
return $result;
773771
}
774772

775-
/**
776-
* The name of the cookie housing the CSRF protection value.
777-
*
778-
* @return String the cookie name
779-
*/
780-
protected function getCSRFTokenCookieName() {
781-
return 'fbcsrf_'.$this->getAppId();
782-
}
783-
784773
/**
785774
* Parses a signed_request and validates the signature.
786775
*
@@ -923,16 +912,19 @@ protected function getCurrentUrl() {
923912
$currentUrl = $protocol . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'];
924913
$parts = parse_url($currentUrl);
925914

926-
// drop known fb params
927915
$query = '';
928916
if (!empty($parts['query'])) {
929-
$params = array();
930-
parse_str($parts['query'], $params);
931-
foreach(self::$DROP_QUERY_PARAMS as $key) {
932-
unset($params[$key]);
917+
// drop known fb params
918+
$params = explode('&', $parts['query']);
919+
$retained_params = array();
920+
foreach ($params as $param) {
921+
if ($this->shouldRetainParam($param)) {
922+
$retained_params[] = $param;
923+
}
933924
}
934-
if (!empty($params)) {
935-
$query = '?' . http_build_query($params, null, '&');
925+
926+
if (!empty($retained_params)) {
927+
$query = '?'.implode($retained_params, '&');
936928
}
937929
}
938930

@@ -947,6 +939,25 @@ protected function getCurrentUrl() {
947939
return $protocol . $parts['host'] . $port . $parts['path'] . $query;
948940
}
949941

942+
/**
943+
* Returns true if and only if the key or key/value pair should
944+
* be retained as part of the query string. This amounts to
945+
* a brute-force search of the very small list of Facebook-specific
946+
* params that should be stripped out.
947+
*
948+
* @param String a key or key/value pair within a URL's query (e.g.
949+
* 'foo=a', 'foo=', or 'foo'.
950+
*/
951+
protected function shouldRetainParam($param) {
952+
foreach (self::$DROP_QUERY_PARAMS as $drop_query_param) {
953+
if (strpos($param, $drop_query_param.'=') === 0) {
954+
return false;
955+
}
956+
}
957+
958+
return true;
959+
}
960+
950961
/**
951962
* Analyzes the supplied result to see if it was thrown
952963
* because the access token is no longer valid. If that is
@@ -1004,7 +1015,7 @@ protected static function base64UrlDecode($input) {
10041015
}
10051016

10061017
/**
1007-
* Each of the following three methods should be overridden in
1018+
* Each of the following four methods should be overridden in
10081019
* a concrete subclass, as they are in the provided Facebook class.
10091020
* The Facebook class uses PHP sessions to provide a primitive
10101021
* persistent store, but another subclass--one that you implement--
@@ -1014,5 +1025,6 @@ protected static function base64UrlDecode($input) {
10141025
*/
10151026
abstract protected function setPersistentData($key, $value);
10161027
abstract protected function getPersistentData($key, $default = false);
1028+
abstract protected function clearPersistentData($key);
10171029
abstract protected function clearAllPersistentData();
10181030
}

‎src/facebook.php‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,21 @@ class Facebook extends BaseFacebook
3333
* @see BaseFacebook::__construct in facebook.php
3434
*/
3535
public function __construct($config) {
36-
parent::__construct($config);
37-
if (!isset($_SESSION)) {
36+
if (!session_id()) {
3837
session_start();
3938
}
39+
parent::__construct($config);
4040
}
4141

42+
protected static $kSupportedKeys =
43+
array('state', 'code', 'access_token', 'user_id');
44+
4245
/**
4346
* Provides the implementations of the inherited abstract
4447
* methods. The implementation uses PHP sessions to maintain
45-
* a store for user ids and access tokens.
48+
* a store for authorization codes, user ids, CSRF states, and
49+
* access tokens.
4650
*/
47-
protected static $kSupportedKeys =
48-
array('code', 'access_token', 'user_id');
49-
5051
protected function setPersistentData($key, $value) {
5152
if (!in_array($key, self::$kSupportedKeys)) {
5253
self::errorLog('Unsupported key passed to setPersistentData.');
@@ -68,10 +69,19 @@ protected function getPersistentData($key, $default = false) {
6869
$_SESSION[$session_var_name] : $default;
6970
}
7071

72+
protected function clearPersistentData($key) {
73+
if (!in_array($key, self::$kSupportedKeys)) {
74+
self::errorLog('Unsupported key passed to clearPersistentData.');
75+
return;
76+
}
77+
78+
$session_var_name = $this->constructSessionVariableName($key);
79+
unset($_SESSION[$session_var_name]);
80+
}
81+
7182
protected function clearAllPersistentData() {
7283
foreach (self::$kSupportedKeys as $key) {
73-
$session_var_name = $this->constructSessionVariableName($key);
74-
unset($_SESSION[$session_var_name]);
84+
$this->clearPersistentData($key);
7585
}
7686
}
7787

‎tests/bootstrap.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?php
2+
3+
$base = realpath(dirname(__FILE__) . '/..');
4+
require "$base/src/base_facebook.php";
5+
require "$base/src/facebook.php";

‎tests/tests.php‎

Lines changed: 72 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,44 @@ public function testSetFileUploadSupport() {
9494
'Expect file upload support to be on.');
9595
}
9696

97+
public function testGetCurrentURL() {
98+
$facebook = new FBGetCurrentURLFacebook(array(
99+
'appId' => self::APP_ID,
100+
'secret' => self::SECRET,
101+
));
102+
103+
// fake the HPHP $_SERVER globals
104+
$_SERVER['HTTP_HOST'] = 'www.test.com';
105+
$_SERVER['REQUEST_URI'] = '/unit-tests.php?one=one&two=two&three=three';
106+
$current_url = $facebook->publicGetCurrentUrl();
107+
$this->assertEquals(
108+
'http://www.test.com/unit-tests.php?one=one&two=two&three=three',
109+
$current_url,
110+
'getCurrentUrl function is changing the current URL');
111+
112+
// ensure structure of valueless GET params is retained (sometimes
113+
// an = sign was present, and sometimes it was not)
114+
// first test when equal signs are present
115+
$_SERVER['HTTP_HOST'] = 'www.test.com';
116+
$_SERVER['REQUEST_URI'] = '/unit-tests.php?one=&two=&three=';
117+
$current_url = $facebook->publicGetCurrentUrl();
118+
$this->assertEquals(
119+
'http://www.test.com/unit-tests.php?one=&two=&three=',
120+
$current_url,
121+
'getCurrentUrl function is changing the current URL');
122+
123+
// now confirm that
124+
$_SERVER['HTTP_HOST'] = 'www.test.com';
125+
$_SERVER['REQUEST_URI'] = '/unit-tests.php?one&two&three';
126+
$current_url = $facebook->publicGetCurrentUrl();
127+
$this->assertEquals(
128+
'http://www.test.com/unit-tests.php?one&two&three',
129+
$current_url,
130+
'getCurrentUrl function is changing the current URL');
131+
}
132+
97133
public function testGetLoginURL() {
98-
$facebook = new TransientFacebook(array(
134+
$facebook = new Facebook(array(
99135
'appId' => self::APP_ID,
100136
'secret' => self::SECRET,
101137
));
@@ -120,7 +156,7 @@ public function testGetLoginURL() {
120156
}
121157

122158
public function testGetLoginURLWithExtraParams() {
123-
$facebook = new TransientFacebook(array(
159+
$facebook = new Facebook(array(
124160
'appId' => self::APP_ID,
125161
'secret' => self::SECRET,
126162
));
@@ -148,30 +184,28 @@ public function testGetLoginURLWithExtraParams() {
148184
}
149185

150186
public function testGetCodeWithValidCSRFState() {
151-
$csrf_cookie_name = FBCode::constructCSRFTokenCookieName(self::APP_ID);
152-
$_COOKIE[$csrf_cookie_name] = $this->generateMD5HashOfRandomValue();
153187
$facebook = new FBCode(array(
154188
'appId' => self::APP_ID,
155189
'secret' => self::SECRET,
156190
));
157191

192+
$facebook->setCSRFStateToken();
158193
$code = $_REQUEST['code'] = $this->generateMD5HashOfRandomValue();
159-
$_REQUEST['state'] = $_COOKIE[$csrf_cookie_name];
194+
$_REQUEST['state'] = $facebook->getCSRFStateToken();
160195
$this->assertEquals($code,
161196
$facebook->publicGetCode(),
162197
'Expect code to be pulled from $_REQUEST[\'code\']');
163198
}
164199

165200
public function testGetCodeWithInvalidCSRFState() {
166-
$csrf_cookie_name = FBCode::constructCSRFTokenCookieName(self::APP_ID);
167-
$_COOKIE[$csrf_cookie_name] = $this->generateMD5HashOfRandomValue();
168201
$facebook = new FBCode(array(
169202
'appId' => self::APP_ID,
170203
'secret' => self::SECRET,
171204
));
172205

206+
$facebook->setCSRFStateToken();
173207
$code = $_REQUEST['code'] = $this->generateMD5HashOfRandomValue();
174-
$_REQUEST['state'] = $_COOKIE[$csrf_cookie_name]."forgery!!!";
208+
$_REQUEST['state'] = $facebook->getCSRFStateToken().'forgery!!!';
175209
$this->assertFalse($facebook->publicGetCode(),
176210
'Expect getCode to fail, CSRF state should not match.');
177211
}
@@ -183,7 +217,7 @@ public function testGetCodeWithMissingCSRFState() {
183217
));
184218

185219
$code = $_REQUEST['code'] = $this->generateMD5HashOfRandomValue();
186-
// don't set $_REQUEST['fbcsrf_<app-id>']
220+
// intentionally don't set CSRF token at all
187221
$this->assertFalse($facebook->publicGetCode(),
188222
'Expect getCode to fail, CSRF state not sent back.');
189223

@@ -562,9 +596,20 @@ public function testAppSecretCall() {
562596
'appId' => self::APP_ID,
563597
'secret' => self::SECRET,
564598
));
565-
$response = $facebook->api('/' . self::APP_ID . '/insights');
566-
$this->assertTrue(count($response['data']) > 0,
567-
'Expect some data back.');
599+
600+
$proper_exception_thrown = false;
601+
try {
602+
$response = $facebook->api('/' . self::APP_ID . '/insights');
603+
$this->fail('Desktop applications need a user token for insights.');
604+
} catch (FacebookApiException $e) {
605+
$proper_exception_thrown =
606+
strpos($e->getMessage(),
607+
'Requires session when calling from a desktop app') !== false;
608+
} catch (Exception $e) {}
609+
610+
$this->assertTrue($proper_exception_thrown,
611+
'Incorrect exception type thrown when trying to gain '.
612+
'insights for desktop app without a user access token.');
568613
}
569614

570615
public function testBase64UrlEncode() {
@@ -734,6 +779,7 @@ protected function setPersistentData($key, $value) {}
734779
protected function getPersistentData($key, $default = false) {
735780
return $default;
736781
}
782+
protected function clearPersistentData($key) {}
737783
protected function clearAllPersistentData() {}
738784
}
739785

@@ -762,18 +808,23 @@ class PersistentFBPublic extends Facebook {
762808
public function publicParseSignedRequest($input) {
763809
return $this->parseSignedRequest($input);
764810
}
811+
765812
public function publicSetPersistentData($key, $value) {
766813
$this->setPersistentData($key, $value);
767814
}
768815
}
769816

770-
class FBCode extends TransientFacebook {
817+
class FBCode extends Facebook {
771818
public function publicGetCode() {
772819
return $this->getCode();
773820
}
774821

775-
public static function constructCSRFTokenCookieName($app_id) {
776-
return 'fbcsrf_'.$app_id;
822+
public function setCSRFStateToken() {
823+
$this->establishCSRFTokenState();
824+
}
825+
826+
public function getCSRFStateToken() {
827+
return $this->getPersistentData('state');
777828
}
778829
}
779830

@@ -782,3 +833,9 @@ public function publicGetApplicationAccessToken() {
782833
return $this->getApplicationAccessToken();
783834
}
784835
}
836+
837+
class FBGetCurrentURLFacebook extends TransientFacebook {
838+
public function publicGetCurrentUrl() {
839+
return $this->getCurrentUrl();
840+
}
841+
}

0 commit comments

Comments
 (0)