@sgiehl opened this Pull Request on November 24th 2020 Member

Description:

Might be a bit hackish solution, but Matomo simply can't handle an encoded date range in the url at various places. So might be the simplest solution to avoid having an encoded value instead of searching for all places that might make problem...

fixes #15238

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on December 11th 2020 Member

I did some digging and I think this can also be fixed by handling %2C or urldecoding in Range::parseDateRange. (The other alternative fix is to urldecode every time 'date' is accessed via Common::getRequestVar, but that seems excessive.)

@sgiehl commented on December 11th 2020 Member

@diosmosis I tried some other stuff as well, but couldn't get it working for all cases. If you have a better solution feel free to simply adjust the PR

@diosmosis commented on December 11th 2020 Member

The problem with replacing the comma is that it will only be done client side in this one place, though we of course change the URL in many places, sometimes setting location.href directly, and it could cause problems w/ segments that contain commas given the extra confusing segment encoding logic.

I just checked multisites, but what are the other cases? I can try to fix those as well.

@sgiehl commented on December 14th 2020 Member

@diosmosis When we want to "support" an escaped comma in the url parameter we need to be sure, that it works on all pages. IIrc it didn't work for various reports

@diosmosis commented on December 15th 2020 Member

Replacing the comma in every part of the URL will have side effects. %2C in a URL is the same as ,, it shouldn't need to be "supported", the real problem is that we don't urldecode query parameters server side implicitly (afaik we do in broadcast, but it's done on the whole URL so I guess might not work great in another portion). If this must be done client side, perhaps instead of replacing it in every parameter (like in the segment which could have strange side effects), we could just change it in the date parameter, which is what's causing problems for the referenced issue.

@diosmosis commented on January 1st 2021 Member

Found the root cause, it wasn't server side. When the url contains an empty hash, the hash is created using the main url values, but it double encodes them. This was due to reportingmenu.controller.js in makeUrl using the date from piwik.getSearchParam(), which returns the value without decoding, to construct a URL via $.param which expects url decoded values. This created a double encoded value which couldn't be parsed.

This Pull Request was closed on January 1st 2021
Powered by GitHub Issue Mirror