-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Make Matomo compatible with passwords containing certain special characters #20048
Conversation
fb75fe2
to
4c5a3f7
Compare
4c5a3f7
to
0e282e1
Compare
… passwords with special chars aren't changed
0e282e1
to
0ede7de
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.
It looks like the special handling for the 'segment' parameter was removed, is this now covered by the @unsanitized
annotation?
We might need a paragraph for the developer docs to explain how to safely use this functionality, for example - the best practice for a method that has multiple parameters but only one needs to be unsanitized.
@bx80 The special handling for segment was only moved some lines above, as it actually didn't work in case no default value was provided. I've also added some docs around the sanitizing in matomo-org/developer-documentation#684 |
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.
This looks good to me 👍
- 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
- n/a Wording review done
- 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
Description:
Matomo currently uses a system to auto sanitize parameters. This is also done for parameters like
password
orpasswordConfirmation
. In some place those parameters are even sanitized twice, causing an unsanitize not to result in the same password as before.As the password is a parameter that will never get printed anywhere it should be totally safe to never sanitize those parameter.
This PR aims to change that everywhere. In addition I've tried to update the tests, so they are using a password containing various special chars.
As this changes the password use for our test fixture, it might be required to update some plugin tests after merging...
fixes #20021
fixes #19857
Review