yeah it does i got all the sql stuff sorta fixed not sure i got all the tables but i think i did i went through it again i think i got them all…
now what i’m doing is working on converting the sql stuff to PDO
Thanks Snoop
The original db script you posted is pretty much nonsense. It looks like someone took some procedural database logic, added a class definition around it, and to make it all work added $this-> in front of everything. It contains no useful error handling and there’s no security.
Also, forget about the pdo db script you posted. It’s 900+ lines of code that doesn’t add any value. The PDO extension itself is well written. About the only useful thing I’ve found you should do for the PDO extension, is extend the PDO class to add a general-purpose query method, that accepts an optional array of input values that determine if you use a prepared or a non-prepared query.
When you use the PDO extension, set the character set to match your database table’s character set, set the error mode to exceptions, set emulated prepared queries to false, and set the default fetch mode to assoc, use a prepared query when supplying external, unknown, dynamic values when the query gets executed, use implicit binding (supply an array of inputs to the ->execute([…]) call), and in most cases let php catch and handle any database exception.
I looked at the add_group.php code you posted, and from an sql standpoint, the name column should be defined as a unique index. You would then just attempt to insert the data and have exception try/catch logic that tests for a duplicate key error number. There is no need for the SELECT query and in fact a race condition exists where concurrent instances of the script can all find that the name value doesn’t exist and will insert a row of data, resulting in duplicates. The design should not be storing the ports data as a comma separated list either. There should be a separate row inserted for each name/port data pair, which would then be defined as a unique composite index.
Next, there’s a serous security hole in this code. By using ‘loged’ (‘yes’) and ‘user_level’ (‘1’) cookies, anyone can become a logged in administrator, simply by setting those cookies to those values.
What the code should be doing is using a unique and cryptographically secure token for the ‘loged’ cookie and querying on each page request to get the current user’s permissions.
The code also contains about four times too much receptive logic, all for the purpose of displaying one of several validation/error messages.
The code also isn’t validating the $_POST[‘ports’] data before using it.
Ok thanks phpdr i’m learning alot but got alot to learn yet lol… thanks for all the info i’m working on the PDO stuff still not sure what i’m going to do either try to find a good PDO db script or try to write one the last one might be a challenge lol… Since i haven’t worked to much with PDO mostly mysql & mysqli… so i don’t know much about it but i’ve been trying to read everything i can about PDO, prepared statements ect.
Thanks Snoop
The following example code shows the points that have been made -
<?php
// add_group.php
// added lang value for the port validation
$lang['error_mess7a'] = 'The port(s) must not be empty.';
// recursive trim call-back function - this should be added to the common.php file
function _trim($val){
if(is_array($val)){
return array_map('_trim',$val);
} else {
return trim($val);
}
}
define("PHPSTATUS_REAL_PATH","./../");
// include is not a function, the () are just clutter, and you should use require for things your code must have in order to work
require PHPSTATUS_REAL_PATH . 'common.php';
$errors = []; // an array to hold user/validation error messages
$post = []; // an array to hold a trimmed working copy of the form data
// using the existing non-secure cookie logic
// if(isset($_COOKIE['loged']) && $_COOKIE['loged'] == 'yes') {
// negated becomes -
if(!isset($_COOKIE['loged']) || $_COOKIE['loged'] != 'yes')
{
// not logged in
$errors['loged'] = 'error_mess6';
} else {
// check user level
//if(isset($_COOKIE['user_level']) && $_COOKIE['user_level'] == '1') {
// negated becomes -
if(!isset($_COOKIE['user_level']) || $_COOKIE['user_level'] != '1')
{
$errors['user_level'] = 'error_mess5';
}
}
// if no user errors, process the post method form data
if(empty($errors) && $_SERVER['REQUEST_METHOD'] == 'POST')
{
// inputs - name , ports (array)
// trim, than validate all inputs
$post = array_map('_trim',$_POST); // note: _trim is a recursive 'trim' user written function
if($post['name'] == '')
{
$errors['name'] = 'error_mess7';
}
// ports is an array of something.
// if it is from checkboxes, it might not be set. you would test that here...
// remove any false (''/0) values
$post['ports'] = array_filter($post['ports']);
if(empty($post['ports']))
{
// no elements after filtering out false values
$errors['ports'] = 'error_mess7a';
}
// if no errors, use the submitted data
if(empty($errors))
{
// as a 1st pass, store data exactly the same as currently being done
$sql = "INSERT INTO {$prefix}_groups (name, ports) VALUES (?,?)";
$stmt = $pdo->prepare($sql);
try { // a 'local' try/catch to handle a specific error type
$stmt->execute([ $post['name'], implode(',',$post['ports']) ]);
} catch (PDOException $e) {
if($e->errorInfo[1] == 1062) // duplicate key error number
{
// 'error_mess8' is for the name already existing
// not sure if 'error_mess9' is specifically for a duplicate error or just any database error.
// if for any database error, you would not bother to tell, even an administrator, about it, since it indicates a programming mistake they cannot do anything about
$errors['name'] = 'error_mess8';
} else {
throw $e; // re-throw the pdoexception if not handled by this logic
}
}
}
// if no errors, success
if(empty($errors))
{
// you would normally do a redirect to the exact same url of the current page
// to display any success message, you would store it in a session variable
// using existing success code -
require "header.php";
$template->getFile(array(
'success' => 'admin/success.tpl')
);
$template->add_vars(array(
'L_SUCCESS' => $lang['success'],
'DISPLAY' => $lang['success_mess1'],
'LINK' => "groups.php")
);
$template->parse("success");
require "footer.php";
exit();
}
}
// display any errors
if(!empty($errors))
{
// not sure if/how the current code would handle displaying multiple error messages
// for demo purposes, just combine the messages into one <br> separated string
require "header.php";
$template->getFile(array(
'error' => 'admin/error.tpl')
);
$msg = []; // array to hold actual error text
foreach($errors as $error)
{
$msg[] = $lang[$error]; // get the error text
}
$template->add_vars(array(
'L_ERROR' => $lang['error'],
'DISPLAY' => implode('<br>',$msg))
);
$template->parse("error");
require "footer.php";
exit();
}Thanks phpdr now i can go through the other files and try to fix em
how can i determine if it is a array and how to handle it?
// ports is an array of something.
// if it is from checkboxes, it might not be set. you would test that here…
here is how the error_messages are setup
$lang[‘error_mess5’] = “You do not have permission to view this page.”;
$lang[‘error_mess6’] = “You are not logged in.”;
$lang[‘error_mess7’] = “You need to fill in a group name”;
$lang[‘error_mess7a’] = ‘The port(s) must not be empty.’;
$lang[‘error_mess8’] = “That group name already exists.”;
$lang[‘error_mess9’] = “Could not add the group.”;
error_mess9 isn’t a db error so should i show an error message for it?
this i’m still not sure about this…
// you would normally do a redirect to the exact same url of the current page
// to display any success message, you would store it in a session variable
or this… lol sorry
// not sure if/how the current code would handle displaying multiple error messages
// for demo purposes, just combine the messages into one
separated string
i’m not sure if it can handle multiple errors.
Thanks Snoop
Hey was wondering if u could look at this file i went through it and tried to make changes i just wanted to see if you could look at it and see if i’m going in right direction lol
Thanks Snoop
The ports input IS an array, based on how it is being handled in the code, i.e. the original join() (implode() in my version.) The issue is if it will be set or not, which will produce a needless php error if it is not. If the form field is an array of type text/number inputs, it will always be set if there’s at least one field, even if the field is empty. If the form field is an array of type checkbox inputs, it may not be set if none of the check boxes are checked.
It is a db error for the INSERT query. The type of things that would cause the INSERT query to fail are programming mistakes (wrong db, table, column names, sql syntax errors, …), duplicate index value (assuming that the name column is defined as a unique index), and out of range values (depending on the database server’s mode.) The error handling that I added takes care of the duplicate index error. For the other two cases, the actual error information needs to be logged on a live/public server, which will happen with the error handling that I used. Telling the user that the group could not be added will leave them wondering what to do, resulting in a bad user experience. For the error handling that I used, they will receive a http 500 error. You can create a custom error page for this, letting the visitor know that the page/action they requested could not be completed (don’t tell them anything specific about the error), and that the site’s owner has been informed
Since I didn’t know the context of the posted code and what affect a redirect would have on it, I use the original in-line success display logic. The reason for doing a redirect to the same page is to cause a get request for that page. This will prevent the browser from trying to re-submit the form data if you reload or navigate back to that page. To change this, you would put the original success code at an appropriate location in the html document (after the error display code), store the success message value ‘success_mess1’ in a session variable (will require a session_start() statement on the page), then execute a redirect to the exact same url of the current page. You would add logic around the success display code to test for the session variable, use the value for the ‘DISPLAY’ tag, clear the session variable, then execute the success display logic.
You could try to see. If you leave both the name and ports inputs empty, you should get two user/validation errors. It would take knowing what the html markup/css is for that part of the output.
LOL. Probably due to the existence of $_COOKIE variables in the code, which, given the security hole in how they are being used, IS pretty darn offensive.
The add_lang.php code will be virtually identical to the add_group.php code, i.e. it’s performing the same steps, just on different input data and a different table name, with the following differences -
- There’s only one input, the name. The ports validation logic and usage in the sql code would be removed.
- The database table name in the sql query would be changed.
- Any add_lang specific error_mess… and success_mess… values would be used.
- The ‘LINK’ tag value in the success display code would be changed.
It should look like this (I marked the areas that are different with a string of *******) -
<?php
// add_lang.php
define("PHPSTATUS_REAL_PATH","./../");
require PHPSTATUS_REAL_PATH . 'common.php';
$errors = []; // an array to hold user/validation error messages
$post = []; // an array to hold a trimmed working copy of the form data
if(!isset($_COOKIE['loged']) || $_COOKIE['loged'] != 'yes')
{
// not logged in
$errors['loged'] = 'error_mess6';
} else {
if(!isset($_COOKIE['user_level']) || $_COOKIE['user_level'] != '1')
{
$errors['user_level'] = 'error_mess5';
}
}
// if no user errors, process the post method form data
if(empty($errors) && $_SERVER['REQUEST_METHOD'] == 'POST')
{
// inputs - name **************** remove the ports input
// trim, than validate all inputs
$post = array_map('_trim',$_POST); // note: _trim is a recursive 'trim' user written function
if($post['name'] == '')
{
$errors['name'] = 'error_mess7'; // ********** change this to the add_lang name empty value
}
// **************** remove the ports validation logic
// if no errors, use the submitted data
if(empty($errors))
{
// *************** change the table name and remove the ports column and place-holder
$sql = "INSERT INTO _lang (name) VALUES (?)";
$stmt = $pdo->prepare($sql);
try { // a 'local' try/catch to handle a specific error type
$stmt->execute([ $post['name'] ]); // **************** remove the ports value
} catch (PDOException $e) {
if($e->errorInfo[1] == 1062) // duplicate key error number
{
$errors['name'] = 'error_mess8'; // *************** change this to the add_lang name already exists value
} else {
throw $e; // re-throw the pdoexception if not handled by this logic
}
}
}
// if no errors, success
if(empty($errors))
{
// you would normally do a redirect to the exact same url of the current page
// to display any success message, you would store it in a session variable
// using existing success code -
require "header.php";
$template->getFile(array(
'success' => 'admin/success.tpl')
);
$template->add_vars(array(
'L_SUCCESS' => $lang['success'],
'DISPLAY' => $lang['success_mess1'], // ************** change this to the add_lang specific value
'LINK' => "groups.php") // ************** change this to the add_lang specific value
);
$template->parse("success");
require "footer.php";
exit();
}
}
// display any errors
if(!empty($errors))
{
// not sure if/how the current code would handle displaying multiple error messages
// for demo purposes, just combine the messages into one <br> separated string
require "header.php";
$template->getFile(array(
'error' => 'admin/error.tpl')
);
$msg = []; // array to hold actual error text
foreach($errors as $error)
{
$msg[] = $lang[$error]; // get the error text
}
$template->add_vars(array(
'L_ERROR' => $lang['error'],
'DISPLAY' => implode('<br>',$msg))
);
$template->parse("error");
require "footer.php";
exit();
}This begs the question, how many add_… or edit/update action files are there, that only differ in the number/name of inputs, table/column names, and some message values? You would normally use a data driven design, where you have a data structure (array or database table) that defines what general-purpose code on the page does. Once you get to this point, you would move toward having a single, general-purpose, file of code that accepts a ‘module’ input (group, lang, …), that tells it to use the correct defining data structure.
Well there is 7 add 5 delete, 6 edit 2 group, 2 lang, 2 server , 1 service, 1 services, 1 setting, 2 theme.
i’m gonna start going through the other ones and try fixing them too
there is 32 files that is just the one directory not sure what else there is in the other files… lol
Thanks Snoop