[RESOLVED] Generated output

HerbRodenhaber

all comments welcome ... im looking to streamline the code as much as posible
thanks

see attached file

quinxorin

The code looks pretty good, but one problem:

SQL Injection.

This line is quite problematic:

$drgcid="WHERE $filter='$data' ORDER BY $sort $direction LIMIT $pagestart,$limit";

Unless this is a page that can only be accessed by admins, you need to validate those variables before passing them to this function. Otherwise, the user could pass a couple of specially crafted strings to the GET variables, and voila. Complete access.

bradgrafelman

First and foremost, you need to learn/develop a coding standard and stick to it (in terms of how the code is written). Pick a method of consistent indentation and stick with it (your editor should be doing this for you - if it isn't then you aren't using an editor meant for programming).

It'sreal lyh ardto readco dethat isn 'tspaced very consi stently,y ou se e .
This:
```
session_start();
session_name('Private');
```
might as well just be this:
```
session_start();
```
since the session_name() call has no affect (hint: PHP already opened the session using the default name; see the PHP manual page for [man]session_name/man to learn how to properly use named sessions).
These assignments:
```
$limit = $_GET['limit'];
$filter = $_GET['filter'];
$data = $_GET['data'];
$direction = $_GET['direction'];
$sort = $_GET['sort'];
$page = $_GET['page'];
```
will generate an E_NOTICE level error message for each assignment if the corresponding external variable wasn't passed.

For that reasons, I prefer to write such assignments like so:
```
$limit = isset($_GET['limit']) ? (int)$_GET['limit'] : 10;
```
Note that in addition to being error-free, this method also has two additional benefits: 1) You can perform some generic data manipulation (such as casting to int), and 2) If the variable was not passed, you can specify a default value (such as 10). All on one line, using the ternary operator.
As quinxorin points out, you need to guard yourself against SQL injection. In fact, I disagree with the "Unless this is a page that can..." stipulation/exception. There should be no exceptions (ever stop to wonder how many exploited security holes could have been prevented if the programmer(s) hadn't thought "Ah, well, we don't need security here, right?..." ? 😉).

On that note, two methods would benefit you greatly here. The first is explicit casting; cast the incoming data to an int/float if you can - that will eliminate all possibility of SQL injection/error.

The second is whitelisting (which achieves the same result.. assuming you don't have SQL errors - such as unescaped single quotes - in your whitelist, that is :p). For example, don't simply insert whatever value the user supplied for $filter, $sort, or $direction; instead, form an [man]array[/man] of valid values for each of those parameters and use something like [man]in_array/man to make sure that the user-supplied data matches one of the known-good values. Otherwise, empty or non-valid data should be ignored and you should instead just supply a default value.
Same problem here:
```
(isset($_POST['submit']) && ($_POST['cname']) && ($_POST['realm']) && ($_POST['update']))
```
as in item #2 above - you'll be generating E_NOTICE level errors if any of those last 3 external variables weren't passed in (hint: you're only using [man]isset/man on the first variable).
On line #62:
```
$char_xml[achievements][achievementsCompleted]
```
will generate E_NOTICE level errors like

Use of undefined constant achievements - assumed 'achievements'
since those array indexes are most likely strings, not constants (meaning they should be delimited with quotes).
Make sure you are outputting a valid HTML document. For example, ID attributes must begin with a letter.

HerbRodenhaber

Thanks Guys for the Input ...Give me some time to work on your suggestions and i will post back with the results
and maybe a few questions

Herb

HerbRodenhaber

the code below should take care of the SQL injection Issue as long as i implemented it correctly

$limit = isset($GET['limit']) ? (int)$GET['limit'] : 10;
$limit = mysql_real_escape_string($limit,$db);
$filter = isset($GET['filter']) ? $GET['filter'] : Type;
$filter = mysql_real_escape_string($filter,$db);
$data = isset($GET['data']) ? (int)$GET['data'] : 92;
$data = mysql_real_escape_string($data,$db);
$direction = isset($GET['direction']) ? $GET['direction'] : asc;

$direction = mysql_real_escape_string($direction,$db);
$sort = isset($GET['sort']) ? $GET['sort'] : Name;
$sort = mysql_real_escape_string($sort,$db);
$page = isset($GET['page']) ? (int)$GET['page'] : 1;
$page = mysql_real_escape_string($page,$db);
$pagestart = (($page -1) * $limit);

still working on the rest ... thanks again guys

bradgrafelman

Don't use [man]mysql_real_escape_string/man on integer data. Not only does it not make sense (hint: what's the last word in that function? 😉), but it's unnecessary - casting the incoming data to an (int)eger ensures you won't leave any room for SQL injection.

HerbRodenhaber

im working this into all the user input strings ...

$user1 = (int)$GET['limit'];
$limit_validate = array(5, 10, 20, 25);
if (in_array($user1, $limit_validate, true)) {
$limit = isset($GET['limit']) ? (int)$_GET['limit'] : 10;
}
else
{
$limit = 10;// Default Value when not set or value not in array
}

thanks for all the comments and suggestions

herb