In a previous code review, it was strongly suggested that I start using PHP Frameworks to improve security for my websites. I tried Laravel and got stuck due to the steep learning curve. Then I tried CodeIgniter and found it to be a good fit. I did a CodeIgniter tutorial and was able to pick up the basics right away, so I converted my Volunteer Signup Website to CodeIgniter.
As this is my first time working with a PHP Framework and with MVC, I want to run it by the community and make sure I am following best practices and organizing everything correctly.
What does this page do?
The sign_up_individual() function in the controllers/Volunteers.php file figures out what event the volunteer is signing up for (my events are called races) by extracting that information from the URL. Then it loads that event's volunteer shifts with check boxes next to them and lets the viewer select as many as they want. Then the rest of the page collects basic information like name, address, phone number, etc. There is a submit button at the bottom of the page.
When the submit button is pressed, CodeIgniter validates all the form data. If validation fails, the page is re-loaded with the errors displayed in a red box at the top of the page. If the viewer's name and e-mail are already in our SQL database, a special page with a "duplicate entry" message is displayed. If the form passes validation and is not a duplicate, a special page with a "success" message is displayed, and a confirmation e-mail is sent.
controllers/Volunteers.php
<?php
class Volunteers extends CI_Controller
{
public function __construct()
{
parent::__construct();
$this->load->model('race_model');
$this->load->model('shift_model');
$this->load->model('volunteer_model');
$this->load->model('volunteer_shift_model');
}
public function sign_up_individual($race_id_and_text = NULL)
{
$data['race'] = $this->race_model->get_race($race_id_and_text);
if ( empty($data['race']) )
{
// TODO: custom 403 page to make CSRF error more graceful, and to provide a link to click to get off the page
// TODO: move this 404 error code to a helper function
$this->output->set_status_header(404);
load_page_with_great_races_sidebar($this, 'errors/html/error_404_custom');
return;
}
$this->load->helper('form');
$this->load->library('form_validation');
trim_all_post_data();
$this->_capitalize_state();
// TODO: per user feedback, convert common phone number formats to desired format to avoid failing form validation
// delete anything that isn't \d or -
// 1111111111 -> 111-111-1111
$race_id = $data['race']['race_id'];
$this->form_validation->set_rules('shift_id[]', 'Volunteer Shift', 'required|callback__check_shifts[' . $race_id . ']');
$this->form_validation->set_rules('email', 'E-Mail', 'required|valid_email');
$this->form_validation->set_rules('firstname', 'First Name', 'required|max_length[100]');
$this->form_validation->set_rules('lastname', 'Last Name', 'required|max_length[100]');
$this->form_validation->set_rules('birthdate', 'Birthdate', 'required|regex_match[/^\d{1,2}[-\/]\d{1,2}[-\/]\d{2,4}$/]');
$this->form_validation->set_rules('address1', 'Address', 'required|max_length[500]');
$this->form_validation->set_rules('city', 'City', 'required|max_length[100]');
$this->form_validation->set_rules('state', 'State', 'required|regex_match[/^[A-Z]{2}$/]');
$this->form_validation->set_rules('zip', 'Zip Code', 'required|regex_match[/^\d{5}$/]');
$this->form_validation->set_rules('phone', 'Cell Phone', 'required|regex_match[/^\d{3}-\d{3}-\d{4}$/]');
$this->form_validation->set_rules('shirt_size', 'Shirt Size', 'required|in_list[Extra Small,Small,Medium,Large,Extra Large,2XL,3XL]');
$this->form_validation->set_rules('emergency_name', 'Emergency Contact Name', 'required|max_length[200]');
$this->form_validation->set_rules('emergency_phone', 'Emergency Contact Phone', 'required|regex_match[/^\d{3}-\d{3}-\d{4}$/]');
$this->form_validation->set_rules('group_name', 'Group Name', 'max_length[200]');
$this->form_validation->set_rules('group_leader', '"Are you the group leader?"', 'in_list[0,1]');
$this->form_validation->set_rules('waiver', '"I agree to the terms of the waiver"', 'required|in_list[1]');
$data['list_of_shifts'] = $this->shift_model->get_race_shifts($data['race']['race_id']);
// Note: SQL WHERE is case insensitive, which is good in this case
$volunteer = $this->volunteer_model->get_volunteer(
$data['race']['race_id'],
$this->input->post('firstname'),
$this->input->post('lastname'),
$this->input->post('email')
);
if ( $volunteer )
{
// TODO: send e-mail with edit_volunteer link
load_page_with_great_races_sidebar($this, 'volunteers/sign_up_individual_duplicate', html_escape($volunteer));
}
elseif ($this->form_validation->run() === FALSE)
{
load_page($this, 'volunteers/sign_up_individual', html_escape($data));
}
else
{
$this->volunteer_model->sign_up_individual($data);
$data['volunteer'] = $this->volunteer_model->get_volunteer(
$data['race']['race_id'],
$this->input->post('firstname'),
$this->input->post('lastname'),
$this->input->post('email')
);
foreach ( $_POST['shift_id'] as $key => $value )
{
$this->volunteer_shift_model->add_shift($data['volunteer']['volunteer_id'], $key);
}
$data['list_of_this_volunteers_shifts'] = $this->shift_model->get_volunteer_shifts($data['volunteer']['volunteer_id']);
$this->load->library('email');
$this->email->set_mailtype('html');
// don't html_escape email header variables
$this->email->from('[email protected]', $data['race']['race_name']);
$this->email->to($this->input->post('email'));
$this->email->subject('Thank you for signing up to volunteer for ' . $data['race']['race_name']);
$this->email->message($this->load->view('volunteers/sign_up_individual_success_email', html_escape($data), true));
$this->email->send();
load_page_with_great_races_sidebar($this, 'volunteers/sign_up_individual_success', html_escape($data));
}
}
// =============================================================================================
// PRIVATE
// =============================================================================================
// false for fail, true for pass
// must be public because of callback
// I experimented with making this 2 callback functions (at_least_one_shift_selected, shift_is_open), but the code was cleaner when I combined them (less duplicate code)
public function _check_shifts($contents_of_post, $race_id)
{
$this->form_validation->set_message('_check_shifts', 'You must select at least one volunteer shift');
$open_shifts = $this->shift_model->get_open_shift_ids($race_id);
// if no shifts were selected
if ( ! isset($_POST['shift_id']) )
{
return FALSE;
}
// if shift_id isn't an array (hacking attempt that will throw a PHP error in the next check if not caught)
elseif ( ! is_array($_POST['shift_id']) )
{
return FALSE;
}
else
{
foreach ($_POST['shift_id'] as $key => $value)
{
// if a value in the array is an array (hacking attempt that will throw a PHP error in the next check if not caught)
if ( is_array($value) )
{
return FALSE;
}
// if a value in the array isn't equal to 1 (hacking attempt)
elseif ( $value != 1 )
{
return FALSE;
}
// if shift selected by user isn't an open shift (hacking attempt, or shift became full between loading form and submitting form)
elseif ( ! in_array($key, $open_shifts) )
{
return FALSE;
}
}
}
return TRUE;
}
private function _capitalize_state()
{
if ( isset($_POST['state']) )
{
$_POST['state'] = strtoupper($_POST['state']);
}
}
}
helpers/mv_helper.php
<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
if ( ! function_exists('load_page') )
{
function load_page($obj, $page_name, $data = NULL)
{
$obj->load->view('templates/header');
$obj->load->view($page_name, $data);
$obj->load->view('templates/footer');
}
}
if ( ! function_exists('load_page_with_great_races_sidebar') )
{
function load_page_with_great_races_sidebar($obj, $page_name, $data = NULL)
{
$data['great_races'] = html_escape($obj->race_model->get_races());
$obj->load->view('templates/header');
$obj->load->view('templates/great_races_header');
$obj->load->view($page_name, $data);
$obj->load->view('templates/great_races_footer', $data);
$obj->load->view('templates/footer');
}
}
if ( ! function_exists('trim_all_post_data') )
{
function trim_all_post_data()
{
array_walk_recursive($_POST,
function (&$val)
{
$val = trim($val);
}
);
}
}
if ( ! function_exists('date_to_sql_date') )
{
function date_to_sql_date($date_string)
{
return date('Y-m-d', strtotime($date_string));
}
}
if ( ! function_exists('current_sql_datetime') )
{
function current_sql_datetime()
{
return date('Y-m-d G:i:s', time());
}
}
models/Race_model.php
<?php
class Race_model extends CI_Model
{
public function __construct()
{
$this->load->database();
$this->load->model('company_model');
$this->load->model('shift_model');
}
public function get_race($race_id_and_text = NULL)
{
// TODO: extract this to a function, I suspect it'll get used a lot
preg_match('/^(\d+)/', $race_id_and_text, $matches);
if ( $matches == NULL )
{
return NULL;
}
$this->db->where('race_id', $matches[0]);
$this->db->where('race_open_to_public', 1);
$query = $this->db->get('races');
$race = $query->row_array();
if ( $race )
{
$race = $this->_add_extra_race_fields($race);
}
return $race;
}
private function _add_extra_race_fields($race)
{
$race['race_details_url'] = base_url() . 'volunteers/view_race_details/' . $race['race_id'] . '-' . url_title($race['race_name']);
$race['race_sign_up_individual_url'] = base_url() . 'volunteers/sign_up_individual/' . $race['race_id'] . '-' . url_title($race['race_name']);
$race['race_sign_up_group_member_url'] = base_url() . 'volunteers/sign_up_group_member/' . $race['race_id'] . '-' . url_title($race['race_name']);
$race['race_sign_up_group_leader_url'] = base_url() . 'volunteers/sign_up_group_leader/' . $race['race_id'] . '-' . url_title($race['race_name']);
$race['race_uri'] = $race['race_id'] . '-' . url_title($race['race_name']);
if ( strlen($race['race_description']) > 170 )
{
$race['race_description_short'] = substr($race['race_description'], 0, 170) . " ...";
}
else
{
$race['race_description_short'] = $race['race_description'];
}
if ( strlen($race['race_description']) > 300 )
{
$race['race_description_medium'] = substr($race['race_description'], 0, 300) . " ...";
}
else
{
$race['race_description_medium'] = $race['race_description'];
}
$race['race_date_pretty'] = date('D M j, Y' , strtotime($race['race_date']));
$race['company_name'] = $this->company_model->get_company_name($race['company_id']);
$race['race_is_full'] = empty($this->shift_model->get_open_shift_ids($race['race_id'])) ? TRUE : FALSE;
return $race;
}
}
models/Volunteer_model.php
<?php
class Volunteer_model extends CI_Model
{
public function __construct()
{
$this->load->database();
}
public function sign_up_individual($parameter_data)
{
$race_date = $parameter_data['race']['race_date'];
$data = array(
'race_id' => $parameter_data['race']['race_id'],
'volunteer_email' => $this->input->post('email'),
'volunteer_firstname' => $this->input->post('firstname'),
'volunteer_lastname' => $this->input->post('lastname'),
'volunteer_birthdate' => date_to_sql_date($this->input->post('birthdate')),
'volunteer_age_on_shift_date' => floor((strtotime($race_date) - strtotime($this->input->post('birthdate'))) / 31556926),
'volunteer_address1' => $this->input->post('address1'),
'volunteer_city' => $this->input->post('city'),
'volunteer_state' => $this->input->post('state'),
'volunteer_zip' => $this->input->post('zip'),
'volunteer_phone' => $this->input->post('phone'),
'volunteer_shirt_size' => $this->input->post('shirt_size'),
'volunteer_emergency_name' => $this->input->post('emergency_name'),
'volunteer_emergency_phone' => $this->input->post('emergency_phone'),
'volunteer_group_name' => $this->input->post('group_name'),
'volunteer_group_leader' => $this->input->post('group_leader'),
'volunteer_waiver_date' => current_sql_datetime(),
'volunteer_waiver_ip' => $this->input->ip_address(),
'volunteer_added_date' => current_sql_datetime(),
'volunteer_added_bywhom' => NULL,
'volunteer_added_ip' => $this->input->ip_address()
);
return $this->db->insert('volunteers', $data);
}
public function get_volunteer($race_id, $firstname, $lastname, $email)
{
$this->db->where('race_id', $race_id);
$this->db->where('volunteer_firstname', $firstname);
$this->db->where('volunteer_lastname', $lastname);
$this->db->where('volunteer_email', $email);
$query = $this->db->get('volunteers');
$volunteer = $query->row_array();
return $volunteer;
}
}
General Questions
Some of the following things feel messy to me, and any specific feedback on them would be appreciated:
- I couldn't figure out a good way to do custom error pages for 403 and 404. 403 has no support at all, and 404 has limited support for custom error pages, but that support only works in a subset of cases. It does not work if you call the
show_404()method. So I decided to just load my own view whenever 404 occurs. I should probably extract that into its own helper function. _check_shifts()callback is a bit messy. I wonder if there's a better way to do form validation on an array of incoming form data in CodeIgniter.- My form preprocessing functions, such as
trim_all_post_data()and$this->_capitalize_state()are a bit messy. I'm surprised that preprocessing isn't better supported in the form_validation class. For example,$this->form_validation->preprocess($post_variable, $rule) - I feel like the separation of SQL into models is causing more SQL queries and denser SQL queries (querying more data than needed) than if I wrote the SQL directly into the controllers. I guess the code is better organized though, so perhaps it's worth it.
- I created methods in some of my models (
_add_extra_race_fields()) that take SQL database data and add additional fields that make the SQL data prettier. I think this is in the spirit of MVC Models. Each model class is supposed to contain all data about whatever the model is. - Is a helper class the best place for the helper functions mentioned above? It feels messy.
- I currently prefix all my SQL table fields with the table name. Is this good practice, or redundant? For example,
races.race_idvsraces.id
Security Questions
In terms of security, I think CodeIgniter does a great job of handling most of that. This website is infinitely more secure than my previous, non-framework websites. However...
- I'm surprised that CI doesn't support some kind of form throttling natively. That seems like a weakness, and I might end up writing a form throttling class to shore up that weakness.
- CSRF is a little annoying, especially as a developer. When testing forms, I have to re-type a ton of form information before submittting it. CSRF can also be annoying to the user, especially when the default 403 error page loads that has no links back to the main website. Then the user tries to refresh the page that threw the CSRF, and ends up submitting their form data again, and gets the CSRF error again. They end up trapped on the CSRF error page with no escape. I may end up turning CSRF off. What are the community's thoughts on CSRF?
- I experimented with using
html_escape()andurl_escape()in various places, and I decided that the best spot to usehtml_escape()was at the same time as calling the view method. In other words, as late as possible. That seems like the simplest and most thorough way to do it. When I did it earlier, such as in the model, I was having problems with my e-mail subject lines (Bob’s Tacosinstead ofBob's Tacos) - If you see any potential security vulnerabilities in this code, please comment on it.
