Opened 7 years ago

Closed 7 years ago

#6913 closed Bug (fixed)

Bad escape sequence \b

Reported by: Andrea Ercolino Owned by: Wiktor Walc
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 (3)

6913.patch (1.8 KB) - added by Wiktor Walc 7 years ago.
6913_2.patch (1.4 KB) - added by Wiktor Walc 7 years ago.
6193_3.patch (5.7 KB) - added by Wiktor Walc 7 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by Andrea Ercolino

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 7 years ago by Sa'ar Zac Elias

Component: GeneralServer : PHP
Keywords: HasPatch added
Status: newconfirmed
Version: 3.5.1 (SVN - trunk)3.1

comment:3 Changed 7 years ago by Wiktor Walc

Milestone: 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 7 years ago by Wiktor Walc

Attachment: 6913.patch added

comment:4 Changed 7 years ago by Wiktor Walc

Owner: set to Wiktor Walc
Status: confirmedreview

comment:5 Changed 7 years ago by Andrea Ercolino

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 7 years ago by Wiktor Walc

Attachment: 6913_2.patch added

comment:6 Changed 7 years ago by Wiktor Walc

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 7 years ago by Andrea Ercolino

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 7 years ago by Wiktor Walc

Status: reviewassigned

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 7 years ago by Wiktor Walc

Attachment: 6193_3.patch added

comment:9 Changed 7 years ago by Wiktor Walc

Keywords: HasPatch removed
Status: assignedreview

comment:10 Changed 7 years ago by Michał

Status: reviewreview_passed

comment:11 Changed 7 years ago by Wiktor Walc

Resolution: fixed
Status: review_passedclosed

Fixed with [6593].

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