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

Bug in parsing of Custom Variables #3090

Closed
anonymous-matomo-user opened this issue Apr 5, 2012 · 13 comments
Closed

Bug in parsing of Custom Variables #3090

anonymous-matomo-user opened this issue Apr 5, 2012 · 13 comments
Labels
Bug For errors / faults / flaws / inconsistencies etc. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Milestone

Comments

@anonymous-matomo-user
Copy link

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.

@mattab
Copy link
Member

mattab commented Apr 6, 2012

Thanks for the report

@diosmosis
Copy link
Member

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

@diosmosis
Copy link
Member

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.

@mattab
Copy link
Member

mattab commented May 28, 2012

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

@diosmosis
Copy link
Member

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

@mattab
Copy link
Member

mattab commented May 29, 2012

(In [6374]) Refs #3090 test build

@mattab
Copy link
Member

mattab commented May 29, 2012

(In [6377]) Refs #3090 test build

@mattab
Copy link
Member

mattab commented May 29, 2012

(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...!

@mattab
Copy link
Member

mattab commented May 30, 2012

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

@mattab
Copy link
Member

mattab commented May 30, 2012

(In [6388]) Refs #3090 test build

@mattab
Copy link
Member

mattab commented May 30, 2012

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

@mattab
Copy link
Member

mattab commented May 30, 2012

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

@mattab
Copy link
Member

mattab commented May 30, 2012

(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-matomo-user anonymous-matomo-user added this to the Piwik 1.8 milestone Jul 8, 2014
This issue was closed.
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. Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical.
Projects
None yet
Development

No branches or pull requests

3 participants