Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) #367

sanga1794 · 2021-02-18T12:59:01Z

Signed-off-by: sanga17 [email protected]

Description

When uploading vault item that contains \n errors with the below message.

Check that backslashes are escaped with another backslash (e.g. C:\Windows) in double-quoted strings.
Looking at that error, we changed the \n to \n and the error stops BUT the content vault item actually have \n and no new line is inserted.

Before Fix:

bundle exec knife vault update test test1 -J ../../chef-repo/root.json -c ../../chef-repo/.chef/knife.rb
ERROR: ChefVault::Exceptions::InvalidValue: Value 'my
     passwords' of key 'text1' contains non-printable characters. Check that backslashes are escaped with another backslash (e.g. C:\\Windows) in double-quoted strings.

bundle exec knife vault show test test1 -c ../../chef-repo/.chef/knife.rb
id:       test1
text1:    my\npasswords
username: root

After Fix:

bundle exec knife vault update test test1 -J ../../chef-repo/root.json -c ../../chef-repo/.chef/knife.rb
WARN: Value 'my
passwordsj' of key 'text1' contains non-printable characters. Check that backslashes are escaped with another backslash (e.g. C:\\Windows) in double-quoted strings.


bundle exec knife vault show test test1 -c ../../chef-repo/.chef/knife.rb
id:       test1
text1:    my
passwords
username: root

Related Issue

Fixed: https://github.com/chef/chef-vault/issues/366

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

lamont-granquist · 2021-02-18T21:33:18Z

lib/chef/knife/mixin/helper.rb

@@ -59,7 +59,7 @@ def validate_json(json)
            next unless printable?(value.to_s)


shouldn't printable just be changed to also match whitespace?

Hi @lamont-granquist , for the whitespace it is working fine without any issue as shwon below

json file content: { "id": "test1", "text1": "my passwords", "username": "root" } bundle exec knife vault update test test1 -J ../../chef-repo/root.json -c ../../chef-repo/.chef/knife.rb bundle exec knife vault show test test1 -c ../../chef-repo/.chef/knife.rb id: test1 text1: my passwords username: root

Based on this is there a need to add warnings for whitespace? It should be be fine here to not add warning IMO. Please suggest.

I'm talking about adding [[:space:]] to the match for printable? which also matches \n and \r and \t in addition to ascii space and various unicode spaces.

you probably want to just use String#valid_encoding? though. or use the String#encode! method to convert the string to UTF-8 but raise on any errors rather than replacing.

I'm talking about adding [[:space:]] to the match for printable? which also matches \n and \r and \t in addition to ascii space and various unicode spaces.

Okk, will add [[:space:]] Thanks @lamont-granquist !

sanga1794 · 2021-04-01T14:20:32Z

Hi @lamont-granquist, I have added the regex for whitesapce, Please have look into it. Thanks!!

TerrieB1 · 2021-05-24T08:14:53Z

Can we get an update on this case for the customer. Thank you

sanga1794 · 2021-05-25T04:59:27Z

@lamont-granquist Please have look into the latest changes, Thanks!!

moritoki-chef · 2021-06-01T04:50:58Z

@sanga1794 Hi. I often use chef-vault and am having trouble with this problem. This problem(pull requiest) seems to have stopped for a long time, is there any problem? I hope for a quick solution. :-)

vkarve-chef · 2021-07-28T12:46:16Z

lib/chef/knife/mixin/helper.rb

@@ -59,7 +59,7 @@ def validate_json(json)
            next unless printable?(value.to_s)

            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
+            ChefVault::Log.warn(msg)


i believe an Exception still needs to be raised as before, except that the allowed characters have expanded.

vkarve-chef · 2021-07-28T12:47:48Z

spec/chef/helper_spec.rb

@@ -5,18 +5,13 @@
  include ChefVault::Mixin::Helper

  let(:json_data) { '{"username": "root", "password": "abcabc"}' }
-  let(:json_data_control_char) { '{"username": "root", "password": "abc\abc"}' }
  let(:buggy_json_data) { '{"username": "root", "password": "abc\abc"' }


will be good to add tests that ensure we allow for tabspace, whitespace and newline going forward.

Signed-off-by: sanga17 <[email protected]>

sanga1794 requested review from a team as code owners February 18, 2021 12:59

sanga1794 changed the title ~~Replaced exception with the warnings and removed related failing specs~~ Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) Feb 18, 2021

lamont-granquist reviewed Feb 18, 2021

View reviewed changes

vkarve-chef reviewed Jul 28, 2021

View reviewed changes

vkarve-chef approved these changes Aug 6, 2021

View reviewed changes

sanga1794 force-pushed the sangmeshA/Fix_chef_vault_new_line_text branch from 366518f to f051821 Compare August 20, 2021 13:28

sanga1794 added 3 commits August 31, 2021 10:05

Replaced exception with the warnings and removed related failing specs

a7aed5c

Signed-off-by: sanga17 <[email protected]>

Added regeex for matching whitespace

bfe0053

Signed-off-by: sanga17 <[email protected]>

Added additional specs for validating vault data with tab or new line

412b297

Signed-off-by: sanga17 <[email protected]>

sanga1794 force-pushed the sangmeshA/Fix_chef_vault_new_line_text branch from f051821 to 412b297 Compare August 31, 2021 04:35

vkarve-chef approved these changes Aug 31, 2021

View reviewed changes

vkarve-chef merged commit 51fff69 into chef:main Sep 1, 2021

This was referenced Nov 26, 2021

Revert #347 #375

Closed

Allow Whitespace Characters as Vault Content #372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) #367

Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) #367

sanga1794 commented Feb 18, 2021 •

edited

Loading

lamont-granquist Feb 18, 2021

sanga1794 Feb 19, 2021

lamont-granquist Feb 23, 2021

lamont-granquist Feb 23, 2021

sanga1794 Feb 24, 2021

sanga1794 commented Apr 1, 2021

TerrieB1 commented May 24, 2021

sanga1794 commented May 25, 2021

moritoki-chef commented Jun 1, 2021

vkarve-chef Jul 28, 2021

vkarve-chef Jul 28, 2021

		@@ -59,7 +59,7 @@ def validate_json(json)
		next unless printable?(value.to_s)

Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) #367

Replaced exception with the warnings and removed related failing specs(used earlier for raising issue) #367

Conversation

sanga1794 commented Feb 18, 2021 • edited Loading

Description

Related Issue

Types of changes

Checklist:

lamont-granquist Feb 18, 2021

Choose a reason for hiding this comment

sanga1794 Feb 19, 2021

Choose a reason for hiding this comment

lamont-granquist Feb 23, 2021

Choose a reason for hiding this comment

lamont-granquist Feb 23, 2021

Choose a reason for hiding this comment

sanga1794 Feb 24, 2021

Choose a reason for hiding this comment

sanga1794 commented Apr 1, 2021

TerrieB1 commented May 24, 2021

sanga1794 commented May 25, 2021

moritoki-chef commented Jun 1, 2021

vkarve-chef Jul 28, 2021

Choose a reason for hiding this comment

vkarve-chef Jul 28, 2021

Choose a reason for hiding this comment

sanga1794 commented Feb 18, 2021 •

edited

Loading