Shrink two queries down to one

[pbismad]

schwim What is your registration process

Could you specifically answer this, in some detail, e.g. what does validating the email address actually involve? Until we know what you know about the steps/process, you are going to get a bunch of information all over the place as to what you should be doing or doing differently.

I can guarantee that any site you have directly registered on, such as this forum, you created a username and entered your contact email at the same time. Separating these provides no utility, security or otherwise. The only functional case where entering an email, then at a later step entering the rest of the account information, would be where you didn't directly register, but instead were invited to register, such as an existing patient registering for a medical records account login, i.e. staff entered your contact email into their system, which sent you an invitation email with a registration completion link in it.

schwim 1) It's not unknown

But it still can contain sql special characters which can break the sql query syntax. Valid email addresses, in the name portion, can contain almost any printing characters and some database servers, such as MySql, happily expands hexadecimal encoded sql, when in a string context, back into the actual sql. You must protect against sql special characters in any value from breaking the sql query syntax, which is how sql injection is accomplished. Prepared queries provide foolproof protection against sql injection for all data types. They also simplify the sql query syntax, by replacing all the extra quotes, concatenation dots, and {} that are used to get php variables into the query statement, with a simple ? place-holder.

schwim I've sanitized

Aside from trimming data, mainly so that you can detect if it is all white-space characters, you should not modify data values. You should only validate it and use it if it is valid. If it is not valid, tell the user what was wrong with it so that they must correct it and provide a valid value.

schwim Is this a security risk, even when disabled or just messy/wasteful coding?

Both. By confirming to a hacker that your code handled an error and produced a cute error message, it gives them feedback that what they did, did cause a detectable error. If you do nothing in your code, except for the mentioned insert/update queries and only for duplicate or out of range values, the response will be, when you are logging php errors (which will now include database statement errors), a http 500 error page. You also want the the simplest code. If you do as suggested, you will have no error handling logic in your code, except for the mentioned cases, and you can simply switch from displaying to logging database statement errors by switching php's display_errors/log_errors settings, which should already be appropriately set in the php.ini on your development and live server.

schwim 4) This is embarrassing but using while ...

While (ha ha) a while loop is a conditional test, it will be skipped if there was no row fetched, that's not what you want in this case. You can just fetch and test at the same time if there was or was not a row of data.

if($row = mysqli_fetch_assoc($result))
{
    // there was a row of data
}
else
{
    // there was not a row of data
}

This might be a good point to mention not creating and maintaining a bunch of verbose (more typing) variable names. All the code after a main comment about this being for a part of the registration process, up to the next main comment for something else, is for the registration process. There's no good reason to keep repeating any part of 'registration' in all the variable names. You have an sql query statement, just use $sql or similar. You have the result from a query, just use $result or $stmt or similar. You have a row of fetched data, just use $row or similar.

schwim 5) I think this ...

We continually see line after line of code that's copying variables to other variables. Programming IS a tedious typing activity. Don't make it harder by doing unnecessary typing, with typo mistakes. Most of the points that have been made results in simpler code, eliminating a bunch of typing. Also, by keeping things like form data and data fetched from a query in an array variable, you open up the possibility of more advanced programming by dynamically operating on the data using array functions.

[schwim]

Weedpacket So the user starts the process, and then almost immediately has to stop to wait for and then confirm the email before resuming? Or do they get to continue through the process in the meantime?

They get to continue the registration process while waiting on the activation email. I tried this tactic on one of my sites and my bot registrations went to 0 and not just for a week or month, but for over a year so far. I'll explain the reason for the change of how it's handled a bit below.

pbismad Could you specifically answer this, in some detail, e.g. what does validating the email address actually involve? Until we know what you know about the steps/process, you are going to get a bunch of information all over the place as to what you should be doing or doing differently.

I can guarantee that any site you have directly registered on, such as this forum, you created a username and entered your contact email at the same time. Separating these provides no utility, security or otherwise. The only functional case where entering an email, then at a later step entering the rest of the account information, would be where you didn't directly register, but instead were invited to register, such as an existing patient registering for a medical records account login, i.e. staff entered your contact email into their system, which sent you an invitation email with a registration completion link in it.

