Post Reply 
 
Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Inconsistent escaping of problem characters for SQL (' & etc.)
02-25-2014, 09:55 AM
Post: #1
Inconsistent escaping of problem characters for SQL (' & etc.)
While setting up my chart of accounts I notice the ampersand exists in the default account "Furniture & Fixtures", however adding a new account with the ampersand results in it being stored in the database and returned as "&" the HTML escape code (which actually isn't processed by the
browser for some reason (this is true for GLAccounts.php)).

For AccountSections.php the same problem occurs, actually it's a bit worse as the escape code will be escaped multiple times AND will generate an error (tested with single quote AKA I renamed "Financed By" with "Owner's Equity")

I understand the importance of escaping some characters for the sake of SQL but those operations should be invisible on output from SQL, right?
Find all posts by this user
Quote this message in a reply
02-25-2014, 10:21 AM (This post was last modified: 02-25-2014 10:30 AM by icedlava.)
Post: #2
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
Hi serakfalcon,

I have also had this issue and it begins in the way that webERP currently handles session, post, and get vars along with the db escape function code, and addslashes/stripslashes used is also a related problem.

I have raised this issue previously, but perhaps it was on the list or in emails as I could not find my posts (yet).

If you need isolated 'fixes' for the account issues you've noticed I have them and can commit them to the code (this is treating 'symptoms' that should not exist if the data is handled correctly). It is code that should not have to be there and could be removed if the root problem is fixed. In this case it is rather innocuous symptom but it could be worse elsewhere.

Data should be treated differently depending on whether it is going into the database, displayed for HTML, sent to a shell command, used with AJAX etc. In webERP it is currently all being treated the same way.

There needs to be a system wide solution in webERP to prepare data and escaping dependent on context, rather than lump everything together and treat the data the same way.

There are isolated fixes in the codebase.

I have implemented a solution in my own private branch of webERP but it requires.
1. Adding correct code to process the data
2. Implement it consistently across the codebase - timeconsuming
3. make provision for or fix existing data that has been incorrectly 'processed' and saved in the database (depending on the extent of the problem).

Cheers,



Noticed this thread when searching for my posts (probably was by email now I think of it).

http://www.weberp.org/forum/showthread.php?tid=833

This is caused by the exact same issue and along with replicating slashes in various database tables can cause big problems - problem is treating the data all the same rather than by context.

The thread discusses band aids being applied to symptoms not a solution.
Find all posts by this user
Quote this message in a reply
02-25-2014, 03:14 PM (This post was last modified: 02-25-2014 03:14 PM by serakfalcon.)
Post: #3
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
Quote:There needs to be a system wide solution in webERP to prepare data and escaping dependent on context, rather than lump everything together and treat the data the same way.

Yes, this exactly. On another related note, all the SQL queries inside the php to render the site is a little scary. MVC anyone? Or have I just been around developers who are a little too strict Huh

In my own case so far I've just gone into the SQL table and modified the offending entries, which has worked fine for me, the issue is, I shouldn't HAVE to do that.
Find all posts by this user
Quote this message in a reply
02-26-2014, 05:33 AM
Post: #4
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
No I am opposed to MVC as I actively discourage abstraction where there is no scope for code reuse. We are all about readability and accessibility of the code contrary to best computer science practice

Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Visit this user's website Find all posts by this user
Quote this message in a reply
02-26-2014, 06:08 AM
Post: #5
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
(02-25-2014 09:55 AM)serakfalcon Wrote:  While setting up my chart of accounts I notice the ampersand exists in the default account "Furniture & Fixtures", however adding a new account with the ampersand results in it being stored in the database and returned as "&" the HTML escape code (which actually isn't processed by the
browser for some reason (this is true for GLAccounts.php)).

This is due to the account description being incorrectly encoded for HTML special characters on line 321. Removing this should allow it to be saved and viewed correctly. Line 321 changes from:

htmlspecialchars($myrow[1],ENT_QUOTES,'UTF-8'),
to
$myrow[1],

Thanks
Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
02-26-2014, 07:03 AM (This post was last modified: 02-26-2014 07:04 AM by icedlava.)
Post: #6
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
Hi Tim
(02-26-2014 06:08 AM)Forums Wrote:  This is due to the account description being incorrectly encoded for HTML special characters on line 321. Removing this should allow it to be saved and viewed correctly. Line 321 changes from:
htmlspecialchars($myrow[1],ENT_QUOTES,'UTF-8'),
to
$myrow[1],

Actually this is incorrect.

Line 321 is correct - you should always do this before displaying data as HTML.

What is incorrect is that the data has been automatically encoded in sessions.inc before saving to the database, and so has to be htmlspecialchar_decoded at line 38 in:

$sql = "UPDATE chartmaster SET accountname='" . html_entity_decode($_POST['AccountName']) . "'

This technically is also incorrect as the strings should go through DB encoding (mysqli_real_escape) however we cannot as the DB_escape_string function incorreclty uses htmlentities again on the data.

The point being made is that treatment of data is inconsistent leading to incorrectly prepared data being added to the database. It is causing bad data getting into the database at many points and inconsistent use of the DB_escape_string which cannot be used as it has htmlspecialchars in it!

Worse, all the vars (get, post, session) are being DB_escape_string processed in session.inc!

I've fixed these functions in my own branch, but there is other code that needs to then be added for processing vars that should be called in appropriate places.

A proper solution should be applied in the code base for webERP.

Cheers,
Find all posts by this user
Quote this message in a reply
02-26-2014, 07:11 AM
Post: #7
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
(02-26-2014 07:03 AM)icedlava Wrote:  Hi Tim
(02-26-2014 06:08 AM)Forums Wrote:  This is due to the account description being incorrectly encoded for HTML special characters on line 321. Removing this should allow it to be saved and viewed correctly. Line 321 changes from:
htmlspecialchars($myrow[1],ENT_QUOTES,'UTF-8'),
to
$myrow[1],

Actually this is incorrect.

Line 321 is correct - you should always do this before displaying data as HTML.

Then I bow to your superior knowledge :-)

I had always understood that once the data had been "sanitised" it didn't need to be "re-sanitised" but I am just going off a few text books rather than any in depth knowledge.

Tim
Visit this user's website Find all posts by this user
Quote this message in a reply
02-26-2014, 08:21 AM
Post: #8
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
Hi Tim,
(02-26-2014 07:11 AM)Forums Wrote:  
(02-26-2014 07:03 AM)icedlava Wrote:  Actually this is incorrect.

Line 321 is correct - you should always do this before displaying data as HTML.

Then I bow to your superior knowledge :-)
I doubt it, probably I read a different book :-)

But sure, technically there should always be preparation of the data before being displayed as HTML by cleaning up with HTMLspecialchars (or HTMLentities perhaps depending ..) and you do not want to double it up by doing it again.
Quote:I had always understood that once the data had been "sanitised" it didn't need to be "re-sanitised"
Well, what is 'sanitised' in this context?
The data is going to be displayed in HTML so should be HTMLspecialchars encoded.

If we htmlspecialchars_decode the data before we save it again (at line 38) it should be displayed correctly with the htmlspecialchars line at 321 as it should be (or htmlentities in some cases is enough).

If data wasn't htmlspecialchars encoded at line 321 it would be a problem in the case where there were entities that should be.

The fact is everything that is a var is htmlspecialchar encoded in weberp in the sessions.inc or db_escape_string function and that is a problem when it gets saved in the database with the entities encoded.
Find all posts by this user
Quote this message in a reply
02-26-2014, 05:36 PM
Post: #9
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
(02-26-2014 05:33 AM)phil Wrote:  No I am opposed to MVC as I actively discourage abstraction where there is no scope for code reuse. We are all about readability and accessibility of the code contrary to best computer science practice

I think there's some room for flexibility when it comes to adopting some of the 'good' things about MVC.
Diving into the code a little bit more, in webERP's case I think some of the 'View' components (table,controls, menus etc.) could be abstracted with no loss of readability or accessibility but it would allow for a lot more flexibility wrt the display. Specifically in my case, since I will likely have to re-write (or at least examine) a good part of the code for my purposes, I'm thinking of integrating Bootstrap while I'm at it. Doing something like that without restructuring the 'display' components of the code would be a little bit of a nightmare. It doesn't bother me if that's not of interest to you upstream into core but I think it would help give webERP a more 'modern' look, with a lot more flexibility, and also has the added benefit that front-end designers won't have to worry about dealing with the 'business logic' and back-end designers don't need to think about html tags and css classes.

More on topic with my original post, the inconsistent escaping of outputs is precisely the sort of thing that abstraction can help with.
Find all posts by this user
Quote this message in a reply
02-26-2014, 05:47 PM
Post: #10
RE: Inconsistent escaping of problem characters for SQL (' & etc.)
I know icedlava has an interest in this - I would be keen to see the results and what the code might look like.
The aim is to see the logic so far as possible in one place and the display code adds context to the logic and so makes the code more readable.
The arguments are well canvased

Phil Daintree
webERP Admin
Logic Works Ltd
http://www.logicworks.co.nz
Visit this user's website Find all posts by this user
Quote this message in a reply
Post Reply 


Forum Jump:


User(s) browsing this thread: