Fixed problem escaping string in JSON #347
Conversation
sanga1794
changed the title
|
@btm @tas50 @lamont-granquist Please review the change. |
| evaled_json = eval(json) | ||
| rescue StandardError => e | ||
| puts e.message | ||
| end |
There was a problem hiding this comment.
What does calling eval on the json do, and what would cause a failure that is not fatal?
There was a problem hiding this comment.
Hi @phiggins, as our vault json contents string with an hash format, using eval converting to the hash.
There was a problem hiding this comment.
Is there a scenario where eval can fail and evaled_json is still a valid value?
There was a problem hiding this comment.
As in vault we accept only hash format, there is less possibility of failing the eval method.
lib/chef/knife/mixin/helper.rb
Outdated
| @@ -27,6 +27,7 @@ def set_mode(mode) | |||
| def merge_values(json, file) | |||
| values = {} | |||
| values.merge!(values_from_file(file)) if file | |||
| validate_json(json) | |||
There was a problem hiding this comment.
Should this validation be moved into values_from_json, so that bad values in json files will get caught in values_from_file?
There was a problem hiding this comment.
yes, will move it, thanks
lib/chef/knife/mixin/helper.rb
Outdated
| return false if string =~ /[^[:print:]]/ | ||
| true |
There was a problem hiding this comment.
| return false if string =~ /[^[:print:]]/ | |
| true | |
| /[^[:print:]]/.match?(string) |
There was a problem hiding this comment.
Since this can be reduced to one line and it is only used in one place, I think it's appropriate to inline this code instead of having a function.
There was a problem hiding this comment.
ok will update it, thanks @phiggins
| msg = "Value '#{value}' of key '#{key}' contains non-printable characters. Check that backslashes are escaped with another backslash (e.g. C:\\\\Windows) in double-quoted strings." | ||
| raise ChefVault::Exceptions::InvalidValue, msg | ||
| end | ||
| end |
There was a problem hiding this comment.
This will fail unless all of the values of the json are strings. That is, { ids: [1,2,3] }, or { id: 2 } will cause this to fail. Will the json always be in that format, or should this code try to handle those cases?
There was a problem hiding this comment.
yes @phiggins, it will work only for string values. any way to handle other formats?
|
Some unit tests would be good to demonstrate what's being fixed in this PR. |
|
@sanga1794 you'll need to rebase this since the commit that came from the UI lacks a DCO sign off. |
sanga1794
force-pushed
the
sangmeshA/Fix_Problem_escaping_string_in_JSON
branch
from
57322d5
to
c578b72
Compare
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
Signed-off-by: sanga17 <sausekar@msystechnologies.com>
…e changes Signed-off-by: sanga17 <sausekar@msystechnologies.com>
sanga1794
force-pushed
the
sangmeshA/Fix_Problem_escaping_string_in_JSON
branch
from
c578b72
to
cd450db
Compare
|
Hello, |
|
@tas50 and @phiggins : This should not have been merged and should be reverted or redone. Valid JSON-spec strings are now rejected if they have completely JSON-valid escape sequences in them like |
Signed-off-by: sanga17 sausekar@msystechnologies.com
Description
knife vault : Problem escaping string in JSON
Related Issue
fixed: #346
Types of changes
Checklist: