Is this MODEL method safe?

As far as I know where method is already escaping the strings, there is no need to do what he suggests.

Yes, you are right, in the docs you can see:

It also allows for safer queries, since the values are escaped automatically by the system.

https://www.codeigniter.com/user_guide/d...lder-class

This is true for every query builder function. However, you seem to be setting your sql in a variable to run like this:

PHP Code:

$this->db->query($sql); 


This is then NOT automatically escaped, so in that case, yes, you should escape the variables as your security advisor suggested.

Hope that helps,

Best wishes,

Paul

PS Well done for not rising to the comments about your actual coding.

No problem. We are here to learn. Can you please come up with some arguments?

Although I have to agree, it is somewhat 'clunky', but then again, if some of the better or more experienced programmers saw some of the code I churn out, they might justifiably have the same reaction. We all code what we can in the best way we know. The beauty of coding (IMHO) is there is always more to learn. Some people might say that if it works it is fine. I personally like beautiful code, but beauty is in the eye of the beholder of course. To me it is code that for the most part, makes sense when you see it, is easy to understand, almost plain, but laid out strictly and beautifully. However, I also love clever code and am a fan of code golf, the cleverness of some people in the formulation of code to do something in a particular way never ceases to amaze me.

Thanks for response. As you can see I am not using $this -> db -> query(). I am just passing that $sql string to the where method . Probably as you already know where method invokes the _wh method which at a certain point escapes the values if you set the _wh method argument $escape as TRUE. 

And yes you are right if I would use RAW queries and execute them with $this- > db-> query($query) I would definitely need to escape them before I call this method. 

Any other advices when playing around with queries except the ones mentioned in the docs?

When using the where method as intended, then yes the values are escaped.

PHP Code:

// Example 1: NOT escaped
$where = "field_1 = {$field_1_value}";
$where .= " AND field_2 = {$field_2_value}";
$this->db->where($where);

// Example 2: IS escaped
$this->db->where("field_1", $field_1_value);
$this->db->where("field_2", $field_2_value); 


Can you spot the difference?

You are constructing a where statement as I did in example 1.
When doing that you MUST escape manually

Passing a value to the where method is optional.
But it is only passed values that are auto escaped.

Hope this clears it up for you

When using the where method as intended, then yes the values are escaped.

PHP Code:

// Example 1: NOT escaped
$where = "field_1 = {$field_1_value}";
$where .= " AND field_2 = {$field_2_value}";
$this->db->where($where);

// Example 2: IS escaped
$this->db->where("field_1", $field_1_value);
$this->db->where("field_2", $field_2_value); 


Can you spot the difference?

You are constructing a where statement as I did in example 1.
When doing that you MUST escape manually

Passing a value to the where method is optional.
But it is only passed values that are auto escaped.

Hope this clears it up for you

In the end the _wh method is called. $escape param is always true so if I change this block:

PHP Code:

if ($escape === TRUE)
{
 $v = ' '.$this->escape($v);
 } 


into

PHP Code:

if ($escape === TRUE)
  {
$v = ' '.$this->escape($v);
$k = ' '.$this->escape($k);
} 


In this way I am able to use both ways. Correct?

When using the where method as intended, then yes the values are escaped.

PHP Code:

// Example 1: NOT escaped
$where = "field_1 = {$field_1_value}";
$where .= " AND field_2 = {$field_2_value}";
$this->db->where($where);

// Example 2: IS escaped
$this->db->where("field_1", $field_1_value);
$this->db->where("field_2", $field_2_value); 


Can you spot the difference?

You are constructing a where statement as I did in example 1.
When doing that you MUST escape manually

Passing a value to the where method is optional.
But it is only passed values that are auto escaped.

Hope this clears it up for you

In the end the _wh method is called. $escape param is always true so if I change this block:

PHP Code:

if ($escape === TRUE)
{
 $v = ' '.$this->escape($v);
 } 


into

PHP Code:

if ($escape === TRUE)
  {
$v = ' '.$this->escape($v);
$k = ' '.$this->escape($k);
} 


