I have e up with the following bits of code to call a method via AJAX in my PHP classes:

PHP:

class Ajax extends Controller {

    private $class;
    private $method;
    private $params;

    function __construct()
    {
        $this->params = $_POST; // Call params
        $call = explode('->', $this->params['call']);
        $this->class = new $call[0]; // e.g. controller->method
        $this->method = $call[1];
        array_shift($this->params);
        $this->parse();
    }

    public function index()
    {
        //Dummy
    }

    public function parse()
    {
        $r = '';
        $r = call_user_func_array(array($this->class, $this->method), $this->params);
        echo $r;
    }
}

Client:

function creditCheck2(id)
{
    $.post(ROOT + 'Ajax', {call: 'Record->creditState', id: id, enquiryid: enquiryId}, function(data) {
        alert(data)
    }, 'json')
}

It seems to work great, but is it secure and could it be better?

---

Just for reference, I have added my code with the changes suggested by the answers:

class Call extends Controller {

  private $class;
  private $method;
  private $params;
  private $authClasses = array(
      'Gallery'
  );

  function __construct()
  {
    $this->params = $_POST; // Call params
    $call = explode('->', $this->params['call']);

    if(!in_array($call[0], $this->authClasses))
    {
      die();
    }
    $this->class = new $call[0]; // e.g. controller->method
    $this->method = $call[1];
    unset($this->params['call']);
    $this->parse();
  }

  public function parse()
  {
    $r = '';
    $param = array();

    // Params in any order...
    $mRef = new ReflectionMethod($this->class, $this->method);
    foreach($mRef->getParameters() as $p) {
      $param[$p->name] = $this->params[$p->name];
    }
    $this->params = $param;

    if($r = @call_user_func_array(array($this->class, $this->method), $this->params))
    {
      echo $r;
    }
    else {

    }
  }

}

I have e up with the following bits of code to call a method via AJAX in my PHP classes:

PHP:

class Ajax extends Controller {

    private $class;
    private $method;
    private $params;

    function __construct()
    {
        $this->params = $_POST; // Call params
        $call = explode('->', $this->params['call']);
        $this->class = new $call[0]; // e.g. controller->method
        $this->method = $call[1];
        array_shift($this->params);
        $this->parse();
    }

    public function index()
    {
        //Dummy
    }

    public function parse()
    {
        $r = '';
        $r = call_user_func_array(array($this->class, $this->method), $this->params);
        echo $r;
    }
}

Client:

function creditCheck2(id)
{
    $.post(ROOT + 'Ajax', {call: 'Record->creditState', id: id, enquiryid: enquiryId}, function(data) {
        alert(data)
    }, 'json')
}

It seems to work great, but is it secure and could it be better?

---

Just for reference, I have added my code with the changes suggested by the answers:

class Call extends Controller {

  private $class;
  private $method;
  private $params;
  private $authClasses = array(
      'Gallery'
  );

  function __construct()
  {
    $this->params = $_POST; // Call params
    $call = explode('->', $this->params['call']);

    if(!in_array($call[0], $this->authClasses))
    {
      die();
    }
    $this->class = new $call[0]; // e.g. controller->method
    $this->method = $call[1];
    unset($this->params['call']);
    $this->parse();
  }

  public function parse()
  {
    $r = '';
    $param = array();

    // Params in any order...
    $mRef = new ReflectionMethod($this->class, $this->method);
    foreach($mRef->getParameters() as $p) {
      $param[$p->name] = $this->params[$p->name];
    }
    $this->params = $param;

    if($r = @call_user_func_array(array($this->class, $this->method), $this->params))
    {
      echo $r;
    }
    else {

    }
  }

}

• php
• javascript
• ajax
• oop
• code-injection

Share Improve this question Follow edited Sep 5, 2013 at 17:37 imperium2335 asked Aug 29, 2013 at 9:11 imperium2335imperium2335 24.2k3838 gold badges117117 silver badges194194 bronze badges 3

• Personally I use the __call magic method and made the method protected, thus removing any unexpected errors. Also depending on what logic your methods are doing you may want to add some sort of filter to the parameters. – Insyte Commented Aug 29, 2013 at 9:14
• @IanBrindley By a filter do you mean passing another param that defines what datatype the other params are? e.g. ...id: id, enquiryid: enquiryId, filter: 'numeric'... – imperium2335 Commented Aug 29, 2013 at 9:23
• @imperium2335, I have updated with Reflection sample. All credit to Jon for pointing the reference. – invisal Commented Aug 29, 2013 at 9:46

Add a ment  | 

2 Answers 2

Sorted by: Reset to default 4

Small issues

It could be better in that array_shift($this->params) unnecessarily assumes that the first item in the params array will always be call. That's not true and it does not agree with the direct access $this->params['call'] you are doing a little earlier. The array_shift should be replaced with simply unset($this->params['call']).

Bigger issues

There is also the problem that the order of values in the params array must match the order of parameters in the signature of the method you are trying to call. I don't think there is a guarantee that the order will be the same as the order of the parameters in the AJAX request, so that's a theoretical problem.

VERY big problem

More importantly, this way of doing things forces the author of the AJAX code to match the order of parameters in the signature of the method you are trying to call. This introduces a horrible level of coupling and is a major problem. What's worse, changing the order of the parameters by mistake will not be apparent. Consider:

public function bankTransfer($fromAccount, $toAccount, $amount);

$.post(ROOT + 'Ajax', {
    call: 'Bank->bankTransfer', 
    from: "sender",
    to: "recipient",
    amount: 42
}, function(data) { ... });

This would work. But if you do this

$.post(ROOT + 'Ajax', {
    call: 'Bank->bankTransfer', 
    to: "recipient", // swapped the order of
    from: "sender",  // these two lines
    amount: 42
}, function(data) { ... });

You will get the opposite result of what is expected. I believe it's immediately obvious that this is extremely bad.

To solve the problem you would have to use reflection to match the array keys in $this->params with the formal names of the parameters of the method being called.

Security

Finally, this code is insecure in that anyone can make a request that directs your code to call any method of any class with the appropriate parameters -- even methods that should not be accessible from a web environment.

This is another serious problem and cannot really be fixed unless you introduce some type of filtering to the dispatch logic.

It seems to work great, but is it secure and could it be better?

Are you using your own framework or using other framework? I believe that it isn't secure at all, if the attacker know what might be inside your framework. For example: there is database class in your framework, attacker can do the following:

{call: 'Database->execute', sql: 'SELECT * FROM information_schema.`tables`'}

Filtering

You can limit the number of class that you allow user to access. For example:

if (!in_array($this->class, array('Record', 'Hello'))) {
    die();
}

Reflection

This is sample of reflection that I just learn (Thanks to @Jon for the reference). This solve the problem of passing argument in different order from the PHP function.

class Email
{
    public function send($from, $to, $msg) {
        return "Send $from to $to: $msg";
    }
}

$rawParam = array('msg' => 'Hello World', 
               'to' => '[email protected]', 
               'from' => '[email protected]');
$param = array();

// Rearrange
$methodRef = new ReflectionMethod('Email', 'send');
foreach($methodRef->getParameters() as $p) {
    $param[$p->name] = $rawParam[$p->name];
}

var_dump($rawParam);
var_dump($param);