Iframe displaying external content

[mattyvx]

Hello!

 

Looking for some advice regarding displaying content from my databases on my members websites via an iframe. Here's what I want to do;

 

Members on my site are rated and reviewed, what I wanted to do was create a piece of code that they could embed on their website to show their reviews. My first instincts where to create an iframe and use the source to pass get variables to my server to identify the member and then use these in a query to select member reviews.

 

Here is the code;

 

For their site;

 

 <iframe frameborder="0" height="250px" width="700px" scrolling="auto" src="http://www.mysite.co.uk/reviews.php?ID=9999&aKey=123456"></iframe>

 

Passes the member ID and a unique access key for that member.

 

The page on my site; (reviews.php)

 

include_once $_SERVER['DOCUMENT_ROOT'] . // path to script files

$ID=mysql_real_escape_string($_GET['ID']);
$aKey=mysql_real_escape_string($_GET['aKey']);

if(empty($aKey) || empty($ID)){ die("Key or ID not set,Selection Error"); exit;}

$reviewresult = mysql_query("SELECT Name,Review,Date FROM Reviews WHERE ID='$ID' AND aKey='$aKey' Order by RAND() Limit 10");

//if there is at least one review
if($reviewresult && mysql_num_rows($reviewresult) > 0)
{
  while($review = mysql_fetch_array($reviewresult))
  {
// output reviews
  }
}

 

Are there any security risks with this script?

Do you think it is the best method to accomplish what I want? Are there better alternatives?

Any suggestions or room for improvement?

 

Thanks!

[BLaZuRE]

Since you know what type of data you're going to get, I suggest using RegEx to validate the ID and key.  For example, [0-9]{4} should work for a 4-digit number.  That way, you know you're never being given anything besides a 4-digit number.  Substituting {1,4} would test for 1 to 4 digits.  Look into preg_match and learn about Perl Regular Expressions.  I'd error on no match rather than on empty var.

 

As it is right now, your code only helps prevent from SQL injection and may be vulnerable to HTML or PHP injection (though there is a limit on $_GET parameter length).

[mattyvx]

thanks, so something like this?;

 

include_once $_SERVER['DOCUMENT_ROOT'] . // path to script files

$ID=mysql_real_escape_string($_GET['ID']);
if(!is_numeric($ID) || strlen($ID > 4)){ die("Invalid ID"); } 

aKey=mysql_real_escape_string($_GET['aKey']);
if(strlen($aKey > 6)){ die("Invalid Key"); } 

$reviewresult = mysql_query("SELECT Name,Review,Date FROM Reviews WHERE ID='$ID' AND aKey='$aKey' Order by RAND() Limit 10");

//if there is at least one review
if($reviewresult && mysql_num_rows($reviewresult) > 0)
{
  while($review = mysql_fetch_array($reviewresult))
  {
// output reviews
  }
}
else
{
die ("Error getting reviews");
}

[BLaZuRE]

Yeah, that'll look too, but I like to think my way's fancier