A contact form with CodeIgniter and Twig

Question

I am working on an online newspaper/blogging application with CodeIgniter 3.1.8 and Twig. I use the Twig template engine for the front-end views.

I have put together this simple contact form which uses a controller, a view, and no model.

The controller:

class Contact extends CI_Controller {

    public function __construct()
    {
        parent::__construct();
    }

    private $headers = "";
    private $to = '[email protected]'; 
    private $email_address = '';
    private $name = '';
    private $subject = ''; 
    private $message = '';
    private $body = '';

    private $message_success = false;
    private $message_fail = false;

    public function index(){    
        $this->form_validation->set_rules('name', 'Name', 'required');
        $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
        $this->form_validation->set_rules('subject', 'Subject', 'required');
        $this->form_validation->set_rules('message', 'Message', 'required');
        $this->form_validation->set_error_delimiters('<p class="form-error">', '</p>');

        if($this->form_validation->run() === FALSE) {
            $data['errors'] = validation_errors();
            $this->displayForm();
        } else {
            //Prepare mail
            $this->subject = "Website Contact Form: " . $this->input->post('subject');
            $this->name = $this->input->post('name');
            $this->email_address = $this->input->post('email');
            $this->message = $this->input->post('message');
            $this->body = "You have received a new message from your website contact form. Here are the details:\n\nName: $this->name\n\nEmail: $this->email_address\n\nMessage:\n$this->message";
            $this->headers = "From: [email protected]\n";
            $this->headers .= "Reply-To: $this->email_address"; 

            //Send mail
            $this->send_mail();
            $this->displayForm();
        }       
    }

    public function displayForm() {
        $data = $this->Static_model->get_static_data();
        $data['base_url'] = base_url("/");
        $data['pages'] = $this->Pages_model->get_pages();
        $data['categories'] = $this->Categories_model->get_categories();
        $data['tagline'] = "Contact us";
        $data['errors'] = validation_errors();
        $data['message_success'] = $this->message_success;
        $data['message_fail'] = $this->message_fail;

        $this->twig->addGlobal('contactForm',"themes/{$data['theme_directory']}/templates/contact.twig");
        $this->twig->display("themes/{$data['theme_directory']}/layout", $data);
    }
    
    //mail sender method
    public function send_mail() {
        if (mail($this->to, $this->subject, $this->body, $this->headers)) {
            $this->message_success = true;
        } else {
            $this->message_fail = true;
        }
    }
}

The contact.twig view:

<div class="wrapper style1">
  <div class="content">
    <div class="contact image fit flush">
      <img src="{{base_url}}themes/caminar/assets/images/woman-with-laptop.jpg" alt="Contact">
    </div>

    <h2>Contact us</h2>

    {% if message_success == true %}
    <div class="alert alert-success alert-dismissible text-center">
      <button type="button" class="close" data-dismiss="alert">&times;</button>
      Your message was sent. We will reply as soon as possible.
    </div>
    {% endif %}

    {% if message_fail == true %}
    <div class="alert alert-danger alert-dismissible text-center">
      <button type="button" class="close" data-dismiss="alert">&times;</button>
      Something went wrong while sending your message.
    </div>
    {% endif %}

    <div id="msgContainer" class="alert alert-hidden alert-success mb-4"></div>

    <form name="contactForm"
      id="ajaxForm"
      action="{{base_url}}contact"
      method="post"
      data-successmsg="Your message was sent. We will reply as soon as possible." 
      data-failmsg="Sorry, we could not deliver your message"
      novalidate>
      <div class="row uniform">
        <div class="form-controll 12u$">
          <input type="text" class="form-control px-1 {{ form_error('name') ? 'error' }}" placeholder="Name" name="name" id="name" value="{{set_value('name' )}}" data-rule-required="true">
          {{ form_error('name') ? form_error('name') }}
        </div>
        <div class="form-controll 12u$">
          <input type="email" class="form-control px-1 {{ form_error('email') ? 'error' }}" placeholder="Email" name="email" id="email" value="{{set_value('email' )}}" data-rule-required="true">
          {{ form_error('email') ? form_error('email') }}
        </div>
      </div>
      <div class="row uniform">
        <div class="form-controll 12u$">
          <input type="text" class="form-control px-1 {{ form_error('subject') ? 'error' }}" placeholder="Subject" name="subject" id="subject" value="{{set_value('subject' )}}" data-rule-required="true">
          {{ form_error('subject') ? form_error('subject') }}
        </div>
        <div class="form-controll 12u$">
          <textarea rows="5" class="form-control px-1 {{ form_error('message') ? 'error' }}" placeholder="Message" name="message" id="message" data-rule-required="true">{{set_value('message')}}</textarea>
          {{ form_error('message') ? form_error('message') }}
        </div>
        <div class="12u$">
          <button type="submit" class="button special fit" id="sendMessageButton">Send</button>
        </div>
      </div>
    </form>
  </div>