000000000000000000Sidebar00000000000000000000
I'm sorry that I haven't clarified my intent, scope or reasoning to this point. One of my shortcomings when asking for support is thinking the solution to my issue is very small and pinpoint when in reality(as in this thread), they often end up in a complete rewrite of the entire framework due to what I've learned in the topic. I should have started out with this info but we'll file it under better late than never:

1) The scope of this project is minuscule compared to what you're probably imagining. I'm writing a user authentication system that will then have modules written to work on top of it. A forum on one site with a member map, a bloggy-type thing on another, a bike wiki on another, etc. This may seem like something that will see a lot of traffic but most of my sites see less than 10 legitimate users a month. A forum I run has three regular members. Many sites are really just me. It's very likely that these won't grow, in spite of what I do with them.

2) My goal is not to write something better than what's out there because I know I can't. I'm writing this mainly for two reasons. First, I like to write PHP. It's a great scripting language for someone like me that doesn't bring any formal training or work history to the table. Secondly, and the important part in context to my questions is that I'm somewhat obsessed with bots, spam and exploits. I love seeing what they try to do and I really enjoy trying to write something that foils them without the assistance of things like CAPTCHA. I use the SFS and Akismet db, but really just for confirmations, my goal is to try to write something that causes the bots and exploiters to get banned while allowing the legitimate user to use the site without issue. I try all kinds of things, many that don't work. The email-first modification to user registration was just something I found that worked extremely well so I've written this to use it. I'm trying other things for the first time that hasn't been pasted in this topic that I hope to see in action.
000000000000000000000000000000000

So I think before I go any further with the issue at hand, I should start working on converting the script to use PDO. I'll just start using this topic for assistance and see if I can't get back to the query in question since the change will likely solve the issues I started the topic for.

Thank you all very much for taking the time to help. I know that it can be frustrating when someone is doing so much wrong with such a small bit of code but you folks have tolerated me for over 15 years. PHPB has been my go-to place for assistance the entire time. You all are pretty awesome. I'll be back when I've got my config file converted for your thoughts on improvement.

[pbismad]

schwim

If you are going down the PDO path, some practices to follow -

1. When you make the connection, set the character set to match your database table(s) so that no character conversion occurs over the connection. If you are using emulated prepared queries (not recommended) this would also help prevent sql injection when the emulator applies the quote method to string data.
2. Set emulated prepared queries to false (you want to use real prepared queries.)
3. Set the error mode to exceptions (you want to use exceptions for errors with query, prepare, and execute calls.)
4. Set the default fetch mode to assoc so that you don't need to specify it in each fetch statement.
5. For a prepared query, use implicit binding by supplying an array of input values to the ->execute([...]) call.

