Jump to content

Looking for a quick critique


moltm4785

Recommended Posts

Wondering if the below can be made any more efficient and just for those wondering I like writing my own functions I don't like using frameworks. 
 

 

  mb_internal_encoding("UTF-8");
    session_start();
    $ROOT_URL = $_SESSION['ROOT_URL'];
    $ROOT_DIR = $_SESSION['ROOT_DIR'];
    include $ROOT_DIR . 'dbconnection/dbconnection.php';
    //UNSETS THE TABLE NAME AND ID SO IT ISN'T PART OF THE LOOP
    $current_table = $_POST['table_name'];
    $SALT = 'xxxxxxxxxx';
    $date_time = date('Y/m/d H:i');
    $column_names_array = array();
    $column_values_array = array();
    $query = $mysqli -> query("SHOW COLUMNS FROM `$current_table`;");
    while ($fetch = $query -> fetch_array())
    {
        $_POST[$fetch[0]] = (!is_array($_POST[$fetch[0]])) ? $mysqli->real_escape_string($_POST[$fetch[0]]) : $_POST[$fetch[0]];
        if (preg_match('/(created|modified)/', $fetch[0]))
        {
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $date_time);
        }
        elseif (is_array($_POST[$fetch[0]]))
        {
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $mysqli -> real_escape_string(join(',', array_unique($_POST[$fetch[0]]))));
        }
        elseif (!empty($_POST[$fetch[0]]))
        {
            $_POST[$fetch[0]] = (preg_match('/password/i', $fetch[0])) ? sha1($SALT . $_POST[$fetch[0]]) : $_POST[$fetch[0]];
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $_POST[$fetch[0]]);
        }
    }
    $query = "INSERT INTO " . $current_table . "(`" . join("`,`", $column_names_array) . "`) VALUES ('" . join("','", $column_values_array) . "');";
    $query_result = ($mysqli -> query($query)) ? true : false;
    echo $query_result;
 

 

Edited by Maq
Link to comment
Share on other sites

Efficiency? You have security problems - only a couple but they're big. Deal with those before you start thinking about efficiency.

 

1. Table name isn't validated.

2a. Arrays aren't sanitized.

2b. I could bypass your string sanitizing by renaming textboxes into arrays with one element.

 

As for a critique,

1. Not a fan of storing root paths in the session. Where are they set anyways?

2. The code that supposedly "unsets the table name" doesn't (but it doesn't matter unless there's a "table_name" column in the table).

3. Don't include the semicolons in queries.

4. Not a fan of looking for keywords like "created" or "modified" or "password".

5. array_unique() the POST values? What if the values are numbers like "1,1,2" and I want all three?

6. You may not like frameworks but you should use one. Even if it's your own.

 

And use [code] tags when you post code.

Link to comment
Share on other sites

 

Efficiency? You have security problems - only a couple but they're big. Deal with those before you start thinking about efficiency.

 

1. Table name isn't validated.

2a. Arrays aren't sanitized.

2b. I could bypass your string sanitizing by renaming textboxes into arrays with one element.

 

As for a critique,

1. Not a fan of storing root paths in the session. Where are they set anyways?

2. The code that supposedly "unsets the table name" doesn't (but it doesn't matter unless there's a "table_name" column in the table).

3. Don't include the semicolons in queries.

4. Not a fan of looking for keywords like "created" or "modified" or "password".

5. array_unique() the POST values? What if the values are numbers like "1,1,2" and I want all three?

6. You may not like frameworks but you should use one. Even if it's your own.

 

And use [code] tags when you post code.

 

I agree thats what I'm building is my own framework. First Thanks for the info but two things 

1. Your right I don't really need to unset the table but for other reasons

2. Can you go into more detail on 2b of first section I had it setup to sanitize everything regardless of what it was but the array would error everything out and so I set it up so that it gets bypassed and then sanitized within the if statement of if(isarray($var))

 

I will go ahead and put those changes you talked about, also I have my full paths starting at the index page itself. Is there a better way to set your paths in one spot and then allow them to propgate throughout the entire site without using sessions?

Edited by moltm4785
Link to comment
Share on other sites

So here is the code with changes, I highlighted what I was talking about with the sanitizing factor. Something that always confused me if $current_table is empty or doesn't exist the page will still run fine and the point or error would still be the same would it not, the query is going to error out. I get its good coding but is that generally a good take on it?

 

 

<?php
    mb_internal_encoding("UTF-8");
    session_start();
    $ROOT_URL = $_SESSION['ROOT_URL'];
    $ROOT_DIR = $_SESSION['ROOT_DIR'];
    include $ROOT_DIR . 'dbconnection/dbconnection.php';
    $current_table = $_POST['table_name'];
    $SALT = 'aB1cD2eF3G';
    $date_time = date('Y/m/d H:i');
    $column_names_array = array();
    $column_values_array = array();
    $query = $mysqli -> query("SHOW COLUMNS FROM `$current_table`;");
    while ($fetch = $query -> fetch_array())
    {
        $_POST[$fetch[0]] = (!is_array($_POST[$fetch[0]])) ? $mysqli->real_escape_string($_POST[$fetch[0]]) : $_POST[$fetch[0]];
        if (preg_match('/(created|modified)/', $fetch[0]))
        {
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $date_time);
        }
        elseif (is_array($_POST[$fetch[0]]))
        {
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $mysqli -> real_escape_string(join(',', $_POST[$fetch[0]])));
        }
        elseif (!empty($_POST[$fetch[0]]))
        {
            $_POST[$fetch[0]] = (preg_match('/password/i', $fetch[0])) ? sha1($SALT . $_POST[$fetch[0]]) : $_POST[$fetch[0]];
            array_push($column_names_array, $fetch[0]);
            array_push($column_values_array, $_POST[$fetch[0]]);
        }
    }
    $query = "INSERT INTO " . $current_table . "(`" . join("`,`", $column_names_array) . "`) VALUES ('" . join("','", $column_values_array) . "')";
    $query_result = ($mysqli -> query($query)) ? true : false;
    echo $query_result;
?>
Edited by moltm4785
Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.