Database Plugin for CKFinder users

Question

I have written a small plugin library for CKFinder that I would like some critiques on.

The library is located here.

If you are looking for where to start, check out plugin.php and hook function in here in the assets/ckfinder/core/connector/php/php5/CommandHandler/ folder.

I would really love to hear your critiques on:

• Code quality
• Code clarity
• How to improve
• Security it self in query mysql like I must use prepare-statement,etc.
• Anything else that needs clarification expansion etc I have also heard that my static column(like my column of table in my plugin.php) should be replaced with something more dynamic, How would I go about doing that?

I'm more interested in what I'm doing wrong than right.

Any opinions on the actual usefulness of the library are welcome.

Plugin.php

function onSave2Dbase($jns=null,$fname=null,$arr=null,$newFileName=null) {
    global $config;
    $ejsplug_config = $config['Plugin_ejsplug'];
    $opt_cfg = $config['Plugin_ejsplug']['opt'];

    $Urlpath = $arr->getUrl();
    $Physicalpath = $arr->getServerPath();

    $link = mysql_connect($ejsplug_config['dbhost'], $ejsplug_config['dbuser'], $ejsplug_config['dbpass'])or die('Could not connect: ' . mysql_error());
    mysql_select_db($ejsplug_config['dbase']) or die('Could not select database');

    if(isset($_SESSION['IDRole']) && !is_null($arr)){
        $gid = intval($_SESSION['IDRole']);
        $usr = $this->S_dbase("select username from users where IDRole='".$gid."'");
        $usr = $usr['username'];

        switch($jns){
            case 'FileUpload':
            if($arr->getResourceTypeName() == "Images"){
                if($arr->getClientPath() == "/slideshow/"){
                    $this->IUD_dbase("INSERT INTO ".$opt_cfg["main_table"]." (foto, keterangan, url_path, physical_path, create_time, create_by) VALUES ('".$fname."', '".$fname."', '".$Urlpath.$fname."', '".$Physicalpath.$fname."', '".date("Y-m-d H:i:s")."', '".$usr."')");
                }
                else{
                    $this->IUD_dbase("INSERT INTO ".$opt_cfg["other_table"]." (nama, keterangan, url_path, physical_path, create_time, create_by) VALUES ('".$fname."', '".$fname."', '".$Urlpath.$fname."', '".$Physicalpath.$fname."', '".date('Y-m-d H:i:s')."', '".$usr."')");
                }
            }
            else{
                $this->IUD_dbase("INSERT INTO ".$opt_cfg["other_table"]." (nama, keterangan, url_path, physical_path, create_time, create_by) VALUES ('".$fname."', '".$fname."', '".$Urlpath.$fname."', '".$Physicalpath.$fname."', '".date('Y-m-d H:i:s')."', '".$usr."')");
            }
            break;

            case 'QuickUpload':
            break;

            case 'DownloadFile':
            case 'Thumbnail':
            break;

            case 'CopyFiles':
            case 'CreateFolder':
            case 'DeleteFiles':
            if($arr->getResourceTypeName() == "Images"){
                if($arr->getClientPath() == "/slideshow/"){
                    $this->IUD_dbase("DELETE FROM ".$opt_cfg["main_table"]." WHERE physical_path ='".$Physicalpath.$fname."'");
                }
                else{
                    $this->IUD_dbase("DELETE FROM ".$opt_cfg["other_table"]." WHERE physical_path ='".$Physicalpath.$fname."'");
                }
            }
            else{
                $this->IUD_dbase("DELETE FROM ".$opt_cfg["other_table"]." WHERE physical_path ='".$Physicalpath.$fname."'");
            }
            break;

            case 'DeleteFolder':
            case 'GetFiles':
            case 'GetFolders':
            case 'Init':
            case 'LoadCookies':
            case 'MoveFiles':
            case 'RenameFile':              
            if($arr->getResourceTypeName() == "Images"){
                if($arr->getClientPath() == "/slideshow/"){
                    $this->IUD_dbase("UPDATE ".$opt_cfg["main_table"]." SET foto='".$newFileName."',keterangan='".$newFileName."',url_path='".$Urlpath.$newFileName."',physical_path='".$Physicalpath.$newFileName."',create_by='".$usr."' WHERE physical_path ='".$Physicalpath.$fname."'");
                }
                else{
                    $this->IUD_dbase("UPDATE ".$opt_cfg["other_table"]." SET nama='".$newFileName."',keterangan='".$newFileName."',url_path='".$Urlpath.$newFileName."',physical_path='".$Physicalpath.$newFileName."',create_by='".$usr."' WHERE physical_path ='".$Physicalpath.$fname."'");
                }
            }
            else{
                $this->IUD_dbase("UPDATE ".$opt_cfg["other_table"]." SET nama='".$newFileName."',keterangan='".$newFileName."',url_path='".$Urlpath.$newFileName."',physical_path='".$Physicalpath.$newFileName."',create_by='".$usr."' WHERE physical_path ='".$Physicalpath.$fname."'");
            }
            break;

            case 'RenameFolder':
            break;

            default:
            break;
        }
    }
    mysql_close($link);
    return true;
}

