Permalink
Browse files

fix(web_services): handle string params with proper escaping

For string params, `serialize_parameters` outputs a string literal of PHP
code, and `addcslashes` was not appropriate for this task, particularly
around escapes in strings. While you can tell it to also escape escape
chars, a second pass would be needed.

The tests now `eval` the output just as the production code does, and
this also makes the tests easier to read.
  • Loading branch information...
mrclay committed Dec 2, 2016
1 parent 3dcc5ef commit 702ce46c44aec2546f953902061166bf3f48a5af
Showing with 42 additions and 39 deletions.
  1. +2 −5 mod/web_services/lib/web_services.php
  2. +40 −34 mod/web_services/tests/ElggCoreWebServicesApiTest.php
@@ -248,7 +248,7 @@ function _elgg_ws_get_parameter_names($method) {
* @param string $method API method name
* @param array $parameters Array of parameters
*
- * @return string or exception E.g. ",'foo',2.1"
+ * @return string or exception E.g. ',"foo",2.1'
* @throws APIException
* @since 1.7.0
* @access private
@@ -292,10 +292,7 @@ function serialise_parameters($method, $parameters) {
break;
case 'string':
- // This is using addcslashes() to escape characters to be inside a
- // single-quoted string literal in PHP code.
- // TODO in 3.0, convert to simple trim(), but this won't be completely BC.
- $serialised_parameters .= ",'" . addcslashes(trim($parameters[$key]), "'") . "'";
+ $serialised_parameters .= ',' . var_export(trim($parameters[$key]), true);
break;
case 'float':
$serialised_parameters .= "," . (float)trim($parameters[$key]);
@@ -210,79 +210,85 @@ public function testVerifyParameters() {
public function testSerialiseParameters() {
+ $evaler = function ($params) {
+ $s = serialise_parameters('test', $params);
+ return eval('return [' . substr($s, 1) . '];');
+ };
+
// int and bool
$this->registerFunction();
$parameters = array('param1' => 1, 'param2' => 0);
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ',1,false');
+ $this->assertIdentical($evaler($parameters), [1, false]);
// string
+ $test_string = 'testing';
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => 'testing');
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",'testing'");
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
// test string with " in it
+ $test_string = 'test"ing';
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => 'test"ing');
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ',\'test"ing\'');
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
// test string with ' in it
+ $test_string = 'test\'ing';
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => 'test\'ing');
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",'test\'ing'");
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
// test string with \ in it
+ $test_string = 'test\\ing';
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => 'test\ing');
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",'test\\ing'");
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
// test string with \' in it
+ $test_string = "test\\'ing";
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => "test\'ing");
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",'test\\\\'ing'"); // test\\'ing
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
+
+ // test string with \ at end
+ $test_string = "testing\\";
+ $this->registerFunction(false, false, array('param1' => array('type' => 'string')));
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
+
// test string reported by twall in #1364
+ $test_string = '{"html":"<div><img src=\\"http://foo.com\\"/>Blah Blah</div>"}';
$this->registerFunction(false, false, array('param1' => array('type' => 'string')));
- $parameters = array('param1' => '{"html":"<div><img src=\\"http://foo.com\\"/>Blah Blah</div>"}');
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",'{\"html\":\"<div><img src=\\\"http://foo.com\\\"/>Blah Blah</div>\"}'");
+ $parameters = array('param1' => $test_string);
+ $this->assertIdentical($evaler($parameters), [$test_string]);
// float
$this->registerFunction(false, false, array('param1' => array('type' => 'float')));
$parameters = array('param1' => 2.5);
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ',2.5');
+ $this->assertIdentical($evaler($parameters), [2.5]);
// indexed array of strings
$this->registerFunction(false, false, array('param1' => array('type' => 'array')));
- $parameters = array('param1' => array('one', 'two'));
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",array('0'=>'one','1'=>'two')");
+ $parameters = array('param1' => ['one', 'two']);
+ $this->assertIdentical($evaler($parameters), [['one', 'two']]);
// associative array of strings
$this->registerFunction(false, false, array('param1' => array('type' => 'array')));
- $parameters = array('param1' => array('first' => 'one', 'second' => 'two'));
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",array('first'=>'one','second'=>'two')");
+ $parameters = array('param1' => ['first' => 'one', 'second' => 'two']);
+ $this->assertIdentical($evaler($parameters), [['first' => 'one', 'second' => 'two']]);
- // indexed array of strings
+ // indexed array of strings (all values cast to strings)
$this->registerFunction(false, false, array('param1' => array('type' => 'array')));
- $parameters = array('param1' => array(1, 2));
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",array('0'=>'1','1'=>'2')");
+ $parameters = array('param1' => [1, 2]);
+ $this->assertIdentical($evaler($parameters), [['1', '2']]);
// test missing optional param
$this->registerFunction(false, false, [
'param1' => ['type' => 'int', 'required' => false],
'param2' => ['type' => 'int'],
]);
$parameters = ['param2' => '2'];
- $s = serialise_parameters('test', $parameters);
- $this->assertIdentical($s, ",null,2");
+ $this->assertIdentical($evaler($parameters), [null, 2]);
// test unknown type
$this->registerFunction(false, false, array('param1' => array('type' => 'bad')));

0 comments on commit 702ce46

Please sign in to comment.