Handle multiple JSON Vulnerability Prefixes in same response

[riaann]

An error ("Unable to parse JSON string") occurs when the same batch response contains multiple JSON vulnerability prefixes (such as when multiple GET operations that return arrays are batched). A unit test was added to illustrate this scenario.

This occurred only with multiple prefixes because string.replace was used on the entire response text, and string.replace only replaces the first occurrence of the substring.

To fix this, I moved the trim function to the "httpAdapter", because that allowed finer control over when the prefix was trimmed - the prefix is now trimmed for each batch response separately and only when parsing a response with a 'json' content type. Further, trimming JSON vulnerability prefixes don't appear relevant for multifetch: the returned data is in an object form, not an array - so the vulnerability does not apply (see this for more details of how the vulnerability manifests: http://haacked.com/archive/2008/11/20/anatomy-of-a-subtle-json-vulnerability.aspx/ ).

[jonsamwell]

Hi @riaann! Thanks for this. I suggest we just change the replace function to use a regex then the vulnerability will be changed globally? This way the functionality remains for all the various adapter in the library and that other people have. Something like (I haven't tested this though so not sure if it will work):

function trimJsonProtectionVulnerability(data) {
  return typeof (data) === 'string' ? data.replace(/\)]}',\n/gi, '') : data;
}

[riaann]

Hi Jon!

I did think of that, but I thought that it makes more sense to limit the replacement to just the response bodies - not all the encoded response details. Up to you, though - I can make that change if you prefer.

As for applying the transform to all adapters, I assume the idea is to keep the behavior consistent between responses received from a batched response and a non-batched response - so that the calling code isn't affected by batched calls. To be completely consistent, we should run the specified or default response transforms on each individual response extracted from a batch. The default response transform includes trimming the prefix and deserializing JSON strings.

In line with that, my preferred option - but also with the most changes - is to remove the JSON prefix stripping and deserialization from angular-http-batcher completely: angular's $http service already applies the default response transforms to the individual, parsed responses - I confirmed this with a modified build on my side that just doesn't call the "convertDataToCorrectType" function. The change to the shipped code is small, as just described, but the tests mostly assume that the objects are already deserialized, so they would need to change.

If you don't want to remove the response transforms out of the library, another option could be to move the conversion of JSON response bodies into objects into "httpBatcher", instead of the individual adapters. This is my second-most preferred approach, but also has quite a few changes: we would need to keep track of the responseTransforms for each request and apply them to each corresponding request body.

What do you think? Please let me know if I can send a new pull request with my preferred option above, if you want to see the effect of the changes.

Thanks, Riaan

On Wed, 23 Sep 2015 at 02:37 Jon Samwell notifications@github.com wrote:

Hi @riaann https://github.com/riaann! Thanks for this. I suggest we just change the replace function to use a regex then the vulnerability will be changed globally? This way the functionality remains for all the various adapter in the library and that other people have. Something like (I haven't tested this though so not sure if it will work):

function trimJsonProtectionVulnerability(data) { return typeof (data) === 'string' ? data.replace(/)]}',\n/gi, '') : data; }

— Reply to this email directly or view it on GitHub https://github.com/jonsamwell/angular-http-batcher/pull/28#issuecomment-142462587 .

[jonsamwell]

HI @riaann sorry for the delayed response!

Let's stick with your approach. Could you just remove the changes to the dist folder files and I accept the merge and do a release :-)

[riaann]

Done!

Please let me know if you need anything else.

[jonsamwell]

Sorry for the delay this is now in the latest release v1.12.0

[riaann]

Great, thanks! :+1: