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
66 changes: 53 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,41 @@ def initialize(collection)
attr_reader :collection

def call(buffer = +"", view_context: nil)
unless escape_csv_injection? == true || escape_csv_injection? == false
raise <<~MESSAGE
You need to define escape_csv_injection? in #{self.class.name}, returning either `true` or `false`.

CSV injection is a security vulnerability where malicious spreadsheet formulae are used to execute code or exfiltrate data when a CSV is opened in a spreadsheet program such as Microsoft Excel or Google Sheets.

For more information, see https://owasp.org/www-community/attacks/CSV_Injection

If you're sure this CSV will never be opened in a spreadsheet program, you can disable CSV injection escapes:

def escape_csv_injection? = false

This is useful when using CSVs for byte-for-byte data exchange between secure systems.

Alternatively, you can enable CSV injection escapes at the cost of data integrity:

def escape_csv_injection? = true

Note: Enabling the CSV injection escapes will prefix any values that start with `=`, `+`, `-`, `@`, `\\t`, or `\\r` with a single quote `'` to prevent them from being interpreted as formulae by spreadsheet programs.

Unfortunately, there is no one-size-fits-all solution to CSV injection. You need to decide based on your specific use case.
MESSAGE
end

@_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 +75,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

# Override and set to `false` to disable CSV injection escapes or `true` to enable.
def escape_csv_injection?
nil
end

def render(renderable)
renderable.call(view_context: @_view_context)
def helpers
@_view_context
end

def escape(value)
value = value.to_s

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

if escape_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 escape_csv_injection?
true
end

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