</div>

The error and success messages at the top of the above view play a role only if JavaScript is disabled in the browser, otherwise the messages are delivered via jQuery Ajax:

(function($) {
  // Add comments via AJAX
  $("#ajaxForm").validate({
    rules: {
      email: {
        email: true
      }
    },

    submitHandler: function(form) {
      var form = $("#ajaxForm"),
      $fields = form.find('input[type="text"],input[type="email"],textarea'),
      $succesMsg = form.data('successmsg'),
      $failMsg = form.data('failmsg'),
      url = form.attr('action'),
      data = form.serialize();
      $.ajax({
        type: "POST",
        url: url,
        data: data,
        success: function() {
          $('#msgContainer').text($succesMsg).slideDown(250).delay(2500).slideUp(250);
          $fields.val('');
        },
        error: function() {
          $('#msgContainer').removeClass('alert-success').addClass('alert-danger')
                            .text($failMsg).slideDown(250).delay(2500).slideUp(250);
        }
      });
    }
  });
})(jQuery); 

The form is validated with the jQuery Validation Plugin.

The script above is designed to "fit" multiple forms as long as they have an attribute id="ajaxForm".

Concerns:

1. Does the contact form have security weaknesses?
2. Is the lack of a model an issue?

Answer 1

• Method declarations PSR-12: The opening brace MUST go on its own line

• Get into the habit of declaring argument types and return types whenever possible throughout your application -- it will help you to maintain good, stable code. PSR-12: When you have a return type declaration present, there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parenthesis with no spaces between the two characters.

• I recommend consistently using camelcase versus snake_case throughout your application. This would serve as an indicator in many instances whether an entity is your work or someone elses (CI's entity or PHP's entity). This is in regards to send_mail(), $email_address, get_pages(), etc. The exception that I make to this is when declaring data- attributes in the markup -- I use all lowercase.

• I see index() doing two things, but I would rather see it have a single responsibility - displaying the form. I was going to later advise that send_mail() should be a protected method, because it seems highly unlikely that you would call that method directly. However, I think it would be best to declare a public ajaxMail() method which can only be accessed when the incoming request is from ajax AND contains valid form data.

• When you are merging associative arrays to form $data use the union operator to DRY your code.

  $data = $this->Static_model->get_static_data()
          + [
                'base_url' => base_url("/");
                'pages' => $this->Pages_model->get_pages(),
                'categories' => $this->Categories_model->get_categories(),
                'tagline' => "Contact us",
                'errors' => validation_errors();
                'message_success' => $this->message_success,
                'message_fail' => $this->message_fail
          ];
  

• I recommend (and only use) PHPMailer versus native mail().

• After splitting the index() method and creating the ajaxMail() method, it will be easy and clear to see the new method will always end by printing a json_encoded string as the response data. This eliminates the need to declare class level properties private $message_success and $message_fail (which are unconditionally declared when the class is instantiated) that only one method will use. The same can be said of the other mail-related properties -- they'll only be used by one method, so keep them isolated there.

• As a consequence of this major refactor, index() will only be calling $this->displayForm();, so there isn't much sense in abstracting the functionality into its own method. Pull that logic into index() and omit the displayForm() method. Now your class has two methods that do two specific, singular things: index() loads the view, and ajaxMail() processes all mail sending requests.

• In the jquery, you are immediately overwriting form at submitHandler: function(form) {, so just omit the argument from the call.

• $fields is a single-use variable, so simply don't declare it.

  form.find('input[type="text"],input[type="email"],textarea').val('');
  

  Actually, it will be cleaner if you target the form fields by their common class.

  $('.form-control').val('');
  

• url = form.attr('action') and data = form.serialize() are also single-use declarations; just write the values directly into the ajax object.

• I would be receiving the response json passed back from ajaxMail() in success: function(response) {. If there was an error, you won't be receiving the json data because something fatal happens (or you can pass a header() declaration ahead of the json response to force the collection of your json in the error handler.

• I don't know if the lack of a model is a problem, but I'd probably engage PHPMailer as a library.

• Are there vulnerabilities? Well, is this feature behind a password-protected wall? You are unconditionally dispatching these emails, are you happy that there is no limiting logic in your code? You have implemented validations, but no sanitizations. You aren't stripping any potential html markup from the user's message, so if you are concerned that folks might be sending dodgy messages, you should probably strip html tags and consider html encoding characters.

Okay, I typed this all up on my mobile and now I am cross-eyed. Good luck with whatever pieces of this review that you decide to implement.

Answer 2

You can improve your security of your project, implementing XSS and CSRF configuration below.

On application/config/config.php

$config['global_xss_filtering'] = TRUE;
$config['csrf_protection']  = TRUE;

This is very important for forms.