Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSONp API request (with callback) strips first and last character from result #18133

Closed
tomudding opened this issue Oct 11, 2021 · 2 comments · Fixed by #18136
Closed

JSONp API request (with callback) strips first and last character from result #18133

tomudding opened this issue Oct 11, 2021 · 2 comments · Fixed by #18136
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone

Comments

@tomudding
Copy link

Expected Behavior

I am making a JSONp request to the Overlay API with a specific callback value, as follows:

https://demo.matomo.cloud/?module=API&method=Overlay.getTranslations&idSite=1&format=JSON&jsoncallback=__jsonp1

I expect to get back the following:

__jsonp1([{"oneClick":"1 click","clicks":"%s clicks","clicksFromXLinks":"%1$s clicks from one of %2$s links","link":"Link"}])

Current Behavior

What is actually returned:

_jsonp1([{"oneClick":"1 click","clicks":"%s clicks","clicksFromXLinks":"%1$s clicks from one of %2$s links","link":"Link"}]

The first and last character are missing.

Possible Solution

This issue comes from this specific check and call to substr(). If the array is simple (i.e., one-dimensional), the first and last character are removed to create an output like {...} instead of [{...}].

// if $array is a simple associative array, remove the JSON root array that is added by renderDataTable
if (!empty($array)
&& Piwik::isAssociativeArray($array)
&& !Piwik::isMultiDimensionalArray($array)
) {
$result = substr($result, 1, strlen($result) - 2);
}
return $result;

However, this does not account for the call to renderDataTable() just before that. Which, when using JSONp with a callback will prepend callback(, and append ).

The substr() will strip the first character of the callback and the closing parentheses. This creates invalid JavaScript, which in turn cannot be used.

The easiest way to fix this would be to add an additional check to the if statement: $this->isJsonp():

 // if $array is a simple associative array, remove the JSON root array that is added by renderDataTable 
 if (!empty($array) 
+    && !this->isJsonp() 
     && Piwik::isAssociativeArray($array) 
     && !Piwik::isMultiDimensionalArray($array) 
 ) { 
     $result = substr($result, 1, strlen($result) - 2); 
 } 
  
 return $result; 

Steps to Reproduce (for Bugs)

  1. Go to https://demo.matomo.cloud/?module=API&method=Overlay.getTranslations&idSite=1&format=JSON&jsoncallback=__jsonp1 or any other API function which returns a simple array.
  2. Observe incorrect output.

Context

Your Environment

  • Matomo Version: 4.4.1
  • PHP Version: 7.4
@tomudding tomudding added the Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. label Oct 11, 2021
@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. and removed Potential Bug Something that might be a bug, but needs validation and confirmation it can be reproduced. labels Oct 11, 2021
@tsteur tsteur added this to the 4.6.0 milestone Oct 11, 2021
@tsteur
Copy link
Member

tsteur commented Oct 11, 2021

Thanks for reporting this @tomudding and providing links to reproduce and thanks for already pointing to where the problem is. This makes things a lot easier. Must have regressed in 66bcb98#diff-287d49d80349087bbbaa1186c9f825689026f878c9bad60f46a735b65011acb2R64

@tomudding
Copy link
Author

tomudding commented Oct 12, 2021

I'd like to point out one thing from my bug report which is actually wrong based on how the code currently functions and what will make properly fixing this a bit more difficult.

I said I expected:

callback([{'simple': true}])

but what I actually expected and should have expected based on the comment in the code (assuming it worked as intended) was:

callback({'simple': true})

Because without a callback I get:

{'simple': true}

This already applies to multidimensional arrays (caught in the first if), they are (as one would expect) like:

// no callback
{'simple': false, 'sub': {'key': 'value'}}
// with callback
callback({'simple': false, 'sub': {'key': 'value'}})

Hence my suggested fix of adding a check for $this->isJsonp() in the second if is actually incorrect.

The rendering of responses is not explained in the documentation, which already caused some confusion. So not stripping the [] may cause more confusion with regards to the other possible outcomes. Especially if this goes undocumented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants