current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I'm just writing my first PHP-Mysqli sample (think about a Wiki 0.0.1) and I would like to ask you if this example is secure or not or if there are any other problems/suggestions you might recommend?

I would like to use prepared statements and not care about sanitizing the input from $_GET, but I don't know if this code is considered secure and OK?

Also, any comments regarding how this functionality (reading a latest revision from a database) should be done is welcome.

require_once 'connect.php';

$id = $_GET['id'];

if ( $stmt = $mysqli->prepare("SELECT text, rev FROM wiki WHERE id = ? ORDER BY rev DESC LIMIT 0,1") ) {

    $stmt->bind_param( "s", $id );
    $stmt->execute();
    $stmt->bind_result( $text, $rev );
    $stmt->fetch();

    echo "rev: " . $rev . ": " . $text;
    echo '<br>';

    $stmt->close();
}

$mysqli->close();

connect.php

$mysqli = new mysqli("localhost", "xxx", "yyy", "zzz");

if (mysqli_connect_errno()) {
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
}

Is it safe to keep connect.php at the same folder as the other php files?

Is there any chance someone could read the plain password in connect.php file?

share|improve this question
    
Don't print errors out so they are visible. 1) It has no meaning to a user (my mum has no idea what a connect failure is nor how to resolve the problem) and makes your site look bad. 2) It provides information to attackers that they do not need. Errors messages should be placed in the log file. The only error message you should display to a user an apology (with an optional guid (that you generate)) and a potential solution (like pleas try latter). If they complain you can use the guid to look up the real error in the log and then resolve the issue. –  Loki Astari Apr 17 '12 at 0:33

2 Answers 2

up vote 1 down vote accepted

Yes, that looks good.

Minor comment: Normally an id is an integer. Should it have been "i" in your bind_param?

Connection Details

First, I am not a security expert. However, I would recommend storing the connection details in a location that is not served by your web-server. It should be somewhere in your PHP include_path or accessible from your autoloader.

Having the file in a directory accessible from your web-server could open up the connection details if you had an incorrectly configured web-server.

share|improve this answer
    
No it's a string, thanks! –  zsero Apr 17 '12 at 13:27
up vote 1 down vote

Verify the id to be an integer. One way to do that is like

$id = (int)$_GET['id'];
if($id > 0) {
 .... //proceed
}

Next, the parameter should be bind using i not s

$stmt->bind_param( "1", $id );
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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