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

Jump to bottom

Use more secure methods #3291

Merged
merged 2 commits into from
Mar 12, 2021
Merged

Use more secure methods #3291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66d4af4
storage_local: Use File.open instead of Kernel.open
cosmo0920 Mar 11, 2021
77d79e9
config: types: Use JSON.parse instead of JSON.load
cosmo0920 Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/fluent/config/types.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ def self.hash_value(val, opts = {}, name = nil)
return nil if val.nil?

param = if val.is_a?(String)
val.start_with?('{') ? JSON.load(val) : Hash[val.strip.split(/\s*,\s*/).map{|v| v.split(':', 2)}]
val.start_with?('{') ? JSON.parse(val) : Hash[val.strip.split(/\s*,\s*/).map{|v| v.split(':', 2)}]
Copy link
Member

Choose a reason for hiding this comment

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

Memo:
JSON.load wraps JSON.parse and it modifies some default options of JSON::Parser:
https://github.com/flori/json/blob/7b452b290502a5cca8fc6403f31275c83e0e3d48/lib/json/common.rb#L569
https://github.com/flori/json/blob/7b452b290502a5cca8fc6403f31275c83e0e3d48/lib/json/common.rb#L422
https://docs.ruby-lang.org/ja/latest/class/JSON=3a=3aParser.html
So that the behavior is also changed a bit but I believe it's no problem on this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification. Yeah, in this case, using JSON.parse instead of JSON.load does not cause issues.

else
val
end
Expand All @@ -213,7 +213,7 @@ def self.array_value(val, opts = {}, name = nil)
return nil if val.nil?

param = if val.is_a?(String)
val.start_with?('[') ? JSON.load(val) : val.strip.split(/\s*,\s*/)
val.start_with?('[') ? JSON.parse(val) : val.strip.split(/\s*,\s*/)
else
val
end
Expand Down
6 changes: 3 additions & 3 deletions lib/fluent/plugin/storage_local.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def configure(conf)
if File.exist?(@path)
raise Fluent::ConfigError, "Plugin storage path '#{@path}' is not readable/writable" unless File.readable?(@path) && File.writable?(@path)
begin
data = open(@path, 'r:utf-8') { |io| io.read }
data = File.open(@path, 'r:utf-8') { |io| io.read }
if data.empty?
log.warn "detect empty plugin storage file during startup. Ignored: #{@path}"
return
Expand Down Expand Up @@ -115,7 +115,7 @@ def load
return if @on_memory
return unless File.exist?(@path)
begin
json_string = open(@path, 'r:utf-8'){ |io| io.read }
json_string = File.open(@path, 'r:utf-8'){ |io| io.read }
json = Yajl::Parser.parse(json_string)
unless json.is_a?(Hash)
log.error "broken content for plugin storage (Hash required: ignored)", type: json.class
Expand All @@ -133,7 +133,7 @@ def save
tmp_path = @path + '.tmp'
begin
json_string = Yajl::Encoder.encode(@store, pretty: @pretty_print)
open(tmp_path, 'w:utf-8', @mode) { |io| io.write json_string; io.fsync }
File.open(tmp_path, 'w:utf-8', @mode) { |io| io.write json_string; io.fsync }
File.rename(tmp_path, @path)
rescue => e
log.error "failed to save data for plugin storage to file", path: @path, tmp: tmp_path, error: e
Expand Down