Improve user input sanitization, fix bugs [SCI-102]

This commit is contained in:
Oleksii Kriuchykhin 2017-01-11 15:50:11 +01:00
parent 4fe3030ad4
commit 0546716a0b
33 changed files with 80 additions and 80 deletions

View file

@ -1,4 +1,6 @@
class CustomFieldsController < ApplicationController
include InputSanitizeHelper
before_action :load_vars, only: [:update, :destroy, :destroy_html]
before_action :load_vars_nested, only: [:create, :destroy_html]
before_action :check_create_permissions, only: :create
@ -15,7 +17,7 @@ class CustomFieldsController < ApplicationController
format.json do
render json: {
id: @custom_field.id,
name: @custom_field.name,
name: escape_input(@custom_field.name),
edit_url:
organization_custom_field_path(@organization, @custom_field),
destroy_html_url:

View file

@ -251,7 +251,7 @@ class ExperimentsController < ApplicationController
format.json do
render json: { message: t('experiments.move.error_flash',
experiment:
sanitize_input(@experiment.name)) },
escape_input(@experiment.name)) },
status: :unprocessable_entity
end
end

View file

@ -55,7 +55,7 @@ class MyModulesController < ApplicationController
partial: "description.html.erb"
}),
title: t('my_modules.description.title',
module: sanitize_input(@my_module.name))
module: escape_input(@my_module.name))
}
}
end
@ -127,7 +127,7 @@ class MyModulesController < ApplicationController
partial: "due_date.html.erb"
}),
title: t('my_modules.due_date.title',
module: sanitize_input(@my_module.name))
module: escape_input(@my_module.name))
}
}
end

View file

@ -106,7 +106,7 @@ class ProjectsController < ApplicationController
locals: { project: @project }
}),
title: t('projects.index.modal_edit_project.modal_title',
project: sanitize_input(@project.name))
project: escape_input(@project.name))
}
}
end

View file

