check_html save no style width

Site Admin Joined: Aug 22, 2007 Posts: 1775

hey!

I have tested some html tags with the CK editor 3-6-3 and got a crazy issue with function check_html.

rnconfig AllowableHTML:

Code:

   'object' => array('style' => 1, 'id' => 1, 'data' => 1, 'width' => 1, 'height' => 1, 'id' => 1, 'type' => 1),   


   'param' => array('name' => 1, 'value' => 1),

my html code:

Code:

<object data="http://www.youtube.com/v/6Ws2WQKH9SE?rel=0&amp;loop=1" style="width: 500px; height: 312px;" type="application/x-shockwave-flash"><param name="wmode" value="Opaque" /><param name="movie" value="http://www.youtube.com/v/6Ws2WQKH9SE?rel=0&amp;loop=1" /></object>

result after save:

Code:

<object data="http://www.youtube.com/v/6Ws2WQKH9SE?rel=0&amp;loop=1" style="500px; height: 312px;" type="application/x-shockwave-flash"><param name="wmode" value="Opaque" /><param name="movie" value="http://www.youtube.com/v/6Ws2WQKH9SE?rel=0&amp;loop=1" /></object>

same thing with a div:

Code:

<div align="center" style="width: 500px; height: 312px;">test</div>

result after save:

Code:

<div align="center" style="500px; height: 312px;">test</div>

for saveing into the database i used this filtering:

Code:

$bodytext = $db->sql_escape_string(check_html($bodytext, ''));

If i remove check_html, then its all fine.

Question

Posted: Fri Jul 06, 2012 9:44 am

I can't address all the issues here but I know that as part of check_html the kses program encodes html entities. I think the term it uses is "normalizes" which has always been ambiguous and misleading to me.

So, if you have an & it will get encoded as & and single and double quotes will get encoded too. Which is not good since that's not what you want stored in the database. Why and where "width" is being removed from your other code is beyond me.

You could remove ckeditor from the mix and just try a test where you set a variable to:

Code:

<div align="center" style="width: 500px; height: 312px;">test</div>

then pass it through check_html and see what results. If the problem occurs then we know it is in check_html. Though I'm not sure what we'd do since kses is unsupported software at this time. Maybe omit the checks wherever possible.

Posted: Sat Jul 07, 2012 3:53 am

I've got the same result with a disabled ckeditor and i have tested with a fresh install of RN25 with the same result. Database is utf8_gerneral_ci - not converted.

Posted: Sat Jul 07, 2012 9:35 am

I see what you are saying... I guess I would suggest using a class to set the width/height in the meantime...

It seems to be not necessarily related to the width attribute, but to the first style declared

<div style="color:black;width:300px;height:400px">abc</div>
becomes
<div style="black;width:300px;height:400px">abc</div>

I have a feeling the issue is occurring in function kses_bad_protocol and/or kses_bad_protocol_once - but that's just a guess at this point.

Posted: Sat Jul 07, 2012 10:13 am

crazy - thanks for testing, too!

registered · Posted: Sat Jul 07, 2012 10:26 am

I just tested it with the latest RN development version using HTMLPurifier and CKeditor. This doesn't seem to be an issue with it. You may want to test it too and make sure....

Posted: Sat Jul 07, 2012 11:48 am

In version RN24 there was no such problem. Is there no way to fix this issue in RN25?

registered · Posted: Sun Jul 08, 2012 9:09 am

I do see that there was quite a bit of change made in the check_html() function between these two versions. I am not sure why the changes were made as I was on sabbatical for that release. You might want to try to replace the check_html() function code in 2.5 with what was in 2.4 and see if it works better for you.

Posted: Sun Jul 08, 2012 1:09 pm

Ok i have checked the news admin file in RN 2.40.01 and there was not used the function check_html to save the bodytext. It was used FixQuotes. But this is outdated. The issue exist only in RN25.

spasticdonkey wrote:

It seems to be not necessarily related to the width attribute, but to the first style declared

i have tested a wrong typing and here is it the same result.

entered:

Code:

<div align="width:20px">test</div>

stored:

Code:

<div align="20px">test</div>

The kses.php is exactly the same file and the old check_html function is not the solution.

Posted: Sun Jul 08, 2012 2:29 pm

I have found an modded kses.php. Near line 314 is an comment with extra lines. I have it tested and it works on my local RN25 but it would be great if someone check if it is clean.

http://www.phpkode.com/source/p/moodle/moodle/lib/kses.php

Code:

          // MDL-2684 - kses stripping CSS styles that it thinks look like protocols


          if ($attrname == 'style') {


              $thisval = $match[1];


          } else {


              $thisval = kses_bad_protocol($match[1], $allowed_protocols);


          }

Posted: Tue Jul 10, 2012 7:23 am

If I am reading that code right, it sure looks like it will now allow ANY style directives which I think is a security hole.

neralex, you say that "the old check_html function is not the solution". Have you tried swapping out that function's code from 2.4 into 2.5 and made NO OTHER CHANGES? If so, are you certain that it really did work in 2.4 previously?

You are right, FixQuotes is outdated (worthless).

Posted: Tue Jul 10, 2012 8:11 am

montego wrote:

Have you tried swapping out that function's code from 2.4 into 2.5 and made NO OTHER CHANGES?

Yes, i have made no changes on the function. It was an copy&paste. Its the mainfile of Palbin's ckeditor. But i think the issue comes not from the function. Try it self, please. I have tried with fresh install, too. The issue makes me sad, because i have many styles settings in my articles and this bug breaks all after a saving in RN25. It would be nice if the matter could take someone.

Posted: Sat Jul 14, 2012 10:09 am

[quote="neralex"]

montego wrote:

Its the mainfile of Palbin's ckeditor.

Well, then that is NOT the code from 2.4's mainfile.php check_html() function as I had requested you to try.

This may have to take Palbin looking at it given you are working with his CKeditor version.

Posted: Sun Jul 15, 2012 10:57 am

Looking at this today.

Posted: Sun Jul 15, 2012 11:53 am

It did work in RN 2.4, but not really. In RN 2.4 we were not using check_html() in news admin. Looking for/at the root cause now.

Posted: Sun Jul 15, 2012 3:00 pm

Palbin wrote:

It did work in RN 2.4, but not really. In RN 2.4 we were not using check_html() in news admin. Looking for/at the root cause now.

Yes it is right! I think we need for RN25 an fix for kses.

Posted: Sun Jul 15, 2012 4:40 pm

Ok I think I have this fixed.

Use this kses.php
http://www.phpnuke-guild.org/highlight_code/kses.phps

This is not required, but you should also edit line 1006 of mainfile.php(line number will vary between versions)

Code:




function check_html ($string, $allowed_html = '', $allowed_protocols = array()) {

The following protocols and style attributes should now be allowed. If you enabled styles for a particular tag all the attributes will be available for that tag. This is not configurable on a per tag basis.

Code:




$allowed_protocols = array('http', 'https', 'ftp', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn');

Code:




   $allowed_attr = array( 'text-align', 'margin', 'color', 'float',


   'border', 'background', 'background-color', 'border-bottom', 'border-bottom-color',


   'border-bottom-style', 'border-bottom-width', 'border-collapse', 'border-color', 'border-left',


   'border-left-color', 'border-left-style', 'border-left-width', 'border-right', 'border-right-color',


   'border-right-style', 'border-right-width', 'border-spacing', 'border-style', 'border-top',


   'border-top-color', 'border-top-style', 'border-top-width', 'border-width', 'caption-side',


   'clear', 'cursor', 'direction', 'font', 'font-family', 'font-size', 'font-style',


   'font-variant', 'font-weight', 'height', 'letter-spacing', 'line-height', 'margin-bottom',


   'margin-left', 'margin-right', 'margin-top', 'overflow', 'padding', 'padding-bottom',


   'padding-left', 'padding-right', 'padding-top', 'text-decoration', 'text-indent', 'vertical-align',


   'width');

Posted: Sun Jul 15, 2012 4:50 pm

Did you mean line 58?

Code:

function kses($string, $allowed_html, $allowed_protocols = array()) {

Replace it with this one

Code:

function check_html ($string, $allowed_html = '', $allowed_protocols = array()) {

Thanks Palbin with taking care of this issue.

Posted: Sun Jul 15, 2012 5:02 pm

That was supposed to say line 1006 of mainfile.php. I have edited my previous post.

Posted: Mon Jul 16, 2012 10:31 am

in RavenNuke 2.5 I have this

Code:

function check_html ($string, $allowed_html = '', $allowed_protocols = array('http', 'https', 'ftp', 'news', 'nntp', 'gopher', 'mailto')) {

Where I should put these one

Quote:

$allowed_attr = array( 'text-align', 'margin', 'color', 'float',
'border', 'background', 'background-color', 'border-bottom', 'border-bottom-color',
'border-bottom-style', 'border-bottom-width', 'border-collapse', 'border-color', 'border-left',
'border-left-color', 'border-left-style', 'border-left-width', 'border-right', 'border-right-color',
'border-right-style', 'border-right-width', 'border-spacing', 'border-style', 'border-top',
'border-top-color', 'border-top-style', 'border-top-width', 'border-width', 'caption-side',
'clear', 'cursor', 'direction', 'font', 'font-family', 'font-size', 'font-style',
'font-variant', 'font-weight', 'height', 'letter-spacing', 'line-height', 'margin-bottom',
'margin-left', 'margin-right', 'margin-top', 'overflow', 'padding', 'padding-bottom',
'padding-left', 'padding-right', 'padding-top', 'text-decoration', 'text-indent', 'vertical-align',
'width');

Posted: Mon Jul 16, 2012 2:58 pm

Replace:

Code:




function check_html ($string, $allowed_html = '', $allowed_protocols = array('http', 'https', 'ftp', 'news', 'nntp', 'gopher', 'mailto')) {

with:

Code:




function check_html ($string, $allowed_html = '', $allowed_protocols = array()) {

Posted: Mon Jul 16, 2012 5:45 pm

Yes, I did it. What is the next step. Thanks for taking care of this important issue

Posted: Tue Jul 17, 2012 6:30 am

Palbin wrote:

Ok I think I have this fixed.

Palbin, let me say thank you. It works fine!

RavensScripts

Posted: Tue Jul 17, 2012 8:01 am

it works perfect! Thanks a lot! Palbin Wink

Posted: Wed Jul 18, 2012 3:16 am

Palbin, I think i have found an bug in your fixed version of the kses.php:

Fatal error: Call to undefined function wp_kses_decode_entities() in F:\wamp\www\raven25\includes\kses\kses.php on line 528

The function wp_kses_no_null is undefined, too. Do you mean kses_decode_entities and kses_no_null ?