[RESOLVED] opt-out link & remove record from the DB

[mzanimephp]

Hi, I've got a script which adds user submitted values to a MySQL DB, like this:

$fn = $_POST['FirstName'];
$ln = $_POST['LastName'];
$em = $_POST['email'];
$dbcnx = mysql_connect("localhost","removed","removed") or die("Error Connecting to DB");
$db = mysql_select_db("prefix_dbname",$dbcnx) or die("Error Selecting Database");
$query = "insert into entries values('$fn', '$ln', '$em')";
$result = mysql_query($query,$dbcnx) or die("Error in Query");

It also sends a confirmation letter to the user, and emails the admin a notification. How can I have it insert a opt-out link inside of the confirmation email with a query that deletes them from the DB. Do I need to associate ID values with each record or something?

[dougal85]

mzanimephp wrote:

Do I need to associate ID values with each record or something?

Not really...

DELETE FROM entries WHERE email_column_name = $email

just make a link at the bottom of an email that takes them to a page, the $_GET value holding there email address.

You could use id's and stuff but its not really needed.

Also, btw...

From the code you have shown, you are open to SQL injection hacks. You need to validate data before it is used in an SQL query.

[mzanimephp]

Thanks. As far as the SQL injection goes, I was afraid you'd say something like that. I'm just barely learning PHP, learning about PHP security is quite another thing.

Any suggestions, as to how I can make it more secure?

I've managed to allow entries to be deleted from the DB via a passed query called $_GET['remove'], like this:

domain.com/test/mail.php?remove=[e-mail]

Only problem is, that this runs every single time it's called. If the email address doesn't exist in the DB, it still says "your email has been removed." If there's more than one instance of the same entry in the DB (which should not happen) it still says "your email has been removed." and deletes both entries, etc...

[dougal85]

what you need to do is run a query to check the email is still in the database.

SELECT * FROM table WHERE email = $email

then use MySQL_row_count and check it is more than 0. then delete.

And then, read this : http://en.wikibooks.org/wiki/Programming😛HP:SQL_Injection

Plenty of other articles on the subject, just Google SQL Injection.

Dougal

[mzanimephp]

OK. I've made an attempt to beef up security. How's this?

$fn = $_POST['FirstName'];
$ln = $_POST['LastName'];
$em = $_POST['email'];
$dbcnx = mysql_connect("localhost","removed","removed") or die("Error Connecting to DB");
$db = mysql_select_db("prefix_dbname",$dbcnx) or die("Error Selecting Database");
if (preg_match("/^[a-zA-Z]/", $fn, $matches) && preg_match("/^[a-zA-Z]/", $ln, $matches) && preg_match("/^.+\@(\[?)[a-zA-Z0-9\-\.]+\.([a-zA-Z]{2,3}|[0-9]{1,3})(\]?)$/", $em, $matches)) {
$query = "insert into entries values('$fn', '$ln', '$em')"; 
$result = mysql_query($query,$dbcnx) or die("Error in Query");
} else { die('Method not allowed.'); }  

mysql_close($dbcnx);

Although, I can never seem to get the 'Method not allowed.' error to display.

[dougal85]

It looks like you are getting the right idea. I personally hate regular expressions and always take ages getting them right.

You cant get it to display that error even if you enter no data? or enter something like '%^$%&$&' just random jargon.

I cant see anything wrong with them at a first glance.

I've always used eregi rather than preg_match in this situation... not sure if that would make a difference.

http://uk.php.net/manual/en/function.eregi.php

[bradgrafelman]

Speaking of never, ever, ever, using ereg() or ereg()...

Weedpacket wrote:

The ereg functions are being downgraded; PHP's developers for some years now have recommended the preg_* family of functions.

Weedpacket wrote:

http://www.php.net/~derick/meeting-notes.html#move-ereg-to-pecl

[mzanimephp]

Thanks brad. I didnt want to use eregi because I was thinking that it would cause me to have to further re-write my code, and then too I've experienced problems in the past with both ereg() and eregi().

[dougal85]

oops. my bad.

[mzanimephp]

Yes, your bad. Can't be giving beginners bad advice man, its just not cool. :queasy:

But regarding security, how does this look guys?

if(empty($_GET) && empty($_POST)) {
  die('Please do not access this file directly.');
}

  $dbcnx = mysql_connect("localhost","removed","removed") or die("Error Connecting to DB");
  $db = mysql_select_db("prefix_dbname",$dbcnx) or die("Error Selecting Database");

if(isset($_GET['remove'])) {
  $match = $_GET['remove'];

  $query = mysql_query("SELECT * FROM entries WHERE email = '$match'");
  if(mysql_num_rows($query) == true) {
  $query .= mysql_query("DELETE FROM entries WHERE email = '$match'");
  die ("<p>The email address <b>".mysql_real_escape_string($match)."</b> has been removed.</p>"
      ."<p><a href='/test/test.php'>OK</a></p>");
  } else {
  die ("<p><b>Error:</b> The email address <b>".mysql_real_escape_string($match)."</b> is not "
      ."in our DB, or has already been removed.</p><p><a href='/test/test.php'>OK</a></p>"); 
  }
  $result = mysql_query($query,$dbcnx) or die("Error in Query");
}

$fn = $_POST['FirstName'];
$ln = $_POST['LastName'];
$em = $_POST['email'];

if (preg_match("/^[a-zA-Z]/", $fn, $matches) && preg_match("/^[a-zA-Z]/", $ln, $matches) && preg_match("/^.+\@(\[?)[a-zA-Z0-9\-\.]+\.([a-zA-Z]{2,3}|[0-9]{1,3})(\]?)$/", $em, $matches)) {
  $query = mysql_query("SELECT * FROM entries WHERE email = '$em'");
  if(mysql_num_rows($query) == false) {
  $query = "INSERT INTO entries values('$fn', '$ln', '$em')"; 
  $result = mysql_query($query,$dbcnx) or die("Error in Query");
  } else {
  die("<p>That email address already exists in the DB.</p><p><a href='/test/test.php'>OK</a></p>");
  }
}

mysql_close($dbcnx);

// etc, etc...

Seems to be working just fine to me.

[dougal85]

looks pretty good. (but apparently what do i know? 😛)

One thing, I don't see any checking of $match

"$match = $_GET['remove']; "

Since its an email, run it through the same preg_match for the email later.

[mzanimephp]

$match can only be an email address. Because only email addresses are allowed through the email field. If the user tries to insert anything else into the email field from the index page it will throw "Error - the email address {insert here} is not valid. " (That part of my code is not shown.)

Or, if they try to inject something else into the removal parameter: mail.php?remove=xxx, the script will throw "Error: The email address {insert here} is not in our DB, or has already been removed."

My only, and final problem seems to be preventing them from entering special characters into the firstname and lastname fields. The Regex doesn't allow something to get sent to the DB, but it still redirects the user to my thankyou.php page... as if it worked.

[dougal85]

but they could inject code that is executed into the SELECT query when checking the email exists.

[bradgrafelman]

As dougal explains, the point of validating all user-supplied data is to prevent any malicious code to reach your SQL queries.

From the point of validating data to prevent MySQL injection attacks, we really don't care if there's an e-mail in your database that matches, or if it's a malformed e-mail address. All we care about is that it doesn't alter your SQL query.

[mzanimephp]

Alright, but this is a little extreme, isnt it? I don't typically see this much regex'ing on scripts that I've downloaded.

How's this?

if(isset($_GET['remove'])) {
  if(preg_match("/^.+\@(\[?)[a-zA-Z0-9\-\.]+\.([a-zA-Z]{2,3}|[0-9]{1,3})(\]?)$/", $_GET['remove'], $matches)) {
  $match = $_GET['remove'];
  } else {
  die("<p>The email address ".$_GET['remove']." is invalid!</p><p><a href='/test/test.php'>OK</a></p>");
  }

[bradgrafelman]

You're right - that's a lot of regex'ing. But then again... who cares if it's a valid e-mail address or not?

$match = mysql_real_escape_string($_GET['remove'];

All you really want to do is make sure the data doesn't break your query. So, just run it through [man]mysql_real_escape_string/man and be done with it.

[dougal85]

To be honest, most PHP developers dont develope secure code. its one of the reasons PHP gets a bad rep.

Any good program you've downloaded will be secure, it might have hidden all the pregs in reusable functions

[bradgrafelman]

While I definitely agree with you in general terms, I don't think it applies in this situation.

In this situation, using a regular expression to validate that the email address is properly formed adds absolutely no security compared to a simpler function such as [man]mysql_real_escape_string/man (which probably runs faster anyway).

[mzanimephp]

who cares if it's a valid e-mail address or not?

We do. If we're going to be sending out emails, validity matters. But anyway thanks guys. Everythings now working just great.

[dougal85]

He means in the SELECT statement. not the insert.

a select statement will return the same result for an invalid email as one that doesnt exist. Therefore, when deleting a person from the database it doesnt really help to validate it as an email.

Therefore, mysql_real_escape_string() would do the job.

Just to clear that up 😉