@ -87,7 +87,7 @@ class ProtocolsController < ApplicationController
format.json do
render json: {
title: I18n.t('protocols.index.preview.title',
protocol: @protocol.name),
protocol: escape_input(@protocol.name)),
html: render_to_string(
partial: 'protocols/index/protocol_preview_modal_body.html.erb',
locals: { protocol: @protocol }
@ -106,7 +106,7 @@ class ProtocolsController < ApplicationController
format.json {
render json: {
title: I18n.t('protocols.index.linked_children.title',
protocol: sanitize_input(@protocol.name)),
protocol: escape_input(@protocol.name)),
html: render_to_string({
partial: "protocols/index/linked_children_modal_body.html.erb",
locals: { protocol: @protocol }
@ -183,7 +183,7 @@ class ProtocolsController < ApplicationController
respond_to do |format|
# sanitize user input
params[:keywords].collect! do |keyword|
sanitize_input(keyword)
escape_input(keyword)
end
if @protocol.update_keywords(params[:keywords])
format.json {
@ -538,10 +538,12 @@ class ProtocolsController < ApplicationController
end
end
p_name = (
@protocol_json["name"].present? &&
!@protocol_json["name"].empty?
) ? @protocol_json["name"] : t("protocols.index.no_protocol_name")
p_name =
if @protocol_json['name'].present? && !@protocol_json['name'].empty?
escape_input(@protocol_json['name'])
else
t('protocols.index.no_protocol_name')
end
if transaction_error
format.json {
render json: { name: p_name, status: :bad_request }, status: :bad_request
@ -703,7 +705,7 @@ class ProtocolsController < ApplicationController
format.json {
render json: {
title: I18n.t('protocols.header.edit_name_modal.title',
protocol: sanitize_input(@protocol.name)),
protocol: escape_input(@protocol.name)),
html: render_to_string({
partial: "protocols/header/edit_name_modal_body.html.erb"
})
@ -717,7 +719,7 @@ class ProtocolsController < ApplicationController
format.json {
render json: {
title: I18n.t('protocols.header.edit_keywords_modal.title',
protocol: sanitize_input(@protocol.name)),
protocol: escape_input(@protocol.name)),
html: render_to_string({
partial: "protocols/header/edit_keywords_modal_body.html.erb"
}),
@ -732,7 +734,7 @@ class ProtocolsController < ApplicationController
format.json {
render json: {
title: I18n.t('protocols.header.edit_authors_modal.title',
protocol: sanitize_input(@protocol.name)),
protocol: escape_input(@protocol.name)),
html: render_to_string({
partial: "protocols/header/edit_authors_modal_body.html.erb"
})
@ -746,7 +748,7 @@ class ProtocolsController < ApplicationController
format.json {
render json: {
title: I18n.t('protocols.header.edit_description_modal.title',
protocol: sanitize_input(@protocol.name)),
protocol: escape_input(@protocol.name)),
html: render_to_string({
partial: "protocols/header/edit_description_modal_body.html.erb"
})

View file

@ -87,8 +87,8 @@ class SamplesController < ApplicationController
id: sample.id,
flash: t(
'samples.create.success_flash',
sample: sample.name,
organization: @organization.name
sample: escape_input(sample.name),
organization: escape_input(@organization.name)
)
},
status: :ok
@ -117,7 +117,7 @@ class SamplesController < ApplicationController
def edit
json = {
sample: {
name: sanitize_input(@sample.name),
name: escape_input(@sample.name),
sample_type: @sample.sample_type.nil? ? "" : @sample.sample_type.id,
sample_group: @sample.sample_group.nil? ? "" : @sample.sample_group.id,
custom_fields: {}
@ -130,7 +130,7 @@ class SamplesController < ApplicationController
@sample.sample_custom_fields.each do |scf|
json[:sample][:custom_fields][scf.custom_field_id] = {
sample_custom_field_id: scf.id,
value: sanitize_input(scf.value)
value: escape_input(scf.value)
}
end
@ -256,8 +256,8 @@ class SamplesController < ApplicationController
id: sample.id,
flash: t(
'samples.update.success_flash',
sample: sample.name,
organization: sanitize_input(@organization.name)
sample: escape_input(sample.name),
organization: escape_input(@organization.name)
)
},
status: :ok

View file

@ -185,7 +185,7 @@ class Users::SettingsController < ApplicationController
}),
heading: I18n.t(
"users.settings.organizations.index.leave_uo_heading",
org: sanitize_input(@user_org.organization.name)
org: escape_input(@user_org.organization.name)
)
}
}
@ -202,8 +202,8 @@ class Users::SettingsController < ApplicationController
}),
heading: I18n.t(
"users.settings.organizations.edit.destroy_uo_heading",
user: sanitize_input(@user_org.user.full_name),
org: sanitize_input(@user_org.organization.name)
user: escape_input(@user_org.user.full_name),
org: escape_input(@user_org.organization.name)
)
}
}

View file

@ -71,10 +71,10 @@ class LoadFromRepositoryProtocolsDatatable < AjaxDatatablesRails::Base
records.map do |record|
{
'DT_RowId': record.id,
'1': sanitize_input(record.name),
'1': escape_input(record.name),
'2': keywords_html(record),
'3': record.nr_of_linked_children,
'4': sanitize_input(record.full_username_str),
'4': escape_input(record.full_username_str),
'5': timestamp_column_html(record),
'6': I18n.l(record.updated_at, format: :full)
}

View file

@ -52,8 +52,8 @@ class OrganizationUsersDatatable < AjaxDatatablesRails::Base
records.map do |record|
{
'DT_RowId': record.id,
'0': sanitize_input(record.user.full_name),
'1': sanitize_input(record.user.email),
'0': escape_input(record.user.full_name),
'1': escape_input(record.user.email),
'2': I18n.l(record.created_at, format: :full),
'3': record.user.active_status_str,
'4': record.role_str,

View file

@ -103,13 +103,13 @@ class ProtocolsDatatable < AjaxDatatablesRails::Base
'DT_CanRestore': can_restore_protocol(protocol),
'DT_CanExport': can_export_protocol(protocol),
'1': if protocol.in_repository_archived?
sanitize_input(record.name)
escape_input(record.name)
else
name_html(record)
end,
'2': keywords_html(record),
'3': modules_html(record),
'4': sanitize_input(record.full_username_str),
'4': escape_input(record.full_username_str),
'5': timestamp_column_html(record),
'6': I18n.l(record.updated_at, format: :full)
}
@ -179,7 +179,7 @@ class ProtocolsDatatable < AjaxDatatablesRails::Base
def name_html(record)
"<a href='#' data-action='protocol-preview'" \
"data-url='#{preview_protocol_path(record)}'>" \
"#{sanitize_input(record.name)}" \
"#{escape_input(record.name)}" \
"</a>"
end

View file

@ -106,22 +106,22 @@ class SampleDatatable < AjaxDatatablesRails::Base
sample = {
'DT_RowId': record.id,
'1': assigned_cell(record),
'2': sanitize_input(record.name),
'2': escape_input(record.name),
'3': if record.sample_type.nil?
I18n.t('samples.table.no_type')
else
sanitize_input(record.sample_type.name)
escape_input(record.sample_type.name)
end,
'4': if record.sample_group.nil?
"<span class='glyphicon glyphicon-asterisk'></span> " +
I18n.t('samples.table.no_group')
else
"<span class='glyphicon glyphicon-asterisk' "\
"style='color: #{record.sample_group.color}'></span> " +
sanitize_input(record.sample_group.name)
"style='color: #{escape_input(record.sample_group.color)}'>"\
"</span> " + escape_input(record.sample_group.name)
end,
'5': I18n.l(record.created_at, format: :full),
'6': sanitize_input(record.user.full_name),
'6': escape_input(record.user.full_name),
'sampleInfoUrl':
Rails.application.routes.url_helpers.edit_sample_path(record.id),
'sampleUpdateUrl':

View file

@ -4,14 +4,14 @@ module AssetsHelper
res = <<-eos
<span
data-status='asset-loading'
data-filename='#{sanitize_input(asset.file_file_name)}'
data-filename='#{escape_input(asset.file_file_name)}'
data-type='#{asset.is_image? ? "image" : "asset"}'
data-present-url='#{file_present_asset_path(asset, format: :json)}'
#{asset.is_image? ? "data-preview-url='" + preview_asset_path(asset) + "'" : ""}'
data-download-url='#{download_asset_path(asset)}'
>
<span class='asset-loading-spinner' id='asset-loading-spinner-#{asset.id}'></span>
#{t('general.file.uploading', fileName: sanitize_input(asset.file_file_name))}
#{t('general.file.uploading', fileName: escape_input(asset.file_file_name))}
</span>
<script type='text/javascript'>
$('#asset-loading-spinner-#{asset.id}').spin(

View file

@ -2,13 +2,13 @@ module ProtocolStatusHelper
def protocol_status_href(protocol)
parent = protocol.parent
res = ""
res << "<a href=\"#\" data-toggle=\"popover\" data-html=\"true\" "
res << "data-trigger=\"focus\" data-placement=\"bottom\" title=\""
res = ''
res << '<a href="#" data-toggle="popover" data-html="true" '
res << 'data-trigger="focus" data-placement="bottom" title="'
res << protocol_status_popover_title(parent) +
'" data-content="' + protocol_status_popover_content(parent) +
'">' + protocol_name(parent) + '</a>'
sanitize_input(res)
res.html_safe
end
private
@ -21,7 +21,7 @@ module ProtocolStatusHelper
if protocol_private_for_current_user?(protocol)
I18n.t('my_modules.protocols.protocol_status_bar.private_parent')
else
sanitize_input(protocol.name)
escape_input(protocol.name)
end
end
@ -43,7 +43,8 @@ module ProtocolStatusHelper
res << "<a href='#' data-toggle='tooltip' data-placement='right' title='" +
I18n.t('my_modules.protocols.protocol_status_bar.added_by_tooltip',
ts: I18n.l(protocol.created_at, format: :full)) + "'>" +
sanitize_input(protocol.added_by.full_name) + '</a></span>'
escape_input(protocol.added_by.full_name) + '</a></span>'
res
end
def protocol_status_popover_content(protocol)
@ -52,7 +53,7 @@ module ProtocolStatusHelper
else
res = "<p>"
if protocol.description.present?
res << sanitize_input(protocol.description)
res << protocol.description
else
res << "<em>" + I18n.t("my_modules.protocols.protocol_status_bar.no_description") + "</em>"
end
@ -68,6 +69,6 @@ module ProtocolStatusHelper
end
res << "</p>"
end
res
sanitize_input(res)
end
end

View file

@ -6,10 +6,10 @@ def render_new_element(hide)
end
def render_report_element(element, provided_locals = nil)
children_html = "".html_safe
children_html = ''
# First, recursively render element's children
if element.comments? or element.project_header?
if element.comments? || element.project_header?
# Render no children
elsif element.result?
# Special handling for result comments

View file

@ -59,6 +59,6 @@ class Checklist < ActiveRecord::Base
private
def sanitize_fields
self.name = escape_input(name)
self.name = sanitize_input(name)
end
end

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("experiments.canvas.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("experiments.canvas.head_title", project: @project.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("experiments.module_archive.head_title", experiment: @experiment.name))) %>
<% provide(:head_title, sanitize_input(t("experiments.module_archive.head_title", experiment: @experiment.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("projects.samples.head_title", project: @experiment.name))) %>
<% provide(:head_title, sanitize_input(t("projects.samples.head_title", project: @experiment.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -30,7 +30,7 @@
<a class="result-panel-collapse-link" href="#result-panel-<%= result.id %>" data-toggle="collapse" data-remote="true">
<span class="glyphicon glyphicon-collapse-down collapse-result-icon"></span>
<strong><%= result.name %></strong> |
<span><%= raw t'my_modules.results.published_on', timestamp: l(result.created_at, format: :full), user: result.user.full_name %></span>
<span><%= sanitize_input t'my_modules.results.published_on', timestamp: l(result.created_at, format: :full), user: result.user.full_name %></span>
</a>
</div>
<div class="panel-collapse collapse" id="result-panel-<%= result.id %>" role="tabpanel">

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("my_modules.activities.head_title", project: @my_module.experiment.project.name, module: @my_module.name))) %>
<% provide(:head_title, sanitize_input(t("my_modules.activities.head_title", project: @my_module.experiment.project.name, module: @my_module.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>
@ -23,4 +23,3 @@
</div>
<%= javascript_include_tag("my_modules/activities") %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("my_modules.module_archive.head_title", project: @my_module.experiment.project.name, module: @my_module.name))) %>
<% provide(:head_title, sanitize_input(t("my_modules.module_archive.head_title", project: @my_module.experiment.project.name, module: @my_module.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("my_modules.protocols.head_title", project: @project.name, module: @my_module.name))) %>
<% provide(:head_title, sanitize_input(t("my_modules.protocols.head_title", project: @project.name, module: @my_module.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("my_modules.results.head_title", project: @project.name, module: @my_module.name))) %>
<% provide(:head_title, sanitize_input(t("my_modules.results.head_title", project: @project.name, module: @my_module.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,8 +1,7 @@
<% provide(:head_title, raw(t("my_modules.samples.head_title", project: @project.name, module: @my_module.name))) %>
<% provide(:head_title, sanitize_input(t("my_modules.samples.head_title", project: @project.name, module: @my_module.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>
<div id="content">
<%= render partial: "shared/samples" %>
</div>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("projects.experiment_archive.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("projects.experiment_archive.head_title", project: @project.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,8 +1,7 @@
<% provide(:head_title, raw(t("projects.samples.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("projects.samples.head_title", project: @project.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>
<div id="content">
<%= render partial: "shared/samples" %>
</div>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("projects.show.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("projects.show.head_title", project: @project.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -75,7 +75,7 @@
<div class="panel panel-default">
<div class="panel-heading">
<strong><%= step.name %></strong> |
<span><%= raw t("protocols.steps.published_on", timestamp: l(step.created_at, format: :full), user: step.user.full_name) %></span>
<span><%= sanitize_input t("protocols.steps.published_on", timestamp: l(step.created_at, format: :full), user: step.user.full_name) %></span>
</div>
<div class="panel-collapse collapse in" id="step-panel-<%= step.id %>" role="tabpanel" aria-expanded="true">
<div class="panel-body">
@ -85,7 +85,7 @@
<em><%= t("protocols.steps.no_description") %></em>
<% else %>
<div class="ql-editor">
<%= step.description.html_safe %>
<%= sanitize_input(step.description) %>
</div>
<% end %>
</div>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("projects.reports.index.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("projects.reports.index.head_title", project: @project.name))) %>
<%= render partial: "shared/sidebar" %>
<%= render partial: "shared/secondary_navigation" %>

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("projects.reports.new.head_title", project: @project.name))) %>
<% provide(:head_title, sanitize_input(t("projects.reports.new.head_title", project: @project.name))) %>
<% provide(:body_class, "report-body") %>
<% provide(:sidebar_wrapper_class, "report-sidebar-wrapper") %>
<% provide(:container_class, "report-container") %>

View file

@ -26,7 +26,7 @@
<a class="step-panel-collapse-link" href="#step-panel-<%= step.id %>" data-toggle="collapse" data-remote="true">
<span class="glyphicon glyphicon-collapse-down collapse-step-icon"></span>
<strong><%= step.name %></strong> |
<span><%= raw t("protocols.steps.published_on", timestamp: l(step.created_at, format: :full), user: step.user.full_name) %></span>
<span><%= sanitize_input t("protocols.steps.published_on", timestamp: l(step.created_at, format: :full), user: step.user.full_name) %></span>
</a>
</div>
<div class="panel-collapse collapse" id="step-panel-<%= step.id %>" role="tabpanel">

View file

@ -1,4 +1,4 @@
<% provide(:head_title, raw(t("notifications.title"))) %>
<% provide(:head_title, sanitize_input(t("notifications.title"))) %>
<div class="notifications-container">
<div class="notifications-header">
<span><%= t('notifications.title') %></span><span class="pull-right"><%= link_to t('nav.user.settings'), preferences_path %></span>

View file

@ -207,16 +207,14 @@ class Constants
'gif', 'jpeg', 'pjpeg', 'png', 'x-png', 'svg+xml', 'bmp'
].freeze
WHITELISTED_TAGS = [
'a', 'b', 'strong', 'i', 'em', 'li', 'ul', 'ol', 'h1', 'del', 'ins',
'h2', 'h3', 'h4', 'h5', 'h6', 'br', 'sub', 'sup', 'p', 'code', 'hr',
'div', 'span', 'u', 's', 'blockquote', 'pre'
].freeze
WHITELISTED_TAGS = %w(
a b strong i em li ul ol h1 del ins h2 h3 h4 h5 h6 br sub sup p code hr div
span u s blockquote pre
).freeze
WHITELISTED_ATTRIBUTES = [
'href', 'src', 'width', 'height', 'alt', 'cite', 'datetime', 'title',
'class', 'name', 'xml:lang', 'abbr', 'style'
].freeze
WHITELISTED_ATTRIBUTES = %w(
href src width height alt cite datetime title class name xml:lang abbr style
).freeze
# Very basic regex to check for validity of emails
BASIC_EMAIL_REGEX = URI::MailTo::EMAIL_REGEXP