In this way I am able to use both ways. Correct?

First: Don't change CORE files. Bad idea

Second: If that was correct I wouldn't tell you that when only passing the first argument you MUST manually escape.
Adding that line will only escape the complete string, and not the values within the string that actually need escaping

When using the where method as intended, then yes the values are escaped.

PHP Code:

// Example 1: NOT escaped
$where = "field_1 = {$field_1_value}";
$where .= " AND field_2 = {$field_2_value}";
$this->db->where($where);

// Example 2: IS escaped
$this->db->where("field_1", $field_1_value);
$this->db->where("field_2", $field_2_value); 


Can you spot the difference?

You are constructing a where statement as I did in example 1.
When doing that you MUST escape manually

Passing a value to the where method is optional.
But it is only passed values that are auto escaped.

Hope this clears it up for you

In the end the _wh method is called. $escape param is always true so if I change this block:

PHP Code:

if ($escape === TRUE)
{
 $v = ' '.$this->escape($v);
 } 


into

PHP Code:

if ($escape === TRUE)
  {
$v = ' '.$this->escape($v);
$k = ' '.$this->escape($k);
} 


In this way I am able to use both ways. Correct?

First: Don't change CORE files. Bad idea

Second: If that was correct I wouldn't tell you that when only passing the first argument you MUST manually escape.
Adding that line will only escape the complete string, and not the values within the string that actually need escaping

I am not changing core classes. I am just replacing the default core class (https://www.codeigniter.com/userguide3/g...re-classes)

[quote pid='345488' dateline='1500495097']
Adding that line will only escape the complete string, and not the values within the string that actually need escaping
[/quote]

You are right. And I am aware that my way is not a standard practice. But in the end the whole string would be escaped (the risk of someone corrupting the database will be lower) and in the worst case there will be a database error when trying to execute the query which can be handled and logged for future analysis.

Correct?

You really don't want to admit that your approach is not the way to do things.

Why is it so hard for you to give in and change your code to use the Query Builder as intended.
Or wrap your passed in variables with $this->db->escape()

There is no half way solution when it comes to security. Why you would trust code that you know will cause an error when someone tries bad queries on your project is just baffling.

You said you are here to learn. Seems you are here to push what you think is oke to use. But I can safely say nobody here would ever use that approach in their code. That you can trust.

Note:
All core files can indeed be extended with the exception of the Database classes.
So your extra line of code would need to be added to the file ./system/database/DB_query_builder.php

https://www.codeigniter.com/userguide3/g...aries.html

You really don't want to admit that your approach is not the way to do things.

Why is it so hard for you to give in and change your code to use the Query Builder as intended.
Or wrap your passed in variables with $this->db->escape()

There is no half way solution when it comes to security. Why you would trust code that you know will cause an error when someone tries bad queries on your project is just baffling.

You said you are here to learn. Seems you are here to push what you think is oke to use. But I can safely say nobody here would ever use that approach in their code. That you can trust.

Note:
All core files can indeed be extended with the exception of the Database classes.
So your extra line of code would need to be added to the file ./system/database/DB_query_builder.php

https://www.codeigniter.com/userguide3/g...aries.html

Martin. I already said several times that I know it is not a good practice and doesn't respect the standards. I already know everything about the query builder. What I wanted to clarify is if my method is secure? 

I know that replacing core files is not a good thing. I know everything you said.

My question is. Using the method described in the previous posts (escaping the complete string) can still let someone do bad things with the query. I already know that the db will return an error most probably which can be handled and logged. So my question: would this still be secure?

No, it will not be secure. As this approach will cause unexpected behaviour and errors.

No, it will not be secure. As this approach will cause unexpected behaviour and errors.

This is what I am trying to do research on. What would the unexpected behaviours and errors be? Can you define some scenarious?

Thanks

@George.adrian

What you do is not about good/best practices or respecting standards. Simply, the line you add is wrong. The escape method has its purpose - it escapes values before they get inserted within the SQL. What you do is escaping string that itself is a SQL-fragment. This is nonsense - in this direction you have nothing to research about.