2
\$\begingroup\$

I am working on a online newspaper/blogging application with CodeIgniter 3.1.8 and Twig.

The application is meant to offer developers and designers the possibility to very easily turn their HTML templates into database-powered websites. To achieve this, I used the Twig template engine, for the frontend.

The main theme has a newsletter subscription form, which made it necessary for the CMS to have a newsletter subscribers management system.

The subscribers list

<div class="card bg-light w-100">
  <div class="card-header d-flex p-2">
    <h6 class="text-dark m-0 align-self-center">Newsletter list</h6>
    <?php echo form_open(base_url('dashboard/subscribers/export'),  ['class' => 'ml-auto']); ?>
      <button type="submit" class="btn btn-sm btn-success"><i class="fa fa-file mr-1"></i> Export CSV</button>
    <?php echo form_close(); ?>
  </div>
  <div class="card-body bg-white p-0">
    <?php if($total_subscribers > 0):?>
      <div class="table-responsive">
        <table class="table table-striped table-sm mb-0 w-100">
          <thead>
            <tr class="row m-0">
              <th class="w-5">#</th>
              <th class="w-50">Email</th>
              <th class="w-25">Subscription date</th>
              <th class="w-20 text-right">Actions</th>
            </tr>
          </thead>
          <tbody>
            <?php foreach ($subscribers as $index => $subscriber): ?>
              <tr id="<?php echo $subscriber->id; ?>" class="row m-0">
                <td class="w-5"><?php $count = $index + 1; echo $count + $offset; ?></td>
                <td class="w-50"><?php echo $subscriber->email; ?></td>
                <td class="w-25"><?php echo $subscriber->subscription_date; ?></td>
                <td class="w-20 text-right">
                  <div class="btn-group">
                    <a href="<?php echo base_url('dashboard/subscribers/edit/' . $subscriber->id); ?>" title="Edit" class="btn btn-sm btn-success"><i class="fa fa-pencil-square-o"></i> Edit</a>
                    <a href="<?php echo base_url('dashboard/subscribers/delete/' . $subscriber->id); ?>" title="Delete" class="delete-subscriber btn btn-sm btn-success"><i class="fa fa-trash"></i> Delete</a>
                  </div> 
                </td>
              </tr>
            <?php endforeach ?>
          </tbody>
        </table>
      </div>
      <div class="card-footer bg-white px-0 py-<?php echo $total_subscribers > $limit ? '1' : '0'?>">                      
        <?php if($total_subscribers > $limit):?>
          <?php $this->load->view("dashboard/partials/pagination");?>
        <?php endif ?>
      </div>              
      <?php else: ?>
        <p class="text-center my-2">No subscribers</p>
      <?php endif ?>
    </div>
  </div>
</div>

The model:

class Newsletter_model extends CI_Model {

    public function subscriber_exists() {   
        $query = $this->db->get_where('newsletter', ['email' => $this->input->post('email')]);
        return $query->num_rows() > 0;
    }

    public function get_num_rows() {
        $query = $this->db->get('newsletter');
        return $query->num_rows(); 
    }

    // Insert subscriber
    public function addSubscriber() {
        $data = [
            'email' => $this->input->post('email'),
            'subscription_date' => date('Y-m-d H:i:s')
        ];
        return $this->db->insert('newsletter', $data);
    }

    // Fetch subscribers
    public function getSubscribers($limit, $offset){
        $this->db->select('newsletter.*');
        $this->db->order_by('newsletter.id', 'ASC');
        $this->db->limit($limit, $offset);
        $query = $this->db->get('newsletter');
        return $query->result();
    }

   // Edit subscriber data
    public function editSubscriber($id){
        $query = $this->db->get_where('newsletter', array('id' => $id));
        if ($query->num_rows() > 0) {
            return $query->row();
        }
    }

    // Update subscriber data
    public function updateSubscriber($id) {
        $data = [
            'email' => $this->input->post('email')
        ];

        $this->db->where('id', $id);
        return $this->db->update('newsletter', $data);
    }

    // Remove subscriber
    public function deleteSubscriber($id) {
        return $this->db->delete('newsletter', array('id' => $id));
    }

    // Expot subscribers
    public function fetchSubscribers() {
        $this->db->select('email, subscription_date');
        $this->db->order_by('newsletter.id', 'DESC');
        $query = $this->db->get('newsletter');
        return $query->result();
    }
}

The controller

class Subscribers extends CI_Controller
{
    public function __construct()
    {
        parent::__construct();
    }
    
    public function index()
    {
        
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        } else {
            // Admin ONLY area!
            if (!$this->session->userdata('user_is_admin')) {
                redirect($this->agent->referrer());
            }
        }
        
