On Thu, Jul 16, 2009 at 11:01 AM, Andrew Ballard <aball...@gmail.com> wrote:

> On Thu, Jul 16, 2009 at 9:33 AM, Miller,
> Terion<tmil...@springfi.gannett.com> wrote:
> >
> > Here is what finally worked:
> >
> >     <?php                                                        $letter
> = isset($_GET['letter']) ? $_GET['letter'] : "A";
>                              //alphabetical pagination links
>                                        echo '<div align="center"><b>';
>                                                  foreach(range('A','Z') as
> $c){                                                          ($letter ==
> $c)                                                            ?
> printf('%s&nbsp',$c)
>    : printf('<a href="?letter=%s">%s</a>&nbsp;',$c,$c);
>                                    }
>                echo "</b></div><p>";
>
>  //Show all restaurants that start with $letter
>                            $sql = "SELECT * FROM restaurants WHERE name LIKE
> '{$letter}%'";
>  $result = mysql_query($sql) or die(mysql_error());
>                                while($row = mysql_fetch_assoc($result)){
>                                                      printf('<div
> align="left" width="100"><b>%s</b><br>%s</br>%s</br></div><hr color=#000
> width=200></hr>',$row['name'],$row['address'],$result['cviolations']);
>                                                  }
>
>                                                        ?>
> > Thanks again everyone!!
>
> Terion,
>
> I hope that isn't your final answer. This has SQL injection written
> all over it since you are neither validating that $letter is actually
> a letter, nor are you escaping it before passing it off to MySQL.
>
> <?php
> $letter = isset($_GET['letter']) ? $_GET['letter'] : 'A';
>
>
> if (!preg_match('/^[A-Z]$/i', $letter) {
>    $letter = 'A';
>    /*
>       Rather than setting $letter to 'A' and continuing,
>       you could generate an error if you end up in here
>       so you can let the user know that what they passed
>       was invalid.
>    */
>
> }
>
>
> //....
> ?>
>
> In this case, it should be safe to use $letter directly in the query
> without passing it through mysql_real_escape_string() since it should
> only contain a single harmless alphanumeric letter, but it wouldn't
> hurt (and may still be a good idea) to go ahead and escape the value
> in the query anyway just in case something in your code changes later
> that might cause some cruft to slip in.
>
> Andrew
>

My point of view:

# i'll use constants for these values
assert( ord('A') == 0x41 );
assert( ord('Z') == 0x5A );

# 1. get the ascii code of the 1st character or from A=0x41
$letter = ord( array_key_exists('letter', $_GET) ? strtoupper(
$_GET['letter']{0} ) : 'A' );

# 2. different solutions
# 2.a check if it is range ussing <= ussing constants (faster)
$letter = chr( 0x41<= $letter && $letter <= 0x5A ? $letter : 0x41 );

# 2. different solutions
# 2.b check if it is range min/max and with constants (faster)
$letter = chr( min( max(0x41, $letter), 0x5A) );

I'd use the 2.b but this has different behaviour when $letter > Z (should
this ever happen?)
In the other hand I think it is the faster one.


-- 
Martin Scotta

Reply via email to