Ticket #6913 (closed Bug: fixed)

Opened 3 years ago

Last modified 3 years ago

Bad escape sequence \b

Reported by: aercolino Owned by: wwalc
Priority: Normal Milestone: CKEditor 3.5.3
Component: Server : PHP Version: 3.1
Keywords: Cc:

Description

https://dev.ckeditor.com/browser/CKEditor/trunk/ckeditor_php5.php#L559 https://dev.ckeditor.com/browser/CKEditor/trunk/ckeditor_php4.php#L569

What's the \b escape sequence in above lines? Zend Studio complains with a warning, and I'm quite sure it's right.

I'd suppress the substitution, what do you think?

Attachments

6913.patch (1.8 KB) - added by wwalc 3 years ago.
6913_2.patch (1.4 KB) - added by wwalc 3 years ago.
6193_3.patch (5.7 KB) - added by wwalc 3 years ago.

Change History

comment:1 Changed 3 years ago by aercolino

PHP doc: http://php.net/manual/en/language.types.string.php#language.types.string.syntax.double

JavaScript doc: https://developer.mozilla.org/en/JavaScript/Guide/Values,_Variables,_and_Literals#Using_Special_Characters_in_Strings

If the intended use was to provide escaping for \b in a JavaScript string, in PHP that \b should be coded '\b' or "
b", not "\b".

In that case, all the source substitutions in ckeditor_php5.php#L559 and ckeditor_php4.php#L569 should be coded the same as \b, otherwise the program is syntactically correct but semantically wrong. In fact, for example, it's purely coincidental that in PHP and JavaScript a new line is represented by the same \n expression.

So,

$jsonReplaces = array(array("\\", "/", "\n", "\t", "\r", "\b", "\f", '"'), array('\\\\', '\\/', '\\n', '\\t', '\\r', '\\b', '\\f', '\"'));

should become

$jsonReplaces = array(array('\\', '/', '\n', '\t', '\r', '\b', '\f', '"'), array('\\\\', '\\/', '\\n', '\\t', '\\r', '\\b', '\\f', '\"'));

I don't know much about ckeditor and its internals, nor about how this jsEncode is used, but it seems a bit buggy. (what is the static keyword for? is the forward slash to be escaped? ...)

comment:2 Changed 3 years ago by Saare

  • Keywords HasPatch added
  • Status changed from new to confirmed
  • Version changed from 3.5.1 (SVN - trunk) to 3.1
  • Component changed from General to Server : PHP

comment:3 Changed 3 years ago by wwalc

  • Milestone set to CKEditor 3.5.2

The static keyword was fine, it was there just to speed up things as static variables are initialized only once. The backspace key was indeed handled in a wrong way.

Changed 3 years ago by wwalc

comment:4 Changed 3 years ago by wwalc

  • Owner set to wwalc
  • Status changed from confirmed to review

comment:5 Changed 3 years ago by aercolino

The conversion of a string S in PHP to a string T in JSON is more like this:

  1. X = convert S to UTF-8 (if needed)
  2. Y = escape with a backslash any double quote, backslash or control character in X
  3. Z = replace any occurrence in Y of the regex
    @</([A-Za-z]+)@
    
    with
    <\/\1
    
  4. T = wrap Z into double quotes

About the solidus issue see http://noteslog.com/post/the-solidus-issue/

About the static, I see that the patch tries to make it useful, but it looks like a joke.

Changed 3 years ago by wwalc

comment:6 Changed 3 years ago by wwalc

The first patch was wrong as we can simply use there a hex value for the backspace character to avoid the function call (I have no idea why I've missed it).

@aercolino:

1) this should be done by the developer as we have no idea which encoding is used
2-4) Regular expressions are slow, it is much faster to escape each occurrence of /, just like it was suggested by the RFC4627 you mentioned. Note that because the string is outputed with double quotes it makes no difference if at the end there is http://example.com or http:\/\/www.example.com

I left the static keyword, it makes no sense to remove it because it speeds up that part of code.

comment:7 Changed 3 years ago by aercolino

Look at this conversion example of a JavaScript regular expression in S:

S = /\/\w+/ 
T = \/\\\/\\w+\/
T = /\\/\\w+/

I think that it makes quite a difference to have slashes escaped or not.

About the static, I know that it can be difficult to change our mind, but believe me, it looks like junior code. There is no reason for using a static there.

And the RFC says that every control character must be escaped. Why do you only treat the special cases there?

comment:8 Changed 3 years ago by wwalc

  • Status changed from review to assigned

If you need to pass a regular expression or any other JavaScript code, use "@@" character to mark such code and it will remain untouched:

$CKEditor->config['foo'] = "@@/\/\w+/";
$CKEditor->config['bar'] = "@@2+2";

Please provide some real sample where escaping slashes is causing a problem with creating a CKEditor instance and/or the configuration option passed to CKEditor is misunderstood by the editor.

Regarding static - there are as many ways of creating the same code as developers out there, so let's just focus on the functionality.

Good point with control characters.

Changed 3 years ago by wwalc

comment:9 Changed 3 years ago by wwalc

  • Status changed from assigned to review
  • Keywords HasPatch removed

comment:10 Changed 3 years ago by mkesicki

  • Status changed from review to review_passed

comment:11 Changed 3 years ago by wwalc

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [6593].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy