1
\$\begingroup\$

I am using the following code to retrieve the URL of a random image in a remote folder that I own.

As a newbie and although this is working I would like to know if this is done properly, if it could be improved and if there is any security issue with this approach.

I am using json_encode and json_decode because I need to have an array as response. Also I am passing $params to remote through cURL because I need the returned image already with it's URL.

Thank you.

if (function_exists('curl_init')){
    $url    = 'http://example.com/images';
    $params = 'path=' . $url;

    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, $url . '/my_remote_script.php');
    curl_setopt($ch, CURLOPT_POST, true);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $params);
    curl_setopt($ch, CURLOPT_HEADER, false);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);

    $images = json_decode(curl_exec($ch));

    curl_close($ch);
}

my_remote_script.php

if (!$_POST['path']){
    die;
}

function getImages($path){
    if ($img_dir = opendir(dirname(__FILE__))){
        $images = array();

        while (($img_file = readdir($img_dir)) !== false){
            if (preg_match('/(\.gif|\.jpg|\.jpeg|\.png|\.GIF|\.JPG|\.JPEG|\.PNG)$/', $img_file)){
                $images[] = $path . '/' . $img_file;
            }
        }

        closedir($img_dir);
        return $images;
    }
}

function randomImage($list){
    $num   = array_rand($list);
    $image = $list[$num];

    return $image;
}

$images = getImages($_POST['path']);
$image  = randomImage($images);

print_r(json_encode($image));
\$\endgroup\$
1
\$\begingroup\$

Here are my thoughts:

  • Why is this a POST operation at all? Why not GET, since you are not changing ANY data whatsoever? The "parameter" you pass is not meaningful (it is just the base URL of the remote service) and it is not really even used in the server side script other than to append to the image name. If that is the case why not just remove it altogether since client already know the base URL of the site anyway.
  • Both the client-side cURL code and server-side code very heavily assume that everything is working correctly (the so called "happy path"). Some examples of that:
    • On server side, don't just do something like die() if the the request is mal-formed. Good services will reply with an HTTP 400-series error if the request is bad. That allows the client-side code to potentially better understand what went wrong, rather just receiving an empty HTTP 200 response.
    • If you were actually passing a meaningful parameter, you might consider stronger server-side validation before working with it. What happens if empty string is passed? What happens if an extraordinarily long string (i.e. buffer overflow attack) is passed?
    • In your getImages function you actually start to handle the edge case by having a conditional around being able to read the directory. Where is the else to handle a guess where directory is not readable? You would likely need to send a 500 series response to client to let them know there is internal server problem that is preventing the request from being fulfilled.
    • On client side, what happens if the server is not able to get a good response?
  • You are kind of relying on undocumented JSON behavior that certain libraries allow when you are just JSON-encoded a string primitive. Technically, the JSON specification requires that the only allowed root structures are objects or arrays. Passing an encoded primitive is not reliable behavior, if you ever have other clients connecting to this service. You should consider explicitly creating and passing an array like $image = array(randomImage($images))
  • You should just be echoing out json_encode($image) not print_r
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.