PHP form and DB query input validation

Question

(retitled this from "Testing for a non-empty query in a GET request?", since that ended up not really being the important part of my question by the time I was done writing it.)

I'm helping a genealogist friend with a simple DB search script. It's fine if users leave all but one input field blank. I'd like to distinguish between 3 cases:

1. someone browsing to the URL with no query parameters
1. form submission with all fields empty
1. form submission with at least one field non-empty

I can probably treat 1 and 2 the same way; this doesn't need to be fancy.

The code previously had an if (isset($_POST['submit'])), and apparently used to work. I want to use a GET query, so people can bookmark searches. I first assumed I could just test $_GET['submit'], but it is never set. For a GET query, I assume I would need a hidden text field called submit in the form to produce a ?submit= in the query URL? Cluttering the URL further isn't what I want, so I guess I'll discard this idea.

Google found a LOT of hits for isset($_GET['submit']), on forums and so on. Usually other bad code in the question caught people's attention, so I never found any discussion of actually using that construct with GET queries. So as a sidetrack to the main question, I'm a curious about how isset($_GET['submit']) became semi-common. Is that usually just from naive people like me changing POST to GET?

Since I'm new to PHP, I'd really appreciate it if anyone could point out any other problems in this snippet of the search script. I'm pretty sure it's wide open to SQL injection, so I need to take the output of an escape function to sanitize them. Is it common to also check the length of input strings, too? The form that submits queries to this script has length limits.

(I know SO doesn't welcome code-review questions, so feel free to ignore this part of my question I guess.)

msearchresultlist.php:
receives queries like ...php/?Surname=Cordes&MaidenName=&GivenName=&Obit= from the form (included at the end of this code block). It prints out a table of matches.
One entry in each row is a hyperlink to a details page for that obituary.

<?PHP
... check that user is logged in to members area ...

if (some_condition_discussed_above) {
    ... open the DB, with hard-coded username/password :( ...

    $GivenName = $_GET['GivenName'];
    if ($GivenName == "")
        {$GivenName = '%';}

    $Surname = $_GET['Surname'];
    if ($Surname == "")
        {$Surname = '%';}

    $MaidenName = $_GET['MaidenName'];
    if ($MaidenName == "")
        {$MaidenName = '%';}

    $Obit = $_GET['Obit'];

    $result = mysql_query ("SELECT * FROM obits
    WHERE GivenName LIKE '%$GivenName%'
    AND MaidenName LIKE '$MaidenName%'
    AND Surname LIKE '$Surname%'
    AND Obit LIKE '%$Obit%'
    ORDER BY Surname ASC, GivenName ASC");
} else {
    $noquery = true;
}
?>


<!DOCTYPE html>
<html lang="en">
... some stuff on the page


<?php

if ($noquery) {
    print "<p>No query found.</p>";
} else {

    print "<table width='600' cellpadding='10px'>";

    if($row = mysql_fetch_array($result)) {
    do {

    print "<tr>";
        print "<td>".$row['Surname']."</td>";
        print "<td>".$row['MaidenName']."</td>";
            print "<td>" . '<a href="msearchresultform.php?ID='.$row['ID'].'">'.$row['GivenName'].'</a>'. "</td>";

        print "<td>".$row['DOD']."</td>";
        print"</tr>";
    } while($row = mysql_fetch_array($result));


---- the form, on another page:
<form action=msearchresultlist.php method=GET>

When searching for a name with an apostrophe, such as O'Neil, use a double apostrophe, ie. O''Neil, not a quote but a double apostrophe. </br>
</br>

Search for:
<p>Last Name: <input type=text name=Surname size=15 maxlength=15>
<p>Maiden Name: <input type=text name=MaidenName size=15 maxlength=15>
<p>Given Name: <input type=text name=GivenName size=15 maxlength=15>
<p>Use % in front of each word in the Obit field, ie %Bridgewater% %Joudrey%
<p>Obit: <input type=text name=Obit size=50 maxlength=50>
<p>
<input type=submit>

I'm aware that a search could dump the entire database by searching on a single wildcard, or something.I think this is my friend's desired result, but I don't want it to happen by accident. i.e. only if the user explicitly searches on a wildcard, not just submits an empty form. This advanced-search script is only available to authenticated users. There's a surnames-only version that's public (at nsobits.ca)

Anyway, in case it wasn't clear what I'm asking: Is there something clever I can do to check that something was filled out in the form? Like some syntax that checks for $_GET having a non-zero-length value for at least one of its keys? Or should I just do if ($GivenName || $Surname || ...)?

yes, it's wide open to sql injection attacks. you should be using a fulltext index for searching like this, and definitely read up on injection defense techniques: http://bobby-tables.com). e.g. stop using mysql_*() functions, and switch to PDO + prepared statements with placeholders — Marc B, Jul 06 '15 at 21:18
There's a lot of srtuff here which makes the question as a whole too broad for [so]. You're right about possibly using [codereview.se], but check their help centre before you post there. Before doing _anything_ else it might pay you to read the [PHP manual on handling forms.](http://php.net/manual/en/tutorial.forms.php) — , Jul 06 '15 at 21:19
Yeah, sorry I have ADHD and can never manage to stop at just one simple question. I always have to make it more complicated! — Peter Cordes, Jul 06 '15 at 21:20
I don't understand why you care so much about `$_GET['submit']` being filled or not. I agree with your earlier statement that use cases 1 & 2 are the same if you don't care whether someone got to the search page via form or other method. This would alleviate any need for some kind of `$_GET['submit']` value. — Mike Brant, Jul 06 '15 at 21:51
@MikeBrant: yeah, by the time I'd thought through needing to validate inputs for cases when the user did submit the form, I had the same conclusion. What I'm still curious about is why you see this `$_GET['submit']` construct in php questions all over the web. Do people really write forms with hidden elements just to have a `submit=` in the query? What's up with that? — Peter Cordes, Jul 06 '15 at 21:57

score 2 · Accepted Answer · edited May 23 '17 at 12:33

The first thing I'd like to mention is the use of the old, and deprecated mysql*()_ functions. You should really be using the PDO or MySQLi libraries instead. There's a really good reply on both how to prevent SQL injections, and how to properly use PDO/MySQLi. Which I can highly recommend reading.

As for the code, this is approximately how I'd do it: (My comments are prefixed CF)

// ... open the DB, with hard-coded username/password :( ...

/**
 * Parses the input, if any, and returns an iterative with the results.
 * @param PDO $db
 * @return boolean|multitype:
 */
function parse_submit($db)
{
    // ... check that user is logged in to members area ...
    if (! isset($_GET['submit_btn'])) {
        // CF: False == No form submitted. We're testing for this specifically later.
        return false;
    }

    // CF: Used for taking care of input, and checking whether we have input.
    $params = array();
    $submitted = false;

    // CF: Preferably add some proper input validation here, to make sure you get what could be a name.
    if (empty($_GET['GivenName'] == "")) {
        $params[':given'] = '%';
    } else {
        $params[':given'] = '%' . $_GET['GivenName'] . '%';
        $submitted = true;  // Update: Was missing this line in my original posting.
    }

    /*
     * CF: Do the rest of the params as above.
     */

    // CF: Here we test for empty submission.
    if (! $submitted) {
        // Return empty array to simulate a non-hit query.
        return array();
    }

    $stmt = $db->prepare("SELECT * FROM obits
        WHERE GivenName LIKE :given
        AND MaidenName LIKE :maiden
        AND Surname LIKE :surname
        AND Obit LIKE :obit
        ORDER BY Surname ASC, GivenName ASC");
    return $stmt->exec($params);
}

/**
 * Shortcut version of htmlspecialchars ()
 * 
 * @param string $string
 * @return string
 */
function hs($string)
{
    return htmlspecialchars($string, null, 'utf-8');
}

$persons = parse_submit($db);

?>
<!DOCTYPE html>
<html lang="en">
... some stuff on the page

<?php

if ($persons === false) {
    print "<p>No query found</p>";
} elseif (empty($persons)) {
    print "<p>No search results found.</p>";
} else {
    // CF: Using a output variable and a template to keep things clean.
    $output = "<table>";
    $outTemplate = <<<EOHTML
    <tr>
        <td>%1\$s</td>
        <td>%2\$s</td>
        <td><a href="msearchresultform.php?ID=%3\$d">%4\$s</a></td>
        <td>%5\$s</td>
    </tr>

EOHTML;

    // CF: Since PDO result sets are iterative using PHP's functions, we use it directly here.
    foreach ($persons as $r) {
        $output .= sprintf($outTemplate, hs($r['Surname']), hs($r['MaidenName']), $r['ID'], hs($r['GivenName']), hs($r['DOD']));
    }

    echo $output . "</table>";
}

For this to work, you'll need to change the action attribute of the form tag to get. To reflect the change in methods of sending the data to the server, and consequently the name of the array you're trying to use. I also recommend changing the name of the submit button to something else than just "submit", in order to avoid problems with the DOM event.

The "clever" thing you're asking about, is the $submitted variable in my example. ;)

Last area of improvement I have for now, is the use of coding style. In your example there is no clear style applied, with the curly braces more or less randomly placed. It's not too important which coding style you adhere to, but pick one and stick to it religiously. You'll save a lot of headache for both yourself and others in the future by doing so. The recommended style at this point in time is PSR-2, which is what most of the major PHP libraries are using.

When you get more experienced with PHP you might also want to look at using template engines, and frameworks. They'll help you a lot in becoming more productive, as long as you have all of the basics down pat.

Post edit update: I see that you haven't named your input control at all, which is why you don't see a "submit=" key-value parameter in the URL.

PS: Gold star for being security conscious!

coding style: At least code inside flow-control is indented, unlike before I touched it. >.< Thanks for the detailed look at the code. Also, thanks for solving the `submit=` mystery! I was really puzzled why it wasn't working for me, and a couple other posters I saw, yet it seemed to be a common idiom. — Peter Cordes, Jul 06 '15 at 22:03
Ah, and I like your change of putting the code at the top of the file into a function, instead of setting a global variable. I didn't want to just move the code down to where the result was used, but I also thought my global was ugly. Yeah, that whole thing looks great. This is probably just a one-off effort for me, since making forms isn't what I usually do, so it's extremely helpful to have some code that operates along the right lines. — Peter Cordes, Jul 06 '15 at 22:07

score 0 · Answer 2 · answered Jul 06 '15 at 21:25

0

You can use:

if(isset($_GET['variableName']) AND !empty($_GET['variableName']))

This will check it is set and is not an empty string.

Alternatively, this function will loop through $_GET (which is an array) and return true if it finds any unfilled values:

function fieldsEmpty(){
    foreach($_GET as $key => $value){
        if(empty($value)){
            return true;
        }
    }
}
if(fieldsEmpty()){
    // handle error
}

answered Jul 06 '15 at 21:25

HenryTK

1,287
8
11

My question was really about when `$_GET['submit']` would EVER be set. It isn't set even when you submit a query. Oh, I forgot to include the html for the form in my question. But I assume it would only be set if you constructed the form so that the query URL was longer, and included `?submit=Submit` or something. `fieldsEmpty()` is what I was thinking I might end up doing, yeah. Thanks. – Peter Cordes Jul 06 '15 at 21:30
What method attribute in the form are you using? Useful article: http://www.w3schools.com/tags/att_form_method.asp – HenryTK Jul 06 '15 at 21:35
editted the question to include the HTML form posted at the end of the code block. I like how the instructions say to use two single quotes for a name like O'Neil. Probably otherwise you're SQL-Injecting part of your field. >. – Peter Cordes Jul 06 '15 at 21:37
1

`$_GET['submit']` would only be set if you had a field called that, and there really is no point in that. You can use `!empty($_GET)` to test whether any query parameters were passed. – HenryTK Jul 06 '15 at 21:56

PHP form and DB query input validation

2 Answers2