CodeIgniter Forums

Full Version: Have I made form function secure enough
You're currently viewing a stripped down version of our content. View the full version with proper formatting.
I would like to know if I have made my form function secure enough.

So if user try's to access form with out a id it redirects etc but also trying make it protected as well.

I have not enabled CSRF should I?

PHP Code:
<?php

class Banners extends CI_Controller {

    public 
$error = array();

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

    public function 
edit() {
        if (
$this->input->server('REQUEST_METHOD') == 'POST') {

        }

        
$this->get_form_edit();

    }

    public function 
index() {
        
$this->get_list();
    }

    protected function 
get_list() {
        
$data['template'] = 'template/banners/banner_list_view';
        
$this->load->view('template/common/template_view'$data);
    }

    protected function 
get_form_edit() {
        
$banner_form_id = isset($_GET['banner_form_id']) ? $_GET['banner_form_id'] : "";

        if (
$banner_form_id) {

            
$data['banner_form_id'] = $banner_form_id;

            
$data['placeholder'] = 'holder.js/100%x75';

            
$data['template'] = 'template/banners/banner_form_edit_view';
            
$this->load->view('template/common/template_view'$data);

        } else {

            
redirect('c=banners');

        }
    }



Is there any thing need to add in view?


PHP Code:
<div class="container">

<
div class="row">

<
div class="col-lg-3 col-md-3 col-sm-12 col-xs-12">

</
div>

<
div class="col-lg-9 col-md-9 col-sm-12 col-xs-12">

<?
php echo form_open_multipart('c=banners&f=edit&banner_form_id=' $banner_form_id);?>

<table id="banners-table" class="table">

<thead></thead>

<tbody></tbody>
    
</table>

<?php echo form_close();?>

</div>

</div>

</div> 
Hello Wolfgang!

I would recommend doing checks and routing for method types in your routes.php. The manual will show you how you can assign the different HTTP methods to different routes.

Otherwise, I don't see any benefit of creating protected routes like you are doing. There shouldn't be any benefit of calling $this->get_list from your index function when compared to simply loading the view from your index function. One thing that a lot of programmers make a mistake on is they think that by making things more complicated makes the application more secure. This cannot be further from the truth.

I'm not sure what type of security you are looking for, but I'd recommend setting up your security somewhere else. Using an Auth library or creating a function in your MY_Controller that you can call to verify your users might be a place to start. But I do warn against building your own Auth library if you are not experienced in it. Authentication is not a piece you want to "build as you learn"
You are not validating the value of $_GET['banner_form_id'], you only check if it exists. Based on the name of the variable I would guess it would represents an integer based id. Eventhough GET variables are filtered by the config (default value: $config['permitted_uri_chars'] = 'a-z0-9~%.:_\-'Wink and your code does not represents any harmful processing (although a bit over-complexing solution to a simple situation).

I would suggest some small modification like:
PHP Code:
$banner_form_id intval$this->input->get('banner_form_id')); 

About your CSRF question. Genaraly there are few reasons why you should not use it although the impact of a succesfull CSRF attack depends on what functionality the form has and the role of the current user.
For more details I would refer you to: https://www.owasp.org/index.php/Cross-Si...heat_Sheet
If not used conversion base other than 10 (which in this scenario would be a case), (int) is faster than (intval).