Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bug in parsing of Custom Variables #3090

Closed
anonymous-piwik-user opened this Issue · 13 comments

3 participants

Anonymous Piwik user Benaka Matthieu Aubry
Anonymous Piwik user

There is a bug in parsing Custom Variables, basically I have something like this on my site:

piwikTracker.setCustomVariable(
2,
"Entry",
'Random Title - " Foobar "',
"page"
);
which will be json encoded as:

{"2":Title - " Foobar ""}

what will happen is that in Tracker/Visit.php piwik will get this from the "cvar" parameter, then sanitize and unsanitize it, after which it looks like:

{"2":Title - " Foobar ""}

this gets then passed to json_decode which fails to parse it due to the wrongly replaced double quotes.

to reproduce just use the piwikTracker.setCustomVariable call similar to above that has " in it.

Matthieu Aubry
Owner

Thanks for the report

Benaka
Collaborator

Attachment: Patch for this issue.
3090.diff.tar.gz

Benaka
Collaborator

I uploaded a patch for this issue. Its small, but modifies getRequestVar, so I think it should be reviewed. Let me know what you think.

Matthieu Aubry
Owner

Thanks for yourpatch!

  • good idea adding json type
  • QA:
I think there should also be one test to check these use cases:
+           $lookingAtProfile = 'looking at "profile page"'; 
+           $lookingAtProfile = '\'looking at "\profile page"\'';
and even maybe with final backslash
+           $lookingAtProfile = '\\looking at "\profile page"\\';
  • Should the following test really be modified? I feel uneasy modifying existing expectations. I'd prefer to see a test added but no modifications, unless there is one necessary.. which we should raise and check that it's safe
-       $this->assertEqual( Piwik_Common::getRequestVar('test', "'}{}}{}{}'", 'string'), "'}{}}{}{}'");
+       $this->assertEqual( Piwik_Common::getRequestVar('test', "'}{}}&#0 39;{}{}'", 'string'), "'}{}}&#0 39;{}{}'");

  • There is a json_decode in: /trunk/plugins/CustomVariables/CustomVariables.php which expects the custom variable value to be a JSON array. The code should be OK unchanged, but we will know for sure since it is tested via tests/integration/EcommerceOrderWithItems.test.php
Benaka
Collaborator

(In [6367]) Fixes #3090, added 'json' request parameter type & related tests.

Matthieu Aubry
Owner

(In [6374]) Refs #3090 test build

Matthieu Aubry
Owner

(In [6377]) Refs #3090 test build

Matthieu Aubry
Owner

(In [6378]) Refs #3090 Maybe the problem comes from when the PHP implementation of json_decode is used? since it passes on capedbuzz+ my laptop but not on CI...!

Matthieu Aubry
Owner

(In [6386]) Refs #3090 test using native json_decode rather than fallback as there might be a bug in the php implementation

Matthieu Aubry
Owner

(In [6388]) Refs #3090 test build

Matthieu Aubry
Owner

(In [6392]) Do we really need a stripslashes for JSON data? really surprising?? Refs #3090 - could it be a magic quote issue?

Matthieu Aubry
Owner

(In [6395]) Refs #3090 if this fix works, for sure it will be ugly ;)

Matthieu Aubry
Owner

(In [6398]) Refs #3090

  • last change bug is fixed.. it was caused by the JSON containing escaping (since magic run time is on)
  • Also fixing "More info" link
Anonymous Piwik user anonymous-piwik-user added this to the Piwik 1.8 milestone
This issue was closed.
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.