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.

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

Prevent CSV injection attacks #662

Merged
merged 14 commits into from
Mar 1, 2024
3 changes: 3 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ Naming/AsciiIdentifiers:

Naming/MethodName:
Enabled: false

Style/ReturnNilInPredicateMethodDefinition:
Enabled: false
joeldrapper marked this conversation as resolved.
Show resolved Hide resolved
46 changes: 33 additions & 13 deletions lib/phlex/csv.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
class Phlex::CSV
include Phlex::Callable

FORMULA_PREFIXES = ["=", "+", "-", "@", "\t", "\r"].to_h { |prefix| [prefix, true] }.freeze
SPACE_CHARACTERS = [" ", "\t", "\r"].to_h { |char| [char, true] }.freeze
joeldrapper marked this conversation as resolved.
Show resolved Hide resolved

def initialize(collection)
@collection = collection
@_headers = []
Expand All @@ -15,17 +18,21 @@ def initialize(collection)
attr_reader :collection

def call(buffer = +"", view_context: nil)
unless prevent_csv_injection? == true || prevent_csv_injection? == false
raise "You must define `prevent_csv_injection?` on #{self.class.inspect}, returning `true` or `false`. See https://owasp.org/www-community/attacks/CSV_Injection"
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By settings prevent_csv_injection? to nil by default, we can force the developer to make an informed decision upfront by raising here. The developer must choose whether to enable or disable the protection, and they have an opportunity to learn about the risk on owasp.org.


@_view_context = view_context

each_item do |record|
collection_yielder(record) do |*args, **kwargs|
yielder(record) do |*args, **kwargs|
view_template(*args, **kwargs)

if @_first && render_headers?
buffer << @_headers.map! { |value| escape(value) }.join(",") << "\n"
buffer << @_headers.join(",") << "\n"
end

buffer << @_current_row.map! { |value| escape(value) }.join(",") << "\n"
buffer << @_current_row.join(",") << "\n"
@_current_column_index = 0
joeldrapper marked this conversation as resolved.
Show resolved Hide resolved
@_current_row.clear
end
Expand All @@ -48,43 +55,56 @@ def content_type

def column(header = nil, value)
if @_first
@_headers << header
@_headers << escape(header)
elsif header != @_headers[@_current_column_index]
raise "Inconsistent header."
end

@_current_row << value
@_current_row << escape(value)
@_current_column_index += 1
end

def each_item(&block)
collection.each(&block)
end

def collection_yielder(record)
def yielder(record)
Comment on lines -64 to +91
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to the main purpose of this PR, I think I prefer yielder for this method, since it's not really yielding the collection, it's really yielding a single item in the collection.

yield(record)
end

def template(...)
nil
end

# Override and set to `false` to disable rendering headers.
def render_headers?
true
end

def helpers
@_view_context
# Override and set to `true` to strip leading and trailing whitespace from values.
def trim_whitespace?
false
end

def render(renderable)
renderable.call(view_context: @_view_context)
# Override and set to `false` to disable CSV injection escapes or `true` to enable.
def prevent_csv_injection?
nil
end

def escape(value)
value = value.to_s
def helpers
@_view_context
end

if value.include?('"') || value.include?(",") || value.include?("\n")
def escape(value)
value = trim_whitespace? ? value.to_s.strip : value.to_s
first_char = value[0]
last_char = value[-1]

if prevent_csv_injection? && FORMULA_PREFIXES[first_char]
# Prefix a single quote to prevent Excel, Google Docs, etc. from interpreting the value as a formula.
# See https://owasp.org/www-community/attacks/CSV_Injection
%("'#{value.gsub('"', '""')}")
elsif (!trim_whitespace? && (SPACE_CHARACTERS[first_char] || SPACE_CHARACTERS[last_char])) || value.include?('"') || value.include?(",") || value.include?("\n")
joeldrapper marked this conversation as resolved.
Show resolved Hide resolved
%("#{value.gsub('"', '""')}")
else
value
Expand Down
4 changes: 4 additions & 0 deletions test/phlex/csv.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
Product = Struct.new(:name, :price)

class Example < Phlex::CSV
def prevent_csv_injection?
true
end

def view_template(product)
column("name", product.name)
column("price", product.price)
Expand Down