Shrink two queries down to one

schwim · May 6, 2022

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 = 			'its@schw.im';
$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 · May 7, 2022

Some improvements...

// Enable us to use Headers
ob_start();

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(); }

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; }

You don't use the badapple session anywhere in the code you have shown.
Do not output internal server errors. It is useless to the user and only good to hackers. ($e->getMessage)
Do not SELECT *. Specify columns by name
You do not need the LIMIT 1 in your query. You are only going to get one result anyways.
You are using stripslashes. Where and why did you add slashes? I think you probably didn't therefore you don't need it
You suddenly jump case at the bottom of the code. Stay consistent and always use lower-case

schwim · May 7, 2022

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 · May 7, 2022

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 · May 7, 2022

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 · May 7, 2022

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 = 			'its@schw.im';
$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 · May 7, 2022

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 · May 7, 2022

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);
}

?>

schwim · May 8, 2022

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] => its@schw.im
    [0] => its@schw.im
    [username] => schwim
    [1] => schwim
    [existing_user] => 0
    [2] => 0
)

users
Array
(
    [email] => its@schw.im
    [0] => its@schw.im
    [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 · May 8, 2022

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

benanamen · May 8, 2022

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