Make Matomo compatible with passwords containing certain special characters #20048
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sgiehl
force-pushed
the
passwordsanitize
branch
5 times, most recently
from
fb75fe2
to
4c5a3f7
Compare
sgiehl
force-pushed
the
passwordsanitize
branch
from
4c5a3f7
to
0e282e1
Compare
… passwords with special chars aren't changed
sgiehl
force-pushed
the
passwordsanitize
branch
from
0e282e1
to
0ede7de
Compare
bx80
reviewed
Dec 11, 2022
There was a problem hiding this comment.
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 |
bx80
approved these changes
Dec 13, 2022
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Matomo currently uses a system to auto sanitize parameters. This is also done for parameters like
passwordorpasswordConfirmation. 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