1

I am using isset() to check if a variable has a value. My issue is that even when the variable has no value (the parameter was not passed in the url) the sql string is still modified to append the where clause.

For example - if this is the URL that is passed

http://internal/test/trbm?userID=3213

And this is my php snippet

$user = urldecode($_GET['userID']);

$sql = "Select * from users"
if (isset($user)) {
    $sql .= " WHERE userID = '$user'";
}

echo $sql;

The echo results are:

Select * from users where userID = '3213'

However, this URL http://internal/test/trbm produces the below echo results, still appending the where clause, even though (at least to me) isset() should return false

Select * from users where userID = ''

The second echo statement (the one right above) my desired/expected to be is

Select * from users

Was isset() the incorrect check to use in this instance? If so, what check should I be using in it's place?

  • Try this query `$sql .= " WHERE userID = '.$user.'";` – Ayaz Ali Shah Jul 12 '17 at 13:37
  • 3
    Use `!empty($user)` because the variable `$user` is always set since `urldecode()` will return an empty string when its input is null. – Michael Berkowski Jul 12 '17 at 13:38
  • Somewhere there's a "definitive guide to isset and empty" post, which I'll attempt to find and link this against as a duplicate. – Michael Berkowski Jul 12 '17 at 13:39
  • Isset check if is variable set. Try empty() function. `$user = urldecode($_GET['userID']);` - $user will be always set, because it result of function urldecode(). `$_GET['userID'])` can be set or not, so you can check `isset($_GET['userID']))`. – Ivan Bolnikh Jul 12 '17 at 13:39
  • 1
    Please also review [How can I prevent SQL injection in PHP](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php#60496) - the value of `$_GET['userID']` even though urlencoded, is still vulnerable to SQL injection via `UNION` queries. – Michael Berkowski Jul 12 '17 at 13:44
  • 1
    @Mr.Developer That will just cause errors. You're not concating properly. And even if you did, it wouldn't change anything. – Qirel Jul 12 '17 at 13:53
  • Not sure why nobody has mentioned that there's no reason to use PHP globals in Joomla! – Lodder Jul 15 '17 at 23:32

5 Answers5

2

$user will be set, because you set it in this line

$user = urldecode($_GET['userID']);

So instead check the $_GET value directly.

$sql = "Select * from users"
if (isset($_GET['userID']) {
    $user = urldecode($_GET['userID']);
    $sql .= " WHERE userID = '$user'";
}

echo $sql;
RiggsFolly
  • 93,638
  • 21
  • 103
  • 149
2

....and the proper way, using Joomla's JInput class:

$userId = JFactory::getApplication()->input->get('userID', '', 'STRING');
$sql    = 'Select * from users';

if (isset($userId)
{
    $sql .= ' WHERE userID = ' . urldecode($userId);
}

echo $sql;
Lodder
  • 19,758
  • 10
  • 59
  • 100
1

Run isset() to check if the $_GET var has been submitted, if so set the $user variable to it, otherwise leave it blank

$user = '';
if (isset($_GET['userID'])) {
    $user = urldecode($_GET['userID']);
}

$sql = "SELECT * FROM users";

if ($user != '') {

    $sql .= " WHERE userID = '$user'";

}

echo $sql;
Kevin P
  • 601
  • 3
  • 9
1

Your problem is that your $user variable IS set, no matter what, because you just set it above. No matter if it's empty or not, it IS set. You just need to check if the $_GET variable is set in the IF statement, and set the $user variable inside that IF statement.

Another way to do this is to use PHP's empty() function - that is assuming the variable isn't supposed to be empty anyways. Empty is not the same as NULL

It is much better to use this check if the variable MUST equal SOMETHING for the page to work (Especially with an ELSE statement with directions for if the variable IS empty), rather than using isset, as it does not have the same checks.

Here is the function bool empty ( mixed $var )

Here is a list of things considered empty:

  • "" (an empty string)
  • 0 (0 as an integer)
  • 0.0 (0 as a float)
  • "0" (0 as a string)
  • NULL
  • FALSE
  • array() (an empty array)
  • $var; (a variable declared, but without a value)

You can learn more at php.net

You will want to check if the variable is empty before setting it to your page variable.

In this case I suggest checking if its NOT empty, and if it's not, then set the variable.

if(!empty($_GET['userID'])) {
    $user = $_GET['userID'];
    $sql .= " WHERE userID = '$user'";
}

You could put an else statement there too to redirect/show an error that there is no userID if you wanted to.


You could of course do the same with isset, but it checks for less things.

You also reverse the direction of the if statement (As in, using empty you want the NOT operator ! but for isset you do not want that.), as you want to make sure it IS set to set the variable.

if(isset($_GET['userID'])) {
    $user = $_GET['userID'];
    $sql .= " WHERE userID = '$user'";
}

You could put an else statement there too to redirect/show an error that there is no userID if you wanted to.

GrumpyCrouton
  • 8,486
  • 7
  • 32
  • 71
0

You can use simply this

if($_GET['userID']) {
    $user = $_GET['userID'];
    $sql .= " WHERE userID = '$user'";
}
Sunil K Samanta
  • 161
  • 1
  • 8