Why does my php search form return everything in the database?

Question

I am writing a database search function for games. I come from a Python background so PHP is very new to me, but I am fluent in HTML/CSS, so I figured it was time to write some PHP. Anyways, my Code should query the database for either the Name of the game, or the System based on the search query. I am using LIKE to accomplish this.

<?php
if(isset($_POST['submit'])){
    if(isset($_GET['go'])){
        if(preg_match("^/[A-Za-z]+/^", $_POST['query'])){
            $query=$_POST['query'];
        }
    }
    $db=mysql_connect ("localhost", "********", "*****") or die ('I cannot connect to the database because: ' . mysql_error());

    $mydb=mysql_select_db("*******");

    $sql="SELECT ID, Name, System FROM Games WHERE Name LIKE '%" . $query . "%' OR System LIKE '%" . $query ."%'";

    $result=mysql_query($sql);

    while($row=mysql_fetch_array($result)){
        $Name=$row['Name'];
        $System=$row['System'];
        $ID=$row['ID'];

    echo "<ul>\n";
    echo "<li>" . "<a href=\"search.php?id=$ID\">" .$Name . " " . $System . "</a></li>\n";
    echo "</ul>";
    }
}

    else{
        echo "<p>Please enter a search query</p>";
    }
?>

And here is my very simple HTML for it

<!DOCTYPE html>
<HTML>

<head>
<link rel="stylesheet" type="text/css" href="assets/style.css">
</head>

<body>
<h1>Search for Games</h1>
<p>You can search by genre, name, or just browse.</p>

<form method="post" action="search.php?go" id="searchform">

<input type="text" name="query">
<input type="submit" name="submit" value="Search">

</form>
</body>

</HTML>

Lastly, here is the website I am running it on so you can see what it does/returns. [removed website due to vulnerabilities]

There are only 3 database entries right now and it doesn't matter if I type in "starfox" or "zelda" I always get all 3 entries returned back to me. What am I doing wrong?

EDIT: I cleaned up my code with mysqli, is this more complete? It works like I want to, but is it safe from SQL Injection?

<?php
if(isset($_POST['submit'])){
    if(isset($_GET['go'])){
            $query=$_POST['query'];
        }
    $db=new mysqli ("localhost", "****", "*****", "******") or die ('I cannot connect to the database because: ' . mysql_error());

    $sql="SELECT ID, Name, System FROM Games WHERE Name LIKE '%" . $query . "%' OR System LIKE '%" . $query ."%'";

    $result = $db->query($sql);

    while($row=$result->fetch_assoc()){
        $Name=$row['Name'];
        $System=$row['System'];
        $ID=$row['ID'];

    echo "<ul>\n";
    echo "<li>" . "<a href=\"search.php?id=$ID\">" .$Name . " " . $System . "</a></li>\n";
    echo "</ul>";
    }
}
    else{
        echo "<p>Please enter a search query</p>";
    }

?>

**Danger**: You are using [an **obsolete** database API](http://stackoverflow.com/q/12859942/19068) and should use a [modern replacement](http://php.net/manual/en/mysqlinfo.api.choosing.php). You are also **vulnerable to [SQL injection attacks](http://bobby-tables.com/)** that a modern API would make it easier to [defend](http://stackoverflow.com/questions/60174/best-way-to-prevent-sql-injection-in-php) yourself from. — Quentin, May 28 '14 at 20:19
Does your query work as a stand-alone in the databases CLI or IDE? — Jay Blanchard, May 28 '14 at 20:21
Ah, I didn't know I was using out of date stuff! I'll look into mysqli and fix that. Thank you. :) — James, May 28 '14 at 20:26

score 3 · Accepted Answer · answered May 28 '14 at 20:23

3

$query is undefined, (or empty), so unless the code:

if(preg_match("^/[A-Za-z]+/^", $_POST['query'])){
        $query=$_POST['query'];
    }

returns true, so your db query is almost certainly

SELECT ID, Name, System FROM Games WHERE Name LIKE '%%' OR System LIKE '%%'

If your select to show all errors you would get a warning.

As an aside, your code is very dangerous, because the query could cause an injection.

EDIT Your form is a $_POST and you are trying to $_GET the values.

answered May 28 '14 at 20:23

FFMG

1,208
1
10
24

We know that the regex won't return true because the question includes sample input. The only value the code is trying to get read from `$_GET` will be in `$_GET` (we can see it in the HTML source code). – Quentin May 28 '14 at 20:26
1

Hmm, the tutorial I was following said that the regular expression was to prevent SQL Injection. But I guess I should be using MySQLi anyways. So I'll change all that and get rid of the regular expression. Thank you. – James May 28 '14 at 20:27
preg_match does not return a boolean, but a position. You should check for preg_match !== false, (note the !== ) – FFMG May 28 '14 at 20:30
I edited my code and posted it, is that more correct? – James May 28 '14 at 20:37
@Vales from a purely technical point of view this is correct and should work. But you are still open to injections... – FFMG May 28 '14 at 20:41
Any ideas on how to fix those vulnerabilities? I am trying with bind_param, but keep getting errors on it. – James May 28 '14 at 21:08
Have a look at [how-do-i-create-a-pdo-parameterized-query-with-a-like-statement](http://stackoverflow.com/a/583348/2800245) – FFMG May 28 '14 at 21:18

score 2 · Answer 2 · answered May 28 '14 at 20:24

Your regular expression matches any string containing a slash, followed by one or more letters, followed by another slash. The search terms you are typing in do not include any slash characters at all so $query will always be an empty string, which will match everything.

You probably want to:

Use / as the delimiter instead of ^ (it is traditional)
Anchor the regex to the start of the string with ^
Anchor the regex to the end of the string with $

Such:

preg_match("/^[A-Za-z]+$/",

Alexandre R. Janini · Answer 3 · 2014-05-28T20:31:32.387

-3

The following line

if(preg_match("^/[A-Za-z]+/^", $_POST['query'])){

should be

if(preg_match("/^[A-Za-z]+$/", $_POST['query'])){

Good luck!

edited May 28 '14 at 20:31

answered May 28 '14 at 20:25

Alexandre R. Janini

498
9
20

Aye, I added the delimitation during debugging because the tutorial had it your way. – James May 28 '14 at 20:28
@Vales the delimitation you had was right and indeed you need one although I would not suggest using `^` for that, regardless that, on this answer, he removed the ending delimitation which will cause it not to compile or fail. – Prix May 28 '14 at 20:31
Sorry. I fixed it. I had switched the `/`and the `^` around. – Alexandre R. Janini May 28 '14 at 20:32

Why does my php search form return everything in the database?

3 Answers3