Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

default to true in JSON.decode to avoid eval() #2562

Merged
merged 2 commits into from Mar 17, 2014

Conversation

Projects
None yet
3 participants
Member

SergioCrisostomo commented Feb 21, 2014

Make the secure parameter default to true, to avoid using eval() as
default.

I supose this is better than having eval() as the default parser

This was commented both on Stackoverflow and mootools.net/comments
http://mootools.net/docs/core/Utilities/JSON#comment-1157192420

@ibolmo ibolmo added this to the 1.5 milestone Feb 21, 2014

@ibolmo ibolmo added the api label Feb 21, 2014

Owner

ibolmo commented Feb 21, 2014

This would break BC. I suppose the sensible option is to put secure on, and allow them to turn it off when they trust the source. You'll need to supply, though, an entry in the 1.4compat layer, and spec coverage that checks:

  • secure is the default option
  • secure is working (e.g. JSON.decode('alert(...)') returns null)
  • secure and setting it to false would still decode

@ibolmo ibolmo and 1 other commented on an outdated diff Feb 21, 2014

Source/Utilities/JSON.js
@@ -69,7 +69,7 @@ JSON.encode = JSON.stringify ? function(obj){
JSON.decode = function(string, secure){
if (!string || typeOf(string) != 'string') return null;
-
+ secure = secure == undefined ? true : secure;
@ibolmo

ibolmo Feb 21, 2014

Owner

if (secure == undefined) secure = true;

@timwienk

timwienk Feb 21, 2014

Owner

Actually:

if (secure == null) secure = true;

@ibolmo ibolmo commented on an outdated diff Feb 21, 2014

Source/Utilities/JSON.js
@@ -69,7 +69,7 @@ JSON.encode = JSON.stringify ? function(obj){
JSON.decode = function(string, secure){
if (!string || typeOf(string) != 'string') return null;
-
+ secure = secure == undefined ? true : secure;
if (secure || JSON.secure){
@ibolmo

ibolmo Feb 21, 2014

Owner

Actually you need to account for JSON.secure's value.

See the following:

secure JSON.secure expected result
null null secure
null false not secured
null true secure
false null not secured
false false not secured
false true not secured
true null secured
true false secured
true true secured

I think then we need to add a JSON.secure = true; some where and add if (secure == undefined) secure = JSON.secure;

Member

SergioCrisostomo commented Feb 21, 2014

Very nice input! Thanks, will fix.

@ibolmo: about JSON.secure, you are right.
Btw, where/why is it used? Do not find it anywhere else in Mootools?

Will make some specs for this also following your suggestions.
Should I add it in the 1.5base or 1.5client?

Owner

ibolmo commented Feb 21, 2014

1.5base, and I don't know but just looking at the code we default for JSON.secure

Owner

ibolmo commented Feb 21, 2014

Right I meant the following:

secure JSON.secure expected result
null -- secure
null false not secured
null true secure
false -- not secured
false false not secured
false true not secured
true -- secured
true false secured
true true secured

But since we'll add a JSON.secure = true;

secure JSON.secure expected result
null true secure
null false not secured
null true secure
false true not secured
false false not secured
false true not secured
true true secured
true false secured
true true secured
Member

SergioCrisostomo commented Feb 21, 2014

@ibolmo the JSON.secure is a bit weird. It was added in the 2.0 branch c04d545 and I find no other reference to it in all Core & More files.
It is not documented in the docs either afaik.

So if we add JSON.secure = true; it has to be added to the docs and developers wanting to use "the eval way" would have to make a double false. On the function parameters and a JSON.secure = false;

I'm ok with adding it. Also ok with leaving it out (i.e not adding JSON.secure = true;).

Owner

ibolmo commented Feb 22, 2014

If you do if (secure == null) secure = JSON.secure; would be enough to
meet all requirements.

On Fri, Feb 21, 2014 at 4:18 PM, Sergio Crisostomo <notifications@github.com

wrote:

@ibolmo https://github.com/ibolmo the JSON.secure is a bit weird. It
was added in c04d545c04d545 I find no other reference to it in all Core & More files.
It is not documented in the docs either afaik.

So if we add JSON.secure = true; it has to be added to the docs and
developers wanting to use "the eval way" would have to make a double true.
On the function parameters and a JSON.secure = false;

I'm ok with both adding it and leaving it out (i.e not adding JSON.secure
= true;).


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2562#issuecomment-35779794
.

Member

SergioCrisostomo commented Feb 22, 2014

@ibolmo , thanks for your time checking this!
I'm still not sure I follow you. I see 3 scenario:

1- Adding both JSON.secure = true; and if (secure == null) secure = JSON.secure;
This means default behavior is secure. And to make it un-secure user has to use JSON.secure = false; which is not documented.
If user uses JSON.secode(string, false); as documented, that alone would turn to true anyway because of JSON.secure = true;.

2- Adding just JSON.secure = true; (and keeping if (secure == null) secure = true;)
This means default behavior is secure. But to override it and go un-secure user needs to use double false:

JSON.secode(string, false);
JSON.secure = false;

3- Using as it is now, in the PR. ( and the right one in my logic)
This means default behavior is secure. To override user would use, as documented already, just JSON.secode(string, false);


JSON.secure seems to be kind of a backdoor/alternative, not documented.

Owner

ibolmo commented Feb 22, 2014

  1. You would need to edit if (secure || JSON.secure){ to: if (secure){
    and you're done.

Even if it's undocumented, we've had people use undocumented interfaces and
when we changed them they came back to us upset. It's small work to make
sure that folks get consistency between minor versions.

On Sat, Feb 22, 2014 at 1:19 AM, Sergio Crisostomo <notifications@github.com

wrote:

@ibolmo https://github.com/ibolmo , thanks for your time checking this!
I'm still not sure I follow you. I see 3 scenario:

1- Adding both JSON.secure = true; and if (secure == null) secure =
JSON.secure;
This means default behavior is secure. And to make it un-secure user has
to use JSON.secure = false; which is not documented.
If user uses JSON.secode(string, false); as documented, that alone would
turn to true anyway because of JSON.secure = true;.

2- Adding just JSON.secure = true; (and keeping if (secure == null)
secure = true;)
This means default behavior is secure. But to override it and go un-secure
user needs to use double false:

JSON.secode(string, false);
JSON.secure = false;

3- Using as it is now, in the PR. ( and the right one in my logic)
This means default behavior is secure. To override user would use, as

documented already, just JSON.secode(string, false);

JSON.secure seems to be kind of a backdoor/alternative, not documented.


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2562#issuecomment-35796783
.

Member

SergioCrisostomo commented Feb 22, 2014

@ibolmo I think I figured it out now with the compat layer.

I added a compat layer, and with that I also removed future reference to JSON.secure without the compat.

Let me know if it looks ok.

@ibolmo ibolmo and 1 other commented on an outdated diff Feb 22, 2014

Source/Utilities/JSON.js
@@ -69,8 +69,13 @@ JSON.encode = JSON.stringify ? function(obj){
JSON.decode = function(string, secure){
if (!string || typeOf(string) != 'string') return null;
+
+ //<1.4compat>
+ if (typeof JSON.secure != "undefined" && !secure) secure = JSON.secure;
@ibolmo

ibolmo Feb 22, 2014

Owner

if (secure == null) secure = JSON.secure;

You need to define JSON.secure = true; outside JSON.decode

@ibolmo

ibolmo Feb 22, 2014

Owner

and document JSON.secure as a global option.

@SergioCrisostomo

SergioCrisostomo Feb 22, 2014

Member

@ibolmo I like the idea to have a documented global JSON.secure, didn't think of that option actually.

We could just keep the code as current PR and document the possibility to use a global JSON.secure.
Your last suggestion would make JSON.secure be ignored if secure is false, because if (secure == null) will turn false and we have a new L78 just looking for secure.

As it is it defaults to secure. If we add extra JSON.secure = true; as global, then the old code that followed the docs using secure = false will be forced to secure.

If we keep this PR, there is no break of BC in the code that active defined secure or JSON.secure.

@ibolmo ibolmo commented on an outdated diff Feb 22, 2014

Source/Utilities/JSON.js
- if (secure || JSON.secure){
+ if (secure == null) secure = true;
@ibolmo

ibolmo Feb 22, 2014

Owner

This would be if (secure == null) secure = JSON.secure;

I don't understand what you mean. Look at the table I proposed. You would add JSON.secure = false in the 1.4compat block.

I like to think of the right API and make the code work around it. You need to look at what it did before the changes, where the API should be, and make the changes considering those factors.

Member

SergioCrisostomo commented Feb 22, 2014

@ibolmo I was misunderstanding the compat layer's behaviour. I updated the PR code and docs now.

@ibolmo ibolmo commented on an outdated diff Feb 22, 2014

Specs/1.5base/Utilities/JSON.js
+description: JSON encoder and decoder.
+
+license: MIT-style license.
+
+SeeAlso: <http://www.json.org/>
+
+requires: [Array, String, Number, Function]
+
+provides: JSON
+
+...
+*/
+describe('JSON', function(){
+
+ var goodString = '{"name":"Jim Cowart","location":{"city":{"name":"Chattanooga","population":167674}}}';
+ var badString = 'alert("I\'m a bad string!")';
@ibolmo

ibolmo Feb 22, 2014

Owner

Could you check the spacing. Looks like you have spaces at the beginning of the lines.

Also can you add a spec for JSON.secure global option.

Member

SergioCrisostomo commented Feb 25, 2014

This PR has to be refactored after Olmo's new Specs PR is merged.

Owner

ibolmo commented Feb 25, 2014

Do a git rebase -i ibolmo/master

On Tue, Feb 25, 2014 at 1:42 AM, Sergio Crisostomo <notifications@github.com

wrote:

This PR has to be refactored after Olmo's new Specs PR is merged.


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2562#issuecomment-35982505
.

Member

SergioCrisostomo commented Feb 25, 2014

@ibolmo will do that.
I supose I don't need the package.yml and Configuration.JS files anymore?

I see also a new directory structure without /1.5base/ so I suppose the mootools-packager script goes forEach directory inside the Specs and the compat layer info goes inside the respective specs .js file, with compat comment tags when needed.

I run the specs adding my new JSON.js specs to the one on JSON.js specs file from your repo and they worked good with grunt.

Owner

ibolmo commented Feb 25, 2014

Yeah you have the right idea.

On Tue, Feb 25, 2014 at 8:59 AM, Sergio Crisostomo <notifications@github.com

wrote:

@ibolmo https://github.com/ibolmo will do that.
Do I still need the package.yml and Configuration.JS files?

I see also a new directory structure without /1.5base/ so I suppose the
mootools-packager script goes forEach directory inside the Specs and the compat
layerhttps://github.com/ibolmo/mootools-core/blob/master/Specs/Element/Element.js#L239-L243info goes inside under the respective specs
.js file, with compat comment tags when needed.

I run the specs adding my new JSON.js specs to the one on JSON.js specs
file from your repo and they worked good with grunt.


Reply to this email directly or view it on GitHubhttps://github.com/mootools/mootools-core/pull/2562#issuecomment-36015625
.

@ibolmo ibolmo modified the milestones: 1.6, 1.5 Mar 3, 2014

Member

SergioCrisostomo commented Mar 6, 2014

@ibolmo updated the PR with new master.

Member

SergioCrisostomo commented Mar 16, 2014

@ibolmo could you merge or give your 👍 on this? would be nice to get this into 1.5 also to avoid eval unnecessarily.

@ibolmo ibolmo commented on an outdated diff Mar 16, 2014

Docs/Utilities/JSON.md
@@ -33,7 +33,9 @@ Converts a JSON string into a JavaScript object.
### Arguments:
1. string - (*string*) The string to evaluate.
-2. secure - (*boolean*, optional: defaults to false) If set to true, checks for any hazardous syntax and returns null if any found.
+2. secure - (*boolean*, optional) If set to true, checks for any hazardous syntax and returns null if any found.
@ibolmo

ibolmo Mar 16, 2014

Owner

(*boolean*, optional: defaults to true)

Owner

ibolmo commented Mar 16, 2014

One little gotcha, but yep it's 👍

ibolmo added a commit that referenced this pull request Mar 17, 2014

Merge pull request #2562 from SergioCrisostomo/JSONdecode
default to true in JSON.decode to avoid eval()

@ibolmo ibolmo merged commit 2769c9d into mootools:master Mar 17, 2014

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment