Native JSON parsing vs eval() #80

Open
MrK- opened this Issue Jul 13, 2012 · 4 comments

2 participants

@MrK-

The ExtJS library contains the following code for dealing with JSON which is obviously used in quite a few places by Dradis:

/**
 * Decodes (parses) a JSON string to an object. If the JSON is invalid, this function throws a SyntaxError unless the safe option is set.
 * @param {String} json The JSON string
 * @return {Object} The resulting object
 */
this.decode = function() {
  var dc;
  return function(json) {
    if (!dc) {
      // setup decoding function on first access
      dc = isNative() ? JSON.parse : doDecode;
    }
    return dc(json);
  };
}();

The isNative() function is defined as follows:

isNative = function() {
  var useNative = null;

  return function() {
    if (useNative === null) {
      useNative = Ext.USE_NATIVE_JSON && window.JSON && JSON.toString() == '[object JSON]';
    }

    return useNative;
  };
}(),

Therefore an application must define Ext.USE_NATIVE_JSON before including the ExtJS library if the browser's native JSON parser is going to get used. I didn't spot anywhere that Dradis does that, which may or may not be an issue.

In any case there are situations where the native method isn't used, and ExtJS falls back to eval(). This strikes me as being on the unsafe side, especially as jQuery handles this situation much more gracefully (trying hard to avoid calling eval() on non-JSON):

parseJSON: function( data ) {
  if ( typeof data !== "string" || !data ) {
    return null;
  }

  // Make sure leading/trailing whitespace is removed (IE can't handle it)
  data = jQuery.trim( data );

  // Attempt to parse using the native JSON parser first
  if ( window.JSON && window.JSON.parse ) {
    return window.JSON.parse( data );
  }

  // Make sure the incoming data is actual JSON
  // Logic borrowed from http://json.org/json2.js
  if ( rvalidchars.test( data.replace( rvalidescape, "@" )
    .replace( rvalidtokens, "]" )
    .replace( rvalidbraces, "")) ) {

    return ( new Function( "return " + data ) )();

  }
  jQuery.error( "Invalid JSON: " + data );
},

I've had success with splicing the jQuery version into ExtJS for peace-of-mind, with something like this (reliant on the fact that Dradis includes jQuery into application.js before it includes ExtJS):

doDecode=function(json){ 
  if (json=='[]') return eval("([])"); 
  return doRealDecode(json);
},
doRealDecode=function(json){ 
  return json?jQuery.parseJSON(json):""
}

Doesn't appear to be a huge issue in the grand scheme of things but it is hard to predict what might get passed through this function...

@etdsoft
Dradis Framework member

@MrK- does it make sense to use the Ext.USE_NATIVE_JSON flag instead? I'm weary about making changes to the ExtJS lib..

@MrK-

The jQuery approach is safer because it will try the native JSON parser and then fall back to eval()-style parsing after performing a few sanity checks. ExtJS doesn't have these checks in place, so I suppose it depends if you class this as a performance issue or the beginnings of a potential security issue (like I said, it is hard to predict what might get eval()'d...)

@etdsoft
Dradis Framework member

Makes sense.

Why split it in two though? Would something like this do the trick?

doDecode=function(json){ 
  if (json=='[]') return [];
  return json?jQuery.parseJSON(json):""
}

(I didn't know about the first eval() seems like we could remove it? and why the first if?)

@MrK-

You're right - it ended up that way whilst debugging but the important part is simply the jQuery.parseJSON() bit.
The 'if' got added because it seemed to be a common case and therefore we could shortcut the JSON parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment