Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#3090 closed Bug (fixed)

Bug in parsing of Custom Variables

Reported by: metadude Owned by:
Priority: major Milestone: Piwik 1.8
Component: Core Keywords:
Cc: Sensitive: no

Description

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":["Entry","Random 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":["Entry","Random 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.

Attachments (1)

3090.diff.tar.gz (2.4 KB) - added by capedfuzz 23 months ago.
Patch for this issue.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 2 years ago by matt (mattab)

  • Priority changed from normal to major

Thanks for the report

comment:2 Changed 2 years ago by matt (mattab)

  • Milestone changed from 1.7.x - Piwik 1.7.x to 1.7.2 - Piwik 1.7.2

Changed 23 months ago by capedfuzz (diosmosis)

Patch for this issue.

comment:3 Changed 23 months ago by capedfuzz (diosmosis)

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.

comment:4 Changed 23 months ago by matt (mattab)

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

comment:5 Changed 23 months ago by capedfuzz (diosmosis)

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:6 Changed 23 months ago by matt (mattab)

(In [6374]) Refs #3090 test build

comment:7 Changed 23 months ago by matt (mattab)

(In [6377]) Refs #3090 test build

comment:8 Changed 23 months ago by matt (mattab)

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

comment:9 Changed 23 months ago by matt (mattab)

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

comment:10 Changed 23 months ago by matt (mattab)

(In [6388]) Refs #3090 test build

comment:11 Changed 23 months ago by matt (mattab)

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

comment:12 Changed 23 months ago by matt (mattab)

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

comment:13 Changed 23 months ago by matt (mattab)

(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
Note: See TracTickets for help on using tickets.