Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Sep 1, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7aed5c
Replaced exception with the warnings and removed related failing specs
sanga1794 Feb 18, 2021
bfe0053
Added regeex for matching whitespace
sanga1794 Feb 24, 2021
412b297
Added additional specs for validating vault data with tab or new line
sanga1794 Aug 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/chef/knife/mixin/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def validate_json(json)
next unless printable?(value.to_s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !


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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
end
Expand All @@ -69,7 +69,7 @@ def validate_json(json)
# returns true if string is free of non-printable characters (escape sequences)
# this returns false for whitespace escape sequences as well, e.g. \n\t
def printable?(string)
/[^[:print:]]/.match(string)
/[^[:print:]]|[[:space:]]/.match(string)
end
end
end
Expand Down
13 changes: 8 additions & 5 deletions spec/chef/helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@
RSpec.describe ChefVault::Mixin::Helper do
include ChefVault::Mixin::Helper

let(:json) { { "username": "root" } }
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"' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


describe "#validate_json" do
it "Raises InvalidValue Exception when invalid data provided" do
expect { validate_json(buggy_json_data) }.to raise_error(ChefVault::Exceptions::InvalidValue)
end

it "Raises InvalidValue Exception when value consist of control characters" do
expect { validate_json(json_data_control_char) }.to raise_error(ChefVault::Exceptions::InvalidValue)
end

it "Not to raise error if valid data provided" do
expect { validate_json(json_data) }.to_not raise_error
end

it "not to raise error if data consist of tab/new line OR space" do
%w{abc\tabc abc\nabc}.each do |pass|
json_data_with_slash = json.merge("password": pass)
expect { validate_json(json_data_with_slash.to_s) }.to_not raise_error
end
end
end
end