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
Removing default sanitizing of all API params and request vars #11786
Comments
Maybe we could also try to make that step by step... we currently use |
That's a great idea! For API there is likely not a workaround possible for now. We could do something crazy and allow underscores like |
not sure if we are using request vars with an underscore anywhere else than here, but that likely might cause problems when using such a "magic" thing. So maybe introducing a new method might be easier, but not sure re naming |
Sorry I meant API parameters, not in |
@justinvelluppillai This would also be a nice to have for Matomo 5. Note: This issue might include some parts of #4231 |
@sgiehl what did you have in mind there? Changing the input sanitizing for the API? And introducing a new method as an alternative to How do we guarantee there won't be any security regression and double escaping issue there and how much work would that one be in total? |
@tsteur No, actually both. For the API I was thinking about switching the default behavior to have no sanitizing, but for the "migration" provide a static list of methods / plugins that would still use sanitizing. That way we could in the beginning add all API methods to the list and then remove one after the other, after checking each. Which we could then also do step by step in ongoing releases. |
@sgiehl how would that affect plugins? And how much work would it be for the work in Matomo 5 and how much work would it be to complete this work fully approximately? |
@tsteur I had some further thoughts on that one. My suggested migration plan would be this one:
For plugins that would mean, that with Matomo 5, we need to set the property to The mentioned changes above should be done quite quickly. Looking through all existing APIs and disabling the auto sanitizing is hard to estimate. Some APIs should be quite simple to check, while others might be harder. I would suggest to create issues for each API once this migration plan is confirmed and then start to plan in only a couple of APIs for each release. |
@sgiehl what would be the rough estimate for the entire project? We would need some estimation. And by having this, how much time can it save us over the next 5 years? |
@tsteur The first step, to introduce the flag and setting it in all our plugins (for Matomo 5), shouldn't be more than half a day. How much time that can save us is hard to say. In the past 5 years there had been a plenty of issues around problems with double encoded stuff and I actually fixed one 2 weeks ago and doing another one at the moment. |
Next step is to document this so it's clear in the future for engineers: matomo-org/developer-documentation#669 Q: Can we close this issue? (and in the future the team would refactor as needed) |
@mattab We have already introduced the possibility to change the behavior where needed. For Matomo 5 I guess this should be enough, as we can start changing it where needed. |
To prevent issues like https://github.com/piwik/plugin-CustomDimensions/issues/62
I would say the current way of sanitizing all input is rather an anti pattern and causes lots of bugs.
We need to check if we can remove this behaviour. Problem is that some functionality may not work anymore 100% and we need to make sure that all values are properly escaped when using them to not run into any security issues after removing it. This will be lots of work and need to see if we can manage this in Piwik 4.
The text was updated successfully, but these errors were encountered: