Skip to content

Loading…

Safer way of using CJavaScript::encode() #551

Closed
qiangxue opened this Issue · 4 comments

3 participants

@qiangxue
Yii Software LLC member

A potential XSS issue is reported in Prado: http://code.google.com/p/prado3/issues/detail?id=391, which shares similar implementation of CJavaScript::encode(). Fortunately, Yii core code is not subject to this XSS issue. But still, it would be better if we could make CJavaScript::encode() safer. The idea is to use a PHP object to represent (mark) a js function, something similar to CDbExpression. We should still keep the old "js:" support for BC purpose, but we should give out explicit warning about potential XSS issue.

@samdark
Yii Software LLC member

Is it enough just to mention that it should not be used to encode data that came from unsafe source?

@samdark
Yii Software LLC member

I think it's not. The problem with keeping BC here is that people will use it same way as before. Looks like the only safe solution is to break BC here.

@samdark samdark was assigned
@samdark samdark added a commit to samdark/yii that referenced this issue
@samdark samdark Chg #551: JavaScript code that does not need escaping should be decla…
…red explicitly using CJavaScriptFunction instead of using "js:". "js:" approach will not work anymore
3d5993f
@samdark samdark added a commit to samdark/yii that referenced this issue
@samdark samdark Enh #551: Added $safe parameter to CJavaScript::encode. If set to tru…
…e, 'js:' will not be allowed
c1ac363
@samdark samdark closed this
@tom--

@samdark you said "Is it enough just to mention that it should not be used to encode data that came from unsafe source? I think it's not. The problem with keeping BC here is that people will use it same way as before. Looks like the only safe solution is to break BC here." and I agree.

But I think it is almost as bad if the $safe parameter to CJavaScript::encode has a default value of false. The only way to alert people to the problem is to break their apps.

@samdark
Yii Software LLC member

@tom-- correct but the thing is that no core widgets are using CJavaScript::encode and its usage in applications is relatively rare. Breaking BC in this case is too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.