private function S_dbase($sql){
    $result = mysql_query($sql) or die('Query failed: ' . mysql_error());
    return mysql_fetch_array($result, MYSQL_ASSOC);
}

private function IUD_dbase($sql){
    // $result = mysql_query($sql) or die('Query failed: ' . mysql_error());
    mysql_query($sql) or die('Query failed: ' . mysql_error());
}

And this is my config.php in ckfinder folder. This is my new modified and not yet committed to my github.

Config.php

<?php
session_start();

function CheckAuthentication() {
    return (isset($_SESSION['IDRole'])?true:false);
}

$config['LicenseName'] = '';
$config['LicenseKey'] = '';

if(!isset($_SESSION['IDRole'])){
    $_SESSION['IDRole'] = 000;
}

$baseUrl = 'http://'.$_SERVER['SERVER_NAME'].'/demo_ckfinder/trunk/assets/user_files/administrator/';
if ($_SESSION['IDRole'] == '4') {
    $homeDir = '_asus/';
} else if ($_SESSION['IDRole'] == '3') {
    $homeDir = '_pengguna/';
}  else if ($_SESSION['IDRole'] == '2') {
    $homeDir = '_admin/';
}  else if ($_SESSION['IDRole'] == '1') {
    $homeDir = '';
} else {
    $homeDir = '_publik/';
}

$baseDir = resolveUrl($baseUrl);

$config['Thumbnails'] = Array(
    'url' => $baseUrl. '_thumbs',
    'directory' => $baseDir . '_thumbs',
    'enabled' => true,
    'directAccess' => true,
    'maxWidth' => 100,
    'maxHeight' => 100,
    'bmpSupported' => false,
    'quality' => 80);


$config['Images'] = Array(
    'maxWidth' => 1600,
    'maxHeight' => 1200,
    'quality' => 80);

$config['RoleSessionVar'] = 'CKFinder_UserRole';

if ($_SESSION['IDRole'] == '4') {
    $config['AccessControl'][] = Array(
        'role' => '*',
        'resourceType' => '*',
        'folder' => '/',
        'folderView' => true,
        'folderCreate' => true,
        'folderRename' => true,
        'folderDelete' => true,
        'fileView' => true,
        'fileUpload' => true,
        'fileRename' => true,
        'fileDelete' => true);
} else if ($_SESSION['IDRole'] == '3') {
    $config['AccessControl'][] = Array(        
        'role' => '*',
        'resourceType' => '*',
        'folder' => '/',
        'folderView' => true,
        'folderCreate' => false,
        'folderRename' => false,
        'folderDelete' => false,
        'fileView' => true,
        'fileUpload' => true,
        'fileRename' => false,
        'fileDelete' => false);
} else if ($_SESSION['IDRole'] == '2') {
    $config['AccessControl'][] = Array(
        'role' => '*',
        'resourceType' => '*',
        'folder' => '/',
        'folderView' => true,
        'folderCreate' => true,
        'folderRename' => true,
        'folderDelete' => true,
        'fileView' => true,
        'fileUpload' => true,
        'fileRename' => true,
        'fileDelete' => true);
} else if ($_SESSION['IDRole'] == '1') {
    $config['AccessControl'][] = Array(
        'role' => '*',
        'resourceType' => '*',
        'folder' => '/',
        'folderView' => true,
        'folderCreate' => true,
        'folderRename' => true,
        'folderDelete' => true,
        'fileView' => true,
        'fileUpload' => true,
        'fileRename' => true,
        'fileDelete' => true);
} else {
    $config['AccessControl'][] = Array(
        'role' => '*',
        'resourceType' => 'Images',
        'folder' => '/',
        'folderView' => true,
        'folderCreate' => false,
        'folderRename' => false,
        'folderDelete' => false,
        'fileView' => true,
        'fileUpload' => false,
        'fileRename' => false,
        'fileDelete' => false);
}



