Proper encoding of segment values for Actions reports. #13481

diosmosis · 2018-09-25T22:32:02Z

Changes:

Encode segmentValue for Actions reports. This extra encoding is required because of how many times a segment is urldecoded:
- The first decode is on the whole segment: https://github.com/matomo-org/matomo/blob/3.x-dev/core/Segment.php#L104 . The corresponding encode was added in 3.6.0 here: https://github.com/matomo-org/matomo/blob/3.x-dev/plugins/Live/javascripts/SegmentedVisitorLog.js#L123
- the second decode is on individual conditions (ie, pageUrl==...): https://github.com/matomo-org/matomo/blob/3.x-dev/core/Segment/SegmentExpression.php#L91
- the third decode is on individual values: https://github.com/matomo-org/matomo/blob/3.x-dev/core/Segment/SegmentExpression.php#L112
This means the value needs to be triple encoded for values like plus signs to be used properly in the segment.
Change the urldecode in Actions filter to a urlencode, since SafeDecodeLabel will already run a urldecode later. This fixes the display for URLs w/, eg, '+' signs in them (eg, http://example.org/with+plus/ will display like that, instead of http://example.org/with plus/).

I am not sure if this fix will have repercussions so I haven't modified the tests. Will do so after someone else gives their opinion.

Fixes #13460

tsteur · 2018-09-25T23:11:08Z

If it fixes your issue and tests pass probably all good to go. It's almost impossible to review it since the segments are just a mess and we never know where they are urlencoded or urldecoded etc.

diosmosis · 2018-09-25T23:12:41Z

It will change a lot of the test output I think (another encoding in the segment property in API output). Think this won't be a BC break? at least not a potentially huge one.

tsteur · 2018-09-25T23:28:59Z

Think this won't be a BC break?

I really don't know. As long as we can still handle the segments urlencoded and urldecoded it should be fine.

fdellwing · 2018-09-26T10:40:18Z

Applied the patch locally and it resolves the issue for me.

core/DataTable/Filter/SafeDecodeLabel.php

tsteur · 2018-12-06T21:39:37Z

#13460 (comment)

Don't have a fix yet, but found the cause of the error. Actions are stored urlencoded in the log_action table, but the value in the segment when opening the segmented visitor log/transitions modal/etc. is not.

I'm not sure this is the case? That might have been the case quite a while ago but shouldn't be the case anymore? at least for me it was stored normally

or you mean the URL is encoded after tracking?

tsteur · 2018-12-06T21:41:00Z

plugins/Contents/tests/System/expected/test_Contents__Actions.getPageUrls_month.xml

@@ -26,6 +26,6 @@
 		<exit_rate>100%</exit_rate>
 		<avg_time_generation>0.333</avg_time_generation>
 		<url>http://www.example.org/page</url>
-		<segment>pageUrl==http%3A%2F%2Fwww.example.org%2Fpage</segment>
+		<segment>pageUrl==http%253A%252F%252Fwww.example.org%252Fpage</segment>


is this double encoded on purpose?

yes, because one decode is for the condition, and one for the value

I don't think it's correct, would expect to have here either:
pageUrl==http%253A%252F%252Fwww.example.org%252Fpage (current behavior)
or
pageUrl%3D%3Dhttp%253A%252F%252Fwww.example.org%252Fpage (current behavior URL encoded)

but pageUrl==http%253A%252F%252Fwww.example.org%252Fpage wouldn't be a correct value since Matomo would then segment based on Page URL http%3A%2F%2Fwww.example.org%2Fpage which wouldn't exist (didn't check but it's possible it currently works anyway since we try the url decoded version in

matomo/core/Segment.php

Lines 99 to 108 in 2020b12

}

// First try with url decoded value. If that fails, try with raw value.

// If that also fails, it will throw the exception

try {

$this->initializeSegment(urldecode($segmentCondition), $idSites);

} catch (Exception $e) {

$this->initializeSegment($segmentCondition, $idSites);

}

}

but still: we wouldn't want to show this double encoded URL in the API output)

It is required to have the value encoded 3 times because it is urldecoded 3 times, see the PR description for more info.

Most URLs don't need to be double encoded, but if the value includes a value that can be decoded, like a plus sign, then it will be interpreted incorrectly if not double encoded. In this case the old API output would be wrong.

core/DataTable/Filter/SafeDecodeLabel.php

tsteur · 2018-12-06T21:43:28Z

I've looked at the PR for quite a while but the PR is quite complex and not trivial to understand especially re side effects etc. Left some comments though. Do you think there would be otherwise a solution where we don't need to change anything except for maybe the segment expression?

diosmosis · 2018-12-13T03:53:10Z

I'm not sure this is the case? That might have been the case quite a while ago but shouldn't be the case anymore?

It's not the case, it looked like it was, but I think I tracked something wrong.

mattab · 2019-05-28T01:11:54Z

@diosmosis Feedback:

Would it be possible to create a failing UI tests in 3.x-dev branch (in a separate PR) which shows the bug(s) currently in Matomo
Then could you update this PR and solve conflicts and show that the bug(s) are now fixed.

Looking forward to merging this 👍

diosmosis · 2019-05-28T02:10:29Z

This change turned out to be rather complex, I doubt I'll be able to finish it soon, and it's not as high value as other tickets. Would vote to put it in the priority backlog.

… is present, otherwise the segmentValue will not be used.

…ng first would break segments w/ correcrtly encoded values.

diosmosis · 2019-07-16T23:23:23Z

@mattab / @tsteur I added a system test for checking the segment values and found a couple more segment related bugs (which are fixed).

diosmosis · 2019-07-17T20:10:34Z

Nevermind, now all the other tests fail.

tsteur · 2019-07-17T22:52:36Z

@diosmosis let me know when it's ready for a review

tsteur · 2019-10-02T20:33:25Z

works 👍 code looks good too 👍

diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 25, 2018

diosmosis added this to the 3.6.1 milestone Sep 25, 2018

diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Oct 1, 2018

diosmosis modified the milestones: 3.6.1, 3.7.0 Oct 10, 2018

weberste mentioned this pull request Oct 11, 2018

Transitions and segmented visitor log provide no data when there is a colon in the query string #13588

Closed

tsteur reviewed Dec 6, 2018

View reviewed changes

core/DataTable/Filter/SafeDecodeLabel.php Outdated Show resolved Hide resolved

tsteur reviewed Dec 6, 2018

View reviewed changes

core/DataTable/Filter/SafeDecodeLabel.php Outdated Show resolved Hide resolved

diosmosis force-pushed the 13460-segment-encoding branch from 3d48cc2 to 95313cb Compare December 12, 2018 02:46

diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Dec 12, 2018

diosmosis mentioned this pull request Dec 26, 2018

Add UI test for complex segment + fix couple bugs to get test to pass. #13909

Merged

diosmosis added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Dec 28, 2018

diosmosis modified the milestones: 3.8.0, 3.9.0 Jan 6, 2019

mattab modified the milestones: 3.9.0, 3.10.0 Mar 18, 2019

diosmosis added 2 commits July 9, 2019 17:42

Getting fix to work.

7ee5ed1

Update expected test files.

a37e21d

diosmosis force-pushed the 13460-segment-encoding branch from 29d9443 to a37e21d Compare July 10, 2019 22:07

diosmosis added 6 commits July 16, 2019 14:35

Merge branch '3.x-dev' into 13460-segment-encoding

1ab2db2

Updating more test files.

7ab0099

When flattening, do not set segment metadata if segmentValue metadata…

d8b8838

… is present, otherwise the segmentValue will not be used.

When initializing segment, try with raw segment first since urldecodi…

155cccc

…ng first would break segments w/ correcrtly encoded values.

Correctly encode page title segment values.

a5ac1f0

Add system test for segment issues.

ee4ce92

diosmosis added 3 commits July 17, 2019 13:47

Better way to pick best segment expression parsing.

5e421a1

Merge branch '3.x-dev' into 13460-segment-encoding

a9315b1

update test files

558d232

Merge branch '3.x-dev' into 13460-segment-encoding

ea01f08

tsteur mentioned this pull request Jul 22, 2019

Export from Visits Over Time widget return empty when using Action URL in Segment #14679

Closed

mattab modified the milestones: 3.11.0, 3.12.0 Jul 23, 2019

mattab added the Major Indicates the severity or impact or benefit of an issue is much higher than normal but not critical. label Aug 1, 2019

diosmosis added 4 commits September 30, 2019 20:26

Merge branch '3.x-dev' into 13460-segment-encoding

7e77bb1

Fixing tests and encoding after merge.

a8ce781

update tests

ada1a6e

Fix integration test.

c55b797

diosmosis added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Oct 2, 2019

Update screenshots

e6e176e

tsteur merged commit 9e34ecd into 3.x-dev Oct 2, 2019

tsteur deleted the 13460-segment-encoding branch October 2, 2019 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper encoding of segment values for Actions reports. #13481

Proper encoding of segment values for Actions reports. #13481

diosmosis commented Sep 25, 2018

tsteur commented Sep 25, 2018

diosmosis commented Sep 25, 2018

tsteur commented Sep 25, 2018

fdellwing commented Sep 26, 2018

tsteur commented Dec 6, 2018

tsteur Dec 6, 2018

diosmosis Dec 13, 2018

mattab Dec 31, 2018

diosmosis Dec 31, 2018 •

edited

tsteur commented Dec 6, 2018

diosmosis commented Dec 13, 2018

mattab commented May 28, 2019

diosmosis commented May 28, 2019

diosmosis commented Jul 16, 2019

diosmosis commented Jul 17, 2019

tsteur commented Jul 17, 2019

tsteur commented Oct 2, 2019

	}

	// First try with url decoded value. If that fails, try with raw value.
	// If that also fails, it will throw the exception
	try {
	$this->initializeSegment(urldecode($segmentCondition), $idSites);
	} catch (Exception $e) {
	$this->initializeSegment($segmentCondition, $idSites);
	}
	}

Proper encoding of segment values for Actions reports. #13481

Proper encoding of segment values for Actions reports. #13481

Conversation

diosmosis commented Sep 25, 2018

tsteur commented Sep 25, 2018

diosmosis commented Sep 25, 2018

tsteur commented Sep 25, 2018

fdellwing commented Sep 26, 2018

tsteur commented Dec 6, 2018

tsteur Dec 6, 2018

Choose a reason for hiding this comment

diosmosis Dec 13, 2018

Choose a reason for hiding this comment

mattab Dec 31, 2018

Choose a reason for hiding this comment

diosmosis Dec 31, 2018 • edited

Choose a reason for hiding this comment

tsteur commented Dec 6, 2018

diosmosis commented Dec 13, 2018

mattab commented May 28, 2019

diosmosis commented May 28, 2019

diosmosis commented Jul 16, 2019

diosmosis commented Jul 17, 2019

tsteur commented Jul 17, 2019

tsteur commented Oct 2, 2019

diosmosis Dec 31, 2018 •

edited