For the SELECT query (which you probably won't be using for this activity), the PDO equivalent would look like -

$sql = "SELECT list out the columns you want FROM registrations WHERE email = ?"; // since the email column MUST be defined as a unique index so that the database will enforce unique values, there's no point in the LIMIT 1, the query will stop when an entry is found
$stmt = $pdo->prepare($sql);
$stmt->execute([$email]);
$row = $stmt->fetch();

[schwim]

Alright, here's my config file after conversion of the statements. The page executes without errors and arrays contain what they're supposed to but just wanted to make sure that I'm handling things like making sure $cookie is safe when used in a statement. Any thoughts on improvement would be most welcome. I'll carry what I learn over to the next page in line for conversion.

<?php

/* File: /includities/configlio.php */


if(!defined('parentInclude')) {
	header("Location: /");
	exit;
}

// Enable us to use Headers
ob_start();

// Set sessions
if(!isset($_SESSION)) {
	session_start();
}

/* Any session setup */
if(!isset($_SESSION['badapple'])){
	$_SESSION['badapple'] = 0;
}

/* DB creds */

/* Connect to the DB */
try {
	$pdo = new PDO("mysql:host=$dbhost;dbname=$dbname", $dbuser, $dbpasswd);
	$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(PDOException $e) {
	echo "Connection failed: " . $e->getMessage();
}

/* Query to get necessary settings for general use. */
$stmt = $pdo->query("SELECT * FROM settings LIMIT 1");
$settings1 = $stmt->fetch();
$is_online = 		$settings1['is_online'];
$offline_excuse = 	$settings1['offline_excuse'];
$site_title = 		stripslashes($settings1['title']);
$site_description = stripslashes($settings1['description']);
$site_keywords = 	stripslashes($settings1['keywords']);


/* Site specific shite*/
$site_cookie = 			'bicyclio_user';
$site_url = 			'https://bicyclio.com';
$site_name = 			'Bicyclio!';
$site_email = 			'[e-mail]';
$rightnow = 			time();
$token = 				mt_rand().$rightnow;
$pubtoken =				substr(session_id(), 0, 10);
$alert = array();
$alert_display = '';

if(isset($_COOKIE[$site_cookie])){
	/* User has a cookie.  Is it valid?*/
	$stmt = $db->prepare("SELECT * FROM users WHERE cookie=:cookie LIMIT 1");
	$user = $stmt->bindParam(':cookie', $_COOKIE[$cookie_name], PDO::PARAM_STR);
	if ($user->rowCount() > 0) {
		$vu_id = 				$user['user_id'];
		$vu_sid = 				session_id();
		$vuname = 				$user['username'];
		$vu_email = 			$user['user_email'];
		$vu_group_id = 			$user['group_id'];
		$vu_role = 				$user['role'];
		$vu_avatar = 			$user['avatar'];
		if($vu_avatar == ''){
			$vu_avatar = 		'no_avatar.png';
		}	
	}else{
		/* Has a cookie but isn't legitimate */
		$is_anon = '1';
	}

}else{
	/* Doesn't have a cookie */
	$is_anon = '1';
}

if($is_anon){
	/* No cookie means visitor is anonymous */
	$vu_username = 					'Anonymous';
	$vu_id = 						'0';
	$vu_sid =			 			session_id();
	$vu_group_id = 					'1'; // 6 eqals bot
	$vu_role = 						'user';
	$vu_avatar = 					'anonymous.png';
}

//Get the forwarded IP if it exists
IF(array_key_exists('X-Forwarded-For', $_SERVER) && filter_var($_SERVER['X-Forwarded-For'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['X-Forwarded-For'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}ELSEIF(array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER) && filter_var($_SERVER['HTTP_X_FORWARDED_FOR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['HTTP_X_FORWARDED_FOR'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}ELSE{
	$vu_ip = 				filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4);
}

?>

[benanamen]

Some improvements...

// Enable us to use Headers
ob_start();

1. ob_start does not "Enable us to use Headers". It enables you to write bad code. Technically, it is to " Turn on output buffering". If you need this on for your code to work, you did something wrong somewhere.

if(!isset($_SESSION)) {
session_start();
}

2. The if is pointless. Get rid of it. session_start will by default start a new session or resume an existing session.

if(!isset($_SESSION['badapple'])){
$_SESSION['badapple'] = 0;
}

3. You don't use the badapple session anywhere in the code you have shown.

4. Do not output internal server errors. It is useless to the user and only good to hackers. ($e->getMessage)

5. Do not SELECT *. Specify columns by name

6. You do not need the LIMIT 1 in your query. You are only going to get one result anyways.

7. You are using stripslashes. Where and why did you add slashes? I think you probably didn't therefore you don't need it

8. You suddenly jump case at the bottom of the code. Stay consistent and always use lower-case

[schwim]

benanamen Do not output internal server errors. It is useless to the user and only good to hackers. ($e->getMessage)

Errors are incredibly useful to me right now, especially as I make these changes. What should I change the catch to so I can continue to retrieve the errors for troubleshooting?

[benanamen]

schwim Errors are incredibly useful to me right now

Errors will ALWAYS be incredibly useful. It's just a matter of where they show up.

You should be working from a local dev setup with error reporting turned all the way up in the php.ini. For production, errors should be off and logged.

If you are working from Windows I would recommend you install Laragon. It is the best WAMP for several reasons, one being automatic hosts so you can access your project from "yourproject.test" instead of localhost/yourproject.
https://laragon.org

[pbismad]

schwim Errors are incredibly useful to me right now, especially as I make these changes. What should I change the catch to so I can continue to retrieve the errors for troubleshooting?

In previous replies in this thread, it was pointed out to only catch and handle database statement exceptions in your code for insert/update queries involving the possibility of duplicate or out of range values. And in all other cases to simply let php catch the exception, which will result in php 'automatically' displaying/logged the raw database statement errors.

[schwim]

Here's my next iteration of the file after suggested changes were implemented. Hopefully I've covered all the bases:

<?php

/* File: /includities/configlio.php */

session_start();


if(!defined('parentInclude')) {
	header("Location: /");
	exit;
}

/* Any session variables that need setup */
/* Badapple is used anywhere in the script where forms are trying to be exploited. */
if(!isset($_SESSION['badapple'])){
	$_SESSION['badapple'] = 0;
}

/* DB creds */

/* Connect to the DB */
try {
	$pdo = new PDO("mysql:host=$dbhost;dbname=$dbname", $dbuser, $dbpasswd);
	$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(PDOException $e) {
	
}

/* Query to get necessary settings for general use. */
$stmt = $pdo->query("SELECT is_online, offline_excuse, title, description, keywords FROM settings");
$settings1 = $stmt->fetch();
$is_online = 		$settings1['is_online'];
$offline_excuse = 	$settings1['offline_excuse'];
$site_title = 		$settings1['title'];
$site_description = $settings1['description'];
$site_keywords = 	$settings1['keywords'];


/* Site specific shite*/
$site_cookie = 			'bicyclio_user';
$site_url = 			'https://bicyclio.com';
$site_name = 			'Bicyclio!';
$site_email = 			'[e-mail]';
$rightnow = 			time();
$token = 				mt_rand().$rightnow;
$pubtoken =				substr(session_id(), 0, 10);
$alert = array();
$alert_display = '';

if(isset($_COOKIE[$site_cookie])){
	/* User has a cookie.  Is it valid?*/
	$stmt = $db->prepare("SELECT user_id, username, user_email, group_id, role, avatar FROM users WHERE cookie=:cookie LIMIT 1");
	$user = $stmt->bindParam(':cookie', $_COOKIE[$cookie_name], PDO::PARAM_STR);
	if ($user->rowCount() > 0) {
		$vu_id = 				$user['user_id'];
		$vu_sid = 				session_id();
		$vuname = 				$user['username'];
		$vu_email = 			$user['user_email'];
		$vu_group_id = 			$user['group_id'];
		$vu_role = 				$user['role'];
		$vu_avatar = 			$user['avatar'];
		if($vu_avatar == ''){
			$vu_avatar = 		'no_avatar.png';
		}	
	}else{
		/* Has a cookie but isn't legitimate */
		$is_anon = '1';
	}

}else{
	/* Doesn't have a cookie */
	$is_anon = '1';
}

if($is_anon){
	/* No cookie means visitor is anonymous */
	$vu_username = 					'Anonymous';
	$vu_id = 						'0';
	$vu_sid =			 			session_id();
	$vu_group_id = 					'1'; // 6 eqals bot
	$vu_role = 						'user';
	$vu_avatar = 					'anonymous.png';
}

//Get the forwarded IP if it exists
if(array_key_exists('X-Forwarded-For', $_SERVER) && filter_var($_SERVER['X-Forwarded-For'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['X-Forwarded-For'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}elseif(array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER) && filter_var($_SERVER['HTTP_X_FORWARDED_FOR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['HTTP_X_FORWARDED_FOR'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}else{
	$vu_ip = 				filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4);
}

?>

[benanamen]

Where is $cookie_name coming from?

You could do without all the variables for nothing from the settings query. You already have variables, just use them.

Remove the try/catch on the connection. You are not doing anything with it and you already set PDO Exceptions. I would suggest you use set_exception_handler to handle the exceptions however you want. What I do with it is log the errors and fire off an email to myself with the error details as well a provide a generic Fatal Error message to the user.

** Do not echo the error message like the example in the manual. It is a poor example and is what you were already doing before.

https://www.php.net/manual/en/function.set-exception-handler.php

If you really want to get this top notch, put the whole project on Github so we can review it as a whole.

[schwim]

benanamen Where is $cookie_name coming from?

My login page contains the blurb that is setting the cookie.

benanamen What I do with it is log the errors and fire off an email to myself with the error details as well a provide a generic Fatal Error message to the user.

Could you show me an example of a query that would call an email function in the event of an error?

benanamen If you really want to get this top notch, put the whole project on Github so we can review it as a whole.

I would like to at least finish of the conversion to the new format and wrap up some of the partial stuff as it's really too much a mess currently to bother anyone else with.

Here's my config file as it stands. I've made the changes suggested and moved some stuff to the index file as it began to not make sense to have it here. Hopefully I'm close to being ready to move the changes on to the other files.

<?php

/* File: /includities/configlio.php */

session_start();


if(!defined('parentInclude')) {
	header("Location: /");
	exit;
}

/* DB creds */

/* Connect to the DB */
$pdo = new PDO("mysql:host=$dbhost;dbname=$dbname", $dbuser, $dbpasswd);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

/* Query to get necessary settings for general use. */
$stmt = $pdo->query("SELECT is_online, offline_excuse, title, description, keywords FROM settings");
$site = $stmt->fetch();

if(isset($_COOKIE[$site_cookie])){
	/* User has a cookie.  Is it valid?*/
	$stmt = $db->prepare("SELECT id, username, user_email, gid, role, avatar FROM users WHERE cookie=:cookie LIMIT 1");
	$usr = $stmt->bindParam(':cookie', $_COOKIE[$cookie_name], PDO::PARAM_STR);
	if ($usr->rowCount() > 0) {
		$usr['sid'] = 				session_id();
		if($usr['avatar'] == ''){
			$usr['avatar'] = 		'no_avatar.png';
		}	
	}else{
		/* Has a cookie but isn't legitimate */
		$is_anon = '1';
	}
}else{
	/* Doesn't have a cookie */
	$is_anon = '1';
}

if($is_anon){
	/* No cookie means visitor is anonymous */
	$usr['user_id'] =				'0';
	$usr['username'] = 				'Anonymous';
	$usr['sid'] =		 			session_id();
	$usr['gid'] = 					'1'; // 6 eqals bot
	$usr['role'] = 					'user';
	$usr['avatar'] =				'anonymous.png';
}

//Get the forwarded IP if it exists
if(array_key_exists('X-Forwarded-For', $_SERVER) && filter_var($_SERVER['X-Forwarded-For'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['X-Forwarded-For'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}elseif(array_key_exists('HTTP_X_FORWARDED_FOR', $_SERVER) && filter_var($_SERVER['HTTP_X_FORWARDED_FOR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)){
	$vu_ip = 				$_SERVER['HTTP_X_FORWARDED_FOR'];
	$vu_proxy = 			$_SERVER['REMOTE_ADDR'];
}else{
	$vu_ip = 				filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP, FILTER_FLAG_IPV4);
}

?>

[benanamen]

Set another connection option to just get one result in the array.
PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC

schwim Could you show me an example of a query that would call an email function in the event of an error?

In your custom function called by set_exemption_handler just do your email call there. When there is an exception, your function is called by set_exemption_handler.

Here is an example that uses error_log. One of the options is to send an email.
https://github.com/benanamen/clean-pdo/blob/master/test.php

https://www.php.net/manual/en/function.error-log.php

[schwim]

Weedpacket SELECT email,(whatever else), 1 as existing_user FROM users WHERE email="$email"
UNION
SELECT email,(whatever else), 0 as existing_user FROM users WHERE email="$email"

I've got a quick question concerning the arrays that are returned for the query:

user_registrations
Array
(
    [email] => [e-mail]
    [0] => [e-mail]
    [username] => schwim
    [1] => schwim
    [existing_user] => 0
    [2] => 0
)

users
Array
(
    [email] => [e-mail]
    [0] => [e-mail]
    [username] => schwim
    [1] => schwim
    [existing_user] => 1
    [2] => 1
)

Can you tell me why the array is doubling the results, once for [email] then again for [0], etc.? Am I performing the query incorrectly?

		$stmt = $pdo->query("SELECT email, username, 0 as existing_user FROM user_registrations WHERE email='$email'
			UNION
		SELECT email, username, 1 as existing_user FROM users WHERE email='$email'");
		$acctchk = $stmt->fetch();
		print_r($acctchk);

[maxxd]

PDO::FETCH_BOTH is the default return mode; it'll return the data in both an indexed and associative array.