Skip to content
Permalink
Browse files Browse the repository at this point in the history
Adding protection against SQL injection
Prior to this commit, the SearchCriteriaForWorksParameter class was to
optimistic in the additional_attributes it received.  This was "okay" in
that these attributes were passed from within the application (and not
from user input).

However, I wanted to tighten that up further which is why I added the
changes to fetch the attributes from a map.

And even with all of this, [brakeman][1] identified this as a weak
chance for SQL injection.  So I also added a config/brakeman.yml file to
skip over this file. (alas I can't skip over a singular violation)

[1]:https://rubygems.org/gems/brakeman
  • Loading branch information
Jeremy Friesen committed Aug 12, 2021
1 parent 641b1d5 commit d1704c7
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 4 deletions.
Expand Up @@ -17,6 +17,22 @@ class SearchCriteriaForWorksParameter
self.default_q = nil
self.default_order = 'title'.freeze
self.default_additional_attributes = ["author_name", "submission_date"].freeze

# Note the parity between this and the additional attributes.
# I'm including the following map to remove a possible SQL
# injection spot.
#
# Each key has a value that is a hash with two keys:
# 1. `key`: - the `sipity_additional_attributes.key` field's value
# 2. `join_as_table_name`: - for the purposes of the join, the
# table name we'll join as.
#
# We need the `join_as_table_name` so that we can join multiple
# times to the same `sipity_additional_attributes` table.
ADDITIONAL_ATTRIBUTE_MAP = {
"author_name" => { key: 'author_name', join_as_table_name: 'author_names' },
"submission_date" => { key: 'submission_date', join_as_table_name: 'submission_dates' },
}
ORDER_BY_OPTIONS = ['title', 'title DESC', 'created_at', 'created_at DESC', 'updated_at', 'updated_at DESC'].freeze

def self.order_options_for_select
Expand Down Expand Up @@ -68,11 +84,12 @@ def apply_and_return_additional_attributes_to(scope:)
select_fields = ["#{work_table_name}.*"]

additional_attributes.each do |attribute|
table_name = attribute.to_s.pluralize
table_name = attribute.fetch(:join_as_table_name)
key = attribute.fetch(:key)
scope = scope.joins(
%(LEFT OUTER JOIN #{attr_table_name} AS #{table_name} ON #{table_name}.work_id = #{work_table_name}.id AND #{table_name}.key = "#{attribute}")
%(LEFT OUTER JOIN #{attr_table_name} AS #{table_name} ON #{table_name}.work_id = #{work_table_name}.id AND #{table_name}.key = "#{key}")
)
select_fields << "#{table_name}.value AS #{attribute}"
select_fields << "#{table_name}.value AS #{key}"
end

scope.select(select_fields.join(", "))
Expand All @@ -84,7 +101,7 @@ def apply_and_return_additional_attributes_to(scope:)
# Doing our due diligence to santize parameters
def additional_attributes=(input)
@additional_attributes = Array(input).map do |attribute_name|
Models::AdditionalAttribute.sanitize_sql_for_conditions(attribute_name.to_s)
ADDITIONAL_ATTRIBUTE_MAP.fetch(attribute_name)
end
end

Expand Down
5 changes: 5 additions & 0 deletions config/brakeman.yml
@@ -0,0 +1,5 @@
---
# We're hand building a SQL line, for which we've taken precautions to
# avoid SQL injection.
:skip_files:
- app/parameters/sipity/parameters/search_criteria_for_works_parameter.rb

0 comments on commit d1704c7

Please sign in to comment.