Fix regression in apply-config-from-env.py #9097
Conversation
|
@klwilson227 Please help review this PR, thanks. |
|
/pulsarbot run-failure-checks |
There was a problem hiding this comment.
Removing the change: will put thing back. But will cause the following issues which may or may not be a concern for all customers:
- setting the value with embedded quotes, may cause some additional support issues as it is not clear that the escaping of the quotes is required to have the quotes apply.
- this opens a issue in security that allows code injection by the value passed as there is no validation. What you set in the yaml is not guaranteed to be the value in the script.
\”myvalue\”;ls /; Would result in ls of / directory being executed.
my tendency is to be more secure with what you set is what ends up in the variable.
If you proceed with the rollback my vote would be to issue a warning message from the script when a space is in the value but it is not quoted. So that the error in setting without the escaped quotes becomes a self correcting problem.
But then the issue with code injection still remains.
|
@klwilson227 this PR just rollback the regression, we already encountered the regression after start the new version Pulsar with the old configuration and the problem is very hidden. Rollback just wants to keep the compatibility, we don’t want to see users who may not be able to start broker after upgrading. I agree with your worry, so could you please help create an issue for it? so that we can start to discuss under the issue to find a better way. |
|
/pulsarbot run-failure-checks |
codelipenghui
force-pushed
the
penghui/config_apply_regression
branch
from
2d1f250
to
bf842be
Compare
|
/pulsarbot run-failure-checks |
### Motivation Fix the regression that introduced in #8709 In #8709, if values contain spaces, the value will be wrapped as "value", this will introduce break changes while users already have some configs with the value that contains spaces, so this PR is reverting this change. If users want to ensure some values are processed as a group, they should use `export key=\"value\"` instead of implicitly adding `""` when encountering spaces (cherry picked from commit 4ad499d)
### Motivation Fix the regression that introduced in #8709 In #8709, if values contain spaces, the value will be wrapped as "value", this will introduce break changes while users already have some configs with the value that contains spaces, so this PR is reverting this change. If users want to ensure some values are processed as a group, they should use `export key=\"value\"` instead of implicitly adding `""` when encountering spaces (cherry picked from commit 4ad499d)
Motivation
Fix the regression that introduced in #8709
In #8709, if values contain spaces, the value will be wrapped as "value", this will introduce break changes while users already have some configs with the value that contains spaces, so this PR is reverting this change.
If users want to ensure some values are processed as a group, they should use
export key=\"value\"instead of implicitly adding""when encountering spacesDoes this pull request potentially affect one of the following parts:
If
yeswas chosen, please highlight the changesDocumentation