$config['DefaultResourceTypes'] = '';

$config['ResourceType'][] = Array(
    'name' => 'Files', // Single quotes not allowed
    'url' => $baseUrl . 'files'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'directory' => $baseDir . 'files'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'maxSize' => '10M',
    'allowedExtensions' => '7z,aiff,asf,avi,bmp,csv,doc,docx,fla,flv,gif,gz,gzip,jpeg,jpg,mid,mov,mp3,mp4,mpc,mpeg,mpg,ods,odt,pdf,png,ppt,pptx,pxd,qt,ram,rar,rm,rmi,rmvb,rtf,sdc,sitd,swf,sxc,sxw,tar,tgz,tif,tiff,txt,vsd,wav,wma,wmv,xls,xlsx,zip',
    'deniedExtensions' => '');

$config['ResourceType'][] = Array(
    'name' => 'Images',
    'url' => $baseUrl . 'images'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'directory' => $baseDir . 'images'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'maxSize' => '10M',
    'allowedExtensions' => 'bmp,gif,jpeg,jpg,png',
    'deniedExtensions' => '');

$config['ResourceType'][] = Array(
    'name' => 'Flash',
    'url' => $baseUrl . 'flash'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'directory' => $baseDir . 'flash'.($homeDir != ''?'/'.$homeDir:$homeDir),
    'maxSize' => '10M',
    'allowedExtensions' => 'swf,flv',
    'deniedExtensions' => '');

$config['CheckDoubleExtension'] = true;

$config['DisallowUnsafeCharacters'] = false;

$config['FilesystemEncoding'] = 'UTF-8';

$config['SecureImageUploads'] = true;

$config['CheckSizeAfterScaling'] = true;

$config['HtmlExtensions'] = array('html', 'htm', 'xml', 'js');

$config['HideFolders'] = Array(".svn", "CVS");

$config['HideFiles'] = Array(".*");

$config['ChmodFiles'] = 0777;

$config['ChmodFolders'] = 0755;

$config['ForceAscii'] = false;

$config['XSendfile'] = false;


include_once "plugins/imageresize/plugin.php";
include_once "plugins/fileeditor/plugin.php";
include_once "plugins/zip/plugin.php";
include_once "plugins/ejsplug/plugin.php";

$config['Plugin_ejsplug'] = array(
    "dbhost" => "your_host",
    "dbuser" => "your_user",
    "dbpass" => "your_pass",
    "dbase" => "your_database",
    "opt" => array(
        "main_table" => "slideshow",
        "other_table" => "userfiles"
    )
);

$config['plugin_imageresize']['smallThumb'] = '90x90';
$config['plugin_imageresize']['mediumThumb'] = '120x120';
$config['plugin_imageresize']['largeThumb'] = '180x180';

Answer 1

Security it self in query mysql like I must use prepare-statement,etc.

Yes, you really should use prepared statements, your current code looks extremely vulnerable. Right now, $gid is the only value that you actually validate in some way before putting into a query, all other variables - if user supplied - will lead to an SQL injection (they could have been cleaned by CKFinder, but I really would not rely on that, and input validation is at best a nice addition to proper escaping or prepared statements). SQL injection is possible in update and insert statements, not only select statements..

Also note that mysql_query is - as it's documentation says - deprecated since 2013, you really should not be using it anymore. Use either mysqli or PDO, and use prepared statements.

Naming

You should handle your variable names uniformly. Either use camelCase, or snake_case, but don't mix them, as that will make it harder to remember names. Also, don't start variable or function names with an uppercase character.

And don't abbreviate variable names. What is $jns? And does $fname stand for $fileName? Maybe, but why is it written out in $newFileName? $usr should be $username, etc.

It's also not really clear that IUD_dbase stands for perform insert, update, or delete query. I would just get rid of it, it doesn't really add any benefit anyways.

Early Return

If you turn this if around: if(isset($_SESSION['IDRole']) && !is_null($arr)){, then you would save one level of nesting:

if(!isset($_SESSION['IDRole']) || is_null($arr)){
    mysql_close($link);
    return true;
}
[long block of code]

Misc

• return (isset($_SESSION['IDRole'])?true:false can be written as return isset($_SESSION['IDRole']), but it could also be removed, as you never use it (which is probably good, as you yourself set $_SESSION['IDRole'], so it will always be set).
• you repeat $Physicalpath.$fname and $Urlpath.$fname multiple times. I would probably just save it in a variable.