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
Deprecate auto sanitize of API parameters & Common::getRequestVar and introduce Request class #19624
Conversation
aef0bd2
to
ae1be1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the code and tested the new parameter. It all seems to work as expected and I can't see any issues.
- Functional review done
- Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
- Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
- Security review done
- Wording review done
- Code review done
- Tests were added if useful/possible
- Breaking changes noted. Reviewed for breaking changes
- This has been done. Developer changelog updated if needed
- Documentation added if needed
- Existing documentation updated if needed
@bx80 thanks for the review Would create a PR for developer docs afterwards, as I guess they suggest to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgiehl Left a comment.
Can we confirm we've done a security review / plan on how to prevent possible issues coming from this change for example related to null bytes? (in PHP and in JS). Things like eg <IMG SRC=java\0script:alert(\"XSS\")>
which may bypass filters/validations etc. Also encoding wise for example are we confident there can't be any possible security regression from this? Is there anything to consider that could possibly create any security regression?
I see we mostly replaced it where we used to use Common::unsanitizeInputValue
so it may be less of an issue for these but going forward if this was used in other places or by plugin developers then any possible security issue we need to prevent in another way or mention if they need to take care of extra steps etc.
Regarding 4.12: It could maybe wait for Matomo 5 unless there was some user related issue this solves that would be worth having in 4.12? This could also give more time later changing the API if needed etc .
@tsteur I've done some testing regarding null bytes for the places where I already replaced the usage of Regarding the version to merge this to. There is no specific issue this will solve for Matomo 4.12. But I thought it might be good to have it in already, so plugin developers might already see the upcoming changes. |
As other plugins may start using these methods, it be great to have it secure from the beginning. I reckon as well that we could always remove the null byte as it shouldn't have any downside I suppose. Not sure if there could be any kind of issues re encoding? |
I'm not sure if that is the correct perspective on that. Imho any code using any type of input parameters should be responsible for handling them safely and correct. The implementation of some global security layer, like We will still have some global methods that can be used to make a string "safe" for usage in the UI. Where this is needed Personally I wouldn't change anything else here. But we for sure should update our documentation, so it's clear for a developer that using the new Request class, requires to handle those values with care. |
With that I mean to have eg the null byte removed, and having docs what people need to be aware of when using these methods. Like what potential security issues could arise? What do they need to pay attention to? People generally don't know much about security and we need to make sure to explain possible security issues that could arise (eg are there any encoding issues) etc so that they won't be running into these problems.
Generally, I would say Security is the most important for us at Matomo and the more built-in security we have, the better. You can put up with certain data issues but you can't put up with security issues. The guide could even recommend to use The question really is what possible security issues could arise from this change? And if there is anything, we need to have the security risk assessed and have it documented. |
From the changes in this PR there shouldn't be any security risk. Our docs should currently all advise to use |
I was more meaning if there's any change or risk in general regarding security by using this new class not just by the changes in this PR. I'm meaning this because the new class is mentioned in the defveloper changelog and we tell developers to use the new one. That means we'd need to analyse what could potentially go wrong if they did use the new methods which some might do and not just the changes in this PR. We wouldn't want any security issues to arise from this. If there's no change in terms of security then it'll be easy. If any kind of security issues around encoding could come up from this change then we'd need to explain this or see if we can prevent the issue in another way. We could also just remove the changelog entry for now and then it maybe wouldn't be an issue. Note also that it's marked as API in the developer changelog but there's no Should we also still maybe directly remove the null byte as that shouldn't cause any issues but improves security by default? Or what would be the disadvantage of not removing it? As we're still supporting |
@tsteur I've now updated the PR.
So the "only" security related risk that remains when using the Request class instead of Personally I think it would be fine to merge this for Matomo 4.12. But if you insist, I can also rebase and merge that to Matomo 5 instead. |
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
I've rebased the PR to |
Note: All test failures are random and unrelated to the changes here. |
… introduce Request class (#19624) * Introduce new API property to disable autosanitizing * Adds new getRequestParam method to replace getRequestVar somewhen * use new method in some places * Introduce new request class instead of Common::getRequestParam * Improve Request class and add tests * Adds changelog * clean up api proxy * code improvements * Added doc blocks * filter null byte values * update tests * update changelog
Description:
In a later major release of Matomo (maybe 6 or 7) we maybe want to finally get rid of the sanitizing system we are currently using. As discussed in #11786 in a first step this PR introduces
Request
was introduced, that allows fetching raw parameters from a requestIn addition this PR marks
Common::getRequestVar
as deprecated, as we should prefer using the request class afterwards and sanitize/escape the values when needed manually.For Matomo 5 we might then consider to change the autosanitize property for APIs to default to false.refs #11786
Review