XMLRPC server does not honor optional parameters

Marc,

Can you post some info here about your knowledge of this issue. From what I recall, its a Drupal core isssue?

Thanks,
Scott

subscribe.

I just did a little more research. The number of parameters is checked in the xmlrpc_server_call function inside xmlrpcs.inc in the core includes directory:

function xmlrpc_server_call($xmlrpc_server, $methodname, $args) {
  // Make sure parameters are in an array
  if ($args && !is_array($args)) {
    $args = array($args);
  }
  // Has this method been mapped to a Drupal function by us or by modules?
  if (!isset($xmlrpc_server->callbacks[$methodname])) {
    return xmlrpc_error(-32601, t('Server error. Requested method %methodname not specified.', array("%methodname" => $xmlrpc_server->message->methodname)));
  }
  $method = $xmlrpc_server->callbacks[$methodname];
  $signature = $xmlrpc_server->signatures[$methodname];

  // If the method has a signature, validate the request against the signature
  if (is_array($signature)) {
    $ok = TRUE;
    $return_type = array_shift($signature);
    // Check the number of arguments
    if (count($args) != count($signature)) {
      return xmlrpc_error(-32602, t('Server error. Wrong number of method parameters.'));
    }

This is where the error occurs. I'm not yet familiar enough with this code to see whether it can be changed easily. It looks like the method signature as recorded by Drupal would have to have a way of noting that some parameters are optional.

There is also another issue in that type checking fails (line 166 same module) so it is 2 part issue. A patch would have to be submitted to drupal core to over come this limitation.

We can simply check that the parameters passed in are less than or equal to the signature. The services module will then validate if we missing any parameters that are not optional.

Initial patch.

I tested this patch and it works for me. On the surface, it seems like there could be implications regarding security/stability regarding this, however, if the XMLRPC interface is used without the services module to fall back on. People could be calling functions with fewer arguments than needed, which could cause unwanted behavior?

I agree and it maybe that patch only gets committed if/when services gets into the core. At which point in time xmlrpc.php will be removed from the root folder and all xmlrpc traffic will be handled by services.

subscribe

subscribe

Wow.. I think this one is pretty bad, perhaps critical. at least the docs should get updated to let developers know they aren't crazy.

How can I help to fix this? If it is a core issue, is there an issue for it in the drupal issue queue?

Best,
Jacob

This patch is for core however it only applies if services is installed because we do the optional checking and validation of the signature.

  // If the method has a signature, validate the request against the signature
  if (is_array($signature)) {

dont give it a signature, then.

Oh and this is a core bug.

Before you ask "but what I do then" just use method.name => method_name in the xmlrpc hook.

Method parameters cannot be validated without a signature, so omitting it does not seem to be a solution.

No reason was given why this is Won't Fix, so I'm setting it back Active.

I bumped into this problem, too, and the above patch seems to solve it.

Hm, I am not so sure. The short version of the xmlrpc info does not check arguments at all. Why bother then?

@chx: I don't think I understand exactly what you mean.

What I'm doing is implementing a hook_service which returns the services provided by my module, including #method, #callback, #args, #return, and #help. Some of my methods under #method are optional, and for these #optional => TRUE is set. When I do that and then send a request via /services/xmlrpc with an optional parameter missing, I get back an error from Drupal.

I know you're proposing that I do something different than the above, but I'm not exactly sure what? If you could provide enough detail to try it out, I'll let you know if it works for me.

Thanks!

scottgifford, hook_services belongs to services module not core. I am talking of hook_xmlrpc and I am quite close to wont fix this issue just want to see why not.

Right, the problem here is a compatibility issue between the services module and XML-RPC support in core. The services module has a notion of an optional parameter, but it is implemented using the core xmlrpc_server_call, which doesn't seem to have any way to express optional parameters.

Maybe the solution is just to cut-n-paste a private copy of xmlrpc_server_call into the services module and make the fix there, but it seems excessive to duplicate about 50 lines of code when a 2-line patch in the core will do the job.

Originally this was a services module issue, so if you decide a fix shouldn't go in core, this issue should probably go back over to the services module instead of being marked wontfix. I can put together a patch duplicating the xmlrpc_server_call code over there and we'll see if the services folks take it.

Now, once again, why do you think xmlrpc_server_call does not support any number of arguments?

  // If the method has a signature, validate the request against the signature
  if (is_array($signature)) {

then dont leave a signature. Just xmlrpcmethodnanme => functionname will do. This is clearly documented: Each array element is either a pair of method => function or an array with four entries:

chx, at least in my case, I want argument checking, just argument checking that supports optional parameters. I don't want to give up checking altogether.

If this won't be fixed in core, let's take it back to Services and get it fixed there. I can cut-n-paste the code from xmlrpc_server_call as described above.

I have the same need as scottgifford and am definitely in favor of getting it fixed in Services, if not core. Code duplication is bad and should be kept to a minimum, but would be worth it for the simpler XML-RPC interfaces that optional parameters allow.

back to drupal issue queue as this needs to be fixed in core.

@marcingy, this isn't going to get fixed in core, @chx is just going to close it over there. If you are willing to accept a fix that duplicates the xmlrpc_server_call, let's take this back to Services and I'll submit a patch that does that. Otherwise, I can submit a documentation patch that #optional doesn't work.

That or leave it open with path attached and if people need this feature they can apply the patch to core or the services module if required - ie I don't want to be putting hacks into the services module otherwise it kinda defeats the drupal way of doing things and opens up potential security issues etc.

To clarify, there is no bug. This a feature request which IMO is a good idea. There is a patch, it's not a complicated one, so now we need tests.

Oh and I still do not know why services cant simply use the non-signature'd version of hook_xmlrpc...

@chx, on its own, providing no signature would do no argument checks at all, which is not the desired behavior.

But, it might make sense to do all argument checking in the Services module, then pass no signature at all to bypass the argument checking in xmlrpc_server_call. I think that's what you're suggesting. In that case, Services would duplicate only the argument checking part of xmlrpc_server_call, which is fairly small and straightforward. Better yet, the bit of needed functionality could be factored out into a new function that Services could simply call.

Regarding @marcingy's patch, I have tested it and it works for me. @heydere also reports it works, and presumably @marcingy tested it when writing the patch. So it seems to be fairly well-tested.

From my reading, though, the patch effectively makes all parameters optional when XML-RPC calls are made without the Services module. If that's the case, doing all argument checking in Services might be the best approach.

We need simpletests for every patch now, this is no exception.

@chx, I started working on a patch for this, and I see exactly what you mean. The Services module already does its own argument checking, so by passing in NULL for the arguments to the core xmlrpc functions, Services can bypass all of the core xmlrpc argument checks and just use its own checks. This patch to the Services module implements this.

The checks in xmlrpcs.inc are a bit more thorough than the ones in Services, but that's easy to improve, and improving it in Services will affect all servers, not just XML-RPC.

@marcingy, what do you think?

I should mention that the above patch is against Services 5.x-0.92. I tested it out on my Drupal 5 server; I don't have a server running Drupal 6.x or 7.x to test with.

scottgifford , the syntax you want is $callbacks[method] = drupal_function_name

Scott

I have done some testing with and it seems to work so I'm happy to commit the patch. If we start getting errors reported I'll back it out though.

Thanks for your work on this issue.

Also applied to 6-x-2-dev

Marc

the syntax you want is $callbacks[method] = drupal_function_name

Syntax corrected and new patch committed.

Thanks @marcingy!