3
\$\begingroup\$

In my db MySQL, I have a table ranks with many fields, one for each page I want limit access for the user in the menu application with PHP control:

  <?php if ($row_ranks['padric'] == '1' ) {
    echo ('<li><a href="padroncini_ric.php">Ricerca</a></li>');
        } 
        else  {
        echo ('<li><a href="#" class="disabled">Ricerca</a></li>');
        } 
  ?>

In the admin panel, I have a page for seting the privileges to single page with relative checkbox.

The checkbox as a name equal at field name in table like this example:

<div>
<input type="checkbox" name="padric" value="1"
<?php if ($row_user_ranks['padric'] == 1) { echo "checked"; } ?>
>Ricerca
</div>

For set the privileges I make this PHP code:

    mysql_select_db($database_geomo, $geomo);
    $query_RSfields = sprintf("SHOW COLUMNS FROM ranks WHERE Field NOT IN ('ID', 'userID', 'gruppo')");
    $RSfields = mysql_query($query_RSfields, $geomo) or die(mysql_error());
    $row_RSfields = mysql_fetch_assoc($RSfields);
    $totalRows_RSfields = mysql_num_rows($RSfields);
    do { 
     $field = $row_RSfields['Field'] ;  

     if (!isset($_POST[$field])) { // set value = 0 for checkbox not checked
         $_POST[$field] = '0' ;
        }

    $updateSQL = sprintf("UPDATE ranks SET $field='".$_POST[$field]."' WHERE userID='".$_POST['ID']."'"); // set privileges for user
    mysql_select_db($database_geomo, $geomo);
    $Result1 = mysql_query($updateSQL, $geomo) or die(mysql_error());

    } while ($row_RSfields = mysql_fetch_assoc($RSfields));

I've made this code in this way because if the checkbox is not checked, it shouldn't return a value and should generate an error "unknown column" for the update SQL.

So, there is another more simply way for doing this job?

\$\endgroup\$
  • 2
    \$\begingroup\$ The very first thing you can do is stop using mysql_query. It has been deprecated for a while now, and only dinosaurs, "24 hours" noobs, and w3schools dropouts still use it. \$\endgroup\$ – cHao Feb 14 '14 at 14:32
  • \$\begingroup\$ @cHao after your suggestion i've made a code review with MySqli connection. \$\endgroup\$ – geomo Feb 15 '14 at 11:10
  • \$\begingroup\$ Your code is vulnerable to SQL-Injection. \$\endgroup\$ – Bobby Feb 15 '14 at 11:13
  • 2
    \$\begingroup\$ @geomo: Let me tell you one thing: If it can be broken, it will be broken. \$\endgroup\$ – Bobby Feb 15 '14 at 12:20
  • 1
    \$\begingroup\$ @SimonAndréForsberg: Yeah, I'll draft something together...now where was that gif, just to make sure... \$\endgroup\$ – Bobby Feb 15 '14 at 15:26
3
\$\begingroup\$

DO NOT USE THE mysql_* FUNCTIONS!

They are DEPRECATED.


<?php if ($row_ranks['padric'] == '1' ) {
  echo ('<li><a href="padroncini_ric.php">Ricerca</a></li>');
      } 
      else  {
      echo ('<li><a href="#" class="disabled">Ricerca</a></li>');
      } 
?>

Your indentation style is a mess, decide what indentation style to use and if to use tabs or spaces (and if yes how many) and then stick to it:

<?php
    if ($row_ranks['padric'] == '1' ) {
        echo '<li><a href="padroncini_ric.php">Ricerca</a></li>';
    } else  {
        echo '<li><a href="#" class="disabled">Ricerca</a></li>';
    } 
?>

Normally there's also no space between the line and the closing semicolon.

Additionally the echo function does not need any parentheses.


if ($row_ranks['padric'] == '1' )

If your code is in your native language, please always convert it to English before posting it here for review. It makes things easier for us.

