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

Ensure unescaped commas are used for periods in url param #16784

Merged
merged 3 commits into from Jan 1, 2021

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Nov 24, 2020

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

@sgiehl sgiehl 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 Nov 24, 2020
@sgiehl sgiehl added this to the 4.1.0 milestone Nov 24, 2020
@diosmosis
Copy link
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
Copy link
Member Author

sgiehl commented Dec 11, 2020

@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
Copy link
Member

diosmosis commented Dec 11, 2020

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
Copy link
Member Author

sgiehl commented Dec 14, 2020

@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
Copy link
Member

diosmosis commented Dec 15, 2020

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.

@mattab mattab modified the milestones: 4.1.0, 4.2.0 Dec 21, 2020
… initially because broadcast does not urldecode but $.param expects urldecoded value)
@diosmosis
Copy link
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.

@diosmosis diosmosis merged commit db5e994 into 4.x-dev Jan 1, 2021
@diosmosis diosmosis deleted the multisiterange branch January 1, 2021 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

going to 'All Websites' dashboard and selecting a date range and clicking a site results in error message
3 participants