        $this->load->library('pagination');
        $config['base_url']             = base_url("dashboard/subscribers");
        $config['query_string_segment'] = 'page';
        $config['total_rows']           = $this->Newsletter_model->get_num_rows();
        $config['per_page']             = 10;
        
        if (!isset($_GET[$config['query_string_segment']]) || $_GET[$config['query_string_segment']] < 1) {
            $_GET[$config['query_string_segment']] = 1;
        }
        
        $limit  = $config['per_page'];
        $offset = ($this->input->get($config['query_string_segment']) - 1) * $limit;
        $this->pagination->initialize($config);
        
        $data                      = $this->Static_model->get_static_data();
        $data['subscribers']       = $this->Newsletter_model->getSubscribers($limit, $offset);
        $data['offset']            = $offset;
        $data['limit']             = $limit;
        $data['total_subscribers'] = $config['total_rows'];
        
        $this->load->view('dashboard/partials/header', $data);
        $this->load->view('dashboard/subscribers');
        $this->load->view('dashboard/partials/footer');
    }
    
    public function edit($id)
    {
        // Only logged in users can edit subscribers
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        }
        
        $data               = $this->Static_model->get_static_data();
        $data['subscriber'] = $this->Newsletter_model->editSubscriber($id);
        
        $this->load->view('dashboard/partials/header', $data);
        $this->load->view('dashboard/edit-subscriber');
        $this->load->view('dashboard/partials/footer');
    }
    
    public function update()
    {
        // Only logged in users can update user profiles
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        }
        
        $id = $this->input->post('subscriber');
        
        $data               = $this->Static_model->get_static_data();
        $data['subscriber'] = $this->Newsletter_model->editSubscriber($id);
        
        $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
        $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');
        
        if (!$this->form_validation->run()) {
            $this->load->view('dashboard/partials/header', $data);
            $this->load->view('dashboard/edit-subscriber');
            $this->load->view('dashboard/partials/footer');
        } else {
            $this->Newsletter_model->updateSubscriber($id);
            $this->session->set_flashdata('subscriber_updated', 'The email address was updated');
            redirect('dashboard/subscribers');
        }
    }
    
    public function delete($id)
    {
        if ($this->Newsletter_model->deleteSubscriber($id)) {
            $this->session->set_flashdata('subscriber_delete_success', "The subscriber was deleted");
        } else {
            $this->session->set_flashdata('subscriber_delete_fail', "Failed to delete subscriber");
        }
        redirect('dashboard/subscribers');
    }
    
    public function export()
    {
        $data        = $this->Static_model->get_static_data();
        $subscribers = $this->Newsletter_model->fetchSubscribers();
        
        $file_name = 'subscribers_' . date('Ymd') . '.csv';
        header("Content-Description: File Transfer");
        header("Content-Disposition: attachment; filename=$file_name");
        header("Content-Type: application/csv;");
        
        // CSV creation 
        $file = fopen(BASEPATH . '../downloads/csv/' . $file_name, 'w');
        $header = array("Email", "Subscription Date");
        fputcsv($file, $header);
        foreach ($subscribers as $subscriber) {
            fputcsv($file, array($subscriber->email, $subscriber->subscription_date));
        }
        fclose($file);
        redirect('dashboard/subscribers');
    }
}

Concerns

  1. Is the subscribers management system (especially the CSV export functionality) secure?

  2. Is it far from optimal from the point of view of code quality?

\$\endgroup\$
2
  • \$\begingroup\$ ...I don't wish to repeat myself. You should not be accessing any superglobals in your model -- but I've said this already. By not implementing the insights that I've provided in the past, I can only assume that you do not value my insights, so I will not be reviewing. Please reread all of the reviews that you have received and then edit your question to implement everything that you've learned. \$\endgroup\$ – mickmackusa Mar 22 at 7:51
  • \$\begingroup\$ @mickmackusa Where do I do that exactly? What, from all I do wrong, lowers the application's security? \$\endgroup\$ – Razvan Zamfir Mar 22 at 10:42
2
\$\begingroup\$

those are my considerations:

  • Do not use Globals like $_POST, $_GET directly. Use that provided by CI, are already XSS cleaned and analyzed
  • Always validate IDs, check that is an integer at least because, in theory, I can write an SQL there instead of an ID
  • Do not use directly $this->input->post in the model. Always validate your data before the model. If something is not ok, you can handle it in the controller, not during an SQL query construction.
  • The export function is not protected. I can launch a DDOS attack there and wait to see your server being overloaded by tons of requests to handle CSV files (very expensive)

I'm sure that someone else can provide maybe more information or considerations

\$\endgroup\$
2
1
\$\begingroup\$