Now what you use here is called a magic string or magic number, it's a fixed value that has some meaning but does not provide any explanation what's-o-ever what that meaning is or why it is that way. Use named constants instead.

const SOME_CONDITION = "1"; // Change the name to something meaningful.

if ($row['padric'] == SOME_CONDITION) {

Also do you now the difference between the equality and identity operator? The equality operator == will perform casting as needed which can yield interesting effects, the identity operator === on the other hand will also compare the types.

Some examples:

0 == 0     -->  true
0 == "0"   -->  true
0 == ""    -->  true
0 == NULL  -->  true

0 === 0     -->  true
0 === "0"   -->  false
0 === ""    -->  false
0 === NULL  -->  false

So always sue === unless you have a compelling reason not to.


$_POST[$field] = '0' ;

First of all, global variables are bad. And after that, writing into the Super-Globals is a bad idea for many reasons. Nobody expects that super globals are modified in any way and that can lead to funny errors which you'll be looking for for days.

Extract the values from the super globals and work with that variable.


$updateSQL = sprintf("UPDATE ranks SET $field='".$_POST[$field]."' WHERE userID='".$_POST['ID']."'"); // set privileges for user

One word (okay, it's two): SQL Injection. There is no, absolutely no (and I mean no) excuse to allow SQL injection. Imagine somebody would manipulate the POST request to contain the value '; DROP TABLE ranks; -- which will yield the query:

UPDATE ranks SET field='field' WHERE userID=''; DROP TABLE ranks; --'

...any questions?

Why are you using sprintf here?


$row_RSfields = mysql_fetch_assoc($RSfields);
do {
    // code 
} while ($row_RSfields = mysql_fetch_assoc($RSfields));

Traditionally it's a while loop:

$result = getResult();
while ($row = fetchRow($result)) {

}

Now let's get to the core problem: You are using the mysql_* functions. You should either use mysqli_* or PDO.

Personally I prefer PDO over mysqli_* because it has an object oriented interface and support multi-statements. So here's an example on how to use PDO:

/**
 * 
 * @var PDO
 */
$pdo = new PDO($dsn, $user, $password, $options);
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); // See footnote!

/**
 *
 * @var PDOStatement
 */
$statement = $pdo->prepare("SELECT field FROM table WHERE id = :id;");
$statement->bindValue(":id", $id, PDO::PARAM_INT);

$statement->execute();

while ($row = $statement->fetch()) {
    echo $row['field'];
}

$statement->closeCursor();

Question on Stack Overflow about when to turn off emulated prepares.


Are you aware of the difference between strings?

'A simple string'
"A string with automatic $variable expansion"
\$\endgroup\$
  • \$\begingroup\$ One word (okay, it's two): code review (with mysqli). About SQL Injection : no way for doing that in this case because there aren't input field but only checkbox checked or not and the value are 1 or 0. More : all page have php require_once for function.php with get_magic_quotes_gpc() for "cleaning" the value. \$\endgroup\$ – geomo Feb 15 '14 at 16:24
  • 1
    \$\begingroup\$ @geomo Just because you aren't able to inject some SQL doesn't mean that someone else can't! \$\endgroup\$ – Simon Forsberg Feb 15 '14 at 16:39
  • 1
    \$\begingroup\$ @geomo: Everything that comes from the other side of the connection, no matter if it comes from GET, POST or a cookie is never to be trusted. Everything that comes from the outside can be forged. \$\endgroup\$ – Bobby Feb 15 '14 at 17:18
  • \$\begingroup\$ @Bobby You are a great coder.. tnx for all. \$\endgroup\$ – geomo Feb 15 '14 at 17:37
  • \$\begingroup\$ @Bobby I read in to another answer this : "The PDO internally builds the query string, calling mysql_real_escape_string() (the C API function) on each bound string value." Now, if mysql_* function are deprecated, the PDO call mysqli_* function instead ? \$\endgroup\$ – geomo Feb 17 '14 at 15:06

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.