1
\$\begingroup\$

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('noreply@marathonvolunteers.com', $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_id vs races.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() and url_escape() in various places, and I decided that the best spot to use html_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&#8217;s Tacos instead of Bob's Tacos)
  • If you see any potential security vulnerabilities in this code, please comment on it.
\$\endgroup\$
  • \$\begingroup\$ If you want to answer, please do so soon. I will probably award the bounty on Wed 10/19 \$\endgroup\$ – AdmiralAdama Oct 19 '16 at 5:35
3
+50
\$\begingroup\$

Let's start with your controller (controllers/Volunteers.php):

  1. You have loaded list of models in __construct() which is not a good practice. Before loading list of models in __construct(), you must be sure, if you need methods from all the loaded models in most of the methods of your controllers.

  2. You loaded all the models without giving them a short object name. Although it is not necessary to give short object name for the loaded model but it saves typing.

    Try something like this:

    $this->load->model('race_model','RM'); // use like $this->RM->method();
    $this->load->model('shift_model',"SM"); // use like $this->SM->method();
    
  3. Using $_POST Global VAR is not good idea as Codeigniter provides $this->input->post() for accessing $_POST array.

  4. function _capitalize_state() is created to change the case of variables only which is waste. you can use strtoupper in validation rules, Codeigniter supports that.

    You can use it like this:

    $this->form_validation->set_rules('state', 'State', 'required|strtoupper');
    
  5. function trim_all_post_data() is a waste. You can use trim function in form validation like this:

    $this->form_validation->set_rules('state', 'State', 'trim|required|strtoupper');
    
  6. For error-handling, you must use error handling function in codeigniter. i.e. show_error(), error_404(), etc. Read the guide here.

  7. You can create a separate function for form validation in your model so that code will not look messy.

    public function __validate()
        {
            $this->load->library("form_validation");
            $V = array(
                array(
                        'field' => 'name',
                        'label' => 'Full Name',
                        'rules' => 'trim|required|min_length[2]|max_length[64]|full_name'
                ),
                array(
                        'field' => 'email',
                        'label' => 'Email',
                        'rules' =>"trim|required|valid_email|is_unique[users.email]",
    
                ));
    
    $this->form_validation->set_rules($V);
            if($this->form_validation->run()==FALSE)
            {
                return false;
            }
    
            return true;
    }
    

    Later on, you can call that in your controller:

    if($this->model_name->__validate())
    {
        //register me
    }
    

    OR, you can save your validation rules to config file. See the form validation guide for Codeigniter on official website and scroll to Saving Sets of Validation Rules to a Config File.

About helpers/mv_helper.php

I already commented about your trim_all_post_data() function above.

For date manipulation function I would say to use date_helper from Codeigniter instead of creating your own functions. We must avoid to write a function if we have inbuilt PHP functions like return date('Y-m-d G:i:s', time());. This single line can be used anywhere and we don't need to pass second arg time() to date it automatically takes current time.

Moreover, you can save your date format into constants and you can pass them like return date(FORMAT_YMD_GIS). This way, you can use multiple formats on multiple sections of the website, you function returns single format only.

I appreciate your code for the load_page function in the helper. These kind of functions are really helpful in loading views. We can load one view into another view as well in Codeigniter.

models/Race_model.php

public function get_race($race_id_and_text = NULL)

You must not use $race_id_and_text kind of variable and process them, if you have to bring data based on race_id and race_open_to_public. Instead, you can pass two integer variables to the function.

models/Volunteer_model.php

volunteer_email putting "volunteer" to each field name is not a good idea, But if you put that for some special reason. you must use the same name in form to make your submission simpler using array_helper like this:

In view form:

<input type="text" name="volunteer_email" />
<input type="text" name="volunteer_firstname" />
<input type="text" name="volunteer_lastname" />

In the model method for inserting data:

$data = elements(array('volunteer_email','volunteer_firstname','volunteer_lastname'), $_POST); 
$this->db->insert('volunteers',$data);
//don't worry about $_POST now you came here after validation;

Note: You must load array_helper to use the elements function.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.