There were some issues with permissions: non-admins were able to edit, delete and export subscribers. Here is the fixed code:

class Subscribers extends CI_Controller
{
    public function __construct()
    {
        parent::__construct();
    }
    
    public function index()
    {
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        } else {
            // Admin ONLY area!
            if (!$this->session->userdata('user_is_admin')) {
                $this->session->set_flashdata('admin_only_pages', 'Only admins are allowed to manage subscribers');
                redirect('dashboard');
            }
        }
        
        $this->load->library('pagination');
        $config['base_url']             = base_url("dashboard/subscribers");
        $config['query_string_segment'] = 'page';
        $config['total_rows']           = $this->Newsletter_model->get_num_rows();
        $config['per_page']             = 10;
        
        if (!isset($_GET[$config['query_string_segment']]) || $_GET[$config['query_string_segment']] < 1) {
            $_GET[$config['query_string_segment']] = 1;
        }
        
        $limit  = $config['per_page'];
        $offset = ($this->input->get($config['query_string_segment']) - 1) * $limit;
        $this->pagination->initialize($config);
        
        $data                      = $this->Static_model->get_static_data();
        $data['subscribers']       = $this->Newsletter_model->getSubscribers($limit, $offset);
        $data['offset']            = $offset;
        $data['limit']             = $limit;
        $data['total_subscribers'] = $config['total_rows'];
        
        $this->load->view('dashboard/partials/header', $data);
        $this->load->view('dashboard/subscribers');
        $this->load->view('dashboard/partials/footer');
    }
    
    public function edit($id)
    {
        // Admin ONLY area!
        if (!$this->session->userdata('user_is_admin')) {
            $this->session->set_flashdata('admin_only_pages', 'Only admins are allowed to edit subscribers');
            redirect('dashboard');
        }
        
        $data               = $this->Static_model->get_static_data();
        $data['subscriber'] = $this->Newsletter_model->editSubscriber($id);
        
        $this->load->view('dashboard/partials/header', $data);
        $this->load->view('dashboard/edit-subscriber');
        $this->load->view('dashboard/partials/footer');
    }
    
    public function update()
    {
        // Only logged in users can update subscribers
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        }
        
        $id = $this->input->post('subscriber');
        
        $data               = $this->Static_model->get_static_data();
        $data['subscriber'] = $this->Newsletter_model->editSubscriber($id);
        
        $this->form_validation->set_rules('email', 'Email', 'required|trim|valid_email');
        $this->form_validation->set_error_delimiters('<p class="error-message">', '</p>');
        
        if (!$this->form_validation->run()) {
            $this->load->view('dashboard/partials/header', $data);
            $this->load->view('dashboard/edit-subscriber');
            $this->load->view('dashboard/partials/footer');
        } else {
            $this->Newsletter_model->updateSubscriber($id);
            $this->session->set_flashdata('subscriber_updated', 'The email address was updated');
            redirect('dashboard/subscribers');
        }
    }
    
    public function delete($id)
    {
        
        // Only admins can delete subscribers
        if ($this->session->userdata('user_is_admin')) {
            
            // Do delete
            if ($this->Newsletter_model->deleteSubscriber($id)) {
                $this->session->set_flashdata('subscriber_delete_success', "The subscriber was deleted");
            } else {
                $this->session->set_flashdata('subscriber_delete_fail', "Failed to delete subscriber");
            }
            redirect('dashboard/subscribers');
            
        } else {
            $this->session->set_flashdata('admin_only_pages', 'Only admins are allowed to delete subscribers');
            redirect('dashboard');
        }
    }
    
    public function export()
    {
        
        if (!$this->session->userdata('is_logged_in')) {
            redirect('login');
        } else {
            // Admin ONLY area!
            if (!$this->session->userdata('user_is_admin')) {
                $this->session->set_flashdata('admin_only_pages', 'Only admins are allowed to export subscribers');
                redirect('dashboard');
            }
        }
        
        $data        = $this->Static_model->get_static_data();
        $subscribers = $this->Newsletter_model->fetchSubscribers();
        
        $file_name = 'subscribers_' . date('Ymd') . '.csv';
        header("Content-Description: File Transfer");
        header("Content-Disposition: attachment; filename=$file_name");
        header("Content-Type: application/csv;");
        
        // CSV creation 
        $file   = fopen(BASEPATH . '../downloads/csv/' . $file_name, 'w');
        $header = array(
            "Email",
            "Subscription Date"
        );
        fputcsv($file, $header);
        foreach ($subscribers as $subscriber) {
            fputcsv($file, array(
                $subscriber->email,
                $subscriber->subscription_date
            ));
        }
        fclose($file);
        redirect('dashboard/subscribers');
    }
}
\$\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.