Version 6.x-3.x-dev 30th July (as well as rc-2)
PHP 5.3.6
OS: OSX
Steps to recreate
Enable services under admin/build/services/list/rest_services/resources. Assuming that you configured your services to use REST and services/rest as an endpoint.
Call:
/services/rest/node -- this will work correctly and return list of nodes available on your site
Now call:
/services/rest/node/NID --- privide some valid node id. You will get empty result.
The same applies for other resources like: file, comment, user.
Bug in Code
There are at least two issues here:
- First is the problematic use of isset() funciton in _RESOURCE_NAME_resource_access() functions inside RESOURCE_NAME_resource.inc files.
- Second is the fact that getControllerArguments() method of RESTServer does not use information about arguments type to convert it to appropriate type.
Ad. 1
At the very top the funciton is checking condition like:
<?php
if (isset($args[0]['account']) || isset($args[0]['data'])) {
...
}
?>
This will always return true if only the $args[0] is defined as string (and it is as described in second issue). PHP behaves a bit unexpectedly here. Array whose elements are strings can be accessed with array notation $arr[0][0], $arr[0][1] etc. What's worse $arr[0]['any_string'] will also work as 'any_string' will be converted to integer causing isset() to always return TRUE as shown in the example code below.
<?php
$arr = array(
'1234'
);
print_r($arr);
if (isset($arr[0]['node'])) {
echo PHP_EOL;
echo 'non-existent key "node" $arr[0]["node"]';
echo 'has value: '. $arr[0]['node'].PHP_EOL;
echo PHP_EOL;
echo 'string key representing number is converted to number:'.PHP_EOL;
echo '$arr[0]["0"] = '. $arr[0]['0'].PHP_EOL;
echo '$arr[0]["1"] = '. $arr[0]['1'].PHP_EOL;
echo '$arr[0]["2"] = '. $arr[0]['2'].PHP_EOL;
echo '$arr[0]["3"] = '. $arr[0]['3'].PHP_EOL;
}
?>
Outputs:
Array
(
[0] => 1234
)
non-existent key "node" $arr[0]["node"]has value: 1
string key representing number is converted to number:
$arr[0]["0"] = 1
$arr[0]["1"] = 2
$arr[0]["2"] = 3
$arr[0]["3"] = 4
To solve this it's enough to replace current if statements with "array friendly" ones like:
<?php
if (is_array($args[0]) && key_exists('node', $args[0])) {
...
?>
On the other hand fixing second issue will automatically resolve this one.
Ad. 2
Inside getControllerArguments() in line 225 of RESTServer.inc the $arguments array is being pushed values of arguments. This $arguments array is being used in handler() method where it's being passed to different callback functions among them the above __resource_access().
If the proper conversion based on the type of arument happened than the variable $args[0] from first issue would be integer not a string and isset() function could be safely used.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | access-hole-1236316-7.patch | 2.19 KB | Peter Swietoslawski |
| #2 | node-1236316_1.png | 145.1 KB | Peter Swietoslawski |
Comments
Comment #1
kylebrowning commentedI can't reproduce this, when i goto
/services/rest/node/NID
I get all of node information that I would expect.
The tests are not failing either so unless you can provide a test that fails this is a cannot reproduce.
Im inclined to set this to a Support Requests.
When you get your empty result, have you checked the returned headers? Is it possible you got a 406?
Heres an image for proof.
https://skitch.com/kylebrowning/fc97k/untitled
Comment #2
Peter Swietoslawski commentedhey Kyle attaching headers of my request.
I'm getting the same error whether running against local Apache on OSX as well as in dev box deployed on LAMP.
As you can see I'm asking for node 24478 and it's being converted to request for node 2. This is where logic:
from _node_resource_access($op = 'view', $args = array()) kicks in. If I change this to what I've suggested above (or comment this out ) it works fine.
So if you canNOT recreate and I can on two different boxes maybe there is a difference in PHP version you're running it against?
Comment #3
kylebrowning commentedWell that can't be commented out as its a backwards compatibility piece for rc1.
Do you have a patch that I could look at to see what you are referring to in your OP.
Comment #4
marcingy commentedI extended the tests to make sure the issue wan't related to $nid > 9
These tests were run on xampp with php 5.3.5, so I suppose it is possible that php 5.3.6 may introduce a new variable. The tests completed successfully - so its all a bit strange.
Comment #5
marcingy commentedNote my test was on d7.
Comment #6
kylebrowning commentedtests in d6
https://skitch.com/kylebrowning/fpy8u/http-drupal6-services-sites-defaul...
Comment #7
Peter Swietoslawski commentedI've did some more research on the issue. Guys your tests are passing but the underlying logic is broken. The problem lies in isset() function which can not be used for checking existence of an element in nested array if there is no guarantee that the elements of array will also be array not a strings.
We are talking about resource access callbacks. Tests are passing as the seet of data you are running them against allows for this. Not so much in case of my data and that is why it does not work for me.
The issue here is that access is being checked for different resource than he resource the request is being made!
To see what's going happen run the code in Netbeans setting breakpoint in node_resource.inc file in line 439. You will see that the logic:
always let's inside if statement allowing $args[0] (effectively node ID) to be reset to 2. Thus node_access invoked later always checks access to node 2 regardless of what node you are asking for. I don't have node with ID 2 in my data set that's why I was able to catch this. Automated tests the way they are built will not catch this.
So as a matter of fact this issue is a security hole.
Attached is a patch fixing issue for:
Taxonomy and System resources are not affected.
Comment #8
Peter Swietoslawski commentedComment #9
kylebrowning commentedI added an is_array() b/c this will have notices if the argument is not an array.
Dbl check its fixed in dev for me, and if so I think were ready to tag another rc
Comment #10
sitiveni commentedThanks Peter for creating the patch; applied it and works like a charm.
I added (as Kyle mentions) is_array() to the condition in order to prevent PHP warning:
Comment #11
kylebrowning commentedsitiveni, can you double check its fixed in dev as well so that I can close this ticket?
Comment #12
sitiveni commentedDownloaded the latest dev version and is_array() is there. Also the resources return the correct object.
Once minor thing though I noted: the tar.gz does not contain is_array() (zip does), maybe you haven't created it?
Also checked git repository, that is okiedokie.
Many thanks to you guys for being fast as the wind!
Comment #13
kylebrowning commentedTHe builds on drupal.org only happen twice a day, so it seems the tar.gz hasn't been rebuilt yet, but if git is good, then were good.
Comment #14
marcingy commentedPeter thanks for your work on getting to the bottom of this nice catch :)
Comment #15
Peter Swietoslawski commentedGuys thx a lot for quick turnaround on this.
Comment #15.0
Peter Swietoslawski commentedAdding different version of module against which bug can be recreated.